[PATCH] hvf: arm: Handle ID_AA64ISAR2_EL1 reads

[PATCH] hvf: arm: Handle ID_AA64ISAR2_EL1 reads

[Alexander Graf]

Recent Linux versions added support to read ID_AA64ISAR2_EL1. On M1,
those reads trap into QEMU which handles them as faults.

However, according to the ARMv8 spec (issue D17783), reads on this
register in older ARMv8 revisions should be RES0. So let's treat it
as such instead.

Reported-by: Ivan Babrou <ivan@cloudflare.com>
Signed-off-by: Alexander Graf <agraf@csgraf.de>
---
 target/arm/hvf/hvf.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/target/arm/hvf/hvf.c b/target/arm/hvf/hvf.c
index 92ad0d29c4..045ec69c7c 100644
--- a/target/arm/hvf/hvf.c
+++ b/target/arm/hvf/hvf.c
@@ -54,6 +54,7 @@
 #define SYSREG_PMCEID1_EL0    SYSREG(3, 3, 9, 12, 7)
 #define SYSREG_PMCCNTR_EL0    SYSREG(3, 3, 9, 13, 0)
 #define SYSREG_PMCCFILTR_EL0  SYSREG(3, 3, 14, 15, 7)
+#define SYSREG_ID_AA64ISAR2_EL1 SYSREG(3, 0, 0, 6, 2)
 
 #define WFX_IS_WFE (1 << 0)
 
@@ -780,6 +781,10 @@ static int hvf_sysreg_read(CPUState *cpu, uint32_t reg, uint32_t rt)
     case SYSREG_OSDLR_EL1:
         /* Dummy register */
         break;
+    case SYSREG_ID_AA64ISAR2_EL1:
+        /* We do not support any of the ISAR2 features yet */
+        val = 0;
+        break;
     default:
         cpu_synchronize_state(cpu);
         trace_hvf_unhandled_sysreg_read(env->pc, reg,
-- 
2.32.0 (Apple Git-132)

Re: [PATCH] hvf: arm: Handle ID_AA64ISAR2_EL1 reads

[Cameron Esfahani]

[-- Attachment #1: Type: text/plain, Size: 1558 bytes --]

Reviewed-by: Cameron Esfahani <dirty@apple.com <mailto:dirty@apple.com>>

Cameron

> On Feb 7, 2022, at 2:52 PM, 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, according to the ARMv8 spec (issue D17783), reads on this
> register in older ARMv8 revisions should be RES0. So let's treat it
> as such instead.
> 
> Reported-by: Ivan Babrou <ivan@cloudflare.com>
> Signed-off-by: Alexander Graf <agraf@csgraf.de>
> ---
> target/arm/hvf/hvf.c | 5 +++++
> 1 file changed, 5 insertions(+)
> 
> diff --git a/target/arm/hvf/hvf.c b/target/arm/hvf/hvf.c
> index 92ad0d29c4..045ec69c7c 100644
> --- a/target/arm/hvf/hvf.c
> +++ b/target/arm/hvf/hvf.c
> @@ -54,6 +54,7 @@
> #define SYSREG_PMCEID1_EL0    SYSREG(3, 3, 9, 12, 7)
> #define SYSREG_PMCCNTR_EL0    SYSREG(3, 3, 9, 13, 0)
> #define SYSREG_PMCCFILTR_EL0  SYSREG(3, 3, 14, 15, 7)
> +#define SYSREG_ID_AA64ISAR2_EL1 SYSREG(3, 0, 0, 6, 2)
> 
> #define WFX_IS_WFE (1 << 0)
> 
> @@ -780,6 +781,10 @@ static int hvf_sysreg_read(CPUState *cpu, uint32_t reg, uint32_t rt)
>     case SYSREG_OSDLR_EL1:
>         /* Dummy register */
>         break;
> +    case SYSREG_ID_AA64ISAR2_EL1:
> +        /* We do not support any of the ISAR2 features yet */
> +        val = 0;
> +        break;
>     default:
>         cpu_synchronize_state(cpu);
>         trace_hvf_unhandled_sysreg_read(env->pc, reg,
> -- 
> 2.32.0 (Apple Git-132)
> 


[-- Attachment #2: Type: text/html, Size: 2907 bytes --]

Re: [PATCH] hvf: arm: Handle ID_AA64ISAR2_EL1 reads

[Ivan Babrou]

The patch addresses the current issue for me, thanks!

Is it possible to make it more future proof? I can imagine a very
similar situation arising in the future and it would be good to be
able to handle it gracefully. If it's not possible, then maybe there's
a way to output some sort of error from qemu that a user can search
for. Right now all one gets is a qemu process using 100% of CPU and
outputting nothing. None of this is required for this patch, but it
would be good to have it at some point.

Reviewed-by: Ivan Babrou <ivan@cloudflare.com>

Re: [PATCH] hvf: arm: Handle ID_AA64ISAR2_EL1 reads

[Peter Maydell]

On Mon, 7 Feb 2022 at 22:52, 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, according to the ARMv8 spec (issue D17783), reads on this
> register in older ARMv8 revisions should be RES0. So let's treat it
> as such instead.
>
> Reported-by: Ivan Babrou <ivan@cloudflare.com>
> Signed-off-by: Alexander Graf <agraf@csgraf.de>
> ---
>  target/arm/hvf/hvf.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/target/arm/hvf/hvf.c b/target/arm/hvf/hvf.c
> index 92ad0d29c4..045ec69c7c 100644
> --- a/target/arm/hvf/hvf.c
> +++ b/target/arm/hvf/hvf.c
> @@ -54,6 +54,7 @@
>  #define SYSREG_PMCEID1_EL0    SYSREG(3, 3, 9, 12, 7)
>  #define SYSREG_PMCCNTR_EL0    SYSREG(3, 3, 9, 13, 0)
>  #define SYSREG_PMCCFILTR_EL0  SYSREG(3, 3, 14, 15, 7)
> +#define SYSREG_ID_AA64ISAR2_EL1 SYSREG(3, 0, 0, 6, 2)
>
>  #define WFX_IS_WFE (1 << 0)
>
> @@ -780,6 +781,10 @@ static int hvf_sysreg_read(CPUState *cpu, uint32_t reg, uint32_t rt)
>      case SYSREG_OSDLR_EL1:
>          /* Dummy register */
>          break;
> +    case SYSREG_ID_AA64ISAR2_EL1:
> +        /* We do not support any of the ISAR2 features yet */
> +        val = 0;
> +        break;
>      default:

We should handle all the architected "this should RAZ/WI"
ID register space, if hvf doesn't do the right thing internally.

thanks
-- PMM