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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

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

Jan

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