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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Wed, 28 Sep 2016, 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@xxxxxxx>
> ---
>  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. */
> +static struct vcpu *get_vcpu_from_collection(struct virt_its *its, int 
> collid)
> +{
> +    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))
> +#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 #define the error


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

same here


> +    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
> + * 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.
> + */
> +static struct vits_itte *get_devid_evid(struct virt_its *its,
> +                                        uint32_t devid, uint32_t evid)
> +{
> +    paddr_t addr = get_itte_address(its, devid, evid);
> +    struct vits_itte *itte;
> +
> +    if (addr == ~0)
> +        return NULL;
> +
> +    /* TODO: check locking for map_guest_pages() */
> +    itte = map_guest_pages(its->d, addr & PAGE_MASK, 1);
> +    if ( !itte )
> +        return NULL;

No need to use the vmap to map 1 page


> +    return itte + (addr & ~PAGE_MASK) / sizeof(struct vits_itte);

Please use () around the div operation for clarity


> +}
> +
> +/* 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);

No need for this, once you use __pa instead of the vmap


> +}
> +
>  /**************************************
>   * 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. */

We need to check that the vlpi hasn't already been added to an LR
register. We can do that with GIC_IRQ_GUEST_VISIBLE.

In case GIC_IRQ_GUEST_VISIBLE is set, we need to clear the lr
(clear_lr). If we don't handle this case, we should at least print a
warning.


> +    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. */
> +        if ( !test_bit(GIC_IRQ_GUEST_VISIBLE, &pirq->status) )
> +            pirq->irq = 0;
> +    }
> +
> +    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);
> +            break;
>          case GITS_CMD_SYNC:
>              /* We handle ITS commands synchronously, so we ignore SYNC. */
>           break;
> -- 
> 2.9.0
> 

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