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

[Vijay Kilari <vijay.kilari@gmail.com>]

On Tue, Jul 28, 2015 at 10:43 PM, Julien Grall <julien.grall@citrix.com> wrote:
> 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/

I think, to manage within 80 char, I used just "vdev"

>
>> +                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.

I have made non-static for compilation purpose. I will try to introduce
this in the patch where it is used. But it is more logical to have this
in this patch. Anyway I forget to make it static in later patches

>
> Regards,
>
> --
> Julien Grall