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

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

Hi Vijay,

On 10/07/2015 09:42, vijay.kilari@gmail.com wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
>
> Add Virtual ITS command processing support to Virtual ITS driver
>
> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> ---
> v4: - Use helper function to read from command queue
>      - Add MOVALL
>      - Removed check for entry in device in domain RB-tree
> ---
>   xen/arch/arm/gic-v3-its.c     |    7 +
>   xen/arch/arm/vgic-v3-its.c    |  391 +++++++++++++++++++++++++++++++++++++++++
>   xen/include/asm-arm/gic-its.h |   19 ++
>   xen/include/asm-arm/gic.h     |    1 +
>   4 files changed, 418 insertions(+)
>
> diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
> index b98d396..9161053 100644
> --- a/xen/arch/arm/gic-v3-its.c
> +++ b/xen/arch/arm/gic-v3-its.c
> @@ -91,6 +91,7 @@ static LIST_HEAD(its_nodes);
>   static DEFINE_SPINLOCK(its_lock);
>   static struct rdist_prop  *gic_rdists;
>   static struct rb_root rb_its_dev;
> +static struct gic_its_info its_data;
>
>   #define gic_data_rdist()    (per_cpu(rdist, smp_processor_id()))
>
> @@ -102,6 +103,11 @@ void dump_cmd(its_cmd_block *cmd)
>   }
>   #endif
>
> +u32 its_get_nr_events(void)
> +{
> +    return (1 << its_data.id_bits);
> +}
> +

Please give a look to the new vgic infrastructure in order to avoid 
introduced helper to pass data to the vgic.

See for instance vgic_v3_setup_hw.

