Re: [PATCH 21/25] KVM: x86: Introduce KVM_TDX_GET_CPUID

[Tony Lindgren <tony.lindgren@linux.intel.com>]

On Mon, Aug 19, 2024 at 01:02:02PM +0800, Xu Yilun wrote:
> On Mon, Aug 12, 2024 at 03:48:16PM -0700, Rick Edgecombe wrote:
> > From: Xiaoyao Li <xiaoyao.li@intel.com>
> > +static int tdx_mask_cpuid(struct kvm_tdx *tdx, struct kvm_cpuid_entry2 *entry)
> > +{
> > +	u64 field_id = TD_MD_FIELD_ID_CPUID_VALUES;
> > +	u64 ebx_eax, edx_ecx;
> > +	u64 err = 0;
> > +
> > +	if (entry->function & TDX_MD_UNREADABLE_LEAF_MASK ||
> > +	    entry->index & TDX_MD_UNREADABLE_SUBLEAF_MASK)
> > +		return -EINVAL;
> > +
> > +	/*
> > +	 * bit 23:17, REVSERVED: reserved, must be 0;
> > +	 * bit 16,    LEAF_31: leaf number bit 31;
> > +	 * bit 15:9,  LEAF_6_0: leaf number bits 6:0, leaf bits 30:7 are
> > +	 *                      implicitly 0;
> > +	 * bit 8,     SUBLEAF_NA: sub-leaf not applicable flag;
> > +	 * bit 7:1,   SUBLEAF_6_0: sub-leaf number bits 6:0. If SUBLEAF_NA is 1,
> > +	 *                         the SUBLEAF_6_0 is all-1.
> > +	 *                         sub-leaf bits 31:7 are implicitly 0;
> > +	 * bit 0,     ELEMENT_I: Element index within field;
> > +	 */
> > +	field_id |= ((entry->function & 0x80000000) ? 1 : 0) << 16;
> > +	field_id |= (entry->function & 0x7f) << 9;
> > +	if (entry->flags & KVM_CPUID_FLAG_SIGNIFCANT_INDEX)
> > +		field_id |= (entry->index & 0x7f) << 1;
> > +	else
> > +		field_id |= 0x1fe;
> > +
> > +	err = tdx_td_metadata_field_read(tdx, field_id, &ebx_eax);
> > +	if (err) //TODO check for specific errors
> > +		goto err_out;
> > +
> > +	entry->eax &= (u32) ebx_eax;
> > +	entry->ebx &= (u32) (ebx_eax >> 32);
> 
> Some fields contains a N-bits wide value instead of a bitmask, why a &=
> just work?

There's the CPUID 0x80000008 workaround, I wonder if we are missing some
other handling though. Do you have some specific CPUIDs bits in mind to
check?

The handling for the supported CPUID values mask from the TDX module is
a bit unclear for sure :)

> > +static int tdx_vcpu_get_cpuid(struct kvm_vcpu *vcpu, struct kvm_tdx_cmd *cmd)
> > +{
> > +	struct kvm_cpuid2 __user *output, *td_cpuid;
> > +	struct kvm_tdx *kvm_tdx = to_kvm_tdx(vcpu->kvm);
> > +	struct kvm_cpuid2 *supported_cpuid;
> > +	int r = 0, i, j = 0;
> > +
> > +	output = u64_to_user_ptr(cmd->data);
> > +	td_cpuid = kzalloc(sizeof(*td_cpuid) +
> > +			sizeof(output->entries[0]) * KVM_MAX_CPUID_ENTRIES,
> > +			GFP_KERNEL);
> > +	if (!td_cpuid)
> > +		return -ENOMEM;
> > +
> > +	r = tdx_get_kvm_supported_cpuid(&supported_cpuid);
> 
> Personally I don't like the definition of this function. I need to look
> into the inner implementation to see if kfree(supported_cpuid); is needed
> or safe. How about:
> 
>   supported_cpuid = tdx_get_kvm_supported_cpuid();
>   if (!supported_cpuid)
> 	goto out_td_cpuid;

So allocate in tdx_get_kvm_supported_cpuid() and the caller frees. Sounds
cleaner to me.

> > +		/*
> > +		 * Work around missing support on old TDX modules, fetch
> > +		 * guest maxpa from gfn_direct_bits.
> > +		 */
> > +		if (output_e->function == 0x80000008) {
> > +			gpa_t gpa_bits = gfn_to_gpa(kvm_gfn_direct_bits(vcpu->kvm));
> > +			unsigned int g_maxpa = __ffs(gpa_bits) + 1;
> > +
> > +			output_e->eax &= ~0x00ff0000;
> > +			output_e->eax |= g_maxpa << 16;
> 
> Is it possible this workaround escapes the KVM supported bits check?

Yes it might need a mask for (g_maxpa << 16) & 0x00ff0000 to avoid setting
the wrong bits, will check.

...
> > +out:
> > +	kfree(td_cpuid);
> > +	kfree(supported_cpuid);
> 
> Traditionally we do:
> 
>   out_supported_cpuid:
> 	kfree(supported_cpuid);
>   out_td_cpuid:
> 	kfree(td_cpuid);
> 
> I'm not sure what's the advantage to make people think more about whether
> kfree is safe.

I'll do a patch for this thanks.

> > --- a/arch/x86/kvm/vmx/tdx.h
> > +++ b/arch/x86/kvm/vmx/tdx.h
> > @@ -25,6 +25,11 @@ struct kvm_tdx {
> >  	bool finalized;
> >  
> >  	u64 tsc_offset;
> > +
> > +	/* For KVM_MAP_MEMORY and KVM_TDX_INIT_MEM_REGION. */
> > +	atomic64_t nr_premapped;
> 
> This doesn't belong to this patch.
> 
> > +
> > +	struct kvm_cpuid2 *cpuid;
> 
> Didn't find the usage of this field.

Thanks will check and drop.

Regards,

Tony