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

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

On Wed, Nov 27, 2019 at 02:07:16AM +0000, Tian, Kevin wrote:
> > From: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> > Sent: Monday, November 18, 2019 10:56 PM
> > 
> > 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);
> 
> Then what's the difference from original logic?

The original logic is:

if ( running && (in_irq() || (v != current)) )
{
        unsigned int cpu = v->processor;

        if ( cpu != smp_processor_id() )
            send_IPI_mask(cpumask_of(cpu), posted_intr_vector);
        else if ( !softirq_pending(cpu) )
            raise_softirq(VCPU_KICK_SOFTIRQ);
}

Which I find much harder to understand. For example I'm not sure of
what's the benefit of doing the cpu != smp_processor_id() check
instead of simply doing v != current (like in the outer if condition).
My suggestion removes one level of nesting and IMO makes the condition
clearer, but maybe that's just my taste.

Also the original comments don't mention at all why a softirq should
be raised if v == current && in_irq, and it took me some time to
figure out why that's required. My proposed change clarifies why this
is needed, and also makes it more obvious that the softirq will only
be raised when in irq context.

Anyway, I'm not going to insist and will drop the change if it's not
deemed useful.

Thanks, Roger.

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