On Mon, Nov 27, 2023 at 04:55:11PM +0800, Zhenzhong Duan wrote:

Implement TDX check in order to generate domain feature capability correctly in case the availability of the feature changed.

For INTEL TDX the verification is: - checking if "/sys/module/kvm_intel/parameters/tdx" contains the value 'Y': meaning TDX is enabled in the host kernel.

Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> --- src/qemu/qemu_capabilities.c | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 83119e871a..5f806c68fb 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -5098,6 +5098,24 @@ virQEMUCapsKVMSupportsSecureGuestAMD(void) }

+/* + * Check whether INTEL Trust Domain Extention (x86) is enabled + */ +static bool +virQEMUCapsKVMSupportsSecureGuestINTEL(void) +{ + g_autofree char *modValue = NULL; + + if (virFileReadValueString(&modValue, "/sys/module/kvm_intel/parameters/tdx") < 0) + return false; + + if (modValue[0] != 'Y') + return false; + + return true; +}

It is worth adding this as a check to tools/virt-host-validate-qemu.c too, but not a requirement for this patch.

+ + /* * Check whether the secure guest functionality is enabled. * See the specific architecture function for details on the verifications made. @@ -5111,7 +5129,8 @@ virQEMUCapsKVMSupportsSecureGuest(void) return virQEMUCapsKVMSupportsSecureGuestS390();

if (ARCH_IS_X86(arch)) - return virQEMUCapsKVMSupportsSecureGuestAMD(); + return virQEMUCapsKVMSupportsSecureGuestAMD() || + virQEMUCapsKVMSupportsSecureGuestINTEL();

return false; }

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

On Mon, Nov 27, 2023 at 04:55:13PM +0800, Zhenzhong Duan wrote:

Extend qemu TDX capability to domain capabilities.

Signed-off-by: Chenyi Qiang <chenyi.qiang@intel.com> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> --- docs/formatdomaincaps.rst | 1 + src/conf/domain_capabilities.c | 1 + src/conf/domain_capabilities.h | 1 + src/conf/schemas/domaincaps.rng | 9 +++++++++ src/qemu/qemu_capabilities.c | 15 +++++++++++++++ 5 files changed, 27 insertions(+)

diff --git a/docs/formatdomaincaps.rst b/docs/formatdomaincaps.rst index ef752a0f3a..3acc9a12b4 100644 --- a/docs/formatdomaincaps.rst +++ b/docs/formatdomaincaps.rst @@ -669,6 +669,7 @@ capabilities. All features occur as children of the main ``features`` element. <value>vapic</value> </enum> </hyperv> + <tdx supported='yes'/> </features> </domainCapabilities>

diff --git a/src/conf/domain_capabilities.c b/src/conf/domain_capabilities.c index f6e09dc584..0f9ddb1e48 100644 --- a/src/conf/domain_capabilities.c +++ b/src/conf/domain_capabilities.c @@ -42,6 +42,7 @@ VIR_ENUM_IMPL(virDomainCapsFeature, "backup", "async-teardown", "s390-pv", + "tdx", );

static virClass *virDomainCapsClass; diff --git a/src/conf/domain_capabilities.h b/src/conf/domain_capabilities.h index 01bcfa2e39..cc44cf2363 100644 --- a/src/conf/domain_capabilities.h +++ b/src/conf/domain_capabilities.h @@ -250,6 +250,7 @@ typedef enum { VIR_DOMAIN_CAPS_FEATURE_BACKUP, VIR_DOMAIN_CAPS_FEATURE_ASYNC_TEARDOWN, VIR_DOMAIN_CAPS_FEATURE_S390_PV, + VIR_DOMAIN_CAPS_FEATURE_TDX,

VIR_DOMAIN_CAPS_FEATURE_LAST } virDomainCapsFeature; diff --git a/src/conf/schemas/domaincaps.rng b/src/conf/schemas/domaincaps.rng index e7aa4a1066..a5522b1e67 100644 --- a/src/conf/schemas/domaincaps.rng +++ b/src/conf/schemas/domaincaps.rng @@ -308,6 +308,9 @@ <optional> <ref name="s390-pv"/> </optional> + <optional> + <ref name="tdx"/> + </optional> <optional> <ref name="sev"/> </optional> @@ -363,6 +366,12 @@ </element> </define>

+ <define name="tdx"> + <element name="tdx"> + <ref name="supported"/> + </element> + </define> + <define name="sev"> <element name="sev"> <ref name="supported"/> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 8764df5e9d..0b4988256f 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -6657,6 +6657,20 @@ virQEMUCapsFillDomainFeatureHypervCaps(virQEMUCaps *qemuCaps, }

+static void +virQEMUCapsFillDomainFeatureTDXCaps(virQEMUCaps *qemuCaps, + virDomainCaps *domCaps) +{ + if (domCaps->arch == VIR_ARCH_X86_64 && + domCaps->virttype == VIR_DOMAIN_VIRT_KVM && + virQEMUCapsGet(qemuCaps, QEMU_CAPS_TDX_GUEST) && + virQEMUCapsGetKVMSupportsSecureGuest(qemuCaps) && + virQEMUCapsGet(qemuCaps, QEMU_CAPS_MACHINE_CONFIDENTAL_GUEST_SUPPORT) &&

Checking QEMU_CAPS_MACHINE_CONFIDENTAL_GUEST_SUPPORT is overkill, as we can assume that is implied to exist by virtue of QEMU_CAPS_TDX_GUEST existing.

+ (STREQ(domCaps->machine, "q35") || STRPREFIX(domCaps->machine, "pc-q35-")))

If QEMU has limited its support for TDX to just q35, then it would be much better if QEMU patches for TDX provided a way to detect this via QMP, so we don't need to do these string comparisons.

+ domCaps->features[VIR_DOMAIN_CAPS_FEATURE_TDX] = VIR_TRISTATE_BOOL_YES; +} + + int virQEMUCapsFillDomainCaps(virQEMUCaps *qemuCaps, virArch hostarch, @@ -6716,6 +6730,7 @@ virQEMUCapsFillDomainCaps(virQEMUCaps *qemuCaps, virQEMUCapsFillDomainFeatureSGXCaps(qemuCaps, domCaps); virQEMUCapsFillDomainFeatureHypervCaps(qemuCaps, domCaps); virQEMUCapsFillDomainDeviceCryptoCaps(qemuCaps, crypto); + virQEMUCapsFillDomainFeatureTDXCaps(qemuCaps, domCaps);

return 0; } -- 2.34.1

With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

On Thu, Jan 11, 2024 at 03:43:34AM +0000, Duan, Zhenzhong wrote:

...

-----Original Message----- From: Daniel P. Berrangé <berrange@redhat.com> Subject: Re: [PATCH rfcv3 03/11] conf: expose TDX feature in domain capabilities

...

Extend qemu TDX capability to domain capabilities.

Signed-off-by: Chenyi Qiang <chenyi.qiang@intel.com> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> --- docs/formatdomaincaps.rst | 1 + src/conf/domain_capabilities.c | 1 + src/conf/domain_capabilities.h | 1 + src/conf/schemas/domaincaps.rng | 9 +++++++++ src/qemu/qemu_capabilities.c | 15 +++++++++++++++ 5 files changed, 27 insertions(+)

diff --git a/docs/formatdomaincaps.rst b/docs/formatdomaincaps.rst index ef752a0f3a..3acc9a12b4 100644 --- a/docs/formatdomaincaps.rst +++ b/docs/formatdomaincaps.rst @@ -669,6 +669,7 @@ capabilities. All features occur as children of the

On Mon, Nov 27, 2023 at 04:55:13PM +0800, Zhenzhong Duan wrote: main ``features`` element.

...

<value>vapic</value> </enum> </hyperv> + <tdx supported='yes'/> </features> </domainCapabilities>

diff --git a/src/conf/domain_capabilities.c

b/src/conf/domain_capabilities.c

...

index f6e09dc584..0f9ddb1e48 100644 --- a/src/conf/domain_capabilities.c +++ b/src/conf/domain_capabilities.c @@ -42,6 +42,7 @@ VIR_ENUM_IMPL(virDomainCapsFeature, "backup", "async-teardown", "s390-pv", + "tdx", );

static virClass *virDomainCapsClass; diff --git a/src/conf/domain_capabilities.h b/src/conf/domain_capabilities.h index 01bcfa2e39..cc44cf2363 100644 --- a/src/conf/domain_capabilities.h +++ b/src/conf/domain_capabilities.h @@ -250,6 +250,7 @@ typedef enum { VIR_DOMAIN_CAPS_FEATURE_BACKUP, VIR_DOMAIN_CAPS_FEATURE_ASYNC_TEARDOWN, VIR_DOMAIN_CAPS_FEATURE_S390_PV, + VIR_DOMAIN_CAPS_FEATURE_TDX,

VIR_DOMAIN_CAPS_FEATURE_LAST } virDomainCapsFeature; diff --git a/src/conf/schemas/domaincaps.rng b/src/conf/schemas/domaincaps.rng index e7aa4a1066..a5522b1e67 100644 --- a/src/conf/schemas/domaincaps.rng +++ b/src/conf/schemas/domaincaps.rng @@ -308,6 +308,9 @@ <optional> <ref name="s390-pv"/> </optional> + <optional> + <ref name="tdx"/> + </optional> <optional> <ref name="sev"/> </optional> @@ -363,6 +366,12 @@ </element> </define>

+ <define name="tdx"> + <element name="tdx"> + <ref name="supported"/> + </element> + </define> + <define name="sev"> <element name="sev"> <ref name="supported"/> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 8764df5e9d..0b4988256f 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -6657,6 +6657,20 @@ virQEMUCapsFillDomainFeatureHypervCaps(virQEMUCaps *qemuCaps, }

+static void +virQEMUCapsFillDomainFeatureTDXCaps(virQEMUCaps *qemuCaps, + virDomainCaps *domCaps) +{ + if (domCaps->arch == VIR_ARCH_X86_64 && + domCaps->virttype == VIR_DOMAIN_VIRT_KVM && + virQEMUCapsGet(qemuCaps, QEMU_CAPS_TDX_GUEST) && + virQEMUCapsGetKVMSupportsSecureGuest(qemuCaps) && + virQEMUCapsGet(qemuCaps, QEMU_CAPS_MACHINE_CONFIDENTAL_GUEST_SUPPORT) &&

Checking QEMU_CAPS_MACHINE_CONFIDENTAL_GUEST_SUPPORT is overkill, as we can assume that is implied to exist by virtue of QEMU_CAPS_TDX_GUEST existing.

Good catch, will remove it.

...

...

+ (STREQ(domCaps->machine, "q35") || STRPREFIX(domCaps- machine, "pc-q35-")))

If QEMU has limited its support for TDX to just q35, then it would be much better if QEMU patches for TDX provided a way to detect this via QMP, so we don't need to do these string comparisons.

IIUC, QEMU has no limitation on using i440FX with TDX. We had this check in libvirt as we thought i440FX is deprecated.

Libvirt has not deprecated i440FX, and neither has QEMU upstream. RHEL, as a downstream, though has deprecated it.

@Yamahata, Isaku, @Li, Xiaoyao, so now we have two choices: 1. remove this limitation in libvirt

Just remove this limitation in libvirt, since RHEL support limitations are not relevant to the code.

2. add QMP on QEMU side to report this limitation to libvirt

Which choice do you like?

With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

On Mon, Nov 27, 2023 at 04:55:15PM +0800, Zhenzhong Duan wrote:

QEMU will provides 'tdx-guest' object which is used to launch encrypted VMs on Intel platform using TDX feature.

Command line looks like: $QEMU ... \ -object tdx-guest,id=lsec0,debug=on,sept-ve-disable=on,mrconfigid=xxx...xxx,mrowner=xxx...xxx,mrownerconfig=xxx...xxx,quote-generation-service=localhost:1234 \ -machine q35,confidential-guest-support=lsec0

Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> --- src/qemu/qemu_command.c | 27 +++++++++++++++++++++++++++ src/qemu/qemu_validate.c | 7 +++++++ 2 files changed, 34 insertions(+)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 89905378e4..45223746f5 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -9645,6 +9645,32 @@ qemuBuildPVCommandLine(virDomainObj *vm, virCommand *cmd) }

+static int +qemuBuildTDXCommandLine(virDomainObj *vm, virCommand *cmd, + virDomainTDXDef *tdx) +{ + g_autoptr(virJSONValue) props = NULL; + qemuDomainObjPrivate *priv = vm->privateData; + + VIR_DEBUG("policy=0x%x", tdx->policy); + + if (qemuMonitorCreateObjectProps(&props, "tdx-guest", "lsec0", + "B:debug", !!(tdx->policy & 0x1), + "b:sept-ve-disable", !!(tdx->policy & 0x10000000),

Here you're expanding the policy integer to named cli args. What is defining the semantics for bits in the policy integer ? What happens if other bits are set besides 0x1 and 0x10000000 - if we are not honouring those bits, we need to report an error when validating the xml

+ "S:mrconfigid", tdx->mrconfigid, + "S:mrowner", tdx->mrowner, + "S:mrownerconfig", tdx->mrownerconfig, + "S:quote-generation-service", tdx->QGS,

This is passing through QEMU JSON directly from the XML which is certainly not allowed. As mentioned in previous patch, we need to model 'SocketAddress' QAPI explicitly in the XML schema.

+ NULL) < 0) + return -1; + + if (qemuBuildObjectCommandlineFromJSON(cmd, props, priv->qemuCaps) < 0) + return -1; + + return 0; x> +} + + static int qemuBuildSecCommandLine(virDomainObj *vm, virCommand *cmd, virDomainSecDef *sec) @@ -9660,6 +9686,7 @@ qemuBuildSecCommandLine(virDomainObj *vm, virCommand *cmd, return qemuBuildPVCommandLine(vm, cmd); break; case VIR_DOMAIN_LAUNCH_SECURITY_TDX: + return qemuBuildTDXCommandLine(vm, cmd, &sec->data.tdx); case VIR_DOMAIN_LAUNCH_SECURITY_NONE: case VIR_DOMAIN_LAUNCH_SECURITY_LAST: virReportEnumRangeError(virDomainLaunchSecurity, sec->sectype); diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c index af630796cd..5a9173e8ff 100644 --- a/src/qemu/qemu_validate.c +++ b/src/qemu/qemu_validate.c @@ -1323,6 +1323,13 @@ qemuValidateDomainDef(const virDomainDef *def, } break; case VIR_DOMAIN_LAUNCH_SECURITY_TDX: + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MACHINE_CONFIDENTAL_GUEST_SUPPORT) ||

This isn't required as it is implied by CAPS_TDX_GUEST

+ !virQEMUCapsGet(qemuCaps, QEMU_CAPS_TDX_GUEST)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("INTEL TDX launch security is not supported with this QEMU binary"));

s/INTEL/Intel/

+ return -1; + }

This is where we need to valid 'policy' bits are supported.

+ break; case VIR_DOMAIN_LAUNCH_SECURITY_NONE: case VIR_DOMAIN_LAUNCH_SECURITY_LAST: virReportEnumRangeError(virDomainLaunchSecurity, def->sec->sectype); -- 2.34.1

With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

-----Original Message----- From: Daniel P. Berrangé <berrange@redhat.com> Subject: Re: [PATCH rfcv3 06/11] qemu: force special parameters enabled for TDX guest

On Mon, Nov 27, 2023 at 04:55:16PM +0800, Zhenzhong Duan wrote:

...

TDX guest requires some special parameters to boot, They are:

"-machine pc-q35-*" "kernel_irqchip=split"

Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> --- src/qemu/qemu_validate.c | 10 ++++++++++ 1 file changed, 10 insertions(+)

diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c index 5a9173e8ff..c4f386fe99 100644 --- a/src/qemu/qemu_validate.c +++ b/src/qemu/qemu_validate.c @@ -1329,6 +1329,16 @@ qemuValidateDomainDef(const virDomainDef *def, _("INTEL TDX launch security is not supported with this QEMU binary")); return -1; } + if (!qemuDomainIsQ35(def)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Intel TDX is supported with q35 machine types only")); + return -1; + }

Ideally QMP 'MachineInfo' struct would report whether TDX is supported so we don't need to hardcode that.

As you suggested in previous mails, I'll remove Q35 check.

...

+ if (def->features[VIR_DOMAIN_FEATURE_IOAPIC] != VIR_DOMAIN_IOAPIC_QEMU) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("INTEL TDX launch security needs split kernel irqchip"));

s/INTEL/Intel/

Ideally QEMU would automatically use the correct ioapic impl when no args are given to QEMU. That would let us do

if (def->features[VIR_DOMAIN_FEATURE_IOAPIC] == VIR_DOMAIN_IOAPIC_KVM) {

thus allowing IOAPIC_NONE (ie QEMU's default) or IOAPIC_QEMU (explicitly requested config). This will make TDX guest "just work" in more scenarios.

It looks the matching QEMU doesn't do this automation for kernel-irqchip yet. @Li, Xiaoyao could you add this automation on QEMU side? Meanwhile I'll apply Daniel's suggested change on libvirt side. Thanks Zhenzhong

...

+ return -1; + } break; case VIR_DOMAIN_LAUNCH_SECURITY_NONE: case VIR_DOMAIN_LAUNCH_SECURITY_LAST: -- 2.34.1

With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

On Mon, Nov 27, 2023 at 04:55:17PM +0800, Zhenzhong Duan wrote:

From: Chenyi Qiang <chenyi.qiang@intel.com>

Add the new flag VIR_DOMAIN_REBOOT_HARD/VIR_DOMAIN_SHUTDOWN_HARD to carry out a hard reboot, which kills the QEMU process and creates a new one with the same definition.

AFAICT, the _HARD flags cause it to issue a QEMU monitor command todo an ACPI shutdown and then re-create QEMU. IOW, it seems like this is just the same as the existing _ACPI flags, and so I'm not sure why we need any new functionality at all. What is the existing ACPI shutdown & fake reboot not able to achieve ?

Hard reboot will be the highest priority to check. If succeed, other reboot policy (i.e. agent and acpi) would be skipped. Otherwise, use the traditional way.

To make the shutdown graceful, the workflow of hard reboot is similar to the existing fakeReboot procedure.

- In qemuDomainObjPrivatePtr we have a bool hardReboot flag. - The virDomainReboot method sets this flag and then triggers a normal "system_powerdown" if we specify the reboot mode "--mode hard" - The QEMU process is started with '-no-shutdown' so that the guest CPUs pause when it powers off the guest. - When we receive the "SHUTDOWN" event from QEMU monitor, if hardReboot is not set, then check fakeReboot or the last choice to invoke qemuProcessKill command to shutdown normally. - If hardReboot was set, we spawn a background thread, which issues qemuProcessStop to kill the QEMU process and cleanup the working environment. Then issue qemuProcessStart to create a new QEMU process with the same domain definition. At last, issues 'cont' to start the CPUs again.

The state transfer during the hardReboot is RUNNING->SHUTDOWN->SHUTOFF->PAUSED->RUNNING, This patch doesn't hide it as the states is not transient.

Signed-off-by: Chenyi Qiang <chenyi.qiang@intel.com> --- include/libvirt/libvirt-domain.h | 2 + src/qemu/qemu_domain.c | 18 +++++++ src/qemu/qemu_domain.h | 4 ++ src/qemu/qemu_driver.c | 76 +++++++++++++++++++++------ src/qemu/qemu_process.c | 88 +++++++++++++++++++++++++++++++- tools/virsh-domain.c | 8 ++- 6 files changed, 177 insertions(+), 19 deletions(-)

diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index a1902546bb..5cedbbd1fc 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -1520,6 +1520,7 @@ typedef enum { VIR_DOMAIN_SHUTDOWN_INITCTL = (1 << 2), /* Use initctl (Since: 1.0.1) */ VIR_DOMAIN_SHUTDOWN_SIGNAL = (1 << 3), /* Send a signal (Since: 1.0.1) */ VIR_DOMAIN_SHUTDOWN_PARAVIRT = (1 << 4), /* Use paravirt guest control (Since: 1.2.5) */ + VIR_DOMAIN_SHUTDOWN_HARD = (1 << 5), /* Powercycle control (Since: 8.6.0) */ } virDomainShutdownFlagValues;

