Re: [PATCH v5 10/22] xen/arm: ITS: Add GITS registers emulation

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

Hi Vijay,

On 27/07/15 12:11, 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/irq.c            |    3 +
>  xen/arch/arm/vgic-v3-its.c    |  365 ++++++++++++++++++++++++++++++++++++++++-
>  xen/include/asm-arm/gic-its.h |   15 ++
>  xen/include/asm-arm/gic.h     |    1 +
>  4 files changed, 381 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
> index 1f38605..85cacb0 100644
> --- a/xen/arch/arm/irq.c
> +++ b/xen/arch/arm/irq.c
> @@ -31,6 +31,9 @@
>  static unsigned int local_irqs_type[NR_LOCAL_IRQS];
>  static DEFINE_SPINLOCK(local_irqs_type_lock);
>  
> +/* Number of LPI supported in XEN */
> +unsigned int num_of_lpis = 8192;
> +

It makes little sense to introduce the support of LPIs in Xen in a patch
called "Add GITS registers emulation".

This should go in a specific ITS (not vITS) patch.

Furthermore, you need to explain where to the 8192 comes from...

Lastly I would rename num_of_lpis into nr_lpis.

>  /* Describe an IRQ assigned to a guest */
>  struct irq_guest
>  {
> diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c
> index 3a003d4..1c7d9b6 100644
> --- a/xen/arch/arm/vgic-v3-its.c
> +++ b/xen/arch/arm/vgic-v3-its.c
> @@ -33,8 +33,16 @@
>  #include <asm/atomic.h>
>  #include <xen/log2.h>
>  
> -#define DEBUG_ITS
> -
> +//#define DEBUG_ITS
> +

This change should go in patch #8.

> +/* 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)
> +#define GITS_BASER_INIT_VAL          ((1UL << GITS_BASER_TYPE_SHIFT) | \
> +                                     (0x7UL << GITS_BASER_ENTRY_SIZE_SHIFT))
>  #ifdef DEBUG_ITS
>  # define DPRINTK(fmt, args...) dprintk(XENLOG_DEBUG, fmt, ##args)
>  #else
> @@ -60,6 +68,14 @@ void vits_setup_hw(struct gic_its_info *its_info)
>      vits_hw.info = its_info;
>  }
>  
> +static inline uint32_t vits_get_max_collections(struct domain *d)
> +{
> +    /* Collection ID is only 16 bit */

16 bit = 65536 not 256.

You need to explain that the ITS is only supporting 256 collections in
hardware and that our implementation doesn't support memory provisioning
for collection.

Furthermore if the number of collection is based on 16 bits, the
function should return uint16_t not uint32_t.


> +    ASSERT(d->max_vcpus < 256);
> +

Please add a comment to explain why d->max_vcpus + 1 with may a
reference to the public spec.

> +    return (d->max_vcpus + 1);
> +}
> +
>  static int vits_access_guest_table(struct domain *d, paddr_t entry, void *addr,
>                                     uint32_t size, bool_t set)
>  {
> @@ -502,7 +518,7 @@ static int vits_read_virt_cmd(struct vcpu *v, struct vgic_its *vits,
>      return 0;
>  }
>  
> -int vits_process_cmd(struct vcpu *v, struct vgic_its *vits)
> +static int vits_process_cmd(struct vcpu *v, struct vgic_its *vits)

Please, Move the static where the function has been defined.

>  {
>      its_cmd_block virt_cmd;
>  
> @@ -527,11 +543,338 @@ err:
>      return 0;
>  }

[..]

> +static int vgic_v3_gits_mmio_read(struct vcpu *v, mmio_info_t *info)
> +{
> +    struct vgic_its *vits = v->domain->arch.vgic.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;
> +    DPRINTK("%pv: vITS: GITS_MMIO_READ offset 0x%"PRIx32"\n", v, gits_reg);
> +
> +    switch ( gits_reg )
> +    {
> +    case GITS_CTLR:
> +        if ( dabt.size != DABT_WORD ) goto bad_width;
> +        vits_spin_lock(vits);
> +        *r = vits->ctrl | GITS_CTLR_QUIESCENT;

Why did you put GITS_CTLR_QUIESCENT?

> +        vits_spin_unlock(vits);
> +        return 1;
> +    case GITS_IIDR:
> +        if ( dabt.size != DABT_WORD ) goto bad_width;
> +        *r = GICV3_GICD_IIDR_VAL;
> +        return 1;
> +    case GITS_TYPER:
> +    case GITS_TYPER + 4:
> +        /*
> +         * GITS_TYPER.HCC = max_vcpus + 1 (max collection supported )
> +         * GITS_TYPER.Devbits = HW supported Devbits size
> +         * GITS_TYPER.IDbits = HW supported IDbits size
> +         * GITS_TYPER.PTA = 0 ( Target addresses are linear processor numbers

Missing )

> +         * GITS_TYPER.ITTSize = Size of struct vitt
> +         * GITS_TYPER.Physical = 1
> +         */
> +        if ( dabt.size != DABT_DOUBLE_WORD &&
> +             dabt.size != DABT_WORD ) goto bad_width;
> +        val = ((vits_get_max_collections(v->domain) << GITS_TYPER_HCC_SHIFT ) |
> +               ((vits_hw.info->dev_bits - 1) << GITS_TYPER_DEVBITS_SHIFT)     |
> +               ((vits_hw.info->eventid_bits - 1) << GITS_TYPER_IDBITS_SHIFT)  |
> +               ((sizeof(struct vitt) - 1) << GITS_TYPER_ITT_SIZE_SHIFT)       |
> +                 GITS_TYPER_PHYSICAL_LPIS);
> +        if ( dabt.size == DABT_DOUBLE_WORD )
> +            *r = val;
> +        else
> +            *r = vits_get_word(gits_reg, val);
> +        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 */

"Read supports only 32/64-bit access"

> +        if ( dabt.size != DABT_DOUBLE_WORD &&
> +             dabt.size != DABT_WORD ) goto bad_width;
> +        vits_spin_lock(vits);
> +        if ( dabt.size == DABT_DOUBLE_WORD )
> +            *r = vits->cmd_base;
> +        else
> +            *r = vits_get_word(gits_reg, vits->cmd_base);
> +        vits_spin_unlock(vits);
> +        return 1;
> +    case GITS_CWRITER:
> +        /* Only read support 32/64-bit access */

Ditto

> +        vits_spin_lock(vits);
> +        val = vits->cmd_write;
> +        vits_spin_unlock(vits);
> +        if ( dabt.size == DABT_DOUBLE_WORD )
> +            *r = val;
> +        else if (dabt.size == DABT_WORD )
> +            *r = (u32)val;
> +        else
> +            goto bad_width;
> +        return 1;
> +    case GITS_CWRITER + 4:
> +        /* BITS[63:20] are RES0 */
> +        goto read_as_zero_32;
> +    case GITS_CREADR:
> +        /* Only read support 32/64-bit access */

Ditto

> +        val = atomic_read(&vits->cmd_read);
> +        if ( dabt.size == DABT_DOUBLE_WORD )
> +            *r = val;
> +        else if (dabt.size == DABT_WORD )
> +            *r = (u32)val;
> +        else
> +            goto bad_width;
> +        return 1;
> +    case GITS_CREADR + 4:
> +        /* BITS[63:20] are RES0 */
> +        goto read_as_zero_32;
> +    case 0x0098 ... 0x009c:
> +    case 0x00a0 ... 0x00fc:
> +    case 0x0140 ... 0xbffc:
> +        /* Reserved -- read ignored */
> +        goto read_as_zero;
> +    case GITS_BASER0:
> +	    /* XXX: Support only 32-bit access */

As said on v4, this comment is wrong. You only support 64-bit.

> +        if ( dabt.size == DABT_DOUBLE_WORD )
> +        {
> +            vits_spin_lock(vits);
> +            *r = vits->baser0;
> +            vits_spin_unlock(vits);
> +        }
> +        else
> +            goto bad_width;

I would prefer to see

if ( dabt.size != DATB_DOUBLE_WORD )
  goto bad_width;

vits_spin_lock(...)

To avoid one indentation layer more for most of the lines and keep the
error before.


> +        return 1;
> +    case GITS_BASER1 ... GITS_BASERN:
> +        goto read_as_zero;
> +    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_32;
> +   default:
> +        dprintk(XENLOG_G_ERR,
> +                "%pv: vITS: unhandled read r%"PRId32" offset 0x%#08"PRIx32"\n",

Reg is definitely not a PRId32.

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

Ditto.

> +            v, dabt.size, dabt.reg, gits_reg);
> +    domain_crash_synchronous();
> +    return 0;
> +
> +read_as_zero_32:
> +    if ( dabt.size != DABT_WORD ) goto bad_width;
> +read_as_zero:
> +    *r = 0;
> +    return 1;
> +}
> +
> +/*
> + * GITS_BASER.Type[58:56], GITS_BASER.Entry_size[55:48]
> + * and GITS_BASER.Shareability[11:10] are read-only.

As said on v4, implemented Shareability as fixed (i.e read-only) is
deprecated. I'd like to see a TODO here.

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

As said on v4,

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 latter. The former is a mandatory.

> +#define GITS_BASER_MASK  (~((0x7UL << GITS_BASER_TYPE_SHIFT)     | \
> +                         (0xffUL << GITS_BASER_ENTRY_SIZE_SHIFT) | \
> +                         (0x3UL << GITS_BASER_SHAREABILITY_SHIFT)))

[..]

> +    case GITS_CWRITER:
> +        if ( dabt.size != DABT_DOUBLE_WORD && dabt.size != DABT_WORD )
> +            goto bad_width;
> +        vits_spin_lock(vits);
> +        /* Only BITS[19:0] are writable */
> +        vits->cmd_write = *r & 0xfffe0;
> +        ret = 1;
> +        if ( vits->ctrl & GITS_CTLR_ENABLE )
> +        {
> +            /* CWRITER should be within the range */
> +            if ( vits->cmd_write < (vits->cmd_qsize & 0xfffe0) )
> +                ret = vits_process_cmd(v, vits);
> +        }

Please do only 1 if rather than 2 nested if for only one line.

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

[..]

>  int vits_domain_init(struct domain *d)
>  {
>      struct vgic_its *vits;
>      int i;
>  
> +    if ( is_hardware_domain(d) )
> +        d->arch.vgic.nr_lpis = num_of_lpis;
> +    else
> +        d->arch.vgic.nr_lpis = NR_LPIS;

NR_LPIS is defined in patch #14. And the name seems to be wrong.

Anyway, I don't understand why you are trying to initialize vITS on
guest. We agree that it should only be used on DOM0 for now until we
effectively need it for the guest.

Furthermore, it miss at least the toolstack in order to get the part
guest ready.

So please ensure that the vITS is not initialized for the guest.

> +
>      d->arch.vgic.vits = xzalloc(struct vgic_its);
>      if ( !d->arch.vgic.vits )
>          return -ENOMEM;
> @@ -539,6 +882,7 @@ int vits_domain_init(struct domain *d)
>      vits = d->arch.vgic.vits;
>  
>      spin_lock_init(&vits->lock);
> +    spin_lock_init(&vits->prop_lock);

The field prop_lock is added in patch #12. Please move the
spin_lock_init there.

To remind you, *every* patch should be able to compile one by one on
both aarch64, and arm32. I suspect this is not currently the case.

>  
>      vits->collections = xzalloc_array(struct its_collection, nr_cpu_ids);
>      if ( !vits->collections )
> @@ -550,6 +894,21 @@ int vits_domain_init(struct domain *d)
>      for ( i = 0; i < nr_cpu_ids; i++ )
>          vits->collections[i].target_address = ~0UL;
>  
> +    d->arch.vgic.id_bits = get_count_order(d->arch.vgic.nr_lpis + FIRST_GIC_LPI);

ID bits is defined in patch #12. And you don't even use it here...

> +    vits->baser0 = GITS_BASER_INIT_VAL;
> +
> +    if ( is_hardware_domain(d) )
> +    {
> +        /*
> +         * Only one virtual ITS is provided to domain.
> +         * Assign first physical ITS address to Dom0 virtual ITS.
> +         */
> +        vits->gits_base = vits_hw.info->its_hw[0].phys_base;
> +        vits->gits_size = vits_hw.info->its_hw[0].phys_size;
> +    }

Here another example where the code for the guest is missing. Please be
consistent i.e either try to support it at the whole or not at all.

The latter seems the more plausible for now.

> +
> +    register_mmio_handler(d, &vgic_gits_mmio_handler, vits->gits_base, SZ_64K);
> +
>      return 0;
>  }
>  
> diff --git a/xen/include/asm-arm/gic-its.h b/xen/include/asm-arm/gic-its.h
> index 23ff66c..0854cde 100644
> --- a/xen/include/asm-arm/gic-its.h
> +++ b/xen/include/asm-arm/gic-its.h
> @@ -21,6 +21,9 @@
>  #include <asm/gic_v3_defs.h>
>  #include <xen/rbtree.h>
>  
> +/* Number of LPI supported */
> +extern unsigned int num_of_lpis;
> +

The prototype of anything declared in irq.c should go in irq.h and not
in the GIC ITS header.

> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> index e330fe3..5f37f56 100644
> --- a/xen/include/asm-arm/gic.h
> +++ b/xen/include/asm-arm/gic.h
> @@ -20,6 +20,7 @@
>  
>  #define NR_GIC_LOCAL_IRQS  NR_LOCAL_IRQS
>  #define NR_GIC_SGI         16
> +#define FIRST_GIC_LPI      8192

It make little sense to define it in a patch called "Add GITS registers
emulation".

Regards,

-- 
Julien Grall