[PATCH 0/5 v6] KVM: nSVM: Check addresses of MSR bitmap and IO bitmap tables on vmrun of nested guests

[PATCH 0/5 v6] KVM: nSVM: Check addresses of MSR bitmap and IO bitmap tables on vmrun of nested guests

[Krish Sadhukhan]

v5 -> v6:
	Rebased all patches.

[PATCH 1/5 v6] KVM: SVM: Define actual size of IOPM and MSRPM tables
[PATCH 2/5 v6] nSVM: Check addresses of MSR and IO permission maps
[PATCH 3/5 v6] KVM: nSVM: Cleanup in nested_svm_vmrun()
[PATCH 4/5 v6] nSVM: Test addresses of MSR and IO permissions maps
[PATCH 5/5 v6] SVM: Use ALIGN macro when aligning 'io_bitmap_area'

 arch/x86/kvm/svm/nested.c | 54 ++++++++++++++++++++++++++++++-----------------
 arch/x86/kvm/svm/svm.c    | 20 +++++++++---------
 arch/x86/kvm/svm/svm.h    |  3 +++
 3 files changed, 48 insertions(+), 29 deletions(-)

Krish Sadhukhan (3):
      KVM: SVM: Define actual size of IOPM and MSRPM tables
      nSVM: Check addresses of MSR and IO permission maps
      KVM: nSVM: Cleanup in nested_svm_vmrun()

 x86/svm.c       |  2 +-
 x86/svm_tests.c | 42 +++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 42 insertions(+), 2 deletions(-)

Krish Sadhukhan (2):
      nSVM: Test addresses of MSR and IO permissions maps
      SVM: Use ALIGN macro when aligning 'io_bitmap_area'

[PATCH 1/5 v6] KVM: SVM: Define actual size of IOPM and MSRPM tables

[Krish Sadhukhan]

Define the actual size of the IOPM and MSRPM tables so that the actual size
can be used when initializing them and when checking the consistency of the
physical addresses. These #defines are placed in svm.h so that they can be
shared.

Suggested-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
---
 arch/x86/kvm/svm/svm.c | 20 ++++++++++----------
 arch/x86/kvm/svm/svm.h |  3 +++
 2 files changed, 13 insertions(+), 10 deletions(-)

diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 58a45bb139f8..d1dd6539ed00 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -56,9 +56,6 @@ static const struct x86_cpu_id svm_cpu_id[] = {
 MODULE_DEVICE_TABLE(x86cpu, svm_cpu_id);
 #endif
 
-#define IOPM_ALLOC_ORDER 2
-#define MSRPM_ALLOC_ORDER 1
-
 #define SEG_TYPE_LDT 2
 #define SEG_TYPE_BUSY_TSS16 3
 
@@ -681,14 +678,15 @@ void set_msr_interception(struct kvm_vcpu *vcpu, u32 *msrpm, u32 msr,
 
 u32 *svm_vcpu_alloc_msrpm(void)
 {
-	struct page *pages = alloc_pages(GFP_KERNEL_ACCOUNT, MSRPM_ALLOC_ORDER);
+	unsigned int order = get_order(MSRPM_ALLOC_SIZE);
+	struct page *pages = alloc_pages(GFP_KERNEL_ACCOUNT, order);
 	u32 *msrpm;
 
 	if (!pages)
 		return NULL;
 
 	msrpm = page_address(pages);
-	memset(msrpm, 0xff, PAGE_SIZE * (1 << MSRPM_ALLOC_ORDER));
+	memset(msrpm, 0xff, PAGE_SIZE * (1 << order));
 
 	return msrpm;
 }
@@ -707,7 +705,7 @@ void svm_vcpu_init_msrpm(struct kvm_vcpu *vcpu, u32 *msrpm)
 
 void svm_vcpu_free_msrpm(u32 *msrpm)
 {
-	__free_pages(virt_to_page(msrpm), MSRPM_ALLOC_ORDER);
+	__free_pages(virt_to_page(msrpm), get_order(MSRPM_ALLOC_SIZE));
 }
 
 static void svm_msr_filter_changed(struct kvm_vcpu *vcpu)
@@ -894,7 +892,8 @@ static void svm_hardware_teardown(void)
 	for_each_possible_cpu(cpu)
 		svm_cpu_uninit(cpu);
 
-	__free_pages(pfn_to_page(iopm_base >> PAGE_SHIFT), IOPM_ALLOC_ORDER);
+	__free_pages(pfn_to_page(iopm_base >> PAGE_SHIFT),
+	    get_order(IOPM_ALLOC_SIZE));
 	iopm_base = 0;
 }
 
@@ -930,14 +929,15 @@ static __init int svm_hardware_setup(void)
 	struct page *iopm_pages;
 	void *iopm_va;
 	int r;
+	unsigned int order = get_order(IOPM_ALLOC_SIZE);
 
-	iopm_pages = alloc_pages(GFP_KERNEL, IOPM_ALLOC_ORDER);
+	iopm_pages = alloc_pages(GFP_KERNEL, order);
 
 	if (!iopm_pages)
 		return -ENOMEM;
 
 	iopm_va = page_address(iopm_pages);
-	memset(iopm_va, 0xff, PAGE_SIZE * (1 << IOPM_ALLOC_ORDER));
+	memset(iopm_va, 0xff, PAGE_SIZE * (1 << order));
 	iopm_base = page_to_pfn(iopm_pages) << PAGE_SHIFT;
 
 	init_msrpm_offsets();
@@ -1408,7 +1408,7 @@ static void svm_free_vcpu(struct kvm_vcpu *vcpu)
 	sev_free_vcpu(vcpu);
 
 	__free_page(pfn_to_page(__sme_clr(svm->vmcb_pa) >> PAGE_SHIFT));
-	__free_pages(virt_to_page(svm->msrpm), MSRPM_ALLOC_ORDER);
+	__free_pages(virt_to_page(svm->msrpm), get_order(MSRPM_ALLOC_SIZE));
 }
 
 static void svm_prepare_guest_switch(struct kvm_vcpu *vcpu)
diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
index 39e071fdab0c..d0a4d7ce8445 100644
--- a/arch/x86/kvm/svm/svm.h
+++ b/arch/x86/kvm/svm/svm.h
@@ -28,6 +28,9 @@ static const u32 host_save_user_msrs[] = {
 };
 #define NR_HOST_SAVE_USER_MSRS ARRAY_SIZE(host_save_user_msrs)
 
+#define IOPM_ALLOC_SIZE PAGE_SIZE * 3
+#define MSRPM_ALLOC_SIZE PAGE_SIZE * 2
+
 #define MAX_DIRECT_ACCESS_MSRS	18
 #define MSRPM_OFFSETS	16
 extern u32 msrpm_offsets[MSRPM_OFFSETS] __read_mostly;
-- 
2.27.0

[PATCH 2/5 v6] nSVM: Check addresses of MSR and IO permission maps

[Krish Sadhukhan]

According to section "Canonicalization and Consistency Checks" in APM vol 2,
the following guest state is illegal:

    "The MSR or IOIO intercept tables extend to a physical address that
     is greater than or equal to the maximum supported physical address."

Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
---
 arch/x86/kvm/svm/nested.c | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
index fb204eaa8bb3..b3988b3a3fd5 100644
--- a/arch/x86/kvm/svm/nested.c
+++ b/arch/x86/kvm/svm/nested.c
@@ -231,7 +231,15 @@ static bool svm_get_nested_state_pages(struct kvm_vcpu *vcpu)
 	return true;
 }
 
