1. Patchew
  2. QEMU
  3. [PATCH v2 0/2] hvf: arm: two small GIC sysreg emulation fixes
  4. View patch

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

Zenghui Yu posted 2 patches 4 months ago
Maintainers: Alexander Graf <agraf@csgraf.de>, Mads Ynddal <mads@ynddal.dk>, Peter Maydell <peter.maydell@linaro.org>
[PATCH v2 1/2] hvf: arm: Add permission check in GIC sysreg handlers
Posted by Zenghui Yu 4 months ago 
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
Re: [PATCH v2 1/2] hvf: arm: Add permission check in GIC sysreg handlers
Posted by Philippe Mathieu-Daudé 4 months ago 
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
Posted by Peter Maydell 3 months, 3 weeks ago 
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 1/2] hvf: arm: Add permission check in GIC sysreg handlers
Posted by Mohamed Mediouni 3 months, 3 weeks ago 

> 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
>
Re: [PATCH v2 1/2] hvf: arm: Add permission check in GIC sysreg handlers
Posted by Zenghui Yu 4 months ago 
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

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.