| Message ID | 20200316160634.3386-2-philmd@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Support disabling TCG on ARM (part 2) | expand |
On 3/16/20 9:06 AM, Philippe Mathieu-Daudé wrote: > +++ b/target/arm/kvm32.c > @@ -22,7 +22,7 @@ > #include "internals.h" > #include "qemu/log.h" > > -static inline void set_feature(uint64_t *features, int feature) > +static inline void kvm_set_feature(uint64_t *features, int feature) Why, what's wrong with the existing name? Plus, with patch 2, you can just remove these. r~
On 3/16/20 9:16 PM, Richard Henderson wrote: > On 3/16/20 9:06 AM, Philippe Mathieu-Daudé wrote: >> +++ b/target/arm/kvm32.c >> @@ -22,7 +22,7 @@ >> #include "internals.h" >> #include "qemu/log.h" >> >> -static inline void set_feature(uint64_t *features, int feature) >> +static inline void kvm_set_feature(uint64_t *features, int feature) > > Why, what's wrong with the existing name? > Plus, with patch 2, you can just remove these. The prototypes are different: void set_feature(uint64_t *features, int feature) void set_feature(CPUARMState *env, int feature) Anyway you are right, I'll use the later prototype instead, thanks.
On 3/17/20 10:09 AM, Philippe Mathieu-Daudé wrote: > On 3/16/20 9:16 PM, Richard Henderson wrote: >> On 3/16/20 9:06 AM, Philippe Mathieu-Daudé wrote: >>> +++ b/target/arm/kvm32.c >>> @@ -22,7 +22,7 @@ >>> #include "internals.h" >>> #include "qemu/log.h" >>> -static inline void set_feature(uint64_t *features, int feature) >>> +static inline void kvm_set_feature(uint64_t *features, int feature) >> >> Why, what's wrong with the existing name? Peter suggested the rename here: https://www.mail-archive.com/qemu-devel@nongnu.org/msg641931.html >> Plus, with patch 2, you can just remove these. Since they don't have the same prototype, they clash: target/arm/kvm64.c:450:20: error: conflicting types for ‘set_feature’ static inline void set_feature(uint64_t *features, int feature) ^~~~~~~~~~~ In file included from target/arm/kvm64.c:30:0: target/arm/internals.h:30:20: note: previous definition of ‘set_feature’ was here static inline void set_feature(CPUARMState *env, int feature) ^~~~~~~~~~~ target/arm/kvm64.c:455:20: error: conflicting types for ‘unset_feature’ static inline void unset_feature(uint64_t *features, int feature) ^~~~~~~~~~~~~ In file included from target/arm/kvm64.c:30:0: target/arm/internals.h:35:20: note: previous definition of ‘unset_feature’ was here static inline void unset_feature(CPUARMState *env, int feature) ^~~~~~~~~~~~~ rules.mak:69: recipe for target 'target/arm/kvm64.o' failed make[1]: *** [target/arm/kvm64.o] Error 1 > > The prototypes are different: > > void set_feature(uint64_t *features, int feature) > > void set_feature(CPUARMState *env, int feature) > > Anyway you are right, I'll use the later prototype instead, thanks. There are ~180 uses of set_feature(CPUARMState,...) and 10 of set_feature(uint64_t,...) (kvm32:4 kvm64:6). We are going to remove kvm32, so replacing 180 set_feature(env) by set_feature(env->features) seems a waste. If you prefer to avoid renaming as kvm_set_feature() another option is to move the declaration in a local "features.h" header that would not be included by kvm*.c. The main problem is the use of the ARMHostCPUFeatures structure which apparently was introduced similar to a CPUClass (commit a96c0514ab7) then lost this in commit c4487d76d52.
On Sun, 19 Apr 2020 at 17:31, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > > On 3/17/20 10:09 AM, Philippe Mathieu-Daudé wrote: > > On 3/16/20 9:16 PM, Richard Henderson wrote: > >> On 3/16/20 9:06 AM, Philippe Mathieu-Daudé wrote: > >>> +++ b/target/arm/kvm32.c > >>> @@ -22,7 +22,7 @@ > >>> #include "internals.h" > >>> #include "qemu/log.h" > >>> -static inline void set_feature(uint64_t *features, int feature) > >>> +static inline void kvm_set_feature(uint64_t *features, int feature) > >> > >> Why, what's wrong with the existing name? > > Peter suggested the rename here: > https://www.mail-archive.com/qemu-devel@nongnu.org/msg641931.html In that message I suggest that if you move the set_feature() function to cpu.h (which is included in lots of places) then that is too generic a name to use for it. The function of the same name here in kvm32.c is fine, because it's 'static inline' and only visible in this file, so the bar for naming is lower. (In fact, it's a demonstration of why you don't want a generic name like 'set_feature' in a widely included header file.) thanks -- PMM
On 4/19/20 9:58 PM, Peter Maydell wrote: > On Sun, 19 Apr 2020 at 17:31, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: >> >> On 3/17/20 10:09 AM, Philippe Mathieu-Daudé wrote: >>> On 3/16/20 9:16 PM, Richard Henderson wrote: >>>> On 3/16/20 9:06 AM, Philippe Mathieu-Daudé wrote: >>>>> +++ b/target/arm/kvm32.c >>>>> @@ -22,7 +22,7 @@ >>>>> #include "internals.h" >>>>> #include "qemu/log.h" >>>>> -static inline void set_feature(uint64_t *features, int feature) >>>>> +static inline void kvm_set_feature(uint64_t *features, int feature) >>>> >>>> Why, what's wrong with the existing name? >> >> Peter suggested the rename here: >> https://www.mail-archive.com/qemu-devel@nongnu.org/msg641931.html > > In that message I suggest that if you move the set_feature() > function to cpu.h (which is included in lots of places) then > that is too generic a name to use for it. The function of > the same name here in kvm32.c is fine, because it's > 'static inline' and only visible in this file, so the bar > for naming is lower. (In fact, it's a demonstration of why > you don't want a generic name like 'set_feature' in a widely > included header file.) And your suggestion is indeed obviously correct... Apparently after 19 months rebasing this work I'm not seeing clearly. Thanks again!
diff --git a/target/arm/kvm32.c b/target/arm/kvm32.c index f271181ab8..0ab28b473a 100644 --- a/target/arm/kvm32.c +++ b/target/arm/kvm32.c @@ -22,7 +22,7 @@ #include "internals.h" #include "qemu/log.h" -static inline void set_feature(uint64_t *features, int feature) +static inline void kvm_set_feature(uint64_t *features, int feature) { *features |= 1ULL << feature; } @@ -146,14 +146,14 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf) * timers; this in turn implies most of the other feature * bits, but a few must be tested. */ - set_feature(&features, ARM_FEATURE_V7VE); - set_feature(&features, ARM_FEATURE_GENERIC_TIMER); + kvm_set_feature(&features, ARM_FEATURE_V7VE); + kvm_set_feature(&features, ARM_FEATURE_GENERIC_TIMER); if (extract32(id_pfr0, 12, 4) == 1) { - set_feature(&features, ARM_FEATURE_THUMB2EE); + kvm_set_feature(&features, ARM_FEATURE_THUMB2EE); } if (extract32(ahcf->isar.mvfr1, 12, 4) == 1) { - set_feature(&features, ARM_FEATURE_NEON); + kvm_set_feature(&features, ARM_FEATURE_NEON); } ahcf->features = features; diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c index be5b31c2b0..ad33e048e4 100644 --- a/target/arm/kvm64.c +++ b/target/arm/kvm64.c @@ -447,12 +447,12 @@ void kvm_arm_pmu_set_irq(CPUState *cs, int irq) } } -static inline void set_feature(uint64_t *features, int feature) +static inline void kvm_set_feature(uint64_t *features, int feature) { *features |= 1ULL << feature; } -static inline void unset_feature(uint64_t *features, int feature) +static inline void kvm_unset_feature(uint64_t *features, int feature) { *features &= ~(1ULL << feature); } @@ -648,11 +648,11 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf) * with VFPv4+Neon; this in turn implies most of the other * feature bits. */ - set_feature(&features, ARM_FEATURE_V8); - set_feature(&features, ARM_FEATURE_NEON); - set_feature(&features, ARM_FEATURE_AARCH64); - set_feature(&features, ARM_FEATURE_PMU); - set_feature(&features, ARM_FEATURE_GENERIC_TIMER); + kvm_set_feature(&features, ARM_FEATURE_V8); + kvm_set_feature(&features, ARM_FEATURE_NEON); + kvm_set_feature(&features, ARM_FEATURE_AARCH64); + kvm_set_feature(&features, ARM_FEATURE_PMU); + kvm_set_feature(&features, ARM_FEATURE_GENERIC_TIMER); ahcf->features = features; @@ -802,7 +802,7 @@ int kvm_arch_init_vcpu(CPUState *cs) if (cpu->has_pmu) { cpu->kvm_init_features[0] |= 1 << KVM_ARM_VCPU_PMU_V3; } else { - unset_feature(&env->features, ARM_FEATURE_PMU); + kvm_unset_feature(&env->features, ARM_FEATURE_PMU); } if (cpu_isar_feature(aa64_sve, cpu)) { assert(kvm_arm_sve_supported(cs));
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- target/arm/kvm32.c | 10 +++++----- target/arm/kvm64.c | 16 ++++++++-------- 2 files changed, 13 insertions(+), 13 deletions(-)