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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
