Validate TD attributes with tdx_caps that fixed-0 bits must be zero and
fixed-1 bits must be set.

Besides, sanity check the attribute bits that have not been supported by
QEMU yet. e.g., debug bit, it will be allowed in the future when debug
TD support lands in QEMU.

Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
Acked-by: Gerd Hoffmann <kraxel@redhat.com>

---
Changes in v3:
- using error_setg() for error report; (Daniel)
---
 target/i386/kvm/tdx.c | 28 ++++++++++++++++++++++++++--
 1 file changed, 26 insertions(+), 2 deletions(-)

diff --git a/target/i386/kvm/tdx.c b/target/i386/kvm/tdx.c
index 6cf81f788fe0..5a9ce2ada89d 100644
--- a/target/i386/kvm/tdx.c
+++ b/target/i386/kvm/tdx.c
@@ -20,6 +20,7 @@
 #include "kvm_i386.h"
 #include "tdx.h"
 
+#define TDX_TD_ATTRIBUTES_DEBUG             BIT_ULL(0)
 #define TDX_TD_ATTRIBUTES_SEPT_VE_DISABLE   BIT_ULL(28)
 #define TDX_TD_ATTRIBUTES_PKS               BIT_ULL(30)
 #define TDX_TD_ATTRIBUTES_PERFMON           BIT_ULL(63)
@@ -141,13 +142,33 @@ static int tdx_kvm_type(X86ConfidentialGuest *cg)
     return KVM_X86_TDX_VM;
 }
 
-static void setup_td_guest_attributes(X86CPU *x86cpu)
+static int tdx_validate_attributes(TdxGuest *tdx, Error **errp)
+{
+    if ((tdx->attributes & ~tdx_caps->supported_attrs)) {
+            error_setg(errp, "Invalid attributes 0x%lx for TDX VM "
+                       "(supported: 0x%llx)",
+                       tdx->attributes, tdx_caps->supported_attrs);
+            return -1;
+    }
+
+    if (tdx->attributes & TDX_TD_ATTRIBUTES_DEBUG) {
+        error_setg(errp, "Current QEMU doesn't support attributes.debug[bit 0] "
+                         "for TDX VM");
+        return -1;
+    }
+
+    return 0;
+}
+
+static int setup_td_guest_attributes(X86CPU *x86cpu, Error **errp)
 {
     CPUX86State *env = &x86cpu->env;
 
     tdx_guest->attributes |= (env->features[FEAT_7_0_ECX] & CPUID_7_0_ECX_PKS) ?
                              TDX_TD_ATTRIBUTES_PKS : 0;
     tdx_guest->attributes |= x86cpu->enable_pmu ? TDX_TD_ATTRIBUTES_PERFMON : 0;
+
+    return tdx_validate_attributes(tdx_guest, errp);
 }
 
 static int setup_td_xfam(X86CPU *x86cpu, Error **errp)
@@ -211,7 +232,10 @@ int tdx_pre_create_vcpu(CPUState *cpu, Error **errp)
     init_vm = g_malloc0(sizeof(struct kvm_tdx_init_vm) +
                         sizeof(struct kvm_cpuid_entry2) * KVM_MAX_CPUID_ENTRIES);
 
-    setup_td_guest_attributes(x86cpu);
+    r = setup_td_guest_attributes(x86cpu, errp);
+    if (r) {
+        return r;
+    }
 
     r = setup_td_xfam(x86cpu, errp);
     if (r) {
-- 
2.34.1

On Tue, 2024-11-05 at 01:23 -0500, Xiaoyao Li wrote:
> -static void setup_td_guest_attributes(X86CPU *x86cpu)
> +static int tdx_validate_attributes(TdxGuest *tdx, Error **errp)
> +{
> +    if ((tdx->attributes & ~tdx_caps->supported_attrs)) {
> +            error_setg(errp, "Invalid attributes 0x%lx for TDX VM "
> +                       "(supported: 0x%llx)",
> +                       tdx->attributes, tdx_caps->supported_attrs);
> +            return -1;
> +    }
> +
> +    if (tdx->attributes & TDX_TD_ATTRIBUTES_DEBUG) {

What is going on here? It doesn't look like debug attribute could be set in this
series, so this is dead code I guess. If there is some concern that attributes
that need extra qemu support could be set in QEMU somehow, it would be better to
have a mask of qemu supported attributes and reject any not in the mask.

> +        error_setg(errp, "Current QEMU doesn't support attributes.debug[bit 0] "
> +                         "for TDX VM");
> +        return -1;
> +    }
> +
> +    return 0;
> +}

On 11/6/2024 4:56 AM, Edgecombe, Rick P wrote:
> On Tue, 2024-11-05 at 01:23 -0500, Xiaoyao Li wrote:
>> -static void setup_td_guest_attributes(X86CPU *x86cpu)
>> +static int tdx_validate_attributes(TdxGuest *tdx, Error **errp)
>> +{
>> +    if ((tdx->attributes & ~tdx_caps->supported_attrs)) {
>> +            error_setg(errp, "Invalid attributes 0x%lx for TDX VM "
>> +                       "(supported: 0x%llx)",
>> +                       tdx->attributes, tdx_caps->supported_attrs);
>> +            return -1;
>> +    }
>> +
>> +    if (tdx->attributes & TDX_TD_ATTRIBUTES_DEBUG) {
> 
> What is going on here? It doesn't look like debug attribute could be set in this
> series, so this is dead code I guess. If there is some concern that attributes
> that need extra qemu support could be set in QEMU somehow, it would be better to
> have a mask of qemu supported attributes and reject any not in the mask.

Good catch and good idea!

Will maintain a mask of supported attributes in QEMU.

>> +        error_setg(errp, "Current QEMU doesn't support attributes.debug[bit 0] "
>> +                         "for TDX VM");
>> +        return -1;
>> +    }
>> +
>> +    return 0;
>> +}
> 

On Tue, Nov 05, 2024 at 01:23:21AM -0500, Xiaoyao Li wrote:
> Validate TD attributes with tdx_caps that fixed-0 bits must be zero and
> fixed-1 bits must be set.
> 
> Besides, sanity check the attribute bits that have not been supported by
> QEMU yet. e.g., debug bit, it will be allowed in the future when debug
> TD support lands in QEMU.
> 
> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
> Acked-by: Gerd Hoffmann <kraxel@redhat.com>
> 
> ---
> Changes in v3:
> - using error_setg() for error report; (Daniel)
> ---
>  target/i386/kvm/tdx.c | 28 ++++++++++++++++++++++++++--
>  1 file changed, 26 insertions(+), 2 deletions(-)
> 
> diff --git a/target/i386/kvm/tdx.c b/target/i386/kvm/tdx.c
> index 6cf81f788fe0..5a9ce2ada89d 100644
> --- a/target/i386/kvm/tdx.c
> +++ b/target/i386/kvm/tdx.c
> @@ -20,6 +20,7 @@
>  #include "kvm_i386.h"
>  #include "tdx.h"
>  
> +#define TDX_TD_ATTRIBUTES_DEBUG             BIT_ULL(0)
>  #define TDX_TD_ATTRIBUTES_SEPT_VE_DISABLE   BIT_ULL(28)
>  #define TDX_TD_ATTRIBUTES_PKS               BIT_ULL(30)
>  #define TDX_TD_ATTRIBUTES_PERFMON           BIT_ULL(63)
> @@ -141,13 +142,33 @@ static int tdx_kvm_type(X86ConfidentialGuest *cg)
>      return KVM_X86_TDX_VM;
>  }
>  
> -static void setup_td_guest_attributes(X86CPU *x86cpu)
> +static int tdx_validate_attributes(TdxGuest *tdx, Error **errp)
> +{
> +    if ((tdx->attributes & ~tdx_caps->supported_attrs)) {
> +            error_setg(errp, "Invalid attributes 0x%lx for TDX VM "
> +                       "(supported: 0x%llx)",
> +                       tdx->attributes, tdx_caps->supported_attrs);
> +            return -1;

Minor whitespace accident, with indentation too deep.

> +    }
> +
> +    if (tdx->attributes & TDX_TD_ATTRIBUTES_DEBUG) {
> +        error_setg(errp, "Current QEMU doesn't support attributes.debug[bit 0] "
> +                         "for TDX VM");
> +        return -1;
> +    }
> +
> +    return 0;
> +}

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 11/5/2024 6:36 PM, Daniel P. Berrangé wrote:
> On Tue, Nov 05, 2024 at 01:23:21AM -0500, Xiaoyao Li wrote:
>> Validate TD attributes with tdx_caps that fixed-0 bits must be zero and
>> fixed-1 bits must be set.
>>
>> Besides, sanity check the attribute bits that have not been supported by
>> QEMU yet. e.g., debug bit, it will be allowed in the future when debug
>> TD support lands in QEMU.
>>
>> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
>> Acked-by: Gerd Hoffmann <kraxel@redhat.com>
>>
>> ---
>> Changes in v3:
>> - using error_setg() for error report; (Daniel)
>> ---
>>   target/i386/kvm/tdx.c | 28 ++++++++++++++++++++++++++--
>>   1 file changed, 26 insertions(+), 2 deletions(-)
>>
>> diff --git a/target/i386/kvm/tdx.c b/target/i386/kvm/tdx.c
>> index 6cf81f788fe0..5a9ce2ada89d 100644
>> --- a/target/i386/kvm/tdx.c
>> +++ b/target/i386/kvm/tdx.c
>> @@ -20,6 +20,7 @@
>>   #include "kvm_i386.h"
>>   #include "tdx.h"
>>   
>> +#define TDX_TD_ATTRIBUTES_DEBUG             BIT_ULL(0)
>>   #define TDX_TD_ATTRIBUTES_SEPT_VE_DISABLE   BIT_ULL(28)
>>   #define TDX_TD_ATTRIBUTES_PKS               BIT_ULL(30)
>>   #define TDX_TD_ATTRIBUTES_PERFMON           BIT_ULL(63)
>> @@ -141,13 +142,33 @@ static int tdx_kvm_type(X86ConfidentialGuest *cg)
>>       return KVM_X86_TDX_VM;
>>   }
>>   
>> -static void setup_td_guest_attributes(X86CPU *x86cpu)
>> +static int tdx_validate_attributes(TdxGuest *tdx, Error **errp)
>> +{
>> +    if ((tdx->attributes & ~tdx_caps->supported_attrs)) {
>> +            error_setg(errp, "Invalid attributes 0x%lx for TDX VM "
>> +                       "(supported: 0x%llx)",
>> +                       tdx->attributes, tdx_caps->supported_attrs);
>> +            return -1;
> 
> Minor whitespace accident, with indentation too deep.

Good catch!

btw, how did you catch it? any tool like checkpatch.pl or just by your eyes?

>> +    }
>> +
>> +    if (tdx->attributes & TDX_TD_ATTRIBUTES_DEBUG) {
>> +        error_setg(errp, "Current QEMU doesn't support attributes.debug[bit 0] "
>> +                         "for TDX VM");
>> +        return -1;
>> +    }
>> +
>> +    return 0;
>> +}
> 
> With regards,
> Daniel

On Tue, Nov 05, 2024 at 07:53:57PM +0800, Xiaoyao Li wrote:
> On 11/5/2024 6:36 PM, Daniel P. Berrangé wrote:
> > On Tue, Nov 05, 2024 at 01:23:21AM -0500, Xiaoyao Li wrote:
> > > Validate TD attributes with tdx_caps that fixed-0 bits must be zero and
> > > fixed-1 bits must be set.
> > > 
> > > Besides, sanity check the attribute bits that have not been supported by
> > > QEMU yet. e.g., debug bit, it will be allowed in the future when debug
> > > TD support lands in QEMU.
> > > 
> > > Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
> > > Acked-by: Gerd Hoffmann <kraxel@redhat.com>
> > > 
> > > ---
> > > Changes in v3:
> > > - using error_setg() for error report; (Daniel)
> > > ---
> > >   target/i386/kvm/tdx.c | 28 ++++++++++++++++++++++++++--
> > >   1 file changed, 26 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/target/i386/kvm/tdx.c b/target/i386/kvm/tdx.c
> > > index 6cf81f788fe0..5a9ce2ada89d 100644
> > > --- a/target/i386/kvm/tdx.c
> > > +++ b/target/i386/kvm/tdx.c
> > > @@ -20,6 +20,7 @@
> > >   #include "kvm_i386.h"
> > >   #include "tdx.h"
> > > +#define TDX_TD_ATTRIBUTES_DEBUG             BIT_ULL(0)
> > >   #define TDX_TD_ATTRIBUTES_SEPT_VE_DISABLE   BIT_ULL(28)
> > >   #define TDX_TD_ATTRIBUTES_PKS               BIT_ULL(30)
> > >   #define TDX_TD_ATTRIBUTES_PERFMON           BIT_ULL(63)
> > > @@ -141,13 +142,33 @@ static int tdx_kvm_type(X86ConfidentialGuest *cg)
> > >       return KVM_X86_TDX_VM;
> > >   }
> > > -static void setup_td_guest_attributes(X86CPU *x86cpu)
> > > +static int tdx_validate_attributes(TdxGuest *tdx, Error **errp)
> > > +{
> > > +    if ((tdx->attributes & ~tdx_caps->supported_attrs)) {
> > > +            error_setg(errp, "Invalid attributes 0x%lx for TDX VM "
> > > +                       "(supported: 0x%llx)",
> > > +                       tdx->attributes, tdx_caps->supported_attrs);
> > > +            return -1;
> > 
> > Minor whitespace accident, with indentation too deep.
> 
> Good catch!
> 
> btw, how did you catch it? any tool like checkpatch.pl or just by your eyes?

Nah, I just notice the mis-alignment when reading the patches.

> 
> > > +    }
> > > +
> > > +    if (tdx->attributes & TDX_TD_ATTRIBUTES_DEBUG) {
> > > +        error_setg(errp, "Current QEMU doesn't support attributes.debug[bit 0] "
> > > +                         "for TDX VM");
> > > +        return -1;
> > > +    }
> > > +
> > > +    return 0;
> > > +}

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 :|