Re: [PATCH v2 01/17] x86/virt/tdx: Embed version info in SEAMCALL leaf function definitions

[Xu Yilun <yilun.xu@linux.intel.com>]

> This is jumping the gun a bit in the changelog.
> 
> What is a SEAMCALL leaf function?
> 
> How does the version fit in?

I think the word "leaf function" is confusing, maybe I should say
"... SEAMCALL interface function definitions..."?

And I missed some more context explanation here:

  SEAMCALL accepts parameters via CPU registers (RAX, RCX, RDX, ...),
  where RAX selects the specific TDX module function to execute. According
  to the TDX module SPEC, RAX is mainly composed of:

  1. leaf number: selects the function.
  2. version number: selects the function version. The newer version
     defines more operations. It also defines additional parameters for
     the rest of registers.

  Newer version SEAMCALL uses the same leaf number as older versions,
  with only the version number increased. Newer version SEAMCALLs are
  guaranteed to be backward compatible.

[...]

> > The old TDX modules that only support TDH.VP.INIT v0 are all deprecated,
> > so only provide the latest (v1) definition.
> 
> No, this isn't how this is going to work.
> 
> What do we *NEED* from v1? Why churn the code if we don't *NEED*
> something from v1 and can live with v0?

Sorry for the confusing words, for this TDH.VP.INIT, kernel is now using
v1, we need v1 for virtual x2APIC. Maybe I should add "No functional change
intended."

> It has *ZERO* to do with the TDX module being deprecated or whatever.

Since newer version SEAMCALLs are always backward compatible. TDH.VP.INIT v0
is only needed when we need to support legacy modules that don't understand
TDH.VP.INIT v1. We don't support such legacy modules, so I meant to
eliminate TDH.VP.INIT v0 definition and reduce histroy burdens.

We have some previous discussion that leads to "kernel don't keep SEAMCALL
version at all":

https://lore.kernel.org/all/ca331aa3-6304-4e07-9ed9-94dc69726382@intel.com/

But I have trouble for not keeping versions for TDH.SYS.CONFIG/UPDATE (in
next patch). No public module supports TDH.SYS.CONFIG/UPDATE v1 so I can't
eliminate TDH.SYS.CONFIG/UPDATE v0. I'm GOOD we are now considering keep
aware of SEAMCALL versions at some level. Keeping in mind which module
is published is another burden...

[...]

> But that whole scheme falls apart the first time the kernel needs
> functionality from v2. You'll need:
> 
> #define TDH_VP_INIT_V0			SEAMCALL_LEAF_VER(22, 0)
> #define TDH_VP_INIT_V1			SEAMCALL_LEAF_VER(22, 1)
> 
> and then the calls will do:
> 
> 	if (foo)
> 		return seamcall(TDH_VP_INIT_V0, &args);
> 	else
> 		return seamcall(TDH_VP_INIT_V1, &args);
> 
> So this 100% goes down the road of needing #defines for *EACH* version.

TDH_VP_INIT_V0 has no use case in current code, so maybe not #define for
EACH version.

And we have 100+ SEAMCALLs in SPEC, 30+ is being used in Linux, 20+ is
comming, only 3 need versions now. I think making versioned definitions
for EACH of them is overkill.

I see you have another suggestion in next patch which avoids the
#defines. Let me continue in that patch.


> The whole seamcall RAX thing is one step too clever. I think Linux did
> the right thing:
> 
> 5	common	open				sys_open
> 288	common	openat				sys_openat
> 437	common	openat2				sys_openat2
> 
> New ABI gets a new base number. No need for the other end of the ABI to
> know that 288 is arguably a later version of 5.

Yes, that's true.

> 
> Ugh. But why is this patch even in here in the first place? Why is this
> even *ASSOCIATED* with DICE-based attestation? Isn't this completely
> orthogonal?

TDH.SYS.UPDATE/CONFIG is the second user of the version stuff. So as
required, fix the open code issue of the first existing user.

https://lore.kernel.org/all/62bec236-4716-4326-8342-1863ad8a3f24@intel.com/

Thanks,
Yilun