KVM Archive on lore.kernel.org
 help / color / Atom feed
From: Krish Sadhukhan <krish.sadhukhan@oracle.com>
To: Sean Christopherson <seanjc@google.com>
Cc: kvm@vger.kernel.org, pbonzini@redhat.com, jmattson@google.com
Subject: Re: [PATCH 3/7 v7] KVM: nSVM: No need to set bits 11:0 in MSRPM and IOPM bitmaps
Date: Thu, 22 Apr 2021 18:12:41 -0700
Message-ID: <a9f74546-6ab7-88fc-83d1-382b380f6264@oracle.com> (raw)
In-Reply-To: <YIG6B+LBsRWcpftK@google.com>


On 4/22/21 11:01 AM, Sean Christopherson wrote:
> On Thu, Apr 22, 2021, Krish Sadhukhan wrote:
>> 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@oracle.com>
>>>>> ---
>>>>>    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"
> The bitmap that's shoved into hardware comes from vmcb02, the bitmap that KVM
> reads to merge into _that_ bitmap comes from vmcb12.
>
> static bool nested_svm_vmrun_msrpm(struct vcpu_svm *svm)
> {
> 	/*
> 	 * This function merges the msr permission bitmaps of kvm and the
> 	 * nested vmcb. It is optimized in that it only merges the parts where
> 	 * the kvm msr permission bitmap may contain zero bits
> 	 */
> 	int i;
>
> 	if (!(vmcb_is_intercept(&svm->nested.ctl, INTERCEPT_MSR_PROT)))
> 		return true;
>
> 	for (i = 0; i < MSRPM_OFFSETS; i++) {
> 		u32 value, p;
> 		u64 offset;
>
> 		if (msrpm_offsets[i] == 0xffffffff)
> 			break;
>
> 		p      = msrpm_offsets[i];
> 		offset = svm->nested.ctl.msrpm_base_pa + (p * 4);
>
> 		if (kvm_vcpu_read_guest(&svm->vcpu, offset, &value, 4)) <- This reads vmcb12
> 			return false;
>
> 		svm->nested.msrpm[p] = svm->msrpm[p] | value; <- Merge vmcb12's bitmap to KVM's bitmap for L2
> 	}
>
> 	svm->vmcb->control.msrpm_base_pa = __sme_set(__pa(svm->nested.msrpm)); <- This is vmcb02
>
> 	return true;
> }

Sorry, I somehow missed the call to copy_vmcb_control_area() in 
nested_load_control_from_vmcb12() where we are actually getting the 
msrpm_base_pa from vmcb12. Thanks for the explanation.

Getting back to your concern that this patch breaks 
nested_svm_vmrun_msrpm().  If L1 passes a valid address in which some 
bits in 11:0 are set, the hardware is anyway going to ignore those bits, 
irrespective of whether we clear them (before my patch) or pass them as 
is (my patch) and therefore what L1 thinks as a valid address will 
effectively be an invalid address to the hardware. The only difference 
my patch makes is it enables tests to verify hardware behavior. Am 
missing something ?


  reply index

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-12 21:56 [PATCH 0/7 v7] KVM: nSVM: Check addresses of MSR bitmap and IO bitmap tables on vmrun of nested guests Krish Sadhukhan
2021-04-12 21:56 ` [PATCH 1/7 v7] KVM: SVM: Define actual size of IOPM and MSRPM tables Krish Sadhukhan
2021-04-12 21:56 ` [PATCH 2/7 v7] KVM: nSVM: Define an exit code to reflect consistency check failure Krish Sadhukhan
2021-04-17 14:17   ` Paolo Bonzini
2021-04-19 17:57     ` Krish Sadhukhan
2021-04-19 18:28       ` Paolo Bonzini
2021-04-19 18:36         ` Jim Mattson
2021-04-12 21:56 ` [PATCH 3/7 v7] KVM: nSVM: No need to set bits 11:0 in MSRPM and IOPM bitmaps Krish Sadhukhan
2021-04-17 14:18   ` Paolo Bonzini
2021-04-20 20:00   ` Sean Christopherson
2021-04-22 17:50     ` Krish Sadhukhan
2021-04-22 17:52       ` Krish Sadhukhan
2021-04-22 17:56       ` Krish Sadhukhan
2021-04-22 18:01         ` Sean Christopherson
2021-04-23  1:12           ` Krish Sadhukhan [this message]
2021-04-23 15:56             ` Sean Christopherson
2021-04-23 20:31               ` Paolo Bonzini
2021-04-26 21:59                 ` Krish Sadhukhan
2021-04-26 22:07                   ` Sean Christopherson
2021-04-12 21:56 ` [PATCH 4/7 v7] nSVM: Check addresses of MSR and IO permission maps Krish Sadhukhan
2021-04-12 21:56 ` [PATCH 5/7 v7] SVM: Use ALIGN macro when aligning 'io_bitmap_area' Krish Sadhukhan
2021-04-12 21:56 ` [PATCH 6/7 v7] nSVM: Define an exit code to reflect consistency check failure Krish Sadhukhan
2021-04-12 21:56 ` [PATCH 7/7 v7] nSVM: Test addresses of MSR and IO permissions maps Krish Sadhukhan
2021-04-17 14:35 ` [PATCH 0/7 v7] KVM: nSVM: Check addresses of MSR bitmap and IO bitmap tables on vmrun of nested guests Paolo Bonzini

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=a9f74546-6ab7-88fc-83d1-382b380f6264@oracle.com \
    --to=krish.sadhukhan@oracle.com \
    --cc=jmattson@google.com \
    --cc=kvm@vger.kernel.org \
    --cc=pbonzini@redhat.com \
    --cc=seanjc@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

KVM Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/kvm/0 kvm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 kvm kvm/ https://lore.kernel.org/kvm \
		kvm@vger.kernel.org
	public-inbox-index kvm

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.kvm


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git