Re: [RFC PATCH v3 08/18] xen/arm: vITS: Add virtual ITS driver

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

Hi Vijay,

On 22/06/15 13:01, 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>
> ---
>  xen/arch/arm/Makefile         |    1 +
>  xen/arch/arm/vgic-v3-its.c    |  266 +++++++++++++++++++++++++++++++++++++++++
>  xen/include/asm-arm/domain.h  |    2 +
>  xen/include/asm-arm/gic-its.h |   49 ++++++++
>  4 files changed, 318 insertions(+)
> 
> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
> index 1821ed2..8590846 100644
> --- a/xen/arch/arm/Makefile
> +++ b/xen/arch/arm/Makefile
> @@ -33,6 +33,7 @@ obj-y += traps.o
>  obj-y += vgic.o vgic-v2.o
>  obj-$(CONFIG_ARM_64) += vgic-v3.o
>  obj-$(CONFIG_ARM_64) += gic-v3-its.o
> +obj-$(CONFIG_ARM_64) += vgic-v3-its.o

I would prefer to the vgic-v3-its code not compiled until you finish to
implement it rather than having every function exported while it should
be static.

>  obj-y += vtimer.o
>  obj-y += vuart.o
>  obj-y += hvm.o
> diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c
> new file mode 100644
> index 0000000..ea52a87
> --- /dev/null
> +++ b/xen/arch/arm/vgic-v3-its.c

[..]

> +/* GITS register definitions */
> +#define VITS_GITS_TYPER_HCC       (0xffU << 24)

Where does this value comes from? The number of supported collection
should be the maximum number of VCPUs supported by the domain.

Note that the hardware can only hold 256 collections, all the other
requires provisioning. But it's fine for now, although a BUILD_BUG_ON in
the vGIC code would be nice.

> +#define VITS_GITS_TYPER_PTA_SHIFT (19)
> +#define VITS_GITS_DEV_BITS        (0x14U << 13)
> +#define VITS_GITS_ID_BITS         (0x13U << 8)

Where do those values come from? This needs at least to come from the
hardware GIC configuration if we plan to only support DOM0 for now.

> +#define VITS_GITS_ITT_SIZE        (0x7U << 4)

The name is misleading, this is not the size of the ITT but the value to
store in the GITS_TYPER register.

> +#define VITS_GITS_DISTRIBUTED     (0x1U << 3)

Why? This bit is implementation defined in the spec.

> +#define VITS_GITS_PLPIS           (0x1U << 0)

For all these defines, why do you hardcode the shift rather using
GITS_TYPER_* you provide in patch #5? It would make the code more easier
to understand.

> +
> +/* GITS_PIDRn register values for ARM implementations */
> +#define GITS_PIDR0_VAL            (0x94)
> +#define GITS_PIDR1_VAL            (0xb4)
> +#define GITS_PIDR2_VAL            (0x3b)
> +#define GITS_PIDR3_VAL            (0x00)
> +#define GITS_PIDR4_VAL            (0x04)
> +
> +// #define DEBUG_ITS
> +
> +#ifdef DEBUG_ITS
> +# define DPRINTK(fmt, args...) dprintk(XENLOG_DEBUG, fmt, ##args)
> +#else
> +# define DPRINTK(fmt, args...) do {} while ( 0 )
> +#endif
> +
> +#ifdef DEBUG_ITS
> +static void dump_cmd(its_cmd_block *cmd)
> +{
> +    printk("CMD[0] = 0x%lx CMD[1] = 0x%lx CMD[2] = 0x%lx CMD[3] = 0x%lx\n",
> +           cmd->raw_cmd[0], cmd->raw_cmd[1], cmd->raw_cmd[2], cmd->raw_cmd[3]);
> +}
> +#endif
> +
> +/* ITS device table helper functions */
> +int vits_vdevice_entry(struct domain *d, uint32_t dev_id,
> +                       struct vdevice_table *entry, int set)

Any functions not exported should be static. I won't repeat for all the
others within this file.

Also, please use bool_t for set.

