Re: [Xen-devel] [PATCH 4/4] xen/x86: Rework inclusion between struct pirq and struct hvm_pirq_dpci

[Jan Beulich <jbeulich@xxxxxxxx>]

On 13.01.2020 22:33, Julien Grall wrote:
> --- a/xen/arch/x86/hvm/irq.c
> +++ b/xen/arch/x86/hvm/irq.c
> @@ -29,7 +29,8 @@
>  
>  bool hvm_domain_use_pirq(const struct domain *d, const struct pirq *pirq)
>  {
> -    return is_hvm_domain(d) && pirq && pirq->arch.hvm.emuirq != IRQ_UNBOUND;
> +    return is_hvm_domain(d) && pirq &&
> +        const_pirq_dpci(pirq)->emuirq != IRQ_UNBOUND;
>  }
>  
>  /* Must be called with hvm_domain->irq_lock hold */
> @@ -396,7 +397,7 @@ int hvm_inject_msi(struct domain *d, uint64_t addr, 
> uint32_t data)
>              struct pirq *info = pirq_info(d, pirq);
>  
>              /* if it is the first time, allocate the pirq */
> -            if ( !info || info->arch.hvm.emuirq == IRQ_UNBOUND )
> +            if ( !info || pirq_dpci(info)->emuirq == IRQ_UNBOUND )
>              {
>                  int rc;
>  
> @@ -409,7 +410,7 @@ int hvm_inject_msi(struct domain *d, uint64_t addr, 
> uint32_t data)
>                  if ( !info )
>                      return -EBUSY;
>              }
> -            else if ( info->arch.hvm.emuirq != IRQ_MSI_EMU )
> +            else if ( pirq_dpci(info)->emuirq != IRQ_MSI_EMU )
>                  return -EINVAL;
>              send_guest_pirq(d, info);
>              return 0;

All of these uses (and others further down) make pretty clear
that the emuirq field doesn't belong in the structure you put it
in - the 'd' in dpci stands for "direct" afaik, and the field is
for a certain variant of emulation of interrupt delivery into
guests, i.e. not really pass-through focused at all.

> @@ -171,8 +172,26 @@ struct hvm_pirq_dpci {
>      struct hvm_gmsi_info gmsi;
>      struct timer timer;
>      struct list_head softirq_list;
> +    int emuirq;
> +    struct pirq pirq;
>  };
>  
> +#define pirq_dpci(p)                                                    \
> +    ((p) ? container_of(p, struct hvm_pirq_dpci, pirq) : NULL)
> +#define const_pirq_dpci(p)                                              \
> +    ((p) ? container_of(p, const struct hvm_pirq_dpci, pirq) : NULL)
> +
> +#define dpci_pirq(pd) (&(pd)->pirq)
> +
> +#define domain_pirq_to_emuirq(d, p) ({                                  \
> +    struct pirq *__pi = pirq_info(d, p);                                \
> +    __pi ? pirq_dpci(__pi)->emuirq : IRQ_UNBOUND;                       \
> +})
> +#define domain_emuirq_to_pirq(d, emuirq) ({                             \
> +    void *__ret = radix_tree_lookup(&(d)->arch.hvm.emuirq_pirq, emuirq);\
> +    __ret ? radix_tree_ptr_to_int(__ret) : IRQ_UNBOUND;                 \
> +})

While for the latter you merely move the bogus double-leading-
underscore macro local variable (which on this occasion I'd
like to ask anyway to be changed), you actively introduce a
new similar name space violation in the domain_pirq_to_emuirq().

> @@ -133,17 +132,10 @@ DECLARE_PER_CPU(unsigned int, irq_count);
>  
>  struct arch_pirq {
>      int irq;
> -    union {
> -        struct hvm_pirq {
> -            int emuirq;
> -            struct hvm_pirq_dpci dpci;
> -        } hvm;
> -    };
> +    /* Is the PIRQ associated to an HVM domain? */
> +    bool hvm;

It looks like this field is needed for only arch_free_pirq_struct().
As it'll make a difference to struct pirq's size, can you not get
away without it? All (perhaps indirect) callers of the function
know the domain, after all.

Jan

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