>   /* RB-tree helpers for its_device */
>   struct its_device *its_find_device(u32 devid)
>   {
> @@ -940,6 +946,7 @@ static int its_probe(struct dt_device_node *node)
>       its->phys_size = its_size;
>       typer = readl_relaxed(its_base + GITS_TYPER);
>       its->ite_size = ((typer >> 4) & 0xf) + 1;
> +    its_data.id_bits = GITS_TYPER_IDBITS(typer);
>
>       its->cmd_base = xzalloc_bytes(ITS_CMD_QUEUE_SZ);
>       if ( !its->cmd_base )
> diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c
> index c63f478..af2bacd 100644
> --- a/xen/arch/arm/vgic-v3-its.c
> +++ b/xen/arch/arm/vgic-v3-its.c
> @@ -31,6 +31,22 @@
>   #include <asm/gic-its.h>
>   #include <xen/log2.h>
>
> +#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->bits[0], cmd->bits[1], cmd->bits[2], cmd->bits[3]);
> +}
> +#endif
> +
>   static int vits_entry(struct domain *d, paddr_t entry, void *addr,
>                         uint32_t size, bool_t set)
>   {
> @@ -202,6 +218,381 @@ void vits_remove_device(struct rb_root *root, struct vits_device *dev)
>           rb_erase(&dev->node, root);
>   }
>
> +static int vgic_its_process_sync(struct vcpu *v, struct vgic_its *vits,
> +                                 its_cmd_block *virt_cmd)
> +{

While the "XXX" wasn't valid, the comment "ignored" was still valid...

> +    DPRINTK("%pv: vITS: SYNC: ta 0x%x \n", v, virt_cmd->sync.ta);
> +
> +    return 0;
> +}
> +
> +static int vgic_its_process_mapvi(struct vcpu *v, struct vgic_its *vits,
> +                                  its_cmd_block *virt_cmd)
> +{
> +    struct vitt entry;
> +    struct domain *d = v->domain;
> +    uint8_t vcol_id, cmd;
> +    uint32_t vid, dev_id, event;
> +
> +    vcol_id = virt_cmd->mapvi.col;
> +    vid = virt_cmd->mapvi.phy_id;
> +    cmd = virt_cmd->mapvi.cmd;
> +    dev_id = virt_cmd->mapvi.devid;
> +
> +    DPRINTK("%pv: vITS: MAPVI: dev_id 0x%x vcol_id %d vid %d \n",
> +             v, dev_id, vcol_id, vid);
> +
> +    if ( vcol_id > (d->max_vcpus + 1) ||  vid > its_get_nr_events() )
> +        return -EINVAL;

As said on v3, checking the validity is pointless as a malicious guest 
can rewrite the
ITT. We only need to check it when the LPI is effectively injected.

If you think this is necessary please explain why...

Furthermore, its_get_nr_events is for the hardware and not the virtual 
ITS. I would prefer to see a field in the vits structure which contains 
the number of event ID bits for a given domain.

[...]

> +static int vgic_its_process_movi(struct vcpu *v, struct vgic_its *vits,
> +                                 its_cmd_block *virt_cmd)
> +{
> +    struct vitt entry;
> +    struct domain *d = v->domain;
> +    uint32_t dev_id, event;
> +    uint8_t vcol_id;
> +
> +    vcol_id = virt_cmd->movi.col;
> +    event = virt_cmd->movi.event;
> +    dev_id = virt_cmd->movi.devid;
> +
> +    DPRINTK("%pv vITS: MOVI: dev_id 0x%x vcol_id %d event %d\n",
> +            v, dev_id, vcol_id, event);
> +
> +    if ( vcol_id > (d->max_vcpus + 1)  || event > its_get_nr_events() )
> +        return -EINVAL;

My comment on the check in the previous function is valid here too.

> +
> +    if ( vits_get_vitt_entry(d, dev_id, event, &entry) )
> +        return -EINVAL;
> +
> +    entry.vcollection = vcol_id;
> +
> +    if ( vits_set_vitt_entry(d, dev_id, event, &entry) )
> +        return -EINVAL;
> +
> +    return 0;
> +}
> +
> +static int vgic_its_process_movall(struct vcpu *v, struct vgic_its *vits,
> +                                   its_cmd_block *virt_cmd)
> +{

/* Ignored */
DPRINTK("%pv vITS: MOVALL: ....",...);

> +    return 0;
> +}
> +
> +static int vgic_its_process_discard(struct vcpu *v, struct vgic_its *vits,
> +                                    its_cmd_block *virt_cmd)
> +{
> +    struct vitt entry;
> +    struct domain *d = v->domain;
> +    uint32_t event, dev_id;
> +
> +    event = virt_cmd->discard.event;
> +    dev_id = virt_cmd->discard.devid;
> +
> +    DPRINTK("%pv vITS: DISCARD: dev_id 0x%x id %d\n",
> +            v, virt_cmd->discard.devid, event);
> +
> +    if ( event > its_get_nr_events() )
> +        return -EINVAL;

Ditto for the check.

> +
> +    if ( vits_get_vitt_entry(d, dev_id, event, &entry) )
> +        return -EINVAL;
> +
> +    entry.valid = false;
> +
> +    if ( vits_set_vitt_entry(d, dev_id, event, &entry) )
> +        return -EINVAL;
> +
> +    return 0;
> +}
> +
> +static int vgic_its_process_inv(struct vcpu *v, struct vgic_its *vits,
> +                                its_cmd_block *virt_cmd)
> +{

Please add

/* Ignored */

> +    DPRINTK("%pv vITS: INV: dev_id 0x%x id %d\n",
> +            v, virt_cmd->inv.devid, virt_cmd->inv.event);
> +
> +    return 0;
> +}
> +
> +static int vgic_its_process_clear(struct vcpu *v, struct vgic_its *vits,
> +                                  its_cmd_block *virt_cmd)
> +{

/* Ignored */

> +    DPRINTK("%pv: vITS: CLEAR: dev_id 0x%x id %d\n",
> +             v, virt_cmd->clear.devid, virt_cmd->clear.event);
> +
> +    return 0;
> +}
> +
> +static int vgic_its_process_invall(struct vcpu *v, struct vgic_its *vits,
> +                                   its_cmd_block *virt_cmd)
> +{

/* Ignored */

> +    DPRINTK("%pv: vITS: INVALL: vCID %d\n", v, virt_cmd->invall.col);
> +
> +    return 0;
> +}
> +
> +static int vgic_its_process_int(struct vcpu *v, struct vgic_its *vits,
> +                                its_cmd_block *virt_cmd)
> +{
> +    struct vitt vitt_entry;
> +    struct domain *d = v->domain;
> +    uint32_t event, dev_id, col_id;
> +
> +    event = virt_cmd->int_cmd.cmd;
> +    dev_id = virt_cmd->int_cmd.devid;
> +
> +    DPRINTK("%pv: vITS: INT: Device 0x%x id %d\n", v, dev_id, event);
> +    if ( event > its_get_nr_events() )
> +        return -EINVAL;

Ditto for the check.

> +
> +    if ( vits_get_vitt_entry(d, dev_id, event, &vitt_entry) )
> +        return -EINVAL;
> +
> +    if ( !vitt_entry.valid )
> +    {
> +        dprintk(XENLOG_G_ERR,
> +                "%pv: vITS: INT CMD invalid event %d for dev 0x%x\n",
> +                v, event, dev_id);
> +        return -EINVAL;
> +    }
> +
> +    col_id = vitt_entry.vcollection;
> +    if ( col_id < d->max_vcpus )
> +    {
> +        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);

As said on v3, the design document [1] suggested to implement the INT 
command using vgic_vcpu_inject_lpi. Is there any issue to do it?

Also, you have to translate the col_id into to a VCPU ID.

> +
> +    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,
> +            virt_cmd->mapd.size);
> +
> +    if ( virt_cmd->mapd.valid )
> +    {
> +        /* itt field is 40 bit. extract 48 bit address by shifting */
> +        dt_entry.vitt_ipa = virt_cmd->mapd.itt << 8;

Could you introduce a define for the 8? It would be more clear than 
open-coding the value twice in the same function.

> +        dt_entry.vitt_size = (1 << (virt_cmd->mapd.size + 1)) *
> +                              sizeof(struct vitt);
> +    }
> +    else
> +    {
> +        dt_entry.vitt_ipa = INVALID_PADDR;
> +        dt_entry.vitt_size = 0;
> +    }
> +
> +    if ( vits_set_vdevice_entry(d, dev_id, &dt_entry) )
> +        return -EINVAL;
> +
> +    return 0;
> +}
> +
> +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;

The target address doesn't have to be valid when the collection is unmapped.

[...]

> +int vgic_its_process_cmd(struct vcpu *v, struct vgic_its *vits)

Missing static, the function will never be called outside of this file.

> +{
> +    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 );
> +
> +    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? */

/* ... */

It looks like to me we want to crash the guest using 
domain_crash_synchronous.

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

This shouldn't be part of gic.h but gic-its.h

Regards,

-- 
Julien Grall