diff mbox series

[v2,18/58] i386/tdx: Validate TD attributes

Message ID 20230818095041.1973309-19-xiaoyao.li@intel.com (mailing list archive)
State New, archived
Headers show
Series TDX QEMU support | expand

Commit Message

Xiaoyao Li Aug. 18, 2023, 9:50 a.m. UTC 
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>
---
 target/i386/kvm/tdx.c | 27 +++++++++++++++++++++++++--
 1 file changed, 25 insertions(+), 2 deletions(-)

Comments

Daniel P. Berrangé Aug. 21, 2023, 9:16 a.m. UTC | #1 
On Fri, Aug 18, 2023 at 05:50:01AM -0400, 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>
> ---
>  target/i386/kvm/tdx.c | 27 +++++++++++++++++++++++++--
>  1 file changed, 25 insertions(+), 2 deletions(-)
> 
> diff --git a/target/i386/kvm/tdx.c b/target/i386/kvm/tdx.c
> index 629abd267da8..73da15377ec3 100644
> --- a/target/i386/kvm/tdx.c
> +++ b/target/i386/kvm/tdx.c
> @@ -32,6 +32,7 @@
>                                       (1U << KVM_FEATURE_PV_SCHED_YIELD) | \
>                                       (1U << KVM_FEATURE_MSI_EXT_DEST_ID))
>  
> +#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)
> @@ -462,13 +463,32 @@ int tdx_kvm_init(MachineState *ms, Error **errp)
>      return 0;
>  }
>  
> -static void setup_td_guest_attributes(X86CPU *x86cpu)
> +static int tdx_validate_attributes(TdxGuest *tdx)
> +{
> +    if (((tdx->attributes & tdx_caps->attrs_fixed0) | tdx_caps->attrs_fixed1) !=
> +        tdx->attributes) {
> +            error_report("Invalid attributes 0x%lx for TDX VM (fixed0 0x%llx, fixed1 0x%llx)",
> +                          tdx->attributes, tdx_caps->attrs_fixed0, tdx_caps->attrs_fixed1);
> +            return -EINVAL;
> +    }
> +
> +    if (tdx->attributes & TDX_TD_ATTRIBUTES_DEBUG) {
> +        error_report("Current QEMU doesn't support attributes.debug[bit 0] for TDX VM");
> +        return -EINVAL;
> +    }

Use error_setg() in both cases, passing in a 'Error **errp' object,
and 'return -1' instead of returning an errno value.

> +
> +    return 0;
> +}
> +
> +static int setup_td_guest_attributes(X86CPU *x86cpu)
>  {
>      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);

Pass along "errp" into this

>  }
>  
>  int tdx_pre_create_vcpu(CPUState *cpu)
> @@ -493,7 +513,10 @@ int tdx_pre_create_vcpu(CPUState *cpu)

In an earlier patch I suggested adding 'Error **errp' to this method...

>          goto out_free;
>      }
>  
> -    setup_td_guest_attributes(x86cpu);
> +    r = setup_td_guest_attributes(x86cpu);

...it can also be passed into this method

> +    if (r) {
> +        goto out;
> +    }
>  
>      init_vm->cpuid.nent = kvm_x86_arch_cpuid(env, init_vm->cpuid.entries, 0);
>      init_vm->attributes = tdx_guest->attributes;
> -- 
> 2.34.1
> 

