[PATCH v2 0/3] KVM: VMX: refactor global page-sized bitmaps

[PATCH v2 0/3] KVM: VMX: refactor global page-sized bitmaps

[Radim Krčmář]

v1: http://www.spinics.net/lists/kvm/msg138118.html

v2:
- split into multiple patches [Paolo]
- add macros for bitmap definitions [Paolo]


Radim Krčmář (3):
  KVM: VMX: remove functions that enable msr intercepts
  KVM: VMX: join functions that disable x2apic msr intercepts
  KVM: VMX: refactor setup of global page-sized bitmaps

 arch/x86/kvm/vmx.c | 213 +++++++++++++----------------------------------------
 1 file changed, 52 insertions(+), 161 deletions(-)

-- 
2.10.0

[PATCH v2 1/3] KVM: VMX: remove functions that enable msr intercepts

[Radim Krčmář]

All intercepts are enabled at the beginning, so they can only be used if
we disabled an intercept that we wanted to have enabled.
This was done for TMCCT to simplify a loop that disables all x2APIC MSR
intercepts, but just keeping TMCCT enabled yields better results.

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 arch/x86/kvm/vmx.c | 57 ++++--------------------------------------------------
 1 file changed, 4 insertions(+), 53 deletions(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index c517c84f48d5..7ea77f0731f4 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -4601,41 +4601,6 @@ static void __vmx_disable_intercept_for_msr(unsigned long *msr_bitmap,
 	}
 }
 
-static void __vmx_enable_intercept_for_msr(unsigned long *msr_bitmap,
-						u32 msr, int type)
-{
-	int f = sizeof(unsigned long);
-
-	if (!cpu_has_vmx_msr_bitmap())
-		return;
-
-	/*
-	 * See Intel PRM Vol. 3, 20.6.9 (MSR-Bitmap Address). Early manuals
-	 * have the write-low and read-high bitmap offsets the wrong way round.
-	 * We can control MSRs 0x00000000-0x00001fff and 0xc0000000-0xc0001fff.
-	 */
-	if (msr <= 0x1fff) {
-		if (type & MSR_TYPE_R)
-			/* read-low */
-			__set_bit(msr, msr_bitmap + 0x000 / f);
-
-		if (type & MSR_TYPE_W)
-			/* write-low */
-			__set_bit(msr, msr_bitmap + 0x800 / f);
-
-	} else if ((msr >= 0xc0000000) && (msr <= 0xc0001fff)) {
-		msr &= 0x1fff;
-		if (type & MSR_TYPE_R)
-			/* read-high */
-			__set_bit(msr, msr_bitmap + 0x400 / f);
-
-		if (type & MSR_TYPE_W)
-			/* write-high */
-			__set_bit(msr, msr_bitmap + 0xc00 / f);
-
-	}
-}
-
 /*
  * If a msr is allowed by L0, we should check whether it is allowed by L1.
  * The corresponding bit will be cleared unless both of L0 and L1 allow it.
@@ -4691,21 +4656,6 @@ static void vmx_disable_intercept_for_msr(u32 msr, bool longmode_only)
 						msr, MSR_TYPE_R | MSR_TYPE_W);
 }
 
-static void vmx_enable_intercept_msr_read_x2apic(u32 msr, bool apicv_active)
-{
-	if (apicv_active) {
-		__vmx_enable_intercept_for_msr(vmx_msr_bitmap_legacy_x2apic_apicv,
-				msr, MSR_TYPE_R);
-		__vmx_enable_intercept_for_msr(vmx_msr_bitmap_longmode_x2apic_apicv,
-				msr, MSR_TYPE_R);
-	} else {
-		__vmx_enable_intercept_for_msr(vmx_msr_bitmap_legacy_x2apic,
-				msr, MSR_TYPE_R);
-		__vmx_enable_intercept_for_msr(vmx_msr_bitmap_longmode_x2apic,
-				msr, MSR_TYPE_R);
-	}
-}
-
 static void vmx_disable_intercept_msr_read_x2apic(u32 msr, bool apicv_active)
 {
 	if (apicv_active) {
@@ -6516,11 +6466,12 @@ static __init int hardware_setup(void)
 	/*
 	 * enable_apicv && kvm_vcpu_apicv_active()
 	 */
-	for (msr = 0x800; msr <= 0x8ff; msr++)
+	for (msr = 0x800; msr <= 0x8ff; msr++) {
+		if (msr == 0x839 /* TMCCT */)
+			continue;
 		vmx_disable_intercept_msr_read_x2apic(msr, true);
+	}
 
-	/* TMCCT */
-	vmx_enable_intercept_msr_read_x2apic(0x839, true);
 	/* TPR */
 	vmx_disable_intercept_msr_write_x2apic(0x808, true);
 	/* EOI */
-- 
2.10.0

[PATCH v2 2/3] KVM: VMX: join functions that disable x2apic msr intercepts

[Radim Krčmář]

vmx_disable_intercept_msr_read_x2apic() and
vmx_disable_intercept_msr_write_x2apic() differed only in the type.
Pass the type to a new function.

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 arch/x86/kvm/vmx.c | 36 ++++++++++--------------------------
 1 file changed, 10 insertions(+), 26 deletions(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 7ea77f0731f4..159cc65755ec 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -4656,33 +4656,18 @@ static void vmx_disable_intercept_for_msr(u32 msr, bool longmode_only)
 						msr, MSR_TYPE_R | MSR_TYPE_W);
 }
 
