|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v9 15/28] ARM: vITS: provide access to struct pending_irq
Hi,
On 17/05/17 16:35, Julien Grall wrote:
> Hi Andre,
>
> On 11/05/17 18:53, Andre Przywara wrote:
>> For each device we allocate one struct pending_irq for each virtual
>> event (MSI).
>> Provide a helper function which returns the pointer to the appropriate
>> struct, to be able to find the right struct when given a virtual
>> deviceID/eventID pair.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
>> ---
>> xen/arch/arm/gic-v3-its.c | 69
>> ++++++++++++++++++++++++++++++++++++++++
>> xen/include/asm-arm/gic_v3_its.h | 4 +++
>> 2 files changed, 73 insertions(+)
>>
>> diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
>> index aebc257..fd6a394 100644
>> --- a/xen/arch/arm/gic-v3-its.c
>> +++ b/xen/arch/arm/gic-v3-its.c
>> @@ -800,6 +800,75 @@ out:
>> return ret;
>> }
>>
>> +/* Must be called with the its_device_lock held. */
>> +static struct its_device *get_its_device(struct domain *d, paddr_t
>> vdoorbell,
>> + uint32_t vdevid)
>> +{
>> + struct rb_node *node = d->arch.vgic.its_devices.rb_node;
>> + struct its_device *dev;
>> +
>> + ASSERT(spin_is_locked(&d->arch.vgic.its_devices_lock));
>> +
>> + while (node)
>> + {
>> + int cmp;
>> +
>> + dev = rb_entry(node, struct its_device, rbnode);
>> + cmp = compare_its_guest_devices(dev, vdoorbell, vdevid);
>> +
>> + if ( !cmp )
>> + return dev;
>> +
>> + if ( cmp > 0 )
>> + node = node->rb_left;
>> + else
>> + node = node->rb_right;
>> + }
>> +
>> + return NULL;
>> +}
>> +
>> +static uint32_t get_host_lpi(struct its_device *dev, uint32_t eventid)
>> +{
>> + uint32_t host_lpi = INVALID_LPI;
>> +
>> + if ( dev && (eventid < dev->eventids) )
>> + host_lpi = dev->host_lpi_blocks[eventid / LPI_BLOCK] +
>> + (eventid % LPI_BLOCK);
>> +
>> + return host_lpi;
>
> IHMO, it would be easier to read if you invert the condition:
>
> if ( !dev || (eventid >= dev->eventids) )
> return INVALID_LPI;
>
> return dev->host_lpi_blocks[eventid / LPI_BLOCK] + (eventid % LPI_BLOCK);
>
> Also, whilst I agree about sanitizing eventid, someone calling this
> function with dev = NULL is already wrong. Defensive programming is
> good, but there are some place where I don't think it is necessary. You
> have to trust a bit the caller, otherwise you will end up making the
> check 10 times before accessing it.
Yeah, good point, I think this function was meant to be more widely used
originally. Given that it's static, has only one caller and is
effectively a one-liner, I will just scrap it altogether and just do it
in-line. That should also address your double check comment below.
>
>> +}
>> +
>> +static struct pending_irq *get_event_pending_irq(struct domain *d,
>> + paddr_t
>> vdoorbell_address,
>> + uint32_t vdevid,
>> + uint32_t veventid,
>
> s/veventid/ as it is not virtual a virtual one and make the call to
> get_host_lpi fairly confusing.
Right, thinking again I believe there is no such thing as a "virtual
event ID", because the event is always a matter of the device and thus
there is no distinction between a physical and a virtual event. So I
will remove all "v"s from the respective identifiers.
>> + uint32_t *host_lpi)
>> +{
>> + struct its_device *dev;
>> + struct pending_irq *pirq = NULL;
>> +
>> + spin_lock(&d->arch.vgic.its_devices_lock);
>> + dev = get_its_device(d, vdoorbell_address, vdevid);
>> + if ( dev && veventid <= dev->eventids )
>
> Why are you using "<=" here and not "<" like in get_host_lpi? Surely one
> of them is wrong.
Oh, good catch!
>> + {
>> + pirq = &dev->pend_irqs[veventid];
>> + if ( host_lpi )
>> + *host_lpi = get_host_lpi(dev, veventid);
>
> Getting the host_lpi is fairly cheap. I would impose to pass host_lpi.
>
> This would also avoid multiple check on the eventid as you currently do.
> I.e
>
> dev = ...
> if ( !dev )
> goto out;
>
> *host_lpi = get_host_lpi(dev, ...);
>
> if ( *host_lpi == INVALID_LPI )
> goto out;
>
> pirq = &dev->pend_irqs[veventid];
>
>
> out:
> spin_unlock(...)
> return pirq;
>
>> + }
>> + 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.
Cheers,
Andre.
>> +
>> /* Scan the DT for any ITS nodes and create a list of host ITSes out
>> of it. */
>> void gicv3_its_dt_init(const struct dt_device_node *node)
>> {
>> diff --git a/xen/include/asm-arm/gic_v3_its.h
>> b/xen/include/asm-arm/gic_v3_its.h
>> index 40f4ef5..d162e89 100644
>> --- a/xen/include/asm-arm/gic_v3_its.h
>> +++ b/xen/include/asm-arm/gic_v3_its.h
>> @@ -169,6 +169,10 @@ int gicv3_its_map_guest_device(struct domain *d,
>> int gicv3_allocate_host_lpi_block(struct domain *d, uint32_t
>> *first_lpi);
>> void gicv3_free_host_lpi_block(uint32_t first_lpi);
>>
>> +struct pending_irq *gicv3_its_get_event_pending_irq(struct domain *d,
>> + paddr_t
>> vdoorbell_address,
>> + uint32_t vdevid,
>> + uint32_t veventid);
>> #else
>>
>> static inline void gicv3_its_dt_init(const struct dt_device_node *node)
>>
>
> Cheers,
>
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |