Re: [ANNOUNCE] PUCK Notes - 2024.04.03 - TDX Upstreaming Strategy

Xiaoyao Li
2024-04-11T15:16:26Z

On 4/11/2024 10:22 PM, Sean Christopherson wrote:
> On Thu, Apr 11, 2024, Rick P Edgecombe wrote:
>> On Tue, 2024-04-09 at 09:26 -0700, Sean Christopherson wrote:
>>>> Haha, if this is the confusion, I see why you reacted that way to "JSON".
>>>> That would be quite the curious choice for a TDX module API.
>>>>
>>>> So it is easy to convert it to a C struct and embed it in KVM. It's just not
>>>> that useful because it will not necessarily be valid for future TDX modules.
>>>
>>> No, I don't want to embed anything in KVM, that's the exact same as hardcoding
>>> crud into KVM, which is what I want to avoid.  I want to be able to roll out a
>>> new TDX module with any kernel changes, and I want userspace to be able to
>>> assert
>>> that, for a given TDX module, the effective guest CPUID configuration aligns
>>> with
>>> userspace's desired the vCPU model, i.e. that the value of fixed bits match up
>>> with the guest CPUID that userspace wants to define.
>>>
>>> Maybe that just means converting the JSON file into some binary format that
>>> the
>>> kernel can already parse.  But I want Intel to commit to providing that
>>> metadata
>>> along with every TDX module.
>>
>> Oof. It turns out in one of the JSON files there is a description of a different
>> interface (TDX module runtime interface) that provides a way to read CPUID data
>> that is configured in a TD, including fixed bits. It works like:
>> 1. VMM queries which CPUID bits are directly configurable.
>> 2. VMM provides directly configurable CPUID bits, along with XFAM and
>> ATTRIBUTES, via TDH.MNG.INIT. (KVM_TDX_INIT_VM)
>> 3. Then VMM can use this other interface via TDH.MNG.RD, to query the resulting
>> values of specific CPUID leafs.
>>
>> This does not provide a way to query the fixed bits specifically, it tells you
>> what ended up getting configuring in a specific TD, which includes the fixed
>> bits and anything else. So we need to do KVM_TDX_INIT_VM before KVM_SET_CPUID in
>> order to have something to check against. But there was discussion of
>> KVM_SET_CPUID on CPU0 having the CPUID state to pass to KVM_TDX_INIT_VM. So that
>> would need to be sorted.
>>
>> If we pass the directly configurable values with KVM_TDX_INIT_VM, like we do
>> today, then the data provided by this interface should allow us to check
>> consistency between KVM_SET_CPUID and the actual configured TD CPUID behavior.
>
> I think it would be a good (optional?) sanity check, e.g. KVM_BUG_ON() if the
> post-KVM_TDX_INIT_VM CPUID set doesn't match KVM's internal data. But that alone
> provides a terrible experience for userspace.
>
> - The VMM would still need to hardcode knowledge of fixed bits, without a way
> to do a sanity check of its own.

Maybe we can do it this way to avoid hardcode:

1. KVM can get the configurable CPUID bits from TDX module with
TDH.SYS.RD (they are the old info of TD_SYSINFO.CPUID_CONFIG[]), and
report them to userspace;

2. userspace configures the configurable CPUID bits and pass them to KVM
to init TD.

3. After TD is initialized via TDH.MNG.INIT, KVM can get a full CPUID
list of TD via TDH.MNG.RD. KVM provides interface to report the full
CPUID list to userspace.

4. Userspace can sanity check the full CPUID list.
- the configurable bits reported in #1 should be what they have been
configured;
- the dynamic bits and other special bits will be checked case by case;
- the rest bits should be fixed. If the value is not what user
wants, userspace prints error to user and stop.

Does it sounds reasonable?

> - Lack of a sanity check means the VMM can't fail VM creation early.
>
> - KVM_SET_CPUID2 doesn't have a way to inform userspace _which_ CPUID bits are
> "bad".
>
> - Neither userspace nor KVM can programming detect when bits are fixed vs.
> flexible. E.g. it's not impossible that userspace would want to do X if a
> feature is fixed, but Y if it's flexible.

flexible (configurable) bits is known to VMM (KVM and userspace) because
TDX module has interface to report them. So we can treat a bit as fixed
if it is not reported in the flexible group. (of course the dynamic bits
are special and excluded.)