qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH 2/3] hvf: implement guest debugging on Apple Silicon hosts


From: Mads Ynddal
Subject: Re: [PATCH 2/3] hvf: implement guest debugging on Apple Silicon hosts
Date: Tue, 8 Nov 2022 12:51:39 +0100

> On 8 Nov 2022, at 11.09, Francesco Cagnin <francesco.cagnin@gmail.com> wrote:
> 
> Mads, thanks for the review and feedbacks. I worked on this a few months
> ago but only managed to have it published now, I'm sorry for work being
> done twice.
> 
> I'll add support for SSTEP_NOIRQ as you suggested.
> 
> For multi-core systems there's no particular issue, just I'm not
> familiar with all of these topics and I'm asking for good practices. KVM
> saves breakpoints information in struct 'KVMState', and following the
> same approach I've updated struct 'HVFState' to store equivalent data.
> To keep separate information for each cpu, KVM uses field 'kvm_state' in
> struct 'CPUState' (see 'include/hw/core/cpu.h'), but at the moment
> there's no analogous field for the HVF state (which is instead defined
> as a single global variable in 'accel/hvf/hvf-accel-ops.c'). Should we
> add a new field to 'CPUState' for the HVF state, or take a different
> approach?
> 
> For your last question regarding `hv_vcpu_set_trap_debug_exceptions()`,
> I think it's possible to move it to `hvf_arch_update_guest_debug()` but
> I'm not confident about `hvf_arch_init_vcpu()`, I will experiment again.

You're welcome. And that's ok. It's just unlucky timing.

In my version, I added the sw breakpoints to `hvf_vcpu_state` so they would
follow each CPU, but it's effectively a copy of the global set of breakpoints. 
I'm no expert on QEMU, as this would have been my first contribution, but AFAIK,
KVM will also add the breakpoints to all CPUs as we can't know which CPU will
take a certain process.

Moving it to `hvf_arch_update_guest_debug` would probably be best. Having it in
`hvf_arch_init_vcpu` would mean that it's always enabled. In some corner-cases,
that might have an adverse effect.

I also noticed you are adding 1 to the WRPs and BRPs. As I interpret the
documentation, you should subtract 1 instead, given the value 0 is reserved:

diff --git a/target/arm/hvf/hvf.c b/target/arm/hvf/hvf.c
index dbc3605f6d..80a583cbd1 100644
--- a/target/arm/hvf/hvf.c
+++ b/target/arm/hvf/hvf.c
@@ -39,11 +39,11 @@ static void hvf_arm_init_debug(CPUState *cpu)
 {
     ARMCPU *arm_cpu = ARM_CPU(cpu);

-    max_hw_bps = 1 + extract64(arm_cpu->isar.id_aa64dfr0, 12, 4);
+    max_hw_bps = extract64(arm_cpu->isar.id_aa64dfr0, 12, 4) - 1;
     hw_breakpoints =
         g_array_sized_new(true, true, sizeof(HWBreakpoint), max_hw_bps);

-    max_hw_wps = 1 + extract64(arm_cpu->isar.id_aa64dfr0, 20, 4);
+    max_hw_wps = extract64(arm_cpu->isar.id_aa64dfr0, 20, 4) - 1;
     hw_watchpoints =
         g_array_sized_new(true, true, sizeof(HWWatchpoint), max_hw_wps);
     return;

But the documentation is a bit ambiguous on that. Maybe we can test it?


reply via email to

[Prev in Thread] Current Thread [Next in Thread]