diff mbox series

aarch64: advertise the GIC system register interface

Message ID alpine.DEB.2.10.1710171708130.27209@sstabellini-ThinkPad-X260
State New
Headers show
Series aarch64: advertise the GIC system register interface | expand

Commit Message

Stefano Stabellini Oct. 18, 2017, 12:10 a.m. UTC 
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>

Comments

Peter Maydell Oct. 19, 2017, 2:46 p.m. UTC | #1 
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
Peter Maydell Oct. 31, 2017, 11:47 a.m. UTC | #2 
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
Stefano Stabellini Oct. 31, 2017, 5:01 p.m. UTC | #3 
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; ?
Peter Maydell Oct. 31, 2017, 5:10 p.m. UTC | #4 
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
Stefano Stabellini Oct. 31, 2017, 6:51 p.m. UTC | #5 
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);
Peter Maydell Oct. 31, 2017, 7:24 p.m. UTC | #6 
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
Stefano Stabellini Nov. 1, 2017, 2:09 a.m. UTC | #7 
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 mbox series

Patch

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 */