Re: [PATCH v2 20/27] ARM: vITS: handle MAPTI command

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

Hi Andre,

On 03/16/2017 11:20 AM, Andre Przywara wrote:
> The MAPTI commands associates a DeviceID/EventID pair with a LPI/CPU
> pair and actually instantiates LPI interrupts.
> We connect the already allocated host LPI to this virtual LPI, so that
> any triggering IRQ on the host can be quickly forwarded to a guest.
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  xen/arch/arm/gic-v3-its.c        | 63 ++++++++++++++++++++++++++++++++++++++++
>  xen/arch/arm/gic-v3-lpi.c        | 18 ++++++++++++
>  xen/arch/arm/vgic-v3-its.c       | 27 +++++++++++++++--
>  xen/include/asm-arm/gic_v3_its.h |  6 ++++
>  4 files changed, 112 insertions(+), 2 deletions(-)
>
> diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
> index 5a2dbec..e2fcf50 100644
> --- a/xen/arch/arm/gic-v3-its.c
> +++ b/xen/arch/arm/gic-v3-its.c
> @@ -724,6 +724,69 @@ restart:
>      spin_unlock(&d->arch.vgic.its_devices_lock);
>  }
>
> +/*
> + * Translates an event for a given guest device ID into the associated host
> + * LPI number. This can be used to look up the mapped guest LPI.
> + */
> +static uint32_t translate_event(struct domain *d, paddr_t doorbell,
> +                                uint32_t devid, uint32_t eventid)
> +{
> +    struct rb_node *node;
> +    struct its_devices *dev;
> +    uint32_t host_lpi = 0;
> +    int cmp;
> +
> +    spin_lock(&d->arch.vgic.its_devices_lock);
> +    node = d->arch.vgic.its_devices.rb_node;
> +    while (node)
> +    {
> +        dev = rb_entry(node, struct its_devices, rbnode);
> +        cmp = compare_its_guest_devices(dev, doorbell, devid);
> +
> +        if ( !cmp )
> +        {
> +            if ( eventid >= dev->eventids )
> +                goto out;
> +
> +            host_lpi = dev->host_lpis[eventid / LPI_BLOCK] +
> +                                (eventid % LPI_BLOCK);
> +            if ( !is_lpi(host_lpi) )

Hmmm, I don't understand this check. host_lpi should always be an LPI. No?

> +                host_lpi = 0;
> +            goto out;
> +        }
> +
> +        if ( cmp > 0 )
> +            node = node->rb_left;
> +        else
> +            node = node->rb_right;
> +    }
> +
> +out:
> +    spin_unlock(&d->arch.vgic.its_devices_lock);
> +
> +    return host_lpi;
> +}
> +
> +/*
> + * Connects the event ID for an already assigned device to the given VCPU/vLPI
> + * pair. The corresponding physical LPI is already mapped on the host side
> + * (when assigning the physical device to the guest), so we just connect the
> + * target VCPU/vLPI pair to that interrupt to inject it properly if it fires.
> + */
> +int gicv3_assign_guest_event(struct domain *d, paddr_t doorbell_address,
> +                             uint32_t devid, uint32_t eventid,

It looks like to me that devid is the virtual deviceID. If so, please 
prefix with 'v', otherwise 'p'.

> +                             struct vcpu *v, uint32_t virt_lpi)
> +{
> +    uint32_t host_lpi = translate_event(d, doorbell_address, devid, eventid);
> +
> +    if ( !host_lpi )
> +        return -ENOENT;
> +
> +    gicv3_lpi_update_host_entry(host_lpi, d->domain_id, v->vcpu_id, virt_lpi);
> +
> +    return 0;
> +}
> +
>  /* 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/arch/arm/gic-v3-lpi.c b/xen/arch/arm/gic-v3-lpi.c
> index 994698e..c110ec9 100644
> --- a/xen/arch/arm/gic-v3-lpi.c
> +++ b/xen/arch/arm/gic-v3-lpi.c
> @@ -153,6 +153,24 @@ void do_LPI(unsigned int lpi)
>      vgic_vcpu_inject_irq(vcpu, hlpi.virt_lpi);
>  }
>
> +int gicv3_lpi_update_host_entry(uint32_t host_lpi, int domain_id,
> +                                unsigned int vcpu_id, uint32_t virt_lpi)
> +{
> +    union host_lpi *hlpip, hlpi;
> +
> +    host_lpi -= LPI_OFFSET;

I would add an ASSERT(host_lpi > LPI_OFFSET);

> +
> +    hlpip = &lpi_data.host_lpis[host_lpi / HOST_LPIS_PER_PAGE][host_lpi % HOST_LPIS_PER_PAGE];
> +
> +    hlpi.virt_lpi = virt_lpi;
> +    hlpi.dom_id = domain_id;
> +    hlpi.vcpu_id = vcpu_id;
> +
> +    write_u64_atomic(&hlpip->data, hlpi.data);
> +
> +    return 0;
> +}
> +
>  static int gicv3_lpi_allocate_pendtable(uint64_t *reg)
>  {
>      uint64_t val;
> diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c
> index c26d5d4..600ff69 100644
> --- a/xen/arch/arm/vgic-v3-its.c
> +++ b/xen/arch/arm/vgic-v3-its.c
> @@ -167,8 +167,8 @@ static bool read_itte(struct virt_its *its, uint32_t devid, uint32_t evid,
>  }
>
>  #define SKIP_LPI_UPDATE 1
> -bool write_itte(struct virt_its *its, uint32_t devid, uint32_t evid,
> -                uint32_t collid, uint32_t vlpi, struct vcpu **vcpu)
> +static bool write_itte(struct virt_its *its, uint32_t devid, uint32_t evid,
> +                       uint32_t collid, uint32_t vlpi, struct vcpu **vcpu)

Please explain in the commit message why you move to static.

>  {
>      struct vits_itte *itte;
>
> @@ -332,6 +332,25 @@ static int its_handle_mapd(struct virt_its *its, uint64_t *cmdptr)
>      return 0;
>  }
>
> +static int its_handle_mapti(struct virt_its *its, uint64_t *cmdptr)
> +{
> +    uint32_t devid = its_cmd_get_deviceid(cmdptr);
> +    uint32_t eventid = its_cmd_get_id(cmdptr);
> +    uint32_t intid = its_cmd_get_physical_id(cmdptr);
> +    int collid = its_cmd_get_collection(cmdptr);

This should be unsigned.

> +    struct vcpu *vcpu;
> +
> +    if ( its_cmd_get_command(cmdptr) == GITS_CMD_MAPI )
> +        intid = eventid;
> +
> +    if ( !write_itte(its, devid, eventid, collid, intid, &vcpu) )
> +        return -1;
> +
> +    gicv3_assign_guest_event(its->d, its->doorbell_address, devid, eventid, vcpu, intid);

If you have a function returning an error, then you should check it and 
not ignoring it.


Cheers,

-- 
Julien Grall

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