* [PATCH v3 1/2] hvf: arm: Use macros for sysreg shift/masking
@ 2022-02-09 12:41 Alexander Graf
2022-02-09 12:41 ` [PATCH v3 2/2] hvf: arm: Handle unknown ID registers as RES0 Alexander Graf
` (3 more replies)
0 siblings, 4 replies; 6+ messages in thread
From: Alexander Graf @ 2022-02-09 12:41 UTC (permalink / raw)
To: Peter Maydell
Cc: Roman Bolshakov, qemu-arm, qemu-devel, Cameron Esfahani,
Ivan Babrou
We are parsing the syndrome field for sysregs in multiple places across
the hvf code, but repeat shift/mask operations with hard coded constants
every time. This is an error prone approach and makes it harder to reason
about the correctness of these operations.
Let's introduce macros that allow us to unify the constants used as well
as create new helpers to extract fields from the sysreg value.
Suggested-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Alexander Graf <agraf@csgraf.de>
---
target/arm/hvf/hvf.c | 69 ++++++++++++++++++++++++++++++--------------
1 file changed, 47 insertions(+), 22 deletions(-)
diff --git a/target/arm/hvf/hvf.c b/target/arm/hvf/hvf.c
index 92ad0d29c4..8d0447ab01 100644
--- a/target/arm/hvf/hvf.c
+++ b/target/arm/hvf/hvf.c
@@ -35,9 +35,34 @@
ENCODE_AA64_CP_REG(CP_REG_ARM64_SYSREG_CP, crn, crm, op0, op1, op2)
#define PL1_WRITE_MASK 0x4
+#define SYSREG_OP0_SHIFT 20
+#define SYSREG_OP0_MASK 0x3
+#define SYSREG_OP0(sysreg) ((sysreg >> SYSREG_OP0_SHIFT) & SYSREG_OP0_MASK)
+#define SYSREG_OP1_SHIFT 14
+#define SYSREG_OP1_MASK 0x7
+#define SYSREG_OP1(sysreg) ((sysreg >> SYSREG_OP1_SHIFT) & SYSREG_OP1_MASK)
+#define SYSREG_CRN_SHIFT 10
+#define SYSREG_CRN_MASK 0xf
+#define SYSREG_CRN(sysreg) ((sysreg >> SYSREG_CRN_SHIFT) & SYSREG_CRN_MASK)
+#define SYSREG_CRM_SHIFT 1
+#define SYSREG_CRM_MASK 0xf
+#define SYSREG_CRM(sysreg) ((sysreg >> SYSREG_CRM_SHIFT) & SYSREG_CRM_MASK)
+#define SYSREG_OP2_SHIFT 17
+#define SYSREG_OP2_MASK 0x7
+#define SYSREG_OP2(sysreg) ((sysreg >> SYSREG_OP2_SHIFT) & SYSREG_OP2_MASK)
+
#define SYSREG(op0, op1, crn, crm, op2) \
- ((op0 << 20) | (op2 << 17) | (op1 << 14) | (crn << 10) | (crm << 1))
-#define SYSREG_MASK SYSREG(0x3, 0x7, 0xf, 0xf, 0x7)
+ ((op0 << SYSREG_OP0_SHIFT) | \
+ (op1 << SYSREG_OP1_SHIFT) | \
+ (crn << SYSREG_CRN_SHIFT) | \
+ (crm << SYSREG_CRM_SHIFT) | \
+ (op2 << SYSREG_OP2_SHIFT))
+#define SYSREG_MASK \
+ SYSREG(SYSREG_OP0_MASK, \
+ SYSREG_OP1_MASK, \
+ SYSREG_CRN_MASK, \
+ SYSREG_CRM_MASK, \
+ SYSREG_OP2_MASK)
#define SYSREG_OSLAR_EL1 SYSREG(2, 0, 1, 0, 4)
#define SYSREG_OSLSR_EL1 SYSREG(2, 0, 1, 1, 4)
#define SYSREG_OSDLR_EL1 SYSREG(2, 0, 1, 3, 4)
@@ -783,21 +808,21 @@ static int hvf_sysreg_read(CPUState *cpu, uint32_t reg, uint32_t rt)
default:
cpu_synchronize_state(cpu);
trace_hvf_unhandled_sysreg_read(env->pc, reg,
- (reg >> 20) & 0x3,
- (reg >> 14) & 0x7,
- (reg >> 10) & 0xf,
- (reg >> 1) & 0xf,
- (reg >> 17) & 0x7);
+ SYSREG_OP0(reg),
+ SYSREG_OP1(reg),
+ SYSREG_CRN(reg),
+ SYSREG_CRM(reg),
+ SYSREG_OP2(reg));
hvf_raise_exception(cpu, EXCP_UDEF, syn_uncategorized());
return 1;
}
trace_hvf_sysreg_read(reg,
- (reg >> 20) & 0x3,
- (reg >> 14) & 0x7,
- (reg >> 10) & 0xf,
- (reg >> 1) & 0xf,
- (reg >> 17) & 0x7,
+ SYSREG_OP0(reg),
+ SYSREG_OP1(reg),
+ SYSREG_CRN(reg),
+ SYSREG_CRM(reg),
+ SYSREG_OP2(reg),
val);
hvf_set_reg(cpu, rt, val);
@@ -886,11 +911,11 @@ static int hvf_sysreg_write(CPUState *cpu, uint32_t reg, uint64_t val)
CPUARMState *env = &arm_cpu->env;
trace_hvf_sysreg_write(reg,
- (reg >> 20) & 0x3,
- (reg >> 14) & 0x7,
- (reg >> 10) & 0xf,
- (reg >> 1) & 0xf,
- (reg >> 17) & 0x7,
+ SYSREG_OP0(reg),
+ SYSREG_OP1(reg),
+ SYSREG_CRN(reg),
+ SYSREG_CRM(reg),
+ SYSREG_OP2(reg),
val);
switch (reg) {
@@ -960,11 +985,11 @@ static int hvf_sysreg_write(CPUState *cpu, uint32_t reg, uint64_t val)
default:
cpu_synchronize_state(cpu);
trace_hvf_unhandled_sysreg_write(env->pc, reg,
- (reg >> 20) & 0x3,
- (reg >> 14) & 0x7,
- (reg >> 10) & 0xf,
- (reg >> 1) & 0xf,
- (reg >> 17) & 0x7);
+ SYSREG_OP0(reg),
+ SYSREG_OP1(reg),
+ SYSREG_CRN(reg),
+ SYSREG_CRM(reg),
+ SYSREG_OP2(reg));
hvf_raise_exception(cpu, EXCP_UDEF, syn_uncategorized());
return 1;
}
--
2.32.0 (Apple Git-132)
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v3 2/2] hvf: arm: Handle unknown ID registers as RES0
2022-02-09 12:41 [PATCH v3 1/2] hvf: arm: Use macros for sysreg shift/masking Alexander Graf
@ 2022-02-09 12:41 ` Alexander Graf
2022-02-16 17:32 ` Peter Maydell
2022-02-11 8:13 ` [PATCH v3 1/2] hvf: arm: Use macros for sysreg shift/masking Cameron Esfahani
` (2 subsequent siblings)
3 siblings, 1 reply; 6+ messages in thread
From: Alexander Graf @ 2022-02-09 12:41 UTC (permalink / raw)
To: Peter Maydell
Cc: qemu-stable, Cameron Esfahani, qemu-devel, Roman Bolshakov,
qemu-arm, Ivan Babrou
Recent Linux versions added support to read ID_AA64ISAR2_EL1. On M1,
those reads trap into QEMU which handles them as faults.
However, AArch64 ID registers should always read as RES0. Let's
handle them accordingly.
This fixes booting Linux 5.17 guests.
Cc: qemu-stable@nongnu.org
Reported-by: Ivan Babrou <ivan@cloudflare.com>
Signed-off-by: Alexander Graf <agraf@csgraf.de>
---
v1 -> v2:
- Handle all ID registers instead of just ID_AA64ISAR2_EL1.
v2 -> v3:
- Use new sysreg parsing macros
- Remove useless check for op2
---
target/arm/hvf/hvf.c | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/target/arm/hvf/hvf.c b/target/arm/hvf/hvf.c
index 8d0447ab01..5ca7aa2dcd 100644
--- a/target/arm/hvf/hvf.c
+++ b/target/arm/hvf/hvf.c
@@ -754,6 +754,15 @@ static bool hvf_handle_psci_call(CPUState *cpu)
return true;
}
+static bool is_id_sysreg(uint32_t reg)
+{
+ return SYSREG_OP0(reg) == 3 &&
+ SYSREG_OP1(reg) == 0 &&
+ SYSREG_CRN(reg) == 0 &&
+ SYSREG_CRM(reg) >= 1 &&
+ SYSREG_CRM(reg) < 8;
+}
+
static int hvf_sysreg_read(CPUState *cpu, uint32_t reg, uint32_t rt)
{
ARMCPU *arm_cpu = ARM_CPU(cpu);
@@ -806,6 +815,11 @@ static int hvf_sysreg_read(CPUState *cpu, uint32_t reg, uint32_t rt)
/* Dummy register */
break;
default:
+ if (is_id_sysreg(reg)) {
+ /* ID system registers read as RES0 */
+ val = 0;
+ break;
+ }
cpu_synchronize_state(cpu);
trace_hvf_unhandled_sysreg_read(env->pc, reg,
SYSREG_OP0(reg),
--
2.32.0 (Apple Git-132)
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v3 1/2] hvf: arm: Use macros for sysreg shift/masking
2022-02-09 12:41 [PATCH v3 1/2] hvf: arm: Use macros for sysreg shift/masking Alexander Graf
2022-02-09 12:41 ` [PATCH v3 2/2] hvf: arm: Handle unknown ID registers as RES0 Alexander Graf
@ 2022-02-11 8:13 ` Cameron Esfahani
2022-02-11 9:40 ` Philippe Mathieu-Daudé via
2022-02-16 17:35 ` Peter Maydell
3 siblings, 0 replies; 6+ messages in thread
From: Cameron Esfahani @ 2022-02-11 8:13 UTC (permalink / raw)
To: Alexander Graf
Cc: Peter Maydell, qemu-arm, Cameron Esfahani via, Roman Bolshakov,
Ivan Babrou
[-- Attachment #1: Type: text/plain, Size: 5889 bytes --]
LGTM
Reviewed-by: Cameron Esfahani <dirty@apple.com <mailto:dirty@apple.com>>
Cameron
> On Feb 9, 2022, at 4:41 AM, Alexander Graf <agraf@csgraf.de> wrote:
>
> We are parsing the syndrome field for sysregs in multiple places across
> the hvf code, but repeat shift/mask operations with hard coded constants
> every time. This is an error prone approach and makes it harder to reason
> about the correctness of these operations.
>
> Let's introduce macros that allow us to unify the constants used as well
> as create new helpers to extract fields from the sysreg value.
>
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Alexander Graf <agraf@csgraf.de>
> ---
> target/arm/hvf/hvf.c | 69 ++++++++++++++++++++++++++++++--------------
> 1 file changed, 47 insertions(+), 22 deletions(-)
>
> diff --git a/target/arm/hvf/hvf.c b/target/arm/hvf/hvf.c
> index 92ad0d29c4..8d0447ab01 100644
> --- a/target/arm/hvf/hvf.c
> +++ b/target/arm/hvf/hvf.c
> @@ -35,9 +35,34 @@
> ENCODE_AA64_CP_REG(CP_REG_ARM64_SYSREG_CP, crn, crm, op0, op1, op2)
> #define PL1_WRITE_MASK 0x4
>
> +#define SYSREG_OP0_SHIFT 20
> +#define SYSREG_OP0_MASK 0x3
> +#define SYSREG_OP0(sysreg) ((sysreg >> SYSREG_OP0_SHIFT) & SYSREG_OP0_MASK)
> +#define SYSREG_OP1_SHIFT 14
> +#define SYSREG_OP1_MASK 0x7
> +#define SYSREG_OP1(sysreg) ((sysreg >> SYSREG_OP1_SHIFT) & SYSREG_OP1_MASK)
> +#define SYSREG_CRN_SHIFT 10
> +#define SYSREG_CRN_MASK 0xf
> +#define SYSREG_CRN(sysreg) ((sysreg >> SYSREG_CRN_SHIFT) & SYSREG_CRN_MASK)
> +#define SYSREG_CRM_SHIFT 1
> +#define SYSREG_CRM_MASK 0xf
> +#define SYSREG_CRM(sysreg) ((sysreg >> SYSREG_CRM_SHIFT) & SYSREG_CRM_MASK)
> +#define SYSREG_OP2_SHIFT 17
> +#define SYSREG_OP2_MASK 0x7
> +#define SYSREG_OP2(sysreg) ((sysreg >> SYSREG_OP2_SHIFT) & SYSREG_OP2_MASK)
> +
> #define SYSREG(op0, op1, crn, crm, op2) \
> - ((op0 << 20) | (op2 << 17) | (op1 << 14) | (crn << 10) | (crm << 1))
> -#define SYSREG_MASK SYSREG(0x3, 0x7, 0xf, 0xf, 0x7)
> + ((op0 << SYSREG_OP0_SHIFT) | \
> + (op1 << SYSREG_OP1_SHIFT) | \
> + (crn << SYSREG_CRN_SHIFT) | \
> + (crm << SYSREG_CRM_SHIFT) | \
> + (op2 << SYSREG_OP2_SHIFT))
> +#define SYSREG_MASK \
> + SYSREG(SYSREG_OP0_MASK, \
> + SYSREG_OP1_MASK, \
> + SYSREG_CRN_MASK, \
> + SYSREG_CRM_MASK, \
> + SYSREG_OP2_MASK)
> #define SYSREG_OSLAR_EL1 SYSREG(2, 0, 1, 0, 4)
> #define SYSREG_OSLSR_EL1 SYSREG(2, 0, 1, 1, 4)
> #define SYSREG_OSDLR_EL1 SYSREG(2, 0, 1, 3, 4)
> @@ -783,21 +808,21 @@ static int hvf_sysreg_read(CPUState *cpu, uint32_t reg, uint32_t rt)
> default:
> cpu_synchronize_state(cpu);
> trace_hvf_unhandled_sysreg_read(env->pc, reg,
> - (reg >> 20) & 0x3,
> - (reg >> 14) & 0x7,
> - (reg >> 10) & 0xf,
> - (reg >> 1) & 0xf,
> - (reg >> 17) & 0x7);
> + SYSREG_OP0(reg),
> + SYSREG_OP1(reg),
> + SYSREG_CRN(reg),
> + SYSREG_CRM(reg),
> + SYSREG_OP2(reg));
> hvf_raise_exception(cpu, EXCP_UDEF, syn_uncategorized());
> return 1;
> }
>
> trace_hvf_sysreg_read(reg,
> - (reg >> 20) & 0x3,
> - (reg >> 14) & 0x7,
> - (reg >> 10) & 0xf,
> - (reg >> 1) & 0xf,
> - (reg >> 17) & 0x7,
> + SYSREG_OP0(reg),
> + SYSREG_OP1(reg),
> + SYSREG_CRN(reg),
> + SYSREG_CRM(reg),
> + SYSREG_OP2(reg),
> val);
> hvf_set_reg(cpu, rt, val);
>
> @@ -886,11 +911,11 @@ static int hvf_sysreg_write(CPUState *cpu, uint32_t reg, uint64_t val)
> CPUARMState *env = &arm_cpu->env;
>
> trace_hvf_sysreg_write(reg,
> - (reg >> 20) & 0x3,
> - (reg >> 14) & 0x7,
> - (reg >> 10) & 0xf,
> - (reg >> 1) & 0xf,
> - (reg >> 17) & 0x7,
> + SYSREG_OP0(reg),
> + SYSREG_OP1(reg),
> + SYSREG_CRN(reg),
> + SYSREG_CRM(reg),
> + SYSREG_OP2(reg),
> val);
>
> switch (reg) {
> @@ -960,11 +985,11 @@ static int hvf_sysreg_write(CPUState *cpu, uint32_t reg, uint64_t val)
> default:
> cpu_synchronize_state(cpu);
> trace_hvf_unhandled_sysreg_write(env->pc, reg,
> - (reg >> 20) & 0x3,
> - (reg >> 14) & 0x7,
> - (reg >> 10) & 0xf,
> - (reg >> 1) & 0xf,
> - (reg >> 17) & 0x7);
> + SYSREG_OP0(reg),
> + SYSREG_OP1(reg),
> + SYSREG_CRN(reg),
> + SYSREG_CRM(reg),
> + SYSREG_OP2(reg));
> hvf_raise_exception(cpu, EXCP_UDEF, syn_uncategorized());
> return 1;
> }
> --
> 2.32.0 (Apple Git-132)
>
[-- Attachment #2: Type: text/html, Size: 16136 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3 1/2] hvf: arm: Use macros for sysreg shift/masking
2022-02-09 12:41 [PATCH v3 1/2] hvf: arm: Use macros for sysreg shift/masking Alexander Graf
2022-02-09 12:41 ` [PATCH v3 2/2] hvf: arm: Handle unknown ID registers as RES0 Alexander Graf
2022-02-11 8:13 ` [PATCH v3 1/2] hvf: arm: Use macros for sysreg shift/masking Cameron Esfahani
@ 2022-02-11 9:40 ` Philippe Mathieu-Daudé via
2022-02-16 17:35 ` Peter Maydell
3 siblings, 0 replies; 6+ messages in thread
From: Philippe Mathieu-Daudé via @ 2022-02-11 9:40 UTC (permalink / raw)
To: Alexander Graf, Peter Maydell
Cc: Roman Bolshakov, qemu-arm, qemu-devel, Cameron Esfahani,
Ivan Babrou
On 9/2/22 13:41, Alexander Graf wrote:
> We are parsing the syndrome field for sysregs in multiple places across
> the hvf code, but repeat shift/mask operations with hard coded constants
> every time. This is an error prone approach and makes it harder to reason
> about the correctness of these operations.
>
> Let's introduce macros that allow us to unify the constants used as well
> as create new helpers to extract fields from the sysreg value.
>
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Alexander Graf <agraf@csgraf.de>
> ---
> target/arm/hvf/hvf.c | 69 ++++++++++++++++++++++++++++++--------------
> 1 file changed, 47 insertions(+), 22 deletions(-)
>
> diff --git a/target/arm/hvf/hvf.c b/target/arm/hvf/hvf.c
> index 92ad0d29c4..8d0447ab01 100644
> --- a/target/arm/hvf/hvf.c
> +++ b/target/arm/hvf/hvf.c
> @@ -35,9 +35,34 @@
> ENCODE_AA64_CP_REG(CP_REG_ARM64_SYSREG_CP, crn, crm, op0, op1, op2)
> #define PL1_WRITE_MASK 0x4
>
> +#define SYSREG_OP0_SHIFT 20
> +#define SYSREG_OP0_MASK 0x3
> +#define SYSREG_OP0(sysreg) ((sysreg >> SYSREG_OP0_SHIFT) & SYSREG_OP0_MASK)
> +#define SYSREG_OP1_SHIFT 14
> +#define SYSREG_OP1_MASK 0x7
> +#define SYSREG_OP1(sysreg) ((sysreg >> SYSREG_OP1_SHIFT) & SYSREG_OP1_MASK)
> +#define SYSREG_CRN_SHIFT 10
> +#define SYSREG_CRN_MASK 0xf
> +#define SYSREG_CRN(sysreg) ((sysreg >> SYSREG_CRN_SHIFT) & SYSREG_CRN_MASK)
> +#define SYSREG_CRM_SHIFT 1
> +#define SYSREG_CRM_MASK 0xf
> +#define SYSREG_CRM(sysreg) ((sysreg >> SYSREG_CRM_SHIFT) & SYSREG_CRM_MASK)
> +#define SYSREG_OP2_SHIFT 17
> +#define SYSREG_OP2_MASK 0x7
> +#define SYSREG_OP2(sysreg) ((sysreg >> SYSREG_OP2_SHIFT) & SYSREG_OP2_MASK)
Alternatively easier to read using the "hw/registerfields.h" macros:
FIELD(SYSREG, OP0, 20, 2)
FIELD(SYSREG, OP2, 17, 3)
FIELD(SYSREG, OP1, 14, 3)
FIELD(SYSREG, CRN, 10, 4)
FIELD(SYSREG, CRM, 1, 4)
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3 2/2] hvf: arm: Handle unknown ID registers as RES0
2022-02-09 12:41 ` [PATCH v3 2/2] hvf: arm: Handle unknown ID registers as RES0 Alexander Graf
@ 2022-02-16 17:32 ` Peter Maydell
0 siblings, 0 replies; 6+ messages in thread
From: Peter Maydell @ 2022-02-16 17:32 UTC (permalink / raw)
To: Alexander Graf
Cc: qemu-stable, Cameron Esfahani, qemu-devel, Roman Bolshakov,
qemu-arm, Ivan Babrou
On Wed, 9 Feb 2022 at 12:41, Alexander Graf <agraf@csgraf.de> wrote:
>
> Recent Linux versions added support to read ID_AA64ISAR2_EL1. On M1,
> those reads trap into QEMU which handles them as faults.
>
> However, AArch64 ID registers should always read as RES0. Let's
> handle them accordingly.
>
> This fixes booting Linux 5.17 guests.
>
> Cc: qemu-stable@nongnu.org
> Reported-by: Ivan Babrou <ivan@cloudflare.com>
> Signed-off-by: Alexander Graf <agraf@csgraf.de>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
thanks
-- PMM
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3 1/2] hvf: arm: Use macros for sysreg shift/masking
2022-02-09 12:41 [PATCH v3 1/2] hvf: arm: Use macros for sysreg shift/masking Alexander Graf
` (2 preceding siblings ...)
2022-02-11 9:40 ` Philippe Mathieu-Daudé via
@ 2022-02-16 17:35 ` Peter Maydell
3 siblings, 0 replies; 6+ messages in thread
From: Peter Maydell @ 2022-02-16 17:35 UTC (permalink / raw)
To: Alexander Graf
Cc: Roman Bolshakov, qemu-arm, qemu-devel, Cameron Esfahani,
Ivan Babrou
On Wed, 9 Feb 2022 at 12:41, Alexander Graf <agraf@csgraf.de> wrote:
>
> We are parsing the syndrome field for sysregs in multiple places across
> the hvf code, but repeat shift/mask operations with hard coded constants
> every time. This is an error prone approach and makes it harder to reason
> about the correctness of these operations.
>
> Let's introduce macros that allow us to unify the constants used as well
> as create new helpers to extract fields from the sysreg value.
>
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Alexander Graf <agraf@csgraf.de>
Applied this and patch 2/2 to target-arm.next, thanks.
(If you can send multi-patch sets with a cover letter that helps the
automated tools, by the way.)
-- PMM
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-02-16 17:55 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-02-09 12:41 [PATCH v3 1/2] hvf: arm: Use macros for sysreg shift/masking Alexander Graf
2022-02-09 12:41 ` [PATCH v3 2/2] hvf: arm: Handle unknown ID registers as RES0 Alexander Graf
2022-02-16 17:32 ` Peter Maydell
2022-02-11 8:13 ` [PATCH v3 1/2] hvf: arm: Use macros for sysreg shift/masking Cameron Esfahani
2022-02-11 9:40 ` Philippe Mathieu-Daudé via
2022-02-16 17:35 ` Peter Maydell
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).