[PATCH 0/2] Fix GDB support for macOS hvf

[PATCH 0/2] Fix GDB support for macOS hvf

[Mads Ynddal]

From: Mads Ynddal <m.ynddal@samsung.com>

In (recent versions of?) macOS, calls to hv_vcpu_set_sys_reg were failing if
they were issued outside of the specific thread that owns the vCPU.

This caused a crash when attaching a debugger through the GDB stub.

This GDB stub has worked before, so it is unclear if Apple changed the
behavior of the function in a release of macOS.

Mads Ynddal (2):
  hvf: avoid repeatedly setting trap debug for each cpu
  hvf: only update sysreg from owning thread

 accel/hvf/hvf-all.c  |  7 ++++++-
 target/arm/hvf/hvf.c | 27 +++++++++++----------------
 2 files changed, 17 insertions(+), 17 deletions(-)

-- 
2.48.1

[PATCH 1/2] hvf: avoid repeatedly setting trap debug for each cpu

[Mads Ynddal]

From: Mads Ynddal <m.ynddal@samsung.com>

hvf_arch_set_traps is already called from a context of a specific
CPUState, so we don't need to do a nested CPU_FOREACH.

It also results in an error from hv_vcpu_set_sys_reg, as it may only be
called from the thread owning the vCPU.

Tested-by: Daniel Gomez <da.gomez@samsung.com>
Signed-off-by: Mads Ynddal <m.ynddal@samsung.com>
---
 target/arm/hvf/hvf.c | 27 +++++++++++----------------
 1 file changed, 11 insertions(+), 16 deletions(-)

diff --git a/target/arm/hvf/hvf.c b/target/arm/hvf/hvf.c
index 2439af63a0..48e4b12725 100644
--- a/target/arm/hvf/hvf.c
+++ b/target/arm/hvf/hvf.c
@@ -2277,28 +2277,23 @@ static inline bool hvf_arm_hw_debug_active(CPUState *cpu)
     return ((cur_hw_wps > 0) || (cur_hw_bps > 0));
 }
 
-static void hvf_arch_set_traps(void)
+static void hvf_arch_set_traps(CPUState *cpu)
 {
-    CPUState *cpu;
     bool should_enable_traps = false;
     hv_return_t r = HV_SUCCESS;
 
     /* Check whether guest debugging is enabled for at least one vCPU; if it
      * is, enable exiting the guest on all vCPUs */
-    CPU_FOREACH(cpu) {
-        should_enable_traps |= cpu->accel->guest_debug_enabled;
-    }
-    CPU_FOREACH(cpu) {
-        /* Set whether debug exceptions exit the guest */
-        r = hv_vcpu_set_trap_debug_exceptions(cpu->accel->fd,
-                                              should_enable_traps);
-        assert_hvf_ok(r);
+    should_enable_traps |= cpu->accel->guest_debug_enabled;
+    /* Set whether debug exceptions exit the guest */
+    r = hv_vcpu_set_trap_debug_exceptions(cpu->accel->fd,
+                                            should_enable_traps);
+    assert_hvf_ok(r);
 
-        /* Set whether accesses to debug registers exit the guest */
-        r = hv_vcpu_set_trap_debug_reg_accesses(cpu->accel->fd,
-                                                should_enable_traps);
-        assert_hvf_ok(r);
-    }
+    /* Set whether accesses to debug registers exit the guest */
+    r = hv_vcpu_set_trap_debug_reg_accesses(cpu->accel->fd,
+                                            should_enable_traps);
+    assert_hvf_ok(r);
 }
 
 void hvf_arch_update_guest_debug(CPUState *cpu)
@@ -2339,7 +2334,7 @@ void hvf_arch_update_guest_debug(CPUState *cpu)
             deposit64(env->cp15.mdscr_el1, MDSCR_EL1_MDE_SHIFT, 1, 0);
     }
 
-    hvf_arch_set_traps();
+    hvf_arch_set_traps(cpu);
 }
 
 bool hvf_arch_supports_guest_debug(void)
-- 
2.48.1

Re: [PATCH 1/2] hvf: avoid repeatedly setting trap debug for each cpu

[Daniel Gomez]

On Wed, Apr 02, 2025 at 03:52:28PM +0100, Mads Ynddal wrote:
> From: Mads Ynddal <m.ynddal@samsung.com>
> 
> hvf_arch_set_traps is already called from a context of a specific
> CPUState, so we don't need to do a nested CPU_FOREACH.
> 
> It also results in an error from hv_vcpu_set_sys_reg, as it may only be
> called from the thread owning the vCPU.
> 

Reported-by: Daniel Gomez <da.gomez@samsung.com>
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2895

> Tested-by: Daniel Gomez <da.gomez@samsung.com>
> Signed-off-by: Mads Ynddal <m.ynddal@samsung.com>
> ---
>  target/arm/hvf/hvf.c | 27 +++++++++++----------------
>  1 file changed, 11 insertions(+), 16 deletions(-)
> 
> diff --git a/target/arm/hvf/hvf.c b/target/arm/hvf/hvf.c
> index 2439af63a0..48e4b12725 100644
> --- a/target/arm/hvf/hvf.c
> +++ b/target/arm/hvf/hvf.c
> @@ -2277,28 +2277,23 @@ static inline bool hvf_arm_hw_debug_active(CPUState *cpu)
>      return ((cur_hw_wps > 0) || (cur_hw_bps > 0));
>  }
>  
> -static void hvf_arch_set_traps(void)
> +static void hvf_arch_set_traps(CPUState *cpu)
>  {
> -    CPUState *cpu;
>      bool should_enable_traps = false;
>      hv_return_t r = HV_SUCCESS;
>  
>      /* Check whether guest debugging is enabled for at least one vCPU; if it
>       * is, enable exiting the guest on all vCPUs */
> -    CPU_FOREACH(cpu) {
> -        should_enable_traps |= cpu->accel->guest_debug_enabled;
> -    }
> -    CPU_FOREACH(cpu) {
> -        /* Set whether debug exceptions exit the guest */
> -        r = hv_vcpu_set_trap_debug_exceptions(cpu->accel->fd,
> -                                              should_enable_traps);
> -        assert_hvf_ok(r);
> +    should_enable_traps |= cpu->accel->guest_debug_enabled;
> +    /* Set whether debug exceptions exit the guest */
> +    r = hv_vcpu_set_trap_debug_exceptions(cpu->accel->fd,
> +                                            should_enable_traps);
> +    assert_hvf_ok(r);
>  
> -        /* Set whether accesses to debug registers exit the guest */
> -        r = hv_vcpu_set_trap_debug_reg_accesses(cpu->accel->fd,
> -                                                should_enable_traps);
> -        assert_hvf_ok(r);
> -    }
> +    /* Set whether accesses to debug registers exit the guest */
> +    r = hv_vcpu_set_trap_debug_reg_accesses(cpu->accel->fd,
> +                                            should_enable_traps);
> +    assert_hvf_ok(r);
>  }
>  
>  void hvf_arch_update_guest_debug(CPUState *cpu)
> @@ -2339,7 +2334,7 @@ void hvf_arch_update_guest_debug(CPUState *cpu)
>              deposit64(env->cp15.mdscr_el1, MDSCR_EL1_MDE_SHIFT, 1, 0);
>      }
>  
> -    hvf_arch_set_traps();
> +    hvf_arch_set_traps(cpu);
>  }
>  
>  bool hvf_arch_supports_guest_debug(void)
> -- 
> 2.48.1
> 
> 

Re: [PATCH 1/2] hvf: avoid repeatedly setting trap debug for each cpu

[Alex Bennée]

Mads Ynddal <mads@ynddal.dk> writes:

> From: Mads Ynddal <m.ynddal@samsung.com>
>
> hvf_arch_set_traps is already called from a context of a specific
> CPUState, so we don't need to do a nested CPU_FOREACH.
>
> It also results in an error from hv_vcpu_set_sys_reg, as it may only be
> called from the thread owning the vCPU.
>
> Tested-by: Daniel Gomez <da.gomez@samsung.com>
> Signed-off-by: Mads Ynddal <m.ynddal@samsung.com>

Looks reasonable to me:

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

[PATCH 2/2] hvf: only update sysreg from owning thread

[Mads Ynddal]

From: Mads Ynddal <m.ynddal@samsung.com>

hv_vcpu_set_sys_reg should only be called from the owning thread of the
vCPU, so to avoid crashes, the call to hvf_update_guest_debug is
dispatched to the individual threads.

Tested-by: Daniel Gomez <da.gomez@samsung.com>
Signed-off-by: Mads Ynddal <m.ynddal@samsung.com>
---
 accel/hvf/hvf-all.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/accel/hvf/hvf-all.c b/accel/hvf/hvf-all.c
index d404e01ade..3fc65d6b23 100644
--- a/accel/hvf/hvf-all.c
+++ b/accel/hvf/hvf-all.c
@@ -58,8 +58,13 @@ int hvf_sw_breakpoints_active(CPUState *cpu)
     return !QTAILQ_EMPTY(&hvf_state->hvf_sw_breakpoints);
 }
 
