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

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

Hi Vijay,

On 10/07/2015 09:42, vijay.kilari@gmail.com wrote:
> +static int vits_entry(struct domain *d, paddr_t entry, void *addr,
> +                      uint32_t size, bool_t set)
> +{
> +    struct page_info *page;
> +    uint64_t offset;
> +    p2m_type_t p2mt;
> +    void *p;
> +
> +    page = get_page_from_gfn(d, paddr_to_pfn(entry), &p2mt, P2M_ALLOC);
> +    if ( !page )
> +    {
> +        dprintk(XENLOG_G_ERR, "%pv: vITS: Failed to get table entry\n",
> +                current);

The function vits_entry will be used when an LPI is injected to the 
guest. At that time, current may not be a VCPU of the domain d.

Therefore, this log will be very confusing if an error occur. I would 
prefer if you only print the domain id using d.

Furthermore, dprintk is a nop on non-debug build. You may want to use 
printk here.

Those comments are valid in every dprintk of this patch.

[..]

> +/* 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), set);
> +}
> +
> +int vits_set_vdevice_entry(struct domain *d, uint32_t devid,
> +                           struct vdevice_table *entry)
> +{
> +    return vits_vdevice_entry(d, devid, entry, 1);
> +}
> +
> +int vits_get_vdevice_entry(struct domain *d, uint32_t devid,
> +                           struct vdevice_table *entry)
> +{
> +    return vits_vdevice_entry(d, devid, entry, 0);
> +}

While I can understand that you export vits_get_vdevice_entry, it's used 
in the LPI injection. I don't understand for vits_set_vdevice_entry.

> +
> +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",

s/dev/vdevid/

> +                current, devid);
> +        return -EINVAL;
> +    }
> +
> +    /* dt_entry is validated when read */
> +    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);
> +}
> +
> +int vits_set_vitt_entry(struct domain *d, uint32_t devid,
> +                        uint32_t event, struct vitt *entry)
> +{
> +    return vits_vitt_entry(d, devid, event, entry, 1);
> +}
> +
> +int vits_get_vitt_entry(struct domain *d, uint32_t devid,
> +                        uint32_t event, struct vitt *entry)
> +{
> +    return vits_vitt_entry(d, devid, event, entry, 0);
> +}

Same remark as vits_*_device_entry.

[..]

> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
> index f1a087e..67e4695 100644
> --- a/xen/include/asm-arm/domain.h
> +++ b/xen/include/asm-arm/domain.h
> @@ -115,6 +115,10 @@ struct arch_domain
>   #endif
>       } vgic;
>
> +#ifdef CONFIG_ARM_64
> +    struct vgic_its *vits;
> +#endif
> +

I would prefer to see this field part of the vgic structure (see just 
above). There is already a #ifdef for GICv3 stuff so it will avoid 2 new 
lines.

>       struct vuart {
>   #define VUART_BUF_SIZE 128
>           char                        *buf;
> diff --git a/xen/include/asm-arm/gic-its.h b/xen/include/asm-arm/gic-its.h
> index e8d244f..d21aefe 100644
> --- a/xen/include/asm-arm/gic-its.h
> +++ b/xen/include/asm-arm/gic-its.h
> @@ -31,7 +31,20 @@ struct its_collection {
>       u16 col_id;
>   };
>
> -/* ITS command structure */
> +/*
> + * Per domain virtual ITS structure.
> + */
> +struct vgic_its
> +{
> +   /* vITT device table ipa */
> +   paddr_t dt_ipa;
> +   /* vITT device table size */
> +   uint64_t dt_size;
> +   /* Radix-tree root of devices attached to this domain */
> +   struct rb_root dev_root;
> +};
> +
> +/* ITS command structures */

Please fix the typo in the patch where you introduced it. Not after.

Regards,

-- 
Julien Grall