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

["Xuquan (Euler)" <xuquan8@huawei.com>]

On September 24, 2016 7:34 AM, Tian Kevin < kevin.tian@intel.com > wrote:
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: Friday, September 23, 2016 11:34 PM
>>
>> >>> On 20.09.16 at 15:30, <xuquan8@huawei.com> 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.
>

Yes, more that the EOI-induced exit is conditional, specifically, the bitmap is set by vmx_set_eoi_exit_bitmap().
Jan, what do you think? While I recall from v1 discussion, you have the same comment. We can dig it deep..

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

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

Indeed.

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

I think IRR is enough. But there is a challenge that the pt interrupt is __not_only__ handled by LAPIC ( see the bottom of pt_update_irq() )..

Quan






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