[PATCH rfcv4 06/13] qemu: Add command line and validation for TDX type

Zhenzhong Duan posted 13 patches 6 months, 2 weeks ago
There is a newer version of this series
[PATCH rfcv4 06/13] qemu: Add command line and validation for TDX type
Posted by Zhenzhong Duan 6 months, 2 weeks ago
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 '{"qom-type":"tdx-guest","id":"lsec0","debug":true,"sept-ve-disable":false,"mrconfigid":"xxx","mrowner":"xxx","mrownerconfig":"xxx"}' \
  -machine pc-q35-6.0,confidential-guest-support=lsec0

Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
 src/conf/domain_conf.h   |  5 +++++
 src/qemu/qemu_command.c  | 31 +++++++++++++++++++++++++++++++
 src/qemu/qemu_validate.c | 11 +++++++++++
 3 files changed, 47 insertions(+)

diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 7882b7a75d..bb4973fce8 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2880,6 +2880,11 @@ struct _virDomainTDXDef {
     char *mrownerconfig;
 };
 
+#define VIR_DOMAIN_TDX_POLICY_DEBUG              0x1
+#define VIR_DOMAIN_TDX_POLICY_SEPT_VE_DISABLE    0x10000000
+#define VIR_DOMAIN_TDX_POLICY_ALLOWED_MASK       (VIR_DOMAIN_TDX_POLICY_DEBUG | \
+                                                  VIR_DOMAIN_TDX_POLICY_SEPT_VE_DISABLE)
+
 struct _virDomainSecDef {
     virDomainLaunchSecurity sectype;
     union {
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index dde2d5fa01..d212d80038 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -9745,6 +9745,36 @@ qemuBuildPVCommandLine(virDomainObj *vm, virCommand *cmd)
 }
 
 
+static int
+qemuBuildTDXCommandLine(virDomainObj *vm, virCommand *cmd,
+                        virDomainTDXDef *tdx)
+{
+    g_autoptr(virJSONValue) props = NULL;
+    qemuDomainObjPrivate *priv = vm->privateData;
+    bool sept_ve_disable = tdx->policy & VIR_DOMAIN_TDX_POLICY_SEPT_VE_DISABLE;
+
+    VIR_DEBUG("policy=0x%llx", tdx->policy);
+
+    if (qemuMonitorCreateObjectProps(&props, "tdx-guest", "lsec0",
+                                     "B:debug", !!(tdx->policy & VIR_DOMAIN_TDX_POLICY_DEBUG),
+                                     "S:mrconfigid", tdx->mrconfigid,
+                                     "S:mrowner", tdx->mrowner,
+                                     "S:mrownerconfig", tdx->mrownerconfig,
+                                     NULL) < 0)
+        return -1;
+
+    /* By default, QEMU set sept-ve-disable which is required by linux kernel */
+    if (!sept_ve_disable &&
+        virJSONValueObjectAdd(&props, "b:sept-ve-disable", false, NULL) < 0)
+        return -1;
+
+    if (qemuBuildObjectCommandlineFromJSON(cmd, props, priv->qemuCaps) < 0)
+        return -1;
+
+    return 0;
+}
+
+
 static int
 qemuBuildSecCommandLine(virDomainObj *vm, virCommand *cmd,
                         virDomainSecDef *sec)
@@ -9760,6 +9790,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 ee25fd70d9..db8493be68 100644
--- a/src/qemu/qemu_validate.c
+++ b/src/qemu/qemu_validate.c
@@ -1323,6 +1323,17 @@ qemuValidateDomainDef(const virDomainDef *def,
             }
             break;
         case VIR_DOMAIN_LAUNCH_SECURITY_TDX:
+            if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_TDX_GUEST)) {
+                virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+                               _("Intel TDX launch security is not supported with this QEMU binary"));
+                return -1;
+            }
+            if (def->sec->data.tdx.policy & ~VIR_DOMAIN_TDX_POLICY_ALLOWED_MASK) {
+                virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+                               _("Only bit0(debug) and bit28(sept-ve-disable) are supported intel TDX launch security policy"));
+                return -1;
+            }
+            break;
         case VIR_DOMAIN_LAUNCH_SECURITY_NONE:
         case VIR_DOMAIN_LAUNCH_SECURITY_LAST:
             virReportEnumRangeError(virDomainLaunchSecurity, def->sec->sectype);
