diff mbox series

[2/2] KVM: nVMX: Update vmcs01 TPR_THRESHOLD if L2 changed L1 TPR

Message ID 20191111123055.93270-3-liran.alon@oracle.com (mailing list archive)
State New, archived
Headers show
Series KVM: nVMX: Update vmcs01 TPR_THRESHOLD if L2 changed L1 TPR | expand

Commit Message

Liran Alon Nov. 11, 2019, 12:30 p.m. UTC 
When L1 don't use TPR-Shadow to run L2, L0 configures vmcs02 without
TPR-Shadow and install intercepts on CR8 access (load and store).

If L1 do not intercept L2 CR8 access, L0 intercepts on those accesses
will emulate load/store on L1's LAPIC TPR. If in this case L2 lowers
TPR such that there is now an injectable interrupt to L1,
apic_update_ppr() will request a KVM_REQ_EVENT which will trigger a call
to update_cr8_intercept() to update TPR-Threshold to highest pending IRR
priority.

However, this update to TPR-Threshold is done while active vmcs is
vmcs02 instead of vmcs01. Thus, when later at some point L0 will
emulate an exit from L2 to L1, L1 will still run with high
TPR-Threshold. This will result in every VMEntry to L1 to immediately
exit on TPR_BELOW_THRESHOLD and continue to do so infinitely until
some condition will cause KVM_REQ_EVENT to be set.
(Note that TPR_BELOW_THRESHOLD exit handler do not set KVM_REQ_EVENT
until apic_update_ppr() will notice a new injectable interrupt for PPR)

To fix this issue, change update_cr8_intercept() such that if L2 lowers
L1's TPR in a way that requires to lower L1's TPR-Threshold, save update
to TPR-Threshold and apply it to vmcs01 when L0 emulates an exit from
L2 to L1.

Reviewed-by: Joao Martins <joao.m.martins@oracle.com>
Signed-off-by: Liran Alon <liran.alon@oracle.com>
---
 arch/x86/kvm/vmx/nested.c | 6 +++++-
 arch/x86/kvm/vmx/vmx.c    | 6 +++++-
 arch/x86/kvm/vmx/vmx.h    | 3 +++
 3 files changed, 13 insertions(+), 2 deletions(-)

Comments

Paolo Bonzini Nov. 11, 2019, 3:02 p.m. UTC | #1 
On 11/11/19 13:30, Liran Alon wrote:
> When L1 don't use TPR-Shadow to run L2, L0 configures vmcs02 without
> TPR-Shadow and install intercepts on CR8 access (load and store).
> 
> If L1 do not intercept L2 CR8 access, L0 intercepts on those accesses
> will emulate load/store on L1's LAPIC TPR. If in this case L2 lowers
> TPR such that there is now an injectable interrupt to L1,
> apic_update_ppr() will request a KVM_REQ_EVENT which will trigger a call
> to update_cr8_intercept() to update TPR-Threshold to highest pending IRR
> priority.
> 
> However, this update to TPR-Threshold is done while active vmcs is
> vmcs02 instead of vmcs01. Thus, when later at some point L0 will
> emulate an exit from L2 to L1, L1 will still run with high
> TPR-Threshold. This will result in every VMEntry to L1 to immediately
> exit on TPR_BELOW_THRESHOLD and continue to do so infinitely until
> some condition will cause KVM_REQ_EVENT to be set.
> (Note that TPR_BELOW_THRESHOLD exit handler do not set KVM_REQ_EVENT
> until apic_update_ppr() will notice a new injectable interrupt for PPR)
> 
> To fix this issue, change update_cr8_intercept() such that if L2 lowers
> L1's TPR in a way that requires to lower L1's TPR-Threshold, save update
> to TPR-Threshold and apply it to vmcs01 when L0 emulates an exit from
> L2 to L1.

Can you explain why the write shouldn't be done to vmcs02 as well?

Paolo

