Re: [Xen-devel] [PATCH for-4.13] x86/vmx: always sync PIR to IRR before vmentry

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Mon, Nov 18, 2019 at 03:19:50PM +0100, Jan Beulich wrote:
> On 18.11.2019 15:03, Roger Pau Monné  wrote:
> > On Mon, Nov 18, 2019 at 01:26:46PM +0100, Jan Beulich wrote:
> >> On 18.11.2019 11:16, Roger Pau Monne wrote:
> >>> @@ -1954,48 +1952,28 @@ static void __vmx_deliver_posted_interrupt(struct 
> >>> vcpu *v)
> >>>       * 2. The target vCPU is the current vCPU and we're in non-interrupt
> >>>       * context.
> >>>       */
> >>> -    if ( running && (in_irq() || (v != current)) )
> >>> -    {
> >>> +    if ( vcpu_runnable(v) && v != current )
> >>
> >> I'm afraid you need to be more careful with the running vs runnable
> >> distinction here. The comment above here becomes stale with the
> >> change (also wrt the removal of in_irq(), which I'm at least uneasy
> >> about), and the new commentary below also largely says/assumes
> >> "running", not "runnable".
> > 
> > I've missed to fix that comment, will take care in the next version.
> > Note also that the comment is quite pointless, it only states what the
> > code below is supposed to do, but doesn't give any reasoning as to why
> > in_irq is relevant here.
> 
> It's main "value" is to refer to vcpu_kick(), which has ...
> 
> > TBH I'm not sure of the point of the in_irq check, I don't think it's
> > relevant for the code here.
> 
> ... a similar in_irq() check. Sadly that one, while having a bigger
> comment, also doesn't explain what it's needed for. It looks like I
> should recall the reason, but I'm sorry - I don't right now.

By reading the message of the commit that introduced the in_irq check
in vcpu_kick:

"The drawback is that {vmx,svm}_intr_assist() now races new event
notifications delivered by IRQ or IPI. We close down this race by
having vcpu_kick() send a dummy softirq -- this gets picked up in
IRQ-sage context and will cause retry of *_intr_assist(). We avoid
delivering the softirq where possible by avoiding it when we are
running in the non-IRQ context of the VCPU to be kicked."

AFAICT in the vcpu_kick case this is done because the softirq should
only be raised when in IRQ context in order to trigger the code in
vmx_do_vmentry to retry the call to vmx_intr_assist (this is relevant
if vcpu_kick is issued from an irq handler executed after
vmx_intr_assist and before the disabling interrupts in
vmx_do_vmentry.

I think we need something along the lines of:

if ( v->is_running && v != current )
    send_IPI_mask(cpumask_of(v->processor), posted_intr_vector);
else if ( v == current && in_irq() && !softirq_pending(smp_processor_id()) )
    raise_softirq(VCPU_KICK_SOFTIRQ);

So that vmx_intr_assist is also retried if a vector is signaled in PIR
on the vCPU currently running between the call to vmx_intr_assist and
the disabling of interrupts in vmx_do_vmentry.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel