Re: [Bug] AMD nested: commit broke VMware

[Sean Christopherson <seanjc@google.com>]

On Wed, May 31, 2023, Maxim Levitsky wrote:
> У вт, 2023-05-30 у 13:34 -0700, Jim Mattson пише:
> > On Tue, May 30, 2023 at 1:10 PM Jim Mattson <jmattson@google.com> wrote:
> > > On Mon, May 29, 2023 at 6:44 AM Maxim Levitsky <mlevitsk@redhat.com> wrote:
> > > > У пн, 2023-05-29 у 14:58 +0200, jwarren@tutanota.com пише:
> > > > > I don't know what would be the best case here, maybe put a quirk
> > > > > there, so it doesn't break "userspace".  Committer's email is dead,
> > > > > so I'm writing here.
> > > > > 
> > > > 
> > > > I have to say that I know about this for long time, because some time
> > > > ago I used  to play with VMware player in a VM on AMD on my spare time,
> > > > on weekends (just doing various crazy things with double nesting,
> > > > running win98 nested, vfio stuff, etc, etc).
> > > > 
> > > > I didn't report it because its a bug in VMWARE - they set a bit in the
> > > > tlb_control without checking CPUID's FLUSHBYASID which states that KVM
> > > > doesn't support setting this bit.
> > > 
> > > I am pretty sure that bit 1 is supposed to be ignored on hardware
> > > without FlushByASID, but I'll have to see if I can dig up an old APM
> > > to verify that.
> > 
> > I couldn't find an APM that old, but even today's APM does not specify
> > that any checks are performed on the TLB_CONTROL field by VMRUN.
> > 
> > While Intel likes to fail VM-entry for illegal VMCS state, AMD prefers
> > to massage the VMCB to render any illegal VMCB state legal. For
> > example, rather than fail VM-entry for a non-canonical address, AMD is
> > inclined to drop the high bits and sign-extend the low bits, so that
> > the address is canonical.
> > 
> > I'm willing to bet that modern CPUs continue to ignore the TLB_CONTROL
> > bits that were noted "reserved" in version 3.22 of the manual, and
> > that Krish simply manufactured the checks in commit 174a921b6975
> > ("nSVM: Check for reserved encodings of TLB_CONTROL in nested VMCB"),
> > without cause.

Ya.  The APM even provides a definition of "reserved" that explicitly calls out
the different reserved qualifiers.  The only fields/values that KVM can actively
enforce are things tagged MBZ.

  reserved
    Fields marked as reserved may be used at some future time.
    To preserve compatibility with future processors, reserved fields require special handling when
    read or written by software. Software must not depend on the state of a reserved field (unless
    qualified as RAZ), nor upon the ability of such fields to return a previously written state.
    If a field is marked reserved without qualification, software must not change the state of that field;
    it must reload that field with the same value returned from a prior read.
    Reserved fields may be qualified as IGN, MBZ, RAZ, or SBZ (see definitions).

> > > > Supporting FLUSHBYASID would fix this, and make nesting faster too,
> > > > but it is far from a trivial job.
> > > > 
> > > > I hope that I will find time to do this soon.

...

> Shall we revert the offending patch then?

Yes please.