[PATCH v2 0/2] hvf: arm: two small GIC sysreg emulation fixes

[PATCH v2 0/2] hvf: arm: two small GIC sysreg emulation fixes

[Zenghui Yu]

* From v1 [1]:
  - add patch #1, as suggested by Peter

[1] https://lore.kernel.org/r/20250315132030.95209-1-zenghui.yu@linux.dev

Zenghui Yu (2):
  hvf: arm: Add permission check in GIC sysreg handlers
  hvf: arm: Emulate ICC_RPR_EL1 accesses properly

 target/arm/hvf/hvf.c | 8 ++++++++
 1 file changed, 8 insertions(+)

-- 
2.34.1

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

[Zenghui Yu]

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
* a GIC sysreg access from EL0 would result in an UNDEF exception which is
  taken to EL1 (without going back to QEMU for emulation).

 target/arm/hvf/hvf.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/target/arm/hvf/hvf.c b/target/arm/hvf/hvf.c
index 0c7396ad6f..1db0b77fb6 100644
--- a/target/arm/hvf/hvf.c
+++ b/target/arm/hvf/hvf.c
@@ -1270,6 +1270,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(1, ri, true)) {
+            return false;
+        }
         if (ri->accessfn) {
             if (ri->accessfn(env, ri, true) != CP_ACCESS_OK) {
                 return false;
@@ -1550,6 +1553,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(1, ri, false)) {
+            return false;
+        }
         if (ri->accessfn) {
             if (ri->accessfn(env, ri, false) != CP_ACCESS_OK) {
                 return false;
-- 
2.34.1

[PATCH v2 2/2] 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.

Fixes: a2260983c655 ("hvf: arm: Add support for GICv3")
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 1db0b77fb6..df556626ce 100644
--- a/target/arm/hvf/hvf.c
+++ b/target/arm/hvf/hvf.c
@@ -1368,6 +1368,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:
@@ -1685,6 +1686,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 v2 2/2] hvf: arm: Emulate ICC_RPR_EL1 accesses properly

[Philippe Mathieu-Daudé]

On 14/7/25 18:01, Zenghui Yu 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.
> 
> Fixes: a2260983c655 ("hvf: arm: Add support for GICv3")
> Signed-off-by: Zenghui Yu <zenghui.yu@linux.dev>
> ---
>   target/arm/hvf/hvf.c | 2 ++
>   1 file changed, 2 insertions(+)

To the best of my knowledge (auditing this is safe for EL<=2),

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

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

[Philippe Mathieu-Daudé]

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(_:_:)

> * a GIC sysreg access from EL0 would result in an UNDEF exception which is
>    taken to EL1 (without going back to QEMU for emulation).
> 
>   target/arm/hvf/hvf.c | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/target/arm/hvf/hvf.c b/target/arm/hvf/hvf.c
> index 0c7396ad6f..1db0b77fb6 100644
> --- a/target/arm/hvf/hvf.c
> +++ b/target/arm/hvf/hvf.c
> @@ -1270,6 +1270,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(1, ri, true)) {
> +            return false;
> +        }
>           if (ri->accessfn) {
>               if (ri->accessfn(env, ri, true) != CP_ACCESS_OK) {
>                   return false;
> @@ -1550,6 +1553,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(1, ri, false)) {
> +            return false;
> +        }
>           if (ri->accessfn) {
>               if (ri->accessfn(env, ri, false) != CP_ACCESS_OK) {
>                   return false;

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

[Zenghui Yu]

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

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

[Peter Maydell]

On Mon, 14 Jul 2025 at 21:04, Philippe Mathieu-Daudé <philmd@linaro.org> 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(_:_:)

True, but for 10.1 I'm going to take these patches as-is, because
they do fix a bug, and handling EL2 in hvf with an emulated GIC is
going to need a more general look at the GIC code anyway.
(My preference would be to use the GICv3 which hvf provides in
macos 15 and up when we can in any case.)

Syncing the whole VM state for any call through to the GIC
emulation would be quite heavyweight. I'm not sure if we exactly
thought through that the state would not be synced here, though:
the GIC emulation was never written to assume that some CPU
registers might not be in sync...

thanks
-- PMM

Re: [PATCH v2 0/2] hvf: arm: two small GIC sysreg emulation fixes

[Peter Maydell]

On Mon, 14 Jul 2025 at 17:01, Zenghui Yu <zenghui.yu@linux.dev> wrote:
>
> * From v1 [1]:
>   - add patch #1, as suggested by Peter
>
> [1] https://lore.kernel.org/r/20250315132030.95209-1-zenghui.yu@linux.dev
>
> Zenghui Yu (2):
>   hvf: arm: Add permission check in GIC sysreg handlers
>   hvf: arm: Emulate ICC_RPR_EL1 accesses properly

Applied to target-arm.next for 10.1, thanks.

-- PMM

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

[Mohamed Mediouni]

> On 21. Jul 2025, at 12:19, Peter Maydell <peter.maydell@linaro.org> wrote:
> 
> On Mon, 14 Jul 2025 at 21:04, Philippe Mathieu-Daudé <philmd@linaro.org> 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(_:_:)
> 
> True, but for 10.1 I'm going to take these patches as-is, because
> they do fix a bug, and handling EL2 in hvf with an emulated GIC is
> going to need a more general look at the GIC code anyway.
> (My preference would be to use the GICv3 which hvf provides in
> macos 15 and up when we can in any case.)
Something to note on the vGIC provided by Hypervisor.framework is that it provides its serialisable internal state as an opaque structure which isn’t guaranteed to not change in the future (with however guarantees that it’ll be readable on newer macOS versions than the one it was generated from).

And is of course not a documented one.
> Syncing the whole VM state for any call through to the GIC
> emulation would be quite heavyweight. I'm not sure if we exactly
> thought through that the state would not be synced here, though:
> the GIC emulation was never written to assume that some CPU
> registers might not be in sync...
> 
> thanks
> -- PMM
>