|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v6 21/31] xen/arm: ITS: Add GITS registers emulation
Hi Vijay,
On 31/08/15 12:06, vijay.kilari@xxxxxxxxx wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@xxxxxxxxxxxxxxxxxx>
>
> Emulate GITS* registers
>
> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@xxxxxxxxxxxxxxxxxx>
> ---
> v6: - Removed unrelated code of this patch
> - Used vgic_regN_{read,write}
> v4: - Removed GICR register emulation
> ---
> xen/arch/arm/vgic-v3-its.c | 337
> ++++++++++++++++++++++++++++++++++++++++-
> xen/include/asm-arm/gic-its.h | 16 ++
> 2 files changed, 352 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c
> index 53f2a27..c384ecf 100644
> --- a/xen/arch/arm/vgic-v3-its.c
> +++ b/xen/arch/arm/vgic-v3-its.c
> @@ -35,6 +35,14 @@
>
> //#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_BASER_INIT_VAL ((1UL << GITS_BASER_TYPE_SHIFT) | \
> + (0x7UL << GITS_BASER_ENTRY_SIZE_SHIFT))
This value is only for BASER0 so GITS_BASER0_INIT_VAL
You also need to explain what means the different values. And please
don't hardcode the size of the Device Entry but the proper sizeof.
> #ifdef DEBUG_ITS
> # define DPRINTK(fmt, args...) dprintk(XENLOG_DEBUG, fmt, ##args)
> #else
> @@ -535,7 +543,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)
You would have been able to keep the static when the patch has been
introduced if you have plumbed vgic-v3 for vITS support latter (i.e part
of #19 moved at the end).
> {
> its_cmd_block virt_cmd;
[...]
> +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);
> + /*
> + * vITS is always quiescent, has no translations in progress and
> + * completed all operations. So set quescent by default.
s/quescent/quiescent/
> + */
> + *r = vgic_reg32_read((vits->ctrl | GITS_CTLR_QUIESCENT), info);
IHMO the Quiescent bit should be set at write time not read time. So we
can use it in the emulation if necessary.
> + 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.dev_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);
> + if ( dabt.size == DABT_DOUBLE_WORD )
> + *r = vgic_reg64_read(val, info);
> + else
> + *r = vgic_reg32_read(val, info);
Can you explain why this is necessary? The goal of vgic_reg64_read is to
handle all access size to 64bit register.
The vgic_reg32_read will only handle access on 32bit and therefore it
won't be possible to access to the most significant word.
> + return 1;
> + case 0x0010 ... 0x007c:
> + case 0xc000 ... 0xffcc:
> + /* Implementation defined -- read ignored */
> + goto read_as_zero;
> + case GITS_CBASER:
> + case GITS_CBASER + 4:
> + /* Read supports only 32/64-bit access */
This comment is not necessary and is confusing as you call the helper now
> + if ( !vgic_reg64_check_access(dabt) ) goto bad_width;
> + vits_spin_lock(vits);
> + if ( dabt.size == DABT_DOUBLE_WORD )
> + *r = vgic_reg64_read(vits->cmd_base, info);
> + else
> + *r = vgic_reg32_read(vits->cmd_base, info);
Same remark as before. I have to say that those helpers are commented
with: "N-bit register helpers". Isn't it clear enough that N correspond
to the size of the GIC register?
> + vits_spin_unlock(vits);
> + return 1;
> + case GITS_CWRITER:
> + /* Read supports only 32/64-bit access */
> + vits_spin_lock(vits);
> + val = vits->cmd_write;
> + vits_spin_unlock(vits);
The size *should* be checked before locking the vITS and read the register.
Unless you have a strong reason to do it, please use the same pattern
everywhere and don't reinvent a new one.
> + if ( dabt.size == DABT_DOUBLE_WORD )
> + *r = vgic_reg64_read(val, info);
> + else if (dabt.size == DABT_WORD )
> + *r = vgic_reg32_read(val, info);
> + else
> + goto bad_width;
> + return 1;
> + case GITS_CWRITER + 4:
> + /* BITS[63:20] are RES0 */
NIT: s/BITS/Bits/
> + goto read_as_zero_32;
> + case GITS_CREADR:
> + /* Read supports only 32/64-bit access */
> + val = atomic_read(&vits->cmd_read);
> + if ( dabt.size == DABT_DOUBLE_WORD )
> + *r = vgic_reg64_read(val, info);
> + else if (dabt.size == DABT_WORD )
> + *r = vgic_reg32_read(val, info);
> + else
> + goto bad_width;
Ditto.
> + return 1;
> + case GITS_CREADR + 4:
> + /* BITS[63:20] are RES0 */
NIT: s/BITS/Bits/
> + goto read_as_zero_32;
> + case 0x0098 ... 0x009c:
> + case 0x00a0 ... 0x00fc:
> + case 0x0140 ... 0xbffc:
> + /* Reserved -- read ignored */
> + goto read_as_zero;
> + case GITS_BASER0:
> + /* Supports only 64-bit access */
The indentation is wrong and I don't see why you can't support 32-bit
with the helpers I've introduced...
> + if ( dabt.size == DABT_DOUBLE_WORD )
> + {
> + vits_spin_lock(vits);
> + *r = vgic_reg64_read(vits->baser0, info);
> + vits_spin_unlock(vits);
> + }
> + else
> + goto bad_width;
Please use the same pattern everywhere. I.e the bad case should be first
then the good case. This will dropped one layer of indentation and keep
all the emulation similar.
Actually, I did the same remark on v5... So if you think this is not the
right thing to do, please explain why and don't silently ignore the
comment...
> + return 1;
> + case GITS_BASER1 ... GITS_BASERN:
> + goto read_as_zero;
Those register as read as zero 64 bits...
> + case GITS_PIDR0:
> + if ( dabt.size != DABT_WORD ) goto bad_width;
> + *r = vgic_reg32_read(GITS_PIDR0_VAL, info);
> + return 1;
> + case GITS_PIDR1:
> + if ( dabt.size != DABT_WORD ) goto bad_width;
> + *r = vgic_reg32_read(GITS_PIDR1_VAL, info);
> + return 1;
> + case GITS_PIDR2:
> + if ( dabt.size != DABT_WORD ) goto bad_width;
> + *r = vgic_reg32_read(GITS_PIDR2_VAL, info);
> + return 1;
> + case GITS_PIDR3:
> + if ( dabt.size != DABT_WORD ) goto bad_width;
> + *r = vgic_reg32_read(GITS_PIDR3_VAL, info);
> + return 1;
> + case GITS_PIDR4:
> + if ( dabt.size != DABT_WORD ) goto bad_width;
> + *r = vgic_reg32_read(GITS_PIDR4_VAL, info);
> + 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",
I forgot to answer to " dabt.reg is unsigned long : 5. In vgic-v3.c it
is printed using %d."
%d matches with 8bit, 16bit and 32bit. In this case, the field reg is
5bits so this should technically PRId8.
You need to use your common sense to use PRIdX when necessary. PRIdX
should be used in sync with uintX_t. There is only few cases where it's
not the case like the domain ID.
In this case, it's a bitfield and therefore not uintX_t. So there is no
point to use PRIdX. I would prefer to see %d here.
> + 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,
> + * Here we support only flat table. So GITS_BASER.Indirect[62]
I would replace by: "Only flat table is supported ...."
> + * is RAZ/WI.
> + * Mask those fields while emulating GITS_BASER reg.
> + * TODO: Shareability is set to 0x0 (Reserved) which is fixed.
> + * Implementing fixed value for Shareability is deprecated.
This is not a TODO but a restriction here. A TODO would explain what we
have to do. I.e
TODO: Support shareability as fixed value is deprecated.
> + */
> +#define GITS_BASER_MASK (~((0x7UL << GITS_BASER_TYPE_SHIFT) | \
> + (0x1UL << GITS_BASER_INDIRECT_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.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);
> + int ret;
> + uint32_t gits_reg, sz, psz;
> + uint64_t val;
> +
> + gits_reg = info->gpa - vits->gits_base;
> +
> + DPRINTK("%pv: vITS: GITS_MMIO_WRITE 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);
> + vgic_reg32_write(&vits->ctrl, (*r & GITS_CTLR_ENABLE), info);
The Quiescent bit should be set here rather than in the read part.
> + vits_spin_unlock(vits);
> + return 1;
> + case GITS_IIDR:
> + /* RO -- write ignored */
> + goto write_ignore;
> + case GITS_TYPER:
> + case GITS_TYPER + 4:
> + /* RO -- write ignored */
> + goto write_ignore;
goto write_ignore_64;
> + case 0x0010 ... 0x007c:
> + case 0xc000 ... 0xffcc:
> + /* Implementation defined -- write ignored */
> + goto write_ignore;
> + case GITS_CBASER:
> + /* XXX: support 32-bit access */
What is missing to support 32-bit access? The helpers I've introduced
should done the job for you...
> + 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);
> + val = SZ_4K * ((*r & GITS_BASER_PAGES_MASK_VAL) + 1);
Please use vits->cmd_base rather than *r. So it will make free to
support 32bit.
> + vgic_reg64_write(&vits->cmd_qsize, val, info);
Why do you use vgic_reg64_write???? cmd_qsize is not at all a register
but not a field.
Also, according to the spec (see 8.19.2 IHI 0069A), this should be done
only when GITS_BASER_VALID is set.
> + if ( vits->cmd_base & GITS_BASER_VALID )
> + 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:0] are writable */
s/BITS/Bits/
> + vgic_reg64_write(&vits->cmd_write, (*r & 0xfffe0), info);
Either your comment or the implementation is wrong. Your comment is
saying that only bits[19:0] is writable but you mask the first 5 bits.
After reading the spec (see 8.19.5 IHI 0069A), I would say that your
comment is wrong. So please fix it.
> + ret = 1;
> + /* CWRITER should be within the range */
The comment is misplaced.
> + if ( (vits->ctrl & GITS_CTLR_ENABLE) &&
> + (vits->cmd_write < (vits->cmd_qsize & 0xfffe0)) )
The mask is not necessary. As the size of the queue should always point
to the last offset.
> + ret = vits_process_cmd(v, vits);
> + vits_spin_unlock(vits);
> + return ret;
> + case GITS_CWRITER + 4:
> + /* BITS[63:20] are RES0 */
Nit: s/BITS/Bits/
> + goto write_ignore_32;
> + case GITS_CREADR:
> + /* RO -- write ignored */
> + goto write_ignore;
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 */
What's the problem to support 32-bit access?
> + if ( dabt.size != DABT_DOUBLE_WORD )
> + goto bad_width;
> + vits_spin_lock(vits);
> + /* RO -- write ignored if GITS_CTLR.Enable = 1 */
Wrong indentation.
> + if ( vits->ctrl & GITS_CTLR_ENABLE )
> + {
> + vits_spin_unlock(vits);
> + goto write_ignore_64;
No need to re-check the size here. You can directly use write_ignore.
> + }
> + val = vits->baser0 | (*r & GITS_BASER_MASK);
> + vgic_reg64_write(&vits->baser0, val, info);
> + val = vits->baser0 & GITS_BASER_PA_MASK;
> + vgic_reg64_write(&vits->dt_ipa, val, info);
Again, vgic_regN_* must only be used for actual register not for access
any field in the vits structure...
> + 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;
> +
> + val = (vits->baser0 & GITS_BASER_PAGES_MASK_VAL)
> + * sz * SZ_1K;
> + vgic_reg64_write(&vits->dt_size, val, info);
Ditto
> + vits_spin_unlock(vits);
> + return 1;
> + case GITS_BASER1 ... GITS_BASERN:
> + if ( !vgic_reg64_check_access(dabt) ) goto bad_width;
> + goto write_ignore;
> + case GITS_PIDR7 ... GITS_PIDR0:
> + /* R0 -- write ignored */
> + goto write_ignore_32;
> + default:
> + dprintk(XENLOG_G_ERR,
> + "%pv vITS: unhandled write r%"PRId32" offset
> 0x%#08"PRIx32"\n",
Same remark as before for PRId32.
> + v, dabt.reg, gits_reg);
> + return 0;
> + }
> +
> +bad_width:
> + dprintk(XENLOG_G_ERR,
> + "%pv: vITS: bad write width %d r%"PRId32" offset
> 0x%#08"PRIx32"\n",
> + v, dabt.size, dabt.reg, gits_reg);
Ditto
> + domain_crash_synchronous();
> + return 0;
> +
> +write_ignore_64:
> + if ( dabt.size != DABT_DOUBLE_WORD ) goto bad_width;
A 64-bit register could be accessed using 32-bit and 64-bit. So you need
to use vgic_reg64_check_access here.
> + return 1;
> +write_ignore_32:
> + if ( dabt.size != DABT_WORD ) goto bad_width;
> + return 1;
> +write_ignore:
> + return 1;
> +}
[...]
> diff --git a/xen/include/asm-arm/gic-its.h b/xen/include/asm-arm/gic-its.h
> index a3d21f7..db1530e 100644
> --- a/xen/include/asm-arm/gic-its.h
> +++ b/xen/include/asm-arm/gic-its.h
[...]
> /*
> * ITS commands
> @@ -124,6 +132,8 @@ struct its_collection {
> struct vgic_its
> {
> spinlock_t lock;
> + /* Emulation of BASER0 */
Please add a word to explain what is the usage of GITS_BASER0.
> + paddr_t baser0;
paddr_t should only be used for actual physical address. This is not the
case of baser0 as it's a 64-bit register.
Please use uint64_t here.
> /* Command queue base */
> paddr_t cmd_base;
> /* Command queue write pointer */
> @@ -132,6 +142,12 @@ 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;
> + /* GICR ctrl register */
> + uint32_t ctrl;
Please put all the emulation register field in the same place. I.e put
baser0 and ctlr together.
> /* 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
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |