Re: [PATCH] kvm: nVMX: fix entry with pending interrupt if APICv is enabled
On 03/10/2018 16:53, Sean Christopherson wrote: > Isn't enable_apicv redundant with kvm_vcpu_apicv_active()? And since > getting RVI requires a VMREAD, I think it would make sense to only > fall into this code if !evaluate_pending_interrupts, e.g.: > > if (!evaluate_pending_interrupts && kvm_vcpu_apicv_active(vcpu)) > evaluate_pending_interrupts |= vmx_get_rvi() > 0; Yes, both suggestions make sense. I'll make it likely(!evaluate_pending_interrupts). Thanks to both you and Nikita for the quick review! Paolo
Re: [PATCH] kvm: nVMX: fix entry with pending interrupt if APICv is enabled
On 03/10/2018 16:53, Sean Christopherson wrote: > Isn't enable_apicv redundant with kvm_vcpu_apicv_active()? And since > getting RVI requires a VMREAD, I think it would make sense to only > fall into this code if !evaluate_pending_interrupts, e.g.: > > if (!evaluate_pending_interrupts && kvm_vcpu_apicv_active(vcpu)) > evaluate_pending_interrupts |= vmx_get_rvi() > 0; Yes, both suggestions make sense. I'll make it likely(!evaluate_pending_interrupts). Thanks to both you and Nikita for the quick review! Paolo
Re: [PATCH] kvm: nVMX: fix entry with pending interrupt if APICv is enabled
On Wed, Oct 03, 2018 at 01:47:47PM +0200, Paolo Bonzini wrote: > Commit b5861e5cf2fcf83031ea3e26b0a69d887adf7d21 introduced a check on > the interrupt-window and NMI-window CPU execution controls in order to > inject an external interrupt vmexit before the first guest instruction > executes. However, when APIC virtualization is enabled the host does not > need a vmexit in order to inject an interrupt at the next interrupt window; > instead, it just places the interrupt vector in RVI and the processor will > inject it as soon as possible. Therefore, on machines with APICv it is > not enough to check the CPU execution controls: the same scenario can also > happen if RVI>0. > > Fixes: b5861e5cf2fcf83031ea3e26b0a69d887adf7d21 > Cc: Nikita Leshchenko > Cc: Sean Christopherson > Cc: Liran Alon > Cc: Radim Krčmář > Signed-off-by: Paolo Bonzini > --- > arch/x86/kvm/vmx.c | 16 +++- > 1 file changed, 11 insertions(+), 5 deletions(-) > > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c > index 6ef2d5b139b9..c0c7689f0049 100644 > --- a/arch/x86/kvm/vmx.c > +++ b/arch/x86/kvm/vmx.c > @@ -10280,6 +10280,11 @@ static void vmx_hwapic_isr_update(struct kvm_vcpu > *vcpu, int max_isr) > } > } > > +static u8 vmx_get_rvi(void) > +{ > + return vmcs_read16(GUEST_INTR_STATUS) & 0xff; > +} > + > static void vmx_set_rvi(int vector) > { > u16 status; > @@ -12593,10 +12598,13 @@ static int enter_vmx_non_root_mode(struct kvm_vcpu > *vcpu, u32 *exit_qual) > struct vmcs12 *vmcs12 = get_vmcs12(vcpu); > bool from_vmentry = !!exit_qual; > u32 dummy_exit_qual; > - u32 vmcs01_cpu_exec_ctrl; > + bool evaluate_pending_interrupts; > int r = 0; > > - vmcs01_cpu_exec_ctrl = vmcs_read32(CPU_BASED_VM_EXEC_CONTROL); > + evaluate_pending_interrupts = vmcs_read32(CPU_BASED_VM_EXEC_CONTROL) & > + (CPU_BASED_VIRTUAL_INTR_PENDING | > CPU_BASED_VIRTUAL_NMI_PENDING); > + if (enable_apicv && kvm_vcpu_apicv_active(vcpu)) Isn't enable_apicv redundant with kvm_vcpu_apicv_active()? And since getting RVI requires a VMREAD, I think it would make sense to only fall into this code if !evaluate_pending_interrupts, e.g.: if (!evaluate_pending_interrupts && kvm_vcpu_apicv_active(vcpu)) evaluate_pending_interrupts |= vmx_get_rvi() > 0; > + evaluate_pending_interrupts |= vmx_get_rvi() > 0; > > enter_guest_mode(vcpu); > > @@ -12650,10 +12658,8 @@ static int enter_vmx_non_root_mode(struct kvm_vcpu > *vcpu, u32 *exit_qual) >* instead. Thus, we force L0 to perform pending event >* evaluation by requesting a KVM_REQ_EVENT. >*/ > - if (vmcs01_cpu_exec_ctrl & > - (CPU_BASED_VIRTUAL_INTR_PENDING | > CPU_BASED_VIRTUAL_NMI_PENDING)) { > + if (evaluate_pending_interrupts) > kvm_make_request(KVM_REQ_EVENT, vcpu); > - } > > /* >* Note no nested_vmx_succeed or nested_vmx_fail here. At this point > -- > 1.8.3.1 >
Re: [PATCH] kvm: nVMX: fix entry with pending interrupt if APICv is enabled
On Wed, Oct 03, 2018 at 01:47:47PM +0200, Paolo Bonzini wrote: > Commit b5861e5cf2fcf83031ea3e26b0a69d887adf7d21 introduced a check on > the interrupt-window and NMI-window CPU execution controls in order to > inject an external interrupt vmexit before the first guest instruction > executes. However, when APIC virtualization is enabled the host does not > need a vmexit in order to inject an interrupt at the next interrupt window; > instead, it just places the interrupt vector in RVI and the processor will > inject it as soon as possible. Therefore, on machines with APICv it is > not enough to check the CPU execution controls: the same scenario can also > happen if RVI>0. > > Fixes: b5861e5cf2fcf83031ea3e26b0a69d887adf7d21 > Cc: Nikita Leshchenko > Cc: Sean Christopherson > Cc: Liran Alon > Cc: Radim Krčmář > Signed-off-by: Paolo Bonzini > --- > arch/x86/kvm/vmx.c | 16 +++- > 1 file changed, 11 insertions(+), 5 deletions(-) > > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c > index 6ef2d5b139b9..c0c7689f0049 100644 > --- a/arch/x86/kvm/vmx.c > +++ b/arch/x86/kvm/vmx.c > @@ -10280,6 +10280,11 @@ static void vmx_hwapic_isr_update(struct kvm_vcpu > *vcpu, int max_isr) > } > } > > +static u8 vmx_get_rvi(void) > +{ > + return vmcs_read16(GUEST_INTR_STATUS) & 0xff; > +} > + > static void vmx_set_rvi(int vector) > { > u16 status; > @@ -12593,10 +12598,13 @@ static int enter_vmx_non_root_mode(struct kvm_vcpu > *vcpu, u32 *exit_qual) > struct vmcs12 *vmcs12 = get_vmcs12(vcpu); > bool from_vmentry = !!exit_qual; > u32 dummy_exit_qual; > - u32 vmcs01_cpu_exec_ctrl; > + bool evaluate_pending_interrupts; > int r = 0; > > - vmcs01_cpu_exec_ctrl = vmcs_read32(CPU_BASED_VM_EXEC_CONTROL); > + evaluate_pending_interrupts = vmcs_read32(CPU_BASED_VM_EXEC_CONTROL) & > + (CPU_BASED_VIRTUAL_INTR_PENDING | > CPU_BASED_VIRTUAL_NMI_PENDING); > + if (enable_apicv && kvm_vcpu_apicv_active(vcpu)) Isn't enable_apicv redundant with kvm_vcpu_apicv_active()? And since getting RVI requires a VMREAD, I think it would make sense to only fall into this code if !evaluate_pending_interrupts, e.g.: if (!evaluate_pending_interrupts && kvm_vcpu_apicv_active(vcpu)) evaluate_pending_interrupts |= vmx_get_rvi() > 0; > + evaluate_pending_interrupts |= vmx_get_rvi() > 0; > > enter_guest_mode(vcpu); > > @@ -12650,10 +12658,8 @@ static int enter_vmx_non_root_mode(struct kvm_vcpu > *vcpu, u32 *exit_qual) >* instead. Thus, we force L0 to perform pending event >* evaluation by requesting a KVM_REQ_EVENT. >*/ > - if (vmcs01_cpu_exec_ctrl & > - (CPU_BASED_VIRTUAL_INTR_PENDING | > CPU_BASED_VIRTUAL_NMI_PENDING)) { > + if (evaluate_pending_interrupts) > kvm_make_request(KVM_REQ_EVENT, vcpu); > - } > > /* >* Note no nested_vmx_succeed or nested_vmx_fail here. At this point > -- > 1.8.3.1 >
Re: [PATCH] kvm: nVMX: fix entry with pending interrupt if APICv is enabled
On 03/10/2018 16:36, Nikita Leshenko wrote: > On Wed, 2018-10-03 at 13:47 +0200, Paolo Bonzini wrote: >> Commit b5861e5cf2fcf83031ea3e26b0a69d887adf7d21 introduced a check on >> the interrupt-window and NMI-window CPU execution controls in order to >> inject an external interrupt vmexit before the first guest instruction >> executes. However, when APIC virtualization is enabled the host does not >> need a vmexit in order to inject an interrupt at the next interrupt window; >> instead, it just places the interrupt vector in RVI and the processor will >> inject it as soon as possible. Therefore, on machines with APICv it is >> not enough to check the CPU execution controls: the same scenario can also >> happen if RVI>0. >> >> Fixes: b5861e5cf2fcf83031ea3e26b0a69d887adf7d21 >> Cc: Nikita Leshchenko >> Cc: Sean Christopherson >> Cc: Liran Alon >> Cc: Radim Krčmář >> Signed-off-by: Paolo Bonzini >> --- >> arch/x86/kvm/vmx.c | 16 +++- >> 1 file changed, 11 insertions(+), 5 deletions(-) >> >> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c >> index 6ef2d5b139b9..c0c7689f0049 100644 >> --- a/arch/x86/kvm/vmx.c >> +++ b/arch/x86/kvm/vmx.c >> @@ -10280,6 +10280,11 @@ static void vmx_hwapic_isr_update(struct kvm_vcpu >> *vcpu, int max_isr) >> } >> } >> >> +static u8 vmx_get_rvi(void) >> +{ >> +return vmcs_read16(GUEST_INTR_STATUS) & 0xff; >> +} >> + >> static void vmx_set_rvi(int vector) >> { >> u16 status; >> @@ -12593,10 +12598,13 @@ static int enter_vmx_non_root_mode(struct kvm_vcpu >> *vcpu, u32 *exit_qual) >> struct vmcs12 *vmcs12 = get_vmcs12(vcpu); >> bool from_vmentry = !!exit_qual; >> u32 dummy_exit_qual; >> -u32 vmcs01_cpu_exec_ctrl; >> +bool evaluate_pending_interrupts; >> int r = 0; >> >> -vmcs01_cpu_exec_ctrl = vmcs_read32(CPU_BASED_VM_EXEC_CONTROL); >> +evaluate_pending_interrupts = vmcs_read32(CPU_BASED_VM_EXEC_CONTROL) & >> +(CPU_BASED_VIRTUAL_INTR_PENDING | >> CPU_BASED_VIRTUAL_NMI_PENDING); >> +if (enable_apicv && kvm_vcpu_apicv_active(vcpu)) >> +evaluate_pending_interrupts |= vmx_get_rvi() > 0; > > You should check for RVI > VPPR, similarly to how it is done in > vmx_guest_apic_has_interrupt(). > > Also, now that you introduced vmx_get_rvi(), it could be nice to use it > in vmx_guest_apic_has_interrupt() as well. > > Apart from that, looks good. > > Reviewed-by: Nikita Leshenko Nevermind, vPPR is of course available in vcpu->arch.apic since this is L1. So v2 is on the way. Paolo
Re: [PATCH] kvm: nVMX: fix entry with pending interrupt if APICv is enabled
On 03/10/2018 16:36, Nikita Leshenko wrote: > On Wed, 2018-10-03 at 13:47 +0200, Paolo Bonzini wrote: >> Commit b5861e5cf2fcf83031ea3e26b0a69d887adf7d21 introduced a check on >> the interrupt-window and NMI-window CPU execution controls in order to >> inject an external interrupt vmexit before the first guest instruction >> executes. However, when APIC virtualization is enabled the host does not >> need a vmexit in order to inject an interrupt at the next interrupt window; >> instead, it just places the interrupt vector in RVI and the processor will >> inject it as soon as possible. Therefore, on machines with APICv it is >> not enough to check the CPU execution controls: the same scenario can also >> happen if RVI>0. >> >> Fixes: b5861e5cf2fcf83031ea3e26b0a69d887adf7d21 >> Cc: Nikita Leshchenko >> Cc: Sean Christopherson >> Cc: Liran Alon >> Cc: Radim Krčmář >> Signed-off-by: Paolo Bonzini >> --- >> arch/x86/kvm/vmx.c | 16 +++- >> 1 file changed, 11 insertions(+), 5 deletions(-) >> >> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c >> index 6ef2d5b139b9..c0c7689f0049 100644 >> --- a/arch/x86/kvm/vmx.c >> +++ b/arch/x86/kvm/vmx.c >> @@ -10280,6 +10280,11 @@ static void vmx_hwapic_isr_update(struct kvm_vcpu >> *vcpu, int max_isr) >> } >> } >> >> +static u8 vmx_get_rvi(void) >> +{ >> +return vmcs_read16(GUEST_INTR_STATUS) & 0xff; >> +} >> + >> static void vmx_set_rvi(int vector) >> { >> u16 status; >> @@ -12593,10 +12598,13 @@ static int enter_vmx_non_root_mode(struct kvm_vcpu >> *vcpu, u32 *exit_qual) >> struct vmcs12 *vmcs12 = get_vmcs12(vcpu); >> bool from_vmentry = !!exit_qual; >> u32 dummy_exit_qual; >> -u32 vmcs01_cpu_exec_ctrl; >> +bool evaluate_pending_interrupts; >> int r = 0; >> >> -vmcs01_cpu_exec_ctrl = vmcs_read32(CPU_BASED_VM_EXEC_CONTROL); >> +evaluate_pending_interrupts = vmcs_read32(CPU_BASED_VM_EXEC_CONTROL) & >> +(CPU_BASED_VIRTUAL_INTR_PENDING | >> CPU_BASED_VIRTUAL_NMI_PENDING); >> +if (enable_apicv && kvm_vcpu_apicv_active(vcpu)) >> +evaluate_pending_interrupts |= vmx_get_rvi() > 0; > > You should check for RVI > VPPR, similarly to how it is done in > vmx_guest_apic_has_interrupt(). > > Also, now that you introduced vmx_get_rvi(), it could be nice to use it > in vmx_guest_apic_has_interrupt() as well. > > Apart from that, looks good. > > Reviewed-by: Nikita Leshenko Nevermind, vPPR is of course available in vcpu->arch.apic since this is L1. So v2 is on the way. Paolo
Re: [PATCH] kvm: nVMX: fix entry with pending interrupt if APICv is enabled
On 03/10/2018 16:36, Nikita Leshenko wrote: > On Wed, 2018-10-03 at 13:47 +0200, Paolo Bonzini wrote: >> Commit b5861e5cf2fcf83031ea3e26b0a69d887adf7d21 introduced a check on >> the interrupt-window and NMI-window CPU execution controls in order to >> inject an external interrupt vmexit before the first guest instruction >> executes. However, when APIC virtualization is enabled the host does not >> need a vmexit in order to inject an interrupt at the next interrupt window; >> instead, it just places the interrupt vector in RVI and the processor will >> inject it as soon as possible. Therefore, on machines with APICv it is >> not enough to check the CPU execution controls: the same scenario can also >> happen if RVI>0. >> >> Fixes: b5861e5cf2fcf83031ea3e26b0a69d887adf7d21 >> Cc: Nikita Leshchenko >> Cc: Sean Christopherson >> Cc: Liran Alon >> Cc: Radim Krčmář >> Signed-off-by: Paolo Bonzini >> --- >> arch/x86/kvm/vmx.c | 16 +++- >> 1 file changed, 11 insertions(+), 5 deletions(-) >> >> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c >> index 6ef2d5b139b9..c0c7689f0049 100644 >> --- a/arch/x86/kvm/vmx.c >> +++ b/arch/x86/kvm/vmx.c >> @@ -10280,6 +10280,11 @@ static void vmx_hwapic_isr_update(struct kvm_vcpu >> *vcpu, int max_isr) >> } >> } >> >> +static u8 vmx_get_rvi(void) >> +{ >> +return vmcs_read16(GUEST_INTR_STATUS) & 0xff; >> +} >> + >> static void vmx_set_rvi(int vector) >> { >> u16 status; >> @@ -12593,10 +12598,13 @@ static int enter_vmx_non_root_mode(struct kvm_vcpu >> *vcpu, u32 *exit_qual) >> struct vmcs12 *vmcs12 = get_vmcs12(vcpu); >> bool from_vmentry = !!exit_qual; >> u32 dummy_exit_qual; >> -u32 vmcs01_cpu_exec_ctrl; >> +bool evaluate_pending_interrupts; >> int r = 0; >> >> -vmcs01_cpu_exec_ctrl = vmcs_read32(CPU_BASED_VM_EXEC_CONTROL); >> +evaluate_pending_interrupts = vmcs_read32(CPU_BASED_VM_EXEC_CONTROL) & >> +(CPU_BASED_VIRTUAL_INTR_PENDING | >> CPU_BASED_VIRTUAL_NMI_PENDING); >> +if (enable_apicv && kvm_vcpu_apicv_active(vcpu)) >> +evaluate_pending_interrupts |= vmx_get_rvi() > 0; > > You should check for RVI > VPPR, similarly to how it is done in > vmx_guest_apic_has_interrupt(). True, however that would only result in a spurious KVM_REQ_EVENT. Unlike vmx_guest_apic_has_interrupt, we would have to check the TPR shadow and SVI instead of the nested APIC page, so you'd have one more VMREAD---and little benefit in the common case. What do you think about adding a comment? > Also, now that you introduced vmx_get_rvi(), it could be nice to use it > in vmx_guest_apic_has_interrupt() as well. True that. Paolo > Apart from that, looks good. > > Reviewed-by: Nikita Leshenko > > Thanks, > Nikita > >> >> enter_guest_mode(vcpu); >> >> @@ -12650,10 +12658,8 @@ static int enter_vmx_non_root_mode(struct kvm_vcpu >> *vcpu, u32 *exit_qual) >> * instead. Thus, we force L0 to perform pending event >> * evaluation by requesting a KVM_REQ_EVENT. >> */ >> -if (vmcs01_cpu_exec_ctrl & >> -(CPU_BASED_VIRTUAL_INTR_PENDING | >> CPU_BASED_VIRTUAL_NMI_PENDING)) { >> +if (evaluate_pending_interrupts) >> kvm_make_request(KVM_REQ_EVENT, vcpu); >> -} >> >> /* >> * Note no nested_vmx_succeed or nested_vmx_fail here. At this point >
Re: [PATCH] kvm: nVMX: fix entry with pending interrupt if APICv is enabled
On 03/10/2018 16:36, Nikita Leshenko wrote: > On Wed, 2018-10-03 at 13:47 +0200, Paolo Bonzini wrote: >> Commit b5861e5cf2fcf83031ea3e26b0a69d887adf7d21 introduced a check on >> the interrupt-window and NMI-window CPU execution controls in order to >> inject an external interrupt vmexit before the first guest instruction >> executes. However, when APIC virtualization is enabled the host does not >> need a vmexit in order to inject an interrupt at the next interrupt window; >> instead, it just places the interrupt vector in RVI and the processor will >> inject it as soon as possible. Therefore, on machines with APICv it is >> not enough to check the CPU execution controls: the same scenario can also >> happen if RVI>0. >> >> Fixes: b5861e5cf2fcf83031ea3e26b0a69d887adf7d21 >> Cc: Nikita Leshchenko >> Cc: Sean Christopherson >> Cc: Liran Alon >> Cc: Radim Krčmář >> Signed-off-by: Paolo Bonzini >> --- >> arch/x86/kvm/vmx.c | 16 +++- >> 1 file changed, 11 insertions(+), 5 deletions(-) >> >> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c >> index 6ef2d5b139b9..c0c7689f0049 100644 >> --- a/arch/x86/kvm/vmx.c >> +++ b/arch/x86/kvm/vmx.c >> @@ -10280,6 +10280,11 @@ static void vmx_hwapic_isr_update(struct kvm_vcpu >> *vcpu, int max_isr) >> } >> } >> >> +static u8 vmx_get_rvi(void) >> +{ >> +return vmcs_read16(GUEST_INTR_STATUS) & 0xff; >> +} >> + >> static void vmx_set_rvi(int vector) >> { >> u16 status; >> @@ -12593,10 +12598,13 @@ static int enter_vmx_non_root_mode(struct kvm_vcpu >> *vcpu, u32 *exit_qual) >> struct vmcs12 *vmcs12 = get_vmcs12(vcpu); >> bool from_vmentry = !!exit_qual; >> u32 dummy_exit_qual; >> -u32 vmcs01_cpu_exec_ctrl; >> +bool evaluate_pending_interrupts; >> int r = 0; >> >> -vmcs01_cpu_exec_ctrl = vmcs_read32(CPU_BASED_VM_EXEC_CONTROL); >> +evaluate_pending_interrupts = vmcs_read32(CPU_BASED_VM_EXEC_CONTROL) & >> +(CPU_BASED_VIRTUAL_INTR_PENDING | >> CPU_BASED_VIRTUAL_NMI_PENDING); >> +if (enable_apicv && kvm_vcpu_apicv_active(vcpu)) >> +evaluate_pending_interrupts |= vmx_get_rvi() > 0; > > You should check for RVI > VPPR, similarly to how it is done in > vmx_guest_apic_has_interrupt(). True, however that would only result in a spurious KVM_REQ_EVENT. Unlike vmx_guest_apic_has_interrupt, we would have to check the TPR shadow and SVI instead of the nested APIC page, so you'd have one more VMREAD---and little benefit in the common case. What do you think about adding a comment? > Also, now that you introduced vmx_get_rvi(), it could be nice to use it > in vmx_guest_apic_has_interrupt() as well. True that. Paolo > Apart from that, looks good. > > Reviewed-by: Nikita Leshenko > > Thanks, > Nikita > >> >> enter_guest_mode(vcpu); >> >> @@ -12650,10 +12658,8 @@ static int enter_vmx_non_root_mode(struct kvm_vcpu >> *vcpu, u32 *exit_qual) >> * instead. Thus, we force L0 to perform pending event >> * evaluation by requesting a KVM_REQ_EVENT. >> */ >> -if (vmcs01_cpu_exec_ctrl & >> -(CPU_BASED_VIRTUAL_INTR_PENDING | >> CPU_BASED_VIRTUAL_NMI_PENDING)) { >> +if (evaluate_pending_interrupts) >> kvm_make_request(KVM_REQ_EVENT, vcpu); >> -} >> >> /* >> * Note no nested_vmx_succeed or nested_vmx_fail here. At this point >
Re: [PATCH] kvm: nVMX: fix entry with pending interrupt if APICv is enabled
On Wed, 2018-10-03 at 13:47 +0200, Paolo Bonzini wrote: > Commit b5861e5cf2fcf83031ea3e26b0a69d887adf7d21 introduced a check on > the interrupt-window and NMI-window CPU execution controls in order to > inject an external interrupt vmexit before the first guest instruction > executes. However, when APIC virtualization is enabled the host does not > need a vmexit in order to inject an interrupt at the next interrupt window; > instead, it just places the interrupt vector in RVI and the processor will > inject it as soon as possible. Therefore, on machines with APICv it is > not enough to check the CPU execution controls: the same scenario can also > happen if RVI>0. > > Fixes: b5861e5cf2fcf83031ea3e26b0a69d887adf7d21 > Cc: Nikita Leshchenko > Cc: Sean Christopherson > Cc: Liran Alon > Cc: Radim Krčmář > Signed-off-by: Paolo Bonzini > --- > arch/x86/kvm/vmx.c | 16 +++- > 1 file changed, 11 insertions(+), 5 deletions(-) > > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c > index 6ef2d5b139b9..c0c7689f0049 100644 > --- a/arch/x86/kvm/vmx.c > +++ b/arch/x86/kvm/vmx.c > @@ -10280,6 +10280,11 @@ static void vmx_hwapic_isr_update(struct kvm_vcpu > *vcpu, int max_isr) > } > } > > +static u8 vmx_get_rvi(void) > +{ > + return vmcs_read16(GUEST_INTR_STATUS) & 0xff; > +} > + > static void vmx_set_rvi(int vector) > { > u16 status; > @@ -12593,10 +12598,13 @@ static int enter_vmx_non_root_mode(struct kvm_vcpu > *vcpu, u32 *exit_qual) > struct vmcs12 *vmcs12 = get_vmcs12(vcpu); > bool from_vmentry = !!exit_qual; > u32 dummy_exit_qual; > - u32 vmcs01_cpu_exec_ctrl; > + bool evaluate_pending_interrupts; > int r = 0; > > - vmcs01_cpu_exec_ctrl = vmcs_read32(CPU_BASED_VM_EXEC_CONTROL); > + evaluate_pending_interrupts = vmcs_read32(CPU_BASED_VM_EXEC_CONTROL) & > + (CPU_BASED_VIRTUAL_INTR_PENDING | > CPU_BASED_VIRTUAL_NMI_PENDING); > + if (enable_apicv && kvm_vcpu_apicv_active(vcpu)) > + evaluate_pending_interrupts |= vmx_get_rvi() > 0; You should check for RVI > VPPR, similarly to how it is done in vmx_guest_apic_has_interrupt(). Also, now that you introduced vmx_get_rvi(), it could be nice to use it in vmx_guest_apic_has_interrupt() as well. Apart from that, looks good. Reviewed-by: Nikita Leshenko Thanks, Nikita > > enter_guest_mode(vcpu); > > @@ -12650,10 +12658,8 @@ static int enter_vmx_non_root_mode(struct kvm_vcpu > *vcpu, u32 *exit_qual) >* instead. Thus, we force L0 to perform pending event >* evaluation by requesting a KVM_REQ_EVENT. >*/ > - if (vmcs01_cpu_exec_ctrl & > - (CPU_BASED_VIRTUAL_INTR_PENDING | > CPU_BASED_VIRTUAL_NMI_PENDING)) { > + if (evaluate_pending_interrupts) > kvm_make_request(KVM_REQ_EVENT, vcpu); > - } > > /* >* Note no nested_vmx_succeed or nested_vmx_fail here. At this point
Re: [PATCH] kvm: nVMX: fix entry with pending interrupt if APICv is enabled
On Wed, 2018-10-03 at 13:47 +0200, Paolo Bonzini wrote: > Commit b5861e5cf2fcf83031ea3e26b0a69d887adf7d21 introduced a check on > the interrupt-window and NMI-window CPU execution controls in order to > inject an external interrupt vmexit before the first guest instruction > executes. However, when APIC virtualization is enabled the host does not > need a vmexit in order to inject an interrupt at the next interrupt window; > instead, it just places the interrupt vector in RVI and the processor will > inject it as soon as possible. Therefore, on machines with APICv it is > not enough to check the CPU execution controls: the same scenario can also > happen if RVI>0. > > Fixes: b5861e5cf2fcf83031ea3e26b0a69d887adf7d21 > Cc: Nikita Leshchenko > Cc: Sean Christopherson > Cc: Liran Alon > Cc: Radim Krčmář > Signed-off-by: Paolo Bonzini > --- > arch/x86/kvm/vmx.c | 16 +++- > 1 file changed, 11 insertions(+), 5 deletions(-) > > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c > index 6ef2d5b139b9..c0c7689f0049 100644 > --- a/arch/x86/kvm/vmx.c > +++ b/arch/x86/kvm/vmx.c > @@ -10280,6 +10280,11 @@ static void vmx_hwapic_isr_update(struct kvm_vcpu > *vcpu, int max_isr) > } > } > > +static u8 vmx_get_rvi(void) > +{ > + return vmcs_read16(GUEST_INTR_STATUS) & 0xff; > +} > + > static void vmx_set_rvi(int vector) > { > u16 status; > @@ -12593,10 +12598,13 @@ static int enter_vmx_non_root_mode(struct kvm_vcpu > *vcpu, u32 *exit_qual) > struct vmcs12 *vmcs12 = get_vmcs12(vcpu); > bool from_vmentry = !!exit_qual; > u32 dummy_exit_qual; > - u32 vmcs01_cpu_exec_ctrl; > + bool evaluate_pending_interrupts; > int r = 0; > > - vmcs01_cpu_exec_ctrl = vmcs_read32(CPU_BASED_VM_EXEC_CONTROL); > + evaluate_pending_interrupts = vmcs_read32(CPU_BASED_VM_EXEC_CONTROL) & > + (CPU_BASED_VIRTUAL_INTR_PENDING | > CPU_BASED_VIRTUAL_NMI_PENDING); > + if (enable_apicv && kvm_vcpu_apicv_active(vcpu)) > + evaluate_pending_interrupts |= vmx_get_rvi() > 0; You should check for RVI > VPPR, similarly to how it is done in vmx_guest_apic_has_interrupt(). Also, now that you introduced vmx_get_rvi(), it could be nice to use it in vmx_guest_apic_has_interrupt() as well. Apart from that, looks good. Reviewed-by: Nikita Leshenko Thanks, Nikita > > enter_guest_mode(vcpu); > > @@ -12650,10 +12658,8 @@ static int enter_vmx_non_root_mode(struct kvm_vcpu > *vcpu, u32 *exit_qual) >* instead. Thus, we force L0 to perform pending event >* evaluation by requesting a KVM_REQ_EVENT. >*/ > - if (vmcs01_cpu_exec_ctrl & > - (CPU_BASED_VIRTUAL_INTR_PENDING | > CPU_BASED_VIRTUAL_NMI_PENDING)) { > + if (evaluate_pending_interrupts) > kvm_make_request(KVM_REQ_EVENT, vcpu); > - } > > /* >* Note no nested_vmx_succeed or nested_vmx_fail here. At this point
[PATCH] kvm: nVMX: fix entry with pending interrupt if APICv is enabled
Commit b5861e5cf2fcf83031ea3e26b0a69d887adf7d21 introduced a check on the interrupt-window and NMI-window CPU execution controls in order to inject an external interrupt vmexit before the first guest instruction executes. However, when APIC virtualization is enabled the host does not need a vmexit in order to inject an interrupt at the next interrupt window; instead, it just places the interrupt vector in RVI and the processor will inject it as soon as possible. Therefore, on machines with APICv it is not enough to check the CPU execution controls: the same scenario can also happen if RVI>0. Fixes: b5861e5cf2fcf83031ea3e26b0a69d887adf7d21 Cc: Nikita Leshchenko Cc: Sean Christopherson Cc: Liran Alon Cc: Radim Krčmář Signed-off-by: Paolo Bonzini --- arch/x86/kvm/vmx.c | 16 +++- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c index 6ef2d5b139b9..c0c7689f0049 100644 --- a/arch/x86/kvm/vmx.c +++ b/arch/x86/kvm/vmx.c @@ -10280,6 +10280,11 @@ static void vmx_hwapic_isr_update(struct kvm_vcpu *vcpu, int max_isr) } } +static u8 vmx_get_rvi(void) +{ + return vmcs_read16(GUEST_INTR_STATUS) & 0xff; +} + static void vmx_set_rvi(int vector) { u16 status; @@ -12593,10 +12598,13 @@ static int enter_vmx_non_root_mode(struct kvm_vcpu *vcpu, u32 *exit_qual) struct vmcs12 *vmcs12 = get_vmcs12(vcpu); bool from_vmentry = !!exit_qual; u32 dummy_exit_qual; - u32 vmcs01_cpu_exec_ctrl; + bool evaluate_pending_interrupts; int r = 0; - vmcs01_cpu_exec_ctrl = vmcs_read32(CPU_BASED_VM_EXEC_CONTROL); + evaluate_pending_interrupts = vmcs_read32(CPU_BASED_VM_EXEC_CONTROL) & + (CPU_BASED_VIRTUAL_INTR_PENDING | CPU_BASED_VIRTUAL_NMI_PENDING); + if (enable_apicv && kvm_vcpu_apicv_active(vcpu)) + evaluate_pending_interrupts |= vmx_get_rvi() > 0; enter_guest_mode(vcpu); @@ -12650,10 +12658,8 @@ static int enter_vmx_non_root_mode(struct kvm_vcpu *vcpu, u32 *exit_qual) * instead. Thus, we force L0 to perform pending event * evaluation by requesting a KVM_REQ_EVENT. */ - if (vmcs01_cpu_exec_ctrl & - (CPU_BASED_VIRTUAL_INTR_PENDING | CPU_BASED_VIRTUAL_NMI_PENDING)) { + if (evaluate_pending_interrupts) kvm_make_request(KVM_REQ_EVENT, vcpu); - } /* * Note no nested_vmx_succeed or nested_vmx_fail here. At this point -- 1.8.3.1
[PATCH] kvm: nVMX: fix entry with pending interrupt if APICv is enabled
Commit b5861e5cf2fcf83031ea3e26b0a69d887adf7d21 introduced a check on the interrupt-window and NMI-window CPU execution controls in order to inject an external interrupt vmexit before the first guest instruction executes. However, when APIC virtualization is enabled the host does not need a vmexit in order to inject an interrupt at the next interrupt window; instead, it just places the interrupt vector in RVI and the processor will inject it as soon as possible. Therefore, on machines with APICv it is not enough to check the CPU execution controls: the same scenario can also happen if RVI>0. Fixes: b5861e5cf2fcf83031ea3e26b0a69d887adf7d21 Cc: Nikita Leshchenko Cc: Sean Christopherson Cc: Liran Alon Cc: Radim Krčmář Signed-off-by: Paolo Bonzini --- arch/x86/kvm/vmx.c | 16 +++- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c index 6ef2d5b139b9..c0c7689f0049 100644 --- a/arch/x86/kvm/vmx.c +++ b/arch/x86/kvm/vmx.c @@ -10280,6 +10280,11 @@ static void vmx_hwapic_isr_update(struct kvm_vcpu *vcpu, int max_isr) } } +static u8 vmx_get_rvi(void) +{ + return vmcs_read16(GUEST_INTR_STATUS) & 0xff; +} + static void vmx_set_rvi(int vector) { u16 status; @@ -12593,10 +12598,13 @@ static int enter_vmx_non_root_mode(struct kvm_vcpu *vcpu, u32 *exit_qual) struct vmcs12 *vmcs12 = get_vmcs12(vcpu); bool from_vmentry = !!exit_qual; u32 dummy_exit_qual; - u32 vmcs01_cpu_exec_ctrl; + bool evaluate_pending_interrupts; int r = 0; - vmcs01_cpu_exec_ctrl = vmcs_read32(CPU_BASED_VM_EXEC_CONTROL); + evaluate_pending_interrupts = vmcs_read32(CPU_BASED_VM_EXEC_CONTROL) & + (CPU_BASED_VIRTUAL_INTR_PENDING | CPU_BASED_VIRTUAL_NMI_PENDING); + if (enable_apicv && kvm_vcpu_apicv_active(vcpu)) + evaluate_pending_interrupts |= vmx_get_rvi() > 0; enter_guest_mode(vcpu); @@ -12650,10 +12658,8 @@ static int enter_vmx_non_root_mode(struct kvm_vcpu *vcpu, u32 *exit_qual) * instead. Thus, we force L0 to perform pending event * evaluation by requesting a KVM_REQ_EVENT. */ - if (vmcs01_cpu_exec_ctrl & - (CPU_BASED_VIRTUAL_INTR_PENDING | CPU_BASED_VIRTUAL_NMI_PENDING)) { + if (evaluate_pending_interrupts) kvm_make_request(KVM_REQ_EVENT, vcpu); - } /* * Note no nested_vmx_succeed or nested_vmx_fail here. At this point -- 1.8.3.1