| Message ID | 20221123121712.72817-1-mads@ynddal.dk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | gdbstub: move update guest debug to accel ops | expand |
> On 23 Nov 2022, at 13.17, Mads Ynddal <mads@ynddal.dk> wrote: > > From: Mads Ynddal <m.ynddal@samsung.com> > > Continuing the refactor of a48e7d9e52 (gdbstub: move guest debug support > check to ops) by removing hardcoded kvm_enabled() from generic cpu.c > code, and replace it with a property of AccelOpsClass. > > Signed-off-by: Mads Ynddal <m.ynddal@samsung.com> > --- > accel/kvm/kvm-accel-ops.c | 1 + > cpu.c | 10 +++++++--- > include/sysemu/accel-ops.h | 1 + > 3 files changed, 9 insertions(+), 3 deletions(-) > +CC Alex Bennée
Mads Ynddal <mads@ynddal.dk> writes: > From: Mads Ynddal <m.ynddal@samsung.com> > > Continuing the refactor of a48e7d9e52 (gdbstub: move guest debug support > check to ops) by removing hardcoded kvm_enabled() from generic cpu.c > code, and replace it with a property of AccelOpsClass. > > Signed-off-by: Mads Ynddal <m.ynddal@samsung.com> Nice. Looks good to me but I'll have a proper look when I go through my gdbstub/next queue. I don't think this is critical for 7.2. > --- > accel/kvm/kvm-accel-ops.c | 1 + > cpu.c | 10 +++++++--- > include/sysemu/accel-ops.h | 1 + > 3 files changed, 9 insertions(+), 3 deletions(-) > > diff --git a/accel/kvm/kvm-accel-ops.c b/accel/kvm/kvm-accel-ops.c > index fbf4fe3497..6ebf9a644f 100644 > --- a/accel/kvm/kvm-accel-ops.c > +++ b/accel/kvm/kvm-accel-ops.c > @@ -99,6 +99,7 @@ static void kvm_accel_ops_class_init(ObjectClass *oc, void *data) > ops->synchronize_pre_loadvm = kvm_cpu_synchronize_pre_loadvm; > > #ifdef KVM_CAP_SET_GUEST_DEBUG > + ops->update_guest_debug = kvm_update_guest_debug; > ops->supports_guest_debug = kvm_supports_guest_debug; > ops->insert_breakpoint = kvm_insert_breakpoint; > ops->remove_breakpoint = kvm_remove_breakpoint; > diff --git a/cpu.c b/cpu.c > index 2a09b05205..ef433a79e3 100644 > --- a/cpu.c > +++ b/cpu.c > @@ -31,8 +31,8 @@ > #include "hw/core/sysemu-cpu-ops.h" > #include "exec/address-spaces.h" > #endif > +#include "sysemu/cpus.h" > #include "sysemu/tcg.h" > -#include "sysemu/kvm.h" > #include "sysemu/replay.h" > #include "exec/cpu-common.h" > #include "exec/exec-all.h" > @@ -378,10 +378,14 @@ void cpu_breakpoint_remove_all(CPUState *cpu, int mask) > void cpu_single_step(CPUState *cpu, int enabled) > { > if (cpu->singlestep_enabled != enabled) { > + const AccelOpsClass *ops = cpus_get_accel(); > + > cpu->singlestep_enabled = enabled; > - if (kvm_enabled()) { > - kvm_update_guest_debug(cpu, 0); > + > + if (ops->update_guest_debug) { > + ops->update_guest_debug(cpu, 0); > } > + > trace_breakpoint_singlestep(cpu->cpu_index, enabled); > } > } > diff --git a/include/sysemu/accel-ops.h b/include/sysemu/accel-ops.h > index 8cc7996def..0a47a2f00c 100644 > --- a/include/sysemu/accel-ops.h > +++ b/include/sysemu/accel-ops.h > @@ -48,6 +48,7 @@ struct AccelOpsClass { > > /* gdbstub hooks */ > bool (*supports_guest_debug)(void); > + int (*update_guest_debug)(CPUState *cpu, unsigned long flags); > int (*insert_breakpoint)(CPUState *cpu, int type, hwaddr addr, hwaddr len); > int (*remove_breakpoint)(CPUState *cpu, int type, hwaddr addr, hwaddr len); > void (*remove_all_breakpoints)(CPUState *cpu);
> On 23 Nov 2022, at 15.05, Alex Bennée <alex.bennee@linaro.org> wrote: > > Nice. Looks good to me but I'll have a proper look when I go through my > gdbstub/next queue. I don't think this is critical for 7.2. > Thanks, and I agree. It can easily wait.
Hi, On 23/11/22 13:17, Mads Ynddal wrote: > From: Mads Ynddal <m.ynddal@samsung.com> > > Continuing the refactor of a48e7d9e52 (gdbstub: move guest debug support > check to ops) by removing hardcoded kvm_enabled() from generic cpu.c > code, and replace it with a property of AccelOpsClass. > > Signed-off-by: Mads Ynddal <m.ynddal@samsung.com> > --- > accel/kvm/kvm-accel-ops.c | 1 + > cpu.c | 10 +++++++--- > include/sysemu/accel-ops.h | 1 + > 3 files changed, 9 insertions(+), 3 deletions(-) > > diff --git a/accel/kvm/kvm-accel-ops.c b/accel/kvm/kvm-accel-ops.c > index fbf4fe3497..6ebf9a644f 100644 > --- a/accel/kvm/kvm-accel-ops.c > +++ b/accel/kvm/kvm-accel-ops.c > @@ -99,6 +99,7 @@ static void kvm_accel_ops_class_init(ObjectClass *oc, void *data) > ops->synchronize_pre_loadvm = kvm_cpu_synchronize_pre_loadvm; > > #ifdef KVM_CAP_SET_GUEST_DEBUG > + ops->update_guest_debug = kvm_update_guest_debug; > ops->supports_guest_debug = kvm_supports_guest_debug; > ops->insert_breakpoint = kvm_insert_breakpoint; > ops->remove_breakpoint = kvm_remove_breakpoint; > diff --git a/cpu.c b/cpu.c > index 2a09b05205..ef433a79e3 100644 > --- a/cpu.c > +++ b/cpu.c > @@ -31,8 +31,8 @@ > #include "hw/core/sysemu-cpu-ops.h" > #include "exec/address-spaces.h" > #endif > +#include "sysemu/cpus.h" > #include "sysemu/tcg.h" > -#include "sysemu/kvm.h" > #include "sysemu/replay.h" > #include "exec/cpu-common.h" > #include "exec/exec-all.h" > @@ -378,10 +378,14 @@ void cpu_breakpoint_remove_all(CPUState *cpu, int mask) > void cpu_single_step(CPUState *cpu, int enabled) > { > if (cpu->singlestep_enabled != enabled) { > + const AccelOpsClass *ops = cpus_get_accel(); > + > cpu->singlestep_enabled = enabled; > - if (kvm_enabled()) { > - kvm_update_guest_debug(cpu, 0); > + > + if (ops->update_guest_debug) { > + ops->update_guest_debug(cpu, 0); Isn't this '0' flag here accelerator-specific? ... > } > + > trace_breakpoint_singlestep(cpu->cpu_index, enabled); > } > } > diff --git a/include/sysemu/accel-ops.h b/include/sysemu/accel-ops.h > index 8cc7996def..0a47a2f00c 100644 > --- a/include/sysemu/accel-ops.h > +++ b/include/sysemu/accel-ops.h > @@ -48,6 +48,7 @@ struct AccelOpsClass { > > /* gdbstub hooks */ > bool (*supports_guest_debug)(void); > + int (*update_guest_debug)(CPUState *cpu, unsigned long flags); ... if so the prototype should be: int (*update_guest_debug)(CPUState *cpu); and the '0' value set within kvm-accel-ops.c handler implementation. > int (*insert_breakpoint)(CPUState *cpu, int type, hwaddr addr, hwaddr len); > int (*remove_breakpoint)(CPUState *cpu, int type, hwaddr addr, hwaddr len); > void (*remove_all_breakpoints)(CPUState *cpu); Regards, Phil.
> Isn't this '0' flag here accelerator-specific? ... > ... if so the prototype should be: > > int (*update_guest_debug)(CPUState *cpu); > > and the '0' value set within kvm-accel-ops.c handler implementation. > You're right, we can avoid the additional variable. We'll then have to wrap `kvm_update_guest_debug`. Would the following be ok? diff --git a/accel/kvm/kvm-accel-ops.c b/accel/kvm/kvm-accel-ops.c index 6ebf9a644f..5e0fb42408 100644 --- a/accel/kvm/kvm-accel-ops.c +++ b/accel/kvm/kvm-accel-ops.c @@ -86,6 +86,10 @@ static bool kvm_cpus_are_resettable(void) return !kvm_enabled() || kvm_cpu_check_are_resettable(); } +static int kvm_update_guest_debug_ops(CPUState *cpu) { + return kvm_update_guest_debug(cpu, 0); +} + static void kvm_accel_ops_class_init(ObjectClass *oc, void *data) { AccelOpsClass *ops = ACCEL_OPS_CLASS(oc); @@ -99,7 +103,7 @@ static void kvm_accel_ops_class_init(ObjectClass *oc, void *data) ops->synchronize_pre_loadvm = kvm_cpu_synchronize_pre_loadvm; #ifdef KVM_CAP_SET_GUEST_DEBUG - ops->update_guest_debug = kvm_update_guest_debug; + ops->update_guest_debug = kvm_update_guest_debug_ops; ops->supports_guest_debug = kvm_supports_guest_debug; ops->insert_breakpoint = kvm_insert_breakpoint; ops->remove_breakpoint = kvm_remove_breakpoint; diff --git a/cpu.c b/cpu.c index ef433a79e3..b2ade96caa 100644 --- a/cpu.c +++ b/cpu.c @@ -383,7 +383,7 @@ void cpu_single_step(CPUState *cpu, int enabled) cpu->singlestep_enabled = enabled; if (ops->update_guest_debug) { - ops->update_guest_debug(cpu, 0); + ops->update_guest_debug(cpu); } trace_breakpoint_singlestep(cpu->cpu_index, enabled); diff --git a/include/sysemu/accel-ops.h b/include/sysemu/accel-ops.h index 0a47a2f00c..cd6a4ef7a5 100644 --- a/include/sysemu/accel-ops.h +++ b/include/sysemu/accel-ops.h @@ -48,7 +48,7 @@ struct AccelOpsClass { /* gdbstub hooks */ bool (*supports_guest_debug)(void); - int (*update_guest_debug)(CPUState *cpu, unsigned long flags); + int (*update_guest_debug)(CPUState *cpu); int (*insert_breakpoint)(CPUState *cpu, int type, hwaddr addr, hwaddr len); int (*remove_breakpoint)(CPUState *cpu, int type, hwaddr addr, hwaddr len); void (*remove_all_breakpoints)(CPUState *cpu); If you have a better name for `kvm_update_guest_debug_ops`, I'm open for suggestions. On a side note. When compiling for an arch that isn't the same as the system (i.e. aarch64 on x86_64), I'm getting a linker-error for cpu.c that `cpus_get_accel` isn't defined. Do I need to move `cpus_get_accel` or somehow #ifdef its use?
Mads Ynddal <mads@ynddal.dk> writes: >> Isn't this '0' flag here accelerator-specific? ... > >> ... if so the prototype should be: >> >> int (*update_guest_debug)(CPUState *cpu); >> >> and the '0' value set within kvm-accel-ops.c handler implementation. >> > > You're right, we can avoid the additional variable. We'll then have to wrap > `kvm_update_guest_debug`. Would the following be ok? > > diff --git a/accel/kvm/kvm-accel-ops.c b/accel/kvm/kvm-accel-ops.c > index 6ebf9a644f..5e0fb42408 100644 > --- a/accel/kvm/kvm-accel-ops.c > +++ b/accel/kvm/kvm-accel-ops.c > @@ -86,6 +86,10 @@ static bool kvm_cpus_are_resettable(void) > return !kvm_enabled() || kvm_cpu_check_are_resettable(); > } > > +static int kvm_update_guest_debug_ops(CPUState *cpu) { > + return kvm_update_guest_debug(cpu, 0); > +} > + > static void kvm_accel_ops_class_init(ObjectClass *oc, void *data) > { > AccelOpsClass *ops = ACCEL_OPS_CLASS(oc); > @@ -99,7 +103,7 @@ static void kvm_accel_ops_class_init(ObjectClass *oc, void *data) > ops->synchronize_pre_loadvm = kvm_cpu_synchronize_pre_loadvm; > > #ifdef KVM_CAP_SET_GUEST_DEBUG > - ops->update_guest_debug = kvm_update_guest_debug; > + ops->update_guest_debug = kvm_update_guest_debug_ops; > ops->supports_guest_debug = kvm_supports_guest_debug; > ops->insert_breakpoint = kvm_insert_breakpoint; > ops->remove_breakpoint = kvm_remove_breakpoint; > diff --git a/cpu.c b/cpu.c > index ef433a79e3..b2ade96caa 100644 > --- a/cpu.c > +++ b/cpu.c > @@ -383,7 +383,7 @@ void cpu_single_step(CPUState *cpu, int enabled) > cpu->singlestep_enabled = enabled; > > if (ops->update_guest_debug) { > - ops->update_guest_debug(cpu, 0); > + ops->update_guest_debug(cpu); > } > > trace_breakpoint_singlestep(cpu->cpu_index, enabled); > diff --git a/include/sysemu/accel-ops.h b/include/sysemu/accel-ops.h > index 0a47a2f00c..cd6a4ef7a5 100644 > --- a/include/sysemu/accel-ops.h > +++ b/include/sysemu/accel-ops.h > @@ -48,7 +48,7 @@ struct AccelOpsClass { > > /* gdbstub hooks */ > bool (*supports_guest_debug)(void); > - int (*update_guest_debug)(CPUState *cpu, unsigned long flags); > + int (*update_guest_debug)(CPUState *cpu); > int (*insert_breakpoint)(CPUState *cpu, int type, hwaddr addr, hwaddr len); > int (*remove_breakpoint)(CPUState *cpu, int type, hwaddr addr, hwaddr len); > void (*remove_all_breakpoints)(CPUState *cpu); > > > If you have a better name for `kvm_update_guest_debug_ops`, I'm open for > suggestions. It will do. You could just call it update_guest_debug as it is an internal static function although I guess that makes grepping a bit of a pain. > On a side note. When compiling for an arch that isn't the same as the system > (i.e. aarch64 on x86_64), I'm getting a linker-error for cpu.c that > `cpus_get_accel` isn't defined. Do I need to move `cpus_get_accel` or somehow > #ifdef its use? Is something being accidentally linked with linux-user and softmmu?
> It will do. You could just call it update_guest_debug as it is an > internal static function although I guess that makes grepping a bit of a > pain. I agree. It should preferably be something unique, to ease grep'ing. > Is something being accidentally linked with linux-user and softmmu? Good question. I'm not familiar enough with the code base to know. I experimented with enabling/disabling linux-user when configuring, and it does affect whether it compiles or not. The following seems to fix it, and I can see the same approach is taken other places in cpu.c. Would this be an acceptable solution? diff --git a/cpu.c b/cpu.c index 6effa5acc9..c9e8700691 100644 --- a/cpu.c +++ b/cpu.c @@ -386,6 +386,7 @@ void cpu_breakpoint_remove_all(CPUState *cpu, int mask) void cpu_single_step(CPUState *cpu, int enabled) { if (cpu->singlestep_enabled != enabled) { +#if !defined(CONFIG_USER_ONLY) const AccelOpsClass *ops = cpus_get_accel(); cpu->singlestep_enabled = enabled; @@ -393,6 +394,7 @@ void cpu_single_step(CPUState *cpu, int enabled) if (ops->update_guest_debug) { ops->update_guest_debug(cpu, 0); } +#endif trace_breakpoint_singlestep(cpu->cpu_index, enabled); } — Mads Ynddal
Mads Ynddal <mads@ynddal.dk> writes: >> It will do. You could just call it update_guest_debug as it is an >> internal static function although I guess that makes grepping a bit of a >> pain. > > I agree. It should preferably be something unique, to ease grep'ing. > >> Is something being accidentally linked with linux-user and softmmu? > > Good question. I'm not familiar enough with the code base to know. > > I experimented with enabling/disabling linux-user when configuring, and it does > affect whether it compiles or not. > > The following seems to fix it, and I can see the same approach is taken other > places in cpu.c. Would this be an acceptable solution? > > diff --git a/cpu.c b/cpu.c > index 6effa5acc9..c9e8700691 100644 > --- a/cpu.c > +++ b/cpu.c > @@ -386,6 +386,7 @@ void cpu_breakpoint_remove_all(CPUState *cpu, int mask) > void cpu_single_step(CPUState *cpu, int enabled) > { > if (cpu->singlestep_enabled != enabled) { > +#if !defined(CONFIG_USER_ONLY) > const AccelOpsClass *ops = cpus_get_accel(); > > cpu->singlestep_enabled = enabled; > @@ -393,6 +394,7 @@ void cpu_single_step(CPUState *cpu, int enabled) > if (ops->update_guest_debug) { > ops->update_guest_debug(cpu, 0); > } > +#endif > > trace_breakpoint_singlestep(cpu->cpu_index, enabled); > } Sorry this dropped of my radar. Yes I think the ifdef will do. Are you going to post a v2 with all the various updates?
> > Sorry this dropped of my radar. Yes I think the ifdef will do. Are you > going to post a v2 with all the various updates? > No worries, I'll make a v2 with the changes. — Mads Ynddal
diff --git a/accel/kvm/kvm-accel-ops.c b/accel/kvm/kvm-accel-ops.c index fbf4fe3497..6ebf9a644f 100644 --- a/accel/kvm/kvm-accel-ops.c +++ b/accel/kvm/kvm-accel-ops.c @@ -99,6 +99,7 @@ static void kvm_accel_ops_class_init(ObjectClass *oc, void *data) ops->synchronize_pre_loadvm = kvm_cpu_synchronize_pre_loadvm; #ifdef KVM_CAP_SET_GUEST_DEBUG + ops->update_guest_debug = kvm_update_guest_debug; ops->supports_guest_debug = kvm_supports_guest_debug; ops->insert_breakpoint = kvm_insert_breakpoint; ops->remove_breakpoint = kvm_remove_breakpoint; diff --git a/cpu.c b/cpu.c index 2a09b05205..ef433a79e3 100644 --- a/cpu.c +++ b/cpu.c @@ -31,8 +31,8 @@ #include "hw/core/sysemu-cpu-ops.h" #include "exec/address-spaces.h" #endif +#include "sysemu/cpus.h" #include "sysemu/tcg.h" -#include "sysemu/kvm.h" #include "sysemu/replay.h" #include "exec/cpu-common.h" #include "exec/exec-all.h" @@ -378,10 +378,14 @@ void cpu_breakpoint_remove_all(CPUState *cpu, int mask) void cpu_single_step(CPUState *cpu, int enabled) { if (cpu->singlestep_enabled != enabled) { + const AccelOpsClass *ops = cpus_get_accel(); + cpu->singlestep_enabled = enabled; - if (kvm_enabled()) { - kvm_update_guest_debug(cpu, 0); + + if (ops->update_guest_debug) { + ops->update_guest_debug(cpu, 0); } + trace_breakpoint_singlestep(cpu->cpu_index, enabled); } } diff --git a/include/sysemu/accel-ops.h b/include/sysemu/accel-ops.h index 8cc7996def..0a47a2f00c 100644 --- a/include/sysemu/accel-ops.h +++ b/include/sysemu/accel-ops.h @@ -48,6 +48,7 @@ struct AccelOpsClass { /* gdbstub hooks */ bool (*supports_guest_debug)(void); + int (*update_guest_debug)(CPUState *cpu, unsigned long flags); int (*insert_breakpoint)(CPUState *cpu, int type, hwaddr addr, hwaddr len); int (*remove_breakpoint)(CPUState *cpu, int type, hwaddr addr, hwaddr len); void (*remove_all_breakpoints)(CPUState *cpu);