Re: [PATCH v5 25/65] i386/tdx: Add property sept-ve-disable for tdx-guest object

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

On 6/13/2024 4:35 PM, Duan, Zhenzhong wrote:
> 
> 
>> -----Original Message-----
>> From: Li, Xiaoyao <xiaoyao.li@intel.com>
>> Subject: Re: [PATCH v5 25/65] i386/tdx: Add property sept-ve-disable for
>> tdx-guest object
>>
>> On 6/6/2024 6:45 PM, Daniel P. Berrangé wrote:
>>> Copying  Zhenzhong Duan as my point relates to the proposed libvirt
>>> TDX patches.
>>>
>>> On Thu, Feb 29, 2024 at 01:36:46AM -0500, Xiaoyao Li wrote:
>>>> Bit 28 of TD attribute, named SEPT_VE_DISABLE. When set to 1, it
>> disables
>>>> EPT violation conversion to #VE on guest TD access of PENDING pages.
>>>>
>>>> Some guest OS (e.g., Linux TD guest) may require this bit as 1.
>>>> Otherwise refuse to boot.
>>>>
>>>> Add sept-ve-disable property for tdx-guest object, for user to configure
>>>> this bit.
>>>>
>>>> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
>>>> Acked-by: Gerd Hoffmann <kraxel@redhat.com>
>>>> Acked-by: Markus Armbruster <armbru@redhat.com>
>>>> ---
>>>> Changes in v4:
>>>> - collect Acked-by from Markus
>>>>
>>>> Changes in v3:
>>>> - update the comment of property @sept-ve-disable to make it more
>>>>     descriptive and use new format. (Daniel and Markus)
>>>> ---
>>>>    qapi/qom.json         |  7 ++++++-
>>>>    target/i386/kvm/tdx.c | 24 ++++++++++++++++++++++++
>>>>    2 files changed, 30 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/qapi/qom.json b/qapi/qom.json
>>>> index 220cc6c98d4b..89ed89b9b46e 100644
>>>> --- a/qapi/qom.json
>>>> +++ b/qapi/qom.json
>>>> @@ -900,10 +900,15 @@
>>>>    #
>>>>    # Properties for tdx-guest objects.
>>>>    #
>>>> +# @sept-ve-disable: toggle bit 28 of TD attributes to control disabling
>>>> +#     of EPT violation conversion to #VE on guest TD access of PENDING
>>>> +#     pages.  Some guest OS (e.g., Linux TD guest) may require this to
>>>> +#     be set, otherwise they refuse to boot.
>>>> +#
>>>>    # Since: 9.0
>>>>    ##
>>>>    { 'struct': 'TdxGuestProperties',
>>>> -  'data': { }}
>>>> +  'data': { '*sept-ve-disable': 'bool' } }
>>>
>>> So this exposes a single boolean property that gets mapped into one
>>> specific bit in the TD attributes:
>>>
>>>> +
>>>> +static void tdx_guest_set_sept_ve_disable(Object *obj, bool value, Error
>> **errp)
>>>> +{
>>>> +    TdxGuest *tdx = TDX_GUEST(obj);
>>>> +
>>>> +    if (value) {
>>>> +        tdx->attributes |= TDX_TD_ATTRIBUTES_SEPT_VE_DISABLE;
>>>> +    } else {
>>>> +        tdx->attributes &= ~TDX_TD_ATTRIBUTES_SEPT_VE_DISABLE;
>>>> +    }
>>>> +}
>>>
>>> If I look at the documentation for TD attributes
>>>
>>>     https://download.01.org/intel-sgx/latest/dcap-
>> latest/linux/docs/Intel_TDX_DCAP_Quoting_Library_API.pdf
>>>
>>> Section "A.3.4. TD Attributes"
>>>
>>> I see "TD attributes" is a 64-bit int, with 5 bits currently
>>> defined "DEBUG", "SEPT_VE_DISABLE", "PKS", "PL", "PERFMON",
>>> and the rest currently reserved for future use. This makes me
>>> wonder about our modelling approach into the future ?
>>>
>>> For the AMD SEV equivalent we've just directly exposed the whole
>>> field as an int:
>>>
>>>        'policy' : 'uint32',
>>>
>>> For the proposed SEV-SNP patches, the same has been done again
>>>
>>> https://lists.nongnu.org/archive/html/qemu-devel/2024-
>> 06/msg00536.html
>>>
>>>        '*policy': 'uint64',
>>>
>>>
>>> The advantage of exposing individual booleans is that it is
>>> self-documenting at the QAPI level, but the disadvantage is
>>> that every time we want to expose ability to control a new
>>> bit in the policy we have to modify QEMU, libvirt, the mgmt
>>> app above libvirt, and whatever tools the end user has to
>>> talk to the mgmt app.
>>>
>>> If we expose a policy int, then newly defined bits only require
>>> a change in QEMU, and everything above QEMU will already be
>>> capable of setting it.
>>>
>>> In fact if I look at the proposed libvirt patches, they have
>>> proposed just exposing a policy "int" field in the XML, which
>>> then has to be unpacked to set the individual QAPI booleans
>>>
>>>
>> https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/message/WXWX
>> EESYUA77DP7YIBP55T2OPSVKV5QW/
>>>
>>> On balance, I think it would be better if QEMU just exposed
>>> the raw TD attributes policy as an uint64 at QAPI, instead
>>> of trying to unpack it to discrete bool fields. This gives
>>> consistency with SEV and SEV-SNP, and with what's proposed
>>> at the libvirt level, and minimizes future changes when
>>> more policy bits are defined.
>>
>> The reasons why introducing individual bit of sept-ve-disable instead of
>> a raw TD attribute as a whole are that
>>
>> 1. other bits like perfmon, PKS, KL are associated with cpu properties,
>> e.g.,
>>
>> 	perfmon -> pmu,
>> 	pks -> pks,
>> 	kl -> keylokcer feature that QEMU currently doesn't support
>>
>> If allowing configuring attribute directly, we need to deal with the
>> inconsistence between attribute vs cpu property.
> 
> What about defining those bits associated with cpu properties reserved
> But other bits work as Daniel suggested way.

I don't understand. Do you mean we provide the interface to configure 
raw 64 bit attributes while some bits of it are reserved?

> Thanks
> Zhenzhong
> 
>>
>> 2. people need to know the exact bit position of each attribute. I don't
>> think it is a user-friendly interface to require user to be aware of
>> such details.
>>
>> For example, if user wants to create a Debug TD, user just needs to set
>> 'debug=on' for tdx-guest object. It's much more friendly than that user
>> needs to set the bit 0 of the attribute.
>>
>>
>>>> +
>>>>    /* tdx guest */
>>>>    OBJECT_DEFINE_TYPE_WITH_INTERFACES(TdxGuest,
>>>>                                       tdx_guest,
>>>> @@ -529,6 +549,10 @@ static void tdx_guest_init(Object *obj)
>>>>        qemu_mutex_init(&tdx->lock);
>>>>
>>>>        tdx->attributes = 0;
>>>> +
>>>> +    object_property_add_bool(obj, "sept-ve-disable",
>>>> +                             tdx_guest_get_sept_ve_disable,
>>>> +                             tdx_guest_set_sept_ve_disable);
>>>>    }
>>>>
>>>>    static void tdx_guest_finalize(Object *obj)
>>>> --
>>>> 2.34.1
>>>>
>>>
>>> With regards,
>>> Daniel
>