On Sat, 15 Mar 2025 at 13:21, Zenghui Yu <zenghui...@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...@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
