Re: [PATCH v9 15/28] ARM: vITS: provide access to struct pending_irq

[Julien Grall <julien.grall@arm.com>]

On 26/05/17 10:10, Andre Przywara wrote:
> Hi,
>
> On 22/05/17 18:19, Julien Grall wrote:
>>
>>
>> On 22/05/17 17:50, Andre Przywara wrote:
>>> Hi,
>>
>> Hi Andre,
>>
>>> On 17/05/17 16:35, Julien Grall wrote:
>>>>> +    }
>>>>> +    spin_unlock(&d->arch.vgic.its_devices_lock);
>>>>> +
>>>>> +    return pirq;
>>>>> +}
>>>>> +
>>>>> +struct pending_irq *gicv3_its_get_event_pending_irq(struct domain *d,
>>>>> +                                                    paddr_t
>>>>> vdoorbell_address,
>>>>> +                                                    uint32_t vdevid,
>>>>> +                                                    uint32_t veventid)
>>>>
>>>> s/veventid/eventid/
>>>>
>>>>> +{
>>>>> +    return get_event_pending_irq(d, vdoorbell_address, vdevid,
>>>>> veventid, NULL);
>>>>> +}
>>>>
>>>> This wrapper looks a bit pointless to me. Why don't you directly expose
>>>> get_event_pending_irq(...)?
>>>
>>> I don't want to expose host_lpi in the exported function, because it's
>>> of no need for the callers and rather cumbersome for them to pass NULL
>>> or the like. But then the algorithm to find host_lpi and pirq is
>>> basically the same, so I came up with this joint static function and an
>>> exported wrapper, which hides the host_lpi.
>>> And there is one user (in gicv3_assign_guest_event()) which needs both,
>>> so ...
>>> If you can think of a better way to address this, I am all ears.
>>
>> It is not that bad to pass NULL everywhere. We already have some other
>> functions like that.
>>
>> How about adding the wrapper as a static inline in the header?
>
> The host LPI is an internal affair of gic-v3-its.c (the parts caring
> about the host control of the ITS). The data structure describing this
> is private to this file and not exported.
>
> The virtual ITS emulation does not need to know about the host LPI, that
> would break the abstraction. So I prefer to simply not export this.

So you never envision someone requiring the host LPI even for debug purpose?

AFAICT, there are no other way to get the host LPI if necessary. It 
really does not hurt to expose it and provide a wrapper.

>
> And I would prefer code design considerations over the cost of one
> unconditional branch here.

As you may know I am all in favor of more helpers over the cost of one 
unconditional branch (see the callback example) when it results to a 
better code design.

But here it is not about code design, it is more about what kind of 
information would you need outside (see above).

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel