Re: [PATCH v2 19/27] ARM: vITS: handle MAPD command

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

Hi Andre,

On 03/16/2017 11:20 AM, Andre Przywara wrote:
> The MAPD command maps a device by associating a memory region for
> storing ITEs with a certain device ID.
> We store the given guest physical address in the device table, and, if
> this command comes from Dom0, tell the host ITS driver about this new
> mapping, so it can issue the corresponing host MAPD command and create
> the required tables.
> We don't map the device tables permanently, as their alignment
> requirement is only 256 Bytes, thus making mapping of several tables
> complicated. Instead we map the device tables on demand when we need
> them later.
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  xen/arch/arm/vgic-v3-its.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 44 insertions(+)
>
> diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c
> index 175a213..c26d5d4 100644
> --- a/xen/arch/arm/vgic-v3-its.c
> +++ b/xen/arch/arm/vgic-v3-its.c
> @@ -38,6 +38,7 @@
>  /* Data structure to describe a virtual ITS */
>  struct virt_its {
>      struct domain *d;
> +    paddr_t doorbell_address;
>      spinlock_t vcmd_lock;       /* protects the virtual command buffer */
>      uint64_t cbaser;
>      uint64_t *cmdbuf;
> @@ -291,6 +292,46 @@ static int its_handle_mapc(struct virt_its *its, uint64_t *cmdptr)
>      return ret;
>  }
>
> +static int its_handle_mapd(struct virt_its *its, uint64_t *cmdptr)
> +{
> +    uint32_t devid = its_cmd_get_deviceid(cmdptr);
> +    int size = its_cmd_get_size(cmdptr);
> +    bool valid = its_cmd_get_validbit(cmdptr);
> +    paddr_t itt_addr = its_cmd_mask_field(cmdptr, 2, 0, 52) & GENMASK(51, 8);
> +    int ret;
> +
> +    if ( !its->dev_table )
> +        return -1;
> +
> +    /*
> +     * There is no easy and clean way for Xen to know the ITS device ID of a
> +     * particular (PCI) device, so we have to rely on the guest telling
> +     * us about it. For *now* we are just using the device ID *Dom0* uses,
> +     * because the driver there has the actual knowledge.
> +     * Eventually this will be replaced with a dedicated hypercall to
> +     * announce pass-through of devices.
> +     */
> +    if ( is_hardware_domain(its->d) )
> +    {
> +        /* Dom0's ITSes are mapped 1:1, so both address are the same. */
> +        ret = gicv3_its_map_guest_device(its->d, its->doorbell_address, devid,
> +                                         its->doorbell_address, devid,
> +                                         BIT(size + 1), valid);

The size should be satinized against the number of event support by the 
vITS.

And it should also be sanitzed in gicv3_its_map_* against the number of 
event supported by the pITS.

> +        if ( ret )
> +            return ret;
> +    }
> +
> +    spin_lock(&its->its_lock);
> +    if ( valid )
> +        its->dev_table[devid] = DEV_TABLE_ENTRY(itt_addr, size + 1);
> +    else
> +        its->dev_table[devid] = 0;
> +
> +    spin_unlock(&its->its_lock);
> +
> +    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,
> @@ -320,6 +361,9 @@ static int vgic_its_handle_cmds(struct domain *d, struct virt_its *its,
>          case GITS_CMD_MAPC:
>              its_handle_mapc(its, cmdptr);
>              break;
> +        case GITS_CMD_MAPD:
> +            its_handle_mapd(its, cmdptr);

Should not you check the return value?

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

Cheers,

-- 
Julien Grall

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