>-----Original Message-----
>From: Li, Xiaoyao <xiaoyao.li@xxxxxxxxx>
>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@xxxxxxxxx>
>>> Acked-by: Gerd Hoffmann <kraxel@xxxxxxxxxx>
>>> Acked-by: Markus Armbruster <armbru@xxxxxxxxxx>
>>> ---
>>> 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@xxxxxxxxxxxxxxxxx/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.
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