int virDomainShutdown (virDomainPtr domain); @@ -1538,6 +1539,7 @@ typedef enum { VIR_DOMAIN_REBOOT_INITCTL = (1 << 2), /* Use initctl (Since: 1.0.1) */ VIR_DOMAIN_REBOOT_SIGNAL = (1 << 3), /* Send a signal (Since: 1.0.1) */ VIR_DOMAIN_REBOOT_PARAVIRT = (1 << 4), /* Use paravirt guest control (Since: 1.2.5) */ + VIR_DOMAIN_REBOOT_HARD = (1 << 5), /* Powercycle control (Since: 8.6.0) */ } virDomainRebootFlagValues;

int virDomainReboot (virDomainPtr domain, diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index ae19ce884b..b65d85f91b 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2614,6 +2614,9 @@ qemuDomainObjPrivateXMLFormat(virBuffer *buf, if (priv->fakeReboot) virBufferAddLit(buf, "<fakereboot/>\n");

+ if (priv->hardReboot) + virBufferAddLit(buf, "<hardreboot/>\n"); + if (priv->qemuDevices && *priv->qemuDevices) { char **tmp = priv->qemuDevices; virBufferAddLit(buf, "<devices>\n"); @@ -3305,6 +3308,8 @@ qemuDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt,

priv->fakeReboot = virXPathBoolean("boolean(./fakereboot)", ctxt) == 1;

+ priv->hardReboot = virXPathBoolean("boolean(./hardreboot)", ctxt) == 1; + if ((n = virXPathNodeSet("./devices/device", ctxt, &nodes)) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("failed to parse qemu device list")); @@ -7445,6 +7450,19 @@ qemuDomainSetFakeReboot(virDomainObj *vm, qemuDomainSaveStatus(vm); }

+void +qemuDomainSetHardReboot(virDomainObj *vm, + bool value) +{ + qemuDomainObjPrivate *priv = vm->privateData; + + if (priv->hardReboot == value) + return; + + priv->hardReboot = value; + qemuDomainSaveStatus(vm); +} + static void qemuDomainCheckRemoveOptionalDisk(virQEMUDriver *driver, virDomainObj *vm, diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 1e56e50672..fd6058c5bc 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -137,6 +137,7 @@ struct _qemuDomainObjPrivate { * -no-reboot instead. */ virTristateBool allowReboot; + bool hardReboot;

unsigned long migMaxBandwidth; char *origname; @@ -700,6 +701,9 @@ qemuDomainRemoveInactiveLocked(virQEMUDriver *driver, void qemuDomainSetFakeReboot(virDomainObj *vm, bool value);

+void qemuDomainSetHardReboot(virDomainObj *vm, + bool value); + int qemuDomainCheckDiskStartupPolicy(virQEMUDriver *driver, virDomainObj *vm, size_t diskIndex, diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d00d2a27c6..86e8efbfcb 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1788,6 +1788,7 @@ qemuDomainShutdownFlagsAgent(virDomainObj *vm, goto endjob;

qemuDomainSetFakeReboot(vm, false); + qemuDomainSetHardReboot(vm, false); agent = qemuDomainObjEnterAgent(vm); ret = qemuAgentShutdown(agent, agentFlag); qemuDomainObjExitAgent(vm, agent); @@ -1800,7 +1801,8 @@ qemuDomainShutdownFlagsAgent(virDomainObj *vm,

static int qemuDomainShutdownFlagsMonitor(virDomainObj *vm, - bool isReboot) + bool isReboot, + bool hard) { int ret = -1; qemuDomainObjPrivate *priv; @@ -1816,7 +1818,17 @@ qemuDomainShutdownFlagsMonitor(virDomainObj *vm, goto endjob; }

- qemuDomainSetFakeReboot(vm, isReboot); + if (hard) { + /* hard reboot control the reboot */ + VIR_DEBUG("Set hard reboot %d in shutdown monitor", isReboot); + qemuDomainSetHardReboot(vm, isReboot); + qemuDomainSetFakeReboot(vm, false); + } else { + /* fake reboot control the reboot */ + VIR_DEBUG("Set fake reboot %d in shutdown monitor", isReboot); + qemuDomainSetFakeReboot(vm, isReboot); + qemuDomainSetHardReboot(vm, false); + } qemuDomainObjEnterMonitor(vm); ret = qemuMonitorSystemPowerdown(priv->mon); qemuDomainObjExitMonitor(vm); @@ -1832,12 +1844,13 @@ static int qemuDomainShutdownFlags(virDomainPtr dom, unsigned int flags) virDomainObj *vm; int ret = -1; qemuDomainObjPrivate *priv; - bool useAgent = false, agentRequested, acpiRequested; + bool useAgent = false, agentRequested, acpiRequested, hardRequested; bool isReboot = false; bool agentForced;

virCheckFlags(VIR_DOMAIN_SHUTDOWN_ACPI_POWER_BTN | - VIR_DOMAIN_SHUTDOWN_GUEST_AGENT, -1); + VIR_DOMAIN_SHUTDOWN_GUEST_AGENT | + VIR_DOMAIN_SHUTDOWN_HARD, -1);

if (!(vm = qemuDomainObjFromDomain(dom))) goto cleanup; @@ -1851,14 +1864,23 @@ static int qemuDomainShutdownFlags(virDomainPtr dom, unsigned int flags) priv = vm->privateData; agentRequested = flags & VIR_DOMAIN_SHUTDOWN_GUEST_AGENT; acpiRequested = flags & VIR_DOMAIN_SHUTDOWN_ACPI_POWER_BTN; + hardRequested = flags & VIR_DOMAIN_SHUTDOWN_HARD; + + if (virDomainShutdownFlagsEnsureACL(dom->conn, vm->def, flags) < 0) + goto cleanup; + + /* Take hard reboot as the highest priority. + * if failed, consider the agent and acpi */ + if (hardRequested) + ret = qemuDomainShutdownFlagsMonitor(vm, isReboot, true); + + if (!ret) + goto cleanup;

/* Prefer agent unless we were requested to not to. */ if (agentRequested || (!flags && priv->agent)) useAgent = true;

- if (virDomainShutdownFlagsEnsureACL(dom->conn, vm->def, flags) < 0) - goto cleanup; - agentForced = agentRequested && !acpiRequested; if (useAgent) { ret = qemuDomainShutdownFlagsAgent(vm, isReboot, agentForced); @@ -1877,7 +1899,7 @@ static int qemuDomainShutdownFlags(virDomainPtr dom, unsigned int flags) goto cleanup; }

- ret = qemuDomainShutdownFlagsMonitor(vm, isReboot); + ret = qemuDomainShutdownFlagsMonitor(vm, isReboot, false); }

cleanup: @@ -1913,6 +1935,7 @@ qemuDomainRebootAgent(virDomainObj *vm, goto endjob;

qemuDomainSetFakeReboot(vm, false); + qemuDomainSetHardReboot(vm, false); agent = qemuDomainObjEnterAgent(vm); ret = qemuAgentShutdown(agent, agentFlag); qemuDomainObjExitAgent(vm, agent); @@ -1925,7 +1948,8 @@ qemuDomainRebootAgent(virDomainObj *vm,

static int qemuDomainRebootMonitor(virDomainObj *vm, - bool isReboot) + bool isReboot, + bool hard) { qemuDomainObjPrivate *priv = vm->privateData; int ret = -1; @@ -1936,7 +1960,15 @@ qemuDomainRebootMonitor(virDomainObj *vm, if (virDomainObjCheckActive(vm) < 0) goto endjob;

- qemuDomainSetFakeReboot(vm, isReboot); + if (hard) { + VIR_DEBUG("Set hard reboot %d in reboot monitor", isReboot); + qemuDomainSetHardReboot(vm, isReboot); + qemuDomainSetFakeReboot(vm, false); + } else { + VIR_DEBUG("Set fake reboot %d in reboot monitor", isReboot); + qemuDomainSetFakeReboot(vm, isReboot); + qemuDomainSetHardReboot(vm, false); + } qemuDomainObjEnterMonitor(vm); ret = qemuMonitorSystemPowerdown(priv->mon); qemuDomainObjExitMonitor(vm); @@ -1953,12 +1985,13 @@ qemuDomainReboot(virDomainPtr dom, unsigned int flags) virDomainObj *vm; int ret = -1; qemuDomainObjPrivate *priv; - bool useAgent = false, agentRequested, acpiRequested; + bool useAgent = false, agentRequested, acpiRequested, hardRequested; bool isReboot = true; bool agentForced;

virCheckFlags(VIR_DOMAIN_REBOOT_ACPI_POWER_BTN | - VIR_DOMAIN_REBOOT_GUEST_AGENT, -1); + VIR_DOMAIN_REBOOT_GUEST_AGENT | + VIR_DOMAIN_REBOOT_HARD, -1);

if (!(vm = qemuDomainObjFromDomain(dom))) goto cleanup; @@ -1972,14 +2005,24 @@ qemuDomainReboot(virDomainPtr dom, unsigned int flags) priv = vm->privateData; agentRequested = flags & VIR_DOMAIN_REBOOT_GUEST_AGENT; acpiRequested = flags & VIR_DOMAIN_REBOOT_ACPI_POWER_BTN; + hardRequested = flags & VIR_DOMAIN_REBOOT_HARD; + + if (virDomainRebootEnsureACL(dom->conn, vm->def, flags) < 0) + goto cleanup; + + /* Take hard reboot as the highest priority. + * This is for TDX which is not allowed to warm reboot. + */ + if (hardRequested) + ret = qemuDomainRebootMonitor(vm, isReboot, true); + + if (!ret) + goto cleanup;

/* Prefer agent unless we were requested to not to. */ if (agentRequested || (!flags && priv->agent)) useAgent = true;

- if (virDomainRebootEnsureACL(dom->conn, vm->def, flags) < 0) - goto cleanup; - agentForced = agentRequested && !acpiRequested; if (useAgent) ret = qemuDomainRebootAgent(vm, isReboot, agentForced); @@ -1992,7 +2035,7 @@ qemuDomainReboot(virDomainPtr dom, unsigned int flags) */ if ((!useAgent) || (ret < 0 && (acpiRequested || !flags))) { - ret = qemuDomainRebootMonitor(vm, isReboot); + ret = qemuDomainRebootMonitor(vm, isReboot, false); }

cleanup: @@ -2095,6 +2138,7 @@ qemuDomainDestroyFlags(virDomainPtr dom, }

qemuDomainSetFakeReboot(vm, false); + qemuDomainSetHardReboot(vm, false);

if (vm->job->asyncJob == VIR_ASYNC_JOB_MIGRATION_IN) stopFlags |= VIR_QEMU_PROCESS_STOP_MIGRATED; diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index f27f6653f5..9fa679e408 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -502,13 +502,98 @@ qemuProcessFakeReboot(void *opaque) virDomainObjEndAPI(&vm); }

+static void +qemuProcessHardReboot(void *opaque) +{ + virDomainObj *vm = opaque; + qemuDomainObjPrivate *priv = vm->privateData; + virQEMUDriver *driver = priv->driver; + unsigned int stopFlags = 0; + virObjectEvent *event = NULL; + virObjectEvent * event2 = NULL; + + VIR_DEBUG("Handle hard reboot: destroy phase"); + + virObjectLock(vm); + if (virDomainObjCheckActive(vm) < 0) + goto cleanup; + + if (qemuProcessBeginStopJob(vm, VIR_JOB_DESTROY, 0) < 0) + goto cleanup; + + if (!virDomainObjIsActive(vm)) { + virReportError(VIR_ERR_OPERATION_INVALID, + "%s", _("domain is not running")); + virDomainObjEndJob(vm); + goto cleanup; + } + + if (vm->job->asyncJob == VIR_ASYNC_JOB_MIGRATION_IN) + stopFlags |= VIR_QEMU_PROCESS_STOP_MIGRATED; + + qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_DESTROYED, + VIR_ASYNC_JOB_NONE, stopFlags); + virDomainAuditStop(vm, "destroyed"); + + event = virDomainEventLifecycleNewFromObj(vm, + VIR_DOMAIN_EVENT_STOPPED, + VIR_DOMAIN_EVENT_STOPPED_DESTROYED); + + virObjectEventStateQueue(driver->domainEventState, event); + /* skip remove inactive domain from active list */ + virDomainObjEndJob(vm); + + VIR_DEBUG("Handle hard reboot: boot phase"); + + if (qemuProcessBeginJob(vm, VIR_DOMAIN_JOB_OPERATION_START, + 0) < 0) { + qemuDomainRemoveInactive(driver, vm, 0, false); + goto cleanup; + } + + if (qemuProcessStart(NULL, driver, vm, NULL, VIR_ASYNC_JOB_START, + NULL, -1, NULL, NULL, + VIR_NETDEV_VPORT_PROFILE_OP_CREATE, + 0) < 0) { + virDomainAuditStart(vm, "booted", false); + qemuDomainRemoveInactive(driver, vm, 0, false); + qemuProcessEndJob(vm); + goto cleanup; + } + + virDomainAuditStart(vm, "booted", true); + event2 = virDomainEventLifecycleNewFromObj(vm, + VIR_DOMAIN_EVENT_STARTED, + VIR_DOMAIN_EVENT_STARTED_BOOTED); + virObjectEventStateQueue(driver->domainEventState, event2); + + qemuProcessEndJob(vm); + + cleanup: + virDomainObjEndAPI(&vm); +}

void qemuProcessShutdownOrReboot(virDomainObj *vm) { qemuDomainObjPrivate *priv = vm->privateData;

- if (priv->fakeReboot || + if (priv->hardReboot) { + g_autofree char *name = g_strdup_printf("hard reboot-%s", vm->def->name); + virThread th; + + virObjectRef(vm); + if (virThreadCreateFull(&th, + false, + qemuProcessHardReboot, + name, + false, + vm) < 0) { + VIR_ERROR(_("Failed to create hard reboot thread, killing domain")); + ignore_value(qemuProcessKill(vm, VIR_QEMU_PROCESS_KILL_NOWAIT)); + virObjectUnref(vm); + } + } else if (priv->fakeReboot || vm->def->onPoweroff == VIR_DOMAIN_LIFECYCLE_ACTION_RESTART) { g_autofree char *name = g_strdup_printf("reboot-%s", vm->def->name); virThread th; @@ -5673,6 +5758,7 @@ qemuProcessInit(virQEMUDriver *driver, } else { vm->def->id = qemuDriverAllocateID(driver); qemuDomainSetFakeReboot(vm, false); + qemuDomainSetFakeReboot(vm, false); virDomainObjSetState(vm, VIR_DOMAIN_PAUSED, VIR_DOMAIN_PAUSED_STARTING_UP);

if (g_atomic_int_add(&driver->nactive, 1) == 0 && driver->inhibitCallback) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 66f933dead..21864c92cb 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -5915,7 +5915,7 @@ static const vshCmdOptDef opts_shutdown[] = { {.name = "mode", .type = VSH_OT_STRING, .completer = virshDomainShutdownModeCompleter, - .help = N_("shutdown mode: acpi|agent|initctl|signal|paravirt") + .help = N_("shutdown mode: acpi|agent|initctl|signal|paravirt|hard") }, {.name = NULL} }; @@ -5952,6 +5952,8 @@ cmdShutdown(vshControl *ctl, const vshCmd *cmd) flags |= VIR_DOMAIN_SHUTDOWN_SIGNAL; } else if (STREQ(mode, "paravirt")) { flags |= VIR_DOMAIN_SHUTDOWN_PARAVIRT; + } else if (STREQ(mode, "hard")) { + flags |= VIR_DOMAIN_SHUTDOWN_HARD; } else { vshError(ctl, _("Unknown mode %1$s value, expecting 'acpi', 'agent', 'initctl', 'signal', or 'paravirt'"), mode); @@ -5995,7 +5997,7 @@ static const vshCmdOptDef opts_reboot[] = { {.name = "mode", .type = VSH_OT_STRING, .completer = virshDomainShutdownModeCompleter, - .help = N_("shutdown mode: acpi|agent|initctl|signal|paravirt") + .help = N_("shutdown mode: acpi|agent|initctl|signal|paravirt|hard") }, {.name = NULL} }; @@ -6031,6 +6033,8 @@ cmdReboot(vshControl *ctl, const vshCmd *cmd) flags |= VIR_DOMAIN_REBOOT_SIGNAL; } else if (STREQ(mode, "paravirt")) { flags |= VIR_DOMAIN_REBOOT_PARAVIRT; + } else if (STREQ(mode, "hard")) { + flags |= VIR_DOMAIN_REBOOT_HARD; } else { vshError(ctl, _("Unknown mode %1$s value, expecting 'acpi', 'agent', 'initctl', 'signal' or 'paravirt'"), mode); -- 2.34.1

With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

-----Original Message----- From: Daniel P. Berrangé <berrange@redhat.com> Subject: Re: [PATCH rfcv3 08/11] qemu: make hard reboot as the TDX default reboot mode

On Mon, Nov 27, 2023 at 04:55:18PM +0800, Zhenzhong Duan wrote:

...

From: Chenyi Qiang <chenyi.qiang@intel.com>

Signed-off-by: Chenyi Qiang <chenyi.qiang@intel.com> --- src/qemu/qemu_driver.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 86e8efbfcb..ba1bb4ecb1 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2013,7 +2013,9 @@ qemuDomainReboot(virDomainPtr dom, unsigned int flags) /* Take hard reboot as the highest priority. * This is for TDX which is not allowed to warm reboot. */ - if (hardRequested) + if ((vm->def->sec && + vm->def->sec->sectype == VIR_DOMAIN_LAUNCH_SECURITY_TDX) || + hardRequested) ret = qemuDomainRebootMonitor(vm, isReboot, true);

if (!ret) @@ -3617,7 +3619,12 @@ processGuestPanicEvent(virQEMUDriver *driver, G_GNUC_FALLTHROUGH;

case VIR_DOMAIN_LIFECYCLE_ACTION_RESTART: - qemuDomainSetFakeReboot(vm, true); + if (vm->def->sec && + vm->def->sec->sectype == VIR_DOMAIN_LAUNCH_SECURITY_TDX) { + qemuDomainSetHardReboot(vm, true); + } else { + qemuDomainSetFakeReboot(vm, true); + } qemuProcessShutdownOrReboot(vm);

I suspect we'll need todo this with SEV too, since IIUC it does not permit soft reboot either.

Yes, unluckily I have no SEV env to see if hard reboot also works for SEV. Thanks Zhenzhong

On Mon, Nov 27, 2023 at 04:55:21PM +0800, Zhenzhong Duan wrote:

After hard reboot, domid is increased by 1 as a new domain. Hard reboot simulate TD-guest reboot by calling qemuProcessStop and qemuProcessStart which will release and recreate domain resource including domid.

This is risky. We use 'domid' as a mechanism to *guarantee* certain paths are unique when re-spawning QEMU, so re-using domid for a new QEMU is liable to cause bugs. The domain ID is not guest visible, so we don't need to preserve it from that POV. I think we should just document domid is going to change for confidential guests when the reboot is faked. Most mgmt apps focus on UUID for most of their work anyway, with domain ID largely irrelevant beyond it being presented in users in a few places.

Define origin_id to save domid and restore it when recreate domain. For persistent domain also need to save domid in newDef.

Also add logic to keep same domid when libvirt restart.

Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> --- src/conf/domain_conf.c | 4 ++++ src/conf/domain_conf.h | 1 + src/qemu/qemu_process.c | 11 +++++++++-- 3 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 08e82c5380..8c33b335bd 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4159,6 +4159,7 @@ void virDomainObjAssignDef(virDomainObj *domain, else virDomainDefFree(domain->newDef); domain->newDef = g_steal_pointer(def); + domain->newDef->origin_id = domain->def->id; return; }

@@ -17874,6 +17875,9 @@ virDomainDefParseIDs(virDomainDef *def, return -1; }

+ /* Restore origin_id to support hard reboot */ + def->origin_id = def->id; + /* Extract domain name */ if (!(def->name = virXPathString("string(./name[1])", ctxt))) { virReportError(VIR_ERR_NO_NAME, NULL); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 3b01850eb4..afd54a07eb 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2957,6 +2957,7 @@ struct _virDomainVirtioOptions { struct _virDomainDef { int virtType; /* enum virDomainVirtType */ int id; + int origin_id; unsigned char uuid[VIR_UUID_BUFLEN];

unsigned char genid[VIR_UUID_BUFLEN]; diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index a9bba19852..6ec649a261 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5757,8 +5757,15 @@ qemuProcessInit(virQEMUDriver *driver, goto cleanup; } } else { - vm->def->id = qemuDriverAllocateID(driver); - qemuDomainSetFakeReboot(vm, false); + if (vm->def->origin_id > 0 && priv->hardReboot) + vm->def->id = vm->def->origin_id; + else + vm->def->id = qemuDriverAllocateID(driver); + vm->def->origin_id = vm->def->id; + if (vm->newDef) + vm->newDef->origin_id = vm->def->id; + + qemuDomainSetHardReboot(vm, false); qemuDomainSetFakeReboot(vm, false); virDomainObjSetState(vm, VIR_DOMAIN_PAUSED, VIR_DOMAIN_PAUSED_STARTING_UP);

-- 2.34.1

With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|