Message-ID: <aQTf0EXxtsi_4UaB@google.com>
Date: Fri, 31 Oct 2025 16:12:00 +0000
From: Sean Christopherson <seanjc@...gle.com>
To: Sagi Shahar <sagis@...gle.com>
Cc: Ira Weiny <ira.weiny@...el.com>, linux-kselftest@...r.kernel.org, 
	Paolo Bonzini <pbonzini@...hat.com>, Shuah Khan <shuah@...nel.org>, 
	Ackerley Tng <ackerleytng@...gle.com>, Ryan Afranji <afranji@...gle.com>, 
	Andrew Jones <ajones@...tanamicro.com>, Isaku Yamahata <isaku.yamahata@...el.com>, 
	Erdem Aktas <erdemaktas@...gle.com>, Rick Edgecombe <rick.p.edgecombe@...el.com>, 
	Roger Wang <runanwang@...gle.com>, Binbin Wu <binbin.wu@...ux.intel.com>, 
	Oliver Upton <oliver.upton@...ux.dev>, "Pratik R. Sampat" <pratikrajesh.sampat@....com>, 
	Reinette Chatre <reinette.chatre@...el.com>, Chao Gao <chao.gao@...el.com>, 
	Chenyi Qiang <chenyi.qiang@...el.com>, linux-kernel@...r.kernel.org, kvm@...r.kernel.org
Subject: Re: [PATCH v12 20/23] KVM: selftests: Add support for TDX TDCALL from guest

On Fri, Oct 31, 2025, Sagi Shahar wrote:
> On Fri, Oct 31, 2025 at 10:15 AM Sean Christopherson <seanjc@...gle.com> wrote:
> >
> > On Fri, Oct 31, 2025, Ira Weiny wrote:
> > > Sagi Shahar wrote:
> > > > From: Erdem Aktas <erdemaktas@...gle.com>
> > > >
> > > > Add support for TDX guests to issue TDCALLs to the TDX module.
> > >
> > > Generally it is nice to have more details.  As someone new to TDX I
> > > have to remind myself what a TDCALL is.  And any random kernel developer
> > > reading this in the future will likely have even less clue than me.
> > >
> > > Paraphrased from the spec:
> > >
> > > TDCALL is the instruction used by the guest TD software (in TDX non-root
> > > mode) to invoke guest-side TDX functions.  TDG.VP.VMCALL helps invoke
> > > services from the host VMM.
> > >
> > > Add support for TDX guests to invoke services from the host VMM.
> >
> > Eh, at some point a baseline amount of knowledge is required.  I highly doubt
> > regurgitating the spec is going to make a huge difference
> >
> > I also dislike the above wording, because it doesn't help understand _why_ KVM
> > selftests need to support TDCALL, or _how_ the functionality will be utilized.
> > E.g. strictly speaking, we could write KVM selftests without ever doing a single
> > TDG.VP.VMCALL, because we control both sides (guest and VMM).  And I have a hard
> > time belive name-dropping TDG.VP.VMCALL is going to connect the dots between
> > TDCALL and the "tunneling" scheme defined by the GHCI for requesting emulation
> > of "legacy" functionality".
> >
> > What I would like to know is why selftests are copy-pasting the kernel's scheme
> > for marshalling data to/from the registers used by TDCALL, how selftests are
> > expected to utilize TDCALL, etc.  I'm confident that if someone actually took the
> > time to write a changelog explaining those details, then what TDCALL "is" will
> > be fairly clear, even if the reader doesn't know exactly what it is.
> >
> > E.g. IMO this is ugly and lazy on multiple fronts:
> 
> To give some context to why this was done this way: Part of the reason
> for the selftests is to test the GHCI protocol itself.

The only part of the protocol that we're actually testing is the guest<=>KVM
interaction.  Testing the guest<=>VMM interaction is self-referential, i.e. we're
validating that we implemented the guest and VMM sides correctly, which is all
kinds of silly.

> Some of the selftests will issue calls with purposely invalid arguments to
> ensure KVM handles these cases properly. For example, issuing a port IO calls
> with sizes other than 1,2 or 4 and ensure we get an error on the guest side.

That's fine, great in fact, but that's completely orthogonal to how selftests
implement the literal guest or VMM code.

> The code was intentionally written to be specific to TDX so we can
> test the TDX GHCI spec itself.

That's 100% possible without copy+pasting the kernel, and also 100% possible
while also providing sane, common interaces.

> As I understand it, you want the selftests to operate at a higher
> level and abstract away the specific GHCI details so that the code can
> be shared between TDX and SEV. 

No, I want us to think critically about what we're actually doing, and I want us
to write code that is maintainable and as easy to follow as possible.

> I can refactor the code to abstract away implementation details. However,
> tests that want to exercise the API at a fine-grained level to test different
> arguments will need to define these TDCALLs themselves.

It's not so much about abstracting details as it is about making it easy to write
tests.  Yes, some TDX-specific tests will need to know the gory details.  But in
the grand scheme, those will be a very tiny percentage of all KVM selftests.

E.g. in all likelihood there should be literally _one_ test to validate that KVM
and the TDX-Module honor the guest<=>KVM GHCI contract.  Or maybe one test per
instruction/operation.  Everything else, even tests that are TDX specific, should
not care one whit about the GHCI.

> These calls were placed in a header that can be included in the guest
> code. I can add higher level wrappers that can be used for common
> code.
> 
> >
> > uint64_t tdg_vp_vmcall_ve_request_mmio_write(uint64_t address, uint64_t size,
> >                                             uint64_t data_in)
> > {
> >        struct tdx_tdcall_args args = {
> >                .r10 = TDG_VP_VMCALL,
> >                .r11 = TDG_VP_VMCALL_VE_REQUEST_MMIO,
> >                .r12 = size,
> >                .r13 = MMIO_WRITE,
> >                .r14 = address,
> >                .r15 = data_in,
> >        };
> >
> >        return __tdx_tdcall(&args, 0);
> > }
> >
> > First, these are KVM selftests, there's no need to provide a super fancy namespace
> > because we are "competing" with thousands upon thousands of lines of code from
> > other components and subsystems.
> >
> > Similarly, tdg_vp_vmcall_ve_request_mmio_write() is absurdly verbose.  Referencing
> > #VE in any way is also flat out wrong.
> 
> This name was taken from the GHCI spec: TDG.VP.VMCALL<#VE.RequestMMIO>
> ("Intel TDX Guest-Hypervisor Communication Interface v1.5" section 3.7)

I know, and I'm saying throw away the GHCI except for when it's absolutely necessary
to care what the GHCI says.

Powered by blists - more mailing lists