Re: [PATCH 3/7 v7] KVM: nSVM: No need to set bits 11:0 in MSRPM and IOPM bitmaps

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 4/22/21 10:50 AM, Krish Sadhukhan wrote:

On 4/20/21 1:00 PM, Sean Christopherson wrote:
On Mon, Apr 12, 2021, Krish Sadhukhan wrote:
According to APM vol 2, hardware ignores the low 12 bits in MSRPM and IOPM bitmaps. Therefore setting/unssetting these bits has no effect as far as VMRUN is concerned. Also, setting/unsetting these bits prevents tests from
verifying hardware behavior.

Signed-off-by: Krish Sadhukhan <krish.sadhukhan@xxxxxxxxxx>
---
  arch/x86/kvm/svm/nested.c | 2 --
  1 file changed, 2 deletions(-)

diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
index ae53ae46ebca..fd42c8b7f99a 100644
--- a/arch/x86/kvm/svm/nested.c
+++ b/arch/x86/kvm/svm/nested.c
@@ -287,8 +287,6 @@ static void nested_load_control_from_vmcb12(struct vcpu_svm *svm,         /* Copy it here because nested_svm_check_controls will check it.  */
      svm->nested.ctl.asid           = control->asid;
-    svm->nested.ctl.msrpm_base_pa &= ~0x0fffULL;
-    svm->nested.ctl.iopm_base_pa  &= ~0x0fffULL;
This will break nested_svm_vmrun_msrpm() if L1 passes an unaligned address.
The shortlog is also wrong, KVM isn't setting bits, it's clearing bits.

I also don't think svm->nested.ctl.msrpm_base_pa makes its way to hardware; IIUC, it's a copy of vmcs12->control.msrpm_base_pa.  The bitmap that gets loaded into
the "real" VMCB is vmcb02->control.msrpm_base_pa.


Not sure if there's a problem with my patch as such, but upon inspecting the code, I see something missing:

    In nested_load_control_from_vmcb12(), we are not really loading msrpm_base_pa from vmcb12 even     though the name of the function suggests so.

    Then nested_vmcb_check_controls() checks msrpm_base_pa from 'nested.ctl' which doesn't have         the copy from vmcb12.

    Then nested_vmcb02_prepare_control() prepares the vmcb02 copy of msrpm_base_pa from vmcb01.ptr->control.msrpm_base_pa.

    Then nested_svm_vmrun_msrpm() uses msrpm_base_pa from 'nested.ctl'.


Aren't we actually using msrpm_base_pa from vmcb01 instead of vmcb02 ?


Sorry, I meant to say,  "from vmcb01 instead of vmcb12"


  }
    /*
--
2.27.0




[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux