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

[Julien Grall <julien.grall@xxxxxxx>]

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.
> +}
> +
> +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.
> +                                                 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.
> +    {
> +        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(...)?
> +
>  /* 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,

--
Julien Grall

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