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

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

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