Re: [PATCH 1/2] KVM: x86: Add emulation support for #GP triggered by VM instructions

[Wei Huang]

On 1/12/21 11:56 AM, Sean Christopherson wrote:
> On Tue, Jan 12, 2021, Andy Lutomirski wrote:
> > > On Jan 12, 2021, at 7:46 AM, Bandan Das <bsd@xxxxxxxxxx> wrote:
> > >
> > > Andy Lutomirski <luto@xxxxxxxxxxxxxx> writes:
> > > ...
> > > > > > > > #endif diff --git a/arch/x86/kvm/mmu/mmu.c
> > > > > > > > b/arch/x86/kvm/mmu/mmu.c index 6d16481aa29d..c5c4aaf01a1a 100644
> > > > > > > > --- a/arch/x86/kvm/mmu/mmu.c +++ b/arch/x86/kvm/mmu/mmu.c @@
> > > > > > > > -50,6 +50,7 @@ #include <asm/io.h> #include <asm/vmx.h> #include
> > > > > > > > <asm/kvm_page_track.h> +#include <asm/e820/api.h> #include
> > > > > > > > "trace.h"
> > > > > > > >
> > > > > > > > extern bool itlb_multihit_kvm_mitigation; @@ -5675,6 +5676,12 @@
> > > > > > > > void kvm_mmu_slot_set_dirty(struct kvm *kvm, }
> > > > > > > > EXPORT_SYMBOL_GPL(kvm_mmu_slot_set_dirty);
> > > > > > > >
> > > > > > > > +bool kvm_is_host_reserved_region(u64 gpa) +{ + return
> > > > > > > > e820__mbapped_raw_any(gpa-1, gpa+1, E820_TYPE_RESERVED); +}
> > > > > > > While _e820__mapped_any()'s doc says '..  checks if any part of
> > > > > > > the range <start,end> is mapped ..' it seems to me that the real
> > > > > > > check is [start, end) so we should use 'gpa' instead of 'gpa-1',
> > > > > > > no?
> > > > > > Why do you need to check GPA at all?
> > > > > To reduce the scope of the workaround.
> > > > >
> > > > > The errata only happens when you use one of SVM instructions in the
> > > > > guest with EAX that happens to be inside one of the host reserved
> > > > > memory regions (for example SMM).
> > > >
> > > > This code reduces the scope of the workaround at the cost of
> > > > increasing the complexity of the workaround and adding a nonsensical
> > > > coupling between KVM and host details and adding an export that really
> > > > doesn’t deserve to be exported.
> > > >
> > > > Is there an actual concrete benefit to this check?
> > >
> > > Besides reducing the scope, my intention for the check was that we should
> > > know if such exceptions occur for any other undiscovered reasons with other
> > > memory types rather than hiding them under this workaround.
> >
> > Ask AMD?

There are several checking before VMRUN launch. The function, 
e820__mapped_raw_any(), was definitely one of the easies way to figure 
out the problematic regions we had.

> > I would also believe that someone somewhere has a firmware that simply omits
> > the problematic region instead of listing it as reserved.
>
> I agree with Andy, odds are very good that attempting to be precise will lead to
> pain due to false negatives.
>
> And, KVM's SVM instruction emulation needs to be be rock solid regardless of
> this behavior since KVM unconditionally intercepts the instruction, i.e. there's
> basically zero risk to KVM.

Are you saying that the instruction decode before 
kvm_is_host_reserved_region() already guarantee the instructions #GP hit 
are SVM execution instructions (see below)? If so, I think this argument 
is fair.

+	switch (ctxt->modrm) {
+	case 0xd8: /* VMRUN */
+	case 0xda: /* VMLOAD */
+	case 0xdb: /* VMSAVE */

Bandan: What is your thoughts about removing kvm_is_host_reserved_region()?