Re: [PATCH 10/25] KVM: TDX: Initialize KVM supported capabilities when module setup

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

On 9/12/2024 4:43 PM, Nikolay Borisov wrote:
> 
> 
> On 12.09.24 г. 11:37 ч., Xiaoyao Li wrote:
>> On 9/12/2024 4:04 PM, Nikolay Borisov wrote:
>>>
>>>
>>> On 5.09.24 г. 16:36 ч., Xiaoyao Li wrote:
>>>> On 9/4/2024 7:58 PM, Nikolay Borisov wrote:
>>>>>
>>>>>
>>>>> On 13.08.24 г. 1:48 ч., Rick Edgecombe wrote:
>>>>>> From: Xiaoyao Li <xiaoyao.li@intel.com>
>>>>>>
>>>>>> While TDX module reports a set of capabilities/features that it
>>>>>> supports, what KVM currently supports might be a subset of them.
>>>>>> E.g., DEBUG and PERFMON are supported by TDX module but currently not
>>>>>> supported by KVM.
>>>>>>
>>>>>> Introduce a new struct kvm_tdx_caps to store KVM's capabilities of 
>>>>>> TDX.
>>>>>> supported_attrs and suppported_xfam are validated against fixed0/1
>>>>>> values enumerated by TDX module. Configurable CPUID bits derive 
>>>>>> from TDX
>>>>>> module plus applying KVM's capabilities (KVM_GET_SUPPORTED_CPUID),
>>>>>> i.e., mask off the bits that are configurable in the view of TDX 
>>>>>> module
>>>>>> but not supported by KVM yet.
>>>>>>
>>>>>> KVM_TDX_CPUID_NO_SUBLEAF is the concept from TDX module, switch it 
>>>>>> to 0
>>>>>> and use KVM_CPUID_FLAG_SIGNIFCANT_INDEX, which are the concept of 
>>>>>> KVM.
>>>>>>
>>>>>> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
>>>>>> Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
>>>>>> ---
>>>>>> uAPI breakout v1:
>>>>>>   - Change setup_kvm_tdx_caps() to use the exported 'struct 
>>>>>> tdx_sysinfo'
>>>>>>     pointer.
>>>>>>   - Change how to copy 'kvm_tdx_cpuid_config' since 'struct 
>>>>>> tdx_sysinfo'
>>>>>>     doesn't have 'kvm_tdx_cpuid_config'.
>>>>>>   - Updates for uAPI changes
>>>>>> ---
>>>>>>   arch/x86/include/uapi/asm/kvm.h |  2 -
>>>>>>   arch/x86/kvm/vmx/tdx.c          | 81 
>>>>>> +++++++++++++++++++++++++++++++++
>>>>>>   2 files changed, 81 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/arch/x86/include/uapi/asm/kvm.h 
>>>>>> b/arch/x86/include/uapi/asm/kvm.h
>>>>>> index 47caf508cca7..c9eb2e2f5559 100644
>>>>>> --- a/arch/x86/include/uapi/asm/kvm.h
>>>>>> +++ b/arch/x86/include/uapi/asm/kvm.h
>>>>>> @@ -952,8 +952,6 @@ struct kvm_tdx_cmd {
>>>>>>       __u64 hw_error;
>>>>>>   };
>>>>>> -#define KVM_TDX_CPUID_NO_SUBLEAF    ((__u32)-1)
>>>>>> -
>>>>>>   struct kvm_tdx_cpuid_config {
>>>>>>       __u32 leaf;
>>>>>>       __u32 sub_leaf;
>>>>>> diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
>>>>>> index 90b44ebaf864..d89973e554f6 100644
>>>>>> --- a/arch/x86/kvm/vmx/tdx.c
>>>>>> +++ b/arch/x86/kvm/vmx/tdx.c
>>>>>> @@ -31,6 +31,19 @@ static void __used tdx_guest_keyid_free(int keyid)
>>>>>>       ida_free(&tdx_guest_keyid_pool, keyid);
>>>>>>   }
>>>>>> +#define KVM_TDX_CPUID_NO_SUBLEAF    ((__u32)-1)
>>>>>> +
>>>>>> +struct kvm_tdx_caps {
>>>>>> +    u64 supported_attrs;
>>>>>> +    u64 supported_xfam;
>>>>>> +
>>>>>> +    u16 num_cpuid_config;
>>>>>> +    /* This must the last member. */
>>>>>> +    DECLARE_FLEX_ARRAY(struct kvm_tdx_cpuid_config, cpuid_configs);
>>>>>> +};
>>>>>> +
>>>>>> +static struct kvm_tdx_caps *kvm_tdx_caps;
>>>>>> +
>>>>>>   static int tdx_get_capabilities(struct kvm_tdx_cmd *cmd)
>>>>>>   {
>>>>>>       const struct tdx_sysinfo_td_conf *td_conf = 
>>>>>> &tdx_sysinfo->td_conf;
>>>>>> @@ -131,6 +144,68 @@ int tdx_vm_ioctl(struct kvm *kvm, void __user 
>>>>>> *argp)
>>>>>>       return r;
>>>>>>   }
>>>>>> +#define KVM_SUPPORTED_TD_ATTRS (TDX_TD_ATTR_SEPT_VE_DISABLE)
>>>>>
>>>>> Why isn't TDX_TD_ATTR_DEBUG added as well?
>>>>
>>>> Because so far KVM doesn't support all the features of a DEBUG TD 
>>>> for userspace. e.g., KVM doesn't provide interface for userspace to 
>>>> read/write private memory of DEBUG TD.
>>>
>>> But this means that you can't really run a TDX with SEPT_VE_DISABLE 
>>> disabled for debugging purposes, so perhaps it might be necessary to 
>>> rethink the condition allowing SEPT_VE_DISABLE to be disabled. 
>>> Without the debug flag and SEPT_VE_DISABLE disabled the code refuses 
>>> to start the VM, what if one wants to debug some SEPT issue by having 
>>> an oops generated inside the vm ?
>>
>> sept_ve_disable is allowed to be disable, i.e., set to 0.
>>
>> I think there must be some misunderstanding.
> 
> There isn't, the current code is:
> 
>    201         if (!(td_attr & ATTR_SEPT_VE_DISABLE)) {
>      1                 const char *msg = "TD misconfiguration: 
> SEPT_VE_DISABLE attribute must be set.";
>      2
>      3                 /* Relax SEPT_VE_DISABLE check for debug TD. */
>      4                 if (td_attr & ATTR_DEBUG)
>      5                         pr_warn("%s\n", msg);
>      6                 else
>      7                         tdx_panic(msg);
>      8         }
> 
> 
> I.e if we disable SEPT_VE_DISABLE without having ATTR_DEBUG it results 
> in a panic.

I see now.

It's linux TD guest's implementation, which requires SEPT_VE_DISABLE 
must be set unless it's a debug TD.

Yes, it can be the motivation to request KVM to add the support of 
ATTRIBUTES.DEBUG. But the support of ATTRIBUTES.DEBUG is not just 
allowing this bit to be set to 1. For DEBUG TD, VMM is allowed to 
read/write the private memory content, cpu registers, and MSRs, VMM is 
allowed to trap the exceptions in TD, VMM is allowed to manipulate the 
VMCS of TD vcpu, etc.

IMHO, for upstream, no need to support all the debug capability as 
described above. But we need firstly define a subset of them as the 
starter of supporting ATTRIBUTES.DEBUG. Otherwise, what is the meaning 
of KVM to allow the DEBUG to be set without providing any debug capability?

For debugging purpose, you can just hack guest kernel to allow 
spet_ve_disable to be 0 without DEBUG bit set, or hack KVM to allow 
DEBUG bit to be set.

>>
>>>>
>>>>> <snip>
>>>>
>>>>
>>