У вт, 2023-08-01 у 10:49 +0200, jwarren@xxxxxxxxxxxx пише:
> Hi
> Are there any thoughts on this?
>
> PS As I see it, that commit didn't fix any real problem (up to 5.15 nothing was broken that required it), but did break nested VMWare Workstation/ESXi for people that use it (yes, it's vmware's code bug, but doubt they will fix it).
>
>
> May 31, 2023, 15:34 by jmattson@xxxxxxxxxx:
>
> > 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."
> >
I will prepare a patch for this hopefully this week.
Jim mentioned that a revert is not full solution for this.
Best regards,
Maxim Levitsky
