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

[Zenghui Yu <zenghui.yu@linux.dev>]

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