From 384380a634834b5e578266a7135397dcb4f77e95 Mon Sep 17 00:00:00 2001 From: Jason Andryuk Date: Wed, 17 Nov 2021 15:44:13 -0500 Subject: [PATCH 1/7] xenmgr: Set TAPDISK3_CRYPTO_KEYDIR Set TAPDISK3_CRYPTO_KEYDIR in the environment when launching xl. It will remain set through the block script and into tap-ctl where it is used. The other approach was to have the block-tap script use xec to call back into xenmgr. That could work. The VM case is easy. The stubdom case is more complicated since you'd have to map from the stubdom's domid back to the VM to get the variables. Since xenmgr has the info, just pass it down. This also takes care of the stubdom case since the same xl process is exec-ing the hotplug scripts. Help-from: Chris Rogers Signed-off-by: Jason Andryuk --- xenmgr/Vm/Actions.hs | 3 ++- xenmgr/XenMgr/Connect/Xl.hs | 7 ++++--- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/xenmgr/Vm/Actions.hs b/xenmgr/Vm/Actions.hs index 8eaead5e..b3f93413 100644 --- a/xenmgr/Vm/Actions.hs +++ b/xenmgr/Vm/Actions.hs @@ -878,7 +878,8 @@ bootVm config reboot then do if reboot then do liftIO $ Xl.signal uuid - else do liftIO $ Xl.start uuid --we start paused by default + else do tapenv <- tapEnvForVm uuid + liftIO $ Xl.start uuid tapenv --we start paused by default else do liftIO $ xsWrite (vmSuspendImageStatePath uuid) "resume" liftIO $ Xl.resumeFromFile uuid suspend_file False True return bootstrap diff --git a/xenmgr/XenMgr/Connect/Xl.hs b/xenmgr/XenMgr/Connect/Xl.hs index 6690e5bf..8afed3a7 100644 --- a/xenmgr/XenMgr/Connect/Xl.hs +++ b/xenmgr/XenMgr/Connect/Xl.hs @@ -232,8 +232,8 @@ signal uuid = do --It should be noted that by design, we start our domains paused to ensure all the --backend components are created and xenstore nodes are written before the domain --begins running. -start :: Uuid -> IO () -start uuid = +start :: Uuid -> [(String, String)] -> IO () +start uuid extraEnv = do --if domain already has a pid don't try to create another. pid <- getXlProcess uuid @@ -243,7 +243,8 @@ start uuid = case state of Shutdown -> do (_, _, Just err, handle) <- createProcess (proc "xl" ["create", configPath uuid, "-p"]){std_err = CreatePipe, - close_fds = True} + close_fds = True, + env = Just extraEnv} ec <- waitForProcess handle stderr <- hGetContents err case ec of From 7f1b5e1768e92fce01e3e08d39ec76be1fc32079 Mon Sep 17 00:00:00 2001 From: Jason Andryuk Date: Mon, 21 Mar 2022 16:26:12 -0400 Subject: [PATCH 2/7] xenmgr: Use xl.cfg on_reboot='destroy' Avoid the need for the reboot signal dance by having xl clean up a domain on shutdown. XenMgr can then restart with just xl start. Signed-off-by: Jason Andryuk --- xenmgr/Vm/Actions.hs | 7 ++----- xenmgr/Vm/Config.hs | 1 + xenmgr/XenMgr/Connect/Xl.hs | 19 +------------------ 3 files changed, 4 insertions(+), 23 deletions(-) diff --git a/xenmgr/Vm/Actions.hs b/xenmgr/Vm/Actions.hs index b3f93413..97b3d317 100644 --- a/xenmgr/Vm/Actions.hs +++ b/xenmgr/Vm/Actions.hs @@ -875,11 +875,8 @@ bootVm config reboot then return False else liftIO (doesFileExist suspend_file) if not exists - then do - if reboot - then do liftIO $ Xl.signal uuid - else do tapenv <- tapEnvForVm uuid - liftIO $ Xl.start uuid tapenv --we start paused by default + then do tapenv <- tapEnvForVm uuid + liftIO $ Xl.start uuid tapenv --we start paused by default else do liftIO $ xsWrite (vmSuspendImageStatePath uuid) "resume" liftIO $ Xl.resumeFromFile uuid suspend_file False True return bootstrap diff --git a/xenmgr/Vm/Config.hs b/xenmgr/Vm/Config.hs index bc1d9b1f..976b4bd1 100644 --- a/xenmgr/Vm/Config.hs +++ b/xenmgr/Vm/Config.hs @@ -606,6 +606,7 @@ getXlConfig cfg = , "pci_msitranslate=1" , "pci_seize=1" , "pci_power_mgmt=1" + , "on_reboot='destroy'" ] ++ nameStr ++ hdtype diff --git a/xenmgr/XenMgr/Connect/Xl.hs b/xenmgr/XenMgr/Connect/Xl.hs index 8afed3a7..06f6bf47 100644 --- a/xenmgr/XenMgr/Connect/Xl.hs +++ b/xenmgr/XenMgr/Connect/Xl.hs @@ -30,7 +30,6 @@ module XenMgr.Connect.Xl , acpiState , waitForAcpiState , waitForState - , signal --xl/toolstack queries , domainID @@ -170,8 +169,7 @@ reboot uuid = exitCode <- system ("xl reboot " ++ domid) case exitCode of ExitSuccess -> return () - _ -> do _ <- system ("xl reboot -F " ++ domid) - return () + _ -> shutdown uuid shutdown :: Uuid -> IO () shutdown uuid = @@ -213,21 +211,6 @@ getXlProcess uuid = do case ec of ExitSuccess -> return $ TT.strip str_pid _ -> return "" - - --- Sends sigusr1 to specified xl process, in order to unblock --- it from a reboot -signal :: Uuid -> IO () -signal uuid = do - pid <- getXlProcess uuid - if pid /= "" - then do - info $ "signal xl process for uuid: " ++ (show uuid) ++ " pid: " ++ pid - readProcessOrDie "kill" ["-s", "SIGUSR1", pid] "" - return () - else do - info $ "Couldn't find xl process for uuid: " ++ (show uuid) - return () --It should be noted that by design, we start our domains paused to ensure all the --backend components are created and xenstore nodes are written before the domain From 8a80d93ac080f0d610d8030fe7e9ba3a74316b5c Mon Sep 17 00:00:00 2001 From: Jason Andryuk Date: Wed, 30 Mar 2022 09:49:35 -0400 Subject: [PATCH 3/7] xenmgr\Xl: start for Shutdown or Rebooted Expand Xl.start to start the domain from state Rebooted in addition to Shutdown. This allows the restartVm path, triggered by a domain-initiated reboot, to re-start the Vm. Otherwise state Rebooted is ignored and Xl.start does nothing. This refactors the bulk of the code into a _start helper to be called from the two cases. Signed-off-by: Jason Andryuk --- xenmgr/XenMgr/Connect/Xl.hs | 25 ++++++++++++++----------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/xenmgr/XenMgr/Connect/Xl.hs b/xenmgr/XenMgr/Connect/Xl.hs index 06f6bf47..3db64b2f 100644 --- a/xenmgr/XenMgr/Connect/Xl.hs +++ b/xenmgr/XenMgr/Connect/Xl.hs @@ -224,20 +224,23 @@ start uuid extraEnv = if pid == "" then do case state of - Shutdown -> do - (_, _, Just err, handle) <- createProcess (proc "xl" ["create", configPath uuid, "-p"]){std_err = CreatePipe, - close_fds = True, - env = Just extraEnv} - ec <- waitForProcess handle - stderr <- hGetContents err - case ec of - ExitSuccess -> return () - _ -> do - updateVmDomainStateIO uuid Shutdown - throw $ XlException $ L.intercalate "
" $ L.lines stderr + Shutdown -> _start + Rebooted -> _start _ -> do return () else do throw $ XlException "Don't try to start a guest twice" + where + _start = do + (_, _, Just err, handle) <- createProcess (proc "xl" ["create", configPath uuid, "-p"]){std_err = CreatePipe, + close_fds = True, + env = Just extraEnv} + ec <- waitForProcess handle + stderr <- hGetContents err + case ec of + ExitSuccess -> return () + _ -> do + updateVmDomainStateIO uuid Shutdown + throw $ XlException $ L.intercalate "
" $ L.lines stderr --if domain has no domid, the domain is already dead. But we should make sure --the xenstore state is set to 'shutdown'. Sometimes when domains crash on startup, From 910509256a0a6686811e140ba939abd2afa3385a Mon Sep 17 00:00:00 2001 From: Jason Andryuk Date: Wed, 4 May 2022 13:13:10 -0400 Subject: [PATCH 4/7] xenmgr: Re-work rebootVM as shutdown & start Replace the use of `xl reboot` by `xl shutdown` and `xl start` to implement the vm reboot DBus command. xl shutdown needs on_reboot="destroy" in the xl.cfg file. This allows us to remove the xenmgr <-> xl signal synchronization. Xl.shutdown & RpcAgent shutdowns both wait for the VM to shutdown, except the `xl shutdown -c` hypercall version which seems to terminate right quick. The waitForState call is therefore not really implementing a timeout out. The corner case is if the VM has ACPI & no PV drivers, but is configured not to react to the power button. In that case we can't do much. If the VM then internally reboots, the xenmgr React code will reboot the VM and the waitForState will eventually timeout. writeXlConfig is removed as rebootVm -> restartVm -> _startVm startVmInternal -> bootVm calls writeXlConfig. Signed-off-by: Jason Andryuk --- xenmgr/Vm/Actions.hs | 14 +++++++------- xenmgr/XenMgr/Expose/VmObject.hs | 2 +- xenmgr/XenMgr/PowerManagement.hs | 2 +- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/xenmgr/Vm/Actions.hs b/xenmgr/Vm/Actions.hs index 97b3d317..efff2ad2 100644 --- a/xenmgr/Vm/Actions.hs +++ b/xenmgr/Vm/Actions.hs @@ -1018,15 +1018,15 @@ applyVmBackendShift bkuuid = do -- Reboot a VM -rebootVm :: Uuid -> Rpc () +rebootVm :: Uuid -> XM () rebootVm uuid = do info $ "rebooting VM " ++ show uuid - -- Write XL configuration file - writeXlConfig =<< getVmConfig uuid True - --Let xl take care of bringing down the domain and updating our state - --When xenmgr sees the 'Rebooted' state, it fires off a startVm call, - --which performs all the normal guest boot tasks, while xl brings up the domain. - liftIO $ Xl.reboot uuid + debug $ "reboot issuing shutdown to " ++ show uuid + liftRpc $ shutdownVm uuid + debug $ "reboot done issuing shutdown to " ++ show uuid + done <- liftIO $ Xl.waitForState uuid Shutdown (Just 60) + debug $ "reboot waitForState returned " ++ show done + when done $ restartVm uuid shutdownVm :: Uuid -> Rpc () shutdownVm uuid = do diff --git a/xenmgr/XenMgr/Expose/VmObject.hs b/xenmgr/XenMgr/Expose/VmObject.hs index 001efd67..8b8c3f6e 100644 --- a/xenmgr/XenMgr/Expose/VmObject.hs +++ b/xenmgr/XenMgr/Expose/VmObject.hs @@ -104,7 +104,7 @@ implementationFor xm uuid = self where , comCitrixXenclientXenmgrVmSwitch = switchVm uuid >> return () , comCitrixXenclientXenmgrVmStart = runXM xm (startVm uuid) >> return () , comCitrixXenclientXenmgrVmStartInternal = runXM xm (startVmInternal uuid False) >> return () - , comCitrixXenclientXenmgrVmReboot = rebootVm uuid + , comCitrixXenclientXenmgrVmReboot = runXM xm (rebootVm uuid) >> return () , comCitrixXenclientXenmgrVmShutdown = runvm invokeShutdownVm , comCitrixXenclientXenmgrVmDestroy = runvm invokeForceShutdownVm , comCitrixXenclientXenmgrVmSleep = sleepVm uuid diff --git a/xenmgr/XenMgr/PowerManagement.hs b/xenmgr/XenMgr/PowerManagement.hs index fee77f8c..7b13ed4e 100644 --- a/xenmgr/XenMgr/PowerManagement.hs +++ b/xenmgr/XenMgr/PowerManagement.hs @@ -283,7 +283,7 @@ resumeS3' uuid S3Pv = do void . liftIO $ Xl.resumeFromSleep uuid info $ "PM: Successfully resumed " ++ show uuid ++ " from S3" resumeS3' uuid S3Restart = do - liftRpc $ rebootVm uuid + a <- rebootVm uuid info $ "PM: Restarted " ++ show uuid ++ " after S3" resumeS3' uuid S3Snapshot = From 9b6d13fc0b14f73bd251d69dc1721218c1409d4a Mon Sep 17 00:00:00 2001 From: Jason Andryuk Date: Thu, 5 May 2022 09:43:01 -0400 Subject: [PATCH 5/7] xenmgr: Remove restartVm and reboot code There is no meaningful difference between restartVm and startVm, so remove restartVm and the unused is_reboot boolean. Signed-off-by: Jason Andryuk --- xenmgr/Vm/Actions.hs | 24 +++++++++--------------- xenmgr/Vm/React.hs | 2 +- xenmgr/XenMgr/Expose/VmObject.hs | 2 +- 3 files changed, 11 insertions(+), 17 deletions(-) diff --git a/xenmgr/Vm/Actions.hs b/xenmgr/Vm/Actions.hs index efff2ad2..631a872c 100644 --- a/xenmgr/Vm/Actions.hs +++ b/xenmgr/Vm/Actions.hs @@ -23,7 +23,6 @@ module Vm.Actions , trashUnusedServiceVms , createVm, CreateVmPms(..), defaultCreateVmPms , removeVm - , restartVm , startVm , startVmInternal , rebootVm @@ -505,16 +504,11 @@ getVhdReferences vhd = concat <$> (mapM (diskVhdReferences vhd) =<< getVms) wher references vhd disk = diskPath disk == vhd startVm :: Uuid -> XM () -startVm uuid = _startVm False uuid - -restartVm :: Uuid -> XM () -restartVm uuid = _startVm True uuid - -_startVm :: Bool -> Uuid -> XM () -_startVm is_reboot uuid = do +startVm uuid = do + info $ "Starting " ++ show uuid withPreCreationState uuid $ do ran <- liftRpc $ runEventScript HardFail uuid getVmRunInsteadofStart [uuidStr uuid] - when (not ran) $ startVmInternal uuid is_reboot + when (not ran) $ startVmInternal uuid --Add a passthrough rule to vm config add_pt_rule_bdf uuid dev = modifyVmPciPtRules uuid $ pciAddRule (form_rule_bdf (show (devAddr dev))) @@ -523,14 +517,14 @@ form_rule_bdf = rule . fromMaybe (error "error parsing rule") . pciAndSlotFromSt rule (addr,sl) = PciPtRuleBDF addr sl -- Start a VM! (maybe, because stuff can happen not) -startVmInternal :: Uuid -> Bool -> XM () -startVmInternal uuid is_reboot = do +startVmInternal :: Uuid -> XM () +startVmInternal uuid = do unlessM (dbExists $ "/vm/" ++ show uuid) $ error ("vm does not have a database entry: " ++ show uuid) info $ "starting VM " ++ show uuid liftRpc $ maybePtGpuFuncs uuid config <- prepareAndCheckConfig uuid case config of - Just c -> info ("done checks for VM " ++ show uuid) >> bootVm c is_reboot + Just c -> info ("done checks for VM " ++ show uuid) >> bootVm c Nothing-> return () where @@ -833,8 +827,8 @@ checkAndPerformSnapshotIfReq uuid disks = do _ -> return disk --other Snapshot types unimplemented for now since UI can't set them -bootVm :: VmConfig -> Bool -> XM () -bootVm config reboot +bootVm :: VmConfig -> XM () +bootVm config = do monitor <- vm_monitor <$> xmRunVm uuid vmContext @@ -1026,7 +1020,7 @@ rebootVm uuid = do debug $ "reboot done issuing shutdown to " ++ show uuid done <- liftIO $ Xl.waitForState uuid Shutdown (Just 60) debug $ "reboot waitForState returned " ++ show done - when done $ restartVm uuid + when done $ startVm uuid shutdownVm :: Uuid -> Rpc () shutdownVm uuid = do diff --git a/xenmgr/Vm/React.hs b/xenmgr/Vm/React.hs index df3467ec..093f4eea 100644 --- a/xenmgr/Vm/React.hs +++ b/xenmgr/Vm/React.hs @@ -315,7 +315,7 @@ whenShutdown xm reason = do maybeCleanupSnapshots if reason == Reboot then do - uuidRpc (backgroundRpc . runXM xm . restartVm) + uuidRpc (backgroundRpc . runXM xm . startVm) else do runXM xm (maybeKeepVmAlive uuid) return () diff --git a/xenmgr/XenMgr/Expose/VmObject.hs b/xenmgr/XenMgr/Expose/VmObject.hs index 8b8c3f6e..faa8ad63 100644 --- a/xenmgr/XenMgr/Expose/VmObject.hs +++ b/xenmgr/XenMgr/Expose/VmObject.hs @@ -103,7 +103,7 @@ implementationFor xm uuid = self where , comCitrixXenclientXenmgrVmDelete = unlessM policyQueryVmDeletion failActionSuppressedByPolicy >> removeVm uuid , comCitrixXenclientXenmgrVmSwitch = switchVm uuid >> return () , comCitrixXenclientXenmgrVmStart = runXM xm (startVm uuid) >> return () - , comCitrixXenclientXenmgrVmStartInternal = runXM xm (startVmInternal uuid False) >> return () + , comCitrixXenclientXenmgrVmStartInternal = runXM xm (startVmInternal uuid) >> return () , comCitrixXenclientXenmgrVmReboot = runXM xm (rebootVm uuid) >> return () , comCitrixXenclientXenmgrVmShutdown = runvm invokeShutdownVm , comCitrixXenclientXenmgrVmDestroy = runvm invokeForceShutdownVm From 6419e6a8d6bcada3c4e1beab1e4b3584265572f6 Mon Sep 17 00:00:00 2001 From: Jason Andryuk Date: Thu, 5 May 2022 09:44:41 -0400 Subject: [PATCH 6/7] xenmgr: Remove xl reboot code This code is no longer called, so remove it. Signed-off-by: Jason Andryuk --- xenmgr/XenMgr/Connect/Xl.hs | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/xenmgr/XenMgr/Connect/Xl.hs b/xenmgr/XenMgr/Connect/Xl.hs index 3db64b2f..b00953c2 100644 --- a/xenmgr/XenMgr/Connect/Xl.hs +++ b/xenmgr/XenMgr/Connect/Xl.hs @@ -20,7 +20,6 @@ module XenMgr.Connect.Xl , pause , destroy , resumeFromSleep - , reboot , sleep , hibernate , suspendToFile @@ -162,15 +161,6 @@ domainXsPath uuid = do --The following functions are all domain lifecycle operations, and self-explanatory -reboot :: Uuid -> IO () -reboot uuid = - do - domid <- getDomainId uuid - exitCode <- system ("xl reboot " ++ domid) - case exitCode of - ExitSuccess -> return () - _ -> shutdown uuid - shutdown :: Uuid -> IO () shutdown uuid = do From edd9f26b8337a1ef48f7a3a59ea4606d3d1c5b7b Mon Sep 17 00:00:00 2001 From: Jason Andryuk Date: Mon, 9 May 2022 12:08:55 -0400 Subject: [PATCH 7/7] xenmgr: Push ACPI power button multiple times Sometimes the shutdown command does not work. It seems like the longer the VM is up, the more likely it is to not shutdown. This is based on Windows 10 without PV drivers. The button press is seen by the VM to some extent because a DPMS off (black) screen will turn back on, but shutdown is not initiated. A second press will trigger it - that was the intent to the xl trigger power line added in commit 1cfb6aa8e63e "xenmgr: Add xl trigger power to HVM shutdown". Re-work the code so a background thread is started that will push the power buttons* 3 times each with a 1 second delay. This will hopefully let the VM recognize the button press without going on for too long. *HVMs have two power and two sleep buttons. One is Xen emulating buttons for HVMs, and the second is from QEMU's acpi-pm-features.patch and ACPI changes. xl trigger power is pushing the Xen one and the xenstore hvm-shutdown write is triggering the QEMU one. With forkIO, the `xl shutdown -F -ww` runs before the `xl trigger power` commands. xl shutdown first tries PV shutdown and then (-F) fallback to ACPI. So this would push the Xen ACPI button before the QEMU one in pushPowerButton. Signed-off-by: Jason Andryuk --- xenmgr/XenMgr/Connect/Xl.hs | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/xenmgr/XenMgr/Connect/Xl.hs b/xenmgr/XenMgr/Connect/Xl.hs index b00953c2..c90646e2 100644 --- a/xenmgr/XenMgr/Connect/Xl.hs +++ b/xenmgr/XenMgr/Connect/Xl.hs @@ -158,6 +158,22 @@ domainXsPath uuid = do "" -> return $ "/local/domain/unknown" _ -> return $ "/local/domain/" ++ domid +pushPowerButton :: Uuid -> Int -> IO () +pushPowerButton uuid count = do + domid <- getDomainId uuid + stubdomid <- getStubDomainID uuid + let xs_path = "/local/domain/" ++ stubdomid ++ "/device-model/" ++ domid + _pushPowerButton uuid domid xs_path 1 count + where + _pushPowerButton :: Uuid -> String -> String -> Int -> Int -> IO () + _pushPowerButton uuid domid xs_path i max = do + debug $ "push power button " ++ show uuid ++ " " ++ show i ++ " of " ++ show max + xsWrite (xs_path ++ "/hvm-shutdown") "poweroff" + system ("xl trigger " ++ domid ++ " power") + if i < max + then do threadDelay $ 10^6 + _pushPowerButton uuid domid xs_path ( i + 1 ) max + else return () --The following functions are all domain lifecycle operations, and self-explanatory @@ -172,8 +188,7 @@ shutdown uuid = Just g -> do exitCode <- system ("xl shutdown -w " ++ domid) case exitCode of ExitSuccess -> return () - _ -> do xsWrite (xs_path ++ "/hvm-shutdown") "poweroff" - _ <- system ("xl trigger " ++ domid ++ " power") + _ -> do forkIO $ pushPowerButton uuid 3 _ <- system ("xl shutdown -F -w " ++ domid) return () Nothing -> do system ("xl shutdown -c -w " ++ domid)