> +{
> +    uint64_t offset;
> +    paddr_t dt_entry;
> +    struct page_info *page;
> +    p2m_type_t p2mt;
> +    void *p;
> +
> +    offset = dev_id * sizeof(struct vdevice_table);
> +    if ( offset > d->arch.vits->dt_size )

If you gonna let the guest to write deci

> +    {
> +        dprintk(XENLOG_G_ERR,
> +                "vITS:d%dv%d: Out of range offset 0x%lx id 0x%x size 0x%lx\n",

You can use %pv to print the domain/vcpuid (see an example in vgic-v3.c).

Also, all the value can be printed in decimal because we have to use
them for addition...

> +                d->domain_id, current->vcpu_id, offset, dev_id,

Please don't mix d and current.

> +                d->arch.vits->dt_size);
> +        return -EINVAL;
> +    }
> +
> +    dt_entry = d->arch.vits->dt_ipa + offset;
> +
> +    page = get_page_from_gfn(d, paddr_to_pfn(dt_entry), &p2mt, P2M_ALLOC);
> +    if ( !page )
> +    {
> +        dprintk(XENLOG_G_ERR, "vITS:d%dv%d Failed to get VITT device table\n",
> +                d->domain_id, current->vcpu_id);
> +        return -EINVAL;
> +    }
> +
> +    if ( !p2m_is_ram(p2mt) )
> +    {
> +        put_page(page);
> +        dprintk(XENLOG_G_ERR, "vITS:d%dv%d with wrong attributes\n",
> +                d->domain_id, current->vcpu_id);
> +        return -EINVAL;
> +        return -EINVAL;

Duplicate "return -EINVAL".

> +    }
> +
> +    p = __map_domain_page(page);
> +    /* Offset within the mapped page */
> +    offset = dt_entry & (PAGE_SIZE - 1);
> +
> +    if ( set )
> +        memcpy(p + offset, entry, sizeof(struct vdevice_table));
> +    else
> +        memcpy(entry, p + offset, sizeof(struct vdevice_table));
> +
> +    unmap_domain_page(p);
> +    put_page(page);
> +
> +    return 0;
> +}
> +
> +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);  
> +}
> +
> +int vits_vitt_entry(struct domain *d, uint32_t devid,
> +                    uint32_t event, struct vitt *entry, int set)
> +{
> +    struct vdevice_table dt_entry;
> +    struct page_info *page;
> +    paddr_t vitt_entry;
> +    p2m_type_t p2mt;
> +    uint64_t offset;
> +    void *p;
> +
> +    if ( vits_get_vdevice_entry(d, devid, &dt_entry) )
> +    {
> +        dprintk(XENLOG_G_ERR, "vITS:d%dv%d Fail to get vdevice for dev 0x%x\n",
> +                d->domain_id, current->vcpu_id, devid);
> +        return -EINVAL;
> +    }
> +
> +    /* dt_entry is validated when read */
> +    offset = event * sizeof(struct vitt);
> +    if ( offset > dt_entry.vitt_size )
> +    {
> +        dprintk(XENLOG_G_ERR, "vITS:d%dv%d ITT out of range\n",
> +                d->domain_id, current->vcpu_id);
> +        return -EINVAL;
> +    }
> +   
> +    vitt_entry = dt_entry.vitt_ipa + offset;
> +    page = get_page_from_gfn(d, paddr_to_pfn(vitt_entry), &p2mt, P2M_ALLOC);
> +    if ( !page )
> +    {
> +        dprintk(XENLOG_G_ERR, "vITS:d%dv%d Failed to get VITT device table\n",
> +                d->domain_id, current->vcpu_id);
> +        return -EINVAL;
> +    }
> +
> +    if ( !p2m_is_ram(p2mt) )
> +    {
> +        put_page(page);
> +        dprintk(XENLOG_G_ERR, "vITS:d%dv%d with wrong attributes\n",
> +                d->domain_id, current->vcpu_id);
> +        return -EINVAL;
> +    }
> +
> +    p = __map_domain_page(page);
> +    /* Offset within the mapped page */
> +    offset = vitt_entry & (PAGE_SIZE - 1);
> +
> +    if ( set )
> +        memcpy(p + offset, entry, sizeof(struct vitt));
> +    else
> +        memcpy(entry, p + offset, sizeof(struct vitt));
> +
> +    unmap_domain_page(p);
> +    put_page(page);

The code to transfer the data is exactly the same as vits_vdevice_entry
except the size of the copy. Can you make an helper? It would avoid
duplicate code.

> +
> +    return 0;
> +}
> +
> +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);
> +}
> +
> +/* RB-tree helpers for vits_device attached to a domain */
> +struct vits_device * find_vits_device(struct rb_root *root, uint32_t devid)

