[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v3 14/16] xen/arm: Add virtual GICv3 support
Hello Vijaya, Thank you for the patch. On 04/15/2014 12:17 PM, vijay.kilari@xxxxxxxxx wrote: > From: Vijaya Kumar K <Vijaya.Kumar@xxxxxxxxxxxxxxxxxx> > > Add virtual GICv3 driver support > > This patch adds only basic v3 support. > Does not support Interrupt Translation support (ITS) > > Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@xxxxxxxxxxxxxxxxxx> > --- > xen/arch/arm/Makefile | 2 +- > xen/arch/arm/vgic-v3.c | 835 > +++++++++++++++++++++++++++++++++++++ > xen/arch/arm/vgic.c | 4 +- > xen/include/asm-arm/gic_v3_defs.h | 2 + > xen/include/asm-arm/vgic.h | 9 +- > 5 files changed, 849 insertions(+), 3 deletions(-) > > diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile > index 39166aa..d269191 100644 > --- a/xen/arch/arm/Makefile > +++ b/xen/arch/arm/Makefile > @@ -26,7 +26,7 @@ obj-y += smpboot.o > obj-y += smp.o > obj-y += shutdown.o > obj-y += traps.o > -obj-y += vgic.o vgic-v2.o > +obj-y += vgic.o vgic-v3.o vgic-v2.o Same as for patch #13, vgic-v3 should only be compiled on armv8 (i.e ARM64). > obj-y += vtimer.o > obj-y += vuart.o > obj-y += hvm.o > diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c > new file mode 100644 > index 0000000..e3d773a > --- /dev/null > +++ b/xen/arch/arm/vgic-v3.c > @@ -0,0 +1,835 @@ > +/* > + * xen/arch/arm/vgic-v3.c > + * > + * ARM Virtual Generic Interrupt Controller v3 support > + * based on xen/arch/arm/vgic.c > + * > + * Vijaya Kumar K <vijaya.kumar@xxxxxxxxxxxxxxxxxx> > + * Copyright (c) 2014 Cavium Inc. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + */ > + > +#include <xen/bitops.h> > +#include <xen/config.h> > +#include <xen/lib.h> > +#include <xen/init.h> > +#include <xen/softirq.h> > +#include <xen/irq.h> > +#include <xen/sched.h> > + > +#include <asm/current.h> > +#include <asm/device.h> > + > +#include <asm/mmio.h> > +#include <asm/gic_v3_defs.h> > +#include <asm/gic.h> > +#include <asm/vgic.h> > + > +#define REG(n) (n) Why an id macro? > +/* > + * Offset of GICD_<FOO><n> with its rank, for GICD_<FOO> with > + * <b>-bits-per-interrupt. > + */ > +/* Shift n >> 2 to make it byte register diff */ > +#define REG_RANK_INDEX(b, n) (((n) >> 2) & ((b)-1)) > + > +/* > + * Returns rank corresponding to a GICD_<FOO><n> register for > + * GICD_<FOO> with <b>-bits-per-interrupt. > + */ > +static struct vgic_irq_rank *vgic_irq_rank(struct vcpu *v, int b, int n) > +{ > + int rank; > + > + /* divide by 4 to make it byte size register difference > + as n is difference of 4 byte size register */ > + n = n >> 2; > + rank = REG_RANK_NR(b, n); > + > + return vgic_get_irqrank(v, rank); > +} > + > +static int vgic_read_priority(struct vcpu *v, int irq) > +{ > + struct vgic_irq_rank *rank = vgic_irq_rank(v, 8, irq); Newline here for more clarity. > + return byte_read(rank->ipriority[REG_RANK_INDEX(8, irq)], 0, irq & 0x3); > +} > + > +static int __vgic_rdistr_mmio_read(struct vcpu *v, mmio_info_t *info, > + uint32_t offset) > +{ You have to check the size for all cases in this function... > + 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 gicr_reg = REG(offset); > + u64 mpidr; > + u64 aff; > + > + switch ( gicr_reg ) > + { > + case GICR_CTLR: > + /* We have implemented LPI's, read zero */ You are not constant in the comment. Did you implement LPI or not? > + goto read_as_zero; > + case GICR_IIDR: > + *r = GICR_IIDR_VAL; > + return 1; > + case GICR_TYPER: > + mpidr = cpu_logical_map(smp_processor_id()); Why do you use the physical CPU ID? Should not it be the vCPU ID? FYI, VCPU are not necessary mapped 1:1 and can be moved. > + aff = mpidr & ((1 << 24) - 1); > + aff |= (mpidr >> 8) & (0xffUL << 24); > + aff = aff << 32; > + aff = (aff | ((u64)smp_processor_id() << 8)); > + aff = (aff | (0x9UL << 0)); > + *r = aff; You only use aff, mpidr in this case, right? If so, please define the both variable in the case. > + return 1; > + case GICR_STATUSR: > + /* Not implemented */ > + goto read_as_zero; This register contains usefull bits for guest debugging. It would be nice to implemented in the future. > + case GICR_WAKER: > + /* Not implemented */ Please explain why it's not implemented. > + goto read_as_zero; > + case GICR_SETLPIR: > + case GICR_CLRLPIR: > + /* WO return fail */ > + return 0; Returning 0 here will inject a data abort exception to the guest. I don't think we want that. From the documentation (see 5.1.2, if you need more details I can explain you privately) read on write-only register should be read 0. There some other manner to handle it, but we don't care for now. > + case GICR_PROPBASER: > + /* LPI's not implemented */ > + goto read_as_zero; Hrmmm... you are read_as_zero is only able to handle 32-bits (see dabt.size != 2). But PROPBASER is a 64 bit register. > + case GICR_PENDBASER: > + /* LPI's not implemented */ > + goto read_as_zero; > + case GICR_INVLPIR: > + case GICR_INVALLR: > + /* WO return fail*/ Same remarks as GICR_SETLPIR... > + return 0; > + case GICR_SYNCR: > + if ( dabt.size != 2 ) goto bad_width; > + /* RO */ > + /* Return as not busy */ > + *r = GICR_SYNCR_NOT_BUSY; Please explain why this value. > + return 1; > + case GICR_MOVLPIR: > + case GICR_MOVALLR: > + /* WO */ Same remarks as GICR_SETLPIR. > + return 0; > + case GICR_PIDR7... GICR_PIDR0: > + *r = GICR_PIDR0_GICV3; Hrrmmm... no. PIDR1 should return 0xB (on ARM implementation)... > + return 1; > + default: > + printk("vGICR: read r%d offset %#08x\n not found", dabt.reg, offset); > + return 0; > + } > +bad_width: > + printk("vGICD: bad read width %d r%d offset %#08x\n", > + dabt.size, dabt.reg, offset); > + domain_crash_synchronous(); > + return 0; > + > +read_as_zero: > + if ( dabt.size != 2 ) goto bad_width; As said previously this function only works for 32 bit access. > + *r = 0; > + return 1; > +} > + > +static int __vgic_rdistr_mmio_write(struct vcpu *v, mmio_info_t *info, > + uint32_t offset) > +{ > + 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 gicr_reg = REG(offset); > + > + switch ( gicr_reg ) > + { > + case GICR_CTLR: > + /* We do not implement LPI's, read zero */ > + goto write_ignore; > + case GICR_IIDR: > + case GICR_TYPER: > + /* RO */ > + goto write_ignore; Why do you sometime gather multiple case, and sometimes not? I would prefer if you don't gather, as you do most of the times... > + case GICR_STATUSR: > + /* Not implemented */ > + goto write_ignore; > + case GICR_WAKER: > + /* Not implemented */ > + goto write_ignore; > + case GICR_SETLPIR: > + /* Not implemented */ > + goto write_ignore; > + case GICR_CLRLPIR: > + /* Not implemented */ > + goto write_ignore; For all the case before, can you explain in a comment why you didn't implement it? > + case GICR_PROPBASER: > + /* LPI is not implemented */ > + goto write_ignore; > + case GICR_PENDBASER: > + /* LPI is not implemented */ > + goto write_ignore; > + case GICR_INVLPIR: > + return 1; > + case GICR_INVALLR: > + return 1; > + case GICR_SYNCR: > + /* RO */ > + goto write_ignore; > + case GICR_MOVLPIR: > + return 1; > + case GICR_MOVALLR: > + return 1; > + case GICR_PIDR7... GICR_PIDR0: > + /* RO */ > + goto write_ignore; > + default: > + printk("vGICR: write r%d offset %#08x\n not found", dabt.reg, > offset); > + return 0; > + } > +bad_width: > + printk("vGICR: bad write width %d r%d=%"PRIregister" offset %#08x\n", > + dabt.size, dabt.reg, *r, offset); > + domain_crash_synchronous(); > + return 0; > + > +write_ignore: > + if ( dabt.size != 2 ) goto bad_width; This as the same issue as __vrdist_mmio_write. [..] > +static int vgic_rdistr_sgi_mmio_write(struct vcpu *v, mmio_info_t *info, > + uint32_t offset) > +{ This function seems to be a copy of vgic_distr_mmio_write (with some printk removed???? and s/GICD/GICR). Can you avoid duplicate code? It will be easier to maintain. [..] > + case GICR_ICENABLER0: > + if ( dabt.size != 2 ) goto bad_width; > + rank = vgic_irq_rank(v, 1, gicr_reg - GICR_ICENABLER0); > + if ( rank == NULL ) goto write_ignore; > + vgic_lock_rank(v, rank); > + rank->ienable &= ~*r; > + vgic_unlock_rank(v, rank); You should call vgic_disable_irqs here. > + return 1; > + case GICR_ISPENDR0: > + if ( dabt.size != 0 && dabt.size != 2 ) goto bad_width; printk.... > + return 0; > + case GICR_ICPENDR0: > + if ( dabt.size != 0 && dabt.size != 2 ) goto bad_width; printk... > + return 0; > + case GICR_ISACTIVER0: > + if ( dabt.size != 2 ) goto bad_width; > + rank = vgic_irq_rank(v, 1, gicr_reg - GICR_ISACTIVER0); > + if ( rank == NULL ) goto write_ignore; > + vgic_lock_rank(v, rank); > + rank->iactive &= ~*r; > + vgic_unlock_rank(v, rank); > + return 1; > + case GICR_ICACTIVER0: > + if ( dabt.size != 2 ) goto bad_width; > + rank = vgic_irq_rank(v, 1, gicr_reg - GICR_ICACTIVER0); > + if ( rank == NULL) goto write_ignore; > + vgic_lock_rank(v, rank); > + rank->iactive &= ~*r; > + vgic_unlock_rank(v, rank); > + return 1; > + case GICR_IPRIORITYR0...GICR_IPRIORITYR7: > + if ( dabt.size != 0 && dabt.size != 2 ) goto bad_width; > + rank = vgic_irq_rank(v, 8, gicr_reg - GICR_IPRIORITYR0); > + if ( rank == NULL) goto write_ignore; > + vgic_lock_rank(v, rank); > + if ( dabt.size == 2 ) > + rank->ipriority[REG_RANK_INDEX(8, gicr_reg - GICR_IPRIORITYR0)] > = *r; > + else > + byte_write(&rank->ipriority[REG_RANK_INDEX(8, gicr_reg - > GICR_IPRIORITYR0)], > + *r, offset); > + vgic_unlock_rank(v, rank); > + return 1; > + case GICR_ICFGR0: > + /* RO */ goto write_ignore; And please specify why it's read-only. > + case GICR_ICFGR1: > + goto write_ignore; Same remarks as GICR_ICFGR0. > +static int vgic_rdistr_mmio_read(struct vcpu *v, mmio_info_t *info) > +{ > + uint32_t offset; > + > + if ( !v->domain->arch.vgic.rdist_stride ) > + offset = info->gpa & (v->domain->arch.vgic.rdist_stride - 1); > + else > + offset = info->gpa & 0x1FFFF; > + > + if ( offset < SZ_64K ) > + return __vgic_rdistr_mmio_read(v, info, offset); > + else if ( (offset - SZ_64K) < SZ_64K ) > + return vgic_rdistr_sgi_mmio_read(v, info, (offset - SZ_64K)); > + else > + dprintk(XENLOG_WARNING, "vGICR: wrong gpa read address %"PRIpaddr"\n", > + info->gpa); gdprintk? > + return 0; > +} > + > +static int vgic_rdistr_mmio_write(struct vcpu *v, mmio_info_t *info) > +{ > + uint32_t offset; > + > + if ( !v->domain->arch.vgic.rdist_stride ) > + offset = info->gpa & (v->domain->arch.vgic.rdist_stride - 1); > + else > + offset = info->gpa & 0x1FFFF; > + > + if ( offset < SZ_64K ) > + return __vgic_rdistr_mmio_write(v, info, offset); > + else if ( (offset - SZ_64K) < SZ_64K ) > + return vgic_rdistr_sgi_mmio_write(v, info, (offset - SZ_64K)); > + else > + dprintk(XENLOG_WARNING, "vGICR: wrong gpa write %"PRIpaddr"\n", > + info->gpa); > + return 0; > +} > + > +static int vgic_distr_mmio_read(struct vcpu *v, mmio_info_t *info) > +{ > + struct hsr_dabt dabt = info->dabt; > + struct cpu_user_regs *regs = guest_cpu_user_regs(); > + register_t *r = select_user_reg(regs, dabt.reg); > + struct vgic_irq_rank *rank; > + int offset = (int)(info->gpa - v->domain->arch.vgic.dbase); > + int gicd_reg = REG(offset); > + Is it a copy of vgic v2 distr + few registers? > + switch ( gicd_reg ) > + { > + case GICD_CTLR: > + if ( dabt.size != 2 ) goto bad_width; > + vgic_lock(v); > + *r = v->domain->arch.vgic.ctlr; > + vgic_unlock(v); > + return 1; > + case GICD_TYPER: > + if ( dabt.size != 2 ) goto bad_width; > + /* No secure world support for guests. */ > + vgic_lock(v); > + *r = ( (v->domain->max_vcpus<<5) & GICD_TYPE_CPUS ) > + |( ((v->domain->arch.vgic.nr_lines/32)) & GICD_TYPE_LINES ); > + vgic_unlock(v); > + return 1; > + case GICD_STATUSR: > + /* Not implemented */ > + goto read_as_zero; > + case GICD_IIDR: > + if ( dabt.size != 2 ) goto bad_width; > + /* > + * XXX Do we need a JEP106 manufacturer ID? > + * Just use the physical h/w value for now > + */ > + *r = GICD_IIDR_VAL; > + return 1; > + /* Implementation defined -- read as zero */ > + case REG(0x020) ... REG(0x03c): Please move the comment after the case. Also can you give a name to this register? For clarity. [..] > + case GICD_PIDR7... GICD_PIDR0: > + /* GICv3 identification value */ > + *r = GICD_PIDR0_GICV3; Same as GICR_PIDR* in rdist read. This is wrong. > + return 1; > + case REG(0x00c): > + case REG(0x044): > + case REG(0x04c): > + case REG(0x05c) ... REG(0x07c): > + case REG(0xf30) ... REG(0x5fcc): > + case REG(0x8000) ... REG(0xbfcc): > + case REG(0xc000) ... REG(0xffcc): > + printk("vGICD: read unknown 0x00c .. 0xfcc r%d offset %#08x\n", > + dabt.reg, offset); What are those registers? > + goto read_as_zero; > + > + default: > + printk("vGICD: unhandled read r%d offset %#08x\n", > + dabt.reg, offset); > + return 0; > + } > + > +bad_width: > + dprintk(XENLOG_ERR, "vGICD: bad read width %d r%d offset %#08x\n", > + dabt.size, dabt.reg, offset); > + domain_crash_synchronous(); > + return 0; > + > +read_as_zero: > + printk("vGICD: read as zero %d r%d offset %#08x\n", > + dabt.size, dabt.reg, offset); You should remove this printk. It will call often. > + if ( dabt.size != 2 ) goto bad_width; > + *r = 0; > + return 1; > +} > + > +static int vgic_distr_mmio_write(struct vcpu *v, mmio_info_t *info) > +{ > + struct hsr_dabt dabt = info->dabt; > + struct cpu_user_regs *regs = guest_cpu_user_regs(); > + register_t *r = select_user_reg(regs, dabt.reg); > + struct vgic_irq_rank *rank; > + int offset = (int)(info->gpa - v->domain->arch.vgic.dbase); > + int gicd_reg = REG(offset); > + uint32_t tr; > + > + switch ( gicd_reg ) > + { > + case GICD_CTLR: > + if ( dabt.size != 2 ) goto bad_width; > + /* Ignore all but the enable bit */ > + v->domain->arch.vgic.ctlr = (*r) & GICD_CTL_ENABLE; > + return 1; > + > + /* R/O -- write ignored */ > + case GICD_TYPER: goto write_ignore. > + case GICD_IIDR: > + goto write_ignore; > + case GICD_STATUSR: > + goto write_ignore; > + case GICD_SETSPI_NSR: > + goto write_ignore; > + case GICD_CLRSPI_NSR: > + goto write_ignore; > + case GICD_SETSPI_SR: > + goto write_ignore; > + case GICD_CLRSPI_SR: > + goto write_ignore; > + /* Implementation defined -- write ignored */ > + case REG(0x020) ... REG(0x03c): > + printk("vGICD: write unknown 0x020 - 0x03c r%d offset %#08x\n", > + dabt.reg, offset); Please move the comment after the case. [..] > +static int vgic_vcpu_init(struct vcpu *v) > +{ > + int i; > + u64 affinity; > + > + /* For SGI and PPI the target is always this CPU */ > + affinity = cpu_logical_map(v->vcpu_id); vcpu_id != physical ID. > + for ( i = 0 ; i < 32 ; i++ ) > + v->arch.vgic.private_irqs->irouter[i] = affinity; > + return 0; > +} [..] > diff --git a/xen/include/asm-arm/gic_v3_defs.h > b/xen/include/asm-arm/gic_v3_defs.h > index 578832b..f7f7932 100644 > --- a/xen/include/asm-arm/gic_v3_defs.h > +++ b/xen/include/asm-arm/gic_v3_defs.h > @@ -94,6 +94,8 @@ > #define GICD_PIDR2_ARCH_MASK (0xf0) > #define GICR_PIDR2_ARCH_MASK GICD_PIDR2_ARCH_MASK > #define GICR_SYNCR_NOT_BUSY 1 > +#define GICD_IIDR_VAL 0x34c What does mean this value? How does Linux behave when this ID register is read? > +#define GICR_IIDR_VAL GICD_IIDR_VAL > > #define GICR_CTLR (0x0000) > #define GICR_IIDR (0x0004) > diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h > index 187846e..0de74eb 100644 > --- a/xen/include/asm-arm/vgic.h > +++ b/xen/include/asm-arm/vgic.h > @@ -25,7 +25,10 @@ struct vgic_irq_rank { > uint32_t ienable, iactive, ipend, pendsgi; > uint32_t icfg[2]; > uint32_t ipriority[8]; > - uint32_t itargets[8]; > + union { > + uint32_t itargets[8]; > + uint64_t irouter[32]; > + }; Can you add a comment why you use an union, and what VGIC version is using what? > }; > > struct vgic_ops { > @@ -51,6 +54,9 @@ static inline int REG_RANK_NR(int b, uint32_t n) > { > switch ( b ) > { > + case 64: return n >> 6; > + case 32: return n >> 5; > + case 16: return n >> 4; It doesn't sounds right to update this common function. What happen if VGIC v2 is trying to use this value? > case 8: return n >> 3; > case 4: return n >> 2; > case 2: return n >> 1; > @@ -89,6 +95,7 @@ void byte_write(uint32_t *reg, uint32_t var, int offset); > struct vgic_irq_rank *vgic_get_irqrank(struct vcpu *v, int rank); > extern void register_vgic_ops(struct vgic_ops *ops); > int vgic_v2_init(struct domain *d); > +int vgic_v3_init(struct domain *d); > > #endif > > 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 |