-- 
2.34.1
Re: [PATCH rfcv4 06/13] qemu: Add command line and validation for TDX type
Posted by Daniel P. Berrangé 6 months ago
On Fri, May 24, 2024 at 02:21:21PM +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 '{"qom-type":"tdx-guest","id":"lsec0","debug":true,"sept-ve-disable":false,"mrconfigid":"xxx","mrowner":"xxx","mrownerconfig":"xxx"}' \
>   -machine pc-q35-6.0,confidential-guest-support=lsec0
> 
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> ---
>  src/conf/domain_conf.h   |  5 +++++
>  src/qemu/qemu_command.c  | 31 +++++++++++++++++++++++++++++++
>  src/qemu/qemu_validate.c | 11 +++++++++++
>  3 files changed, 47 insertions(+)
> 
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 7882b7a75d..bb4973fce8 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -2880,6 +2880,11 @@ struct _virDomainTDXDef {
>      char *mrownerconfig;
>  };
>  
> +#define VIR_DOMAIN_TDX_POLICY_DEBUG              0x1
> +#define VIR_DOMAIN_TDX_POLICY_SEPT_VE_DISABLE    0x10000000
> +#define VIR_DOMAIN_TDX_POLICY_ALLOWED_MASK       (VIR_DOMAIN_TDX_POLICY_DEBUG | \
> +                                                  VIR_DOMAIN_TDX_POLICY_SEPT_VE_DISABLE)
> +
>  struct _virDomainSecDef {
>      virDomainLaunchSecurity sectype;
>      union {
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index dde2d5fa01..d212d80038 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -9745,6 +9745,36 @@ qemuBuildPVCommandLine(virDomainObj *vm, virCommand *cmd)
>  }
>  
>  
> +static int
> +qemuBuildTDXCommandLine(virDomainObj *vm, virCommand *cmd,
> +                        virDomainTDXDef *tdx)
> +{
> +    g_autoptr(virJSONValue) props = NULL;
> +    qemuDomainObjPrivate *priv = vm->privateData;
> +    bool sept_ve_disable = tdx->policy & VIR_DOMAIN_TDX_POLICY_SEPT_VE_DISABLE;
> +
> +    VIR_DEBUG("policy=0x%llx", tdx->policy);
> +
> +    if (qemuMonitorCreateObjectProps(&props, "tdx-guest", "lsec0",
> +                                     "B:debug", !!(tdx->policy & VIR_DOMAIN_TDX_POLICY_DEBUG),
> +                                     "S:mrconfigid", tdx->mrconfigid,
> +                                     "S:mrowner", tdx->mrowner,
> +                                     "S:mrownerconfig", tdx->mrownerconfig,
> +                                     NULL) < 0)
> +        return -1;

I like that you've just exposed the "policy" as an int field in libvirt,
but find it unpleasant that we have to unpack it to pass bits to QEMU,
whereupon QEMU re-packs it into the original int field we already had.

I think this is a mistake in the QEMU QAPI design - QEMU shoud just accept
the policy in 'int' format. I've CC'd you on a mail to qemu-devel where I
raise this point.

> +
> +    /* By default, QEMU set sept-ve-disable which is required by linux kernel */
> +    if (!sept_ve_disable &&
> +        virJSONValueObjectAdd(&props, "b:sept-ve-disable", false, NULL) < 0)
> +        return -1;
> +
> +    if (qemuBuildObjectCommandlineFromJSON(cmd, props, priv->qemuCaps) < 0)
> +        return -1;
> +
> +    return 0;
> +}
> +
> +
>  static int
>  qemuBuildSecCommandLine(virDomainObj *vm, virCommand *cmd,
>                          virDomainSecDef *sec)
> @@ -9760,6 +9790,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 ee25fd70d9..db8493be68 100644
> --- a/src/qemu/qemu_validate.c
> +++ b/src/qemu/qemu_validate.c
> @@ -1323,6 +1323,17 @@ qemuValidateDomainDef(const virDomainDef *def,
>              }
>              break;
>          case VIR_DOMAIN_LAUNCH_SECURITY_TDX:
> +            if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_TDX_GUEST)) {
> +                virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                               _("Intel TDX launch security is not supported with this QEMU binary"));
> +                return -1;
> +            }
> +            if (def->sec->data.tdx.policy & ~VIR_DOMAIN_TDX_POLICY_ALLOWED_MASK) {
> +                virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                               _("Only bit0(debug) and bit28(sept-ve-disable) are supported intel TDX launch security policy"));
> +                return -1;
> +            }
> +            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 :|
RE: [PATCH rfcv4 06/13] qemu: Add command line and validation for TDX type
Posted by Duan, Zhenzhong 6 months ago

