Re: [Xen-devel] [PATCH v2] x86/vpt: guarantee the return value of pt_update_irq() set in vIRR or PIR

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 13.10.17 at 07:10, <chao.gao@xxxxxxxxx> wrote:
> --- a/xen/arch/x86/hvm/irq.c
> +++ b/xen/arch/x86/hvm/irq.c
> @@ -168,11 +168,13 @@ void hvm_gsi_deassert(struct domain *d, unsigned int 
> gsi)
>      spin_unlock(&d->arch.hvm_domain.irq_lock);
>  }
>  
> -void hvm_isa_irq_assert(
> -    struct domain *d, unsigned int isa_irq)
> +int hvm_isa_irq_assert(struct domain *d, unsigned int isa_irq,
> +                       int (*get_vector)(const struct domain *d,
> +                                         unsigned int gsi))
>  {
>      struct hvm_irq *hvm_irq = hvm_domain_irq(d);
>      unsigned int gsi = hvm_isa_irq_to_gsi(isa_irq);
> +    int vector = 0;

Why zero (which is valid aiui) instead of e.g. -1?

> @@ -292,25 +292,38 @@ int pt_update_irq(struct vcpu *v)
>  
>      spin_unlock(&v->arch.hvm_vcpu.tm_lock);
>  
> +    /*
> +     * If periodic timer interrut is handled by lapic, its vector in
> +     * IRR is returned and used to set eoi_exit_bitmap for virtual
> +     * interrupt delivery case. Otherwise return -1 to do nothing.
> +     */
>      if ( is_lapic )
> +    {
>          vlapic_set_irq(vcpu_vlapic(v), irq, 0);
> +        pt_vector = irq;
> +    }
>      else
>      {
>          hvm_isa_irq_deassert(v->domain, irq);
> -        hvm_isa_irq_assert(v->domain, irq);
> +        if ( platform_legacy_irq(irq) && vlapic_accept_pic_intr(v) &&
> +             v->domain->arch.hvm_domain.vpic[irq >> 3].int_output )
> +        {
> +            hvm_isa_irq_assert(v->domain, irq, NULL);
> +            pt_vector = -1;
> +        }
> +        else
> +        {
> +            pt_vector = hvm_isa_irq_assert(v->domain, irq, 
> vioapic_get_vector);

I like that you got away without introducing a new wrapper function
at all.

> --- a/xen/include/asm-x86/hvm/vmx/vmx.h
> +++ b/xen/include/asm-x86/hvm/vmx/vmx.h
> @@ -109,6 +109,11 @@ static inline int pi_test_and_set_pir(int vector, struct 
> pi_desc *pi_desc)
>      return test_and_set_bit(vector, pi_desc->pir);
>  }
>  
> +static inline int pi_test_pir(int vector, const struct pi_desc *pi_desc)

This should not be a signed quantity - uint8_t or unsigned int
please.

I wouldn't mind making suitable adjustments while committing (and
then adding my R-b), but that requires your feedback which way
things should be.

Also please don't forget to Cc the release manager, unless you
intend this fix only for after 4.10.

Jan


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