These two variables should check whether SPEC_CTRL and PRED_CMD are
supposed to be passed through to L2 guests or not. While
msr_write_intercepted_l01 would return 'true' if it is not passed through.
So just invert the result of msr_write_intercepted_l01 to implement the
correct semantics.
Fixes: 086e7d4118cc ("KVM: VMX: Allow direct access to MSR_IA32_SPEC_CTRL")
Signed-off-by: KarimAllah Ahmed <[email protected]>
Cc: Paolo Bonzini <[email protected]>
Cc: Radim Krčmář <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
arch/x86/kvm/vmx.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index bee4c49..599179b 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -10219,8 +10219,8 @@ static inline bool nested_vmx_merge_msr_bitmap(struct kvm_vcpu *vcpu,
* updated to reflect this when L1 (or its L2s) actually write to
* the MSR.
*/
- bool pred_cmd = msr_write_intercepted_l01(vcpu, MSR_IA32_PRED_CMD);
- bool spec_ctrl = msr_write_intercepted_l01(vcpu, MSR_IA32_SPEC_CTRL);
+ bool pred_cmd = !msr_write_intercepted_l01(vcpu, MSR_IA32_PRED_CMD);
+ bool spec_ctrl = !msr_write_intercepted_l01(vcpu, MSR_IA32_SPEC_CTRL);
if (!nested_cpu_has_virt_x2apic_mode(vmcs12) &&
!pred_cmd && !spec_ctrl)
--
2.7.4
... otherwise we will just be running with the L1 MSR BITMAP!
It does not seem that we ever update the MSR_BITMAP when the nested guest
is running. The only place where we update the MSR_BITMAP field in VMCS is
for the L1 guest!
Signed-off-by: KarimAllah Ahmed <[email protected]>
Cc: Paolo Bonzini <[email protected]>
Cc: Radim Krčmář <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
arch/x86/kvm/vmx.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 91e3539..f40be10 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -10589,6 +10589,9 @@ static int prepare_vmcs02(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12,
vmcs_writel(GUEST_GDTR_BASE, vmcs12->guest_gdtr_base);
vmcs_writel(GUEST_IDTR_BASE, vmcs12->guest_idtr_base);
+ if (cpu_has_vmx_msr_bitmap())
+ vmcs_write64(MSR_BITMAP, __pa(vmx->nested.vmcs02.msr_bitmap));
+
if (from_vmentry &&
(vmcs12->vm_entry_controls & VM_ENTRY_LOAD_DEBUG_CONTROLS)) {
kvm_set_dr(vcpu, 7, vmcs12->guest_dr7);
--
2.7.4
We either clear the CPU_BASED_USE_MSR_BITMAPS and end up intercepting all
MSR accesses or create a valid L02 MSR bitmap and use that. This decision
has to be made every time we evaluate whether we are going to generate the
L02 MSR bitmap.
Before commit 086e7d4118cc ("KVM: VMX: Allow direct access to MSR_IA32_SPEC_CTRL")
this was probably OK since the decision was always identical. This is no
longer the case now since the MSR bitmap might actually change once we
decide to not intercept SPEC_CTRL and PRED_CMD.
Signed-off-by: KarimAllah Ahmed <[email protected]>
Cc: Paolo Bonzini <[email protected]>
Cc: Radim Krčmář <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
arch/x86/kvm/vmx.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 599179b..91e3539 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -10130,7 +10130,8 @@ static void nested_get_vmcs12_pages(struct kvm_vcpu *vcpu,
if (cpu_has_vmx_msr_bitmap() &&
nested_cpu_has(vmcs12, CPU_BASED_USE_MSR_BITMAPS) &&
nested_vmx_merge_msr_bitmap(vcpu, vmcs12))
- ;
+ vmcs_set_bits(CPU_BASED_VM_EXEC_CONTROL,
+ CPU_BASED_USE_MSR_BITMAPS);
else
vmcs_clear_bits(CPU_BASED_VM_EXEC_CONTROL,
CPU_BASED_USE_MSR_BITMAPS);
--
2.7.4
On Thu, Feb 8, 2018 at 2:53 PM, KarimAllah Ahmed <[email protected]> wrote:
> These two variables should check whether SPEC_CTRL and PRED_CMD are
> supposed to be passed through to L2 guests or not. While
> msr_write_intercepted_l01 would return 'true' if it is not passed through.
>
> So just invert the result of msr_write_intercepted_l01 to implement the
> correct semantics.
>
> Fixes: 086e7d4118cc ("KVM: VMX: Allow direct access to MSR_IA32_SPEC_CTRL")
> Signed-off-by: KarimAllah Ahmed <[email protected]>
> Cc: Paolo Bonzini <[email protected]>
> Cc: Radim Krčmář <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
Reviewed-by: Jim Mattson <[email protected]>
On Thu, Feb 8, 2018 at 2:53 PM, KarimAllah Ahmed <[email protected]> wrote:
> ... otherwise we will just be running with the L1 MSR BITMAP!
>
> It does not seem that we ever update the MSR_BITMAP when the nested guest
> is running. The only place where we update the MSR_BITMAP field in VMCS is
> for the L1 guest!
>
> Signed-off-by: KarimAllah Ahmed <[email protected]>
> Cc: Paolo Bonzini <[email protected]>
> Cc: Radim Krčmář <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> ---
> arch/x86/kvm/vmx.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 91e3539..f40be10 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -10589,6 +10589,9 @@ static int prepare_vmcs02(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12,
> vmcs_writel(GUEST_GDTR_BASE, vmcs12->guest_gdtr_base);
> vmcs_writel(GUEST_IDTR_BASE, vmcs12->guest_idtr_base);
>
> + if (cpu_has_vmx_msr_bitmap())
> + vmcs_write64(MSR_BITMAP, __pa(vmx->nested.vmcs02.msr_bitmap));
> +
This is already in David's "[PATCH 5/9] KVM: VMX: make MSR bitmaps
per-VCPU," isn't it?
> if (from_vmentry &&
> (vmcs12->vm_entry_controls & VM_ENTRY_LOAD_DEBUG_CONTROLS)) {
> kvm_set_dr(vcpu, 7, vmcs12->guest_dr7);
> --
> 2.7.4
>
On 02/10/2018 12:26 AM, Jim Mattson wrote:
> On Thu, Feb 8, 2018 at 2:53 PM, KarimAllah Ahmed <[email protected]> wrote:
>> ... otherwise we will just be running with the L1 MSR BITMAP!
>>
>> It does not seem that we ever update the MSR_BITMAP when the nested guest
>> is running. The only place where we update the MSR_BITMAP field in VMCS is
>> for the L1 guest!
>>
>> Signed-off-by: KarimAllah Ahmed <[email protected]>
>> Cc: Paolo Bonzini <[email protected]>
>> Cc: Radim Krčmář <[email protected]>
>> Cc: [email protected]
>> Cc: [email protected]
>> ---
>> arch/x86/kvm/vmx.c | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>> index 91e3539..f40be10 100644
>> --- a/arch/x86/kvm/vmx.c
>> +++ b/arch/x86/kvm/vmx.c
>> @@ -10589,6 +10589,9 @@ static int prepare_vmcs02(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12,
>> vmcs_writel(GUEST_GDTR_BASE, vmcs12->guest_gdtr_base);
>> vmcs_writel(GUEST_IDTR_BASE, vmcs12->guest_idtr_base);
>>
>> + if (cpu_has_vmx_msr_bitmap())
>> + vmcs_write64(MSR_BITMAP, __pa(vmx->nested.vmcs02.msr_bitmap));
>> +
>
> This is already in David's "[PATCH 5/9] KVM: VMX: make MSR bitmaps
> per-VCPU," isn't it?
I assume you are referring to this:
https://patchwork.kernel.org/patch/10194819/
.. which is now:
commit 904e14fb7cb9 ("KVM: VMX: make MSR bitmaps per-VCPU")
right?
If this is the case, then I do not see where the MSR_BITMAP is being
updated here. In fact, would not this be the commit that actually broke
it?
Now MSR_BITMAP is only set in vmx_vcpu_setup:
if (cpu_has_vmx_msr_bitmap())
vmcs_write64(MSR_BITMAP, __pa(vmx->vmcs01.msr_bitmap));
>
>> if (from_vmentry &&
>> (vmcs12->vm_entry_controls & VM_ENTRY_LOAD_DEBUG_CONTROLS)) {
>> kvm_set_dr(vcpu, 7, vmcs12->guest_dr7);
>> --
>> 2.7.4
>>
>
Amazon Development Center Germany GmbH
Berlin - Dresden - Aachen
main office: Krausenstr. 38, 10117 Berlin
Geschaeftsfuehrer: Dr. Ralf Herbrich, Christian Schlaeger
Ust-ID: DE289237879
Eingetragen am Amtsgericht Charlottenburg HRB 149173 B
On Fri, Feb 9, 2018 at 3:41 PM, KarimAllah Ahmed <[email protected]> wrote:
> I assume you are referring to this:
>
> https://patchwork.kernel.org/patch/10194819/
>
> .. which is now:
>
> commit 904e14fb7cb9 ("KVM: VMX: make MSR bitmaps per-VCPU")
>
> right?
>
> If this is the case, then I do not see where the MSR_BITMAP is being
> updated here. In fact, would not this be the commit that actually broke
> it?
I'm referring to
<[email protected]>, which has:
@@ -10043,6 +9954,9 @@ static void prepare_vmcs02(struct kvm_vcpu
*vcpu, struct vmcs12 *vmcs12)
if (kvm_has_tsc_control)
decache_tsc_multiplier(vmx);
+ if (cpu_has_vmx_msr_bitmap())
+ vmcs_write64(MSR_BITMAP, __pa(vmx->nested.vmcs02.msr_bitmap));
+
if (enable_vpid) {
/*
* There is no direct mapping between vpid02 and vpid12, the
On 02/10/2018 12:57 AM, Jim Mattson wrote:
> On Fri, Feb 9, 2018 at 3:41 PM, KarimAllah Ahmed <[email protected]> wrote:
>
>> I assume you are referring to this:
>>
>> https://patchwork.kernel.org/patch/10194819/
>>
>> .. which is now:
>>
>> commit 904e14fb7cb9 ("KVM: VMX: make MSR bitmaps per-VCPU")
>>
>> right?
>>
>> If this is the case, then I do not see where the MSR_BITMAP is being
>> updated here. In fact, would not this be the commit that actually broke
>> it?
>
> I'm referring to
> <[email protected]>, which has:
>
> @@ -10043,6 +9954,9 @@ static void prepare_vmcs02(struct kvm_vcpu
> *vcpu, struct vmcs12 *vmcs12)
> if (kvm_has_tsc_control)
> decache_tsc_multiplier(vmx);
>
> + if (cpu_has_vmx_msr_bitmap())
> + vmcs_write64(MSR_BITMAP, __pa(vmx->nested.vmcs02.msr_bitmap));
> +
> if (enable_vpid) {
> /*
> * There is no direct mapping between vpid02 and vpid12, the
>
Ooops, My bad! I must have ingested and old version of this commit that
did not have this hunk! Now actually looking at the upstream commit and
the backports from David, it is indeed there.
Sorry for the noise, please ignore this patch :)
Amazon Development Center Germany GmbH
Berlin - Dresden - Aachen
main office: Krausenstr. 38, 10117 Berlin
Geschaeftsfuehrer: Dr. Ralf Herbrich, Christian Schlaeger
Ust-ID: DE289237879
Eingetragen am Amtsgericht Charlottenburg HRB 149173 B
On Fri, 2018-02-09 at 15:15 -0800, Jim Mattson wrote:
> On Thu, Feb 8, 2018 at 2:53 PM, KarimAllah Ahmed <[email protected]> wrote:
> >
> > These two variables should check whether SPEC_CTRL and PRED_CMD are
> > supposed to be passed through to L2 guests or not. While
> > msr_write_intercepted_l01 would return 'true' if it is not passed through.
> >
> > So just invert the result of msr_write_intercepted_l01 to implement the
> > correct semantics.
> >
> > Fixes: 086e7d4118cc ("KVM: VMX: Allow direct access to MSR_IA32_SPEC_CTRL")
> > Signed-off-by: KarimAllah Ahmed <[email protected]>
> > Cc: Paolo Bonzini <[email protected]>
> > Cc: Radim Krčmář <[email protected]>
> > Cc: [email protected]
> > Cc: [email protected]
> Reviewed-by: Jim Mattson <[email protected]>
We should take this (and 2/3 but not 3/3) through tip/x86/pti too,
right?