Re: [RFC PATCH 13/24] ARM: vITS: handle CLEAR command

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

Hi Andre,

On 28/09/16 19:24, Andre Przywara wrote:
> This introduces the ITS command handler for the CLEAR command, which
> clears the pending state of an LPI.
> This removes a not-yet injected, but already queued IRQ from a VCPU.
>
> In addition this patch introduces the lookup function which translates
> a given DeviceID/EventID pair into a pointer to our vITTE structure.
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  xen/arch/arm/vgic-its.c | 115 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 115 insertions(+)
>
> diff --git a/xen/arch/arm/vgic-its.c b/xen/arch/arm/vgic-its.c
> index 875b992..99d9e9c 100644
> --- a/xen/arch/arm/vgic-its.c
> +++ b/xen/arch/arm/vgic-its.c
> @@ -61,6 +61,73 @@ struct vits_itte
>      uint64_t collection:16;
>  };
>
> +#define UNMAPPED_COLLECTION      ((uint16_t)~0)
> +
> +/* Must be called with the ITS lock held. */

This comment is a call to have an ASSERT in the function.

> +static struct vcpu *get_vcpu_from_collection(struct virt_its *its, int collid)

Please use unsigned int.

> +{
> +    uint16_t vcpu_id;
> +
> +    if ( collid >= its->max_collections )
> +        return NULL;
> +
> +    vcpu_id = its->coll_table[collid];
> +    if ( vcpu_id == UNMAPPED_COLLECTION || vcpu_id >= its->d->max_vcpus )
> +        return NULL;
> +
> +    return its->d->vcpu[vcpu_id];
> +}
> +
> +#define DEV_TABLE_ITT_ADDR(x) ((x) & GENMASK(51, 8))
> +#define DEV_TABLE_ITT_SIZE(x) (BIT(((x) & GENMASK(7, 0)) + 1))

The layout of dev_table[...] really needs to be explained. It took me 
quite a while to understand how it works. For instance why you skip the 
first 8 bits for the address...

> +#define DEV_TABLE_ENTRY(addr, bits)                     \
> +        (((addr) & GENMASK(51, 8)) | (((bits) - 1) & GENMASK(7, 0)))
> +
> +static paddr_t get_itte_address(struct virt_its *its,
> +                                uint32_t devid, uint32_t evid)
> +{
> +    paddr_t addr;
> +
> +    if ( devid >= its->max_devices )
> +        return ~0;

Please use INVALID_PADDR here.

> +
> +    if ( evid >= DEV_TABLE_ITT_SIZE(its->dev_table[devid]) )
> +        return ~0;

Ditto.

> +
> +    addr = DEV_TABLE_ITT_ADDR(its->dev_table[devid]);
> +
> +    return addr + evid * sizeof(struct vits_itte);
> +}
> +
> +/* Looks up a given deviceID/eventID pair on an ITS and returns a pointer to

Coding style:

/*
  * Foo

> + * the corresponding ITTE. This maps the respective guest page into Xen.
> + * Once finished with handling the ITTE, call put_devid_evid() to unmap
> + * the page again.
> + * Must be called with the ITS lock held.

This is a call for an ASSERT in the code.

> + */
> +static struct vits_itte *get_devid_evid(struct virt_its *its,
> +                                        uint32_t devid, uint32_t evid)

The naming of the function is confusing. It doesn't look up a device 
ID/event ID but an IIT. So I would rename it to find_itte.

> +{
> +    paddr_t addr = get_itte_address(its, devid, evid);
> +    struct vits_itte *itte;
> +
> +    if (addr == ~0)

Coding style: if ( ... )

And return INVALID_PADDR.

> +        return NULL;
> +
> +    /* TODO: check locking for map_guest_pages() */
> +    itte = map_guest_pages(its->d, addr & PAGE_MASK, 1);
> +    if ( !itte )
> +        return NULL;
> +
> +    return itte + (addr & ~PAGE_MASK) / sizeof(struct vits_itte);
> +}
> +
> +/* Must be called with the ITS lock held. */
> +static void put_devid_evid(struct virt_its *its, struct vits_itte *itte)
> +{
> +    unmap_guest_pages(itte, 1);
> +}
> +
>  /**************************************
>   * Functions that handle ITS commands *
>   **************************************/
> @@ -80,6 +147,51 @@ static uint64_t its_cmd_mask_field(uint64_t *its_cmd,
>  #define its_cmd_get_target_addr(cmd)    its_cmd_mask_field(cmd, 2, 16, 32)
>  #define its_cmd_get_validbit(cmd)       its_cmd_mask_field(cmd, 2, 63,  1)
>
> +static int its_handle_clear(struct virt_its *its, uint64_t *cmdptr)
> +{
> +    uint32_t devid = its_cmd_get_deviceid(cmdptr);
> +    uint32_t eventid = its_cmd_get_id(cmdptr);
> +    struct pending_irq *pirq;
> +    struct vits_itte *itte;
> +    struct vcpu *vcpu;
> +    uint32_t vlpi;
> +
> +    spin_lock(&its->its_lock);
> +
> +    itte = get_devid_evid(its, devid, eventid);
> +    if ( !itte )
> +    {
> +        spin_unlock(&its->its_lock);
> +        return -1;
> +    }
> +
> +    vcpu = get_vcpu_from_collection(its, itte->collection);
> +    if ( !vcpu )
> +    {
> +        spin_unlock(&its->its_lock);
> +        return -1;
> +    }
> +
> +    vlpi = itte->vlpi;
> +
> +    put_devid_evid(its, itte);
> +    spin_unlock(&its->its_lock);
> +
> +    /* Remove a pending, but not yet injected guest IRQ. */
> +    pirq = lpi_to_pending(vcpu, vlpi, false);
> +    if ( pirq )
> +    {
> +        clear_bit(GIC_IRQ_GUEST_QUEUED, &pirq->status);
> +        gic_remove_from_queues(vcpu, vlpi);
> +
> +        /* Mark this pending IRQ struct as availabe again. */

NIT: s/availabe/available/

> +        if ( !test_bit(GIC_IRQ_GUEST_VISIBLE, &pirq->status) )
> +            pirq->irq = 0;

This code should be in a separate helper. It will be helpful to make the 
structure available again easily without open coding it.

> +    }
> +
> +    return 0;
> +}
> +
>  #define ITS_CMD_BUFFER_SIZE(baser)      ((((baser) & 0xff) + 1) << 12)
>
>  static int vgic_its_handle_cmds(struct domain *d, struct virt_its *its,
> @@ -100,6 +212,9 @@ static int vgic_its_handle_cmds(struct domain *d, struct virt_its *its,
>          cmdptr = its->cmdbuf + (its->creadr / sizeof(*its->cmdbuf));
>          switch (its_cmd_get_command(cmdptr))
>          {
> +        case GITS_CMD_CLEAR:
> +            its_handle_clear(its, cmdptr);

Should not you check the return for its_handle_clear?

> +            break;
>          case GITS_CMD_SYNC:
>              /* We handle ITS commands synchronously, so we ignore SYNC. */
>  	    break;
>

Regards,

-- 
Julien Grall

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