Re: [PATCH v2 1/2] hvf: arm: Add permission check in GIC sysreg handlers

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

Hi Philippe,

On 2025/7/15 04:04, Philippe Mathieu-Daudé wrote:
> On 14/7/25 18:01, Zenghui Yu wrote:
> > Quoting Peter Maydell:
> >
> > " 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). "
> >
> > Before we add more registers in GIC sysreg handlers, let's get it correct
> > by adding the .access checks to hvf_sysreg_read_cp() and
> > hvf_sysreg_write_cp(). With that, a sysreg access with invalid permission
> > will result in an UNDEFINED exception.
> >
> > Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> > Signed-off-by: Zenghui Yu <zenghui.yu@linux.dev>
> > ---
> >
> > I hard-code the @current_el parameter of cp_access_ok() to 1 because
> >
> > * we only support EL0 and EL1 in HVF, and
> 
> This might change with this work:
> https://lore.kernel.org/qemu-devel/20250620172751.94231-1-philmd@linaro.org/
> and plan to leverage M3/M4 for EL2 support:
> https://developer.apple.com/documentation/hypervisor/hv_vm_config_set_el2_enabled(_:_:)

Thanks for the heads-up! I hadn't noticed that and need to have a
further look at both.

An alternative would be using arm_current_el() as the @current_el [1],
plus a cpu_synchronize_state() before cp_access_ok() to synchronize
env->pstate from HVF. I'm not sure if it works for the new split-accel.

P.S., there is another arm_current_el() (in hvf.c, pmswinc_write()/
pmu_counter_enabled()) for which we haven't called
cpu_synchronize_state() to synchronize env->pstate. Is it wrong?

Probably we can do an overall cpu_synchronize_state() on every "handle
VMEXIT", to fix at least the above issue, which however definitely hurts
the performance.. What do you think?

[1] https://lore.kernel.org/r/d9c8d200-4453-48d7-b14a-8e15a7cf6602@linux.dev

Thanks,
Zenghui