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 <[email protected]> >> Cc: Sean Christopherson <[email protected]> >> Cc: Liran Alon <[email protected]> >> Cc: Radim Krčmář <[email protected]> >> Signed-off-by: Paolo Bonzini <[email protected]> >> --- >> 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 <[email protected]> > > 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 >
