RE: [PATCH v3 00/10] kvm/arm: Introduce a customizable aarch64 KVM host model

[Cornelia Huck <cohuck@redhat.com>]

On Tue, May 27 2025, Cornelia Huck <cohuck@redhat.com> wrote:

> On Mon, May 26 2025, Cornelia Huck <cohuck@redhat.com> wrote:
>
>> On Fri, May 23 2025, Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com> wrote:
>>
>>> Hi,
>>>
>>>> -----Original Message-----
>>>> From: Cornelia Huck <cohuck@redhat.com>
>>>> Sent: Monday, April 14, 2025 5:39 PM
>>>> To: eric.auger.pro@gmail.com; eric.auger@redhat.com; qemu-
>>>> devel@nongnu.org; qemu-arm@nongnu.org; kvmarm@lists.linux.dev;
>>>> peter.maydell@linaro.org; richard.henderson@linaro.org;
>>>> alex.bennee@linaro.org; maz@kernel.org; oliver.upton@linux.dev;
>>>> sebott@redhat.com; Shameerali Kolothum Thodi
>>>> <shameerali.kolothum.thodi@huawei.com>; armbru@redhat.com;
>>>> berrange@redhat.com; abologna@redhat.com; jdenemar@redhat.com
>>>> Cc: agraf@csgraf.de; shahuang@redhat.com; mark.rutland@arm.com;
>>>> philmd@linaro.org; pbonzini@redhat.com; Cornelia Huck
>>>> <cohuck@redhat.com>
>>>> Subject: [PATCH v3 00/10] kvm/arm: Introduce a customizable aarch64 KVM
>>>> host model
>>>
>>> [..]
>>>
>>> )
>>>> 
>>>> Code also available at
>>>> https://gitlab.com/cohuck/qemu/-/tree/arm-cpu-model-
>>>> rfcv3?ref_type=heads
>>>
>>> I had a spin with the above branch, but Qemu boot fails,
>>>
>>> ERROR:../target/arm/cpu64.c:57:get_sysreg_idx: code should not be reached
>>> Bail out! ERROR:../target/arm/cpu64.c:57:get_sysreg_idx: code should not be reached
>>>
>>> From a quick debug, it looks like the below path results in an invalid ID idx.
>>>
>>> kvm_arm_expose_idreg_properties()
>>>  kvm_idx_to_idregs_idx(0)
>>>   get_sysreg_idx(0xc000)  --> id_register seems to start at 0xc008
>>>
>>> Haven't debugged further.
>>>
>>> I am running against a 6.15-rc1 kernel after updating the Qemu branch by,
>>> ./update-aarch64-sysreg-code.sh  path_to_6.15-rc1
>>>
>>> Not sure I am  missing anything. Please check and let me know.
>>
>> Thanks for trying this out; I'll try to re-create this here.
>> (I think I've messed up those conversion functions often enough...)
>
> The conversion functions are not at fault here, but we're missing
> registers. If we have MIDR and friends writable, they show up in the
> masks returned by the kernel, but they are not present in the kernel's
> sysreg file where we generate our definitions from, and
> kvm_idx_to_idregs_idx() asserts instead of returning an error, which is
> kind of suboptimal...
>
> So I see two possible ways to fix this:
> - add MIDR and friends to the kernel's sysreg file
> - add MIDR and friends in QEMU's cpu-sysregs.h.inc file, and only append
>   generated definitions there
>
> First option means one more round trip, second options has more
> potential for messing things up if we keep stuff local to QEMU.

With the patch below, things work for me with a 6.15+ kernel. It's a bit
messy, though, and raises questions (how do we want to handle those regs
across accelerators, for example, or how we can make sure that the code
is more robust when registers are added.)

My biggest question, however, is how this interacts with the framework
to provide lists of MIDR/REVIDR/AIDR for errata management. The hack
below adds properties to configure those regs, I guess we'd want to
suppress adding the props in order to avoid conflicts.

WDYT?

diff --git a/scripts/gen-cpu-sysreg-properties.awk b/scripts/gen-cpu-sysreg-properties.awk
index 6740e814f733..7afd9afd2650 100755
--- a/scripts/gen-cpu-sysreg-properties.awk
+++ b/scripts/gen-cpu-sysreg-properties.awk
@@ -109,6 +109,27 @@ END {
 	if (__current_block_depth != 0)
 		fatal("Missing terminator for " block_current() " block")
 
+	# Manually add MIDR/REVIDR/AIDR
+	print ""
+	print "    /* MIDR_EL1 */"
+	print "    ARM64SysReg *MIDR_EL1 = arm64_sysreg_get(MIDR_EL1_IDX);"
+	print "    MIDR_EL1->name = \"MIDR_EL1\";"
+	print "    arm64_sysreg_add_field(MIDR_EL1, \"Implementer\", 24, 31);"
+	print "    arm64_sysreg_add_field(MIDR_EL1, \"Variant\", 20, 23);"
+	print "    arm64_sysreg_add_field(MIDR_EL1, \"Architecture\", 16, 19);"
+	print "    arm64_sysreg_add_field(MIDR_EL1, \"PartNum\", 4, 15);"
+	print "    arm64_sysreg_add_field(MIDR_EL1, \"Revision\", 0, 3);"
+	print ""
+	print "    /* REVIDR_EL1 */"
+	print "    ARM64SysReg *REVIDR_EL1 = arm64_sysreg_get(REVIDR_EL1_IDX);"
+	print "    REVIDR_EL1->name = \"REVIDR_EL1\";"
+	print "    arm64_sysreg_add_field(REVIDR_EL1, \"IMPDEF\", 0, 63);"
+	print ""
+	print "    /* AIDR_EL1 */"
+	print "    ARM64SysReg *AIDR_EL1 = arm64_sysreg_get(AIDR_EL1_IDX);"
+	print "    AIDR_EL1->name = \"AIDR_EL1\";"
+	print "    arm64_sysreg_add_field(AIDR_EL1, \"IMPDEF\", 0, 63);"
+	print ""
 	print "}"
 }
 
diff --git a/scripts/gen-cpu-sysregs-header.awk b/scripts/gen-cpu-sysregs-header.awk
index 452e51035d52..2eb561b693dc 100755
--- a/scripts/gen-cpu-sysregs-header.awk
+++ b/scripts/gen-cpu-sysregs-header.awk
@@ -7,7 +7,10 @@
 BEGIN {
     print ""
 } END {
-    print ""
+    /* add MIDR, REVIDR, and AIDR */
+    print "DEF(MIDR_EL1, 3, 0, 0, 0, 0)"
+    print "DEF(REVIDR_EL1, 3, 0, 0, 0, 6)"
+    print "DEF(AIDR_EL1, 3, 1, 0, 0, 7)"
 }
 
 # skip blank lines and comment lines
diff --git a/target/arm/cpu-sysreg-properties.c b/target/arm/cpu-sysreg-properties.c
index 29c4c8ada115..bc1ae5e1a224 100644
--- a/target/arm/cpu-sysreg-properties.c
+++ b/target/arm/cpu-sysreg-properties.c
@@ -715,4 +715,24 @@ void initialize_cpu_sysreg_properties(void)
 
 /* For S2PIR_EL2 fields see PIRx_ELx */
 
+
+    /* MIDR_EL1 */
+    ARM64SysReg *MIDR_EL1 = arm64_sysreg_get(MIDR_EL1_IDX);
+    MIDR_EL1->name = "MIDR_EL1";
+    arm64_sysreg_add_field(MIDR_EL1, "Implementer", 24, 31);
+    arm64_sysreg_add_field(MIDR_EL1, "Variant", 20, 23);
+    arm64_sysreg_add_field(MIDR_EL1, "Architecture", 16, 19);
+    arm64_sysreg_add_field(MIDR_EL1, "PartNum", 4, 15);
+    arm64_sysreg_add_field(MIDR_EL1, "Revision", 0, 3);
+
+    /* REVIDR_EL1 */
+    ARM64SysReg *REVIDR_EL1 = arm64_sysreg_get(REVIDR_EL1_IDX);
+    REVIDR_EL1->name = "REVIDR_EL1";
+    arm64_sysreg_add_field(REVIDR_EL1, "IMPDEF", 0, 63);
+
+    /* AIDR_EL1 */
+    ARM64SysReg *AIDR_EL1 = arm64_sysreg_get(AIDR_EL1_IDX);
+    AIDR_EL1->name = "AIDR_EL1";
+    arm64_sysreg_add_field(AIDR_EL1, "IMPDEF", 0, 63);
+
 }
diff --git a/target/arm/cpu-sysregs.h.inc b/target/arm/cpu-sysregs.h.inc
index 02aae133eb67..8f0927ce0422 100644
--- a/target/arm/cpu-sysregs.h.inc
+++ b/target/arm/cpu-sysregs.h.inc
@@ -49,4 +49,6 @@ DEF(SMIDR_EL1, 3, 1, 0, 0, 6)
 DEF(CSSELR_EL1, 3, 2, 0, 0, 0)
 DEF(CTR_EL0, 3, 3, 0, 0, 1)
 DEF(DCZID_EL0, 3, 3, 0, 0, 7)
-
+DEF(MIDR_EL1, 3, 0, 0, 0, 0)
+DEF(REVIDR_EL1, 3, 0, 0, 0, 6)
+DEF(AIDR_EL1, 3, 1, 0, 0, 7)
diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
index 95bb728a77f1..7454f329157c 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
@@ -54,7 +54,7 @@ int get_sysreg_idx(ARMSysRegs sysreg)
     switch (sysreg) {
 #include "cpu-sysregs.h.inc"
     }
-    g_assert_not_reached();
+    return -1;
 }
 
 #undef DEF