-static void vmx_disable_intercept_msr_read_x2apic(u32 msr, bool apicv_active)
+static void vmx_disable_intercept_msr_x2apic(u32 msr, int type, bool apicv_active)
 {
 	if (apicv_active) {
 		__vmx_disable_intercept_for_msr(vmx_msr_bitmap_legacy_x2apic_apicv,
-				msr, MSR_TYPE_R);
+				msr, type);
 		__vmx_disable_intercept_for_msr(vmx_msr_bitmap_longmode_x2apic_apicv,
-				msr, MSR_TYPE_R);
+				msr, type);
 	} else {
 		__vmx_disable_intercept_for_msr(vmx_msr_bitmap_legacy_x2apic,
-				msr, MSR_TYPE_R);
+				msr, type);
 		__vmx_disable_intercept_for_msr(vmx_msr_bitmap_longmode_x2apic,
-				msr, MSR_TYPE_R);
-	}
-}
-
-static void vmx_disable_intercept_msr_write_x2apic(u32 msr, bool apicv_active)
-{
-	if (apicv_active) {
-		__vmx_disable_intercept_for_msr(vmx_msr_bitmap_legacy_x2apic_apicv,
-				msr, MSR_TYPE_W);
-		__vmx_disable_intercept_for_msr(vmx_msr_bitmap_longmode_x2apic_apicv,
-				msr, MSR_TYPE_W);
-	} else {
-		__vmx_disable_intercept_for_msr(vmx_msr_bitmap_legacy_x2apic,
-				msr, MSR_TYPE_W);
-		__vmx_disable_intercept_for_msr(vmx_msr_bitmap_longmode_x2apic,
-				msr, MSR_TYPE_W);
+				msr, type);
 	}
 }
 
@@ -6469,23 +6454,22 @@ static __init int hardware_setup(void)
 	for (msr = 0x800; msr <= 0x8ff; msr++) {
 		if (msr == 0x839 /* TMCCT */)
 			continue;
-		vmx_disable_intercept_msr_read_x2apic(msr, true);
+		vmx_disable_intercept_msr_x2apic(msr, MSR_TYPE_R, true);
 	}
 
 	/* TPR */
-	vmx_disable_intercept_msr_write_x2apic(0x808, true);
+	vmx_disable_intercept_msr_x2apic(0x808, MSR_TYPE_W, true);
 	/* EOI */
-	vmx_disable_intercept_msr_write_x2apic(0x80b, true);
+	vmx_disable_intercept_msr_x2apic(0x80b, MSR_TYPE_W, true);
 	/* SELF-IPI */
-	vmx_disable_intercept_msr_write_x2apic(0x83f, true);
+	vmx_disable_intercept_msr_x2apic(0x83f, MSR_TYPE_W, true);
 
 	/*
 	 * (enable_apicv && !kvm_vcpu_apicv_active()) ||
 	 * 	!enable_apicv
 	 */
 	/* TPR */
