Re: [PATCH v5 07/22] xen/arm: ITS: Add virtual ITS driver

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

Hi Vijay,

On 27/07/15 12:11, vijay.kilari@gmail.com wrote:
> diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c
> new file mode 100644
> index 0000000..60f8332
> --- /dev/null
> +++ b/xen/arch/arm/vgic-v3-its.c
> +static int vits_access_guest_table(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 )
> +    {
> +        printk(XENLOG_G_ERR "d%"PRId32": vITS: Failed to get table entry\n",

A domain id is encoded on 16 bits and not 32 bits. Furthermore it's an
unsigned. Here you will print signed.

My remark will be the same for all similar use within 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;
> +    struct vgic_its *vits = d->arch.vgic.vits;

const struct

[..]

> +int vits_set_vdevice_entry(struct domain *d, uint32_t devid,
> +                           struct vdevice_table *entry)
> +{
> +    return vits_vdevice_entry(d, devid, entry, 1);
> +}

The prototype is not specified in the header. So either you need to add
the prototype, if used outside the file, or set static.

[..]

> +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) )
> +    {
> +        printk(XENLOG_G_ERR
> +                "d%"PRId32": vITS: Fail to get vdevice for vdev 0x%"PRIx32"\n",

s/vdev/vdevid/

> +                d->domain_id, devid);
> +        return -EINVAL;
> +    }
> +
> +    /* dt_entry is validated in vits_get_vdevice_entry */

s/is validated/has been validated/

[..]

> +int vits_set_vitt_entry(struct domain *d, uint32_t devid,
> +                        uint32_t event, struct vitt *entry)

Same remark as vits_set_vdevice_entry.

Regards,

-- 
Julien Grall