[PATCH] KVM: x86: Keep masked bits unmodified on kvm_set_shared_msr

[PATCH] KVM: x86: Keep masked bits unmodified on kvm_set_shared_msr

[Nadav Amit]

Currently, when an msr is updated using kvm_set_shared_msr the masked bits are
zeroed.  This behavior is currently valid since the only MSR with partial mask
is EFER, in which only SCE might be unmasked. However, using the
kvm_set_shared_msr for other purposes becomes impossible.

This patch keeps the masked bits unmodified while setting a shared msr.

Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
---
 arch/x86/kvm/x86.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 5f5edb6..ee42410 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -236,6 +236,7 @@ void kvm_set_shared_msr(unsigned slot, u64 value, u64 mask)
 
 	if (((value ^ smsr->values[slot].curr) & mask) == 0)
 		return;
+	value = (smsr->values[slot].curr & ~mask) | (value & mask);
 	smsr->values[slot].curr = value;
 	wrmsrl(shared_msrs_global.msrs[slot], value);
 	if (!smsr->registered) {
-- 
1.9.1

Re: [PATCH] KVM: x86: Keep masked bits unmodified on kvm_set_shared_msr

[Wanpeng Li]

Hi Nadav,
On Wed, Aug 20, 2014 at 03:11:51PM +0300, Nadav Amit wrote:
>Currently, when an msr is updated using kvm_set_shared_msr the masked bits are
>zeroed.  This behavior is currently valid since the only MSR with partial mask

Why zeroed? vmx_vcpu_setup() set all mask to -1ull.

>is EFER, in which only SCE might be unmasked. However, using the

Do you mean SCE might be masked? 

>kvm_set_shared_msr for other purposes becomes impossible.
>
>This patch keeps the masked bits unmodified while setting a shared msr.
>

Do you mean "keeps the unmasked bits unmodified" instead of "keeps the
masked bits unmodified"?

Regards,
Wanpeng Li 

>Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
>---
> arch/x86/kvm/x86.c | 1 +
> 1 file changed, 1 insertion(+)
>
>diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>index 5f5edb6..ee42410 100644
>--- a/arch/x86/kvm/x86.c
>+++ b/arch/x86/kvm/x86.c
>@@ -236,6 +236,7 @@ void kvm_set_shared_msr(unsigned slot, u64 value, u64 mask)
> 
> 	if (((value ^ smsr->values[slot].curr) & mask) == 0)
> 		return;
>+	value = (smsr->values[slot].curr & ~mask) | (value & mask);
> 	smsr->values[slot].curr = value;
> 	wrmsrl(shared_msrs_global.msrs[slot], value);
> 	if (!smsr->registered) {
>-- 
>1.9.1
>
>--
>To unsubscribe from this list: send the line "unsubscribe kvm" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] KVM: x86: Keep masked bits unmodified on kvm_set_shared_msr

[Paolo Bonzini]

Il 21/08/2014 10:05, Wanpeng Li ha scritto:
> Hi Nadav,
> On Wed, Aug 20, 2014 at 03:11:51PM +0300, Nadav Amit wrote:
>> Currently, when an msr is updated using kvm_set_shared_msr the masked bits are
>> zeroed.  This behavior is currently valid since the only MSR with partial mask
> 
> Why zeroed? vmx_vcpu_setup() set all mask to -1ull.

He meant they are passed as zero in the WRMSR but actually they're not
zeroed.  They're set to the value that is passed to kvm_set_shared_msr,
and this value is massaged elsewhere to do mix guest and host bugs.  See
update_transition_efer.

So I'm removing this patch, it's wrong.

Paolo

>> is EFER, in which only SCE might be unmasked. However, using the
> 
> Do you mean SCE might be masked? 
> 
>> kvm_set_shared_msr for other purposes becomes impossible.
>>
>> This patch keeps the masked bits unmodified while setting a shared msr.
>>
> 
> Do you mean "keeps the unmasked bits unmodified" instead of "keeps the
> masked bits unmodified"?
> 
> Regards,
> Wanpeng Li 
> 
>> Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
>> ---
>> arch/x86/kvm/x86.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index 5f5edb6..ee42410 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -236,6 +236,7 @@ void kvm_set_shared_msr(unsigned slot, u64 value, u64 mask)
>>
>> 	if (((value ^ smsr->values[slot].curr) & mask) == 0)
>> 		return;
>> +	value = (smsr->values[slot].curr & ~mask) | (value & mask);
>> 	smsr->values[slot].curr = value;
>> 	wrmsrl(shared_msrs_global.msrs[slot], value);
>> 	if (!smsr->registered) {
>> -- 
>> 1.9.1
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe kvm" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] KVM: x86: Keep masked bits unmodified on kvm_set_shared_msr

[Nadav Amit]

[-- Attachment #1: Type: text/plain, Size: 1393 bytes --]


On Aug 21, 2014, at 2:56 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:

> Il 21/08/2014 10:05, Wanpeng Li ha scritto:
>> Hi Nadav,
>> On Wed, Aug 20, 2014 at 03:11:51PM +0300, Nadav Amit wrote:
>>> Currently, when an msr is updated using kvm_set_shared_msr the masked bits are
>>> zeroed.  This behavior is currently valid since the only MSR with partial mask
>> 
>> Why zeroed? vmx_vcpu_setup() set all mask to -1ull.
> 
> He meant they are passed as zero in the WRMSR but actually they're not
> zeroed.  They're set to the value that is passed to kvm_set_shared_msr,
> and this value is massaged elsewhere to do mix guest and host bugs.  See
> update_transition_efer.
> 
> So I'm removing this patch, it's wrong.

I stand corrected - they are massaged in update_transition_efer.

The question is whether this massaging is specific to EFER, or a general one.
Currently update_transition_efer does:

        guest_efer &= ~ignore_bits;
        guest_efer |= host_efer & ignore_bits;
	vmx->guest_msrs[efer_offset].data = guest_efer;

I think this is a general behaviour - taking the masked bits from the host, and the rest from the guest. Therefore, it makes sense to put this logic into kvm_set_shared_msr.
I understand the EFER is currently the only MSR which is only partially masked. Nonetheless, kvm_set_shared_msr can be useful for other purposes.

Nadav

[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH] KVM: x86: Keep masked bits unmodified on kvm_set_shared_msr

[Paolo Bonzini]

Il 21/08/2014 14:19, Nadav Amit ha scritto:
>> > 
>> > He meant they are passed as zero in the WRMSR but actually they're not
>> > zeroed.  They're set to the value that is passed to kvm_set_shared_msr,
>> > and this value is massaged elsewhere to do mix guest and host bugs.  See
>> > update_transition_efer.
>> > 
>> > So I'm removing this patch, it's wrong.
> I stand corrected - they are massaged in update_transition_efer.
> 
> The question is whether this massaging is specific to EFER, or a general one.
> Currently update_transition_efer does:
> 
>         guest_efer &= ~ignore_bits;
>         guest_efer |= host_efer & ignore_bits;
> 	vmx->guest_msrs[efer_offset].data = guest_efer;
> 
> I think this is a general behaviour - taking the masked bits from the
> host, and the rest from the guest. Therefore, it makes sense to put
> this logic into kvm_set_shared_msr. I understand the EFER is
> currently the only MSR which is only partially masked. Nonetheless,
> kvm_set_shared_msr can be useful for other purposes.

Yes, I agree.  But right now it's not particularly interesting to do it:
you're not using the functionality in e.g. the MISC_ENABLE patch, so
it's just a matter of defining the semantics of the .data field, basically.

Paolo

Re: [PATCH] KVM: x86: Keep masked bits unmodified on kvm_set_shared_msr

[Nadav Amit]

[-- Attachment #1: Type: text/plain, Size: 1523 bytes --]


On Aug 21, 2014, at 3:31 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:

> Il 21/08/2014 14:19, Nadav Amit ha scritto:
>>>> 
>>>> He meant they are passed as zero in the WRMSR but actually they're not
>>>> zeroed.  They're set to the value that is passed to kvm_set_shared_msr,
>>>> and this value is massaged elsewhere to do mix guest and host bugs.  See
>>>> update_transition_efer.
>>>> 
>>>> So I'm removing this patch, it's wrong.
>> I stand corrected - they are massaged in update_transition_efer.
>> 
>> The question is whether this massaging is specific to EFER, or a general one.
>> Currently update_transition_efer does:
>> 
>>        guest_efer &= ~ignore_bits;
>>        guest_efer |= host_efer & ignore_bits;
>> 	vmx->guest_msrs[efer_offset].data = guest_efer;
>> 
>> I think this is a general behaviour - taking the masked bits from the
>> host, and the rest from the guest. Therefore, it makes sense to put
>> this logic into kvm_set_shared_msr. I understand the EFER is
>> currently the only MSR which is only partially masked. Nonetheless,
>> kvm_set_shared_msr can be useful for other purposes.
> 
> Yes, I agree.  But right now it's not particularly interesting to do it:
> you're not using the functionality in e.g. the MISC_ENABLE patch, so
> it's just a matter of defining the semantics of the .data field, basically.

No problem. Once I release a patch that needs this functionality, I’ll resubmit this one (while updating update_transition_efer).

Thanks,
Nadav


[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH] KVM: x86: Keep masked bits unmodified on kvm_set_shared_msr

[Wanpeng Li]

Hi Paolo,
On Thu, Aug 21, 2014 at 01:56:46PM +0200, Paolo Bonzini wrote:
>Il 21/08/2014 10:05, Wanpeng Li ha scritto:
>> Hi Nadav,
>> On Wed, Aug 20, 2014 at 03:11:51PM +0300, Nadav Amit wrote:
>>> Currently, when an msr is updated using kvm_set_shared_msr the masked bits are
>>> zeroed.  This behavior is currently valid since the only MSR with partial mask
>> 
>> Why zeroed? vmx_vcpu_setup() set all mask to -1ull.
>
>He meant they are passed as zero in the WRMSR but actually they're not

I fail to understand "they are passed as zero". Could you explain more
in details? ;-)

Regards,
Wanpeng Li 

>zeroed.  They're set to the value that is passed to kvm_set_shared_msr,
>and this value is massaged elsewhere to do mix guest and host bugs.  See
>update_transition_efer.
>
>So I'm removing this patch, it's wrong.
>
>Paolo
>
>>> is EFER, in which only SCE might be unmasked. However, using the
>> 
>> Do you mean SCE might be masked? 
>> 
>>> kvm_set_shared_msr for other purposes becomes impossible.
>>>
>>> This patch keeps the masked bits unmodified while setting a shared msr.
>>>
>> 
>> Do you mean "keeps the unmasked bits unmodified" instead of "keeps the
>> masked bits unmodified"?
>> 
>> Regards,
>> Wanpeng Li 
>> 
>>> Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
>>> ---
>>> arch/x86/kvm/x86.c | 1 +
>>> 1 file changed, 1 insertion(+)
>>>
>>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>>> index 5f5edb6..ee42410 100644
>>> --- a/arch/x86/kvm/x86.c
>>> +++ b/arch/x86/kvm/x86.c
>>> @@ -236,6 +236,7 @@ void kvm_set_shared_msr(unsigned slot, u64 value, u64 mask)
>>>
>>> 	if (((value ^ smsr->values[slot].curr) & mask) == 0)
>>> 		return;
>>> +	value = (smsr->values[slot].curr & ~mask) | (value & mask);
>>> 	smsr->values[slot].curr = value;
>>> 	wrmsrl(shared_msrs_global.msrs[slot], value);
>>> 	if (!smsr->registered) {
>>> -- 
>>> 1.9.1
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe kvm" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] KVM: x86: Keep masked bits unmodified on kvm_set_shared_msr

[Nadav Amit]

[-- Attachment #1: Type: text/plain, Size: 1543 bytes --]


On Aug 22, 2014, at 7:13 AM, Wanpeng Li <wanpeng.li@linux.intel.com> wrote:

> Hi Paolo,
> On Thu, Aug 21, 2014 at 01:56:46PM +0200, Paolo Bonzini wrote:
>> Il 21/08/2014 10:05, Wanpeng Li ha scritto:
>>> Hi Nadav,
>>> On Wed, Aug 20, 2014 at 03:11:51PM +0300, Nadav Amit wrote:
>>>> Currently, when an msr is updated using kvm_set_shared_msr the masked bits are
>>>> zeroed.  This behavior is currently valid since the only MSR with partial mask
>>> 
>>> Why zeroed? vmx_vcpu_setup() set all mask to -1ull.
>> 
>> He meant they are passed as zero in the WRMSR but actually they're not
> 
> I fail to understand "they are passed as zero". Could you explain more
> in details? ;-)

Let’s ignore what I wrongly said. ;-)

Anyhow, the current semantics of kvm_set_shared_msr is that the unmasked bits of the value are compared against the current MSR value.
If they are not equal, the value is set to the MSR. This behaviour requires that the caller would set the value given as parameter according to the guest/host value.
If the caller naively does so (not setting the bits which are not in the mask), it would get zero in the unmasked bits in the MSR.
Currently, the only caller to kvm_set_shared_msr with a mask which is not full does it, and sets the unmasked bits of the value according to the host MSR value.
I argued that this behaviour is general, so the semantics of kvm_set_shared_msr should change. 
Paolo said that there are no current other callers to the function with a mask which is not full.

Nadav

[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 495 bytes --]