Re: [Xen-devel] [PATCH v2] x86/apicv: fix RTC periodic timer and apicv issue

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

> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: Friday, September 23, 2016 11:34 PM
> 
> >>> On 20.09.16 at 15:30, <xuquan8@xxxxxxxxxx> wrote:
> > --- a/xen/arch/x86/hvm/vlapic.c
> > +++ b/xen/arch/x86/hvm/vlapic.c
> > @@ -433,6 +433,12 @@ void vlapic_EOI_set(struct vlapic *vlapic)
> >  void vlapic_handle_EOI(struct vlapic *vlapic, u8 vector)
> >  {
> >      struct domain *d = vlapic_domain(vlapic);
> > +    struct vcpu *v = vlapic_vcpu(vlapic);
> > +    struct hvm_intack pt_intack;
> > +
> > +    pt_intack.vector = vector;
> > +    pt_intack.source = hvm_intsrc_lapic;
> > +    pt_intr_post(v, pt_intack);
> 
> This also sits on the EOI LAPIC register write path, i.e. the change
> then also affects non-apicv environments.

The new logic should be entered only when EOI-induced exit happens.

> 
> Furthermore - don't we have the same problem as with v1 again
> then? What prevents multiple EOIs to come here before the timer
> interrupt actually gets handled? You'd then clear ->irq_issued
> each time, rendering your change to pt_update_irq() ineffective.

based on this patch, one irq_issued should cause only one EOI on
related pt vector and this EOI exit clears irq_issued then next EOI
would be seen only upon another injection when irq_issued is set
again... However there might be an issue if this pt vector is shared 
with another device interrupt, which although is not a typical case...

> 
> In any event please use hvm_intack_lapic() instead of open
> coding it (and then I don't think you need a local variable at all).
> 
> > --- a/xen/arch/x86/hvm/vmx/intr.c
> > +++ b/xen/arch/x86/hvm/vmx/intr.c
> > @@ -333,8 +333,6 @@ void vmx_intr_assist(void)
> >              clear_bit(i, &v->arch.hvm_vmx.eoi_exitmap_changed);
> >              __vmwrite(EOI_EXIT_BITMAP(i), 
> > v->arch.hvm_vmx.eoi_exit_bitmap[i]);
> >          }
> > -
> > -        pt_intr_post(v, intack);
> >      }
> 
> I'll defer to the VMX maintainers to determine whether removing this
> but not the other one is correct.

This is correct. The other one is for non-apicv scenario.

> 
> > --- a/xen/arch/x86/hvm/vpt.c
> > +++ b/xen/arch/x86/hvm/vpt.c
> > @@ -252,7 +252,8 @@ int pt_update_irq(struct vcpu *v)
> >              }
> >              else
> >              {
> > -                if ( (pt->last_plt_gtime + pt->period) < max_lag )
> > +                if ( (pt->last_plt_gtime + pt->period) < max_lag &&
> > +                     !pt->irq_issued )
> >                  {
> >                      max_lag = pt->last_plt_gtime + pt->period;
> >                      earliest_pt = pt;
> 
> Looking at the rest of the code I really wonder why this check
> hadn't been there from the beginning. Tim, do recall whether
> this was intentional (as opposed to simply having been an
> oversight)?
> 

Possibly simplify the emulation by relying on IRR/ISR to handling pending
situation?

Thanks
Kevin

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