Re: [PATCH v3 06/13] KVM/nVMX: Use kvm_vcpu_map when mapping the L1 MSR bitmap

["Raslan, KarimAllah" <karahmed@amazon.de>]

On Mon, 2018-10-22 at 14:42 -0700, Jim Mattson wrote:
> On Sat, Oct 20, 2018 at 3:22 PM, KarimAllah Ahmed <karahmed@amazon.de> wrote:
> > 
> > Use kvm_vcpu_map when mapping the L1 MSR bitmap since using
> > kvm_vcpu_gpa_to_page() and kmap() will only work for guest memory that has
> > a "struct page".
> > 
> > Signed-off-by: KarimAllah Ahmed <karahmed@amazon.de>
> > ---
> > v1 -> v2:
> > - Do not change the lifecycle of the mapping (pbonzini)
> > ---
> >  arch/x86/kvm/vmx.c | 14 ++++++++------
> >  1 file changed, 8 insertions(+), 6 deletions(-)
> > 
> > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> > index d857401..5b15ca2 100644
> > --- a/arch/x86/kvm/vmx.c
> > +++ b/arch/x86/kvm/vmx.c
> > @@ -847,6 +847,9 @@ struct nested_vmx {
> >         struct page *apic_access_page;
> >         struct page *virtual_apic_page;
> >         struct page *pi_desc_page;
> > +
> > +       struct kvm_host_map msr_bitmap_map;
> > +
> >         struct pi_desc *pi_desc;
> >         bool pi_pending;
> >         u16 posted_intr_nv;
> > @@ -11546,9 +11549,10 @@ static inline bool nested_vmx_prepare_msr_bitmap(struct kvm_vcpu *vcpu,
> >                                                  struct vmcs12 *vmcs12)
> >  {
> >         int msr;
> > -       struct page *page;
> >         unsigned long *msr_bitmap_l1;
> >         unsigned long *msr_bitmap_l0 = to_vmx(vcpu)->nested.vmcs02.msr_bitmap;
> > +       struct kvm_host_map *map = &to_vmx(vcpu)->nested.msr_bitmap_map;
> > +
> >         /*
> >          * pred_cmd & spec_ctrl are trying to verify two things:
> >          *
> > @@ -11574,11 +11578,10 @@ static inline bool nested_vmx_prepare_msr_bitmap(struct kvm_vcpu *vcpu,
> >             !pred_cmd && !spec_ctrl)
> >                 return false;
> > 
> > -       page = kvm_vcpu_gpa_to_page(vcpu, vmcs12->msr_bitmap);
> > -       if (is_error_page(page))
> > +       if (kvm_vcpu_map(vcpu, gpa_to_gfn(vmcs12->msr_bitmap), map))
> 
> Isn't this the sort of high frequency operation that should not use the new API?

With the current implementation of the API, yes. The performance will be 
horrible. This does not affect the current users though (i.e. when guest memory 
is backed by "struct page").

I have a few patches that implements a pfn_cache on top of this as suggested by 
Paolo. This would allow this API to be used for this type of high-frequency 
mappings.

For example with this pfn_cache, booting an Ubuntu was 10x faster (from ~ 2 
minutes to 13 seconds).

> 
> > 
> >                 return false;
> > 
> > -       msr_bitmap_l1 = (unsigned long *)kmap(page);
> > +       msr_bitmap_l1 = (unsigned long *)map->hva;
> >         if (nested_cpu_has_apic_reg_virt(vmcs12)) {
> >                 /*
> >                  * L0 need not intercept reads for MSRs between 0x800 and 0x8ff, it
> > @@ -11626,8 +11629,7 @@ static inline bool nested_vmx_prepare_msr_bitmap(struct kvm_vcpu *vcpu,
> >                                         MSR_IA32_PRED_CMD,
> >                                         MSR_TYPE_W);
> > 
> > -       kunmap(page);
> > -       kvm_release_page_clean(page);
> > +       kvm_vcpu_unmap(&to_vmx(vcpu)->nested.msr_bitmap_map);
> > 
> >         return true;
> >  }
> > --
> > 2.7.4
> > 
> 
Amazon Development Center Germany GmbH
Berlin - Dresden - Aachen
main office: Krausenstr. 38, 10117 Berlin
Geschaeftsfuehrer: Dr. Ralf Herbrich, Christian Schlaeger
Ust-ID: DE289237879
Eingetragen am Amtsgericht Charlottenburg HRB 149173 B