Re: [PATCH v4 07/17] xen/arm: ITS: Add virtual ITS commands support

[Ian Campbell <ian.campbell@citrix.com>]

On Fri, 2015-07-10 at 13:12 +0530, vijay.kilari@gmail.com wrote:
[...]
> +static int vgic_its_process_int(struct vcpu *v, struct vgic_its *vits,
> +                                its_cmd_block *virt_cmd)
> +{
> [...]
> +
> +    col_id = vitt_entry.vcollection;
> +    if ( col_id < d->max_vcpus )

I think the condition here is backwards? And might be missing a + 1?

I think you've ended up open coding this max_vcpus+1 a lot. I think you
should encapsulate it into an valid_vcollection(d, col_id) helper and
use it throughout.

> +    {
> +        dprintk(XENLOG_G_ERR,
> +                "%pv: vITS: INT CMD invalid col_id  %d for dev 0x%x\n",
> +                v, col_id, dev_id);
> +        return -EINVAL;
> +    }
> +
> +    vgic_vcpu_inject_irq(d->vcpu[col_id], vitt_entry.vlpi);
> +
> +    return 0;
> +}
> +
> +static int vgic_its_add_device(struct vcpu *v, struct vgic_its *vits,
> +                               its_cmd_block *virt_cmd)
> +{
> +    struct domain *d = v->domain;
> +    struct vdevice_table dt_entry;
> +    uint32_t dev_id = virt_cmd->mapd.devid;
> +
> +    DPRINTK("%pv: vITS: Add device dev_id 0x%x vitt_ipa = 0x%lx size %d\n",
> +            v, dev_id, (u64)virt_cmd->mapd.itt << 8,

Where you have uintXX_t types being printed please always use %"PRIxXX"
or %"PRIdXX" etc and not just %x or %lx etc. This is good practice even
in code which is only compiled for 64-bit.

Please also avoid uXX types in favour of uintXX_t (i.e. use the latter)
in code that hasn't come from elsewhere.

Both of these comments likely apply to all sort of bits of this series.


> +static int vgic_its_process_mapc(struct vcpu *v, struct vgic_its *vits,
> +                                 its_cmd_block *virt_cmd)
> +{
> +    struct domain *d = v->domain;
> +    uint8_t vcol_id;
> +    uint64_t vta = 0;
> +
> +    vcol_id = virt_cmd->mapc.col;
> +    vta = virt_cmd->mapc.ta;
> +
> +    DPRINTK("%pv: vITS: MAPC: vCID %d vTA 0x%lx valid %d \n",
> +            v, vcol_id, vta, virt_cmd->mapc.valid);
> +
> +    if ( vcol_id > (d->max_vcpus + 1) || vta > v->domain->max_vcpus )
> +        return -EINVAL;
> +
> +    if ( virt_cmd->mapc.valid )
> +        d->arch.vits->collections[vcol_id].target_address = vta;
> +    else
> +        d->arch.vits->collections[vcol_id].target_address = ~0UL;

You should use INVALID_PADDR here.
> +int vgic_its_process_cmd(struct vcpu *v, struct vgic_its *vits)
> +{
> +    its_cmd_block virt_cmd;
> +
> +    ASSERT(spin_is_locked(&vits->lock));
> +
> +    do {
> +        if ( vgic_its_read_virt_cmd(v, vits, &virt_cmd) )
> +            goto err;
> +        if ( vgic_its_parse_its_command(v, vits, &virt_cmd) )
> +            goto err;
> +        vgic_its_update_read_ptr(v, vits);
> +    } while ( vits->cmd_write != vits->cmd_write_save );

I can't find anywhere other than here where vits->cmd_write is touched.
What am I missing?

> +    DPRINTK("%pv: vITS: write_save 0x%lx write 0x%lx\n",
> +            v, vits->cmd_write_save,
> +            vits->cmd_write);
> +
> +    return 1;
> +err:
> +    dprintk(XENLOG_G_ERR, "%pv: vITS: Failed to process guest cmd\n", v);
> +    /*XXX: Be nice to guest though we cannot process command? */
> +    return 0;
> +}
> +
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> index 44c2317..fdd96c8 100644
> --- a/xen/include/asm-arm/gic.h
> +++ b/xen/include/asm-arm/gic.h
> @@ -24,6 +24,7 @@
>  #define NR_GIC_LPI         4096
>  #define MAX_LPI            (FIRST_GIC_LPI + NR_GIC_LPI)
>  #define MAX_RDIST_COUNT    4
> +#define BIT_48_12_MASK     0xfffffffff000UL

I think you should use ~PAGE_MASK instead of defining this.

Ian.