Re: [PATCH 2/5] nSVM: Check for optional commands and reserved encodings of TLB_CONTROL in nested VMCB

From: Sean Christopherson <seanjc@google.com>
To: Stefan Sterz <s.sterz@proxmox.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>,
	Krish Sadhukhan <krish.sadhukhan@oracle.com>,
	kvm@vger.kernel.org, jmattson@google.com, vkuznets@redhat.com,
	wanpengli@tencent.com, joro@8bytes.org
Subject: Re: [PATCH 2/5] nSVM: Check for optional commands and reserved encodings of TLB_CONTROL in nested VMCB
Date: Wed, 6 Sep 2023 13:40:43 -0700	[thread overview]
Message-ID: <ZPjjy94x2BDIitOo@google.com> (raw)
In-Reply-To: <b9915c9c-4cf6-051a-2d91-44cc6380f455@proxmox.com>

On Wed, Sep 06, 2023, Stefan Sterz wrote:
> On 28.09.21 18:55, Paolo Bonzini wrote:
> > On 21/09/21 01:51, Krish Sadhukhan wrote:
> >> +{
> >> +    switch(tlb_ctl) {
> >> +        case TLB_CONTROL_DO_NOTHING:
> >> +        case TLB_CONTROL_FLUSH_ALL_ASID:
> >> +            return true;
> >> +        case TLB_CONTROL_FLUSH_ASID:
> >> +        case TLB_CONTROL_FLUSH_ASID_LOCAL:
> >> +            if (guest_cpuid_has(vcpu, X86_FEATURE_FLUSHBYASID))
> >> +                return true;
> >> +            fallthrough;
> >
> > Since nested FLUSHBYASID is not supported yet, this second set of case
> > labels can go away.
> >
> > Queued with that change, thanks.
> >
> > Paolo
> >
> 
> Are there any plans to support FLUSHBYASID in the future? It seems
> VMWare Workstation and ESXi require this feature to run on top of KVM
> [1]. This means that after the introduction of this check these VMs fail
> to boot and report missing features. Hence, upgrading to a newer kernel
> version is not an option for some users.

IIUC, there's two different issues.  KVM "broke" Workstation 16 by adding a bogus
consistency check.  And Workstation 17 managed to make things worse by trying to
do the right thing (assert that a feature is supported instead of blindly using it).

I say the above consistency check is bogus because I can't find anything in the APM
that states that TLB_CONTROL is actually checked.  Unless something is called out
in "Canonicalization and Consistency Checks" or listed as MBZ (Must Be Zero), AMD
behavior is typically to let software shoot itself in the foot.

So unless I'm missing something, commit 174a921b6975 ("nSVM: Check for reserved
encodings of TLB_CONTROL in nested VMCB") should be reverted.

However, that doesn't help Workstation 17.  On the other hand, I don't see how
Workstation 17 could ever have worked on KVM, since KVM has never advertised
FLUSHBYASID to L1.

That said, "supporting" FLUSHBYASID is trivial, KVM just needs to advertise the
bit to userspace.  That'll work because KVM's TLB flush "handling" for nSVM is
to just flush everything on every transition (it's been a TODO for a long, long
time).

> Sorry if I misunderstood something or if this is not the right place to ask.
> 
> [1]: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/2008583