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

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

On Tue, Nov 26, 2019 at 05:50:32PM +0100, Jan Beulich wrote:
> On 26.11.2019 14:26, Roger Pau Monne wrote:
> > --- a/xen/arch/x86/hvm/irq.c
> > +++ b/xen/arch/x86/hvm/irq.c
> > @@ -515,7 +515,11 @@ void hvm_set_callback_via(struct domain *d, uint64_t 
> > via)
> >  struct hvm_intack hvm_vcpu_has_pending_irq(struct vcpu *v)
> >  {
> >      struct hvm_domain *plat = &v->domain->arch.hvm;
> > -    int vector;
> > +    /*
> > +     * Always call vlapic_has_pending_irq so that PIR is synced into IRR 
> > when
> > +     * using posted interrupts.
> > +     */
> > +    int vector = vlapic_has_pending_irq(v);
> 
> Did you consider doing this conditionally either here ...
> 
> > @@ -530,7 +534,6 @@ struct hvm_intack hvm_vcpu_has_pending_irq(struct vcpu 
> > *v)
> >      if ( vlapic_accept_pic_intr(v) && plat->vpic[0].int_output )
> >          return hvm_intack_pic(0);
> >  
> > -    vector = vlapic_has_pending_irq(v);
> >      if ( vector != -1 )
> >          return hvm_intack_lapic(vector);
> 
> ... or here? I ask not only because the function isn't exactly
> cheap to call (as iirc you did also mention during the v2
> discussion), but also because of its interaction with Viridian
> and nested mode. In case of problems there, avoiding the use
> of interrupt posting would be a workaround in such cases then.

TBH my preference was to do the PIR to IRR sync in vmx_intr_assist by
directly calling vmx_sync_pir_to_irr because it was IMO less intrusive
and confined to VMX code. I think this approach is more risky as
vlapic_has_pending_irq does way more than simply syncing PIR to IRR.

> > --- a/xen/arch/x86/hvm/vmx/vmx.c
> > +++ b/xen/arch/x86/hvm/vmx/vmx.c
> > @@ -1945,57 +1945,31 @@ static void vmx_process_isr(int isr, struct vcpu *v)
> >  
> >  static void __vmx_deliver_posted_interrupt(struct vcpu *v)
> >  {
> > -    bool_t running = v->is_running;
> > -
> >      vcpu_unblock(v);
> > -    /*
> > -     * Just like vcpu_kick(), nothing is needed for the following two 
> > cases:
> > -     * 1. The target vCPU is not running, meaning it is blocked or 
> > runnable.
> > -     * 2. The target vCPU is the current vCPU and we're in non-interrupt
> > -     * context.
> > -     */
> > -    if ( running && (in_irq() || (v != current)) )
> > -    {
> > +    if ( v->is_running && v != current )
> 
> I continue to be concerned by this evaluation of ->is_running
> _after_ vcpu_unblock(), when previously (just like vcpu_kick()
> does) it was intentionally done before.

If the unblock sets v->is_running to true that's fine, Xen will send a
posted interrupt IPI and the destination pCPU will either be in root
mode and thus raise a softirq or in non-root mode and perform the PIR
to IRR sync and possible interrupt injection.

I see that caching the value of is_running might be helpful in order
to prevent pointless IPI'ing. If the vCPU wasn't running before the
unblock there's no reason to send an IPI to it, because the sync of
PIR to IRR will happen at vmentry anyway.

> I wonder anyway
> whether this and the change to irq.c should really be in a
> single patch, the more that you start the respective part of
> the description with "While there also simplify ...". But in
> the end it is up to Kevin's or Jun's judgement anyway.

Yes, that makes sense. Will wait for feedback from Kevin or Jun before
sending a new version anyway.

Thanks, Roger.

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