Re: [PATCH v6 16/31] xen/arm: ITS: Add virtual ITS commands support

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

Hi Vijay,

This patch looks good to me. Mostly coding style comment and question
about your code/comments. See below.

On 31/08/15 12:06, vijay.kilari@gmail.com wrote:
> diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c
> index 14c38b3..fabbad0 100644
> --- a/xen/arch/arm/vgic-v3-its.c
> +++ b/xen/arch/arm/vgic-v3-its.c

[...]

> +bool_t is_valid_collection(struct domain *d, uint32_t col)

You should have define the prototype of is_valid_collection within this
patch rather than in patch #13.

> +{
> +    return (col <= (d->max_vcpus + 1));

col <= (vtis_get_max_collections(d)) to avoid hardcoding the number of
collection in multiple place.

> +}
> +
> +static inline uint16_t vits_get_max_collections(struct domain *d)
> +{
> +    /*
> +     * ITS only supports upto 256 collections without

up to

> +     * provisioning external memory. As per vITS design, number of
> +     * vCPUS should not exceed max number of collections.
> +     */
> +    ASSERT(d->max_vcpus < 256);

The problem with ASSERT is it's disappearing with non-debug build. Do
you ensure somewhere that it's never possible to create a domain with
more than 256 CPUs when vITS is in use?

Otherwise, bad things would happen if we are trying to create a guest
with more than 256 vCPUs.

> +
> +    return (d->max_vcpus + 1);

As said on v5, please add a comment to explain why d->max_vcpus + 1. It
could be a reference to the public spec...

> +}
> +

[...]

> +static int vits_process_int(struct vcpu *v, struct vgic_its *vits,
> +                            its_cmd_block *virt_cmd)
> +{
> +    uint32_t event, dev_id ;

Spurious space after dev_id.

> +
> +    event = virt_cmd->int_cmd.cmd;
> +    dev_id = virt_cmd->int_cmd.devid;
> +
> +    DPRINTK("%pv: vITS: INT: Device 0x%"PRIx32" id %"PRIu32"\n",
> +            v, dev_id, event);
> +
> +    /* TODO: Inject LPI */
> +
> +    return 0;
> +}


[...]

> +static int vits_process_mapc(struct vcpu *v, struct vgic_its *vits,
> +                             its_cmd_block *virt_cmd)
> +{
> +    uint16_t vcol_id;
> +    uint64_t vta = 0;

Setting vta to 0 is not necessary as you override the value 2 lines below.

Note that you could have directly assign the value in the declaration of
the variables. It would have drop 2 lines.

> +
> +    vcol_id = virt_cmd->mapc.col;
> +    vta = virt_cmd->mapc.ta;
> +
> +    DPRINTK("%pv: vITS: MAPC: vCID %"PRIu16" vTA 0x%"PRIx64" valid %"PRIu8"\n",
> +            v, vcol_id, vta, virt_cmd->mapc.valid);
> +
> +    if ( !is_valid_collection(v->domain, vcol_id) )
> +        return -EINVAL;
> +
> +    if ( virt_cmd->mapc.valid )
> +    {
> +        if ( vta > v->domain->max_vcpus )
> +            return -EINVAL;
> +        vits->collections[vcol_id].target_address = vta;
> +    }
> +    else
> +        vits->collections[vcol_id].target_address = INVALID_PADDR;
> +
> +    return 0;
> +}

[...]

