Re: [PATCH v3 06/13] KVM/nVMX: Use kvm_vcpu_map when mapping the L1 MSR bitmap
On Mon, 2018-10-22 at 14:42 -0700, Jim Mattson wrote:
> On Sat, Oct 20, 2018 at 3:22 PM, KarimAllah Ahmed 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
> > ---
> > 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
Re: [PATCH v3 06/13] KVM/nVMX: Use kvm_vcpu_map when mapping the L1 MSR bitmap
On Sat, Oct 20, 2018 at 3:22 PM, KarimAllah Ahmed 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
> ---
> 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
>
[PATCH v3 06/13] KVM/nVMX: Use kvm_vcpu_map when mapping the L1 MSR bitmap
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
---
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))
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
