[v3,06/13] KVM/nVMX: Use kvm_vcpu_map when mapping the L1 MSR bitmap
diff mbox series

Message ID 1540074145-31285-7-git-send-email-karahmed@amazon.de
State New
Headers show
Series
  • KVM/X86: Introduce a new guest mapping interface
Related show

Commit Message

KarimAllah Ahmed Oct. 20, 2018, 10:22 p.m. UTC
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(-)

Comments

Jim Mattson Oct. 22, 2018, 9:42 p.m. UTC | #1
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?

>                 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
>
KarimAllah Ahmed Oct. 22, 2018, 9:49 p.m. UTC | #2
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

Patch
diff mbox series

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))
 		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;
 }