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 18:03, Julien Grall wrote:
> On 14/01/2020 16:50, Jan Beulich wrote:
>> 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.
> 
> Well, I did lost two days debugging a problem because of the allocation 
> (the memory were getting corrupted randomly). The comment you added may 
> help to avoid this problem but I still think that trying to allocate 
> half a pirq is a pretty bad idea.

To me, not significantly different from your container_of() approach.

>> 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?
> 
> I am not sure what you are suggesting here. Could you extend your thought?

Right now we have

struct arch_pirq {
    int irq;
    union {
        struct hvm_pirq {
            int emuirq;
            struct hvm_pirq_dpci dpci;
        } hvm;
    };
};

What I'm suggesting is to keep

struct hvm_pirq {
     int emuirq;
     struct hvm_pirq_dpci dpci;
};

and add struct arch_pirq into there. Arguably it could even
be first in there, thus allowing xfree() to free the whole
thing no matter whether passed a struct hvm_pirq * or a
struct arch_pirq * (and eliminating the need for a per-
arch abstraction of the freeing).

>>>>> @@ -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.
> 
> The fact such code has been added in Xen in the past clearly shows that 
> the coding style is not sufficient to back your point here.
> 
> So rather than complaining that I don't follow an unwritten rule, you 
> could have suggested it. This would have came accross as less rude.

If anything I said came across as rude, I'd like to apologize.
As an explanation (not an excuse), please be aware that I've
had to request changes to comply to name space rules far too
often that I would recall towards whom I did send these, or
that I would assume any of the regular contributors could in
fact never have noticed this so far.

I do insist on my point though that we, earning our money with
programming, and hence probably calling ourselves "professional
programmers", should know and honor basic principles of the
standards of languages we're using in our day to day work. The
fact that code violating this had been added to Xen in the past
does not make this any better; the excuse there may well be
that it started out as a research project, where such
considerations may not have mattered all this much. (FAOD I
explicitly said "basic principles" - I don't expect everyone to
know every corner case.)

Do you want me to submit a patch adding something like "It
probably goes without saying that the underlying language
standards or specifications are to be honored", perhaps close
to the top of ./CODING_STYLE?

Jan

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