Re: [Xen-devel] [PATCH v7 17/28] xen/arm: ITS: Add GITS registers emulation

[Julien Grall <julien.grall@xxxxxxxxxx>]

Hi Vijay,

On 18/09/15 14:09, vijay.kilari@xxxxxxxxx wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@xxxxxxxxxxxxxxxxxx>
> 
> Emulate GITS* registers
> 
> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@xxxxxxxxxxxxxxxxxx>
> ---
> v7: - Fixed wrong usage of vgic_regN* helpers
>     - coding styles and comments
>     - GITS_BASER0 is always overwritten with new value every time
>       it is updated.
> v6: - Removed unrelated code of this patch
>     - Used vgic_regN_{read,write}
> v4: - Removed GICR register emulation
> ---
>  xen/arch/arm/vgic-v3-its.c    |  315 
> +++++++++++++++++++++++++++++++++++++++++
>  xen/include/asm-arm/gic-its.h |    8 ++
>  xen/include/asm-arm/vits.h    |    8 ++
>  3 files changed, 331 insertions(+)
> 
> diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c
> index dba0b9e..0d97fcb 100644
> --- a/xen/arch/arm/vgic-v3-its.c
> +++ b/xen/arch/arm/vgic-v3-its.c
> @@ -36,6 +36,15 @@
>  
>  //#define DEBUG_ITS
>  
> +/* 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_BASER0_INIT_VAL         ((1UL << GITS_BASER_TYPE_SHIFT) | \
> +                                      (sizeof(struct vitt) <<          \
> +                                      GITS_BASER_ENTRY_SIZE_SHIFT))

As asked on v6, I'd like to see a comment explaining the different
values and the usage of BASER0. I.e something like "BASER0 is used to
ask the guest to allocate memory for the Device table".

>  #ifdef DEBUG_ITS
>  # define DPRINTK(fmt, args...) dprintk(XENLOG_DEBUG, fmt, ##args)
>  #else
> @@ -503,6 +512,301 @@ 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.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;
> +    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);
> +        /*
> +         * vITS is always quiescent, has no translations in progress and
> +         * completed all operations. So set quiescent by default.
> +         */

Well, this comment should have been moved at the same place as you moved
the setting of QUIESCENT bit (i.e write).

> +        *r = vgic_reg32_read(vits->ctrl, info);
> +        vits_spin_unlock(vits);
> +        return 1;
> +    case GITS_IIDR:
> +        if ( dabt.size != DABT_WORD ) goto bad_width;
> +        *r = vgic_reg32_read(GICV3_GICD_IIDR_VAL, info);
> +        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)
> +         * GITS_TYPER.ITTSize = Size of struct vitt
> +         * GITS_TYPER.Physical = 1
> +         */
> +        if ( !vgic_reg64_check_access(dabt) ) goto bad_width;
> +        val = ((vits_get_max_collections(v->domain) << GITS_TYPER_HCC_SHIFT 
> ) |
> +               ((vits_hw.devID_bits - 1) << GITS_TYPER_DEVBITS_SHIFT)        
>  |
> +               ((vits_hw.eventID_bits - 1) << GITS_TYPER_IDBITS_SHIFT)       
>  |
> +               ((sizeof(struct vitt) - 1) << GITS_TYPER_ITT_SIZE_SHIFT)      
>  |
> +                 GITS_TYPER_PHYSICAL_LPIS);
> +        *r = vgic_reg64_read(val, info);
> +        return 1;
> +    case 0x0010 ... 0x007c:
> +    case 0xc000 ... 0xffcc:
> +        /* Implementation defined -- read ignored */
> +        goto read_as_zero;
> +    case GITS_CBASER:
> +    case GITS_CBASER + 4:
> +        if ( !vgic_reg64_check_access(dabt) ) goto bad_width;
> +        vits_spin_lock(vits);
> +        *r = vgic_reg64_read(vits->cmd_base, info);

I mentioned it in v6 for cmd_qsize but fail to notice it for cmd_base.

You are using cmd_base as a 64-bit register, although you've defined
cmd_base a paddr_t. Even though the two are 64-bit this is conceptually
completely different, you have to define cmd_base using uint64_t.

> +        vits_spin_unlock(vits);
> +        return 1;
> +    case GITS_CWRITER:
> +        if ( !vgic_reg64_check_access(dabt) ) goto bad_width;
> +        vits_spin_lock(vits);
> +        *r = vgic_reg64_read(vits->cmd_write, info);

Ditto for cmd_write.

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

[...]