-int hvf_update_guest_debug(CPUState *cpu)
+static void do_hvf_update_guest_debug(CPUState *cpu, run_on_cpu_data arg)
 {
     hvf_arch_update_guest_debug(cpu);
+}
+
+int hvf_update_guest_debug(CPUState *cpu)
+{
+    run_on_cpu(cpu, do_hvf_update_guest_debug, RUN_ON_CPU_NULL);
     return 0;
 }
-- 
2.48.1

Re: [PATCH 2/2] hvf: only update sysreg from owning thread

[Alex Bennée]

Mads Ynddal <mads@ynddal.dk> writes:

> From: Mads Ynddal <m.ynddal@samsung.com>
>
> hv_vcpu_set_sys_reg should only be called from the owning thread of the
> vCPU, so to avoid crashes, the call to hvf_update_guest_debug is
> dispatched to the individual threads.
>
> Tested-by: Daniel Gomez <da.gomez@samsung.com>
> Signed-off-by: Mads Ynddal <m.ynddal@samsung.com>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH 2/2] hvf: only update sysreg from owning thread

[Peter Maydell]

On Wed, 2 Apr 2025 at 14:52, Mads Ynddal <mads@ynddal.dk> wrote:
>
> From: Mads Ynddal <m.ynddal@samsung.com>
>
> hv_vcpu_set_sys_reg should only be called from the owning thread of the
> vCPU, so to avoid crashes, the call to hvf_update_guest_debug is
> dispatched to the individual threads.
>
> Tested-by: Daniel Gomez <da.gomez@samsung.com>
> Signed-off-by: Mads Ynddal <m.ynddal@samsung.com>
> ---
>  accel/hvf/hvf-all.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/accel/hvf/hvf-all.c b/accel/hvf/hvf-all.c
> index d404e01ade..3fc65d6b23 100644
> --- a/accel/hvf/hvf-all.c
> +++ b/accel/hvf/hvf-all.c
> @@ -58,8 +58,13 @@ int hvf_sw_breakpoints_active(CPUState *cpu)
>      return !QTAILQ_EMPTY(&hvf_state->hvf_sw_breakpoints);
>  }
>
> -int hvf_update_guest_debug(CPUState *cpu)
> +static void do_hvf_update_guest_debug(CPUState *cpu, run_on_cpu_data arg)
>  {
>      hvf_arch_update_guest_debug(cpu);
> +}
> +
> +int hvf_update_guest_debug(CPUState *cpu)
> +{
> +    run_on_cpu(cpu, do_hvf_update_guest_debug, RUN_ON_CPU_NULL);
>      return 0;
>  }

run_on_cpu() implicitly drops the BQL, so it is a potential
really nasty footgun in this kind of situation where callers
to update_guest_debug aren't expecting the BQL to be dropped
on them. But we already use run_on_cpu() in the KVM equivalent
kvm_update_guest_debug(), so presumably it's also OK here...

-- PMM

Re: [PATCH 0/2] Fix GDB support for macOS hvf

[Mads Ynddal]

> In (recent versions of?) macOS, calls to hv_vcpu_set_sys_reg were failing if
> they were issued outside of the specific thread that owns the vCPU.
> 
> This caused a crash when attaching a debugger through the GDB stub.
> 
> This GDB stub has worked before, so it is unclear if Apple changed the
> behavior of the function in a release of macOS.
> 
> Mads Ynddal (2):
>  hvf: avoid repeatedly setting trap debug for each cpu
>  hvf: only update sysreg from owning thread
> 
> accel/hvf/hvf-all.c  |  7 ++++++-
> target/arm/hvf/hvf.c | 27 +++++++++++----------------
> 2 files changed, 17 insertions(+), 17 deletions(-)
> 

Daniel Gomez and I have been using this patch for the past two weeks. It seems to be the right fix for the issue.

—
Mads Ynddal

Re: [PATCH 0/2] Fix GDB support for macOS hvf

[Peter Maydell]

On Wed, 2 Apr 2025 at 14:52, Mads Ynddal <mads@ynddal.dk> wrote:
>
> From: Mads Ynddal <m.ynddal@samsung.com>
>
> In (recent versions of?) macOS, calls to hv_vcpu_set_sys_reg were failing if
> they were issued outside of the specific thread that owns the vCPU.
>
> This caused a crash when attaching a debugger through the GDB stub.
>
> This GDB stub has worked before, so it is unclear if Apple changed the
> behavior of the function in a release of macOS.



Applied to target-arm.next, thanks.

-- PMM