On 17 February 2017 at 06:31, <vijay.kil...@gmail.com> wrote: > From: Vijaya Kumar K <vijaya.ku...@cavium.com> > > Reset CPU interface registers of GICv3 when CPU is reset. > For this, ARMCPRegInfo struct is registered with one ICC > register whose resetfn is called when cpu is reset. > > All the ICC registers are reset under one single register > reset function instead of calling resetfn for each ICC > register. > > Signed-off-by: Vijaya Kumar K <vijaya.ku...@cavium.com> > --- > hw/intc/arm_gicv3_kvm.c | 58 > +++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 58 insertions(+) > > diff --git a/hw/intc/arm_gicv3_kvm.c b/hw/intc/arm_gicv3_kvm.c > index cda1af4..6377dc3 100644 > --- a/hw/intc/arm_gicv3_kvm.c > +++ b/hw/intc/arm_gicv3_kvm.c > @@ -604,6 +604,34 @@ static void kvm_arm_gicv3_get(GICv3State *s) > } > } > > +static void arm_gicv3_icc_reset(CPUARMState *env, const ARMCPRegInfo *ri) > +{ > + ARMCPU *cpu; > + GICv3State *s; > + GICv3CPUState *c; > + > + c = (GICv3CPUState *)env->gicv3state; > + assert(!(!c || !c->cpu || !c->gic));
I thought I'd made a comment about these asserts on a previous version of this code... they're pretty unnecessary since if any of them are untrue we'll just segfault in this function. The aim of an assert is to turn a hard-to-debug failure that only becomes visible late into an easy-to-debug failure that happens earlier. Assertions which don't move the failure significantly forward in time or turn an obscure problem into a clear one aren't really worth having. > + s = c->gic; > + cpu = ARM_CPU(c->cpu); > + > + /* Initialize to actual HW supported configuration */ > + kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_CPU_SYSREGS, > + KVM_VGIC_ATTR(ICC_CTLR_EL1, cpu->mp_affinity), > + &c->icc_ctlr_el1[GICV3_NS], false); > + > + c->icc_ctlr_el1[GICV3_S] = c->icc_ctlr_el1[GICV3_NS]; > + c->icc_pmr_el1 = 0; > + c->icc_bpr[GICV3_G0] = GIC_MIN_BPR; > + c->icc_bpr[GICV3_G1] = GIC_MIN_BPR; > + c->icc_bpr[GICV3_G1NS] = GIC_MIN_BPR; > + > + c->icc_sre_el1 = 0x7; > + memset(c->icc_apr, 0, sizeof(c->icc_apr)); > + memset(c->icc_igrpen, 0, sizeof(c->icc_igrpen)); > +} > + > static void kvm_arm_gicv3_reset(DeviceState *dev) > { > GICv3State *s = ARM_GICV3_COMMON(dev); > @@ -621,6 +649,30 @@ static void kvm_arm_gicv3_reset(DeviceState *dev) > kvm_arm_gicv3_put(s); > } > > +/* > + * CPU interface registers of GIC needs to be reset on CPU reset. > + * For the calling arm_gicv3_icc_reset() on CPU reset, we register > + * below ARMCPRegInfo. As we reset the whole cpu interface under single > + * register reset, we define only one register of CPU interface instead > + * of defining all the registers. > + */ > +static const ARMCPRegInfo gicv3_cpuif_reginfo[] = { > + { .name = "ICC_CTLR_EL1", .state = ARM_CP_STATE_BOTH, > + .opc0 = 3, .opc1 = 0, .crn = 12, .crm = 12, .opc2 = 4, > + .type = ARM_CP_NO_RAW, > + .access = PL1_RW, > + .readfn = arm_cp_read_zero, > + .writefn = arm_cp_write_ignore, > + /* > + * We hang the whole cpu interface reset routine off here > + * rather than parcelling it out into one little function > + * per register > + */ > + .resetfn = arm_gicv3_icc_reset, > + }, > + REGINFO_SENTINEL I asked for a comment saying why we can't use ARM_CP_NOP... thanks -- PMM