Re: [RFC PATCH 6/7] accel/hvf: Use CPU_FOREACH_HVF()

[BALATON Zoltan]

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