Re: [Xen-devel] [RFC v2 14/15] Suppress posting interrupts when 'SN' is set

["Wu, Feng" <feng.wu@xxxxxxxxx>]

> -----Original Message-----
> From: Wu, Feng
> Sent: Monday, June 15, 2015 5:20 PM
> To: Jan Beulich
> Cc: andrew.cooper3@xxxxxxxxxx; george.dunlap@xxxxxxxxxxxxx; Tian, Kevin;
> Zhang, Yang Z; xen-devel@xxxxxxxxxxxxx; keir@xxxxxxx; Wu, Feng
> Subject: RE: [RFC v2 14/15] Suppress posting interrupts when 'SN' is set
> 
> 
> 
> > -----Original Message-----
> > From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> > Sent: Friday, June 12, 2015 6:47 PM
> > To: Wu, Feng
> > Cc: andrew.cooper3@xxxxxxxxxx; george.dunlap@xxxxxxxxxxxxx; Tian, Kevin;
> > Zhang, Yang Z; xen-devel@xxxxxxxxxxxxx; keir@xxxxxxx
> > Subject: RE: [RFC v2 14/15] Suppress posting interrupts when 'SN' is set
> >
> > >>> On 12.06.15 at 11:40, <feng.wu@xxxxxxxxx> wrote:
> >
> > >
> > >> -----Original Message-----
> > >> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> > >> Sent: Wednesday, June 10, 2015 2:49 PM
> > >> To: Wu, Feng
> > >> Cc: andrew.cooper3@xxxxxxxxxx; george.dunlap@xxxxxxxxxxxxx; Tian,
> Kevin;
> > >> Zhang, Yang Z; xen-devel@xxxxxxxxxxxxx; keir@xxxxxxx
> > >> Subject: Re: [RFC v2 14/15] Suppress posting interrupts when 'SN' is set
> > >>
> > >> >>> On 08.05.15 at 11:07, <feng.wu@xxxxxxxxx> wrote:
> > >> > --- a/xen/arch/x86/hvm/vmx/vmx.c
> > >> > +++ b/xen/arch/x86/hvm/vmx/vmx.c
> > >> > @@ -1664,9 +1664,20 @@ static void
> > >> __vmx_deliver_posted_interrupt(struct vcpu *v)
> > >> >
> > >> >  static void vmx_deliver_posted_intr(struct vcpu *v, u8 vector)
> > >> >  {
> > >> > +    int r, sn;
> > >> > +
> > >> >      if ( pi_test_and_set_pir(vector, &v->arch.hvm_vmx.pi_desc) )
> > >> >          return;
> > >> >
> > >> > +    /*
> > >> > +     * Currently, we don't support urgent interrupt, all interrupts
> > >> > +     * are recognized as non-urgent interrupt, so we cannot send
> > >> > +     * posted-interrupt when 'SN' is set.
> > >> > +     */
> > >> > +
> > >> > +    sn = v->arch.hvm_vmx.pi_desc.sn;
> > >> > +    r = pi_test_and_set_on(&v->arch.hvm_vmx.pi_desc);
> > >>
> > >> I'm probably misunderstanding something here, but to me this looks
> > >> like a change that would need to be done quite a bit earlier in the
> > >> series (i.e. at this point it looks like it's fixing a bug/oversight of 
> > >> an
> > >> earlier patch).
> > >
> > > From hardware p.o.v, if 'SN' is set, the hardware doesn't send 
> > > notification
> > > event.
> > > vmx_deliver_posted_intr() is the software way to delivery
> posted-interrupts,
> > > so
> > > we need to follow the HW's behavior. Hence we check 'SN' first, and not
> send
> > > notification event if it is set.
> >
> > Right, that's what I understood; my question wasn't "why", but
> > "doesn't this need to be done earlier in the series".
> 
> Yes, it should be done earlier.
> 
> Thanks,
> Feng
> 
> >
> > >> Apart from that I'm also not understanding the synchronization
> > >> aspect here: What if SN gets set after having been latched above,
> > >> but before the latched value gets looked at below?
> > >
> > > Yes, that is a question. Here is the scenario your described above, right?
> > >
> > >   ......
> > >
> > >   sn = v->arch.hvm_vmx.pi_desc.sn; /*sn gets 0 here*/
> > >
> > >   v->arch.hvm_vmx.pi_desc.sn gets set by others
> > >
> > >   else if ( !r && !sn ) /*Oops, sn cannot reflect the real
> > v->arch.hvm_vmx.pi_desc.sn here*/
> > >
> > >   ......
> >
> > Yes.
> >
> > > Maybe I need think about how to handle this.
> >
> > Apparently, unless you can find a reason why this is not a problem.

Seems it is a little tricky here. What we need to do for this is
like something below:

......

    else if ( !pi_test_sn(&v->arch.hvm_vmx.pi_desc) ||
          !pi_test_and_set_on(&v->arch.hvm_vmx.pi_desc) )
    {
        __vmx_deliver_posted_interrupt(v);
        return;
    }

......

But how to handle this if 'SN' is set between pi_test_sn() and
pi_test_and_set_on() ? Seems we cannot guarantee this two
operations are atomic in software way.But thinking a bit
more, maybe this solution is acceptable, 'SN' is only set when
the vCPU's state is going to be runnable, which means the
vCPU is not running in non-root, in this case, it doesn't matter
whether we send notification event or not, as long as the
interrupt is stored in PIR, and they will be delivered to guest
during the next vm-entry.

Any ideas about this?

Thanks,
Feng

> >
> > Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel