Re: [PATCH v4 06/17] xen/arm: ITS: Add virtual ITS driver

[Ian Campbell <ijc@xen.org>]

On Fri, 2015-07-10 at 13:12 +0530, vijay.kilari@gmail.com wrote:
> +static int vits_entry(struct domain *d, paddr_t entry, void *addr,
> +                      uint32_t size, bool_t set)
> +{
> [...]
> +}
> +
> +/* ITS device table helper functions */
> +static int vits_vdevice_entry(struct domain *d, uint32_t dev_id,
> +                              struct vdevice_table *entry, bool_t set)
> +{
> +    uint64_t offset;
> +    paddr_t dt_entry;
> +
> +    BUILD_BUG_ON(sizeof(struct vdevice_table) != 16);
> +
> +    offset = dev_id * sizeof(struct vdevice_table);
> +    if ( offset > d->arch.vits->dt_size )
> +    {
> +        dprintk(XENLOG_G_ERR,
> +                "%pv: vITS: Out of range offset %ld id 0x%x size %ld\n",
> +                current, offset, dev_id, d->arch.vits->dt_size);
> +        return -EINVAL;
> +    }
> +
> +    dt_entry = d->arch.vits->dt_ipa + offset;
> +
> +    return vits_entry(d, dt_entry, (void *)entry,
> +                      sizeof(struct vdevice_table),

Please drop the (void *) cast here, you can pass a "foo *" to a "void *"
without one.

It took me a little while to work out why this was void * before I
realised that vits_entry was a generic helper used for different types
of table. "vits_access_guest_table" to make it clear what it is doing.

> +static int vits_vitt_entry(struct domain *d, uint32_t devid,
> +                           uint32_t event, struct vitt *entry, bool_t set)
> +{
> +    struct vdevice_table dt_entry;
> +    paddr_t vitt_entry;
> +    uint64_t offset;
> +
> +    BUILD_BUG_ON(sizeof(struct vitt) != 8);
> +
> +    if ( vits_get_vdevice_entry(d, devid, &dt_entry) )
> +    {
> +        dprintk(XENLOG_G_ERR, "%pv: vITS: Fail to get vdevice for dev 0x%x\n",
> +                current, devid);
> +        return -EINVAL;
> +    }
> +
> +    /* dt_entry is validated when read */

Please can you say "is validated by vits_get_vdevice_entry" to be very
clear.

> +    offset = event * sizeof(struct vitt);
> +    if ( offset > dt_entry.vitt_size )
> +    {
> +        dprintk(XENLOG_G_ERR, "%pv: vITS: ITT out of range\n", current);
> +        return -EINVAL;
> +    }
> +   
> +    vitt_entry = dt_entry.vitt_ipa + offset;
> +
> +    return vits_entry(d, vitt_entry, (void *)entry,
> +                      sizeof(struct vitt), set);

Same story WRT the cast.

[...]
> @@ -171,11 +184,41 @@ struct its_device {
>      struct rb_node          node;
>  };
>  
> +struct vits_device {
> +    uint32_t vdevid;
> +    struct its_device *its_dev;
> +    struct rb_node node;
> +};
> +

Please add a big comment here:

 /*
  * struct vdevice_table and struct vitt are typically stored in memory
  * which has been provided by the guest out of its own address space 
  * and which remains accessible to the guest.
  *
  * Therefore great care _must_ be taken when accessing an entry in
  * either table to validate the sanity of any values which are used
  */

> +struct vdevice_table {
> +    uint64_t vitt_ipa;
> +    uint32_t vitt_size;
> +    uint32_t padding;
> +};
> +
> +struct vitt {
> +    uint16_t valid:1;
> +    uint16_t pad:15;
> +    uint16_t vcollection;
> +    uint32_t vlpi;
> +};

Ian.