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.
