Xiaoyao Li <xiaoyao...@intel.com> writes:
> On 8/22/2023 2:22 PM, Markus Armbruster wrote:
>> Xiaoyao Li <xiaoyao...@intel.com> writes:
>>
>>> Introduce tdx-guest object which implements the interface of
>>> CONFIDENTIAL_GUEST_SUPPORT, and will be used to create TDX VMs (TDs) by
>>>
>>> qemu -machine ...,confidential-guest-support=tdx0 \
>>> -object tdx-guset,id=tdx0
>>
>> Typo: tdx-guest
>
> Will fix it.
>
>>> It has only one property 'attributes' with fixed value 0 and not
>>> configurable so far.
>>
>> This must refer to TdxGuest member @attributes.
>> "Property" suggests QOM property, which @attributes isn't, at least not
>> in this patch. Will it become a QOM property later in this series?
>
> At least not in this series. Maybe in the future there is request to directly
> configure the whole attributes via QOM property, but none from now.
>
> I will change the description of it to avoid confusion.
>
>> Hmm, @attributes appears to remain unused until PATCH 14. Recommend to
>> delay its addition until then.
>
> IMHO, it's not suitable to introduce it in patch 14. Using a separate patch
> seems unnecessary. I'll leave it in this patch unless strong objection on it.
Not worth arguing about.
>>> Signed-off-by: Xiaoyao Li <xiaoyao...@intel.com>
>>> Acked-by: Gerd Hoffmann <kra...@redhat.com>
>>> ---
>>> changes from RFC-V4
>>> - make @attributes not user-settable
>>> ---
>>> configs/devices/i386-softmmu/default.mak | 1 +
>>> hw/i386/Kconfig | 5 +++
>>> qapi/qom.json | 12 +++++++
>>> target/i386/kvm/meson.build | 2 ++
>>> target/i386/kvm/tdx.c | 40 ++++++++++++++++++++++++
>>> target/i386/kvm/tdx.h | 19 +++++++++++
>>> 6 files changed, 79 insertions(+)
>>> create mode 100644 target/i386/kvm/tdx.c
>>> create mode 100644 target/i386/kvm/tdx.h
>>>
>>> diff --git a/configs/devices/i386-softmmu/default.mak
>>> b/configs/devices/i386-softmmu/default.mak
>>> index 598c6646dfc0..9b5ec59d65b0 100644
>>> --- a/configs/devices/i386-softmmu/default.mak
>>> +++ b/configs/devices/i386-softmmu/default.mak
>>> @@ -18,6 +18,7 @@
>>> #CONFIG_QXL=n
>>> #CONFIG_SEV=n
>>> #CONFIG_SGA=n
>>> +#CONFIG_TDX=n
>>> #CONFIG_TEST_DEVICES=n
>>> #CONFIG_TPM_CRB=n
>>> #CONFIG_TPM_TIS_ISA=n
>>> diff --git a/hw/i386/Kconfig b/hw/i386/Kconfig
>>> index 9051083c1e78..929f6c3f0e85 100644
>>> --- a/hw/i386/Kconfig
>>> +++ b/hw/i386/Kconfig
>>> @@ -10,6 +10,10 @@ config SGX
>>> bool
>>> depends on KVM
>>> +config TDX
>>> + bool
>>> + depends on KVM
>>> +
>>> config PC
>>> bool
>>> imply APPLESMC
>>> @@ -26,6 +30,7 @@ config PC
>>> imply QXL
>>> imply SEV
>>> imply SGX
>>> + imply TDX
>>> imply TEST_DEVICES
>>> imply TPM_CRB
>>> imply TPM_TIS_ISA
>>> diff --git a/qapi/qom.json b/qapi/qom.json
>>> index e0b2044e3d20..2ca7ce7c0da5 100644
>>> --- a/qapi/qom.json
>>> +++ b/qapi/qom.json
>>> @@ -866,6 +866,16 @@
>>> 'reduced-phys-bits': 'uint32',
>>> '*kernel-hashes': 'bool' } }
>>> +##
>>> +# @TdxGuestProperties:
>>> +#
>>> +# Properties for tdx-guest objects.
>>> +#
>>> +# Since: 8.2
>>> +##
>>> +{ 'struct': 'TdxGuestProperties',
>>> + 'data': { }}
>>> +
>>> ##
>>> # @ThreadContextProperties:
>>> #
>>> @@ -944,6 +954,7 @@
>>> 'sev-guest',
>>> 'thread-context',
>>> 's390-pv-guest',
>>> + 'tdx-guest',
>>> 'throttle-group',
>>> 'tls-creds-anon',
>>> 'tls-creds-psk',
>>> @@ -1010,6 +1021,7 @@
>>> 'secret_keyring': { 'type': 'SecretKeyringProperties',
>>> 'if': 'CONFIG_SECRET_KEYRING' },
>>> 'sev-guest': 'SevGuestProperties',
>>> + 'tdx-guest': 'TdxGuestProperties',
>>> 'thread-context': 'ThreadContextProperties',
>>> 'throttle-group': 'ThrottleGroupProperties',
>>> 'tls-creds-anon': 'TlsCredsAnonProperties',
>>
>> Actually useful only when CONFIG_TDX is on, but can't make it
>> conditional here, as CONFIG_TDX is poisoned.
>
> In fact, I just followed what SEV did.
Yup.
> To me, it looks OK to make it conditional on CONFIG_TDX. Could you please
> elaborate "but can't make it conditional here, as CONFIG_TDX is poisoned." ?
CONFIG_TDX is one of the macros that can only be used in
target-dependent code. Enforced by config-poison.h's
#pragma GCC poison CONFIG_TDX
The code generated from qom.json is target-independent.
To use 'if': 'CONFIG_TDX', we'd have to move the definition to a
target-dependent QAPI module, say qom-machine.json. Sadly, that's more
trouble than it's worth.
[...]
