On Tue, 7 Nov 2017, Peter Maydell wrote: > The CPU ID registers ID_AA64PFR0_EL1, ID_PFR1_EL1 and ID_PFR1 > have a field for reporting presence of GICv3 system registers. > We need to report this field correctly in order for Xen to > work as a guest inside QEMU emulation. We mustn't incorrectly > claim the sysregs exist when they don't, though, or Linux will > crash. > > Unfortunately the way we've designed the GICv3 emulation in QEMU > puts the system registers as part of the GICv3 device, which > may be created after the CPU proper has been realized. This > means that we don't know at the point when we define the ID > registers what the correct value is. Handle this by switching > them to calling a function at runtime to read the value, where > we can fill in the GIC field appropriately. > > Signed-off-by: Peter Maydell <[email protected]>
Tested-by: Stefano Stabellini <[email protected]> > --- > In retrospect I think having the sysregs emulation in the > GIC device was a bit of a design error -- we should have > split it like the hardware does, with a defined protocol > between the GIC and the CPU interface. (In real hardware the > CPU can have the GIC system registers even though it's not > connected to an actual GICv3, and we don't/can't emulate > that with our current design.) > --- > target/arm/helper.c | 44 ++++++++++++++++++++++++++++++++++++++++---- > 1 file changed, 40 insertions(+), 4 deletions(-) > > diff --git a/target/arm/helper.c b/target/arm/helper.c > index f61fb3e..35c5bd6 100644 > --- a/target/arm/helper.c > +++ b/target/arm/helper.c > @@ -4549,6 +4549,33 @@ static void define_debug_regs(ARMCPU *cpu) > } > } > > +/* We don't know until after realize whether there's a GICv3 > + * attached, and that is what registers the gicv3 sysregs. > + * So we have to fill in the GIC fields in ID_PFR/ID_PFR1_EL1/ID_AA64PFR0_EL1 > + * at runtime. > + */ > +static uint64_t id_pfr1_read(CPUARMState *env, const ARMCPRegInfo *ri) > +{ > + ARMCPU *cpu = arm_env_get_cpu(env); > + uint64_t pfr1 = cpu->id_pfr1; > + > + if (env->gicv3state) { > + pfr1 |= 1 << 28; > + } > + return pfr1; > +} > + > +static uint64_t id_aa64pfr0_read(CPUARMState *env, const ARMCPRegInfo *ri) > +{ > + ARMCPU *cpu = arm_env_get_cpu(env); > + uint64_t pfr0 = cpu->id_aa64pfr0; > + > + if (env->gicv3state) { > + pfr0 |= 1 << 24; > + } > + return pfr0; > +} > + > void register_cp_regs_for_features(ARMCPU *cpu) > { > /* Register all the coprocessor registers based on feature bits */ > @@ -4573,10 +4600,14 @@ void register_cp_regs_for_features(ARMCPU *cpu) > .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 1, .opc2 = 0, > .access = PL1_R, .type = ARM_CP_CONST, > .resetvalue = cpu->id_pfr0 }, > + /* ID_PFR1 is not a plain ARM_CP_CONST because we don't know > + * the value of the GIC field until after we define these regs. > + */ > { .name = "ID_PFR1", .state = ARM_CP_STATE_BOTH, > .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 1, .opc2 = 1, > - .access = PL1_R, .type = ARM_CP_CONST, > - .resetvalue = cpu->id_pfr1 }, > + .access = PL1_R, .type = ARM_CP_NO_RAW, > + .readfn = id_pfr1_read, > + .writefn = arm_cp_write_ignore }, > { .name = "ID_DFR0", .state = ARM_CP_STATE_BOTH, > .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 1, .opc2 = 2, > .access = PL1_R, .type = ARM_CP_CONST, > @@ -4692,10 +4723,15 @@ void register_cp_regs_for_features(ARMCPU *cpu) > * define new registers here. > */ > ARMCPRegInfo v8_idregs[] = { > + /* ID_AA64PFR0_EL1 is not a plain ARM_CP_CONST because we don't > + * know the right value for the GIC field until after we > + * define these regs. > + */ > { .name = "ID_AA64PFR0_EL1", .state = ARM_CP_STATE_AA64, > .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 4, .opc2 = 0, > - .access = PL1_R, .type = ARM_CP_CONST, > - .resetvalue = cpu->id_aa64pfr0 }, > + .access = PL1_R, .type = ARM_CP_NO_RAW, > + .readfn = id_aa64pfr0_read, > + .writefn = arm_cp_write_ignore }, > { .name = "ID_AA64PFR1_EL1", .state = ARM_CP_STATE_AA64, > .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 4, .opc2 = 1, > .access = PL1_R, .type = ARM_CP_CONST, > -- > 2.7.4 >
