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

[Julien Grall <julien@xxxxxxx>]

Hi Jan,

On 14/01/2020 16:08, Jan Beulich wrote:
> 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.
I am happy to keep emuirq in struct pirq if you are happy with slightly 
increasing the size allocated on PV.
The main thing I want to get rid of is the weird allocation size we do 
today.
> > @@ -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().
AFAIK, there is nothing in the coding style forbidding your "bogus" 
naming. So I just followed the rest of the code.
> > @@ -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.
The free is done through an RCU callback with no extra parameters to 
tell how it can be freed.
The only way I can think of to get rid of the field is to introduce two 
different callback for the free. We would use a different callback 
depending on the domain type.
How does that sound?

Cheers,

--
Julien Grall

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