-static bool nested_vmcb_check_controls(struct vmcb_control_area *control)
+static bool nested_svm_check_bitmap_pa(struct kvm_vcpu *vcpu, u64 pa,
+				       u32 size)
+{
+	u64 last_pa = PAGE_ALIGN(pa) + size - 1;
+	return (kvm_vcpu_is_legal_gpa(vcpu, last_pa));
+}
+
+static bool nested_vmcb_check_controls(struct kvm_vcpu *vcpu,
+				       struct vmcb_control_area *control)
 {
 	if ((vmcb_is_intercept(control, INTERCEPT_VMRUN)) == 0)
 		return false;
@@ -243,6 +251,13 @@ static bool nested_vmcb_check_controls(struct vmcb_control_area *control)
 	    !npt_enabled)
 		return false;
 
+	if (!nested_svm_check_bitmap_pa(vcpu, control->msrpm_base_pa,
+	    MSRPM_ALLOC_SIZE))
+		return false;
+	if (!nested_svm_check_bitmap_pa(vcpu, control->iopm_base_pa,
+	    IOPM_ALLOC_SIZE - PAGE_SIZE + 1))
+		return false;
+
 	return true;
 }
 
@@ -275,7 +290,7 @@ static bool nested_vmcb_check_save(struct vcpu_svm *svm, struct vmcb *vmcb12)
 		    kvm_vcpu_is_illegal_gpa(vcpu, vmcb12->save.cr3))
 			return false;
 	}
-	if (!kvm_is_valid_cr4(&svm->vcpu, vmcb12->save.cr4))
+	if (!kvm_is_valid_cr4(vcpu, vmcb12->save.cr4))
 		return false;
 
 	return true;