> +static int vgic_v3_gits_mmio_write(struct vcpu *v, mmio_info_t *info)
> +{

[...]

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

You haven't asked my question on v6... What is missing to support 32-bt
support? AFAICT the register helpers should the job for you. Correct?

> +        if ( dabt.size != DABT_DOUBLE_WORD )
> +            goto bad_width;
> +        vits_spin_lock(vits);
> +        if ( vits->ctrl & GITS_CTLR_ENABLE )
> +        {
> +           /* RO -- write ignored */
> +            vits_spin_unlock(vits);
> +            goto write_ignore;
> +        }
> +        vgic_reg64_write(&vits->cmd_base, *r, info);
> +        if ( vits->cmd_base & GITS_BASER_VALID )
> +        {
> +            val = ((vits->cmd_base & GITS_BASER_PAGES_MASK_VAL) + 1) * SZ_4K;
> +            vits->cmd_qsize = val;
> +            atomic_set(&vits->cmd_read, 0);
> +        }
> +        vits_spin_unlock(vits);
> +        return 1;
> +    case GITS_CWRITER:
> +        if ( !vgic_reg64_check_access(dabt) ) goto bad_width;
> +        vits_spin_lock(vits);
> +        /* Only Bits[19:5] are writable */
> +        vgic_reg64_write(&vits->cmd_write, (*r & 0xfffe0), info);
> +        ret = 1;
> +        /* CWRITER should be within command queue range */
> +        if ( (vits->ctrl & GITS_CTLR_ENABLE) &&
> +             (vits->cmd_write < vits->cmd_qsize) )
> +            ret = vits_process_cmd(v, vits);
> +        vits_spin_unlock(vits);
> +        return ret;
> +    case GITS_CWRITER + 4:
> +        /* Bits[63:20] are RES0 */
> +        goto write_ignore_32;
> +    case GITS_CREADR:
> +        /* RO -- write ignored */
> +        goto write_ignore_64;
> +    case 0x0098 ... 0x009c:
> +    case 0x00a0 ... 0x00fc:
> +    case 0x0140 ... 0xbffc:
> +        /* Reserved -- write ignored */
> +        goto write_ignore;
> +    case GITS_BASER0:
> +        /* Support only 64-bit access */

You didn't answer my question on v6. What's the problem to support
32-bit access?

If it's not fixed right now, this have to be fixed before Xen 4.7 is
released because some OS may decide to use 32 bit which are entirely valid.

> +        if ( dabt.size != DABT_DOUBLE_WORD )
> +            goto bad_width;
> +        vits_spin_lock(vits);
> +        /* RO -- write ignored if GITS_CTLR.Enable = 1 */
> +        if ( vits->ctrl & GITS_CTLR_ENABLE )
> +        {
> +            vits_spin_unlock(vits);
> +            goto write_ignore;
> +        }
> +        val = GITS_BASER0_INIT_VAL | (*r & GITS_BASER_MASK);
> +        vgic_reg64_write(&vits->baser0, val, info);
> +        vits->dt_ipa = vits->baser0 & GITS_BASER_PA_MASK;
> +        psz = (vits->baser0 >> 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
> +            /* 0x11 and 0x10 are treated as 64K size */
> +            sz = 64;
> +
> +        vits->dt_size = (vits->baser0 & GITS_BASER_PAGES_MASK_VAL) * sz * 
> SZ_1K;
> +        vits_spin_unlock(vits);
> +        return 1;
> +    case GITS_BASER1 ... GITS_BASERN:
> +        if ( !vgic_reg64_check_access(dabt) ) goto bad_width;
> +        goto write_ignore;

NIT: These 2 lines can be replaced by

goto write_ignore_64;

[...]

> diff --git a/xen/include/asm-arm/vits.h b/xen/include/asm-arm/vits.h
> index 295cce4..dc02762 100644
> --- a/xen/include/asm-arm/vits.h
> +++ b/xen/include/asm-arm/vits.h
> @@ -24,6 +24,10 @@
>  struct vgic_its
>  {
>     spinlock_t lock;
> +   /* Emulation of BASER0 */

As asked on v6, please add a comment to explain the usage of baser0.

> +   uint64_t baser0;
> +   /* GICR ctrl register */
> +   uint32_t ctrl;
>     /* Command queue base */
>     paddr_t cmd_base;
>     /* Command queue write pointer */
> @@ -32,6 +36,10 @@ struct vgic_its
>     atomic_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;
>     /* vITT device table ipa */
>     paddr_t dt_ipa;
>     /* vITT device table size */
> 

Regards

-- 
Julien Grall

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