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

["Tian, Kevin" <kevin.tian@xxxxxxxxx>]

> 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?

> 
> 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.
> 

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