On Fri, Oct 31, 2025, Sagi Shahar wrote:
> On Fri, Oct 31, 2025 at 10:15 AM Sean Christopherson <[email protected]>
> wrote:
> >
> > On Fri, Oct 31, 2025, Ira Weiny wrote:
> > > Sagi Shahar wrote:
> > > > From: Erdem Aktas <[email protected]>
> > > >
> > > > 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.