-	vmx_disable_intercept_msr_read_x2apic(0x808, false);
-	vmx_disable_intercept_msr_write_x2apic(0x808, false);
+	vmx_disable_intercept_msr_x2apic(0x808, MSR_TYPE_R | MSR_TYPE_W, false);
 
 	if (enable_ept) {
 		kvm_mmu_set_mask_ptes(VMX_EPT_READABLE_MASK,
-- 
2.10.0

[PATCH v2 3/3] KVM: VMX: refactor setup of global page-sized bitmaps

[Radim Krčmář]

We've had 10 page-sized bitmaps that were being allocated and freed one
by one when we could just use a cycle.

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 arch/x86/kvm/vmx.c | 120 +++++++++++++++++------------------------------------
 1 file changed, 38 insertions(+), 82 deletions(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 159cc65755ec..bfd41149c377 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -921,16 +921,32 @@ static DEFINE_PER_CPU(struct desc_ptr, host_gdt);
 static DEFINE_PER_CPU(struct list_head, blocked_vcpu_on_cpu);
 static DEFINE_PER_CPU(spinlock_t, blocked_vcpu_on_cpu_lock);
 
-static unsigned long *vmx_io_bitmap_a;
-static unsigned long *vmx_io_bitmap_b;
-static unsigned long *vmx_msr_bitmap_legacy;
-static unsigned long *vmx_msr_bitmap_longmode;
-static unsigned long *vmx_msr_bitmap_legacy_x2apic_apicv;
-static unsigned long *vmx_msr_bitmap_longmode_x2apic_apicv;
-static unsigned long *vmx_msr_bitmap_legacy_x2apic;
-static unsigned long *vmx_msr_bitmap_longmode_x2apic;
-static unsigned long *vmx_vmread_bitmap;
-static unsigned long *vmx_vmwrite_bitmap;
+enum {
+	VMX_IO_BITMAP_A,
+	VMX_IO_BITMAP_B,
+	VMX_MSR_BITMAP_LEGACY,
+	VMX_MSR_BITMAP_LONGMODE,
+	VMX_MSR_BITMAP_LEGACY_X2APIC_APICV,
+	VMX_MSR_BITMAP_LONGMODE_X2APIC_APICV,
+	VMX_MSR_BITMAP_LEGACY_X2APIC,
+	VMX_MSR_BITMAP_LONGMODE_X2APIC,
+	VMX_VMREAD_BITMAP,
+	VMX_VMWRITE_BITMAP,
+	VMX_BITMAP_NR
+};
+
+static unsigned long *vmx_bitmap[VMX_BITMAP_NR];
+
+#define vmx_io_bitmap_a                      (vmx_bitmap[VMX_IO_BITMAP_A])
+#define vmx_io_bitmap_b                      (vmx_bitmap[VMX_IO_BITMAP_B])
+#define vmx_msr_bitmap_legacy                (vmx_bitmap[VMX_MSR_BITMAP_LEGACY])
+#define vmx_msr_bitmap_longmode              (vmx_bitmap[VMX_MSR_BITMAP_LONGMODE])
+#define vmx_msr_bitmap_legacy_x2apic_apicv   (vmx_bitmap[VMX_MSR_BITMAP_LEGACY_X2APIC_APICV])
+#define vmx_msr_bitmap_longmode_x2apic_apicv (vmx_bitmap[VMX_MSR_BITMAP_LONGMODE_X2APIC_APICV])
+#define vmx_msr_bitmap_legacy_x2apic         (vmx_bitmap[VMX_MSR_BITMAP_LEGACY_X2APIC])
+#define vmx_msr_bitmap_longmode_x2apic       (vmx_bitmap[VMX_MSR_BITMAP_LONGMODE_X2APIC])
+#define vmx_vmread_bitmap                    (vmx_bitmap[VMX_VMREAD_BITMAP])
+#define vmx_vmwrite_bitmap                   (vmx_bitmap[VMX_VMWRITE_BITMAP])
 
 static bool cpu_has_load_ia32_efer;
 static bool cpu_has_load_perf_global_ctrl;
@@ -6313,50 +6329,13 @@ static __init int hardware_setup(void)
 	for (i = 0; i < ARRAY_SIZE(vmx_msr_index); ++i)
 		kvm_define_shared_msr(i, vmx_msr_index[i]);
 
-	vmx_io_bitmap_a = (unsigned long *)__get_free_page(GFP_KERNEL);
-	if (!vmx_io_bitmap_a)
-		return r;
+	for (i = 0; i < VMX_BITMAP_NR; i++) {
+		vmx_bitmap[i] = (unsigned long *)__get_free_page(GFP_KERNEL);
+		if (!vmx_bitmap[i])
+			goto out;
+	}
 
 	vmx_io_bitmap_b = (unsigned long *)__get_free_page(GFP_KERNEL);
-	if (!vmx_io_bitmap_b)
-		goto out;
-
-	vmx_msr_bitmap_legacy = (unsigned long *)__get_free_page(GFP_KERNEL);
-	if (!vmx_msr_bitmap_legacy)
-		goto out1;
-
-	vmx_msr_bitmap_legacy_x2apic_apicv =
-				(unsigned long *)__get_free_page(GFP_KERNEL);
-	if (!vmx_msr_bitmap_legacy_x2apic_apicv)
-		goto out2;
-
-	vmx_msr_bitmap_legacy_x2apic =
-				(unsigned long *)__get_free_page(GFP_KERNEL);
-	if (!vmx_msr_bitmap_legacy_x2apic)
-		goto out3;
-
-	vmx_msr_bitmap_longmode = (unsigned long *)__get_free_page(GFP_KERNEL);
-	if (!vmx_msr_bitmap_longmode)
-		goto out4;
-
-	vmx_msr_bitmap_longmode_x2apic_apicv =
-				(unsigned long *)__get_free_page(GFP_KERNEL);
-	if (!vmx_msr_bitmap_longmode_x2apic_apicv)
-		goto out5;
-
-	vmx_msr_bitmap_longmode_x2apic =
-				(unsigned long *)__get_free_page(GFP_KERNEL);
-	if (!vmx_msr_bitmap_longmode_x2apic)
-		goto out6;
-
-	vmx_vmread_bitmap = (unsigned long *)__get_free_page(GFP_KERNEL);
-	if (!vmx_vmread_bitmap)
-		goto out7;
-
-	vmx_vmwrite_bitmap = (unsigned long *)__get_free_page(GFP_KERNEL);
-	if (!vmx_vmwrite_bitmap)
-		goto out8;
-
 	memset(vmx_vmread_bitmap, 0xff, PAGE_SIZE);
 	memset(vmx_vmwrite_bitmap, 0xff, PAGE_SIZE);
 
@@ -6374,7 +6353,7 @@ static __init int hardware_setup(void)
 
 	if (setup_vmcs_config(&vmcs_config) < 0) {
 		r = -EIO;
-		goto out9;
+		goto out;
 	}
 
 	if (boot_cpu_has(X86_FEATURE_NX))
@@ -6516,42 +6495,19 @@ static __init int hardware_setup(void)
 
 	return alloc_kvm_area();
 
-out9:
-	free_page((unsigned long)vmx_vmwrite_bitmap);
-out8:
-	free_page((unsigned long)vmx_vmread_bitmap);
-out7:
-	free_page((unsigned long)vmx_msr_bitmap_longmode_x2apic);
-out6:
-	free_page((unsigned long)vmx_msr_bitmap_longmode_x2apic_apicv);
-out5:
-	free_page((unsigned long)vmx_msr_bitmap_longmode);
-out4:
-	free_page((unsigned long)vmx_msr_bitmap_legacy_x2apic);
-out3:
-	free_page((unsigned long)vmx_msr_bitmap_legacy_x2apic_apicv);
-out2:
-	free_page((unsigned long)vmx_msr_bitmap_legacy);
-out1:
-	free_page((unsigned long)vmx_io_bitmap_b);
 out:
-	free_page((unsigned long)vmx_io_bitmap_a);
+	for (i = 0; i < VMX_BITMAP_NR; i++)
+		free_page((unsigned long)vmx_bitmap[i]);
 
     return r;
 }
 
 static __exit void hardware_unsetup(void)
 {
-	free_page((unsigned long)vmx_msr_bitmap_legacy_x2apic_apicv);
-	free_page((unsigned long)vmx_msr_bitmap_legacy_x2apic);
-	free_page((unsigned long)vmx_msr_bitmap_longmode_x2apic_apicv);
-	free_page((unsigned long)vmx_msr_bitmap_longmode_x2apic);
-	free_page((unsigned long)vmx_msr_bitmap_legacy);
-	free_page((unsigned long)vmx_msr_bitmap_longmode);
-	free_page((unsigned long)vmx_io_bitmap_b);
-	free_page((unsigned long)vmx_io_bitmap_a);
-	free_page((unsigned long)vmx_vmwrite_bitmap);
-	free_page((unsigned long)vmx_vmread_bitmap);
+	int i;
+
+	for (i = 0; i < VMX_BITMAP_NR; i++)
+		free_page((unsigned long)vmx_bitmap[i]);
 
 	free_kvm_area();
 }
-- 
2.10.0

Re: [PATCH v2 2/3] KVM: VMX: join functions that disable x2apic msr intercepts

[Paolo Bonzini]

On 29/09/2016 22:41, Radim Krčmář wrote:
>  	for (msr = 0x800; msr <= 0x8ff; msr++) {
>  		if (msr == 0x839 /* TMCCT */)
>  			continue;
> -		vmx_disable_intercept_msr_read_x2apic(msr, true);
> +		vmx_disable_intercept_msr_x2apic(msr, MSR_TYPE_R, true);
>  	}
>  
>  	/* TPR */
> -	vmx_disable_intercept_msr_write_x2apic(0x808, true);
> +	vmx_disable_intercept_msr_x2apic(0x808, MSR_TYPE_W, true);
>  	/* EOI */
> -	vmx_disable_intercept_msr_write_x2apic(0x80b, true);
> +	vmx_disable_intercept_msr_x2apic(0x80b, MSR_TYPE_W, true);
>  	/* SELF-IPI */
> -	vmx_disable_intercept_msr_write_x2apic(0x83f, true);
> +	vmx_disable_intercept_msr_x2apic(0x83f, MSR_TYPE_W, true);
>  
>  	/*
>  	 * (enable_apicv && !kvm_vcpu_apicv_active()) ||
>  	 * 	!enable_apicv
>  	 */
>  	/* TPR */
> -	vmx_disable_intercept_msr_read_x2apic(0x808, false);
> -	vmx_disable_intercept_msr_write_x2apic(0x808, false);
> +	vmx_disable_intercept_msr_x2apic(0x808, MSR_TYPE_R | MSR_TYPE_W, false);

Alternatively you could place the two function calls for 0x808 together:

 	for (msr = 0x800; msr <= 0x8ff; msr++)
		vmx_disable_intercept_msr_x2apic(msr, MSR_TYPE_R, true);

 	/*
	 * TPR reads and writes can be virtualized even if virtual interrupt delivery
         * is not in use.
	 */
	vmx_disable_intercept_msr_x2apic(0x808, MSR_TYPE_R | MSR_TYPE_W, false);
	vmx_disable_intercept_msr_x2apic(0x808, MSR_TYPE_W, true);

	/* EOI */
	vmx_disable_intercept_msr_x2apic(0x80b, MSR_TYPE_W, true);

 	/* SELF-IPI */
	vmx_disable_intercept_msr_x2apic(0x83f, MSR_TYPE_W, true);

Paolo

Re: [PATCH v2 3/3] KVM: VMX: refactor setup of global page-sized bitmaps

[Paolo Bonzini]

On 29/09/2016 22:41, Radim Krčmář wrote:
> We've had 10 page-sized bitmaps that were being allocated and freed one
> by one when we could just use a cycle.
> 
> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> ---
>  arch/x86/kvm/vmx.c | 120 +++++++++++++++++------------------------------------
>  1 file changed, 38 insertions(+), 82 deletions(-)

Yes. :)

Paolo

> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 159cc65755ec..bfd41149c377 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -921,16 +921,32 @@ static DEFINE_PER_CPU(struct desc_ptr, host_gdt);
>  static DEFINE_PER_CPU(struct list_head, blocked_vcpu_on_cpu);
>  static DEFINE_PER_CPU(spinlock_t, blocked_vcpu_on_cpu_lock);
>  
> -static unsigned long *vmx_io_bitmap_a;
> -static unsigned long *vmx_io_bitmap_b;
> -static unsigned long *vmx_msr_bitmap_legacy;
> -static unsigned long *vmx_msr_bitmap_longmode;
> -static unsigned long *vmx_msr_bitmap_legacy_x2apic_apicv;
> -static unsigned long *vmx_msr_bitmap_longmode_x2apic_apicv;
> -static unsigned long *vmx_msr_bitmap_legacy_x2apic;
> -static unsigned long *vmx_msr_bitmap_longmode_x2apic;
> -static unsigned long *vmx_vmread_bitmap;
> -static unsigned long *vmx_vmwrite_bitmap;
> +enum {
> +	VMX_IO_BITMAP_A,
> +	VMX_IO_BITMAP_B,
> +	VMX_MSR_BITMAP_LEGACY,
> +	VMX_MSR_BITMAP_LONGMODE,
> +	VMX_MSR_BITMAP_LEGACY_X2APIC_APICV,
> +	VMX_MSR_BITMAP_LONGMODE_X2APIC_APICV,
> +	VMX_MSR_BITMAP_LEGACY_X2APIC,
> +	VMX_MSR_BITMAP_LONGMODE_X2APIC,
> +	VMX_VMREAD_BITMAP,
> +	VMX_VMWRITE_BITMAP,
> +	VMX_BITMAP_NR
> +};
> +
> +static unsigned long *vmx_bitmap[VMX_BITMAP_NR];
> +
> +#define vmx_io_bitmap_a                      (vmx_bitmap[VMX_IO_BITMAP_A])
> +#define vmx_io_bitmap_b                      (vmx_bitmap[VMX_IO_BITMAP_B])
> +#define vmx_msr_bitmap_legacy                (vmx_bitmap[VMX_MSR_BITMAP_LEGACY])
> +#define vmx_msr_bitmap_longmode              (vmx_bitmap[VMX_MSR_BITMAP_LONGMODE])
> +#define vmx_msr_bitmap_legacy_x2apic_apicv   (vmx_bitmap[VMX_MSR_BITMAP_LEGACY_X2APIC_APICV])
> +#define vmx_msr_bitmap_longmode_x2apic_apicv (vmx_bitmap[VMX_MSR_BITMAP_LONGMODE_X2APIC_APICV])
> +#define vmx_msr_bitmap_legacy_x2apic         (vmx_bitmap[VMX_MSR_BITMAP_LEGACY_X2APIC])
> +#define vmx_msr_bitmap_longmode_x2apic       (vmx_bitmap[VMX_MSR_BITMAP_LONGMODE_X2APIC])
> +#define vmx_vmread_bitmap                    (vmx_bitmap[VMX_VMREAD_BITMAP])
> +#define vmx_vmwrite_bitmap                   (vmx_bitmap[VMX_VMWRITE_BITMAP])
>  
>  static bool cpu_has_load_ia32_efer;
>  static bool cpu_has_load_perf_global_ctrl;
> @@ -6313,50 +6329,13 @@ static __init int hardware_setup(void)
>  	for (i = 0; i < ARRAY_SIZE(vmx_msr_index); ++i)
>  		kvm_define_shared_msr(i, vmx_msr_index[i]);
>  
> -	vmx_io_bitmap_a = (unsigned long *)__get_free_page(GFP_KERNEL);
> -	if (!vmx_io_bitmap_a)
> -		return r;
> +	for (i = 0; i < VMX_BITMAP_NR; i++) {
> +		vmx_bitmap[i] = (unsigned long *)__get_free_page(GFP_KERNEL);
> +		if (!vmx_bitmap[i])
> +			goto out;
> +	}
>  
>  	vmx_io_bitmap_b = (unsigned long *)__get_free_page(GFP_KERNEL);
> -	if (!vmx_io_bitmap_b)
> -		goto out;
> -
> -	vmx_msr_bitmap_legacy = (unsigned long *)__get_free_page(GFP_KERNEL);
> -	if (!vmx_msr_bitmap_legacy)
> -		goto out1;
> -
> -	vmx_msr_bitmap_legacy_x2apic_apicv =
> -				(unsigned long *)__get_free_page(GFP_KERNEL);
> -	if (!vmx_msr_bitmap_legacy_x2apic_apicv)
> -		goto out2;
> -
> -	vmx_msr_bitmap_legacy_x2apic =
> -				(unsigned long *)__get_free_page(GFP_KERNEL);
> -	if (!vmx_msr_bitmap_legacy_x2apic)
> -		goto out3;
> -
> -	vmx_msr_bitmap_longmode = (unsigned long *)__get_free_page(GFP_KERNEL);
> -	if (!vmx_msr_bitmap_longmode)
> -		goto out4;
> -
> -	vmx_msr_bitmap_longmode_x2apic_apicv =
> -				(unsigned long *)__get_free_page(GFP_KERNEL);
> -	if (!vmx_msr_bitmap_longmode_x2apic_apicv)
> -		goto out5;
> -
> -	vmx_msr_bitmap_longmode_x2apic =
> -				(unsigned long *)__get_free_page(GFP_KERNEL);
> -	if (!vmx_msr_bitmap_longmode_x2apic)
> -		goto out6;
> -
> -	vmx_vmread_bitmap = (unsigned long *)__get_free_page(GFP_KERNEL);
> -	if (!vmx_vmread_bitmap)
> -		goto out7;
> -
> -	vmx_vmwrite_bitmap = (unsigned long *)__get_free_page(GFP_KERNEL);
> -	if (!vmx_vmwrite_bitmap)
> -		goto out8;
> -
>  	memset(vmx_vmread_bitmap, 0xff, PAGE_SIZE);
>  	memset(vmx_vmwrite_bitmap, 0xff, PAGE_SIZE);
>  
> @@ -6374,7 +6353,7 @@ static __init int hardware_setup(void)
>  
>  	if (setup_vmcs_config(&vmcs_config) < 0) {
>  		r = -EIO;
> -		goto out9;
> +		goto out;
>  	}
>  
>  	if (boot_cpu_has(X86_FEATURE_NX))
> @@ -6516,42 +6495,19 @@ static __init int hardware_setup(void)
>  
>  	return alloc_kvm_area();
>  
> -out9:
> -	free_page((unsigned long)vmx_vmwrite_bitmap);
> -out8:
> -	free_page((unsigned long)vmx_vmread_bitmap);
> -out7:
> -	free_page((unsigned long)vmx_msr_bitmap_longmode_x2apic);
> -out6:
> -	free_page((unsigned long)vmx_msr_bitmap_longmode_x2apic_apicv);
> -out5:
> -	free_page((unsigned long)vmx_msr_bitmap_longmode);
> -out4:
> -	free_page((unsigned long)vmx_msr_bitmap_legacy_x2apic);
> -out3:
> -	free_page((unsigned long)vmx_msr_bitmap_legacy_x2apic_apicv);
> -out2:
> -	free_page((unsigned long)vmx_msr_bitmap_legacy);
> -out1:
> -	free_page((unsigned long)vmx_io_bitmap_b);
>  out:
> -	free_page((unsigned long)vmx_io_bitmap_a);
> +	for (i = 0; i < VMX_BITMAP_NR; i++)
> +		free_page((unsigned long)vmx_bitmap[i]);
>  
>      return r;
>  }
>  
>  static __exit void hardware_unsetup(void)
>  {
> -	free_page((unsigned long)vmx_msr_bitmap_legacy_x2apic_apicv);
> -	free_page((unsigned long)vmx_msr_bitmap_legacy_x2apic);
> -	free_page((unsigned long)vmx_msr_bitmap_longmode_x2apic_apicv);
> -	free_page((unsigned long)vmx_msr_bitmap_longmode_x2apic);
> -	free_page((unsigned long)vmx_msr_bitmap_legacy);
> -	free_page((unsigned long)vmx_msr_bitmap_longmode);
> -	free_page((unsigned long)vmx_io_bitmap_b);
> -	free_page((unsigned long)vmx_io_bitmap_a);
> -	free_page((unsigned long)vmx_vmwrite_bitmap);
> -	free_page((unsigned long)vmx_vmread_bitmap);
> +	int i;
> +
> +	for (i = 0; i < VMX_BITMAP_NR; i++)
> +		free_page((unsigned long)vmx_bitmap[i]);
>  
>  	free_kvm_area();
>  }
> 

