Drop "KVM: TDX: Handle TDG.VP.VMCALL<GetTdVmCallInfo> hypercall"

Drop "KVM: TDX: Handle TDG.VP.VMCALL<GetTdVmCallInfo> hypercall"

[Edgecombe, Rick P]

Hi,

We should consider dropping/reverting "KVM: TDX: Handle
TDG.VP.VMCALL<GetTdVmCallInfo> hypercall" from the base TDX merge. The reason is
because Xiaoyao noticed that the GHCI spec[0] implies that KVM should only
return success for that TDVMCALL if *all* TDVMCALLs are supported, but KVM does
that despite skipping implementing a few. On investigation there are also no
users except its selftest[1], and the spec is ambiguous on the right way to
handle the case in question.

The spec talks about VMMs not supporting all TDVMCALLs, but doesn't say how to
enumerate this (i.e. it doesn't says what the GetTdVmCallInfo VMMs should do
instead of succeed). It actually doesn't cover how to handle if the guest calls
an unsupported TDVMCALL either. Historically, KVM patches have returned
TDVMCALL_STATUS_INVALID_OPERAND for any unknown TDVMCALL, as a reasonable
interpretation of the ambiguous spec. So the spec needs to get clarified in this
whole area.

Since there are no real callers let's just drop GetTdVmCallInfo for now. We can
add it back when the GHCI folks amend the spec to close the ambiguities. As a
bonus we can save some code for the merge.

We dropped the patch internally and did some testing. Also, Binbin and I
searched the guest code for any rare callers. Everything seems fine to drop it.

If we want to leave it in, it's probably not a disaster. We'll just slightly
diverge from the spec. It may not be a problem depending on how the ambiguity
resolves in future spec updates.

Thanks,

Rick

[0]
https://www.intel.com/content/www/us/en/content-details/726790/guest-host-communication-interface-ghci-for-intel-trust-domain-extensions-intel-tdx.html
[1]
https://lore.kernel.org/kvm/20250414214801.2693294-14-sagis@google.com/

Re: Drop "KVM: TDX: Handle TDG.VP.VMCALL<GetTdVmCallInfo> hypercall"

[Paolo Bonzini]

On 4/16/25 05:12, Edgecombe, Rick P wrote:
> Hi,
> 
> We should consider dropping/reverting "KVM: TDX: Handle
> TDG.VP.VMCALL<GetTdVmCallInfo> hypercall" from the base TDX merge. The reason is
> because Xiaoyao noticed that the GHCI spec[0] implies that KVM should only
> return success for that TDVMCALL if *all* TDVMCALLs are supported, but KVM does
> that despite skipping implementing a few. On investigation there are also no
> users except its selftest[1], and the spec is ambiguous on the right way to
> handle the case in question.
> 
> The spec talks about VMMs not supporting all TDVMCALLs, but doesn't say how to
> enumerate this (i.e. it doesn't says what the GetTdVmCallInfo VMMs should do
> instead of succeed). It actually doesn't cover how to handle if the guest calls
> an unsupported TDVMCALL either. Historically, KVM patches have returned
> TDVMCALL_STATUS_INVALID_OPERAND for any unknown TDVMCALL, as a reasonable
> interpretation of the ambiguous spec. So the spec needs to get clarified in this
> whole area.

It does, but I think we should just implement the remaining TDVMCALLs 
before 6.16 is out, which is in a while.  All that is left is really the 
userspace TDVMCALLs GetQuote, ReportFatalError and 
SetupEventNotifyInterrupt.

For Instruction.PCONFIG and for VE.RequestMMIO a dummy implementation is 
valid and returning success is probably even better than invalid-operand.

Paolo

> Since there are no real callers let's just drop GetTdVmCallInfo for now. We can
> add it back when the GHCI folks amend the spec to close the ambiguities. As a
> bonus we can save some code for the merge.
> 
> We dropped the patch internally and did some testing. Also, Binbin and I
> searched the guest code for any rare callers. Everything seems fine to drop it.
> 
> If we want to leave it in, it's probably not a disaster. We'll just slightly
> diverge from the spec. It may not be a problem depending on how the ambiguity
> resolves in future spec updates.
> 
> Thanks,
> 
> Rick
> 
> [0]
> https://www.intel.com/content/www/us/en/content-details/726790/guest-host-communication-interface-ghci-for-intel-trust-domain-extensions-intel-tdx.html
> [1]
> https://lore.kernel.org/kvm/20250414214801.2693294-14-sagis@google.com/

Re: Drop "KVM: TDX: Handle TDG.VP.VMCALL<GetTdVmCallInfo> hypercall"

[Edgecombe, Rick P]

+Kirill

