Re: [PATCH 25/25] KVM: x86: Add CPUID bits missing from KVM_GET_SUPPORTED_CPUID

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

On 9/12/2024 10:09 PM, Paolo Bonzini wrote:
> On Thu, Sep 12, 2024 at 9:48 AM Xiaoyao Li <xiaoyao.li@intel.com> wrote:
>> On 9/11/2024 1:52 AM, Paolo Bonzini wrote:
>>> On 8/13/24 00:48, Rick Edgecombe wrote:
>>>> For KVM_TDX_CAPABILITIES, there was also the problem of bits that are
>>>> actually supported by KVM, but missing from get_supported_cpuid() for one
>>>> reason or another. These include X86_FEATURE_MWAIT, X86_FEATURE_HT and
>>>> X86_FEATURE_TSC_DEADLINE_TIMER. This is currently worked around in
>>>> QEMU by
>>>> adjusting which features are expected.
>>
>> I'm not sure what issue/problem can be worked around in QEMU.
>> QEMU doesn't expect these bit are reported by KVM as supported for TDX.
>> QEMU just accepts the result reported by KVM.
> 
> QEMU already adds some extra bits, for example:
> 
>          ret |= CPUID_EXT_HYPERVISOR;
>          if (kvm_irqchip_in_kernel() &&
>                  kvm_check_extension(s, KVM_CAP_TSC_DEADLINE_TIMER)) {
>              ret |= CPUID_EXT_TSC_DEADLINE_TIMER;
>          }
> 
>> The problem is, TDX module and the hardware allow these bits be
>> configured for TD guest, but KVM doesn't allow. It leads to users cannot
>> create a TD with these bits on.
> 
> KVM is not going to have any checks, it's only going to pass the
> CPUID to the TDX module and return an error if the check fails
> in the TDX module.

If so, new feature can be enabled for TDs out of KVM's control.

Is it acceptable?

> KVM can have a TDX-specific version of KVM_GET_SUPPORTED_CPUID, so
> that we can keep a variant of the "get supported bits and pass them
> to KVM_SET_CPUID2" logic, but that's it.
> 
>>> This is the kind of API that we need to present for TDX, even if the
>>> details on how to get the supported CPUID are different.  Not because
>>> it's a great API, but rather because it's a known API.
>>
>> However there are differences for TDX. For legacy VMs, the result of
>> KVM_GET_SUPPORTED_CPUID isn't used to filter the input of KVM_SET_CPUID2.
>> But for TDX, it needs to filter the input of KVM_TDX_VM_INIT.CPUID[]
>> because TDX module only allows the bits that are reported as
>> configurable to be set to 1.
> 
> Yes, that's userspace's responsibility.
> 
>> With current designed API, QEMU can only know which bits are
>> configurable before KVM_TDX_VM_INIT, i.e., which bits can be set to 1 or
>> 0 freely.
> 
> The API needs userspace to have full knowledge of the
> requirements of the TDX module, if it wants to change the
> defaults provided by KVM.
> 
> This is the same as for non-TDX VMs (including SNP).  The only
> difference is that TDX and SNP fails, while non-confidential VMs
> get slightly garbage CPUID.
> 
>> For other bits not reported as configurable, QEMU can know the exact
>> value of them via KVM_TDX_GET_CPUID, after KVM_TDX_VM_INIT and before
>> TD's running. With it, QEMU can validate the return value is matched
>> with what QEMU wants to set that determined by users input. If not
>> matched, QEMU can provide some warnings like what for legacy VMs:
>>
>>     - TDX doesn't support requested feature: CPUID.01H.ECX.tsc-deadline
>> [bit 24]
>>     - TDX forcibly sets features: CPUID.01H:ECX.hypervisor [bit 31]
>>
>> If there are ioctls to report the fixed0 bits and fixed1 bits for TDX,
>> QEMU can validate the user's configuration earlier.
> 
> Yes, that's fine.
> 
> Paolo
>