> -	vmcs_write32(TPR_THRESHOLD, tpr_threshold);
> +
> +	if (is_guest_mode(vcpu))
> +		to_vmx(vcpu)->nested.l1_tpr_threshold = tpr_threshold;
> +	else
> +		vmcs_write32(TPR_THRESHOLD, tpr_threshold);
>  }
>  
>  void vmx_set_virtual_apic_mode(struct kvm_vcpu *vcpu)
> diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
> index bee16687dc0b..43331dfafffe 100644
> --- a/arch/x86/kvm/vmx/vmx.h
> +++ b/arch/x86/kvm/vmx/vmx.h
> @@ -167,6 +167,9 @@ struct nested_vmx {
>  	u64 vmcs01_debugctl;
>  	u64 vmcs01_guest_bndcfgs;
>  
> +	/* to migrate it to L1 if L2 writes to L1's CR8 directly */
> +	int l1_tpr_threshold;
> +
>  	u16 vpid02;
>  	u16 last_vpid;
>  
>
Liran Alon Nov. 11, 2019, 3:24 p.m. UTC | #2 
> On 11 Nov 2019, at 17:02, Paolo Bonzini <pbonzini@redhat.com> wrote:
> 
> On 11/11/19 13:30, Liran Alon wrote:
>> When L1 don't use TPR-Shadow to run L2, L0 configures vmcs02 without
>> TPR-Shadow and install intercepts on CR8 access (load and store).
>> 
>> If L1 do not intercept L2 CR8 access, L0 intercepts on those accesses
>> will emulate load/store on L1's LAPIC TPR. If in this case L2 lowers
>> TPR such that there is now an injectable interrupt to L1,
>> apic_update_ppr() will request a KVM_REQ_EVENT which will trigger a call
>> to update_cr8_intercept() to update TPR-Threshold to highest pending IRR
>> priority.
>> 
>> However, this update to TPR-Threshold is done while active vmcs is
>> vmcs02 instead of vmcs01. Thus, when later at some point L0 will
>> emulate an exit from L2 to L1, L1 will still run with high
>> TPR-Threshold. This will result in every VMEntry to L1 to immediately
>> exit on TPR_BELOW_THRESHOLD and continue to do so infinitely until
>> some condition will cause KVM_REQ_EVENT to be set.
>> (Note that TPR_BELOW_THRESHOLD exit handler do not set KVM_REQ_EVENT
>> until apic_update_ppr() will notice a new injectable interrupt for PPR)
>> 
>> To fix this issue, change update_cr8_intercept() such that if L2 lowers
>> L1's TPR in a way that requires to lower L1's TPR-Threshold, save update
>> to TPR-Threshold and apply it to vmcs01 when L0 emulates an exit from
>> L2 to L1.
> 
> Can you explain why the write shouldn't be done to vmcs02 as well?
> 
> Paolo

Because when L1 don’t use TPR-Shadow, L0 configures vmcs02 without TPR-Shadow.
Thus, writing to vmcs02->tpr_threshold doesn’t have any effect.

If l1 do use TPR-Shadow, then VMX’s update_cr8_intercept() doesn’t write to vmcs at all,
because it means L1 defines a vTPR for L2 and thus doesn’t provide it direct access to L1 TPR.

-Liran

> 
>> -	vmcs_write32(TPR_THRESHOLD, tpr_threshold);
>> +
>> +	if (is_guest_mode(vcpu))
>> +		to_vmx(vcpu)->nested.l1_tpr_threshold = tpr_threshold;
>> +	else
>> +		vmcs_write32(TPR_THRESHOLD, tpr_threshold);
>> }
>> 
>> void vmx_set_virtual_apic_mode(struct kvm_vcpu *vcpu)
>> diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
>> index bee16687dc0b..43331dfafffe 100644
>> --- a/arch/x86/kvm/vmx/vmx.h
>> +++ b/arch/x86/kvm/vmx/vmx.h
>> @@ -167,6 +167,9 @@ struct nested_vmx {
>> 	u64 vmcs01_debugctl;
>> 	u64 vmcs01_guest_bndcfgs;
>> 
>> +	/* to migrate it to L1 if L2 writes to L1's CR8 directly */
>> +	int l1_tpr_threshold;
>> +
>> 	u16 vpid02;
>> 	u16 last_vpid;
>> 
>> 
>
Paolo Bonzini Nov. 11, 2019, 3:59 p.m. UTC | #3 
On 11/11/19 16:24, Liran Alon wrote:
> Because when L1 don’t use TPR-Shadow, L0 configures vmcs02 without TPR-Shadow.
> Thus, writing to vmcs02->tpr_threshold doesn’t have any effect.

Uh, sorry that was obvious:

 	if (exec_control & CPU_BASED_TPR_SHADOW)
 		vmcs_write32(TPR_THRESHOLD, vmcs12->tpr_threshold);

Paolo

> If l1 do use TPR-Shadow, then VMX’s update_cr8_intercept() doesn’t write to vmcs at all,
> because it means L1 defines a vTPR for L2 and thus doesn’t provide it direct access to L1 TPR.
Paolo Bonzini Nov. 11, 2019, 4:07 p.m. UTC | #4 
On 11/11/19 16:24, Liran Alon wrote:
>> Can you explain why the write shouldn't be done to vmcs02 as well?
>
> Because when L1 don’t use TPR-Shadow, L0 configures vmcs02 without TPR-Shadow.
> Thus, writing to vmcs02->tpr_threshold doesn’t have any effect.
> 
> If l1 do use TPR-Shadow, then VMX’s update_cr8_intercept() doesn’t write to vmcs at all,
> because it means L1 defines a vTPR for L2 and thus doesn’t provide it direct access to L1 TPR.

But I'm still not sure about another aspect of the patch.  The write to
vmcs01 can be done even if TPR_SHADOW was set in vmcs12, because no one
takes care of clearing vmx->nested.l1_tpr_threshold.  Should
"vmx->nested.l1_tpr_threshold = -1;" be outside the if?

Also, what happens to_vmx(vcpu)->nested.l1_tpr_threshold if the guest is
migrated while L2 is running without TPR shadow?  Perhaps it would be
easier to just rerun update_cr8_intercept on nested_vmx_vmexit.

Paolo
Liran Alon Nov. 11, 2019, 4:17 p.m. UTC | #5 
> On 11 Nov 2019, at 18:07, Paolo Bonzini <pbonzini@redhat.com> wrote:
> 
> On 11/11/19 16:24, Liran Alon wrote:
>>> Can you explain why the write shouldn't be done to vmcs02 as well?
>> 
>> Because when L1 don’t use TPR-Shadow, L0 configures vmcs02 without TPR-Shadow.
>> Thus, writing to vmcs02->tpr_threshold doesn’t have any effect.
>> 
>> If l1 do use TPR-Shadow, then VMX’s update_cr8_intercept() doesn’t write to vmcs at all,
>> because it means L1 defines a vTPR for L2 and thus doesn’t provide it direct access to L1 TPR.
> 
> But I'm still not sure about another aspect of the patch.  The write to
> vmcs01 can be done even if TPR_SHADOW was set in vmcs12, because no one
> takes care of clearing vmx->nested.l1_tpr_threshold.  Should
> "vmx->nested.l1_tpr_threshold = -1;" be outside the if?

If I understand you correctly, you refer to the case where L1 first enters L2 without TPR-Shadow,
then L2 lowers L1 TPR directly (which load vmx->nested.l1_tpr_threshold with value), then an
emualted exit happen from L2 to L1 which writes to vmcs01->tpr_threshold the value of
vmx->nested.l1_tpr_threshold. Then L1 enters again L2 but this time with TPR-Shadow and
prepare_vmcs02_early() doesn’t clear vmx->nested.l1_tpr_threshold which will cause next
exit from L2 to L1 to wrongly write the value of vmx->nested.l1_tpr_threshold to vmcs01->tpr_threshold.

So yes I think you are right. Good catch.
We should move vmx->nested.l1_tpr_threshold = -1; outside of the if.
Should I send v2 or will you change on apply?

> 
> Also, what happens to_vmx(vcpu)->nested.l1_tpr_threshold if the guest is
> migrated while L2 is running without TPR shadow?  Perhaps it would be
> easier to just rerun update_cr8_intercept on nested_vmx_vmexit.
> 

On restore of state during migration, kvm_apic_set_state() must be called which
will also request a KVM_REQ_EVENT which will make sure to call update_cr8_intercept().
If vCPU is currently in guest-mode, this should update vmx->nested.l1_tpr_threshold.

-Liran

> Paolo
Paolo Bonzini Nov. 11, 2019, 4:50 p.m. UTC | #6 
On 11/11/19 17:17, Liran Alon wrote:
> If I understand you correctly, you refer to the case where L1 first enters L2 without TPR-Shadow,
> then L2 lowers L1 TPR directly (which load vmx->nested.l1_tpr_threshold with value), then an
> emualted exit happen from L2 to L1 which writes to vmcs01->tpr_threshold the value of
> vmx->nested.l1_tpr_threshold. Then L1 enters again L2 but this time with TPR-Shadow and
> prepare_vmcs02_early() doesn’t clear vmx->nested.l1_tpr_threshold which will cause next
> exit from L2 to L1 to wrongly write the value of vmx->nested.l1_tpr_threshold to vmcs01->tpr_threshold.
> 
> So yes I think you are right. Good catch.
> We should move vmx->nested.l1_tpr_threshold = -1; outside of the if.
> Should I send v2 or will you change on apply?

I can do that too.

>> Also, what happens to_vmx(vcpu)->nested.l1_tpr_threshold if the guest is
>> migrated while L2 is running without TPR shadow?  Perhaps it would be
>> easier to just rerun update_cr8_intercept on nested_vmx_vmexit.
>>
> On restore of state during migration, kvm_apic_set_state() must be called which
> will also request a KVM_REQ_EVENT which will make sure to call update_cr8_intercept().
> If vCPU is currently in guest-mode, this should update vmx->nested.l1_tpr_threshold.

Okay, that makes sense.  I was half-sure that update_cr8_intercept()
would be called, but I couldn't think of the exact path.

Paolo
diff mbox series

Patch

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index 2c4336ac7576..9197f6631c02 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -2075,11 +2075,13 @@  static void prepare_vmcs02_early(struct vcpu_vmx *vmx, struct vmcs12 *vmcs12)
 
 	if (exec_control & CPU_BASED_TPR_SHADOW)
 		vmcs_write32(TPR_THRESHOLD, vmcs12->tpr_threshold);
+	else {
+		vmx->nested.l1_tpr_threshold = -1;
 #ifdef CONFIG_X86_64
-	else
 		exec_control |= CPU_BASED_CR8_LOAD_EXITING |
 				CPU_BASED_CR8_STORE_EXITING;
 #endif
+	}
 
 	/*
 	 * A vmexit (to either L1 hypervisor or L0 userspace) is always needed
@@ -4113,6 +4115,8 @@  void nested_vmx_vmexit(struct kvm_vcpu *vcpu, u32 exit_reason,
 	vmcs_write32(VM_EXIT_MSR_LOAD_COUNT, vmx->msr_autoload.host.nr);
 	vmcs_write32(VM_ENTRY_MSR_LOAD_COUNT, vmx->msr_autoload.guest.nr);
 	vmcs_write64(TSC_OFFSET, vcpu->arch.tsc_offset);
+	if (vmx->nested.l1_tpr_threshold != -1)
+		vmcs_write32(TPR_THRESHOLD, vmx->nested.l1_tpr_threshold);
 
 	if (kvm_has_tsc_control)
 		decache_tsc_multiplier(vmx);
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index d5742378d031..c4667631a14f 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -6020,7 +6020,11 @@  static void update_cr8_intercept(struct kvm_vcpu *vcpu, int tpr, int irr)
 		return;
 
 	tpr_threshold = ((irr == -1) || (tpr < irr)) ? 0 : irr;
-	vmcs_write32(TPR_THRESHOLD, tpr_threshold);
+
+	if (is_guest_mode(vcpu))
+		to_vmx(vcpu)->nested.l1_tpr_threshold = tpr_threshold;
+	else
+		vmcs_write32(TPR_THRESHOLD, tpr_threshold);
 }
 
 void vmx_set_virtual_apic_mode(struct kvm_vcpu *vcpu)
diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
index bee16687dc0b..43331dfafffe 100644
--- a/arch/x86/kvm/vmx/vmx.h
+++ b/arch/x86/kvm/vmx/vmx.h
@@ -167,6 +167,9 @@  struct nested_vmx {
 	u64 vmcs01_debugctl;
 	u64 vmcs01_guest_bndcfgs;
 
+	/* to migrate it to L1 if L2 writes to L1's CR8 directly */
+	int l1_tpr_threshold;
+
 	u16 vpid02;
 	u16 last_vpid;