Re: [PATCH v2 2/3] KVM: VMX: join functions that disable x2apic msr intercepts

[Radim Krčmář]

2016-09-30 10:29+0200, Paolo Bonzini:
> On 29/09/2016 22:41, Radim Krčmář wrote:
>>  	for (msr = 0x800; msr <= 0x8ff; msr++) {
>>  		if (msr == 0x839 /* TMCCT */)
>>  			continue;
>> -		vmx_disable_intercept_msr_read_x2apic(msr, true);
>> +		vmx_disable_intercept_msr_x2apic(msr, MSR_TYPE_R, true);
>>  	}
>>  
>>  	/* TPR */
>> -	vmx_disable_intercept_msr_write_x2apic(0x808, true);
>> +	vmx_disable_intercept_msr_x2apic(0x808, MSR_TYPE_W, true);
>>  	/* EOI */
>> -	vmx_disable_intercept_msr_write_x2apic(0x80b, true);
>> +	vmx_disable_intercept_msr_x2apic(0x80b, MSR_TYPE_W, true);
>>  	/* SELF-IPI */
>> -	vmx_disable_intercept_msr_write_x2apic(0x83f, true);
>> +	vmx_disable_intercept_msr_x2apic(0x83f, MSR_TYPE_W, true);
>>  
>>  	/*
>>  	 * (enable_apicv && !kvm_vcpu_apicv_active()) ||
>>  	 * 	!enable_apicv
>>  	 */
>>  	/* TPR */
>> -	vmx_disable_intercept_msr_read_x2apic(0x808, false);
>> -	vmx_disable_intercept_msr_write_x2apic(0x808, false);
>> +	vmx_disable_intercept_msr_x2apic(0x808, MSR_TYPE_R | MSR_TYPE_W, false);
> 
> Alternatively you could place the two function calls for 0x808 together:
> 
>  	for (msr = 0x800; msr <= 0x8ff; msr++)
> 		vmx_disable_intercept_msr_x2apic(msr, MSR_TYPE_R, true);
> 
>  	/*
> 	 * TPR reads and writes can be virtualized even if virtual interrupt delivery
>          * is not in use.
> 	 */
> 	vmx_disable_intercept_msr_x2apic(0x808, MSR_TYPE_R | MSR_TYPE_W, false);
> 	vmx_disable_intercept_msr_x2apic(0x808, MSR_TYPE_W, true);
> 
> 	/* EOI */
> 	vmx_disable_intercept_msr_x2apic(0x80b, MSR_TYPE_W, true);
> 
>  	/* SELF-IPI */
> 	vmx_disable_intercept_msr_x2apic(0x83f, MSR_TYPE_W, true);

