Re: [RFC PATCH v4 13/36] i386/tdx: Validate TD attributes

[Xiaoyao Li <xiaoyao.li@intel.com>]

On 5/23/2022 5:39 PM, Gerd Hoffmann wrote:
>> Validate TD attributes with tdx_caps that fixed-0 bits must be zero and
>> fixed-1 bits must be set.
> 
>> -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;
>> +    }
> 
> So, how is this supposed to work?  Patch #2 introduces attributes as
> user-settable property.  So do users have to manually figure and pass
> the correct value, so the check passes?  Specifically the fixed1 check?
> 
> I think 'attributes' should not be user-settable in the first place.
> Each feature-bit which is actually user-settable (and not already
> covered by another option like pmu) should be a separate attribute for
> tdx-object.  Then the tdx code can create attributes from hardware
> capabilities and user settings.

In patch #2, tdx-guest.attributes is defined as a field to hold a 64 
bits value of attributes but it doesn't provide any getter/setter for 
it. So it's *not* user-settable.

Did I miss something? (I'm not good at QEMU object)

> When user-settable options might not be available depending on hardware
> capabilities best practice is to create them as OnOffAuto properties.
> 
>    Auto == qemu can pick the value, typical behavior is to enable the
>            feature if the hardware supports it.
>    On == must enable, if it isn't possible throw an error and exit.
>    Off == must disable, if it isn't possible throw an error and exit.
> 
> take care,
>    Gerd
>