Context:
https://lore.kernel.org/kvm/da3e2f6bdc67b1b02d99a6b57ffc9df48a0f4743.camel@intel.com/

On Fri, 2025-04-18 at 15:24 +0200, Paolo Bonzini wrote:
> It does, but I think we should just implement the remaining TDVMCALLs 
> before 6.16 is out, which is in a while.  All that is left is really the 
> userspace TDVMCALLs GetQuote, ReportFatalError and 
> SetupEventNotifyInterrupt.
> 
> For Instruction.PCONFIG and for VE.RequestMMIO a dummy implementation is 
> valid and returning success is probably even better than invalid-operand.

You might be looking at the 1.0 spec. The 1.5 spec has the same GetTdVmCallInfo
success criteria of supporting all calls, but adds a couple more. Here are the
missing calls listed in the 1.5 spec.

TDG.VP.VMCALL<GETQUOTE> - Have patches, but missing
TDG.VP.VMCALL<SETUPEVENTNOTIFYINTERRUPT> - Have patches, but missing
TDG.VP.VMCALL<INSTRUCTION.WBINVD> - Missing
TDG.VP.VMCALL<INSTRUCTION.PCONFIG> - Missing
TDG.VP.VMCALL<Service.Query> - Missing
TDG.VP.VMCALL<Service.MigTD> - Missing

The GHCI also has the following disclaimer:
"This document is a work in progress and is subject to change based on customer
feedback and internal analysis."

So I'm not sure if following the GetTdVmCallInfo spec to the letter is worth the
cost of too much dead code, compared to asking them to update it. How were you
looking at it? In any case we can prep some minimal implementations and see how
it looks.

Xiaoyao was tossing around the idea of adding a dedicated "not implemented"
return code too. It could make it simpler to evolve the GHCI spec vs the all or
nothing approach. To me, the main finding here is that we need to have more
clarity on how the GHCI will evolve going forward.

Re: Drop "KVM: TDX: Handle TDG.VP.VMCALL<GetTdVmCallInfo> hypercall"

[Paolo Bonzini]

On Sat, Apr 19, 2025 at 12:16 AM Edgecombe, Rick P
<rick.p.edgecombe@intel.com> wrote:
> TDG.VP.VMCALL<INSTRUCTION.WBINVD> - Missing
> TDG.VP.VMCALL<INSTRUCTION.PCONFIG> - Missing

WBINVD and PCONFIG need to be implemented (PCONFIG can be a stub).

> TDG.VP.VMCALL<Service.Query> - Missing
> TDG.VP.VMCALL<Service.MigTD> - Missing

Service needs to be implemented and return Unsupported (0xFFFFFFFE)
for all services.

> TDG.VP.VMCALL<GETQUOTE> - Have patches, but missing
> TDG.VP.VMCALL<SETUPEVENTNOTIFYINTERRUPT> - Have patches, but missing

These two need to be supported by userspace and one could say that
(therefore) GetTdVmCallInfo would also have to be implemented by
userspace. This probably is a good idea in general to leave the door
open to more GetTdVmCallInfo leaves.

In order to make it easy for userspace to implement GetQuote, it would
be nice to have a status for Unsupported
listed for GetQuote, because they need to add it anyway for future tdvcalls

SetupEventNotifyInterrupt can be a stub if GetQuote is unsupported;
therefore it's also trivial for userspace to implement it if the specs
adds the "unsupported" return code for GetQuote.

> Xiaoyao was tossing around the idea of adding a dedicated "not implemented"
> return code too. It could make it simpler to evolve the GHCI spec vs the all or
> nothing approach. To me, the main finding here is that we need to have more
> clarity on how the GHCI will evolve going forward.

I agree that both of these are independently useful, the main action
item for KVM being to move TdVmCallInfo to userspace and add support
for the other two userspace TDCALLs.

Adding WBINVD/PCONFIG/Service is also something that has to be done,
but less urgent since nobody is using them.


Paolo

Re: Drop "KVM: TDX: Handle TDG.VP.VMCALL<GetTdVmCallInfo> hypercall"

[Shutemov, Kirill]

On Wed, Apr 23, 2025 at 04:09:50PM +0200, Paolo Bonzini wrote:
> On Sat, Apr 19, 2025 at 12:16 AM Edgecombe, Rick P
> <rick.p.edgecombe@intel.com> wrote:
> > TDG.VP.VMCALL<INSTRUCTION.WBINVD> - Missing
> > TDG.VP.VMCALL<INSTRUCTION.PCONFIG> - Missing
> 
> WBINVD and PCONFIG need to be implemented (PCONFIG can be a stub).

On the guest side I actively avoided using WBINVD as the only way for VMM
to implement it is to do WBINVD on the host side which is too disruptive
for the system. It is possible way to DoS the host.