I have performed this change, thanks, and applied to kvm/queue;
please check the complete patch below:
---8<---
vmx_disable_intercept_msr_read_x2apic() and
vmx_disable_intercept_msr_write_x2apic() differed only in the type.
Pass the type to a new function.

[Ordered and commented TPR intercept according to Paolo's suggestion.]
Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 arch/x86/kvm/vmx.c | 51 +++++++++++++++------------------------------------
 1 file changed, 15 insertions(+), 36 deletions(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 8c83268f6f4c..69ff1be3db7b 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -4656,33 +4656,18 @@ static void vmx_disable_intercept_for_msr(u32 msr, bool longmode_only)
 						msr, MSR_TYPE_R | MSR_TYPE_W);
 }
 
-static void vmx_disable_intercept_msr_read_x2apic(u32 msr, bool apicv_active)
+static void vmx_disable_intercept_msr_x2apic(u32 msr, int type, bool apicv_active)
 {
 	if (apicv_active) {
 		__vmx_disable_intercept_for_msr(vmx_msr_bitmap_legacy_x2apic_apicv,
-				msr, MSR_TYPE_R);
+				msr, type);
 		__vmx_disable_intercept_for_msr(vmx_msr_bitmap_longmode_x2apic_apicv,
-				msr, MSR_TYPE_R);
+				msr, type);
 	} else {
 		__vmx_disable_intercept_for_msr(vmx_msr_bitmap_legacy_x2apic,
-				msr, MSR_TYPE_R);
+				msr, type);
 		__vmx_disable_intercept_for_msr(vmx_msr_bitmap_longmode_x2apic,
-				msr, MSR_TYPE_R);
-	}
-}
-
-static void vmx_disable_intercept_msr_write_x2apic(u32 msr, bool apicv_active)
-{
-	if (apicv_active) {
-		__vmx_disable_intercept_for_msr(vmx_msr_bitmap_legacy_x2apic_apicv,
-				msr, MSR_TYPE_W);
-		__vmx_disable_intercept_for_msr(vmx_msr_bitmap_longmode_x2apic_apicv,
-				msr, MSR_TYPE_W);
-	} else {
-		__vmx_disable_intercept_for_msr(vmx_msr_bitmap_legacy_x2apic,
-				msr, MSR_TYPE_W);
-		__vmx_disable_intercept_for_msr(vmx_msr_bitmap_longmode_x2apic,
-				msr, MSR_TYPE_W);
+				msr, type);
 	}
 }
 
