Re: [patch 4/4] KVM: VMX: update vcpu posted-interrupt descriptor when assigning device

[Marcelo Tosatti <mtosatti@redhat.com>]

On Tue, May 11, 2021 at 08:51:24PM -0300, Marcelo Tosatti wrote:
> On Tue, May 11, 2021 at 05:35:41PM -0400, Peter Xu wrote:
> > On Tue, May 11, 2021 at 02:18:10PM -0300, Marcelo Tosatti wrote:
> > > On Tue, May 11, 2021 at 12:19:56PM -0400, Peter Xu wrote:
> > > > On Tue, May 11, 2021 at 11:51:57AM -0300, Marcelo Tosatti wrote:
> > > > > On Tue, May 11, 2021 at 10:39:11AM -0400, Peter Xu wrote:
> > > > > > On Fri, May 07, 2021 at 07:08:31PM -0300, Marcelo Tosatti wrote:
> > > > > > > > Wondering whether we should add a pi_test_on() check in kvm_vcpu_has_events()
> > > > > > > > somehow, so that even without customized ->vcpu_check_block we should be able
> > > > > > > > to break the block loop (as kvm_arch_vcpu_runnable will return true properly)?
> > > > > > > 
> > > > > > > static int kvm_vcpu_check_block(struct kvm_vcpu *vcpu)
> > > > > > > {
> > > > > > >         int ret = -EINTR;
> > > > > > >         int idx = srcu_read_lock(&vcpu->kvm->srcu);
> > > > > > > 
> > > > > > >         if (kvm_arch_vcpu_runnable(vcpu)) {
> > > > > > >                 kvm_make_request(KVM_REQ_UNHALT, vcpu); <---
> > > > > > >                 goto out;
> > > > > > >         }
> > > > > > > 
> > > > > > > Don't want to unhalt the vcpu.
> > > > > > 
> > > > > > Could you elaborate?  It's not obvious to me why we can't do that if
> > > > > > pi_test_on() returns true..  we have pending post interrupts anyways, so
> > > > > > shouldn't we stop halting?  Thanks!
> > > > > 
> > > > > pi_test_on() only returns true when an interrupt is signalled by the
> > > > > device. But the sequence of events is:
> > > > > 
> > > > > 
> > > > > 1. pCPU idles without notification vector configured to wakeup vector.
> > > > > 
> > > > > 2. PCI device is hotplugged, assigned device count increases from 0 to 1.
> > > > > 
> > > > > <arbitrary amount of time>
> > > > > 
> > > > > 3. device generates interrupt, sets ON bit to true in the posted
> > > > > interrupt descriptor.
> > > > > 
> > > > > We want to exit kvm_vcpu_block after 2, but before 3 (where ON bit
> > > > > is not set).
> > > > 
> > > > Ah yes.. thanks.
> > > > 
> > > > Besides the current approach, I'm thinking maybe it'll be cleaner/less LOC to
> > > > define a KVM_REQ_UNBLOCK to replace the pre_block hook (in x86's kvm_host.h):
> > > > 
> > > > #define KVM_REQ_UNBLOCK			KVM_ARCH_REQ(31)
> > > > 
> > > > We can set it in vmx_pi_start_assignment(), then check+clear it in
> > > > kvm_vcpu_has_events() (or make it a bool in kvm_vcpu struct?).
> > > 
> > > Can't check it in kvm_vcpu_has_events() because that will set
> > > KVM_REQ_UNHALT (which we don't want).
> > 
> > I thought it was okay to break the guest HLT? 
> 
> Intel:
> 
> "HLT-HALT
> 
> Description
> 
> Stops instruction execution and places the processor in a HALT state. An enabled interrupt (including NMI and
> SMI), a debug exception, the BINIT# signal, the INIT# signal, or the RESET# signal will resume execution. If an
> interrupt (including NMI) is used to resume execution after a HLT instruction, the saved instruction pointer
> (CS:EIP) points to the instruction following the HLT instruction."
> 
> AMD:
> 
> "6.5 Processor Halt
> The processor halt instruction (HLT) halts instruction execution, leaving the processor in the halt state.
> No registers or machine state are modified as a result of executing the HLT instruction. The processor
> remains in the halt state until one of the following occurs:
> • A non-maskable interrupt (NMI).
> • An enabled, maskable interrupt (INTR).
> • Processor reset (RESET).
> • Processor initialization (INIT).
> • System-management interrupt (SMI)."
> 
> The KVM_REQ_UNBLOCK patch will resume execution even any such event

						  even without any such event

> occuring. So the behaviour would be different from baremetal.