Re: [Xen-devel] [RFC PATCH v2 14/22] xen/arm: its: Add emulation of ITS control registers

[Julien Grall <julien.grall@xxxxxxxxxx>]

Hello Vijay,

On 19/03/2015 14:38, vijay.kilari@xxxxxxxxx wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@xxxxxxxxxxxxxxxxxx>
>
> Add support for emulating GITS_* registers
>
> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@xxxxxxxxxxxxxxxxxx>
> ---
> v2: - Each Virtual ITS is attached to Physical ITS.
>      - Introduce helper function to lock and unlock
>        virtual ITS lock.
>      - Introduced helper to get virtual ITS structure pointer
>        based on emulation address.
> ---
>   xen/arch/arm/gic-v3-its.c     |    8 +
>   xen/arch/arm/vgic-v3-its.c    |  412 +++++++++++++++++++++++++++++++++++++++++
>   xen/include/asm-arm/gic-its.h |    1 +
>   3 files changed, 421 insertions(+)
>
> diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
> index a9aab73..e382f8d 100644
> --- a/xen/arch/arm/gic-v3-its.c
> +++ b/xen/arch/arm/gic-v3-its.c
> @@ -101,6 +101,8 @@ struct its_node {
>   };
>
>   uint32_t pta_type;
> +/* Number of physical its nodes present */
> +uint32_t nr_its = 0;
This variable is not exported so static.

Although, I'm not convinced this variable is useful. See my comments later.

>   #define ITS_ITT_ALIGN         SZ_256
>
> @@ -146,6 +148,11 @@ uint32_t its_get_pta_type(void)
>         return pta_type;
>   }
>
> +uint32_t its_get_nr_its(void)
> +{
> +       return nr_its;
> +}
> +
>   struct its_node * its_get_phys_node(uint32_t dev_id)
>   {
>         struct its_node *its;
> @@ -1170,6 +1177,7 @@ static int its_probe(struct dt_device_node *node)
>         }
>         spin_lock(&its_lock);
>         list_add(&its->entry, &its_nodes);
> +       nr_its++;
>         spin_unlock(&its_lock);
>
>         return 0;
> diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c
> index 7530a88..4d8945f 100644
> --- a/xen/arch/arm/vgic-v3-its.c
> +++ b/xen/arch/arm/vgic-v3-its.c
> @@ -869,6 +869,418 @@ err:
>       return 0;
>   }
>
> +struct vgic_its *its_to_vits(struct vcpu *v, paddr_t phys_base)
This function is not exported so static.

Also, it looks like to me that the vcpu is not necessary. Please use 
struct domain *d.
> +{
> +    struct vgic_its *vits = NULL;
> +    int i;
> +
> +    /* Mask 64K offset */
> +    phys_base = phys_base & ~(SZ_64K - 1);
> +    if ( is_hardware_domain(v->domain) )
Why do you need to have a specific case for the hardware domain? All the 
vITS code should be domain agnostic except the initialization function.
That would make the code a lot simpler.

> +    {
> +        for ( i = 0; i < its_get_nr_its(); i++ )
I would prefer if you introduce a new field in domain->arch to store the 
number of ITS for the domain.

> +        {
> +            if ( v->domain->arch.vits[i].phys_base == phys_base )
> +            {
> +                vits =  &v->domain->arch.vits[i];
> +                break;
> +            }
> +        }
> +    }
> +    else
> +        vits = &v->domain->arch.vits[0];
You should not assume that the guest as only one vITS.

> +
> +    return vits;
> +}
> +
> +static inline void vits_spin_lock(struct vgic_its *vits)
> +{
> +    spin_lock(&vits->lock);
> +}
> +
> +static inline void vits_spin_unlock(struct vgic_its *vits)
> +{
> +    spin_unlock(&vits->lock);
> +}
> +
> +static int vgic_v3_gits_mmio_read(struct vcpu *v, mmio_info_t *info)
> +{
> +    struct vgic_its *vits;
> +    struct hsr_dabt dabt = info->dabt;
> +    struct cpu_user_regs *regs = guest_cpu_user_regs();
> +    register_t *r = select_user_reg(regs, dabt.reg);
> +    uint64_t val = 0;
> +    uint32_t index, gits_reg;
> +
> +    vits = its_to_vits(v, info->gpa);
> +    if ( vits == NULL ) BUG_ON(1);
BUG_ON(vits != NULL);

Although I would document this BUG_ON to explain that its_to_vits should 
never fail because MMIOs registered always point to an ITS.
Though, an ASSERT maybe better here.

> +
> +    gits_reg = info->gpa - vits->phys_base;
> +
> +    if ( gits_reg >= SZ_64K )
> +    {
> +        gdprintk(XENLOG_G_WARNING, "vGITS: unknown gpa read address \
> +                  %"PRIpaddr"\n", info->gpa);
> +        return 0;
> +    }
This can never happen, you always register 64K range.

> +
> +    switch ( gits_reg )
> +    {
> +    case GITS_CTLR:
> +        if ( dabt.size != DABT_WORD ) goto bad_width;
Missing implementation for GITS_CTLR

> +        return 1;
> +    case GITS_IIDR:
> +        if ( dabt.size != DABT_WORD ) goto bad_width;
Missing implementation for GITS_IIDR

> +        return 1;
> +    case GITS_TYPER:
> +         /* GITS_TYPER support word read */
> +        vits_spin_lock(vits);
> +        val = ((its_get_pta_type() << VITS_GITS_TYPER_PTA_SHIFT) |
As said on a previous patch, each ITS may have a different value in PTA. 
I think it would make the command emulation simpler if we use an 
hardcoded PTA (PTA = 0 i.e using linear processor numbers seems the 
simpler).
> +               VITS_GITS_TYPER_HCC   | VITS_GITS_DEV_BITS |
I will comment the value here..

Where does the value of HCC and DEV_BITS come from? Both of them looks 
wrong to me.
> +               VITS_GITS_ID_BITS     | VITS_GITS_ITT_SIZE |
Ditto for ID_BITS and ITT_SIZE.

Although, it looks like that ITT_SIZE should be the same as the 
hardware. This is because you let the guest allocation the ITT.
I would also rename ITT_SIZE to ITT_ENTRY_SIZE.

> +               VITS_GITS_DISTRIBUTED | VITS_GITS_PLPIS);
The bit 3 is marked as implementation defined. So why did you name it 
DISTRIBUTED?
> +        if ( dabt.size == DABT_DOUBLE_WORD )
> +            *r = val;
> +        else if ( dabt.size == DABT_WORD )
> +            *r = (u32)(val >> 32);
> +        else
> +        {
> +            vits_spin_unlock(vits);
> +            goto bad_width;
> +        }
> +        vits_spin_unlock(vits);
The vits_spin_unlock could be done before setting *r.

> +        return 1;
> +    case GITS_TYPER + 4:
I don't like the idea to duplicate the code for GITS_TYPER just for 
reading the top word. Isn't possible to merge the 2 switch case?
Reading the spec again, it's not mandatory support support 32-bit access 
on 64-bit registers.
Given that we don't support GICv3 for 32-bit guest, I would completely 
drop the 32-bit access on 64-bit guest.
> +        if (dabt.size != DABT_WORD ) goto bad_width;
if ( ...

> +        vits_spin_lock(vits);
> +        val = ((its_get_pta_type() << VITS_GITS_TYPER_PTA_SHIFT) |
> +               VITS_GITS_TYPER_HCC   | VITS_GITS_DEV_BITS |
> +               VITS_GITS_ID_BITS     | VITS_GITS_ITT_SIZE |
> +               VITS_GITS_DISTRIBUTED | VITS_GITS_PLPIS);
> +        *r = (u32)val;
> +        vits_spin_unlock(vits);
> +        return 1;
> +    case 0x0010 ... 0x007c:
> +    case 0xc000 ... 0xffcc:
> +        /* Implementation defined -- read ignored */
> +        dprintk(XENLOG_ERR,
> +                "vGITS: read unknown 0x000c - 0x007c r%d offset %#08x\n",
> +                dabt.reg, gits_reg);
Please don't use XENLOG_ERR in guest code. Also, this printk is not 
useful and has been dropped in other emulation.
> +        goto read_as_zero;
> +    case GITS_CBASER:
> +        vits_spin_lock(vits);
> +        if ( dabt.size == DABT_DOUBLE_WORD )
> +            *r = vits->cmd_base && 0xc7ffffffffffffffUL;
Why the && and what does mean this constant?

> +        else if ( dabt.size == DABT_WORD )
> +            *r = (u32)vits->cmd_base;
> +        else
> +        {
> +            vits_spin_unlock(vits);
> +            goto bad_width;
> +        }
> +        vits_spin_unlock(vits);
> +        return 1;
> +    case GITS_CBASER + 4:
> +         /* CBASER support word read */
> +        if (dabt.size != DABT_WORD ) goto bad_width;
if ( ...

> +        vits_spin_lock(vits);
> +        *r = (u32)(vits->cmd_base >> 32);
> +        vits_spin_unlock(vits);
> +        return 1;
Same remark as GITS_TYPER for the word read support.

> +    case GITS_CWRITER:
> +        vits_spin_lock(vits);
> +        if ( dabt.size == DABT_DOUBLE_WORD )
> +            *r = vits->cmd_write;
> +        else if ( dabt.size == DABT_WORD )
> +            *r = (u32)vits->cmd_write;
> +        else
> +        {
> +            vits_spin_unlock(vits);
> +            goto bad_width;
> +        }
> +        vits_spin_unlock(vits);
> +        return 1;
> +    case GITS_CWRITER + 4:
> +         /* CWRITER support word read */
> +        if ( dabt.size != DABT_WORD ) goto bad_width;
> +        vits_spin_lock(vits);
> +        *r = (u32)(vits->cmd_write >> 32);
> +        vits_spin_unlock(vits);
> +        return 1;
Ditt for the word-read

> +    case GITS_CREADR:
> +        vits_spin_lock(vits);
> +        if ( dabt.size == DABT_DOUBLE_WORD )
> +            *r = vits->cmd_read;
> +        else if ( dabt.size == DABT_WORD )
> +            *r = (u32)vits->cmd_read;
> +        else
> +        {
> +            vits_spin_unlock(vits);
> +            goto bad_width;
> +        }
> +        vits_spin_unlock(vits);
> +        return 1;
> +    case GITS_CREADR + 4:
> +         /* CREADR support word read */
> +        if ( dabt.size != DABT_WORD ) goto bad_width;
> +        vits_spin_lock(vits);
> +        *r = (u32)(vits->cmd_read >> 32);
> +        vits_spin_unlock(vits);
> +        return 1;
Ditto

> +    case 0x0098 ... 0x009c:
> +    case 0x00a0 ... 0x00fc:
> +    case 0x0140 ... 0xbffc:
> +        /* Reserved -- read ignored */
> +        dprintk(XENLOG_ERR,
> +                "vGITS: read unknown 0x0098-9c or 0x00a0-fc r%d offset 
> %#08x\n",
> +                dabt.reg, gits_reg);
No need of printk here.

> +        goto read_as_zero;
> +    case GITS_BASER ... GITS_BASERN:
The spec says that registers are RES0 if not implemented.
As you use at all baser outside the register emulation, I would 
implement them RAZ/WI.
That would avoid a wrong write implementation.

> +        vits_spin_lock(vits);
> +        index = (gits_reg - GITS_BASER) / 8;
> +        if ( dabt.size == DABT_DOUBLE_WORD )
> +            *r = vits->baser[index];
> +        else if ( dabt.size == DABT_WORD )
> +        {
> +            if ( (gits_reg % 8) == 0 )
> +                *r = (u32)vits->baser[index];
> +            else
> +                *r = (u32)(vits->baser[index] >> 32);
> +        }
> +        else
> +        {
> +            vits_spin_unlock(vits);
> +            goto bad_width;
> +        }
> +        vits_spin_unlock(vits);
> +        return 1;
> +    case GITS_PIDR0:
> +        if ( dabt.size != DABT_WORD ) goto bad_width;
> +        *r = GITS_PIDR0_VAL;
> +        return 1;
> +    case GITS_PIDR1:
> +        if ( dabt.size != DABT_WORD ) goto bad_width;
> +        *r = GITS_PIDR1_VAL;
> +        return 1;
> +    case GITS_PIDR2:
> +        if ( dabt.size != DABT_WORD ) goto bad_width;
> +        *r = GITS_PIDR2_VAL;
> +        return 1;
> +    case GITS_PIDR3:
> +        if ( dabt.size != DABT_WORD ) goto bad_width;
> +        *r = GITS_PIDR3_VAL;
> +        return 1;
> +    case GITS_PIDR4:
> +        if ( dabt.size != DABT_WORD ) goto bad_width;
> +        *r = GITS_PIDR4_VAL;
> +        return 1;
> +    case GITS_PIDR5 ... GITS_PIDR7:
> +        goto read_as_zero;
Please check that we access is done via a word-access by introducing a 
new label read_as_zero_32 (for instance see the vgic v2 emulation).
> +   default:
> +        dprintk(XENLOG_ERR, "vGITS: unhandled read r%d offset %#08x\n",
> +               dabt.reg, gits_reg);
printk(XENLOG_G_ERR "%pv: ....", v,...)

Also it may be useful to printk which vITS is in use.

> +        return 0;
> +    }
> +
> +bad_width:
> +    dprintk(XENLOG_ERR, "vGITS: bad read width %d r%d offset %#08x\n",
> +           dabt.size, dabt.reg, gits_reg);
printk(XENLOG_G_ERR "%pv: ...", v,...)

Same remark for printing the vITS.

> +    domain_crash_synchronous();
> +    return 0;
> +
> +read_as_zero:
> +    if ( dabt.size != DABT_WORD ) goto bad_width;
How do you know that all RAZ access 32-bit access? See implementation 
defined registers for instance.
I would prefer to introduce multiple label:

read_as_zero_32: /* RAZ 32-bit */
    if ( dabt.size != DABT_WORD ) goto bad_width;

read_as_zero: /* Not check necessary */
    *r = 0;

And use the correctly label for goto in the emulation. So the code would 
be self-documented too.
> +    *r = 0;
> +    return 1;
> +}
> +
> +static int vgic_v3_gits_mmio_write(struct vcpu *v, mmio_info_t *info)
> +{
> +    struct vgic_its *vits;
> +    struct hsr_dabt dabt = info->dabt;
> +    struct cpu_user_regs *regs = guest_cpu_user_regs();
> +    register_t *r = select_user_reg(regs, dabt.reg);
> +    int ret;
> +    uint32_t index, gits_reg;
> +    uint64_t val;
> +
> +    vits = its_to_vits(v, info->gpa);
> +    if ( vits == NULL ) BUG_ON(1);
Same remark as the BUG_ON in vgic_v3_gits_mmio_read.

I'm wondering if it would be better to extend the read/write handler to 
get an opaque pointer in parameter.
In this case, it would contain the vits and would avoid the its_to_vits 
every time.
> +    gits_reg = info->gpa - vits->phys_base;
> +
> +    if ( gits_reg >= SZ_64K )
> +    {
> +        gdprintk(XENLOG_G_WARNING, "vGIC-ITS: unknown gpa write address"
> +                 " %"PRIpaddr"\n", info->gpa);
> +        return 0;
> +    }
This check is not necessary.

> +    switch ( gits_reg )
> +    {
> +    case GITS_CTLR:
> +        if ( dabt.size != DABT_WORD ) goto bad_width;
> +        vits_spin_lock(vits);
> +        vits->ctrl = *r;
Only bit[0] (Enabled) is writable.

> +        vits_spin_unlock(vits);
> +        return 1;
> +    case GITS_IIDR:
> +        /* R0 -- write ignored */
> +        goto write_ignore;
goto write_ignore_32;

> +    case GITS_TYPER:
> +    case GITS_TYPER + 4:
> +        /* R0 -- write ignored */
> +        goto write_ignore;
Please explicitly check the access size. That would avoid to crash the 
guest when TYPER is write with a 64-bit access.
> +    case 0x0010 ... 0x007c:
> +    case 0xc000 ... 0xffcc:
> +        /* Implementation defined -- write ignored */
> +        dprintk(XENLOG_ERR,
> +                "vGITS: write to unknown 0x000c - 0x007c r%d offset %#08x\n",
> +                dabt.reg, gits_reg);
Please drop the dprintk.

> +        goto write_ignore;
> +    case GITS_CBASER:
> +        if ( dabt.size == DABT_BYTE ) goto bad_width;
Please do the check in the invert way. i.e
(dabt.size != DABT_WORD) && (dabt.size != DABT_DOUBLE_WORD)

Also GITS_CBASER is read-only when GITS_CTLR.Enable is Zero or 
GITS_CTLR.Quiescent is zero.
> +        vits_spin_lock(vits);
> +        if ( dabt.size == DABT_DOUBLE_WORD )
> +            vits->cmd_base = *r;
> +        else
> +        {
> +            val = vits->cmd_base & 0xffffffff00000000UL;
The mask is difficult to read. And not all the bits are writeable.

> +            val = (*r) | val;
> +            vits->cmd_base =  val;
> +        }
> +        vits->cmd_qsize  =  SZ_4K * ((*r & 0xff) + 1);
Please use a define for the mask.
Also I would use cmd_qsize to know if the valid is set or not. I.e 
cmd_qsize = 0 => command queue not valid.
You forgot to update GITS_CREADR (i.e setting to 0) when GITS_CBASER is 
successfully written.
> +        vits_spin_unlock(vits);
> +        return 1;
> +    case GITS_CBASER + 4:
32-bit support is not necessary and make the code more complex for nothing.

> +         /* CBASER support word read */
> +        if (dabt.size != DABT_WORD ) goto bad_width;
> +        vits_spin_lock(vits);
> +        val = vits->cmd_base & 0xffffffffUL;
> +        val = ((*r & 0xffffffffUL) << 32 ) | val;
> +        vits->cmd_base =  val;
> +        /* No Need to update cmd_qsize with higher word write */
> +        vits_spin_unlock(vits);
> +        return 1;
> +    case GITS_CWRITER:
> +        if ( dabt.size == DABT_BYTE ) goto bad_width;
> +        vits_spin_lock(vits);
> +        if ( dabt.size == DABT_DOUBLE_WORD )
> +            vits->cmd_write = *r;
Only Bits[19:5] are writable.

> +        else
> +        {
> +            val = vits->cmd_write & 0xffffffff00000000UL;
> +            val = (*r) | val;
> +            vits->cmd_write =  val;
> +        }
No validation of the value written by the guest? Given your 
implementation of the command processing, any invalid value will end up 
to an infinite loop in the hypervisor. Whoops :).
> +        ret = vgic_its_process_cmd(v, vits);
> +        vits_spin_unlock(vits);
> +        return ret;
> +    case GITS_CWRITER + 4:
Same remark as GITS_CBASER for the 32-bit support.

> +        if (dabt.size != DABT_WORD ) goto bad_width;
> +        vits_spin_lock(vits);
> +        val = vits->cmd_write & 0xffffffffUL;
> +        val = ((*r & 0xffffffffUL) << 32) | val;
> +        vits->cmd_write =  val;
> +        ret = vgic_its_process_cmd(v, vits);
> +        vits_spin_unlock(vits);
> +        return ret;
> +    case GITS_CREADR:
> +        /* R0 -- write ignored */
> +        goto write_ignore;
> +    case 0x0098 ... 0x009c:
> +    case 0x00a0 ... 0x00fc:
> +    case 0x0140 ... 0xbffc:
> +        /* Reserved -- write ignored */
> +        dprintk(XENLOG_ERR,
> +                "vGITS: write to unknown 0x98-9c or 0xa0-fc r%d offset 
> %#08x\n",
> +                dabt.reg, gits_reg);
Please drop the dprintk

> +        goto write_ignore;
> +    case GITS_BASER ... GITS_BASERN:
> +        /* Nothing to do with this values. Just store and emulate */
As you don't use those values at all, write ignore would be better.

> +        vits_spin_lock(vits);
> +        index = (gits_reg - GITS_BASER) / 8;
> +        if ( dabt.size == DABT_DOUBLE_WORD )
> +            vits->baser[index] = *r;
> +        else if ( dabt.size == DABT_WORD )
> +        {
> +            if ( (gits_reg % 8) == 0 )
> +            {
> +                val = vits->cmd_write & 0xffffffff00000000UL;
cmd_write seems to come out of nowhere...

> +                val = (*r) | val;
> +                vits->baser[index] = val;
> +            }
> +            else
> +            {
> +                val = vits->baser[index] & 0xffffffffUL;
> +                val = ((*r & 0xffffffffUL) << 32) | val;
> +                vits->baser[index] = val;
> +            }
> +        }
> +        else
> +        {
> +            goto bad_width;
> +            vits_spin_unlock(vits);
> +        }
> +        vits_spin_unlock(vits);
> +        return 1;
> +    case GITS_PIDR7 ... GITS_PIDR0:
> +        /* R0 -- write ignored */
> +        goto write_ignore;
> +   default:
> +        dprintk(XENLOG_ERR, "vGITS: unhandled write r%d offset %#08x\n",
> +                dabt.reg, gits_reg);
printk(XENLOG_G_ERR "%pv: .....", v, ....);

+ Print which ITS is in use.

> +        return 0;
> +    }
> +
> +bad_width:
> +    dprintk(XENLOG_ERR, "vGITS: bad write width %d r%d offset %#08x\n",
> +           dabt.size, dabt.reg, gits_reg);
Ditto

> +    domain_crash_synchronous();
> +    return 0;
> +
> +write_ignore:
> +    if ( dabt.size != DABT_WORD ) goto bad_width;
> +    *r = 0;
> +    return 1;
Same remark as read_ignore.

> +}
> +
> +static const struct mmio_handler_ops vgic_gits_mmio_handler = {
> +    .read_handler  = vgic_v3_gits_mmio_read,
> +    .write_handler = vgic_v3_gits_mmio_write,
> +};
> +
> +int vgic_its_domain_init(struct domain *d)
You forgot to add the prototype of this function in the header...

> +{
This code is not really part of the ITS registers emulation...

Your patchs series splitting is really confusing.

> +    uint32_t num_its;
> +    int i;
> +
> +    num_its =  its_get_nr_its();
> +
> +    d->arch.vits = xzalloc_array(struct vgic_its, num_its);
Hmm... why did you use the number of physical ITS rather than the number 
of vITS used by the guest.
It would avoid to waste so much memory for every domain.

> +    if ( d->arch.vits == NULL )
> +        return -ENOMEM;
> +
> +    spin_lock_init(&d->arch.vits->lock);
> +
> +    spin_lock_init(&d->arch.vits_devs.lock);
> +    INIT_LIST_HEAD(&d->arch.vits_devs.dev_list);
> +
> +    d->arch.lpi_conf = xzalloc(struct vgic_lpi_conf);
> +    if ( d->arch.lpi_conf == NULL )
> +         return -ENOMEM;
> +
> +    for ( i = 0; i < num_its; i++)
> +    {
> +         spin_lock_init(&d->arch.vits[i].lock);
> +         register_mmio_handler(d, &vgic_gits_mmio_handler,
> +                               d->arch.vits[i].phys_base,
> +                               SZ_64K);
> +    }
> +
> +    return 0;
> +}
> +
>   /*
>    * Local variables:
>    * mode: C
> diff --git a/xen/include/asm-arm/gic-its.h b/xen/include/asm-arm/gic-its.h
> index 70ec913..82cfbdc 100644
> --- a/xen/include/asm-arm/gic-its.h
> +++ b/xen/include/asm-arm/gic-its.h
> @@ -227,6 +227,7 @@ int gic_its_send_cmd(struct vcpu *v, struct its_node *its,
>   void its_lpi_free(unsigned long *bitmap, int base, int nr_ids);
>   unsigned long *its_lpi_alloc_chunks(int nirqs, int *base, int *nr_ids);
>   uint32_t its_get_pta_type(void);
> +uint32_t its_get_nr_its(void);
>   struct its_node * its_get_phys_node(uint32_t dev_id);
>   #endif /* __ASM_ARM_GIC_ITS_H__ */
Regards,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel