[PATCH] hvf: arm: Emulate ICC_RPR_EL1 accesses properly

[PATCH] hvf: arm: Emulate ICC_RPR_EL1 accesses properly

[Zenghui Yu]

Commit a2260983c655 ("hvf: arm: Add support for GICv3") added GICv3 support
by implementing emulation for a few system registers. ICC_RPR_EL1 was
defined but not plugged in the sysreg handlers (for no good reason).

Fix it.

Signed-off-by: Zenghui Yu <zenghui.yu@linux.dev>
---
 target/arm/hvf/hvf.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/target/arm/hvf/hvf.c b/target/arm/hvf/hvf.c
index 87e35c1b71..650b7f4256 100644
--- a/target/arm/hvf/hvf.c
+++ b/target/arm/hvf/hvf.c
@@ -1359,6 +1359,7 @@ static int hvf_sysreg_read(CPUState *cpu, uint32_t reg, uint64_t *val)
     case SYSREG_ICC_IGRPEN0_EL1:
     case SYSREG_ICC_IGRPEN1_EL1:
     case SYSREG_ICC_PMR_EL1:
+    case SYSREG_ICC_RPR_EL1:
     case SYSREG_ICC_SGI0R_EL1:
     case SYSREG_ICC_SGI1R_EL1:
     case SYSREG_ICC_SRE_EL1:
@@ -1673,6 +1674,7 @@ static int hvf_sysreg_write(CPUState *cpu, uint32_t reg, uint64_t val)
     case SYSREG_ICC_IGRPEN0_EL1:
     case SYSREG_ICC_IGRPEN1_EL1:
     case SYSREG_ICC_PMR_EL1:
+    case SYSREG_ICC_RPR_EL1:
     case SYSREG_ICC_SGI0R_EL1:
     case SYSREG_ICC_SGI1R_EL1:
     case SYSREG_ICC_SRE_EL1:
-- 
2.34.1

Re: [PATCH] hvf: arm: Emulate ICC_RPR_EL1 accesses properly

[Peter Maydell]

On Sat, 15 Mar 2025 at 13:21, Zenghui Yu <zenghui.yu@linux.dev> wrote:
>
> Commit a2260983c655 ("hvf: arm: Add support for GICv3") added GICv3 support
> by implementing emulation for a few system registers. ICC_RPR_EL1 was
> defined but not plugged in the sysreg handlers (for no good reason).
>
> Fix it.
>
> Signed-off-by: Zenghui Yu <zenghui.yu@linux.dev>
> ---
>  target/arm/hvf/hvf.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/target/arm/hvf/hvf.c b/target/arm/hvf/hvf.c
> index 87e35c1b71..650b7f4256 100644
> --- a/target/arm/hvf/hvf.c
> +++ b/target/arm/hvf/hvf.c
> @@ -1359,6 +1359,7 @@ static int hvf_sysreg_read(CPUState *cpu, uint32_t reg, uint64_t *val)
>      case SYSREG_ICC_IGRPEN0_EL1:
>      case SYSREG_ICC_IGRPEN1_EL1:
>      case SYSREG_ICC_PMR_EL1:
> +    case SYSREG_ICC_RPR_EL1:
>      case SYSREG_ICC_SGI0R_EL1:
>      case SYSREG_ICC_SGI1R_EL1:
>      case SYSREG_ICC_SRE_EL1:
> @@ -1673,6 +1674,7 @@ static int hvf_sysreg_write(CPUState *cpu, uint32_t reg, uint64_t val)
>      case SYSREG_ICC_IGRPEN0_EL1:
>      case SYSREG_ICC_IGRPEN1_EL1:
>      case SYSREG_ICC_PMR_EL1:
> +    case SYSREG_ICC_RPR_EL1:
>      case SYSREG_ICC_SGI0R_EL1:
>      case SYSREG_ICC_SGI1R_EL1:
>      case SYSREG_ICC_SRE_EL1:

ICC_RPR_EL1 is a read-only register. But hvf_sysreg_read_cp()
and hvf_sysreg_write_cp() do not check the .access field of the
ARMCPRegInfo to ensure that they forbid writes to registers that
are marked with a .access field that says they're read-only
(and ditto reads to write-only registers). So either we should
not list ICC_RPR_EL1 in this list in hvf_sysreg_write(), or
else we should add the .access checks to hvf_sysreg_read_cp()
and hvf_sysreg_write_cp().

I would favour the second of those two options, because it's
more robust and means we only need to care about the access
permissions of a register in one place. Plus we already get
this wrong for some registers: for instance ICC_SGI1R_EL1
is write-only but we will permit the guest to read it.

So I suggest a 2-patch series:
 * patch 1: add the checks on .access to hvf_sysreg_read_cp()
   and hvf_sysreg_write_cp(): they need to call
   cp_access_ok() to check this
 * patch 2: can then be this patch without modification

thanks
-- PMM

Re: [PATCH] hvf: arm: Emulate ICC_RPR_EL1 accesses properly

[Zenghui Yu]

On 2025/3/19 00:56, Peter Maydell wrote:
> On Sat, 15 Mar 2025 at 13:21, Zenghui Yu <zenghui.yu@linux.dev> wrote:
> >
> > Commit a2260983c655 ("hvf: arm: Add support for GICv3") added GICv3 support
> > by implementing emulation for a few system registers. ICC_RPR_EL1 was
> > defined but not plugged in the sysreg handlers (for no good reason).
> >
> > Fix it.
> >
> > Signed-off-by: Zenghui Yu <zenghui.yu@linux.dev>
> > ---
> >  target/arm/hvf/hvf.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/target/arm/hvf/hvf.c b/target/arm/hvf/hvf.c
> > index 87e35c1b71..650b7f4256 100644
> > --- a/target/arm/hvf/hvf.c
> > +++ b/target/arm/hvf/hvf.c
> > @@ -1359,6 +1359,7 @@ static int hvf_sysreg_read(CPUState *cpu, uint32_t reg, uint64_t *val)
> >      case SYSREG_ICC_IGRPEN0_EL1:
> >      case SYSREG_ICC_IGRPEN1_EL1:
> >      case SYSREG_ICC_PMR_EL1:
> > +    case SYSREG_ICC_RPR_EL1:
> >      case SYSREG_ICC_SGI0R_EL1:
> >      case SYSREG_ICC_SGI1R_EL1:
> >      case SYSREG_ICC_SRE_EL1:
> > @@ -1673,6 +1674,7 @@ static int hvf_sysreg_write(CPUState *cpu, uint32_t reg, uint64_t val)
> >      case SYSREG_ICC_IGRPEN0_EL1:
> >      case SYSREG_ICC_IGRPEN1_EL1:
> >      case SYSREG_ICC_PMR_EL1:
> > +    case SYSREG_ICC_RPR_EL1:
> >      case SYSREG_ICC_SGI0R_EL1:
> >      case SYSREG_ICC_SGI1R_EL1:
> >      case SYSREG_ICC_SRE_EL1:
> 
> ICC_RPR_EL1 is a read-only register.

Yup! Writes to it should result in an UNDEFINED exception. I completely
missed that point..

> But hvf_sysreg_read_cp()
> and hvf_sysreg_write_cp() do not check the .access field of the
> ARMCPRegInfo to ensure that they forbid writes to registers that
> are marked with a .access field that says they're read-only
> (and ditto reads to write-only registers). So either we should
> not list ICC_RPR_EL1 in this list in hvf_sysreg_write(), or
> else we should add the .access checks to hvf_sysreg_read_cp()
> and hvf_sysreg_write_cp().
> 
> I would favour the second of those two options, because it's
> more robust and means we only need to care about the access
> permissions of a register in one place. Plus we already get
> this wrong for some registers: for instance ICC_SGI1R_EL1
> is write-only but we will permit the guest to read it.
> 
> So I suggest a 2-patch series:
>  * patch 1: add the checks on .access to hvf_sysreg_read_cp()
>    and hvf_sysreg_write_cp(): they need to call
>    cp_access_ok() to check this

Thanks for your detailed suggestion Peter! I come up with something like

diff --git a/target/arm/hvf/hvf.c b/target/arm/hvf/hvf.c
index 650b7f4256..a7ca7975e0 100644
--- a/target/arm/hvf/hvf.c
+++ b/target/arm/hvf/hvf.c
@@ -1264,6 +1264,9 @@ static bool hvf_sysreg_read_cp(CPUState *cpu,
uint32_t reg, uint64_t *val)

     ri = get_arm_cp_reginfo(arm_cpu->cp_regs, hvf_reg2cp_reg(reg));
     if (ri) {
+        if (!cp_access_ok(arm_current_el(env), ri, true)) {
+            return false;
+        }
         if (ri->accessfn) {
             if (ri->accessfn(env, ri, true) != CP_ACCESS_OK) {
                 return false;
@@ -1545,6 +1548,9 @@ static bool hvf_sysreg_write_cp(CPUState *cpu,
uint32_t reg, uint64_t val)
     ri = get_arm_cp_reginfo(arm_cpu->cp_regs, hvf_reg2cp_reg(reg));

     if (ri) {
+        if (!cp_access_ok(arm_current_el(env), ri, false)) {
+            return false;
+        }
         if (ri->accessfn) {
             if (ri->accessfn(env, ri, false) != CP_ACCESS_OK) {
                 return false;

I'll do some tests before sending it out.

Thanks,
Zenghui

Re: [PATCH] hvf: arm: Emulate ICC_RPR_EL1 accesses properly

[Zenghui Yu]

Hi Peter,

Sorry for the long delay..

On 2025/3/21 00:55, Zenghui Yu wrote:
> On 2025/3/19 00:56, Peter Maydell wrote:
> >
> > ICC_RPR_EL1 is a read-only register.
> 
> Yup! Writes to it should result in an UNDEFINED exception. I completely
> missed that point..
> 
> > But hvf_sysreg_read_cp()
> > and hvf_sysreg_write_cp() do not check the .access field of the
> > ARMCPRegInfo to ensure that they forbid writes to registers that
> > are marked with a .access field that says they're read-only
> > (and ditto reads to write-only registers). So either we should
> > not list ICC_RPR_EL1 in this list in hvf_sysreg_write(), or
> > else we should add the .access checks to hvf_sysreg_read_cp()
> > and hvf_sysreg_write_cp().
> >
> > I would favour the second of those two options, because it's
> > more robust and means we only need to care about the access
> > permissions of a register in one place. Plus we already get
> > this wrong for some registers: for instance ICC_SGI1R_EL1
> > is write-only but we will permit the guest to read it.
> >
> > So I suggest a 2-patch series:
> >  * patch 1: add the checks on .access to hvf_sysreg_read_cp()
> >    and hvf_sysreg_write_cp(): they need to call
> >    cp_access_ok() to check this
> 
> Thanks for your detailed suggestion Peter! I come up with something like
> 
> diff --git a/target/arm/hvf/hvf.c b/target/arm/hvf/hvf.c
> index 650b7f4256..a7ca7975e0 100644
> --- a/target/arm/hvf/hvf.c
> +++ b/target/arm/hvf/hvf.c
> @@ -1264,6 +1264,9 @@ static bool hvf_sysreg_read_cp(CPUState *cpu, uint32_t reg, uint64_t *val)
> 
>      ri = get_arm_cp_reginfo(arm_cpu->cp_regs, hvf_reg2cp_reg(reg));
>      if (ri) {
> +        if (!cp_access_ok(arm_current_el(env), ri, true)) {

I wonder if arm_current_el() can be used at it to determine the current
exception EL.

|static inline int arm_current_el(CPUARMState *env)
|{
|    // ...
|
|    if (is_a64(env)) {
|        return extract32(env->pstate, 2, 2);
|    }

I failed to find where env->pstate gets updated on vcpu exit. Please fix
me up if I've missed any obvious points.

> +            return false;
> +        }
>          if (ri->accessfn) {
>              if (ri->accessfn(env, ri, true) != CP_ACCESS_OK) {
>                  return false;
> @@ -1545,6 +1548,9 @@ static bool hvf_sysreg_write_cp(CPUState *cpu, uint32_t reg, uint64_t val)
>      ri = get_arm_cp_reginfo(arm_cpu->cp_regs, hvf_reg2cp_reg(reg));
> 
>      if (ri) {
> +        if (!cp_access_ok(arm_current_el(env), ri, false)) {
> +            return false;
> +        }
>          if (ri->accessfn) {
>              if (ri->accessfn(env, ri, false) != CP_ACCESS_OK) {
>                  return false;

Thanks,
Zenghui