Do we really want to implement it on KVM side? It is good incentive for
guests to avoid WBINVD.

Hm. Maybe we would need it for partitioning scenario where L2 guest
doesn't care if it runs under TDX and uses WBINVD.

It would be neat to have per-KeyID WBINVD, but HW cannot do this.

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

Re: Drop "KVM: TDX: Handle TDG.VP.VMCALL<GetTdVmCallInfo> hypercall"

[Paolo Bonzini]

On Thu, Apr 24, 2025 at 8:52 AM Shutemov, Kirill
<kirill.shutemov@intel.com> wrote:
>
> On Wed, Apr 23, 2025 at 04:09:50PM +0200, Paolo Bonzini wrote:
> > On Sat, Apr 19, 2025 at 12:16 AM Edgecombe, Rick P
> > <rick.p.edgecombe@intel.com> wrote:
> > > TDG.VP.VMCALL<INSTRUCTION.WBINVD> - Missing
> > > TDG.VP.VMCALL<INSTRUCTION.PCONFIG> - Missing
> >
> > WBINVD and PCONFIG need to be implemented (PCONFIG can be a stub).
>
> On the guest side I actively avoided using WBINVD as the only way for VMM
> to implement it is to do WBINVD on the host side which is too disruptive
> for the system. It is possible way to DoS the host.
>
> Do we really want to implement it on KVM side? It is good incentive for
> guests to avoid WBINVD.

KVM does not do anything for WBINVD unless you have assigned devices
with non-coherent DMA, so in practice WBINVD would basically be a nop.

Paolo

Re: Drop "KVM: TDX: Handle TDG.VP.VMCALL<GetTdVmCallInfo> hypercall"

[Binbin Wu]

On 4/23/2025 10:09 PM, Paolo Bonzini wrote:
> On Sat, Apr 19, 2025 at 12:16 AM Edgecombe, Rick P
> <rick.p.edgecombe@intel.com> wrote:
>> TDG.VP.VMCALL<INSTRUCTION.WBINVD> - Missing
>> TDG.VP.VMCALL<INSTRUCTION.PCONFIG> - Missing
> WBINVD and PCONFIG need to be implemented (PCONFIG can be a stub).
>
>> TDG.VP.VMCALL<Service.Query> - Missing
>> TDG.VP.VMCALL<Service.MigTD> - Missing
> Service needs to be implemented and return Unsupported (0xFFFFFFFE)
> for all services.
>
>> TDG.VP.VMCALL<GETQUOTE> - Have patches, but missing
>> TDG.VP.VMCALL<SETUPEVENTNOTIFYINTERRUPT> - Have patches, but missing
> These two need to be supported by userspace and one could say that
> (therefore) GetTdVmCallInfo would also have to be implemented by
> userspace. This probably is a good idea in general to leave the door
> open to more GetTdVmCallInfo leaves.
>
> In order to make it easy for userspace to implement GetQuote, it would
> be nice to have a status for Unsupported
> listed for GetQuote, because they need to add it anyway for future tdvcalls
>
> SetupEventNotifyInterrupt can be a stub if GetQuote is unsupported;
> therefore it's also trivial for userspace to implement it if the specs
> adds the "unsupported" return code for GetQuote.

IIUC, there are two things:
1. Add a "unsupported" status code to GHCI spec and list "unsupported" as the
    possible return code for GetQuote.
2. Userspace still needs handle the exit reason due to GetQuote and
    SetupEventNotifyInterrupt. But Userspace has two choices:
    - To have a full implementation for GetQuote, and also a full implementation
      for SetupEventNotifyInterrupt.
    - Just return "unsupported" for GetQuote, and the handling for
      SetupEventNotifyInterrupt can be a stub.

It is correct?

>
>> Xiaoyao was tossing around the idea of adding a dedicated "not implemented"
>> return code too. It could make it simpler to evolve the GHCI spec vs the all or
>> nothing approach. To me, the main finding here is that we need to have more
>> clarity on how the GHCI will evolve going forward.
> I agree that both of these are independently useful, the main action
> item for KVM being to move TdVmCallInfo to userspace

Implementing GetTdVmCallInfo in userspace is helpful only when there is GHCI
spec update, i.e., userspace is able to return the bitmaps for supported
TDVMCALLs, right?

Before the GHCI is changed, can we just leave the implementation of
GetTdVmCallInfo as it is or we want the userspace to implement GetTdVmCallInfo
now?


>   and add support
> for the other two userspace TDCALLs.
>
> Adding WBINVD/PCONFIG/Service is also something that has to be done,
> but less urgent since nobody is using them.
>
>
> Paolo
>
>