From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [RFC PATCH v2 14/22] xen/arm: its: Add emulation of ITS control registers Date: Tue, 24 Mar 2015 17:12:30 +0000 Message-ID: <55119AFE.6060803@linaro.org> References: <1426775889-29442-1-git-send-email-vijay.kilari@gmail.com> <1426775889-29442-15-git-send-email-vijay.kilari@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1426775889-29442-15-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@caviumnetworks.com, manish.jaggi@caviumnetworks.com List-Id: xen-devel@lists.xenproject.org Hello Vijay, On 19/03/2015 14:38, vijay.kilari@gmail.com wrote: > From: Vijaya Kumar K > > Add support for emulating GITS_* registers > > Signed-off-by: Vijaya Kumar K > --- > v2: - Each Virtual ITS is attached to Physical ITS. > - Introduce helper function to lock and unlock > virtual ITS lock. > - Introduced helper to get virtual ITS structure pointer > based on emulation address. > --- > xen/arch/arm/gic-v3-its.c | 8 + > xen/arch/arm/vgic-v3-its.c | 412 +++++++++++++++++++++++++++++++++++++++++ > xen/include/asm-arm/gic-its.h | 1 + > 3 files changed, 421 insertions(+) > > diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c > index a9aab73..e382f8d 100644 > --- a/xen/arch/arm/gic-v3-its.c > +++ b/xen/arch/arm/gic-v3-its.c > @@ -101,6 +101,8 @@ struct its_node { > }; > > uint32_t pta_type; > +/* Number of physical its nodes present */ > +uint32_t nr_its = 0; This variable is not exported so static. Although, I'm not convinced this variable is useful. See my comments later. > > #define ITS_ITT_ALIGN SZ_256 > > @@ -146,6 +148,11 @@ uint32_t its_get_pta_type(void) > return pta_type; > } > > +uint32_t its_get_nr_its(void) > +{ > + return nr_its; > +} > + > struct its_node * its_get_phys_node(uint32_t dev_id) > { > struct its_node *its; > @@ -1170,6 +1177,7 @@ static int its_probe(struct dt_device_node *node) > } > spin_lock(&its_lock); > list_add(&its->entry, &its_nodes); > + nr_its++; > spin_unlock(&its_lock); > > return 0; > diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c > index 7530a88..4d8945f 100644 > --- a/xen/arch/arm/vgic-v3-its.c > +++ b/xen/arch/arm/vgic-v3-its.c > @@ -869,6 +869,418 @@ err: > return 0; > } > > +struct vgic_its *its_to_vits(struct vcpu *v, paddr_t phys_base) This function is not exported so static. Also, it looks like to me that the vcpu is not necessary. Please use struct domain *d. > +{ > + struct vgic_its *vits = NULL; > + int i; > + > + /* Mask 64K offset */ > + phys_base = phys_base & ~(SZ_64K - 1); > + if ( is_hardware_domain(v->domain) ) Why do you need to have a specific case for the hardware domain? All the vITS code should be domain agnostic except the initialization function. That would make the code a lot simpler. > + { > + for ( i = 0; i < its_get_nr_its(); i++ ) I would prefer if you introduce a new field in domain->arch to store the number of ITS for the domain. > + { > + if ( v->domain->arch.vits[i].phys_base == phys_base ) > + { > + vits = &v->domain->arch.vits[i]; > + break; > + } > + } > + } > + else > + vits = &v->domain->arch.vits[0]; You should not assume that the guest as only one vITS. > + > + return vits; > +} > + > +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; > + 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 index, gits_reg; > + > + vits = its_to_vits(v, info->gpa); > + if ( vits == NULL ) BUG_ON(1); BUG_ON(vits != NULL); Although I would document this BUG_ON to explain that its_to_vits should never fail because MMIOs registered always point to an ITS. Though, an ASSERT maybe better here. > + > + gits_reg = info->gpa - vits->phys_base; > + > + if ( gits_reg >= SZ_64K ) > + { > + gdprintk(XENLOG_G_WARNING, "vGITS: unknown gpa read address \ > + %"PRIpaddr"\n", info->gpa); > + return 0; > + } This can never happen, you always register 64K range. > + > + switch ( gits_reg ) > + { > + case GITS_CTLR: > + if ( dabt.size != DABT_WORD ) goto bad_width; Missing implementation for GITS_CTLR > + return 1; > + case GITS_IIDR: > + if ( dabt.size != DABT_WORD ) goto bad_width; Missing implementation for GITS_IIDR > + return 1; > + case GITS_TYPER: > + /* GITS_TYPER support word read */ > + vits_spin_lock(vits); > + val = ((its_get_pta_type() << VITS_GITS_TYPER_PTA_SHIFT) | As said on a previous patch, each ITS may have a different value in PTA. I think it would make the command emulation simpler if we use an hardcoded PTA (PTA = 0 i.e using linear processor numbers seems the simpler). > + VITS_GITS_TYPER_HCC | VITS_GITS_DEV_BITS | I will comment the value here.. Where does the value of HCC and DEV_BITS come from? Both of them looks wrong to me. > + VITS_GITS_ID_BITS | VITS_GITS_ITT_SIZE | Ditto for ID_BITS and ITT_SIZE. Although, it looks like that ITT_SIZE should be the same as the hardware. This is because you let the guest allocation the ITT. I would also rename ITT_SIZE to ITT_ENTRY_SIZE. > + VITS_GITS_DISTRIBUTED | VITS_GITS_PLPIS); The bit 3 is marked as implementation defined. So why did you name it DISTRIBUTED? > + if ( dabt.size == DABT_DOUBLE_WORD ) > + *r = val; > + else if ( dabt.size == DABT_WORD ) > + *r = (u32)(val >> 32); > + else > + { > + vits_spin_unlock(vits); > + goto bad_width; > + } > + vits_spin_unlock(vits); The vits_spin_unlock could be done before setting *r. > + return 1; > + case GITS_TYPER + 4: I don't like the idea to duplicate the code for GITS_TYPER just for reading the top word. Isn't possible to merge the 2 switch case? Reading the spec again, it's not mandatory support support 32-bit access on 64-bit registers. Given that we don't support GICv3 for 32-bit guest, I would completely drop the 32-bit access on 64-bit guest. > + if (dabt.size != DABT_WORD ) goto bad_width; if ( ... > + vits_spin_lock(vits); > + val = ((its_get_pta_type() << VITS_GITS_TYPER_PTA_SHIFT) | > + VITS_GITS_TYPER_HCC | VITS_GITS_DEV_BITS | > + VITS_GITS_ID_BITS | VITS_GITS_ITT_SIZE | > + VITS_GITS_DISTRIBUTED | VITS_GITS_PLPIS); > + *r = (u32)val; > + vits_spin_unlock(vits); > + return 1; > + case 0x0010 ... 0x007c: > + case 0xc000 ... 0xffcc: > + /* Implementation defined -- read ignored */ > + dprintk(XENLOG_ERR, > + "vGITS: read unknown 0x000c - 0x007c r%d offset %#08x\n", > + dabt.reg, gits_reg); Please don't use XENLOG_ERR in guest code. Also, this printk is not useful and has been dropped in other emulation. > + goto read_as_zero; > + case GITS_CBASER: > + vits_spin_lock(vits); > + if ( dabt.size == DABT_DOUBLE_WORD ) > + *r = vits->cmd_base && 0xc7ffffffffffffffUL; Why the && and what does mean this constant? > + else if ( dabt.size == DABT_WORD ) > + *r = (u32)vits->cmd_base; > + else > + { > + vits_spin_unlock(vits); > + goto bad_width; > + } > + vits_spin_unlock(vits); > + return 1; > + case GITS_CBASER + 4: > + /* CBASER support word read */ > + if (dabt.size != DABT_WORD ) goto bad_width; if ( ... > + vits_spin_lock(vits); > + *r = (u32)(vits->cmd_base >> 32); > + vits_spin_unlock(vits); > + return 1; Same remark as GITS_TYPER for the word read support. > + case GITS_CWRITER: > + vits_spin_lock(vits); > + if ( dabt.size == DABT_DOUBLE_WORD ) > + *r = vits->cmd_write; > + else if ( dabt.size == DABT_WORD ) > + *r = (u32)vits->cmd_write; > + else > + { > + vits_spin_unlock(vits); > + goto bad_width; > + } > + vits_spin_unlock(vits); > + return 1; > + case GITS_CWRITER + 4: > + /* CWRITER support word read */ > + if ( dabt.size != DABT_WORD ) goto bad_width; > + vits_spin_lock(vits); > + *r = (u32)(vits->cmd_write >> 32); > + vits_spin_unlock(vits); > + return 1; Ditt for the word-read > + case GITS_CREADR: > + vits_spin_lock(vits); > + if ( dabt.size == DABT_DOUBLE_WORD ) > + *r = vits->cmd_read; > + else if ( dabt.size == DABT_WORD ) > + *r = (u32)vits->cmd_read; > + else > + { > + vits_spin_unlock(vits); > + goto bad_width; > + } > + vits_spin_unlock(vits); > + return 1; > + case GITS_CREADR + 4: > + /* CREADR support word read */ > + if ( dabt.size != DABT_WORD ) goto bad_width; > + vits_spin_lock(vits); > + *r = (u32)(vits->cmd_read >> 32); > + vits_spin_unlock(vits); > + return 1; Ditto > + case 0x0098 ... 0x009c: > + case 0x00a0 ... 0x00fc: > + case 0x0140 ... 0xbffc: > + /* Reserved -- read ignored */ > + dprintk(XENLOG_ERR, > + "vGITS: read unknown 0x0098-9c or 0x00a0-fc r%d offset %#08x\n", > + dabt.reg, gits_reg); No need of printk here. > + goto read_as_zero; > + case GITS_BASER ... GITS_BASERN: The spec says that registers are RES0 if not implemented. As you use at all baser outside the register emulation, I would implement them RAZ/WI. That would avoid a wrong write implementation. > + vits_spin_lock(vits); > + index = (gits_reg - GITS_BASER) / 8; > + if ( dabt.size == DABT_DOUBLE_WORD ) > + *r = vits->baser[index]; > + else if ( dabt.size == DABT_WORD ) > + { > + if ( (gits_reg % 8) == 0 ) > + *r = (u32)vits->baser[index]; > + else > + *r = (u32)(vits->baser[index] >> 32); > + } > + else > + { > + vits_spin_unlock(vits); > + goto bad_width; > + } > + vits_spin_unlock(vits); > + return 1; > + 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; Please check that we access is done via a word-access by introducing a new label read_as_zero_32 (for instance see the vgic v2 emulation). > + default: > + dprintk(XENLOG_ERR, "vGITS: unhandled read r%d offset %#08x\n", > + dabt.reg, gits_reg); printk(XENLOG_G_ERR "%pv: ....", v,...) Also it may be useful to printk which vITS is in use. > + return 0; > + } > + > +bad_width: > + dprintk(XENLOG_ERR, "vGITS: bad read width %d r%d offset %#08x\n", > + dabt.size, dabt.reg, gits_reg); printk(XENLOG_G_ERR "%pv: ...", v,...) Same remark for printing the vITS. > + domain_crash_synchronous(); > + return 0; > + > +read_as_zero: > + if ( dabt.size != DABT_WORD ) goto bad_width; How do you know that all RAZ access 32-bit access? See implementation defined registers for instance. I would prefer to introduce multiple label: read_as_zero_32: /* RAZ 32-bit */ if ( dabt.size != DABT_WORD ) goto bad_width; read_as_zero: /* Not check necessary */ *r = 0; And use the correctly label for goto in the emulation. So the code would be self-documented too. > + *r = 0; > + return 1; > +} > + > +static int vgic_v3_gits_mmio_write(struct vcpu *v, mmio_info_t *info) > +{ > + struct vgic_its *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 index, gits_reg; > + uint64_t val; > + > + vits = its_to_vits(v, info->gpa); > + if ( vits == NULL ) BUG_ON(1); Same remark as the BUG_ON in vgic_v3_gits_mmio_read. I'm wondering if it would be better to extend the read/write handler to get an opaque pointer in parameter. In this case, it would contain the vits and would avoid the its_to_vits every time. > + gits_reg = info->gpa - vits->phys_base; > + > + if ( gits_reg >= SZ_64K ) > + { > + gdprintk(XENLOG_G_WARNING, "vGIC-ITS: unknown gpa write address" > + " %"PRIpaddr"\n", info->gpa); > + return 0; > + } This check is not necessary. > + switch ( gits_reg ) > + { > + case GITS_CTLR: > + if ( dabt.size != DABT_WORD ) goto bad_width; > + vits_spin_lock(vits); > + vits->ctrl = *r; Only bit[0] (Enabled) is writable. > + vits_spin_unlock(vits); > + return 1; > + case GITS_IIDR: > + /* R0 -- write ignored */ > + goto write_ignore; goto write_ignore_32; > + case GITS_TYPER: > + case GITS_TYPER + 4: > + /* R0 -- write ignored */ > + goto write_ignore; Please explicitly check the access size. That would avoid to crash the guest when TYPER is write with a 64-bit access. > + case 0x0010 ... 0x007c: > + case 0xc000 ... 0xffcc: > + /* Implementation defined -- write ignored */ > + dprintk(XENLOG_ERR, > + "vGITS: write to unknown 0x000c - 0x007c r%d offset %#08x\n", > + dabt.reg, gits_reg); Please drop the dprintk. > + goto write_ignore; > + case GITS_CBASER: > + if ( dabt.size == DABT_BYTE ) goto bad_width; Please do the check in the invert way. i.e (dabt.size != DABT_WORD) && (dabt.size != DABT_DOUBLE_WORD) Also GITS_CBASER is read-only when GITS_CTLR.Enable is Zero or GITS_CTLR.Quiescent is zero. > + vits_spin_lock(vits); > + if ( dabt.size == DABT_DOUBLE_WORD ) > + vits->cmd_base = *r; > + else > + { > + val = vits->cmd_base & 0xffffffff00000000UL; The mask is difficult to read. And not all the bits are writeable. > + val = (*r) | val; > + vits->cmd_base = val; > + } > + vits->cmd_qsize = SZ_4K * ((*r & 0xff) + 1); Please use a define for the mask. Also I would use cmd_qsize to know if the valid is set or not. I.e cmd_qsize = 0 => command queue not valid. You forgot to update GITS_CREADR (i.e setting to 0) when GITS_CBASER is successfully written. > + vits_spin_unlock(vits); > + return 1; > + case GITS_CBASER + 4: 32-bit support is not necessary and make the code more complex for nothing. > + /* CBASER support word read */ > + if (dabt.size != DABT_WORD ) goto bad_width; > + vits_spin_lock(vits); > + val = vits->cmd_base & 0xffffffffUL; > + val = ((*r & 0xffffffffUL) << 32 ) | val; > + vits->cmd_base = val; > + /* No Need to update cmd_qsize with higher word write */ > + vits_spin_unlock(vits); > + return 1; > + case GITS_CWRITER: > + if ( dabt.size == DABT_BYTE ) goto bad_width; > + vits_spin_lock(vits); > + if ( dabt.size == DABT_DOUBLE_WORD ) > + vits->cmd_write = *r; Only Bits[19:5] are writable. > + else > + { > + val = vits->cmd_write & 0xffffffff00000000UL; > + val = (*r) | val; > + vits->cmd_write = val; > + } No validation of the value written by the guest? Given your implementation of the command processing, any invalid value will end up to an infinite loop in the hypervisor. Whoops :). > + ret = vgic_its_process_cmd(v, vits); > + vits_spin_unlock(vits); > + return ret; > + case GITS_CWRITER + 4: Same remark as GITS_CBASER for the 32-bit support. > + if (dabt.size != DABT_WORD ) goto bad_width; > + vits_spin_lock(vits); > + val = vits->cmd_write & 0xffffffffUL; > + val = ((*r & 0xffffffffUL) << 32) | val; > + vits->cmd_write = val; > + ret = vgic_its_process_cmd(v, vits); > + vits_spin_unlock(vits); > + return ret; > + case GITS_CREADR: > + /* R0 -- write ignored */ > + goto write_ignore; > + case 0x0098 ... 0x009c: > + case 0x00a0 ... 0x00fc: > + case 0x0140 ... 0xbffc: > + /* Reserved -- write ignored */ > + dprintk(XENLOG_ERR, > + "vGITS: write to unknown 0x98-9c or 0xa0-fc r%d offset %#08x\n", > + dabt.reg, gits_reg); Please drop the dprintk > + goto write_ignore; > + case GITS_BASER ... GITS_BASERN: > + /* Nothing to do with this values. Just store and emulate */ As you don't use those values at all, write ignore would be better. > + vits_spin_lock(vits); > + index = (gits_reg - GITS_BASER) / 8; > + if ( dabt.size == DABT_DOUBLE_WORD ) > + vits->baser[index] = *r; > + else if ( dabt.size == DABT_WORD ) > + { > + if ( (gits_reg % 8) == 0 ) > + { > + val = vits->cmd_write & 0xffffffff00000000UL; cmd_write seems to come out of nowhere... > + val = (*r) | val; > + vits->baser[index] = val; > + } > + else > + { > + val = vits->baser[index] & 0xffffffffUL; > + val = ((*r & 0xffffffffUL) << 32) | val; > + vits->baser[index] = val; > + } > + } > + else > + { > + goto bad_width; > + vits_spin_unlock(vits); > + } > + vits_spin_unlock(vits); > + return 1; > + case GITS_PIDR7 ... GITS_PIDR0: > + /* R0 -- write ignored */ > + goto write_ignore; > + default: > + dprintk(XENLOG_ERR, "vGITS: unhandled write r%d offset %#08x\n", > + dabt.reg, gits_reg); printk(XENLOG_G_ERR "%pv: .....", v, ....); + Print which ITS is in use. > + return 0; > + } > + > +bad_width: > + dprintk(XENLOG_ERR, "vGITS: bad write width %d r%d offset %#08x\n", > + dabt.size, dabt.reg, gits_reg); Ditto > + domain_crash_synchronous(); > + return 0; > + > +write_ignore: > + if ( dabt.size != DABT_WORD ) goto bad_width; > + *r = 0; > + return 1; Same remark as read_ignore. > +} > + > +static const struct mmio_handler_ops vgic_gits_mmio_handler = { > + .read_handler = vgic_v3_gits_mmio_read, > + .write_handler = vgic_v3_gits_mmio_write, > +}; > + > +int vgic_its_domain_init(struct domain *d) You forgot to add the prototype of this function in the header... > +{ This code is not really part of the ITS registers emulation... Your patchs series splitting is really confusing. > + uint32_t num_its; > + int i; > + > + num_its = its_get_nr_its(); > + > + d->arch.vits = xzalloc_array(struct vgic_its, num_its); Hmm... why did you use the number of physical ITS rather than the number of vITS used by the guest. It would avoid to waste so much memory for every domain. > + if ( d->arch.vits == NULL ) > + return -ENOMEM; > + > + spin_lock_init(&d->arch.vits->lock); > + > + spin_lock_init(&d->arch.vits_devs.lock); > + INIT_LIST_HEAD(&d->arch.vits_devs.dev_list); > + > + d->arch.lpi_conf = xzalloc(struct vgic_lpi_conf); > + if ( d->arch.lpi_conf == NULL ) > + return -ENOMEM; > + > + for ( i = 0; i < num_its; i++) > + { > + spin_lock_init(&d->arch.vits[i].lock); > + register_mmio_handler(d, &vgic_gits_mmio_handler, > + d->arch.vits[i].phys_base, > + SZ_64K); > + } > + > + return 0; > +} > + > /* > * Local variables: > * mode: C > diff --git a/xen/include/asm-arm/gic-its.h b/xen/include/asm-arm/gic-its.h > index 70ec913..82cfbdc 100644 > --- a/xen/include/asm-arm/gic-its.h > +++ b/xen/include/asm-arm/gic-its.h > @@ -227,6 +227,7 @@ int gic_its_send_cmd(struct vcpu *v, struct its_node *its, > void its_lpi_free(unsigned long *bitmap, int base, int nr_ids); > unsigned long *its_lpi_alloc_chunks(int nirqs, int *base, int *nr_ids); > uint32_t its_get_pta_type(void); > +uint32_t its_get_nr_its(void); > struct its_node * its_get_phys_node(uint32_t dev_id); > #endif /* __ASM_ARM_GIC_ITS_H__ */ Regards, -- Julien Grall