> +static int vits_read_virt_cmd(struct vcpu *v, struct vgic_its *vits,
> +                              its_cmd_block *virt_cmd)
> +{
> +    paddr_t maddr;
> +    struct domain *d = v->domain;
> +    int ret;
> +
> +    ASSERT(spin_is_locked(&vits->lock));
> +
> +    if ( !(vits->cmd_base & GITS_CBASER_VALID) )
> +    {
> +        dprintk(XENLOG_G_ERR, "%pv: vITS: Invalid CBASER\n", v);
> +        return 0;
> +    }
> +
> +    /* Map only the page that is required */

IHMO the "CMD Q can be more than 1 page" was valid and useful. Sorry if
I wasn't enough clear that I wanted the typo fixed on the second sentence.

> +    maddr = (vits->cmd_base & GITS_CBASER_PA_MASK) +
> +             atomic_read(&vits->cmd_read);
> +
> +    DPRINTK("%pv: vITS: Mapping CMD Q maddr 0x%"PRIx64" read 0x%"PRIx32"\n",
> +            v, maddr, atomic_read(&vits->cmd_read));
> +
> +    ret = vits_access_guest_table(d, maddr, (void *)virt_cmd,
> +                                  sizeof(its_cmd_block), 0);
> +    if ( ret )
> +    {
> +        dprintk(XENLOG_G_ERR,
> +                "%pv: vITS: Failed to get command page @page 0x%"PRIx32"\n",
> +                v, atomic_read(&vits->cmd_read));
> +        return -EINVAL;
> +    }
> +
> +    /* No command queue is created by vits to check on Q full */

I don't understand this comment. What do you mean?

> +    atomic_add(sizeof(its_cmd_block), &vits->cmd_read);
> +    if ( atomic_read(&vits->cmd_read) == vits->cmd_qsize )
> +    {
> +         DPRINTK("%pv: vITS: Reset read @ 0x%"PRIx32" qsize 0x%"PRIx64"\n",
> +                 v, atomic_read(&vits->cmd_read), vits->cmd_qsize);
> +
> +         atomic_set(&vits->cmd_read, 0);
> +    }
> +
> +    return 0;
> +}

[...]

> +int vits_domain_init(struct domain *d)
> +{
> +    struct vgic_its *vits;
> +    int i;
> +
> +    ASSERT(is_hardware_domain(d));
> +
> +    d->arch.vgic.vits = xzalloc(struct vgic_its);
> +    if ( !d->arch.vgic.vits )
> +        return -ENOMEM;
> +
> +    vits = d->arch.vgic.vits;
> +
> +    spin_lock_init(&vits->lock);
> +
> +    vits->collections = xzalloc_array(struct its_collection,
> +                                      vits_get_max_collections(d));
> +    if ( !vits->collections )
> +        return -ENOMEM;
> +
> +    for ( i = 0; i < vits_get_max_collections(d); i++ )
> +        vits->collections[i].target_address = ~0UL;

You are using 2 different values for the invalid address: ~0UL and
INVALID_PADDR.

They are not defined the same way and even though they may give the same
result for target_address by luck you should always use the same
definition everywhere.

I would prefer to see INVALID_PADDR even though a define
VITS_TARGET_INVALID (or similar) would have been nice. Although, I'm
fine if you don't add the define.

> +
> +    return 0;
> +}

[...]

> diff --git a/xen/include/asm-arm/gic-its.h b/xen/include/asm-arm/gic-its.h
> index 42f6551..4327ba2 100644
> --- a/xen/include/asm-arm/gic-its.h
> +++ b/xen/include/asm-arm/gic-its.h
> @@ -21,6 +21,7 @@
>  #include <asm/gic_v3_defs.h>
>  #include <xen/rbtree.h>
>  
> +#define MAPC_ITT_IPA_SHIFT              8

I would much prefer to see this define with the definition of the
command structure.

>  /*
>   * ITS registers, offsets from ITS_base
>   */
> @@ -59,6 +60,7 @@
>  #define GITS_CBASER_InnerShareable      (1UL << 10)
>  #define GITS_CBASER_SHAREABILITY_MASK   (3UL << 10)
>  #define GITS_CBASER_CACHEABILITY_MASK   (7UL << 59)
> +#define GITS_CBASER_PA_MASK             (0xfffffffff000UL)
>  
>  #define GITS_BASER_NR_REGS              8
>  

Regards,

-- 
Julien Grall