| Message ID | 20250106200258.37008-7-philmd@linaro.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | accel: Add per-accelerator vCPUs queue | expand |
On 1/6/25 5:02 PM, Philippe Mathieu-Daudé wrote: > Only iterate over HVF vCPUs when running HVF specific code. > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> > --- > include/system/hvf_int.h | 4 ++++ > accel/hvf/hvf-accel-ops.c | 9 +++++---- > target/arm/hvf/hvf.c | 4 ++-- > 3 files changed, 11 insertions(+), 6 deletions(-) > > diff --git a/include/system/hvf_int.h b/include/system/hvf_int.h > index 42ae18433f0..3cf64faabd1 100644 > --- a/include/system/hvf_int.h > +++ b/include/system/hvf_int.h > @@ -11,6 +11,8 @@ > #ifndef HVF_INT_H > #define HVF_INT_H > > +#include "system/hw_accel.h" > + > #ifdef __aarch64__ > #include <Hypervisor/Hypervisor.h> > typedef hv_vcpu_t hvf_vcpuid; > @@ -74,4 +76,6 @@ int hvf_put_registers(CPUState *); > int hvf_get_registers(CPUState *); > void hvf_kick_vcpu_thread(CPUState *cpu); > > +#define CPU_FOREACH_HVF(cpu) CPU_FOREACH_HWACCEL(cpu) Cosmetic comment: given that this is HVF specific code and we only support one hw accelerator at a time, I'd skip this alias and use CPU_FOREACH_HWACCEL(cpu) directly. It would make it easier when grepping to see where and how the macro is being used. Same thing in the next patch with CPU_FOREACH_KVM(). LGTM otherwise. Thanks, Daniel > + > #endif > diff --git a/accel/hvf/hvf-accel-ops.c b/accel/hvf/hvf-accel-ops.c > index 945ba720513..bbbe2f8d45b 100644 > --- a/accel/hvf/hvf-accel-ops.c > +++ b/accel/hvf/hvf-accel-ops.c > @@ -504,7 +504,7 @@ static int hvf_insert_breakpoint(CPUState *cpu, int type, vaddr addr, vaddr len) > } > } > > - CPU_FOREACH(cpu) { > + CPU_FOREACH_HVF(cpu) { > err = hvf_update_guest_debug(cpu); > if (err) { > return err; > @@ -543,7 +543,7 @@ static int hvf_remove_breakpoint(CPUState *cpu, int type, vaddr addr, vaddr len) > } > } > > - CPU_FOREACH(cpu) { > + CPU_FOREACH_HVF(cpu) { > err = hvf_update_guest_debug(cpu); > if (err) { > return err; > @@ -560,7 +560,7 @@ static void hvf_remove_all_breakpoints(CPUState *cpu) > QTAILQ_FOREACH_SAFE(bp, &hvf_state->hvf_sw_breakpoints, entry, next) { > if (hvf_arch_remove_sw_breakpoint(cpu, bp) != 0) { > /* Try harder to find a CPU that currently sees the breakpoint. */ > - CPU_FOREACH(tmpcpu) > + CPU_FOREACH_HVF(tmpcpu) > { > if (hvf_arch_remove_sw_breakpoint(tmpcpu, bp) == 0) { > break; > @@ -572,7 +572,7 @@ static void hvf_remove_all_breakpoints(CPUState *cpu) > } > hvf_arch_remove_all_hw_breakpoints(); > > - CPU_FOREACH(cpu) { > + CPU_FOREACH_HVF(cpu) { > hvf_update_guest_debug(cpu); > } > } > @@ -581,6 +581,7 @@ static void hvf_accel_ops_class_init(ObjectClass *oc, void *data) > { > AccelOpsClass *ops = ACCEL_OPS_CLASS(oc); > > + ops->get_cpus_queue = hw_accel_get_cpus_queue; > ops->create_vcpu_thread = hvf_start_vcpu_thread; > ops->kick_vcpu_thread = hvf_kick_vcpu_thread; > > diff --git a/target/arm/hvf/hvf.c b/target/arm/hvf/hvf.c > index 0afd96018e0..13400ff0d5f 100644 > --- a/target/arm/hvf/hvf.c > +++ b/target/arm/hvf/hvf.c > @@ -2269,10 +2269,10 @@ static void hvf_arch_set_traps(void) > > /* Check whether guest debugging is enabled for at least one vCPU; if it > * is, enable exiting the guest on all vCPUs */ > - CPU_FOREACH(cpu) { > + CPU_FOREACH_HVF(cpu) { > should_enable_traps |= cpu->accel->guest_debug_enabled; > } > - CPU_FOREACH(cpu) { > + CPU_FOREACH_HVF(cpu) { > /* Set whether debug exceptions exit the guest */ > r = hv_vcpu_set_trap_debug_exceptions(cpu->accel->fd, > should_enable_traps);
On 6/1/25 21:33, Daniel Henrique Barboza wrote: > > > On 1/6/25 5:02 PM, Philippe Mathieu-Daudé wrote: >> Only iterate over HVF vCPUs when running HVF specific code. >> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> >> --- >> include/system/hvf_int.h | 4 ++++ >> accel/hvf/hvf-accel-ops.c | 9 +++++---- >> target/arm/hvf/hvf.c | 4 ++-- >> 3 files changed, 11 insertions(+), 6 deletions(-) >> >> diff --git a/include/system/hvf_int.h b/include/system/hvf_int.h >> index 42ae18433f0..3cf64faabd1 100644 >> --- a/include/system/hvf_int.h >> +++ b/include/system/hvf_int.h >> @@ -11,6 +11,8 @@ >> #ifndef HVF_INT_H >> #define HVF_INT_H >> +#include "system/hw_accel.h" >> + >> #ifdef __aarch64__ >> #include <Hypervisor/Hypervisor.h> >> typedef hv_vcpu_t hvf_vcpuid; >> @@ -74,4 +76,6 @@ int hvf_put_registers(CPUState *); >> int hvf_get_registers(CPUState *); >> void hvf_kick_vcpu_thread(CPUState *cpu); >> +#define CPU_FOREACH_HVF(cpu) CPU_FOREACH_HWACCEL(cpu) > > > Cosmetic comment: given that this is HVF specific code and we only > support one hw > accelerator at a time, I'd skip this alias and use > CPU_FOREACH_HWACCEL(cpu) directly. > It would make it easier when grepping to see where and how the macro is > being used. I find it more useful to grep for a particular accelerator, or for all of them: $ git grep CPU_FOREACH_ accel/hvf/hvf-accel-ops.c:507: CPU_FOREACH_HVF(cpu) { accel/hvf/hvf-accel-ops.c:546: CPU_FOREACH_HVF(cpu) { accel/kvm/kvm-all.c:875: CPU_FOREACH_KVM(cpu) { accel/kvm/kvm-all.c:938: CPU_FOREACH_KVM(cpu) { accel/tcg/cputlb.c:372: CPU_FOREACH_TCG(cpu) { accel/tcg/cputlb.c:650: CPU_FOREACH_TCG(dst_cpu) {
On Mon, 6 Jan 2025, Philippe Mathieu-Daudé wrote: > On 6/1/25 21:33, Daniel Henrique Barboza wrote: >> >> >> On 1/6/25 5:02 PM, Philippe Mathieu-Daudé wrote: >>> Only iterate over HVF vCPUs when running HVF specific code. >>> >>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> >>> --- >>> include/system/hvf_int.h | 4 ++++ >>> accel/hvf/hvf-accel-ops.c | 9 +++++---- >>> target/arm/hvf/hvf.c | 4 ++-- >>> 3 files changed, 11 insertions(+), 6 deletions(-) >>> >>> diff --git a/include/system/hvf_int.h b/include/system/hvf_int.h >>> index 42ae18433f0..3cf64faabd1 100644 >>> --- a/include/system/hvf_int.h >>> +++ b/include/system/hvf_int.h >>> @@ -11,6 +11,8 @@ >>> #ifndef HVF_INT_H >>> #define HVF_INT_H >>> +#include "system/hw_accel.h" >>> + >>> #ifdef __aarch64__ >>> #include <Hypervisor/Hypervisor.h> >>> typedef hv_vcpu_t hvf_vcpuid; >>> @@ -74,4 +76,6 @@ int hvf_put_registers(CPUState *); >>> int hvf_get_registers(CPUState *); >>> void hvf_kick_vcpu_thread(CPUState *cpu); >>> +#define CPU_FOREACH_HVF(cpu) CPU_FOREACH_HWACCEL(cpu) >> >> >> Cosmetic comment: given that this is HVF specific code and we only support >> one hw >> accelerator at a time, I'd skip this alias and use CPU_FOREACH_HWACCEL(cpu) >> directly. >> It would make it easier when grepping to see where and how the macro is >> being used. > > I find it more useful to grep for a particular accelerator, or for > all of them: > > $ git grep CPU_FOREACH_ > accel/hvf/hvf-accel-ops.c:507: CPU_FOREACH_HVF(cpu) { > accel/hvf/hvf-accel-ops.c:546: CPU_FOREACH_HVF(cpu) { > accel/kvm/kvm-all.c:875: CPU_FOREACH_KVM(cpu) { > accel/kvm/kvm-all.c:938: CPU_FOREACH_KVM(cpu) { > accel/tcg/cputlb.c:372: CPU_FOREACH_TCG(cpu) { > accel/tcg/cputlb.c:650: CPU_FOREACH_TCG(dst_cpu) { But then you need to define a new macro for every new accelerator. Maybe it's simpler to have CPU_FOREACH take the queue as a parameter so no separate macro is needed for each accel (and they cannot get inconsistent by changing only one of them in the future). Regards, BALATON Zoltan
diff --git a/include/system/hvf_int.h b/include/system/hvf_int.h index 42ae18433f0..3cf64faabd1 100644 --- a/include/system/hvf_int.h +++ b/include/system/hvf_int.h @@ -11,6 +11,8 @@ #ifndef HVF_INT_H #define HVF_INT_H +#include "system/hw_accel.h" + #ifdef __aarch64__ #include <Hypervisor/Hypervisor.h> typedef hv_vcpu_t hvf_vcpuid; @@ -74,4 +76,6 @@ int hvf_put_registers(CPUState *); int hvf_get_registers(CPUState *); void hvf_kick_vcpu_thread(CPUState *cpu); +#define CPU_FOREACH_HVF(cpu) CPU_FOREACH_HWACCEL(cpu) + #endif diff --git a/accel/hvf/hvf-accel-ops.c b/accel/hvf/hvf-accel-ops.c index 945ba720513..bbbe2f8d45b 100644 --- a/accel/hvf/hvf-accel-ops.c +++ b/accel/hvf/hvf-accel-ops.c @@ -504,7 +504,7 @@ static int hvf_insert_breakpoint(CPUState *cpu, int type, vaddr addr, vaddr len) } } - CPU_FOREACH(cpu) { + CPU_FOREACH_HVF(cpu) { err = hvf_update_guest_debug(cpu); if (err) { return err; @@ -543,7 +543,7 @@ static int hvf_remove_breakpoint(CPUState *cpu, int type, vaddr addr, vaddr len) } } - CPU_FOREACH(cpu) { + CPU_FOREACH_HVF(cpu) { err = hvf_update_guest_debug(cpu); if (err) { return err; @@ -560,7 +560,7 @@ static void hvf_remove_all_breakpoints(CPUState *cpu) QTAILQ_FOREACH_SAFE(bp, &hvf_state->hvf_sw_breakpoints, entry, next) { if (hvf_arch_remove_sw_breakpoint(cpu, bp) != 0) { /* Try harder to find a CPU that currently sees the breakpoint. */ - CPU_FOREACH(tmpcpu) + CPU_FOREACH_HVF(tmpcpu) { if (hvf_arch_remove_sw_breakpoint(tmpcpu, bp) == 0) { break; @@ -572,7 +572,7 @@ static void hvf_remove_all_breakpoints(CPUState *cpu) } hvf_arch_remove_all_hw_breakpoints(); - CPU_FOREACH(cpu) { + CPU_FOREACH_HVF(cpu) { hvf_update_guest_debug(cpu); } } @@ -581,6 +581,7 @@ static void hvf_accel_ops_class_init(ObjectClass *oc, void *data) { AccelOpsClass *ops = ACCEL_OPS_CLASS(oc); + ops->get_cpus_queue = hw_accel_get_cpus_queue; ops->create_vcpu_thread = hvf_start_vcpu_thread; ops->kick_vcpu_thread = hvf_kick_vcpu_thread; diff --git a/target/arm/hvf/hvf.c b/target/arm/hvf/hvf.c index 0afd96018e0..13400ff0d5f 100644 --- a/target/arm/hvf/hvf.c +++ b/target/arm/hvf/hvf.c @@ -2269,10 +2269,10 @@ static void hvf_arch_set_traps(void) /* Check whether guest debugging is enabled for at least one vCPU; if it * is, enable exiting the guest on all vCPUs */ - CPU_FOREACH(cpu) { + CPU_FOREACH_HVF(cpu) { should_enable_traps |= cpu->accel->guest_debug_enabled; } - CPU_FOREACH(cpu) { + CPU_FOREACH_HVF(cpu) { /* Set whether debug exceptions exit the guest */ r = hv_vcpu_set_trap_debug_exceptions(cpu->accel->fd, should_enable_traps);
Only iterate over HVF vCPUs when running HVF specific code. Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> --- include/system/hvf_int.h | 4 ++++ accel/hvf/hvf-accel-ops.c | 9 +++++---- target/arm/hvf/hvf.c | 4 ++-- 3 files changed, 11 insertions(+), 6 deletions(-)