Re: [PATCH 10/13] x86/virt/tdx: Add SEAMCALL wrappers to remove a TD private page

[Yan Zhao]

On Wed, Jan 08, 2025 at 08:31:14AM -0800, Dave Hansen wrote:
> On 1/7/25 16:41, Yan Zhao wrote:
> > There is a proposed fix to change the type of KeyID to u16 as shown below (not
> > yet split and sent out). Do you think this change to u16 makes sense?
> 
> I just think that the concept of a KeyID and the current implementation
> on today's hardware are different things. Don't confuse an
> implementation with the _concept_.
> 
> It can also make a lot of sense to pass around a 16-bit value in an
> 'int' in some cases. Think about NUMA nodes. You can't have negative
> NUMA nodes in hardware, but we use 'int' in the kernel everywhere
> because NUMA_NO_NODE gets passed around a lot.
> 
> Anyway, my point is that the underlying hardware types stop having
> meaning at _some_ level of abstraction in the interfaces.
Thanks for explaining the reasoning behind the preference to "int" and pointing
to the example of NUMA nodes.
It helps a lot for me to understand the underlying principle in kernel design!

Regarding the TDX hkid, do we need a similar check for the hkid (as that for
NUMA nodes) to avoid unexpected SEAMCALL error or overflow?

static inline bool numa_valid_node(int nid)
{
        return nid >= 0 && nid < MAX_NUMNODES;
}


> I'd personally probably just keep 'hkid' as an int everywhere until the
> point where it gets shoved into the TDX module ABI.
> 
> Oh, and casts like this:
> 
> >  static inline void tdx_disassociate_vp(struct kvm_vcpu *vcpu)
> > @@ -2354,7 +2354,8 @@ static int __tdx_td_init(struct kvm *kvm, struct td_params *td_params,
> >  	ret = tdx_guest_keyid_alloc();
> >  	if (ret < 0)
> >  		return ret;
> > -	kvm_tdx->hkid = ret;
> > +	kvm_tdx->hkid = (u16)ret;
> > +	kvm_tdx->hkid_assigned = true;
> 
> are a bit silly, don't you think?
Agreed. That's the part I don't like about this fix.