With regards,
Daniel
Xiaoyao Li Aug. 22, 2023, 2:21 p.m. UTC | #2 
On 8/21/2023 5:16 PM, Daniel P. Berrangé wrote:
> On Fri, Aug 18, 2023 at 05:50:01AM -0400, 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>
>> ---
>>   target/i386/kvm/tdx.c | 27 +++++++++++++++++++++++++--
>>   1 file changed, 25 insertions(+), 2 deletions(-)
>>
>> diff --git a/target/i386/kvm/tdx.c b/target/i386/kvm/tdx.c
>> index 629abd267da8..73da15377ec3 100644
>> --- a/target/i386/kvm/tdx.c
>> +++ b/target/i386/kvm/tdx.c
>> @@ -32,6 +32,7 @@
>>                                        (1U << KVM_FEATURE_PV_SCHED_YIELD) | \
>>                                        (1U << KVM_FEATURE_MSI_EXT_DEST_ID))
>>   
>> +#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)
>> @@ -462,13 +463,32 @@ int tdx_kvm_init(MachineState *ms, Error **errp)
>>       return 0;
>>   }
>>   
>> -static void setup_td_guest_attributes(X86CPU *x86cpu)
>> +static int tdx_validate_attributes(TdxGuest *tdx)
>> +{
>> +    if (((tdx->attributes & tdx_caps->attrs_fixed0) | tdx_caps->attrs_fixed1) !=
>> +        tdx->attributes) {
>> +            error_report("Invalid attributes 0x%lx for TDX VM (fixed0 0x%llx, fixed1 0x%llx)",
>> +                          tdx->attributes, tdx_caps->attrs_fixed0, tdx_caps->attrs_fixed1);
>> +            return -EINVAL;
>> +    }
>> +
>> +    if (tdx->attributes & TDX_TD_ATTRIBUTES_DEBUG) {
>> +        error_report("Current QEMU doesn't support attributes.debug[bit 0] for TDX VM");
>> +        return -EINVAL;
>> +    }
> 
> Use error_setg() in both cases, passing in a 'Error **errp' object,
> and 'return -1' instead of returning an errno value.

Will do it in next version.

thanks!

>> +
>> +    return 0;
>> +}
>> +
>> +static int setup_td_guest_attributes(X86CPU *x86cpu)
>>   {
>>       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);
> 
> Pass along "errp" into this
> 
>>   }
>>   
>>   int tdx_pre_create_vcpu(CPUState *cpu)
>> @@ -493,7 +513,10 @@ int tdx_pre_create_vcpu(CPUState *cpu)
> 
> In an earlier patch I suggested adding 'Error **errp' to this method...
> 
>>           goto out_free;
>>       }
>>   
>> -    setup_td_guest_attributes(x86cpu);
>> +    r = setup_td_guest_attributes(x86cpu);
> 
> ...it can also be passed into this method
> 
>> +    if (r) {
>> +        goto out;
>> +    }
>>   
>>       init_vm->cpuid.nent = kvm_x86_arch_cpuid(env, init_vm->cpuid.entries, 0);
>>       init_vm->attributes = tdx_guest->attributes;
>> -- 
>> 2.34.1
>>
> 
> With regards,
> Daniel
Xiaoyao Li Aug. 22, 2023, 2:30 p.m. UTC | #3 
On 8/21/2023 5:16 PM, Daniel P. Berrangé wrote:
> On Fri, Aug 18, 2023 at 05:50:01AM -0400, 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>
>> ---
>>   target/i386/kvm/tdx.c | 27 +++++++++++++++++++++++++--
>>   1 file changed, 25 insertions(+), 2 deletions(-)
>>
>> diff --git a/target/i386/kvm/tdx.c b/target/i386/kvm/tdx.c
>> index 629abd267da8..73da15377ec3 100644
>> --- a/target/i386/kvm/tdx.c
>> +++ b/target/i386/kvm/tdx.c
>> @@ -32,6 +32,7 @@
>>                                        (1U << KVM_FEATURE_PV_SCHED_YIELD) | \
>>                                        (1U << KVM_FEATURE_MSI_EXT_DEST_ID))
>>   
>> +#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)
>> @@ -462,13 +463,32 @@ int tdx_kvm_init(MachineState *ms, Error **errp)
>>       return 0;
>>   }
>>   
>> -static void setup_td_guest_attributes(X86CPU *x86cpu)
>> +static int tdx_validate_attributes(TdxGuest *tdx)
>> +{
>> +    if (((tdx->attributes & tdx_caps->attrs_fixed0) | tdx_caps->attrs_fixed1) !=
>> +        tdx->attributes) {
>> +            error_report("Invalid attributes 0x%lx for TDX VM (fixed0 0x%llx, fixed1 0x%llx)",
>> +                          tdx->attributes, tdx_caps->attrs_fixed0, tdx_caps->attrs_fixed1);
>> +            return -EINVAL;
>> +    }
>> +
>> +    if (tdx->attributes & TDX_TD_ATTRIBUTES_DEBUG) {
>> +        error_report("Current QEMU doesn't support attributes.debug[bit 0] for TDX VM");
>> +        return -EINVAL;
>> +    }
> 
> Use error_setg() in both cases, passing in a 'Error **errp' object,
> and 'return -1' instead of returning an errno value.
> 

why return -1 instead of -EINVAL?
Daniel P. Berrangé Aug. 22, 2023, 2:42 p.m. UTC | #4 
On Tue, Aug 22, 2023 at 10:30:47PM +0800, Xiaoyao Li wrote:
> On 8/21/2023 5:16 PM, Daniel P. Berrangé wrote:
> > On Fri, Aug 18, 2023 at 05:50:01AM -0400, 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>
> > > ---
> > >   target/i386/kvm/tdx.c | 27 +++++++++++++++++++++++++--
> > >   1 file changed, 25 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/target/i386/kvm/tdx.c b/target/i386/kvm/tdx.c
> > > index 629abd267da8..73da15377ec3 100644
> > > --- a/target/i386/kvm/tdx.c
> > > +++ b/target/i386/kvm/tdx.c
> > > @@ -32,6 +32,7 @@
> > >                                        (1U << KVM_FEATURE_PV_SCHED_YIELD) | \
> > >                                        (1U << KVM_FEATURE_MSI_EXT_DEST_ID))
> > > +#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)
> > > @@ -462,13 +463,32 @@ int tdx_kvm_init(MachineState *ms, Error **errp)
> > >       return 0;
> > >   }
> > > -static void setup_td_guest_attributes(X86CPU *x86cpu)
> > > +static int tdx_validate_attributes(TdxGuest *tdx)
> > > +{
> > > +    if (((tdx->attributes & tdx_caps->attrs_fixed0) | tdx_caps->attrs_fixed1) !=
> > > +        tdx->attributes) {
> > > +            error_report("Invalid attributes 0x%lx for TDX VM (fixed0 0x%llx, fixed1 0x%llx)",
> > > +                          tdx->attributes, tdx_caps->attrs_fixed0, tdx_caps->attrs_fixed1);
> > > +            return -EINVAL;
> > > +    }
> > > +
> > > +    if (tdx->attributes & TDX_TD_ATTRIBUTES_DEBUG) {
> > > +        error_report("Current QEMU doesn't support attributes.debug[bit 0] for TDX VM");
> > > +        return -EINVAL;
> > > +    }
> > 
> > Use error_setg() in both cases, passing in a 'Error **errp' object,
> > and 'return -1' instead of returning an errno value.
> > 
> 
> why return -1 instead of -EINVAL?

Returning errno values is useful if the method isn't providing an
"Error **errp" parameter, because it lets the caller report a
more detailed error message via strerror(). Once you add a Error **
parameter though, there is almost never any reason for the caller
to care about the original errno value, and so we use 0 / -1 as
success/fail indicators.

With regards,
Daniel
Xiaoyao Li Aug. 23, 2023, 7:31 a.m. UTC | #5 
On 8/22/2023 10:42 PM, Daniel P. Berrangé wrote:
> On Tue, Aug 22, 2023 at 10:30:47PM +0800, Xiaoyao Li wrote:
>> On 8/21/2023 5:16 PM, Daniel P. Berrangé wrote:
>>> On Fri, Aug 18, 2023 at 05:50:01AM -0400, 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>
>>>> ---
>>>>    target/i386/kvm/tdx.c | 27 +++++++++++++++++++++++++--
>>>>    1 file changed, 25 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/target/i386/kvm/tdx.c b/target/i386/kvm/tdx.c
>>>> index 629abd267da8..73da15377ec3 100644
>>>> --- a/target/i386/kvm/tdx.c
>>>> +++ b/target/i386/kvm/tdx.c
>>>> @@ -32,6 +32,7 @@
>>>>                                         (1U << KVM_FEATURE_PV_SCHED_YIELD) | \
>>>>                                         (1U << KVM_FEATURE_MSI_EXT_DEST_ID))
>>>> +#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)
>>>> @@ -462,13 +463,32 @@ int tdx_kvm_init(MachineState *ms, Error **errp)
>>>>        return 0;
>>>>    }
>>>> -static void setup_td_guest_attributes(X86CPU *x86cpu)
>>>> +static int tdx_validate_attributes(TdxGuest *tdx)
>>>> +{
>>>> +    if (((tdx->attributes & tdx_caps->attrs_fixed0) | tdx_caps->attrs_fixed1) !=
>>>> +        tdx->attributes) {
>>>> +            error_report("Invalid attributes 0x%lx for TDX VM (fixed0 0x%llx, fixed1 0x%llx)",
>>>> +                          tdx->attributes, tdx_caps->attrs_fixed0, tdx_caps->attrs_fixed1);
>>>> +            return -EINVAL;
>>>> +    }
>>>> +
>>>> +    if (tdx->attributes & TDX_TD_ATTRIBUTES_DEBUG) {
>>>> +        error_report("Current QEMU doesn't support attributes.debug[bit 0] for TDX VM");
>>>> +        return -EINVAL;
>>>> +    }
>>>
>>> Use error_setg() in both cases, passing in a 'Error **errp' object,
>>> and 'return -1' instead of returning an errno value.
>>>
>>
>> why return -1 instead of -EINVAL?
> 
> Returning errno values is useful if the method isn't providing an
> "Error **errp" parameter, because it lets the caller report a
> more detailed error message via strerror(). Once you add a Error **
> parameter though, there is almost never any reason for the caller
> to care about the original errno value, and so we use 0 / -1 as
> success/fail indicators.

I see.

Thanks,
-Xiaoyao

> With regards,
> Daniel
diff mbox series

Patch

diff --git a/target/i386/kvm/tdx.c b/target/i386/kvm/tdx.c
index 629abd267da8..73da15377ec3 100644
--- a/target/i386/kvm/tdx.c
+++ b/target/i386/kvm/tdx.c
@@ -32,6 +32,7 @@ 
                                      (1U << KVM_FEATURE_PV_SCHED_YIELD) | \
                                      (1U << KVM_FEATURE_MSI_EXT_DEST_ID))
 