@@ -531,7 +546,7 @@ int nested_svm_vmrun(struct vcpu_svm *svm)
 	load_nested_vmcb_control(svm, &vmcb12->control);
 
 	if (!nested_vmcb_check_save(svm, vmcb12) ||
-	    !nested_vmcb_check_controls(&svm->nested.ctl)) {
+	    !nested_vmcb_check_controls(&svm->vcpu, &svm->nested.ctl)) {
 		vmcb12->control.exit_code    = SVM_EXIT_ERR;
 		vmcb12->control.exit_code_hi = 0;
 		vmcb12->control.exit_info_1  = 0;
@@ -1207,7 +1222,7 @@ static int svm_set_nested_state(struct kvm_vcpu *vcpu,
 		goto out_free;
 
 	ret = -EINVAL;
-	if (!nested_vmcb_check_controls(ctl))
+	if (!nested_vmcb_check_controls(vcpu, ctl))
 		goto out_free;
 
 	/*
-- 
2.27.0

[PATCH 3/5 v6] KVM: nSVM: Cleanup in nested_svm_vmrun()

[Krish Sadhukhan]

Use local variables to derefence svm->vcpu and svm->vmcb as they make the
code tidier.

Suggested-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
---
 arch/x86/kvm/svm/nested.c | 31 ++++++++++++++++---------------
 1 file changed, 16 insertions(+), 15 deletions(-)

diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
index b3988b3a3fd5..3fd51fe63170 100644
--- a/arch/x86/kvm/svm/nested.c
+++ b/arch/x86/kvm/svm/nested.c
@@ -517,26 +517,27 @@ int nested_svm_vmrun(struct vcpu_svm *svm)
 {
 	int ret;
 	struct vmcb *vmcb12;
+	struct kvm_vcpu *vcpu = &svm->vcpu;
 	struct vmcb *hsave = svm->nested.hsave;
 	struct vmcb *vmcb = svm->vmcb;
 	struct kvm_host_map map;
 	u64 vmcb12_gpa;
 
-	if (is_smm(&svm->vcpu)) {
-		kvm_queue_exception(&svm->vcpu, UD_VECTOR);
+	if (is_smm(vcpu)) {
+		kvm_queue_exception(vcpu, UD_VECTOR);
 		return 1;
 	}
 
-	vmcb12_gpa = svm->vmcb->save.rax;
-	ret = kvm_vcpu_map(&svm->vcpu, gpa_to_gfn(vmcb12_gpa), &map);
+	vmcb12_gpa = vmcb->save.rax;
+	ret = kvm_vcpu_map(vcpu, gpa_to_gfn(vmcb12_gpa), &map);
 	if (ret == -EINVAL) {
-		kvm_inject_gp(&svm->vcpu, 0);
+		kvm_inject_gp(vcpu, 0);
 		return 1;
 	} else if (ret) {
-		return kvm_skip_emulated_instruction(&svm->vcpu);
+		return kvm_skip_emulated_instruction(vcpu);
 	}
 
-	ret = kvm_skip_emulated_instruction(&svm->vcpu);
+	ret = kvm_skip_emulated_instruction(vcpu);
 
 	vmcb12 = map.hva;
 
@@ -556,8 +557,8 @@ int nested_svm_vmrun(struct vcpu_svm *svm)
 
 
 	/* Clear internal status */
-	kvm_clear_exception_queue(&svm->vcpu);
-	kvm_clear_interrupt_queue(&svm->vcpu);
+	kvm_clear_exception_queue(vcpu);
+	kvm_clear_interrupt_queue(vcpu);
 
 	/*
 	 * Save the old vmcb, so we don't need to pick what we save, but can
@@ -569,17 +570,17 @@ int nested_svm_vmrun(struct vcpu_svm *svm)
 	hsave->save.ds     = vmcb->save.ds;
 	hsave->save.gdtr   = vmcb->save.gdtr;
 	hsave->save.idtr   = vmcb->save.idtr;
-	hsave->save.efer   = svm->vcpu.arch.efer;
-	hsave->save.cr0    = kvm_read_cr0(&svm->vcpu);
+	hsave->save.efer   = vcpu->arch.efer;
+	hsave->save.cr0    = kvm_read_cr0(vcpu);
 	hsave->save.cr4    = svm->vcpu.arch.cr4;
-	hsave->save.rflags = kvm_get_rflags(&svm->vcpu);
-	hsave->save.rip    = kvm_rip_read(&svm->vcpu);
+	hsave->save.rflags = kvm_get_rflags(vcpu);
+	hsave->save.rip    = kvm_rip_read(vcpu);
 	hsave->save.rsp    = vmcb->save.rsp;
 	hsave->save.rax    = vmcb->save.rax;
 	if (npt_enabled)
 		hsave->save.cr3    = vmcb->save.cr3;
 	else
-		hsave->save.cr3    = kvm_read_cr3(&svm->vcpu);
+		hsave->save.cr3    = kvm_read_cr3(vcpu);
 
 	copy_vmcb_control_area(&hsave->control, &vmcb->control);
 
@@ -602,7 +603,7 @@ int nested_svm_vmrun(struct vcpu_svm *svm)
 	nested_svm_vmexit(svm);
 
 out:
-	kvm_vcpu_unmap(&svm->vcpu, &map, true);
+	kvm_vcpu_unmap(vcpu, &map, true);
 
 	return ret;
 }
-- 
2.27.0

[PATCH 4/5 v6] nSVM: Test addresses of MSR and IO permissions maps

[Krish Sadhukhan]

According to section "Canonicalization and Consistency Checks" in APM vol 2,
the following guest state is illegal:

    "The MSR or IOIO intercept tables extend to a physical address that
     is greater than or equal to the maximum supported physical address."

Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
(cherry picked from commit 0513cf071255c7d5a1b7a813d017bbdd2d1da263)
Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
---
 x86/svm_tests.c | 42 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 41 insertions(+), 1 deletion(-)

diff --git a/x86/svm_tests.c b/x86/svm_tests.c
index 29a0b59..7014c40 100644
--- a/x86/svm_tests.c
+++ b/x86/svm_tests.c
@@ -2304,15 +2304,55 @@ static void test_dr(void)
 	vmcb->save.dr7 = dr_saved;
 }
 
+/*
+ * If the MSR or IOIO intercept table extends to a physical address that
+ * is greater than or equal to the maximum supported physical address, the
+ * guest state is illegal.
+ *
+ * [APM vol 2]
+ */
+static void test_msrpm_iopm_bitmap_addrs(void)
+{
+	u64 saved_intercepts = vmcb->control.intercept;
+	u64 bitmap_addr_1 =
+	    (u64)(((u64)1 << cpuid_maxphyaddr()) - PAGE_SIZE);
+	u64 bitmap_addr_2 =
+	    (u64)(((u64)1 << cpuid_maxphyaddr()) - PAGE_SIZE * 2);
+
+	/*
+	 * MSR bitmap address
+	 */
+	vmcb->control.intercept = saved_intercepts | 1ULL << INTERCEPT_MSR_PROT;
+	vmcb->control.msrpm_base_pa = bitmap_addr_1;
+	report(svm_vmrun() == SVM_EXIT_ERR, "Test MSRPM address: %lx",
+	    bitmap_addr_1);
+	vmcb->control.msrpm_base_pa = bitmap_addr_2;
+	report(svm_vmrun() == SVM_EXIT_ERR, "Test MSRPM address: %lx",
+	    bitmap_addr_2);
+
+	/*
+	 * IOIO bitmap address
+	 */
+	vmcb->control.intercept = saved_intercepts | 1ULL << INTERCEPT_IOIO_PROT;
+	vmcb->control.iopm_base_pa = bitmap_addr_1;
+	report(svm_vmrun() == SVM_EXIT_ERR, "Test IOPM address: %lx",
+	    bitmap_addr_1);
+	vmcb->control.iopm_base_pa = bitmap_addr_2 += 1;
+	report(svm_vmrun() == SVM_EXIT_ERR, "Test IOPM address: %lx",
+	    bitmap_addr_2);
+
+	vmcb->control.intercept = saved_intercepts;
+}
+
 static void svm_guest_state_test(void)
 {
 	test_set_guest(basic_guest_main);
-
 	test_efer();
 	test_cr0();
 	test_cr3();
 	test_cr4();
 	test_dr();
+	test_msrpm_iopm_bitmap_addrs();
 }
 
 
-- 
2.27.0

[PATCH 5/5 v6] SVM: Use ALIGN macro when aligning 'io_bitmap_area'

[Krish Sadhukhan]

Since the macro is available and we already use it for MSR bitmap table, use
it for aligning IO bitmap table also.

Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
(cherry picked from commit bddbbf66215b4fc0076d1391296f11ce3bee7c63)
Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
---
 x86/svm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/x86/svm.c b/x86/svm.c
index a1808c7..846cf2a 100644
--- a/x86/svm.c
+++ b/x86/svm.c
@@ -298,7 +298,7 @@ static void setup_svm(void)
 	wrmsr(MSR_EFER, rdmsr(MSR_EFER) | EFER_SVME);
 	wrmsr(MSR_EFER, rdmsr(MSR_EFER) | EFER_NX);
 
-	io_bitmap = (void *) (((ulong)io_bitmap_area + 4095) & ~4095);
+	io_bitmap = (void *) ALIGN((ulong)io_bitmap_area, PAGE_SIZE);
 
 	msr_bitmap = (void *) ALIGN((ulong)msr_bitmap_area, PAGE_SIZE);
 
-- 
2.27.0

Re: [PATCH 0/5 v6] KVM: nSVM: Check addresses of MSR bitmap and IO bitmap tables on vmrun of nested guests

[Sean Christopherson]

On Thu, Apr 01, 2021, Krish Sadhukhan wrote:
> v5 -> v6:
> 	Rebased all patches.

Please rebase to kvm/queue, not kvm/master.  All of the patches conflict, and
the conflicts in patches 02 and 03 are not trivial.

> [PATCH 1/5 v6] KVM: SVM: Define actual size of IOPM and MSRPM tables
> [PATCH 2/5 v6] nSVM: Check addresses of MSR and IO permission maps
> [PATCH 3/5 v6] KVM: nSVM: Cleanup in nested_svm_vmrun()
> [PATCH 4/5 v6] nSVM: Test addresses of MSR and IO permissions maps
> [PATCH 5/5 v6] SVM: Use ALIGN macro when aligning 'io_bitmap_area'
> 
>  arch/x86/kvm/svm/nested.c | 54 ++++++++++++++++++++++++++++++-----------------
>  arch/x86/kvm/svm/svm.c    | 20 +++++++++---------
>  arch/x86/kvm/svm/svm.h    |  3 +++
>  3 files changed, 48 insertions(+), 29 deletions(-)
> 
> Krish Sadhukhan (3):
>       KVM: SVM: Define actual size of IOPM and MSRPM tables
>       nSVM: Check addresses of MSR and IO permission maps
>       KVM: nSVM: Cleanup in nested_svm_vmrun()
> 
>  x86/svm.c       |  2 +-
>  x86/svm_tests.c | 42 +++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 42 insertions(+), 2 deletions(-)
> 
> Krish Sadhukhan (2):
>       nSVM: Test addresses of MSR and IO permissions maps
>       SVM: Use ALIGN macro when aligning 'io_bitmap_area'
> 

Re: [PATCH 0/5 v6] KVM: nSVM: Check addresses of MSR bitmap and IO bitmap tables on vmrun of nested guests

[Paolo Bonzini]

On 02/04/21 19:38, Sean Christopherson wrote:
>> v5 -> v6:
>> 	Rebased all patches.
>
> Please rebase to kvm/queue, not kvm/master.
... or at least kvm/next.

Paolo

Re: [PATCH 2/5 v6] nSVM: Check addresses of MSR and IO permission maps

[Sean Christopherson]

On Thu, Apr 01, 2021, Krish Sadhukhan wrote:
> According to section "Canonicalization and Consistency Checks" in APM vol 2,
> the following guest state is illegal:
> 
>     "The MSR or IOIO intercept tables extend to a physical address that
>      is greater than or equal to the maximum supported physical address."
> 
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
> ---
>  arch/x86/kvm/svm/nested.c | 23 +++++++++++++++++++----
>  1 file changed, 19 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> index fb204eaa8bb3..b3988b3a3fd5 100644
> --- a/arch/x86/kvm/svm/nested.c
> +++ b/arch/x86/kvm/svm/nested.c
> @@ -231,7 +231,15 @@ static bool svm_get_nested_state_pages(struct kvm_vcpu *vcpu)
>  	return true;
>  }
>  
> -static bool nested_vmcb_check_controls(struct vmcb_control_area *control)
> +static bool nested_svm_check_bitmap_pa(struct kvm_vcpu *vcpu, u64 pa,
> +				       u32 size)

No need to put "u32 size)" on a separate line.

> +{
> +	u64 last_pa = PAGE_ALIGN(pa) + size - 1;

Space between declaration and code.  A comment about the silliness of hardware
silently ignoring bits 11:0 would also be nice.

> +	return (kvm_vcpu_is_legal_gpa(vcpu, last_pa));

This fails to handle the case where "pa == -1", as the above will wrap to a
legal address and generate a false negative.

	/* blah blah blah */
	pa = PAGE_ALIGN(pa);
	return kvm_vcpu_is_legal_gpa(pa) &&
	       kvm_vcpu_is_legal_gpa(pa + size - 1);

> +}
> +
> +static bool nested_vmcb_check_controls(struct kvm_vcpu *vcpu,
> +				       struct vmcb_control_area *control)
>  {
>  	if ((vmcb_is_intercept(control, INTERCEPT_VMRUN)) == 0)
>  		return false;
> @@ -243,6 +251,13 @@ static bool nested_vmcb_check_controls(struct vmcb_control_area *control)
>  	    !npt_enabled)
>  		return false;
>  
> +	if (!nested_svm_check_bitmap_pa(vcpu, control->msrpm_base_pa,
> +	    MSRPM_ALLOC_SIZE))
> +		return false;
> +	if (!nested_svm_check_bitmap_pa(vcpu, control->iopm_base_pa,
> +	    IOPM_ALLOC_SIZE - PAGE_SIZE + 1))

Align with the function's paranthesis, not the if statements, for both.

	
	if (!nested_svm_check_bitmap_pa(vcpu, control->iopm_base_pa,
					IOPM_ALLOC_SIZE - PAGE_SIZE + 1))
> +		return false;
> +
>  	return true;
>  }
>  
> @@ -275,7 +290,7 @@ static bool nested_vmcb_check_save(struct vcpu_svm *svm, struct vmcb *vmcb12)
>  		    kvm_vcpu_is_illegal_gpa(vcpu, vmcb12->save.cr3))
>  			return false;
>  	}
> -	if (!kvm_is_valid_cr4(&svm->vcpu, vmcb12->save.cr4))
> +	if (!kvm_is_valid_cr4(vcpu, vmcb12->save.cr4))
>  		return false;
>  
>  	return true;
> @@ -531,7 +546,7 @@ int nested_svm_vmrun(struct vcpu_svm *svm)
>  	load_nested_vmcb_control(svm, &vmcb12->control);
>  
>  	if (!nested_vmcb_check_save(svm, vmcb12) ||
> -	    !nested_vmcb_check_controls(&svm->nested.ctl)) {
> +	    !nested_vmcb_check_controls(&svm->vcpu, &svm->nested.ctl)) {

Pass "vcpu" directly (probably only once you rebase).

>  		vmcb12->control.exit_code    = SVM_EXIT_ERR;
>  		vmcb12->control.exit_code_hi = 0;
>  		vmcb12->control.exit_info_1  = 0;
> @@ -1207,7 +1222,7 @@ static int svm_set_nested_state(struct kvm_vcpu *vcpu,
>  		goto out_free;
>  
>  	ret = -EINVAL;
> -	if (!nested_vmcb_check_controls(ctl))
> +	if (!nested_vmcb_check_controls(vcpu, ctl))
>  		goto out_free;
>  
>  	/*
> -- 
> 2.27.0
> 

Re: [PATCH 1/5 v6] KVM: SVM: Define actual size of IOPM and MSRPM tables

[Sean Christopherson]

On Thu, Apr 01, 2021, Krish Sadhukhan wrote:
> Define the actual size of the IOPM and MSRPM tables so that the actual size
> can be used when initializing them and when checking the consistency of the
> physical addresses. These #defines are placed in svm.h so that they can be
> shared.
>  static void svm_prepare_guest_switch(struct kvm_vcpu *vcpu)
> diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
> index 39e071fdab0c..d0a4d7ce8445 100644
> --- a/arch/x86/kvm/svm/svm.h
> +++ b/arch/x86/kvm/svm/svm.h
> @@ -28,6 +28,9 @@ static const u32 host_save_user_msrs[] = {
>  };
>  #define NR_HOST_SAVE_USER_MSRS ARRAY_SIZE(host_save_user_msrs)
>  
> +#define IOPM_ALLOC_SIZE PAGE_SIZE * 3
> +#define MSRPM_ALLOC_SIZE PAGE_SIZE * 2

Drop the "ALLOC", this is the architectural size, not the size that's allocated.

It'd also be nice to align the values.  My personal preference would be to have
the number of pages come first, i.e. "3 pages" versus "4096 3-byte chunks", but
that's definitely getting deep into nitpick territory. :-)

#define IOPM_ALLOC_SIZE  3 * PAGE_SIZE
#define MSRPM_ALLOC_SIZE 2 * PAGE_SIZE
> +
>  #define MAX_DIRECT_ACCESS_MSRS	18
>  #define MSRPM_OFFSETS	16
>  extern u32 msrpm_offsets[MSRPM_OFFSETS] __read_mostly;
> -- 
> 2.27.0
>