On Wed, May 31, 2023 at 7:41 AM Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
>
> 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@xxxxxxxxxx> wrote:
> > > > On Mon, May 29, 2023 at 6:44 AM Maxim Levitsky <mlevitsk@xxxxxxxxxx> wrote:
> > > > > У пн, 2023-05-29 у 14:58 +0200, jwarren@xxxxxxxxxxxx пише:
> > > > > > 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.
It's not quite that simple.
The vmcb12 TLB_CONTROL field needs to be sanitized on its way into the
vmcb02 (perhaps in nested_copy_vmcb_control_to_cache()?). Bits 63:2
should be cleared. Also, if the guest CPUID does not advertise support
for FlushByASID, then bit 1 should be cleared. Note that the vmcb12
TLB_CONTROL field itself must not be modified, since the APM
specifically states, "The VMRUN instruction reads, but does not
change, the value of the TLB_CONTROL field."