>-----Original Message-----
>From: Daniel P. Berrangé <berrange@redhat.com>
>Subject: Re: [PATCH rfcv4 06/13] qemu: Add command line and validation
>for TDX type
>
>On Fri, May 24, 2024 at 02:21:21PM +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 '{"qom-type":"tdx-guest","id":"lsec0","debug":true,"sept-ve-
>disable":false,"mrconfigid":"xxx","mrowner":"xxx","mrownerconfig":"xxx"}' \
>>   -machine pc-q35-6.0,confidential-guest-support=lsec0
>>
>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>> ---
>>  src/conf/domain_conf.h   |  5 +++++
>>  src/qemu/qemu_command.c  | 31
>+++++++++++++++++++++++++++++++
>>  src/qemu/qemu_validate.c | 11 +++++++++++
>>  3 files changed, 47 insertions(+)
>>
>> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
>> index 7882b7a75d..bb4973fce8 100644
>> --- a/src/conf/domain_conf.h
>> +++ b/src/conf/domain_conf.h
>> @@ -2880,6 +2880,11 @@ struct _virDomainTDXDef {
>>      char *mrownerconfig;
>>  };
>>
>> +#define VIR_DOMAIN_TDX_POLICY_DEBUG              0x1
>> +#define VIR_DOMAIN_TDX_POLICY_SEPT_VE_DISABLE    0x10000000
>> +#define VIR_DOMAIN_TDX_POLICY_ALLOWED_MASK
>(VIR_DOMAIN_TDX_POLICY_DEBUG | \
>> +
>VIR_DOMAIN_TDX_POLICY_SEPT_VE_DISABLE)
>> +
>>  struct _virDomainSecDef {
>>      virDomainLaunchSecurity sectype;
>>      union {
>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>> index dde2d5fa01..d212d80038 100644
>> --- a/src/qemu/qemu_command.c
>> +++ b/src/qemu/qemu_command.c
>> @@ -9745,6 +9745,36 @@ qemuBuildPVCommandLine(virDomainObj
>*vm, virCommand *cmd)
>>  }
>>
>>
>> +static int
>> +qemuBuildTDXCommandLine(virDomainObj *vm, virCommand *cmd,
>> +                        virDomainTDXDef *tdx)
>> +{
>> +    g_autoptr(virJSONValue) props = NULL;
>> +    qemuDomainObjPrivate *priv = vm->privateData;
>> +    bool sept_ve_disable = tdx->policy &
>VIR_DOMAIN_TDX_POLICY_SEPT_VE_DISABLE;
>> +
>> +    VIR_DEBUG("policy=0x%llx", tdx->policy);
>> +
>> +    if (qemuMonitorCreateObjectProps(&props, "tdx-guest", "lsec0",
>> +                                     "B:debug", !!(tdx->policy &
>VIR_DOMAIN_TDX_POLICY_DEBUG),
>> +                                     "S:mrconfigid", tdx->mrconfigid,
>> +                                     "S:mrowner", tdx->mrowner,
>> +                                     "S:mrownerconfig", tdx->mrownerconfig,
>> +                                     NULL) < 0)
>> +        return -1;
>
>I like that you've just exposed the "policy" as an int field in libvirt,
>but find it unpleasant that we have to unpack it to pass bits to QEMU,
>whereupon QEMU re-packs it into the original int field we already had.

Yes.

>
>I think this is a mistake in the QEMU QAPI design - QEMU shoud just accept
>the policy in 'int' format. I've CC'd you on a mail to qemu-devel where I
>raise this point.

Agree with your point.

Thanks
Zhenzhong