Re: [PATCH v6 15/31] xen/arm: ITS: Add virtual ITS driver

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

Hi Vijay,

On 31/08/15 12:06, vijay.kilari@gmail.com wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> 
> This patch introduces virtual ITS driver with following
> functionality
>  - Introduces helper functions to manage device table and
>    ITT table in guest memory
>  - Helper function to handle virtual ITS devices assigned
>    to domain
> 
> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> ---
> v6: - Exported vits_access_guest_memory() api

This is exported because the GICR emulation has to use it, right?

If so, there is a slight problem. The function is misnamed and gives the
impression that the GICv3 property table has to use the vITS. This is
not true. The vITS makes usage of the GICv3 and not the invert.

I would prefer to see this function either in vgic-v3.c or vgic.c.
Overall, this function is generic enough to be in the common framework.

Of course you would need to rename the function ;).

> v5: - Removed RB tree that manages vitual ITS devices
> v4: - Rename functions {find,remove,insert}_vits_* to
>       vits_{find,remove,insert}.
>     - Add common helper function to map and read/write dt
>       or vitt table entry.
>     - Removed unused code
> ---
>  xen/arch/arm/vgic-v3-its.c    |  162 +++++++++++++++++++++++++++++++++++++++++
>  xen/include/asm-arm/domain.h  |    2 +
>  xen/include/asm-arm/gic-its.h |   38 ++++++++++
>  3 files changed, 202 insertions(+)
> 
> diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c

[...]

> +/* 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;
> +    const struct vgic_its *vits = d->arch.vgic.vits;
> +
> +    BUILD_BUG_ON(sizeof(struct vdevice_table) != 16);
> +
> +    offset = dev_id * sizeof(struct vdevice_table);
> +    if ( offset > vits->dt_size )
> +    {
> +        printk(XENLOG_G_ERR
> +               "d%"PRIu16":vITS:Out of range off 0x%"PRIx64" id 0x%"PRIx32"\n",

NIT: Please use the same format everywhere. I.e space after colon. For
instance:

"d%"PRIu16": vITS: My msg"


> +               d->domain_id, offset, dev_id);
> +        return -EINVAL;
> +    }
> +
> +    dt_entry = vits->dt_ipa + offset;
> +
> +    return vits_access_guest_table(d, dt_entry, entry,
> +                                   sizeof(struct vdevice_table), set);
> +}
> +
> +static 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);
> +}
> +
> +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%"PRIu16": vITS: Fail to get vdevice for vdevid 0x%"PRIx32"\n",

NIT: this is technically too long and would have been avoid by using %d
rather than PRIu16 for the domain ID.

FIY, as Ian said on the previous version, we commonly use %d when we
have to print the domain ID.

> +               d->domain_id, devid);
> +        return -EINVAL;
> +    }
> +
> +    /* dt_entry has been validated in vits_get_vdevice_entry */
> +    offset = event * sizeof(struct vitt);
> +    if ( offset > dt_entry.vitt_size )
> +    {
> +        printk(XENLOG_G_ERR "d%"PRIu16": vITS: ITT out of range\n",
> +               d->domain_id);
> +        return -EINVAL;
> +    }
> +
> +    vitt_entry = dt_entry.vitt_ipa + offset;
> +
> +    return vits_access_guest_table(d, vitt_entry, entry,
> +                                   sizeof(struct vitt), set);
> +}
> +
> +static 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);
> +}
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
> index 56aa208..986a4d6 100644
> --- a/xen/include/asm-arm/domain.h
> +++ b/xen/include/asm-arm/domain.h
> @@ -104,6 +104,8 @@ struct arch_domain
>          paddr_t dbase; /* Distributor base address */
>          paddr_t cbase; /* CPU base address */
>  #ifdef HAS_GICV3
> +        /* Virtual ITS */
> +        struct vgic_its *vits;

NIT: I would have prefer to see the addition of this field at the end of
the vgic structure. We don't usually add a field in the middle of a
structure unless there is a good reason.

>          /* GIC V3 addressing */
>          /* List of contiguous occupied by the redistributors */
>          struct vgic_rdist_region {
> diff --git a/xen/include/asm-arm/gic-its.h b/xen/include/asm-arm/gic-its.h
> index 7a46e21..42f6551 100644
> --- a/xen/include/asm-arm/gic-its.h
> +++ b/xen/include/asm-arm/gic-its.h
> @@ -116,6 +116,17 @@ struct its_collection {
>      u16 col_id;
>  };
>  
> +/*
> + * Per domain virtual ITS structure.
> + */
> +struct vgic_its
> +{
> +   /* vITT device table ipa */
> +   paddr_t dt_ipa;
> +   /* vITT device table size */
> +   uint64_t dt_size;
> +};
> +
>  /* ITS command structure */
>  typedef union {
>      u64 bits[4];
> @@ -275,6 +286,27 @@ struct its_device {
>      struct rb_node          node;
>  };
>  
> +/*
> + * 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;
> +};
> +

Can you put all the definition of the vITS in a single place rather than
putting them in random place within the header? It would make easier to
read the header latter.

A better solution would be to move the vits structure/function in a new
header.

>  void irqdesc_set_lpi_event(struct irq_desc *desc, unsigned id);
>  unsigned int irqdesc_get_lpi_event(struct irq_desc *desc);
>  struct its_device *irqdesc_get_its_device(struct irq_desc *desc);
> @@ -284,6 +316,12 @@ int its_init(struct rdist_prop *rdists);
>  int its_cpu_init(void);
>  int its_add_device(u32 devid, u32 nr_ites, struct dt_device_node *dt_its);
>  int its_assign_device(struct domain *d, u32 vdevid, u32 pdevid);
> +int vits_access_guest_table(struct domain *d, paddr_t entry, void *addr,
> +                            uint32_t size, bool_t set);
> +int vits_get_vitt_entry(struct domain *d, uint32_t devid,
> +                        uint32_t event, struct vitt *entry);
> +int vits_get_vdevice_entry(struct domain *d, uint32_t devid,
> +                           struct vdevice_table *entry);
>  #endif /* __ASM_ARM_GIC_ITS_H__ */
>  /*
> 

Regards,

-- 
Julien Grall