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 <peter.maydell@linaro.org>
---
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
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 <peter.maydell@linaro.org>
Tested-by: Stefano Stabellini <sstabellini@kernel.org>
> ---
> 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
>
On 11/07/2017 04:01 PM, 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 <peter.maydell@linaro.org> > --- Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
On Tue, Nov 7, 2017 at 7:01 AM, Peter Maydell <peter.maydell@linaro.org> 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 <peter.maydell@linaro.org>
Is this going to make it into 2.11?
Alistair
> ---
> 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
>
>
On 15 November 2017 at 01:14, Alistair Francis <alistair23@gmail.com> wrote: > On Tue, Nov 7, 2017 at 7:01 AM, Peter Maydell <peter.maydell@linaro.org> 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 <peter.maydell@linaro.org> > > Is this going to make it into 2.11? Yes, it's supposed to go into 2.11 -- I think I just forgot to put it in the rc1 pullreq, but it'll go in rc2. thanks -- PMM
On Wed, Nov 15, 2017 at 3:57 AM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 15 November 2017 at 01:14, Alistair Francis <alistair23@gmail.com> wrote: >> On Tue, Nov 7, 2017 at 7:01 AM, Peter Maydell <peter.maydell@linaro.org> 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 <peter.maydell@linaro.org> >> >> Is this going to make it into 2.11? > > Yes, it's supposed to go into 2.11 -- I think I just forgot > to put it in the rc1 pullreq, but it'll go in rc2. Great! Thanks Peter Alistair > > thanks > -- PMM
© 2016 - 2024 Red Hat, Inc.