Re: [PATCH v4 09/17] xen/arm: ITS: Add GITS registers emulation

[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>
>
> Emulate GITS* registers
>
> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> ---
> v4: - Removed GICR register emulation
> ---
>   xen/arch/arm/gic-v3-its.c         |   11 ++
>   xen/arch/arm/vgic-v3-its.c        |  319 ++++++++++++++++++++++++++++++++++++-
>   xen/include/asm-arm/gic-its.h     |   10 ++
>   xen/include/asm-arm/gic_v3_defs.h |   11 ++
>   4 files changed, 349 insertions(+), 2 deletions(-)
>
> diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
> index 1d2fdde..5e6c7f2 100644
> --- a/xen/arch/arm/gic-v3-its.c
> +++ b/xen/arch/arm/gic-v3-its.c
> @@ -109,6 +109,16 @@ u32 its_get_nr_events(void)
>       return (1 << its_data.id_bits);
>   }
>
> +u32 its_get_id_bits(void)
> +{
> +    return its_data.id_bits;
> +}
> +
> +u32 its_get_dev_bits(void)
> +{
> +    return its_data.dev_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.

>   static struct its_node * its_get_phys_node(u32 dev_id)
>   {
>       /* TODO: For now return ITS0 node.
> @@ -1309,6 +1319,7 @@ static int its_probe(struct dt_device_node *node)
>       typer = readl_relaxed(its_base + GITS_TYPER);
>       its->ite_size = ((typer >> 4) & 0xf) + 1;
>       its_data.id_bits = GITS_TYPER_IDBITS(typer);
> +    its_data.dev_bits = GITS_TYPER_DEVBITS(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 af2bacd..abf60e2 100644
> --- a/xen/arch/arm/vgic-v3-its.c
> +++ b/xen/arch/arm/vgic-v3-its.c
> @@ -31,7 +31,9 @@
>   #include <asm/gic-its.h>
>   #include <xen/log2.h>
>
> -#define DEBUG_ITS
> +// #define DEBUG_ITS

This changes should be merged in the patch which add #define DEBUG_ITS 
(i.e #7).

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

As said on v3, this name is misleading, this is not the size of the ITT 
but the value to store in the GITS_TYPER register.

In any case, you should not hardcode the size of the entry and use 
sizeof(struct vitt) as you did well for the device and other things.

> +#define VITS_GITS_PLPIS           (0x1U)

Please introduce GITS_TYPER_PHYSICAL rather than hardcoding the value 
for the vITS.

>
>   #ifdef DEBUG_ITS
>   # define DPRINTK(fmt, args...) dprintk(XENLOG_DEBUG, fmt, ##args)
> @@ -568,7 +570,7 @@ static int vgic_its_read_virt_cmd(struct vcpu *v,
>       return 0;
>   }
>
> -int vgic_its_process_cmd(struct vcpu *v, struct vgic_its *vits)
> +static int vgic_its_process_cmd(struct vcpu *v, struct vgic_its *vits)

The static belongs to the patch where vgic_its_process_cmd has been 
added (i.e patch #7).

>   {
>       its_cmd_block virt_cmd;
>
> @@ -593,6 +595,319 @@ err:
>       return 0;
>   }
>
> +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 = v->domain->arch.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 gits_reg;
> +
> +    gits_reg = info->gpa - vits->gits_base;
> +
> +    switch ( gits_reg )
> +    {
> +    case GITS_CTLR:
> +        if ( dabt.size != DABT_WORD ) goto bad_width;
> +        vits_spin_lock(vits);
> +        *r = vits->ctrl & GITS_CTLR_ENABLE;

The & GITS_CTLR_ENABLE is not necessary. You already ensure in the write 
emulation that only this bit saved. So

*r = vits->ctlr;

> +        vits_spin_unlock(vits);
> +        return 1;

[..]

> +    case GITS_TYPER:
> +    case GITS_TYPER + 4:
> +        vits_spin_lock(vits);

Why the lock? AFAICT you don't touch the vITS structure.

Also, as said on v3, please add a comment explaining the configuration 
of this field. It would avoid to scratch his head trying to understand 
why PTA is not mentioned...

> +        val = (((v->domain->max_vcpus + 1) << GITS_TYPER_HCC_SHIFT ) |

I would prefer to see a macro computing the number of collection.

Although, HCC is only able to encore 16-bit of collection ID (i.e 256 
collection => 255 VCPUs). Please make a note and add a BUILD_ON_BUG or 
anything else to ensure that it will never happen within this register 
emulation (i.e GITS_TYPER).

> +                (its_get_dev_bits() - 1) << GITS_TYPER_DEVBITS_SHIFT |
> +                (its_get_id_bits() - 1) << GITS_TYPER_IDBITS_SHIFT   |

Please introduce fields in the vits structure to store the number of 
devID and eventID bits.

> +                 VITS_GITS_ITT_SIZE | VITS_GITS_PLPIS);
> +        if ( (gits_reg % 8) == 0 && dabt.size == DABT_DOUBLE_WORD )

Why the gits_reg % 8 ? AFAICT, 64-bit access should always be aligned 
when trapped in Xen. I did some test and wasn't able to proof the invert.

If it's not true we have to fix for every IO handler and not only this 
one as all the emulation assume a such things. So it has to be done 
generically.

> +            *r = val;
> +        else if ( dabt.size == DABT_WORD )
> +        {
> +            if ( (gits_reg % 8) == 0 )
> +                *r = (u32)(val >> 32);
> +            else
> +                *r = (u32)val;
> +        }

You duplicate this code in a lot of register emulation and this code is 
quite hard to read. This is a call to introduce an helper.

> +        else
> +        {
> +            vits_spin_unlock(vits);
> +            goto bad_width;
> +        }
> +        vits_spin_unlock(vits);
> +        return 1;
> +    case 0x0010 ... 0x007c:
> +    case 0xc000 ... 0xffcc:
> +        /* Implementation defined -- read ignored */
> +        goto read_as_zero;
> +    case GITS_CBASER:
> +    case GITS_CBASER + 4:
> +        /* Only read support 32/64-bit access */
> +        vits_spin_lock(vits);
> +        if ( (gits_reg % 8) == 0 && dabt.size == DABT_DOUBLE_WORD )
> +            *r = vits->cmd_base;
> +        else if ( dabt.size == DABT_WORD )
> +        {
> +            if ( (gits_reg % 8) == 0 )
> +                *r = (u32)vits->cmd_base;
> +            else
> +                *r = (u32)(vits->cmd_base >> 32);
> +        }
> +        else
> +        {
> +            vits_spin_unlock(vits);
> +            goto bad_width;
> +        }
> +        vits_spin_unlock(vits);

You could have avoid the double unlock by saving the vits->cmd_base in a 
temporary value i.e

vits_spin_lock(vits)
val = vits->cmd_base;
vits_spin_unlock(vits)

if ( .... )
...

> +        return 1;
> +    case GITS_CWRITER:
> +    case GITS_CWRITER + 4:
> +        /* Only read support 32/64-bit access */
> +        vits_spin_lock(vits);
> +        if ( (gits_reg % 8) == 0 && dabt.size == DABT_DOUBLE_WORD )
> +            *r = vits->cmd_write;
> +        else if ( dabt.size == DABT_WORD )
> +        {
> +            if ( (gits_reg % 8) == 0 )
> +                *r = (u32)vits->cmd_write;
> +            else
> +                *r = (u32)(vits->cmd_write >> 32);
> +        }
> +        else
> +        {
> +            vits_spin_unlock(vits);
> +            goto bad_width;

Ditto

> +        }
> +        vits_spin_unlock(vits);
> +        return 1;
> +    case GITS_CREADR:
> +    case GITS_CREADR + 4:
> +        /* Only read support 32/64-bit access */
> +        vits_spin_lock(vits);
> +        if ( (gits_reg % 8) == 0 && dabt.size == DABT_DOUBLE_WORD )
> +            *r = vits->cmd_read;
> +        else if ( dabt.size == DABT_WORD )
> +        {
> +            if ( (gits_reg % 8) == 0 )
> +                *r = (u32)vits->cmd_read;
> +            else
> +                *r = (u32)(vits->cmd_read >> 32);
> +        }
> +        else
> +        {
> +            vits_spin_unlock(vits);
> +            goto bad_width;
> +        }

ditto

> +        vits_spin_unlock(vits);
> +        return 1;
> +    case 0x0098 ... 0x009c:
> +    case 0x00a0 ... 0x00fc:
> +    case 0x0140 ... 0xbffc:
> +        /* Reserved -- read ignored */
> +        goto read_as_zero;
> +    case GITS_BASER:

Please use GITS_BASER0 rather than GITS_RATHER. It's more meaningful.

> +        /* XXX: Support only 32-bit access */

This comment is wrong. You only support 64-bit access.

If you add the helper suggested above to read either 32-bit or 64-bit, 
it would make the 32-bit support for GITS_BASER free.

> +        if ( dabt.size != DABT_DOUBLE_WORD ||
> +             (gits_reg % 8) != 0 )

The second part of the condition is not useful.

> +            goto bad_width;
> +        vits_spin_lock(vits);
> +        *r = vits->baser;
> +        vits_spin_unlock(vits);
> +        return 1;
> +    case GITS_BASER1 ... GITS_BASERN:
> +        if ( dabt.size != DABT_DOUBLE_WORD ) goto bad_width;
> +        *r = 0;
> +        return 1;

The *r = 0 ... return 1 could be replaced by goto read_as_zero;

[...]

> +/*
> + * GITS_BASER.Type[58:56],GITS_BASER.Entry_size[55:48]
> + * and GITS_BASER.Shareability[11:10] are read-only.

Implemented Shareability as fixed (i.e read-only) is deprecated. I don't 
mind if we do it for now. Although I'd like to see a comment about it here.

> + * Mask those fields while emulating GITS_BASER reg.
> + */

Other fields are (or could be RO) in GITS_BASER:
	- Indirect: we only support flat table
	- Page_Size: it's fine to only support 4KB granularity. It also means 
less code.

I don't mind if you don't do the later.

> +#define GITS_BASER_MASK  (~((0x7UL << GITS_BASER_TYPE_SHIFT)     | \
> +                         (0xffUL << GITS_BASER_ENTRY_SIZE_SHIFT) | \
> +                         (0x3UL << GITS_BASER_SHAREABILITY_SHIFT)))
> +
> +static int vgic_v3_gits_mmio_write(struct vcpu *v, mmio_info_t *info)
> +{
> +    struct vgic_its *vits = v->domain->arch.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 gits_reg, sz, psz;
> +
> +    gits_reg = info->gpa - vits->gits_base;
> +
> +    switch ( gits_reg )
> +    {
> +    case GITS_CTLR:
> +        if ( dabt.size != DABT_WORD )
> +            goto bad_width;
> +        vits_spin_lock(vits);
> +        vits->ctrl = *r & GITS_CTLR_ENABLE;
> +        vits_spin_unlock(vits);
> +        return 1;
> +    case GITS_IIDR:
> +        /* R0 -- write ignored */

The field is read-only not read-as-zero. Please use RO rather than R0.

> +        goto write_ignore;
> +    case GITS_TYPER:
> +    case GITS_TYPER + 4:
> +        /* R0 -- write ignored */

s/R0/RO/

> +        goto write_ignore;
> +    case 0x0010 ... 0x007c:
> +    case 0xc000 ... 0xffcc:
> +        /* Implementation defined -- write ignored */
> +        goto write_ignore;
> +    case GITS_CBASER:

GITS_CBASER is read-only when GITS_CTLR.Enable is Zero or 
GITS_CTLR.Quiescent is zero.

Please handle this case.

> +        /* XXX: support 32-bit access */
> +        if ( dabt.size != DABT_DOUBLE_WORD )
> +            goto bad_width;
> +        vits_spin_lock(vits);
> +        vits->cmd_base = *r;
> +        vits->cmd_qsize  =  SZ_4K * ((*r & GITS_BASER_PAGES_MASK_VAL) + 1);
> +        if ( vits->cmd_qsize & GITS_BASER_VALID )
> +            vits->cmd_read = 0;
> +        vits_spin_unlock(vits);
> +        return 1;
> +    case GITS_CBASER + 4:
> +         /* XXX: Does not support word write */
> +        goto bad_width;

You can drop this case, it's not necessary.

> +    case GITS_CWRITER:
> +        if ( dabt.size == DABT_BYTE ) goto bad_width;
> +        /* XXX: Validate val */

It's not so hard to check the validity of the offset... Please do it.

> +        vits_spin_lock(vits);
> +        vits->cmd_write = *r & 0xfffe0;

Can you add a comment explaining why the 0xfffe0? i.e the only bit 
[19:5] are writeable.

> +        if ( !(vits->ctrl & GITS_CTLR_ENABLE) )
> +            return 1;
> +        ret = vgic_its_process_cmd(v, vits);
> +        vits_spin_unlock(vits);
> +        return ret;
> +    case GITS_CWRITER + 4:
> +        if (dabt.size != DABT_WORD ) goto bad_width;

I had to go through the previous review in order to understand why you 
do that. Please add a comment to say that the top half of the register 
is read-only.

> +        return 1;
> +    case GITS_CREADR:
> +        /* R0 -- write ignored */

s/R0/RO/

> +        goto write_ignore;
> +    case 0x0098 ... 0x009c:
> +    case 0x00a0 ... 0x00fc:
> +    case 0x0140 ... 0xbffc:
> +        /* Reserved -- write ignored */
> +        goto write_ignore;
> +    case GITS_BASER0:

GITS_CBASER is read-only when GITS_CTLR.Enable is Zero or 
GITS_CTLR.Quiescent is zero.

It's quite important to do it as, AFAICT, you don't have any protection 
on the usage of dt_ipa in vits_get_vdevice_entry.

Although this won't protect everything as it may be possible to receive 
an LPI before the GITS is effectively setup (see patch #8) or because 
the guest decided to clear GITS_CRTL.Enable.

> +        /* XXX: Support 32-bit access */
> +        if ( dabt.size != DABT_DOUBLE_WORD )
> +            goto bad_width;
> +        vits_spin_lock(vits);
> +        vits->baser = vits->baser | (*r & GITS_BASER_MASK);

As said on v3, this doesn't do what you want. It's an or operation so if 
bit is 0 in the *r it won't be clear in the resulting vits->baser.

The correct solution would be:
    vits->baser &= ~GITS_BASER_MASK;
    vits->baser |= *r | GITS_BASER_MASK;

> +        vits->dt_ipa = vits->baser & BIT_48_12_MASK;
> +        psz = (vits->baser >> GITS_BASER_PAGE_SIZE_SHIFT) &
> +               GITS_BASER_PAGE_SIZE_MASK_VAL;
> +        if ( psz == GITS_BASER_PAGE_SIZE_4K_VAL )
> +            sz = 4;
> +        else if ( psz == GITS_BASER_PAGE_SIZE_16K_VAL )
> +            sz = 16;
> +        else

Please add a comment to explain that 0x11 is treated as 0x10 i.e 64KB.

> +            sz = 64;
> +
> +        vits->dt_size = (vits->baser & GITS_BASER_PAGES_MASK_VAL)
> +                        * sz * SZ_1K;
> +        vits_spin_unlock(vits);
> +        return 1;
> +    case GITS_BASER1 ... GITS_BASERN:

Please handle the 32-bit access here. I.e smth like

if ( !DOUBLE_WORD || !WORD )
   goto bad_write

return 1;

> +        goto write_ignore_64;
> +    case GITS_PIDR7 ... GITS_PIDR0:
> +        /* R0 -- write ignored */
> +        goto write_ignore_32;
> +   default:
> +        dprintk(XENLOG_G_ERR, "%pv vITS: unhandled write r%d offset %#08x\n",
> +                v, dabt.reg, gits_reg);
> +        return 0;
> +    }
> +
> +bad_width:
> +    dprintk(XENLOG_G_ERR, "%pv: vITS: bad write width %d r%d offset %#08x\n",
> +            v, dabt.size, dabt.reg, gits_reg);
> +    domain_crash_synchronous();
> +    return 0;
> +
> +write_ignore_64:
> +    if ( dabt.size != DABT_DOUBLE_WORD ) goto bad_width;
> +    return 1;
> +write_ignore_32:
> +    if ( dabt.size != DABT_WORD ) goto bad_width;
> +    return 1;
> +write_ignore:
> +    *r = 0;
> +    return 1;
> +}
> +
> +
> +static const struct mmio_handler_ops vgic_gits_mmio_handler = {
> +    .read_handler  = vgic_v3_gits_mmio_read,
> +    .write_handler = vgic_v3_gits_mmio_write,
> +};
> +
>   /*
>    * Local variables:
>    * mode: C
> diff --git a/xen/include/asm-arm/gic-its.h b/xen/include/asm-arm/gic-its.h
> index f041efc..9c004c2 100644
> --- a/xen/include/asm-arm/gic-its.h
> +++ b/xen/include/asm-arm/gic-its.h
> @@ -37,6 +37,8 @@ struct its_collection {
>   struct vgic_its
>   {
>      spinlock_t lock;
> +   /* Emulation of BASER0 */
> +   paddr_t baser;

Please rename it to baser0.

>      /* Command queue base */
>      paddr_t cmd_base;
>      /* Command queue write pointer */
> @@ -47,6 +49,12 @@ struct vgic_its
>      paddr_t cmd_read;
>      /* Command queue size */
>      unsigned long cmd_qsize;
> +   /* ITS mmio physical base */
> +   paddr_t gits_base;
> +   /* ITS mmio physical size */
> +   unsigned long gits_size;

I don't see any usage of gits_size.

> +   /* GICR ctrl register */
> +   uint32_t ctrl;
>      /* vITT device table ipa */
>      paddr_t dt_ipa;
>      /* vITT device table size */
> @@ -237,6 +245,8 @@ struct gic_its_info {
>       uint32_t dev_bits;
>   };

[..]

> diff --git a/xen/include/asm-arm/gic_v3_defs.h b/xen/include/asm-arm/gic_v3_defs.h
> index 0443ae7..84366df 100644
> --- a/xen/include/asm-arm/gic_v3_defs.h
> +++ b/xen/include/asm-arm/gic_v3_defs.h
> @@ -202,6 +202,7 @@
>   #define GITS_TYPER_IDBITS(r)		((((r) >> GITS_TYPER_IDBITS_SHIFT) & 0x1f) + 1)
>   #define GITS_TYPER_PTA                  (1UL << 19)
>   #define GITS_TYPER_HCC_SHIFT            (24)
> +#define GITS_TYPER_ITT_SIZE_SHIFT       (4)
>
>   #define GITS_CBASER_VALID               (1UL << 63)
>   #define GITS_CBASER_nC                  (1UL << 59)
> @@ -228,6 +229,10 @@
>   #define GITS_BASER_PAGE_SIZE_16K        (1UL << GITS_BASER_PAGE_SIZE_SHIFT)
>   #define GITS_BASER_PAGE_SIZE_64K        (2UL << GITS_BASER_PAGE_SIZE_SHIFT)
>   #define GITS_BASER_PAGE_SIZE_MASK       (3UL << GITS_BASER_PAGE_SIZE_SHIFT)
> +#define GITS_BASER_PAGE_SIZE_4K_VAL     (0)
> +#define GITS_BASER_PAGE_SIZE_16K_VAL    (1)
> +#define GITS_BASER_PAGE_SIZE_MASK_VAL   (0x3)
> +#define GITS_BASER_PAGES_MASK_VAL       (0xff)
>   #define GITS_BASER_TYPE_NONE            0
>   #define GITS_BASER_TYPE_DEVICE          1
>   #define GITS_BASER_TYPE_VCPU            2
> @@ -236,6 +241,12 @@
>   #define GITS_BASER_TYPE_RESERVED5       5
>   #define GITS_BASER_TYPE_RESERVED6       6
>   #define GITS_BASER_TYPE_RESERVED7       7
> +/* 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)

Those new defines are part of the vITS implementation and should not be 
exposed ouside of vgic-v3-its.c.

FWIW, this is what we do for GICv3 and GICv2.

Regards,

-- 
Julien Grall