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

["Jan Beulich" <JBeulich@suse.com>]

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

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.

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.

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

Jan


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