Re: [Bug] AMD nested: commit broke VMware

[jwarren@tutanota.com]

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@google.com:

> On Wed, May 31, 2023 at 7:41 AM Sean Christopherson <seanjc@google.com> 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@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.
>>
>
> 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."
>