Received-SPF: pass (google.com: domain of linux-kernel+bounces-140734-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Message-ID: <2c11bb62-874e-4e9e-89b1-859df5b560bc@intel.com>
Date: Thu, 11 Apr 2024 23:16:19 +0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [ANNOUNCE] PUCK Notes - 2024.04.03 - TDX Upstreaming Strategy
To: Sean Christopherson <seanjc@google.com>,
 Rick P Edgecombe <rick.p.edgecombe@intel.com>
Cc: "davidskidmore@google.com" <davidskidmore@google.com>,
 "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
 "srutherford@google.com" <srutherford@google.com>,
 "pankaj.gupta@amd.com" <pankaj.gupta@amd.com>,
 "kvm@vger.kernel.org" <kvm@vger.kernel.org>,
 Isaku Yamahata <isaku.yamahata@intel.com>, Wei W Wang <wei.w.wang@intel.com>
References: <ZhQ8UCf40UeGyfE_@google.com>
 <5faaeaa7bc66dbc4ea86a64ef8e8f9b22fd22ef4.camel@intel.com>
 <ZhRxWxRLbnrqwQYw@google.com>
 <957b26d18ba7db611ed6582366066667267d10b8.camel@intel.com>
 <ZhSb28hHoyJ55-ga@google.com>
 <8b40f8b1d1fa915116ef1c95a13db0e55d3d91f2.camel@intel.com>
 <ZhVdh4afvTPq5ssx@google.com>
 <4ae4769a6f343a2f4d3648e4348810df069f24b7.camel@intel.com>
 <ZhVsHVqaff7AKagu@google.com>
 <b1d112bf0ff55073c4e33a76377f17d48dc038ac.camel@intel.com>
 <ZhfyNLKsTBUOI7Vp@google.com>
Content-Language: en-US
From: Xiaoyao Li <xiaoyao.li@intel.com>
In-Reply-To: <ZhfyNLKsTBUOI7Vp@google.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit

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.)