Message ID | alpine.DEB.2.10.1710171708130.27209@sstabellini-ThinkPad-X260 |
---|---|
State | New |
Headers | show |
Series | aarch64: advertise the GIC system register interface | expand |
On 18 October 2017 at 01:10, Stefano Stabellini <sstabellini@kernel.org> wrote: > Advertise the presence of the GIC system register interface (1<<24) > according to H9.248 of the ARM ARM. > > This patch allows Xen to boot on QEMU aarch64. > > Signed-off-by: Stefano Stabellini <sstabellini@kernel.org> > > diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c > index 670c07a..a451763 100644 > --- a/target/arm/cpu64.c > +++ b/target/arm/cpu64.c > @@ -136,7 +136,7 @@ static void aarch64_a57_initfn(Object *obj) > cpu->id_isar3 = 0x01112131; > cpu->id_isar4 = 0x00011142; > cpu->id_isar5 = 0x00011121; > - cpu->id_aa64pfr0 = 0x00002222; > + cpu->id_aa64pfr0 = 0x01002222; > cpu->id_aa64dfr0 = 0x10305106; > cpu->pmceid0 = 0x00000000; > cpu->pmceid1 = 0x00000000; > @@ -196,7 +196,7 @@ static void aarch64_a53_initfn(Object *obj) > cpu->id_isar3 = 0x01112131; > cpu->id_isar4 = 0x00011142; > cpu->id_isar5 = 0x00011121; > - cpu->id_aa64pfr0 = 0x00002222; > + cpu->id_aa64pfr0 = 0x01002222; > cpu->id_aa64dfr0 = 0x10305106; > cpu->id_aa64isar0 = 0x00011120; > cpu->id_aa64mmfr0 = 0x00001122; /* 40 bit physical addr */ Whoops -- we missed this when we added the GICv3 support, because Linux doesn't check it. Applied to target-arm.next, thanks. -- PMM
On 19 October 2017 at 15:46, Peter Maydell <peter.maydell@linaro.org> wrote: > On 18 October 2017 at 01:10, Stefano Stabellini <sstabellini@kernel.org> wrote: >> Advertise the presence of the GIC system register interface (1<<24) >> according to H9.248 of the ARM ARM. >> >> This patch allows Xen to boot on QEMU aarch64. >> >> Signed-off-by: Stefano Stabellini <sstabellini@kernel.org> >> >> diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c >> index 670c07a..a451763 100644 >> --- a/target/arm/cpu64.c >> +++ b/target/arm/cpu64.c >> @@ -136,7 +136,7 @@ static void aarch64_a57_initfn(Object *obj) >> cpu->id_isar3 = 0x01112131; >> cpu->id_isar4 = 0x00011142; >> cpu->id_isar5 = 0x00011121; >> - cpu->id_aa64pfr0 = 0x00002222; >> + cpu->id_aa64pfr0 = 0x01002222; >> cpu->id_aa64dfr0 = 0x10305106; >> cpu->pmceid0 = 0x00000000; >> cpu->pmceid1 = 0x00000000; >> @@ -196,7 +196,7 @@ static void aarch64_a53_initfn(Object *obj) >> cpu->id_isar3 = 0x01112131; >> cpu->id_isar4 = 0x00011142; >> cpu->id_isar5 = 0x00011121; >> - cpu->id_aa64pfr0 = 0x00002222; >> + cpu->id_aa64pfr0 = 0x01002222; >> cpu->id_aa64dfr0 = 0x10305106; >> cpu->id_aa64isar0 = 0x00011120; >> cpu->id_aa64mmfr0 = 0x00001122; /* 40 bit physical addr */ > > Whoops -- we missed this when we added the GICv3 support, because > Linux doesn't check it. > > Applied to target-arm.next, thanks. Unfortunately I've just noticed that this breaks booting Linux in the "not using gicv3" case. This is because we don't actually define the GICv3 cpu interface registers unless the board instantiates a GICv3. We mustn't advertise the GICv3 sysregs in the ID register unless we actually have them. Not sure how best to fix this -- it's a consequence of the design decision we made to have the sysregs implementation be in the gicv3 code. There's no useful feature bit in the CPU to hang this off either, it's an effect of whether the board model happens to wire up a gicv3 or not. So we can only figure this out fairly late on (probably at CPU reset time), which is a bit tedious for getting ID reg values right. In the meantime I've dropped this patch from target-arm.next. thanks -- PMM
On Tue, 31 Oct 2017, Peter Maydell wrote: > On 19 October 2017 at 15:46, Peter Maydell <peter.maydell@linaro.org> wrote: > > On 18 October 2017 at 01:10, Stefano Stabellini <sstabellini@kernel.org> wrote: > >> Advertise the presence of the GIC system register interface (1<<24) > >> according to H9.248 of the ARM ARM. > >> > >> This patch allows Xen to boot on QEMU aarch64. > >> > >> Signed-off-by: Stefano Stabellini <sstabellini@kernel.org> > >> > >> diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c > >> index 670c07a..a451763 100644 > >> --- a/target/arm/cpu64.c > >> +++ b/target/arm/cpu64.c > >> @@ -136,7 +136,7 @@ static void aarch64_a57_initfn(Object *obj) > >> cpu->id_isar3 = 0x01112131; > >> cpu->id_isar4 = 0x00011142; > >> cpu->id_isar5 = 0x00011121; > >> - cpu->id_aa64pfr0 = 0x00002222; > >> + cpu->id_aa64pfr0 = 0x01002222; > >> cpu->id_aa64dfr0 = 0x10305106; > >> cpu->pmceid0 = 0x00000000; > >> cpu->pmceid1 = 0x00000000; > >> @@ -196,7 +196,7 @@ static void aarch64_a53_initfn(Object *obj) > >> cpu->id_isar3 = 0x01112131; > >> cpu->id_isar4 = 0x00011142; > >> cpu->id_isar5 = 0x00011121; > >> - cpu->id_aa64pfr0 = 0x00002222; > >> + cpu->id_aa64pfr0 = 0x01002222; > >> cpu->id_aa64dfr0 = 0x10305106; > >> cpu->id_aa64isar0 = 0x00011120; > >> cpu->id_aa64mmfr0 = 0x00001122; /* 40 bit physical addr */ > > > > Whoops -- we missed this when we added the GICv3 support, because > > Linux doesn't check it. > > > > Applied to target-arm.next, thanks. > > Unfortunately I've just noticed that this breaks booting Linux > in the "not using gicv3" case. This is because we don't actually > define the GICv3 cpu interface registers unless the board > instantiates a GICv3. We mustn't advertise the GICv3 sysregs > in the ID register unless we actually have them. > > Not sure how best to fix this -- it's a consequence of the > design decision we made to have the sysregs implementation > be in the gicv3 code. There's no useful feature bit in the CPU > to hang this off either, it's an effect of whether the board > model happens to wire up a gicv3 or not. So we can only figure > this out fairly late on (probably at CPU reset time), which > is a bit tedious for getting ID reg values right. > > In the meantime I've dropped this patch from target-arm.next. Fixing QEMU is harder than I expected. Would it be possible to update id_aa64pfr0 at CPU reset time? Like cpu->id_aa64pfr0 |= 0x01000000; ?
On 31 October 2017 at 17:01, Stefano Stabellini <sstabellini@kernel.org> wrote: > Fixing QEMU is harder than I expected. Would it be possible to update > id_aa64pfr0 at CPU reset time? Like cpu->id_aa64pfr0 |= 0x01000000; ? At that point we've already called register_cp_regs_for_features(), which is where we read cpu->id_aa64pfr0 when we're creating the cpreg. So if you change it after that it's too late. But that function is called at CPU realize time, which is before we've created the GIC object... thanks -- PMM
On Tue, 31 Oct 2017, Peter Maydell wrote: > On 31 October 2017 at 17:01, Stefano Stabellini <sstabellini@kernel.org> wrote: > > Fixing QEMU is harder than I expected. Would it be possible to update > > id_aa64pfr0 at CPU reset time? Like cpu->id_aa64pfr0 |= 0x01000000; ? > > At that point we've already called register_cp_regs_for_features(), > which is where we read cpu->id_aa64pfr0 when we're creating the > cpreg. So if you change it after that it's too late. But that > function is called at CPU realize time, which is before we've > created the GIC object... What about something along the lines of diff --git a/hw/arm/virt.c b/hw/arm/virt.c index 9e18b41..0851071 100644 --- a/hw/arm/virt.c +++ b/hw/arm/virt.c @@ -1401,6 +1400,10 @@ static void machvirt_init(MachineState *machine) object_property_set_link(cpuobj, OBJECT(secure_sysmem), "secure-memory", &error_abort); } + if (vms->gic_version == 3) { + ARMCPU *cpu = ARM_CPU(cpuobj); + cpu->id_aa64pfr0 |= 0x01000000; + } object_property_set_bool(cpuobj, true, "realized", NULL); object_unref(cpuobj);
On 31 October 2017 at 18:51, Stefano Stabellini <sstabellini@kernel.org> wrote: > On Tue, 31 Oct 2017, Peter Maydell wrote: >> On 31 October 2017 at 17:01, Stefano Stabellini <sstabellini@kernel.org> wrote: >> > Fixing QEMU is harder than I expected. Would it be possible to update >> > id_aa64pfr0 at CPU reset time? Like cpu->id_aa64pfr0 |= 0x01000000; ? >> >> At that point we've already called register_cp_regs_for_features(), >> which is where we read cpu->id_aa64pfr0 when we're creating the >> cpreg. So if you change it after that it's too late. But that >> function is called at CPU realize time, which is before we've >> created the GIC object... > > What about something along the lines of > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c > index 9e18b41..0851071 100644 > --- a/hw/arm/virt.c > +++ b/hw/arm/virt.c > @@ -1401,6 +1400,10 @@ static void machvirt_init(MachineState *machine) > object_property_set_link(cpuobj, OBJECT(secure_sysmem), > "secure-memory", &error_abort); > } > + if (vms->gic_version == 3) { > + ARMCPU *cpu = ARM_CPU(cpuobj); > + cpu->id_aa64pfr0 |= 0x01000000; > + } > > object_property_set_bool(cpuobj, true, "realized", NULL); > object_unref(cpuobj); Definitely not -- the virt board should not be poking about inside the internals of the CPU object. The slightly cleaned up version of this idea is that you give the CPU an object property for "claim the GICv3 registers exist in the ID registers" and have virt.c set it. That feels like an ugly workaround for something we really ought not to have to tell the board about at all, though. thanks -- PMM
On Tue, 31 Oct 2017, Peter Maydell wrote: > On 31 October 2017 at 18:51, Stefano Stabellini <sstabellini@kernel.org> wrote: > > On Tue, 31 Oct 2017, Peter Maydell wrote: > >> On 31 October 2017 at 17:01, Stefano Stabellini <sstabellini@kernel.org> wrote: > >> > Fixing QEMU is harder than I expected. Would it be possible to update > >> > id_aa64pfr0 at CPU reset time? Like cpu->id_aa64pfr0 |= 0x01000000; ? > >> > >> At that point we've already called register_cp_regs_for_features(), > >> which is where we read cpu->id_aa64pfr0 when we're creating the > >> cpreg. So if you change it after that it's too late. But that > >> function is called at CPU realize time, which is before we've > >> created the GIC object... > > > > What about something along the lines of > > > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c > > index 9e18b41..0851071 100644 > > --- a/hw/arm/virt.c > > +++ b/hw/arm/virt.c > > @@ -1401,6 +1400,10 @@ static void machvirt_init(MachineState *machine) > > object_property_set_link(cpuobj, OBJECT(secure_sysmem), > > "secure-memory", &error_abort); > > } > > + if (vms->gic_version == 3) { > > + ARMCPU *cpu = ARM_CPU(cpuobj); > > + cpu->id_aa64pfr0 |= 0x01000000; > > + } > > > > object_property_set_bool(cpuobj, true, "realized", NULL); > > object_unref(cpuobj); > > Definitely not -- the virt board should not be poking about inside the > internals of the CPU object. > > The slightly cleaned up version of this idea is that you give the > CPU an object property for "claim the GICv3 registers exist in the > ID registers" and have virt.c set it. That feels like an ugly > workaround for something we really ought not to have to tell the > board about at all, though. Something like the following? diff --git a/hw/arm/virt.c b/hw/arm/virt.c index 9e18b41..369d36b 100644 --- a/hw/arm/virt.c +++ b/hw/arm/virt.c @@ -1401,6 +1401,9 @@ static void machvirt_init(MachineState *machine) object_property_set_link(cpuobj, OBJECT(secure_sysmem), "secure-memory", &error_abort); } + if (vms->gic_version == 3) { + object_property_set_bool(cpuobj, true, "gicv3-sysregs", NULL); + } object_property_set_bool(cpuobj, true, "realized", NULL); object_unref(cpuobj); diff --git a/target/arm/cpu.c b/target/arm/cpu.c index 88578f3..259cad1 100644 --- a/target/arm/cpu.c +++ b/target/arm/cpu.c @@ -1690,6 +1690,7 @@ static Property arm_cpu_properties[] = { DEFINE_PROP_UINT64("mp-affinity", ARMCPU, mp_affinity, ARM64_AFFINITY_INVALID), DEFINE_PROP_INT32("node-id", ARMCPU, node_id, CPU_UNSET_NUMA_NODE_ID), + DEFINE_PROP_BOOL("gicv3-sysregs", ARMCPU, gicv3_sysregs, false), DEFINE_PROP_END_OF_LIST() }; diff --git a/target/arm/cpu.h b/target/arm/cpu.h index 89d49cd..0015b37 100644 --- a/target/arm/cpu.h +++ b/target/arm/cpu.h @@ -657,6 +657,9 @@ struct ARMCPU { /* Should CPU start in PSCI powered-off state? */ bool start_powered_off; + /* GICv3 sysregs present */ + bool gicv3_sysregs; + /* Current power state, access guarded by BQL */ ARMPSCIState power_state; diff --git a/target/arm/helper.c b/target/arm/helper.c index 37af750..6f21900 100644 --- a/target/arm/helper.c +++ b/target/arm/helper.c @@ -4687,7 +4687,8 @@ void register_cp_regs_for_features(ARMCPU *cpu) { .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 }, + .resetvalue = cpu->gicv3_sysregs ? (cpu->id_aa64pfr0|0x01000000) : + cpu->id_aa64pfr0 }, { .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,
diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c index 670c07a..a451763 100644 --- a/target/arm/cpu64.c +++ b/target/arm/cpu64.c @@ -136,7 +136,7 @@ static void aarch64_a57_initfn(Object *obj) cpu->id_isar3 = 0x01112131; cpu->id_isar4 = 0x00011142; cpu->id_isar5 = 0x00011121; - cpu->id_aa64pfr0 = 0x00002222; + cpu->id_aa64pfr0 = 0x01002222; cpu->id_aa64dfr0 = 0x10305106; cpu->pmceid0 = 0x00000000; cpu->pmceid1 = 0x00000000; @@ -196,7 +196,7 @@ static void aarch64_a53_initfn(Object *obj) cpu->id_isar3 = 0x01112131; cpu->id_isar4 = 0x00011142; cpu->id_isar5 = 0x00011121; - cpu->id_aa64pfr0 = 0x00002222; + cpu->id_aa64pfr0 = 0x01002222; cpu->id_aa64dfr0 = 0x10305106; cpu->id_aa64isar0 = 0x00011120; cpu->id_aa64mmfr0 = 0x00001122; /* 40 bit physical addr */
Advertise the presence of the GIC system register interface (1<<24) according to H9.248 of the ARM ARM. This patch allows Xen to boot on QEMU aarch64. Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>