+#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)
@@ -462,13 +463,32 @@  int tdx_kvm_init(MachineState *ms, Error **errp)
     return 0;
 }
 
-static void setup_td_guest_attributes(X86CPU *x86cpu)
+static int tdx_validate_attributes(TdxGuest *tdx)
+{
+    if (((tdx->attributes & tdx_caps->attrs_fixed0) | tdx_caps->attrs_fixed1) !=
+        tdx->attributes) {
+            error_report("Invalid attributes 0x%lx for TDX VM (fixed0 0x%llx, fixed1 0x%llx)",
+                          tdx->attributes, tdx_caps->attrs_fixed0, tdx_caps->attrs_fixed1);
+            return -EINVAL;
+    }
+
+    if (tdx->attributes & TDX_TD_ATTRIBUTES_DEBUG) {
+        error_report("Current QEMU doesn't support attributes.debug[bit 0] for TDX VM");
+        return -EINVAL;
+    }
+
+    return 0;
+}
+
+static int setup_td_guest_attributes(X86CPU *x86cpu)
 {
     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);
 }
 
 int tdx_pre_create_vcpu(CPUState *cpu)
@@ -493,7 +513,10 @@  int tdx_pre_create_vcpu(CPUState *cpu)
         goto out_free;
     }
 
-    setup_td_guest_attributes(x86cpu);
+    r = setup_td_guest_attributes(x86cpu);
+    if (r) {
+        goto out;
+    }
 
     init_vm->cpuid.nent = kvm_x86_arch_cpuid(env, init_vm->cpuid.entries, 0);
     init_vm->attributes = tdx_guest->attributes;