[RFC PATCH 0/3] target/arm: move common aarch64 init to helpers and make cpu max standalone

[RFC PATCH 0/3] target/arm: move common aarch64 init to helpers and make cpu max standalone

[Leif Lindholm]

As a follow-up to the thread that started with
https://lists.gnu.org/archive/html/qemu-devel/2020-06/msg01018.html

Most of the a57/a53/a72 init functions is currently identical lines,
with some individual options set up differently.

Some of that duplication can be considered to be likely to apply to
any (or at least the majoroty of) aarch64 processors to be added in
the future.

And finally, the "max" cpu init function currently calls the a57 one,
with some non-obvious side effects including:
- cpu dtb_compatible string set to "arm,cortex-a57"
- kvm_target set (to A57 value) in tcg mode
- id_mmfr0 containing a value present in real A57 cpus of the revision
  qemu emulates, but banned by the architecture specification
- emulating implementation-defined cortex cp registers

This series:
- Creates a common initialization function for all aarch64 cpus, which
  also sets up the MIDR.
- Creates a common initialization function for the cortex cpus defined
  in cpu64.h.
- Detaches the "max" cpu from the A57 init, making use of both of the
  above helper functions.
- Can definitely be split up into additional patches (especially 3/3).

Leif Lindholm (3):
  target/arm: commonalize aarch64 cpu init
  target/arm: move cpu64 cortex processor common init settings to
    function
  target/arm: use cortex...common_init for cpu64 max

 target/arm/cpu-qom.h |   3 +
 target/arm/cpu64.c   | 155 ++++++++++++++++++-------------------------
 2 files changed, 66 insertions(+), 92 deletions(-)

-- 
2.20.1

[RFC PATCH 1/3] target/arm: commonalize aarch64 cpu init

[Leif Lindholm]

Some basic options will be set by all aarch64 platforms.
Break those out into a separate aarch64_cpu_common_init function, which
also takes implementer, partnum, variant, and revision as arguments to
set up MIDR.

Invoke this to remove duplication between a57/a53/a72 init.

Signed-off-by: Leif Lindholm <leif@nuviainc.com>
---
 target/arm/cpu-qom.h |  3 +++
 target/arm/cpu64.c   | 46 ++++++++++++++++++++++++--------------------
 2 files changed, 28 insertions(+), 21 deletions(-)

diff --git a/target/arm/cpu-qom.h b/target/arm/cpu-qom.h
index 56395b87f6..48f6303308 100644
--- a/target/arm/cpu-qom.h
+++ b/target/arm/cpu-qom.h
@@ -44,6 +44,9 @@ typedef struct ARMCPUInfo {
 void arm_cpu_register(const ARMCPUInfo *info);
 void aarch64_cpu_register(const ARMCPUInfo *info);
 
+void aarch64_cpu_common_init(Object *obj, uint8_t impl, uint16_t part,
+                             uint8_t var, uint8_t rev);
+
 /**
  * ARMCPUClass:
  * @parent_realize: The parent class' realize handler.
diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
index cbc5c3868f..79786e034f 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
@@ -29,6 +29,8 @@
 #include "kvm_arm.h"
 #include "qapi/visitor.h"
 
+#define MIDR_IMPLEMENTER_ARM 0x41
+
 #ifndef CONFIG_USER_ONLY
 static uint64_t a57_a53_l2ctlr_read(CPUARMState *env, const ARMCPRegInfo *ri)
 {
@@ -86,11 +88,19 @@ static const ARMCPRegInfo cortex_a72_a57_a53_cp_reginfo[] = {
     REGINFO_SENTINEL
 };
 
-static void aarch64_a57_initfn(Object *obj)
+void aarch64_cpu_common_init(Object *obj, uint8_t impl, uint16_t part,
+                             uint8_t var, uint8_t rev)
 {
     ARMCPU *cpu = ARM_CPU(obj);
+    uint64_t t;
+
+    t = FIELD_DP64(0, MIDR_EL1, IMPLEMENTER, impl);
+    t = FIELD_DP64(t, MIDR_EL1, ARCHITECTURE, 0xf);
+    t = FIELD_DP64(t, MIDR_EL1, PARTNUM, part);
+    t = FIELD_DP64(t, MIDR_EL1, VARIANT, var);
+    t = FIELD_DP64(t, MIDR_EL1, REVISION, rev);
+    cpu->midr = t;
 
-    cpu->dtb_compatible = "arm,cortex-a57";
     set_feature(&cpu->env, ARM_FEATURE_V8);
     set_feature(&cpu->env, ARM_FEATURE_NEON);
     set_feature(&cpu->env, ARM_FEATURE_GENERIC_TIMER);
@@ -99,8 +109,16 @@ static void aarch64_a57_initfn(Object *obj)
     set_feature(&cpu->env, ARM_FEATURE_EL2);
     set_feature(&cpu->env, ARM_FEATURE_EL3);
     set_feature(&cpu->env, ARM_FEATURE_PMU);
+}
+
+static void aarch64_a57_initfn(Object *obj)
+{
+    ARMCPU *cpu = ARM_CPU(obj);
+
+    aarch64_cpu_common_init(obj, MIDR_IMPLEMENTER_ARM, 0xd07, 1, 0);
+
+    cpu->dtb_compatible = "arm,cortex-a57";
     cpu->kvm_target = QEMU_KVM_ARM_TARGET_CORTEX_A57;
-    cpu->midr = 0x411fd070;
     cpu->revidr = 0x00000000;
     cpu->reset_fpsid = 0x41034070;
     cpu->isar.mvfr0 = 0x10110222;
@@ -143,17 +161,10 @@ static void aarch64_a53_initfn(Object *obj)
 {
     ARMCPU *cpu = ARM_CPU(obj);
 
+    aarch64_cpu_common_init(obj, MIDR_IMPLEMENTER_ARM, 0xd03, 0, 4);
+
     cpu->dtb_compatible = "arm,cortex-a53";
-    set_feature(&cpu->env, ARM_FEATURE_V8);
-    set_feature(&cpu->env, ARM_FEATURE_NEON);
-    set_feature(&cpu->env, ARM_FEATURE_GENERIC_TIMER);
-    set_feature(&cpu->env, ARM_FEATURE_AARCH64);
-    set_feature(&cpu->env, ARM_FEATURE_CBAR_RO);
-    set_feature(&cpu->env, ARM_FEATURE_EL2);
-    set_feature(&cpu->env, ARM_FEATURE_EL3);
-    set_feature(&cpu->env, ARM_FEATURE_PMU);
     cpu->kvm_target = QEMU_KVM_ARM_TARGET_CORTEX_A53;
-    cpu->midr = 0x410fd034;
     cpu->revidr = 0x00000000;
     cpu->reset_fpsid = 0x41034070;
     cpu->isar.mvfr0 = 0x10110222;
@@ -196,16 +207,9 @@ static void aarch64_a72_initfn(Object *obj)
 {
     ARMCPU *cpu = ARM_CPU(obj);
 
+    aarch64_cpu_common_init(obj, MIDR_IMPLEMENTER_ARM, 0xd08, 0, 3);
+
     cpu->dtb_compatible = "arm,cortex-a72";
-    set_feature(&cpu->env, ARM_FEATURE_V8);
-    set_feature(&cpu->env, ARM_FEATURE_NEON);
-    set_feature(&cpu->env, ARM_FEATURE_GENERIC_TIMER);
-    set_feature(&cpu->env, ARM_FEATURE_AARCH64);
-    set_feature(&cpu->env, ARM_FEATURE_CBAR_RO);
-    set_feature(&cpu->env, ARM_FEATURE_EL2);
-    set_feature(&cpu->env, ARM_FEATURE_EL3);
-    set_feature(&cpu->env, ARM_FEATURE_PMU);
-    cpu->midr = 0x410fd083;
     cpu->revidr = 0x00000000;
     cpu->reset_fpsid = 0x41034080;
     cpu->isar.mvfr0 = 0x10110222;
-- 
2.20.1

Re: [RFC PATCH 1/3] target/arm: commonalize aarch64 cpu init

[Peter Maydell]

On Mon, 8 Jun 2020 at 12:40, Leif Lindholm <leif@nuviainc.com> wrote:
>
> Some basic options will be set by all aarch64 platforms.
> Break those out into a separate aarch64_cpu_common_init function, which
> also takes implementer, partnum, variant, and revision as arguments to
> set up MIDR.
>
> Invoke this to remove duplication between a57/a53/a72 init.
>
> Signed-off-by: Leif Lindholm <leif@nuviainc.com>

I'm afraid I disagree with this patch's approach. All
these three CPUs are different implementations, and it's
just coincidence that they happen to set a lot of the
same feature flags.

Eventually the hope is that the feature flag checks will
be replaced with direct tests of the ID register bits,
at which point the feature flag settings will mostly
just go away.

thanks
-- PMM

[RFC PATCH 2/3] target/arm: move cpu64 cortex processor common init settings to function

[Leif Lindholm]

Move the id register initializations identical between the platforms in
this file into a standalone helper function, and change the cpu-specific

The value of mmfr0 set for a57 and a53 violates the ARM architecture
reference manual, but matches the values set in actual hardware r1p0 a57
and r0p4 a53. The function sets the architectually correct value, and the
a57/a53 init functions override it after the fact.

Signed-off-by: Leif Lindholm <leif@nuviainc.com>
---
 target/arm/cpu64.c | 107 ++++++++++++++-------------------------------
 1 file changed, 34 insertions(+), 73 deletions(-)

diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
index 79786e034f..9927c1f75d 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
@@ -88,6 +88,35 @@ static const ARMCPRegInfo cortex_a72_a57_a53_cp_reginfo[] = {
     REGINFO_SENTINEL
 };
 
+static void cortex_a72_a57_a53_common_init(ARMCPU *cpu)
+{
+    cpu->revidr = 0x00000000;
+    cpu->isar.mvfr0 = 0x10110222;
+    cpu->isar.mvfr1 = 0x12111111;
+    cpu->isar.mvfr2 = 0x00000043;
+    cpu->reset_sctlr = 0x00c50838;
+    cpu->id_pfr0 = 0x00000131;
+    cpu->id_pfr1 = 0x00011011;
+    cpu->isar.id_dfr0 = 0x03010066;
+    cpu->id_afr0 = 0x00000000;
+    cpu->isar.id_mmfr0 = 0x10201105;
+    cpu->isar.id_mmfr1 = 0x40000000;
+    cpu->isar.id_mmfr2 = 0x01260000;
+    cpu->isar.id_mmfr3 = 0x02102211;
+    cpu->isar.id_isar0 = 0x02101110;
+    cpu->isar.id_isar1 = 0x13112111;
+    cpu->isar.id_isar2 = 0x21232042;
+    cpu->isar.id_isar3 = 0x01112131;
+    cpu->isar.id_isar4 = 0x00011142;
+    cpu->isar.id_isar5 = 0x00011121;
+    cpu->isar.id_isar6 = 0;
+    cpu->isar.id_aa64pfr0 = 0x00002222;
+    cpu->isar.id_aa64dfr0 = 0x10305106;
+    cpu->isar.id_aa64isar0 = 0x00011120;
+    cpu->isar.id_aa64mmfr0 = 0x00001124;
+    cpu->isar.dbgdidr = 0x3516d000;
+}
+
 void aarch64_cpu_common_init(Object *obj, uint8_t impl, uint16_t part,
                              uint8_t var, uint8_t rev)
 {
@@ -116,36 +145,13 @@ static void aarch64_a57_initfn(Object *obj)
     ARMCPU *cpu = ARM_CPU(obj);
 
     aarch64_cpu_common_init(obj, MIDR_IMPLEMENTER_ARM, 0xd07, 1, 0);
+    cortex_a72_a57_a53_common_init(cpu);
 
     cpu->dtb_compatible = "arm,cortex-a57";
     cpu->kvm_target = QEMU_KVM_ARM_TARGET_CORTEX_A57;
-    cpu->revidr = 0x00000000;
     cpu->reset_fpsid = 0x41034070;
-    cpu->isar.mvfr0 = 0x10110222;
-    cpu->isar.mvfr1 = 0x12111111;
-    cpu->isar.mvfr2 = 0x00000043;
     cpu->ctr = 0x8444c004;
-    cpu->reset_sctlr = 0x00c50838;
-    cpu->id_pfr0 = 0x00000131;
-    cpu->id_pfr1 = 0x00011011;
-    cpu->isar.id_dfr0 = 0x03010066;
-    cpu->id_afr0 = 0x00000000;
-    cpu->isar.id_mmfr0 = 0x10101105;
-    cpu->isar.id_mmfr1 = 0x40000000;
-    cpu->isar.id_mmfr2 = 0x01260000;
-    cpu->isar.id_mmfr3 = 0x02102211;
-    cpu->isar.id_isar0 = 0x02101110;
-    cpu->isar.id_isar1 = 0x13112111;
-    cpu->isar.id_isar2 = 0x21232042;
-    cpu->isar.id_isar3 = 0x01112131;
-    cpu->isar.id_isar4 = 0x00011142;
-    cpu->isar.id_isar5 = 0x00011121;
-    cpu->isar.id_isar6 = 0;
-    cpu->isar.id_aa64pfr0 = 0x00002222;
-    cpu->isar.id_aa64dfr0 = 0x10305106;
-    cpu->isar.id_aa64isar0 = 0x00011120;
-    cpu->isar.id_aa64mmfr0 = 0x00001124;
-    cpu->isar.dbgdidr = 0x3516d000;
+    cpu->isar.id_mmfr0 = 0x10101105; /* Match documented value for r1p0 */
     cpu->clidr = 0x0a200023;
     cpu->ccsidr[0] = 0x701fe00a; /* 32KB L1 dcache */
     cpu->ccsidr[1] = 0x201fe012; /* 48KB L1 icache */
@@ -162,36 +168,14 @@ static void aarch64_a53_initfn(Object *obj)
     ARMCPU *cpu = ARM_CPU(obj);
 
     aarch64_cpu_common_init(obj, MIDR_IMPLEMENTER_ARM, 0xd03, 0, 4);
+    cortex_a72_a57_a53_common_init(cpu);
 
     cpu->dtb_compatible = "arm,cortex-a53";
     cpu->kvm_target = QEMU_KVM_ARM_TARGET_CORTEX_A53;
-    cpu->revidr = 0x00000000;
     cpu->reset_fpsid = 0x41034070;
-    cpu->isar.mvfr0 = 0x10110222;
-    cpu->isar.mvfr1 = 0x12111111;
-    cpu->isar.mvfr2 = 0x00000043;
     cpu->ctr = 0x84448004; /* L1Ip = VIPT */
-    cpu->reset_sctlr = 0x00c50838;
-    cpu->id_pfr0 = 0x00000131;
-    cpu->id_pfr1 = 0x00011011;
-    cpu->isar.id_dfr0 = 0x03010066;
-    cpu->id_afr0 = 0x00000000;
-    cpu->isar.id_mmfr0 = 0x10101105;
-    cpu->isar.id_mmfr1 = 0x40000000;
-    cpu->isar.id_mmfr2 = 0x01260000;
-    cpu->isar.id_mmfr3 = 0x02102211;
-    cpu->isar.id_isar0 = 0x02101110;
-    cpu->isar.id_isar1 = 0x13112111;
-    cpu->isar.id_isar2 = 0x21232042;
-    cpu->isar.id_isar3 = 0x01112131;
-    cpu->isar.id_isar4 = 0x00011142;
-    cpu->isar.id_isar5 = 0x00011121;
-    cpu->isar.id_isar6 = 0;
-    cpu->isar.id_aa64pfr0 = 0x00002222;
-    cpu->isar.id_aa64dfr0 = 0x10305106;
-    cpu->isar.id_aa64isar0 = 0x00011120;
+    cpu->isar.id_mmfr0 = 0x10101105; /* Match documented value for r0p4 */
     cpu->isar.id_aa64mmfr0 = 0x00001122; /* 40 bit physical addr */
-    cpu->isar.dbgdidr = 0x3516d000;
     cpu->clidr = 0x0a200023;
     cpu->ccsidr[0] = 0x700fe01a; /* 32KB L1 dcache */
     cpu->ccsidr[1] = 0x201fe00a; /* 32KB L1 icache */
@@ -208,34 +192,11 @@ static void aarch64_a72_initfn(Object *obj)
     ARMCPU *cpu = ARM_CPU(obj);
 
     aarch64_cpu_common_init(obj, MIDR_IMPLEMENTER_ARM, 0xd08, 0, 3);
+    cortex_a72_a57_a53_common_init(cpu);
 
     cpu->dtb_compatible = "arm,cortex-a72";
-    cpu->revidr = 0x00000000;
     cpu->reset_fpsid = 0x41034080;
-    cpu->isar.mvfr0 = 0x10110222;
-    cpu->isar.mvfr1 = 0x12111111;
-    cpu->isar.mvfr2 = 0x00000043;
     cpu->ctr = 0x8444c004;
-    cpu->reset_sctlr = 0x00c50838;
-    cpu->id_pfr0 = 0x00000131;
-    cpu->id_pfr1 = 0x00011011;
-    cpu->isar.id_dfr0 = 0x03010066;
-    cpu->id_afr0 = 0x00000000;
-    cpu->isar.id_mmfr0 = 0x10201105;
-    cpu->isar.id_mmfr1 = 0x40000000;
-    cpu->isar.id_mmfr2 = 0x01260000;
-    cpu->isar.id_mmfr3 = 0x02102211;
-    cpu->isar.id_isar0 = 0x02101110;
-    cpu->isar.id_isar1 = 0x13112111;
-    cpu->isar.id_isar2 = 0x21232042;
-    cpu->isar.id_isar3 = 0x01112131;
-    cpu->isar.id_isar4 = 0x00011142;
-    cpu->isar.id_isar5 = 0x00011121;
-    cpu->isar.id_aa64pfr0 = 0x00002222;
-    cpu->isar.id_aa64dfr0 = 0x10305106;
-    cpu->isar.id_aa64isar0 = 0x00011120;
-    cpu->isar.id_aa64mmfr0 = 0x00001124;
-    cpu->isar.dbgdidr = 0x3516d000;
     cpu->clidr = 0x0a200023;
     cpu->ccsidr[0] = 0x701fe00a; /* 32KB L1 dcache */
     cpu->ccsidr[1] = 0x201fe012; /* 48KB L1 icache */
-- 
2.20.1

Re: [RFC PATCH 2/3] target/arm: move cpu64 cortex processor common init settings to function

[Peter Maydell]

On Mon, 8 Jun 2020 at 12:40, Leif Lindholm <leif@nuviainc.com> wrote:
>
> Move the id register initializations identical between the platforms in
> this file into a standalone helper function, and change the cpu-specific
>
> The value of mmfr0 set for a57 and a53 violates the ARM architecture
> reference manual, but matches the values set in actual hardware r1p0 a57
> and r0p4 a53. The function sets the architectually correct value, and the
> a57/a53 init functions override it after the fact.

...and pulling out the ID register value settings into
a common function is definitely not something we want to
do. Any new CPU should set all the ID register values and
we want to be able to easily compare those settings against
the TRM.

thanks
-- PMM

[RFC PATCH 3/3] target/arm: use cortex...common_init for cpu64 max

[Leif Lindholm]

Drop the call to aarch64_a57_initfn from aarch64_max_initfn, replacing it
with calls to aarch64_cpu_common_init and cortex_a72_a57_a53_common_init.

Cache and GIC configuration is now set directly, using aarch64_a72_initfn
as a template.
Set cpu->dtb_compatible to "qemu,aarch64-max".

This has the following effects apart from the ones mentioned above:
- kvm_target will no longer be explicitly initialized for cpu max in tcg
  mode.
- id_mmfr0 will now be set to an architecturally permitted value.
- define_arm_cp_regs() is no longer called, since those registers are
  implementation defined and specific to the supported cortex-a
  processors.

Signed-off-by: Leif Lindholm <leif@nuviainc.com>
---
 target/arm/cpu64.c | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
index 9927c1f75d..452efe78bf 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
@@ -561,26 +561,20 @@ static void aarch64_max_initfn(Object *obj)
     } else {
         uint64_t t;
         uint32_t u;
-        aarch64_a57_initfn(obj);
 
+        cpu->dtb_compatible = "qemu,aarch64-max";
         /*
          * Reset MIDR so the guest doesn't mistake our 'max' CPU type for a real
          * one and try to apply errata workarounds or use impdef features we
          * don't provide.
          * An IMPLEMENTER field of 0 means "reserved for software use";
-         * ARCHITECTURE must be 0xf indicating "v7 or later, check ID registers
-         * to see which features are present";
          * the VARIANT, PARTNUM and REVISION fields are all implementation
          * defined and we choose to define PARTNUM just in case guest
          * code needs to distinguish this QEMU CPU from other software
          * implementations, though this shouldn't be needed.
          */
-        t = FIELD_DP64(0, MIDR_EL1, IMPLEMENTER, 0);
-        t = FIELD_DP64(t, MIDR_EL1, ARCHITECTURE, 0xf);
-        t = FIELD_DP64(t, MIDR_EL1, PARTNUM, 'Q');
-        t = FIELD_DP64(t, MIDR_EL1, VARIANT, 0);
-        t = FIELD_DP64(t, MIDR_EL1, REVISION, 0);
-        cpu->midr = t;
+        aarch64_cpu_common_init(obj, 0, 'Q', 0, 0);
+        cortex_a72_a57_a53_common_init(cpu);
 
         t = cpu->isar.id_aa64isar0;
         t = FIELD_DP64(t, ID_AA64ISAR0, AES, 2); /* AES + PMULL */
@@ -680,12 +674,24 @@ static void aarch64_max_initfn(Object *obj)
          * and enabling SVE in system mode is more useful in the short term.
          */
 
+        cpu->reset_fpsid = 0x41034080;
+        cpu->clidr = 0x0a200023;
+        cpu->ccsidr[0] = 0x701fe00a; /* 32KB L1 dcache */
+        cpu->ccsidr[1] = 0x201fe012; /* 48KB L1 icache */
+        cpu->ccsidr[2] = 0x707fe07a; /* 1MB L2 cache */
+        cpu->gic_num_lrs = 4;
+        cpu->gic_vpribits = 5;
+        cpu->gic_vprebits = 5;
+
 #ifdef CONFIG_USER_ONLY
         /* For usermode -cpu max we can use a larger and more efficient DCZ
          * blocksize since we don't have to follow what the hardware does.
          */
         cpu->ctr = 0x80038003; /* 32 byte I and D cacheline size, VIPT icache */
         cpu->dcz_blocksize = 7; /*  512 bytes */
+#else
+        cpu->ctr = 0x8444c004;
+        cpu->dcz_blocksize = 4; /* 64 bytes */
 #endif
     }
 
-- 
2.20.1