Coding style: vits_device *find_

> +{
> +    struct rb_node *node = root->rb_node;
> +
> +    while ( node )
> +    {
> +        struct vits_device *dev;
> +
> +        dev = container_of(node, struct vits_device, node);
> +
> +        if ( devid < dev->vdevid )
> +            node = node->rb_left;
> +        else if ( devid > dev->vdevid )
> +            node = node->rb_right;
> +        else
> +            return dev;
> +    }
> +
> +    return NULL;
> +}
> +
> +int insert_vits_device(struct rb_root *root, struct vits_device *dev)
> +{
> +    struct rb_node **new, *parent;
> +
> +    new = &root->rb_node;
> +    parent = NULL;
> +    while ( *new )
> +    {
> +        struct vits_device *this;
> +
> +        this  = container_of(*new, struct vits_device, node);
> +
> +        parent = *new;
> +        if ( dev->vdevid < this->vdevid )
> +            new = &((*new)->rb_left);
> +        else if ( dev->vdevid > this->vdevid )
> +            new = &((*new)->rb_right);
> +        else
> +            return -EEXIST;
> +    }
> +
> +    rb_link_node(&dev->node, parent, new);
> +    rb_insert_color(&dev->node, root);
> +
> +    return 0;
> +}
> +
> +int remove_vits_device(struct rb_root *root, struct vits_device *dev)

This function can be void.

> +{
> +    if ( dev )
> +        rb_erase(&dev->node, root);
> +
> +    return 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 f1a087e..da73cf5 100644
> --- a/xen/include/asm-arm/domain.h
> +++ b/xen/include/asm-arm/domain.h
> @@ -115,6 +115,8 @@ struct arch_domain
>  #endif
>      } vgic;
>  
> +    struct vgic_its *vits;
> +
>      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 a47cf26..a1099a1 100644
> --- a/xen/include/asm-arm/gic-its.h
> +++ b/xen/include/asm-arm/gic-its.h
> @@ -31,6 +31,35 @@ struct its_collection {
>      u16 col_id;
>  };
>  
> +/*
> + * Per domain virtual ITS structure.
> + * One per Physical ITS node available for the domain

Left over of the previous version? AFAICT, the agreed design [1] only
expose one vITS per domain.


> + */
> +struct vgic_its
> +{
> +   spinlock_t lock;
> +   /* Command queue base */
> +   paddr_t cmd_base;
> +   /* Command queue write pointer */
> +   paddr_t cmd_write;
> +   /* Command queue write saved pointer */
> +   paddr_t cmd_write_save;
> +   /* Command queue read pointer */
> +   paddr_t cmd_read;
> +   /* Command queue size */
> +   unsigned long cmd_qsize;
> +   /* ITS physical node */
> +   struct its_node *its;

This is not necessary.

> +   /* 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;
> +   /* collections mapped */
> +   struct its_collection *collections;
> +};
> +
>  /* ITS command structures */
>  typedef struct __packed {
>      u64 cmd:8;
> @@ -200,6 +229,26 @@ struct its_device {
>      struct rb_node          node;
>  };
>  
> +struct vits_device {
> +    uint32_t vdevid;
> +    uint32_t pdevid;
> +    struct its_device *its_dev;
> +    struct rb_node node;
> +};

We spoke about a specific structure in the design [2] but you introduced
a new one. Why?

Having everything in the its_device would help to catch a device
attached to 2 different domains...

Also, the field pdevid is not vits specific but its.

> +
> +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;
> +};

You forgot to add the BUILD_BUG_ON for each structure here. It's very
important as you decided to hardcoded the value in the registers.

> +
>  static inline uint8_t its_decode_cmd(its_cmd_block *cmd)
>  {
>      return cmd->raw_cmd[0] & 0xff;
> 

Regards,

[1]
http://xenbits.xensource.com/people/ianc/vits/draftF.html#vits-to-pits-mapping
[2]
http://xenbits.xensource.com/people/ianc/vits/draftF.html#vits-to-pits-mapping


-- 
Julien Grall