| Message ID | 20250513173928.77376-17-philmd@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | target/arm: More header rework around arm_feature() & multiprocessing.h | expand |
On 5/13/25 18:39, Philippe Mathieu-Daudé wrote: > arm_cpu_has_feature() is equivalent of arm_feature(), however > while the latter uses CPUARMState so is target-specific, the > former doesn't and can be called by target-agnostic code in hw/. CPUARMState is no more target-specific than ARMCPU. Did you really mean to use CPUState? Or is it merely that arm_cpu_has_feature is out-of-line? r~ > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> > Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org> > --- > target/arm/cpu_has_feature.h | 2 ++ > target/arm/cpu.c | 7 +++++++ > 2 files changed, 9 insertions(+) > > diff --git a/target/arm/cpu_has_feature.h b/target/arm/cpu_has_feature.h > index 2adfccd9208..352f9d75bed 100644 > --- a/target/arm/cpu_has_feature.h > +++ b/target/arm/cpu_has_feature.h > @@ -62,4 +62,6 @@ typedef enum arm_features { > ARM_FEATURE_BACKCOMPAT_CNTFRQ, /* 62.5MHz timer default */ > } ArmCpuFeature; > > +bool arm_cpu_has_feature(ARMCPU *cpu, ArmCpuFeature feature); > + > #endif > diff --git a/target/arm/cpu.c b/target/arm/cpu.c > index 8c9d161f2ef..759636a3b0e 100644 > --- a/target/arm/cpu.c > +++ b/target/arm/cpu.c > @@ -54,6 +54,13 @@ > #include "target/arm/gtimer.h" > #include "target/arm/multiprocessing.h" > > +bool arm_cpu_has_feature(ARMCPU *cpu, ArmCpuFeature feature) > +{ > + CPUARMState *env = &cpu->env; > + > + return arm_feature(env, feature); > +} > + > static void arm_cpu_set_pc(CPUState *cs, vaddr value) > { > ARMCPU *cpu = ARM_CPU(cs);
On 14/5/25 10:24, Richard Henderson wrote: > On 5/13/25 18:39, Philippe Mathieu-Daudé wrote: >> arm_cpu_has_feature() is equivalent of arm_feature(), however >> while the latter uses CPUARMState so is target-specific, the >> former doesn't and can be called by target-agnostic code in hw/. > > CPUARMState is no more target-specific than ARMCPU. ARMCPU is forward-declared as opaque pointer in target/arm/cpu-qom.h, so we can expose prototypes using it to non-ARM units. CPUARMState is only declared in "cpu.h", itself only accessible by ARM-related units. > > Did you really mean to use CPUState? > Or is it merely that arm_cpu_has_feature is out-of-line? > > > r~ > >> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> >> Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org> >> --- >> target/arm/cpu_has_feature.h | 2 ++ >> target/arm/cpu.c | 7 +++++++ >> 2 files changed, 9 insertions(+) >> >> diff --git a/target/arm/cpu_has_feature.h b/target/arm/cpu_has_feature.h >> index 2adfccd9208..352f9d75bed 100644 >> --- a/target/arm/cpu_has_feature.h >> +++ b/target/arm/cpu_has_feature.h >> @@ -62,4 +62,6 @@ typedef enum arm_features { >> ARM_FEATURE_BACKCOMPAT_CNTFRQ, /* 62.5MHz timer default */ >> } ArmCpuFeature; >> +bool arm_cpu_has_feature(ARMCPU *cpu, ArmCpuFeature feature); >> + >> #endif >> diff --git a/target/arm/cpu.c b/target/arm/cpu.c >> index 8c9d161f2ef..759636a3b0e 100644 >> --- a/target/arm/cpu.c >> +++ b/target/arm/cpu.c >> @@ -54,6 +54,13 @@ >> #include "target/arm/gtimer.h" >> #include "target/arm/multiprocessing.h" >> +bool arm_cpu_has_feature(ARMCPU *cpu, ArmCpuFeature feature) >> +{ >> + CPUARMState *env = &cpu->env; >> + >> + return arm_feature(env, feature); >> +} >> + >> static void arm_cpu_set_pc(CPUState *cs, vaddr value) >> { >> ARMCPU *cpu = ARM_CPU(cs); >
On 5/14/25 9:53 AM, Philippe Mathieu-Daudé wrote: > On 14/5/25 10:24, Richard Henderson wrote: >> On 5/13/25 18:39, Philippe Mathieu-Daudé wrote: >>> arm_cpu_has_feature() is equivalent of arm_feature(), however >>> while the latter uses CPUARMState so is target-specific, the >>> former doesn't and can be called by target-agnostic code in hw/. >> >> CPUARMState is no more target-specific than ARMCPU. > > ARMCPU is forward-declared as opaque pointer in target/arm/cpu-qom.h, > so we can expose prototypes using it to non-ARM units. > CPUARMState is only declared in "cpu.h", itself only accessible by > ARM-related units. > Maybe we can simply postpone introduction of arm_cpu_has_feature() when it will be really needed. Patches 17 and 18 are not strictly needed, as cpu.h (which resolves to target/arm/cpu.h implicitely) is perfectly accessible to code in hw/arm without any problem. I asked in the past if there was a real need for this function, but didn't have a clear answer on where it's mandatory. In this series, it's an optional change. That said, I don't want block any work arguing over this, so if you feel it's better to have it, so be it, and let's pull this. >> >> Did you really mean to use CPUState? >> Or is it merely that arm_cpu_has_feature is out-of-line? >> >> >> r~ >> >>> >>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> >>> Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org> >>> --- >>> target/arm/cpu_has_feature.h | 2 ++ >>> target/arm/cpu.c | 7 +++++++ >>> 2 files changed, 9 insertions(+) >>> >>> diff --git a/target/arm/cpu_has_feature.h b/target/arm/cpu_has_feature.h >>> index 2adfccd9208..352f9d75bed 100644 >>> --- a/target/arm/cpu_has_feature.h >>> +++ b/target/arm/cpu_has_feature.h >>> @@ -62,4 +62,6 @@ typedef enum arm_features { >>> ARM_FEATURE_BACKCOMPAT_CNTFRQ, /* 62.5MHz timer default */ >>> } ArmCpuFeature; >>> +bool arm_cpu_has_feature(ARMCPU *cpu, ArmCpuFeature feature); >>> + >>> #endif >>> diff --git a/target/arm/cpu.c b/target/arm/cpu.c >>> index 8c9d161f2ef..759636a3b0e 100644 >>> --- a/target/arm/cpu.c >>> +++ b/target/arm/cpu.c >>> @@ -54,6 +54,13 @@ >>> #include "target/arm/gtimer.h" >>> #include "target/arm/multiprocessing.h" >>> +bool arm_cpu_has_feature(ARMCPU *cpu, ArmCpuFeature feature) >>> +{ >>> + CPUARMState *env = &cpu->env; >>> + >>> + return arm_feature(env, feature); >>> +} >>> + >>> static void arm_cpu_set_pc(CPUState *cs, vaddr value) >>> { >>> ARMCPU *cpu = ARM_CPU(cs); >> >
On 14/5/25 18:59, Pierrick Bouvier wrote: > On 5/14/25 9:53 AM, Philippe Mathieu-Daudé wrote: >> On 14/5/25 10:24, Richard Henderson wrote: >>> On 5/13/25 18:39, Philippe Mathieu-Daudé wrote: >>>> arm_cpu_has_feature() is equivalent of arm_feature(), however >>>> while the latter uses CPUARMState so is target-specific, the >>>> former doesn't and can be called by target-agnostic code in hw/. >>> >>> CPUARMState is no more target-specific than ARMCPU. >> >> ARMCPU is forward-declared as opaque pointer in target/arm/cpu-qom.h, >> so we can expose prototypes using it to non-ARM units. >> CPUARMState is only declared in "cpu.h", itself only accessible by >> ARM-related units. >> > > Maybe we can simply postpone introduction of arm_cpu_has_feature() when > it will be really needed. > > Patches 17 and 18 are not strictly needed, as cpu.h (which resolves to > target/arm/cpu.h implicitely) is perfectly accessible to code in hw/arm > without any problem. OK. Peter, would you be OK to take reviewed patches #1 up to #15 (the previous one) or do you rather I respin them? Regards, Phil.
On 5/15/25 4:05 AM, Philippe Mathieu-Daudé wrote: > On 14/5/25 18:59, Pierrick Bouvier wrote: >> On 5/14/25 9:53 AM, Philippe Mathieu-Daudé wrote: >>> On 14/5/25 10:24, Richard Henderson wrote: >>>> On 5/13/25 18:39, Philippe Mathieu-Daudé wrote: >>>>> arm_cpu_has_feature() is equivalent of arm_feature(), however >>>>> while the latter uses CPUARMState so is target-specific, the >>>>> former doesn't and can be called by target-agnostic code in hw/. >>>> >>>> CPUARMState is no more target-specific than ARMCPU. >>> >>> ARMCPU is forward-declared as opaque pointer in target/arm/cpu-qom.h, >>> so we can expose prototypes using it to non-ARM units. >>> CPUARMState is only declared in "cpu.h", itself only accessible by >>> ARM-related units. >>> >> >> Maybe we can simply postpone introduction of arm_cpu_has_feature() when >> it will be really needed. >> >> Patches 17 and 18 are not strictly needed, as cpu.h (which resolves to >> target/arm/cpu.h implicitely) is perfectly accessible to code in hw/arm >> without any problem. > > OK. > > Peter, would you be OK to take reviewed patches #1 up to #15 (the > previous one) or do you rather I respin them? > In case you respin, feel free to include the base series, so we can combine both. > Regards, > > Phil.
On 15/5/25 17:22, Pierrick Bouvier wrote: > On 5/15/25 4:05 AM, Philippe Mathieu-Daudé wrote: >> On 14/5/25 18:59, Pierrick Bouvier wrote: >>> On 5/14/25 9:53 AM, Philippe Mathieu-Daudé wrote: >>>> On 14/5/25 10:24, Richard Henderson wrote: >>>>> On 5/13/25 18:39, Philippe Mathieu-Daudé wrote: >>>>>> arm_cpu_has_feature() is equivalent of arm_feature(), however >>>>>> while the latter uses CPUARMState so is target-specific, the >>>>>> former doesn't and can be called by target-agnostic code in hw/. >>>>> >>>>> CPUARMState is no more target-specific than ARMCPU. >>>> >>>> ARMCPU is forward-declared as opaque pointer in target/arm/cpu-qom.h, >>>> so we can expose prototypes using it to non-ARM units. >>>> CPUARMState is only declared in "cpu.h", itself only accessible by >>>> ARM-related units. >>>> >>> >>> Maybe we can simply postpone introduction of arm_cpu_has_feature() when >>> it will be really needed. >>> >>> Patches 17 and 18 are not strictly needed, as cpu.h (which resolves to >>> target/arm/cpu.h implicitely) is perfectly accessible to code in hw/arm >>> without any problem. >> >> OK. >> >> Peter, would you be OK to take reviewed patches #1 up to #15 (the >> previous one) or do you rather I respin them? >> > > In case you respin, feel free to include the base series, so we can > combine both. Isn't the base already pulled in by Peter? I thought it was: https://lore.kernel.org/qemu-devel/20250515102546.2149601-1-peter.maydell@linaro.org/
On 5/15/25 9:14 AM, Philippe Mathieu-Daudé wrote: > On 15/5/25 17:22, Pierrick Bouvier wrote: >> On 5/15/25 4:05 AM, Philippe Mathieu-Daudé wrote: >>> On 14/5/25 18:59, Pierrick Bouvier wrote: >>>> On 5/14/25 9:53 AM, Philippe Mathieu-Daudé wrote: >>>>> On 14/5/25 10:24, Richard Henderson wrote: >>>>>> On 5/13/25 18:39, Philippe Mathieu-Daudé wrote: >>>>>>> arm_cpu_has_feature() is equivalent of arm_feature(), however >>>>>>> while the latter uses CPUARMState so is target-specific, the >>>>>>> former doesn't and can be called by target-agnostic code in hw/. >>>>>> >>>>>> CPUARMState is no more target-specific than ARMCPU. >>>>> >>>>> ARMCPU is forward-declared as opaque pointer in target/arm/cpu-qom.h, >>>>> so we can expose prototypes using it to non-ARM units. >>>>> CPUARMState is only declared in "cpu.h", itself only accessible by >>>>> ARM-related units. >>>>> >>>> >>>> Maybe we can simply postpone introduction of arm_cpu_has_feature() when >>>> it will be really needed. >>>> >>>> Patches 17 and 18 are not strictly needed, as cpu.h (which resolves to >>>> target/arm/cpu.h implicitely) is perfectly accessible to code in hw/arm >>>> without any problem. >>> >>> OK. >>> >>> Peter, would you be OK to take reviewed patches #1 up to #15 (the >>> previous one) or do you rather I respin them? >>> >> >> In case you respin, feel free to include the base series, so we can >> combine both. > > Isn't the base already pulled in by Peter? I thought it was: > Oh, I missed it, thanks. > https://lore.kernel.org/qemu-devel/20250515102546.2149601-1-peter.maydell@linaro.org/
diff --git a/target/arm/cpu_has_feature.h b/target/arm/cpu_has_feature.h index 2adfccd9208..352f9d75bed 100644 --- a/target/arm/cpu_has_feature.h +++ b/target/arm/cpu_has_feature.h @@ -62,4 +62,6 @@ typedef enum arm_features { ARM_FEATURE_BACKCOMPAT_CNTFRQ, /* 62.5MHz timer default */ } ArmCpuFeature; +bool arm_cpu_has_feature(ARMCPU *cpu, ArmCpuFeature feature); + #endif diff --git a/target/arm/cpu.c b/target/arm/cpu.c index 8c9d161f2ef..759636a3b0e 100644 --- a/target/arm/cpu.c +++ b/target/arm/cpu.c @@ -54,6 +54,13 @@ #include "target/arm/gtimer.h" #include "target/arm/multiprocessing.h" +bool arm_cpu_has_feature(ARMCPU *cpu, ArmCpuFeature feature) +{ + CPUARMState *env = &cpu->env; + + return arm_feature(env, feature); +} + static void arm_cpu_set_pc(CPUState *cs, vaddr value) { ARMCPU *cpu = ARM_CPU(cs);