@@ -6463,29 +6448,23 @@ static __init int hardware_setup(void)
 
 	set_bit(0, vmx_vpid_bitmap); /* 0 is reserved for host */
 
-	/*
-	 * enable_apicv && kvm_vcpu_apicv_active()
-	 */
 	for (msr = 0x800; msr <= 0x8ff; msr++) {
 		if (msr == 0x839 /* TMCCT */)
 			continue;
-		vmx_disable_intercept_msr_read_x2apic(msr, true);
+		vmx_disable_intercept_msr_x2apic(msr, MSR_TYPE_R, true);
 	}
 
-	/* TPR */
-	vmx_disable_intercept_msr_write_x2apic(0x808, true);
-	/* EOI */
-	vmx_disable_intercept_msr_write_x2apic(0x80b, true);
-	/* SELF-IPI */
-	vmx_disable_intercept_msr_write_x2apic(0x83f, true);
-
 	/*
-	 * (enable_apicv && !kvm_vcpu_apicv_active()) ||
-	 * 	!enable_apicv
+	 * TPR reads and writes can be virtualized even if virtual interrupt
+	 * delivery is not in use.
 	 */
-	/* TPR */
-	vmx_disable_intercept_msr_read_x2apic(0x808, false);
-	vmx_disable_intercept_msr_write_x2apic(0x808, false);
+	vmx_disable_intercept_msr_x2apic(0x808, MSR_TYPE_W, true);
+	vmx_disable_intercept_msr_x2apic(0x808, MSR_TYPE_R | MSR_TYPE_W, false);
+
+	/* EOI */
+	vmx_disable_intercept_msr_x2apic(0x80b, MSR_TYPE_W, true);
+	/* SELF-IPI */
+	vmx_disable_intercept_msr_x2apic(0x83f, MSR_TYPE_W, true);
 
 	if (enable_ept) {
 		kvm_mmu_set_mask_ptes(VMX_EPT_READABLE_MASK,

Re: [PATCH v2 2/3] KVM: VMX: join functions that disable x2apic msr intercepts

[Wanpeng Li]

2016-10-07 20:30 GMT+08:00 Radim Krčmář <rkrcmar@redhat.com>:
> 2016-09-30 10:29+0200, Paolo Bonzini:
>> On 29/09/2016 22:41, Radim Krčmář wrote:
>>>      for (msr = 0x800; msr <= 0x8ff; msr++) {
>>>              if (msr == 0x839 /* TMCCT */)
>>>                      continue;
>>> -            vmx_disable_intercept_msr_read_x2apic(msr, true);
>>> +            vmx_disable_intercept_msr_x2apic(msr, MSR_TYPE_R, true);
>>>      }
>>>
>>>      /* TPR */
>>> -    vmx_disable_intercept_msr_write_x2apic(0x808, true);
>>> +    vmx_disable_intercept_msr_x2apic(0x808, MSR_TYPE_W, true);
>>>      /* EOI */
>>> -    vmx_disable_intercept_msr_write_x2apic(0x80b, true);
>>> +    vmx_disable_intercept_msr_x2apic(0x80b, MSR_TYPE_W, true);
>>>      /* SELF-IPI */
>>> -    vmx_disable_intercept_msr_write_x2apic(0x83f, true);
>>> +    vmx_disable_intercept_msr_x2apic(0x83f, MSR_TYPE_W, true);
>>>
>>>      /*
>>>       * (enable_apicv && !kvm_vcpu_apicv_active()) ||
>>>       *      !enable_apicv
>>>       */
>>>      /* TPR */
>>> -    vmx_disable_intercept_msr_read_x2apic(0x808, false);
>>> -    vmx_disable_intercept_msr_write_x2apic(0x808, false);
>>> +    vmx_disable_intercept_msr_x2apic(0x808, MSR_TYPE_R | MSR_TYPE_W, false);
>>
>> Alternatively you could place the two function calls for 0x808 together:
>>
>>       for (msr = 0x800; msr <= 0x8ff; msr++)
>>               vmx_disable_intercept_msr_x2apic(msr, MSR_TYPE_R, true);
>>
>>       /*
>>        * TPR reads and writes can be virtualized even if virtual interrupt delivery
>>          * is not in use.
>>        */
>>       vmx_disable_intercept_msr_x2apic(0x808, MSR_TYPE_R | MSR_TYPE_W, false);
>>       vmx_disable_intercept_msr_x2apic(0x808, MSR_TYPE_W, true);
>>
>>       /* EOI */
>>       vmx_disable_intercept_msr_x2apic(0x80b, MSR_TYPE_W, true);
>>
>>       /* SELF-IPI */
>>       vmx_disable_intercept_msr_x2apic(0x83f, MSR_TYPE_W, true);
>
> I have performed this change, thanks, and applied to kvm/queue;
> please check the complete patch below:
> ---8<---
> vmx_disable_intercept_msr_read_x2apic() and
> vmx_disable_intercept_msr_write_x2apic() differed only in the type.
> Pass the type to a new function.
>
> [Ordered and commented TPR intercept according to Paolo's suggestion.]

Looks good to me. :)

Regards,
Wanpeng Li

> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> ---
>  arch/x86/kvm/vmx.c | 51 +++++++++++++++------------------------------------
>  1 file changed, 15 insertions(+), 36 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 8c83268f6f4c..69ff1be3db7b 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -4656,33 +4656,18 @@ static void vmx_disable_intercept_for_msr(u32 msr, bool longmode_only)
>                                                 msr, MSR_TYPE_R | MSR_TYPE_W);
>  }
>
> -static void vmx_disable_intercept_msr_read_x2apic(u32 msr, bool apicv_active)
> +static void vmx_disable_intercept_msr_x2apic(u32 msr, int type, bool apicv_active)
>  {
>         if (apicv_active) {
>                 __vmx_disable_intercept_for_msr(vmx_msr_bitmap_legacy_x2apic_apicv,
> -                               msr, MSR_TYPE_R);
> +                               msr, type);
>                 __vmx_disable_intercept_for_msr(vmx_msr_bitmap_longmode_x2apic_apicv,
> -                               msr, MSR_TYPE_R);
> +                               msr, type);
>         } else {
>                 __vmx_disable_intercept_for_msr(vmx_msr_bitmap_legacy_x2apic,
> -                               msr, MSR_TYPE_R);
> +                               msr, type);
>                 __vmx_disable_intercept_for_msr(vmx_msr_bitmap_longmode_x2apic,
> -                               msr, MSR_TYPE_R);
> -       }
> -}
> -
> -static void vmx_disable_intercept_msr_write_x2apic(u32 msr, bool apicv_active)
> -{
> -       if (apicv_active) {
> -               __vmx_disable_intercept_for_msr(vmx_msr_bitmap_legacy_x2apic_apicv,
> -                               msr, MSR_TYPE_W);
> -               __vmx_disable_intercept_for_msr(vmx_msr_bitmap_longmode_x2apic_apicv,
> -                               msr, MSR_TYPE_W);
> -       } else {
> -               __vmx_disable_intercept_for_msr(vmx_msr_bitmap_legacy_x2apic,
> -                               msr, MSR_TYPE_W);
> -               __vmx_disable_intercept_for_msr(vmx_msr_bitmap_longmode_x2apic,
> -                               msr, MSR_TYPE_W);
> +                               msr, type);
>         }
>  }
>
> @@ -6463,29 +6448,23 @@ static __init int hardware_setup(void)
>
>         set_bit(0, vmx_vpid_bitmap); /* 0 is reserved for host */
>
> -       /*
> -        * enable_apicv && kvm_vcpu_apicv_active()
> -        */
>         for (msr = 0x800; msr <= 0x8ff; msr++) {
>                 if (msr == 0x839 /* TMCCT */)
>                         continue;
> -               vmx_disable_intercept_msr_read_x2apic(msr, true);
> +               vmx_disable_intercept_msr_x2apic(msr, MSR_TYPE_R, true);
>         }
>
> -       /* TPR */
> -       vmx_disable_intercept_msr_write_x2apic(0x808, true);
> -       /* EOI */
> -       vmx_disable_intercept_msr_write_x2apic(0x80b, true);
> -       /* SELF-IPI */
> -       vmx_disable_intercept_msr_write_x2apic(0x83f, true);
> -
>         /*
> -        * (enable_apicv && !kvm_vcpu_apicv_active()) ||
> -        *      !enable_apicv
> +        * TPR reads and writes can be virtualized even if virtual interrupt
> +        * delivery is not in use.
>          */
> -       /* TPR */
> -       vmx_disable_intercept_msr_read_x2apic(0x808, false);
> -       vmx_disable_intercept_msr_write_x2apic(0x808, false);
> +       vmx_disable_intercept_msr_x2apic(0x808, MSR_TYPE_W, true);
> +       vmx_disable_intercept_msr_x2apic(0x808, MSR_TYPE_R | MSR_TYPE_W, false);
> +
> +       /* EOI */
> +       vmx_disable_intercept_msr_x2apic(0x80b, MSR_TYPE_W, true);
> +       /* SELF-IPI */
> +       vmx_disable_intercept_msr_x2apic(0x83f, MSR_TYPE_W, true);
>
>         if (enable_ept) {
>                 kvm_mmu_set_mask_ptes(VMX_EPT_READABLE_MASK,