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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 14.01.2020 17:26, Julien Grall wrote:
> 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.

While I understand this, to be honest I'd rather not see the size
grow for no good (to PV) reason. I don't think the current model is
_this_ bad. But if you really want to push for it, why can't the
two parts continue to live in a wrapper HVM structure, just like
they 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.

Our coding style document is not to re-iterate C standard rules,
I think, and hence yes, you won't find anything to this effect
there.

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

That's exactly what I was thinking of as a possible solution.

Jan

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