From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH v6 21/31] xen/arm: ITS: Add GITS registers emulation Date: Mon, 7 Sep 2015 14:14:16 +0100 Message-ID: <55ED8DA8.6060306@citrix.com> References: <1441019208-2764-1-git-send-email-vijay.kilari@gmail.com> <1441019208-2764-22-git-send-email-vijay.kilari@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1441019208-2764-22-git-send-email-vijay.kilari@gmail.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: vijay.kilari@gmail.com, Ian.Campbell@citrix.com, stefano.stabellini@eu.citrix.com, stefano.stabellini@citrix.com, tim@xen.org, xen-devel@lists.xen.org Cc: Prasun.Kapoor@caviumnetworks.com, Vijaya Kumar K , manish.jaggi@caviumnetworks.com List-Id: xen-devel@lists.xenproject.org Hi Vijay, On 31/08/15 12:06, vijay.kilari@gmail.com wrote: > From: Vijaya Kumar K > > Emulate GITS* registers > > Signed-off-by: Vijaya Kumar K > --- > 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