| Message ID | 20250206001744.3155465-1-coltonlewis@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] KVM: arm64: Remove cyclical dependency in arm_pmuv3.h | expand |
Colton, On Thu, 06 Feb 2025 00:17:44 +0000, Colton Lewis <coltonlewis@google.com> wrote: > > asm/kvm_host.h includes asm/arm_pmu.h which includes perf/arm_pmuv3.h > which includes asm/arm_pmuv3.h which includes asm/kvm_host.h This > causes confusing compilation problems why trying to use anything > defined in any of the headers in any other headers. Header guards is > the only reason this cycle didn't create tons of redefinition > warnings. > > The motivating example was figuring out it was impossible to use the > hypercall macros kvm_call_hyp* from kvm_host.h in arm_pmuv3.h. The > compiler will insist they aren't defined even though kvm_host.h is > included. Many other examples are lurking which could confuse > developers in the future. Well, that's because contrary to what you have asserted in v1, not all include files are legitimate to use willy-nilly. You have no business directly using asm/kvm_host.h, and linux/kvm_host.h is what you need. > > Break the cycle by taking asm/kvm_host.h out of asm/arm_pmuv3.h > because asm/kvm_host.h is huge and we only need a few functions from > it. Move the required declarations to a new header asm/kvm_pmu.h. > > Signed-off-by: Colton Lewis <coltonlewis@google.com> > --- > > Possibly spinning more definitions out of asm/kvm_host.h would be a > good idea, but I'm not interested in getting bogged down in which > functions ideally belong where. This is sufficient to break the Tough luck. I'm not interested in half baked solutions, and "what belongs where" *is* the problem that needs solving. > cyclical dependency and get rid of the compilation issues. Though I > mention the one example I found, many other similar problems could > confuse developers in the future. > > v2: > * Make a new header instead of moving kvm functions into the > dedicated pmuv3 header > > v1: > https://lore.kernel.org/kvm/20250204195708.1703531-1-coltonlewis@google.com/ > > arch/arm64/include/asm/arm_pmuv3.h | 3 +-- > arch/arm64/include/asm/kvm_host.h | 14 -------------- > arch/arm64/include/asm/kvm_pmu.h | 26 ++++++++++++++++++++++++++ > include/kvm/arm_pmu.h | 1 - > 4 files changed, 27 insertions(+), 17 deletions(-) > create mode 100644 arch/arm64/include/asm/kvm_pmu.h > > diff --git a/arch/arm64/include/asm/arm_pmuv3.h b/arch/arm64/include/asm/arm_pmuv3.h > index 8a777dec8d88..54dd27a7a19f 100644 > --- a/arch/arm64/include/asm/arm_pmuv3.h > +++ b/arch/arm64/include/asm/arm_pmuv3.h > @@ -6,9 +6,8 @@ > #ifndef __ASM_PMUV3_H > #define __ASM_PMUV3_H > > -#include <asm/kvm_host.h> > - > #include <asm/cpufeature.h> > +#include <asm/kvm_pmu.h> > #include <asm/sysreg.h> > > #define RETURN_READ_PMEVCNTRN(n) \ > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h > index 7cfa024de4e3..6d4a2e7ab310 100644 > --- a/arch/arm64/include/asm/kvm_host.h > +++ b/arch/arm64/include/asm/kvm_host.h > @@ -1385,25 +1385,11 @@ void kvm_arch_vcpu_ctxflush_fp(struct kvm_vcpu *vcpu); > void kvm_arch_vcpu_ctxsync_fp(struct kvm_vcpu *vcpu); > void kvm_arch_vcpu_put_fp(struct kvm_vcpu *vcpu); > > -static inline bool kvm_pmu_counter_deferred(struct perf_event_attr *attr) > -{ > - return (!has_vhe() && attr->exclude_host); > -} > - > #ifdef CONFIG_KVM > -void kvm_set_pmu_events(u64 set, struct perf_event_attr *attr); > -void kvm_clr_pmu_events(u64 clr); > -bool kvm_set_pmuserenr(u64 val); > void kvm_enable_trbe(void); > void kvm_disable_trbe(void); > void kvm_tracing_set_el1_configuration(u64 trfcr_while_in_guest); > #else > -static inline void kvm_set_pmu_events(u64 set, struct perf_event_attr *attr) {} > -static inline void kvm_clr_pmu_events(u64 clr) {} > -static inline bool kvm_set_pmuserenr(u64 val) > -{ > - return false; > -} > static inline void kvm_enable_trbe(void) {} > static inline void kvm_disable_trbe(void) {} > static inline void kvm_tracing_set_el1_configuration(u64 trfcr_while_in_guest) {} > diff --git a/arch/arm64/include/asm/kvm_pmu.h b/arch/arm64/include/asm/kvm_pmu.h > new file mode 100644 > index 000000000000..3a8f737504d2 > --- /dev/null > +++ b/arch/arm64/include/asm/kvm_pmu.h > @@ -0,0 +1,26 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > + > +#ifndef __KVM_PMU_H > +#define __KVM_PMU_H > + > +void kvm_vcpu_pmu_resync_el0(void); > + > +#ifdef CONFIG_KVM > +void kvm_set_pmu_events(u64 set, struct perf_event_attr *attr); > +void kvm_clr_pmu_events(u64 clr); > +bool kvm_set_pmuserenr(u64 val); > +#else > +static inline void kvm_set_pmu_events(u64 set, struct perf_event_attr *attr) {} > +static inline void kvm_clr_pmu_events(u64 clr) {} > +static inline bool kvm_set_pmuserenr(u64 val) > +{ > + return false; > +} > +#endif > + > +static inline bool kvm_pmu_counter_deferred(struct perf_event_attr *attr) > +{ > + return (!has_vhe() && attr->exclude_host); > +} > + > +#endif How does this solve your problem of using the HYP-calling macros? > diff --git a/include/kvm/arm_pmu.h b/include/kvm/arm_pmu.h > index 147bd3ee4f7b..2c78b1b1a9bb 100644 > --- a/include/kvm/arm_pmu.h > +++ b/include/kvm/arm_pmu.h > @@ -74,7 +74,6 @@ int kvm_arm_pmu_v3_enable(struct kvm_vcpu *vcpu); > struct kvm_pmu_events *kvm_get_pmu_events(void); > void kvm_vcpu_pmu_restore_guest(struct kvm_vcpu *vcpu); > void kvm_vcpu_pmu_restore_host(struct kvm_vcpu *vcpu); > -void kvm_vcpu_pmu_resync_el0(void); > > #define kvm_vcpu_has_pmu(vcpu) \ > (vcpu_has_feature(vcpu, KVM_ARM_VCPU_PMU_V3)) > > base-commit: 2014c95afecee3e76ca4a56956a936e23283f05b I'm absolutely not keen on *two* PMU-related include files. They both describe internal APIs, and don't see a good reasoning for this arbitrary split other than "it works better for me and I don't want to do more than strictly necessary". For example, include/kvm was only introduced to be able to share files between architectures, and with 32bit KVM/arm being long dead, this serves no purpose anymore. Moving these things out of the way would be a good start and would provide a better base for further change. So please present a rationale on what needs to go where and why based on their usage pattern rather than personal convenience, and then we'll look at a possible patch. But not the other way around. Thanks, M.
Hi Colton,
kernel test robot noticed the following build errors:
[auto build test ERROR on 2014c95afecee3e76ca4a56956a936e23283f05b]
url: https://github.com/intel-lab-lkp/linux/commits/Colton-Lewis/KVM-arm64-Remove-cyclical-dependency-in-arm_pmuv3-h/20250206-082034
base: 2014c95afecee3e76ca4a56956a936e23283f05b
patch link: https://lore.kernel.org/r/20250206001744.3155465-1-coltonlewis%40google.com
patch subject: [PATCH v2] KVM: arm64: Remove cyclical dependency in arm_pmuv3.h
config: arm64-allnoconfig (https://download.01.org/0day-ci/archive/20250207/202502070744.hqS7ZVVX-lkp@intel.com/config)
compiler: aarch64-linux-gcc (GCC) 14.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250207/202502070744.hqS7ZVVX-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202502070744.hqS7ZVVX-lkp@intel.com/
All errors (new ones prefixed by >>):
In file included from arch/arm64/include/asm/kvm_host.h:38,
from include/linux/kvm_host.h:45,
from arch/arm64/kernel/asm-offsets.c:15:
>> include/kvm/arm_pmu.h:177:20: error: static declaration of 'kvm_vcpu_pmu_resync_el0' follows non-static declaration
177 | static inline void kvm_vcpu_pmu_resync_el0(void) {}
| ^~~~~~~~~~~~~~~~~~~~~~~
In file included from arch/arm64/include/asm/arm_pmuv3.h:10,
from include/linux/perf/arm_pmuv3.h:316,
from include/kvm/arm_pmu.h:11:
arch/arm64/include/asm/kvm_pmu.h:6:6: note: previous declaration of 'kvm_vcpu_pmu_resync_el0' with type 'void(void)'
6 | void kvm_vcpu_pmu_resync_el0(void);
| ^~~~~~~~~~~~~~~~~~~~~~~
make[3]: *** [scripts/Makefile.build:102: arch/arm64/kernel/asm-offsets.s] Error 1
make[3]: Target 'prepare' not remade because of errors.
make[2]: *** [Makefile:1264: prepare0] Error 2
make[2]: Target 'prepare' not remade because of errors.
make[1]: *** [Makefile:251: __sub-make] Error 2
make[1]: Target 'prepare' not remade because of errors.
make: *** [Makefile:251: __sub-make] Error 2
make: Target 'prepare' not remade because of errors.
vim +/kvm_vcpu_pmu_resync_el0 +177 include/kvm/arm_pmu.h
5421db1be3b11c5 Marc Zyngier 2021-04-14 163
20492a62b99bd43 Marc Zyngier 2022-05-16 164 #define kvm_vcpu_has_pmu(vcpu) ({ false; })
20492a62b99bd43 Marc Zyngier 2022-05-16 165 static inline void kvm_pmu_update_vcpu_events(struct kvm_vcpu *vcpu) {}
20492a62b99bd43 Marc Zyngier 2022-05-16 166 static inline void kvm_vcpu_pmu_restore_guest(struct kvm_vcpu *vcpu) {}
20492a62b99bd43 Marc Zyngier 2022-05-16 167 static inline void kvm_vcpu_pmu_restore_host(struct kvm_vcpu *vcpu) {}
27131b199f9fdc0 Raghavendra Rao Ananta 2023-10-20 168 static inline void kvm_vcpu_reload_pmu(struct kvm_vcpu *vcpu) {}
3d0dba5764b9430 Marc Zyngier 2022-11-13 169 static inline u8 kvm_arm_pmu_get_pmuver_limit(void)
3d0dba5764b9430 Marc Zyngier 2022-11-13 170 {
3d0dba5764b9430 Marc Zyngier 2022-11-13 171 return 0;
3d0dba5764b9430 Marc Zyngier 2022-11-13 172 }
bc512d6a9b92390 Oliver Upton 2023-10-19 173 static inline u64 kvm_pmu_evtyper_mask(struct kvm *kvm)
bc512d6a9b92390 Oliver Upton 2023-10-19 174 {
bc512d6a9b92390 Oliver Upton 2023-10-19 175 return 0;
bc512d6a9b92390 Oliver Upton 2023-10-19 176 }
b1f778a223a2a8a Marc Zyngier 2023-08-20 @177 static inline void kvm_vcpu_pmu_resync_el0(void) {}
20492a62b99bd43 Marc Zyngier 2022-05-16 178
On Thu, 06 Feb 2025 19:45:51 +0000, Colton Lewis <coltonlewis@google.com> wrote: > > Hey Marc, thanks for the review. I thought of a different solution at > the very bottom. Please let me know if that is preferable. > > Marc Zyngier <maz@kernel.org> writes: > > > Colton, > > > On Thu, 06 Feb 2025 00:17:44 +0000, > > Colton Lewis <coltonlewis@google.com> wrote: > > >> asm/kvm_host.h includes asm/arm_pmu.h which includes perf/arm_pmuv3.h > >> which includes asm/arm_pmuv3.h which includes asm/kvm_host.h This > >> causes confusing compilation problems why trying to use anything > >> defined in any of the headers in any other headers. Header guards is > >> the only reason this cycle didn't create tons of redefinition > >> warnings. > > >> The motivating example was figuring out it was impossible to use the > >> hypercall macros kvm_call_hyp* from kvm_host.h in arm_pmuv3.h. The > >> compiler will insist they aren't defined even though kvm_host.h is > >> included. Many other examples are lurking which could confuse > >> developers in the future. > > > Well, that's because contrary to what you have asserted in v1, not all > > include files are legitimate to use willy-nilly. You have no business > > directly using asm/kvm_host.h, and linux/kvm_host.h is what you need. > > That's what I'm trying to fix. I'm trying to *remove* asm/kvm_host.h > from being included in asm/arm_pmu.h. > > I agree with you that it *should not be included there* but it currently > is. And I can't drop-in replace the include with linux/kvm_host.h > because the that just adds another link in the cyclical dependency. > > > >> Break the cycle by taking asm/kvm_host.h out of asm/arm_pmuv3.h > >> because asm/kvm_host.h is huge and we only need a few functions from > >> it. Move the required declarations to a new header asm/kvm_pmu.h. > > >> Signed-off-by: Colton Lewis <coltonlewis@google.com> > >> --- > > >> Possibly spinning more definitions out of asm/kvm_host.h would be a > >> good idea, but I'm not interested in getting bogged down in which > >> functions ideally belong where. This is sufficient to break the > > > Tough luck. I'm not interested in half baked solutions, and "what > > belongs where" *is* the problem that needs solving. > > Fair point, but a small solution is not half-baked if it is better than > what we have. No. Less crap is still crap. This sort of reasoning may fly for a quick fix that would otherwise result in a a crash or something similarly unpleasant. But for a rework that is only there to enable your particular project, you have all the time in the world to do it right. > > >> cyclical dependency and get rid of the compilation issues. Though I > >> mention the one example I found, many other similar problems could > >> confuse developers in the future. > > >> v2: > >> * Make a new header instead of moving kvm functions into the > >> dedicated pmuv3 header > > >> v1: > >> https://lore.kernel.org/kvm/20250204195708.1703531-1-coltonlewis@google.com/ > > >> arch/arm64/include/asm/arm_pmuv3.h | 3 +-- > >> arch/arm64/include/asm/kvm_host.h | 14 -------------- > >> arch/arm64/include/asm/kvm_pmu.h | 26 ++++++++++++++++++++++++++ > >> include/kvm/arm_pmu.h | 1 - > >> 4 files changed, 27 insertions(+), 17 deletions(-) > >> create mode 100644 arch/arm64/include/asm/kvm_pmu.h > > >> diff --git a/arch/arm64/include/asm/arm_pmuv3.h > >> b/arch/arm64/include/asm/arm_pmuv3.h > >> index 8a777dec8d88..54dd27a7a19f 100644 > >> --- a/arch/arm64/include/asm/arm_pmuv3.h > >> +++ b/arch/arm64/include/asm/arm_pmuv3.h > >> @@ -6,9 +6,8 @@ > >> #ifndef __ASM_PMUV3_H > >> #define __ASM_PMUV3_H > > >> -#include <asm/kvm_host.h> > >> - > >> #include <asm/cpufeature.h> > >> +#include <asm/kvm_pmu.h> > >> #include <asm/sysreg.h> > > >> #define RETURN_READ_PMEVCNTRN(n) \ > >> diff --git a/arch/arm64/include/asm/kvm_host.h > >> b/arch/arm64/include/asm/kvm_host.h > >> index 7cfa024de4e3..6d4a2e7ab310 100644 > >> --- a/arch/arm64/include/asm/kvm_host.h > >> +++ b/arch/arm64/include/asm/kvm_host.h > >> @@ -1385,25 +1385,11 @@ void kvm_arch_vcpu_ctxflush_fp(struct > >> kvm_vcpu *vcpu); > >> void kvm_arch_vcpu_ctxsync_fp(struct kvm_vcpu *vcpu); > >> void kvm_arch_vcpu_put_fp(struct kvm_vcpu *vcpu); > > >> -static inline bool kvm_pmu_counter_deferred(struct perf_event_attr > >> *attr) > >> -{ > >> - return (!has_vhe() && attr->exclude_host); > >> -} > >> - > >> #ifdef CONFIG_KVM > >> -void kvm_set_pmu_events(u64 set, struct perf_event_attr *attr); > >> -void kvm_clr_pmu_events(u64 clr); > >> -bool kvm_set_pmuserenr(u64 val); > >> void kvm_enable_trbe(void); > >> void kvm_disable_trbe(void); > >> void kvm_tracing_set_el1_configuration(u64 trfcr_while_in_guest); > >> #else > >> -static inline void kvm_set_pmu_events(u64 set, struct > >> perf_event_attr *attr) {} > >> -static inline void kvm_clr_pmu_events(u64 clr) {} > >> -static inline bool kvm_set_pmuserenr(u64 val) > >> -{ > >> - return false; > >> -} > >> static inline void kvm_enable_trbe(void) {} > >> static inline void kvm_disable_trbe(void) {} > >> static inline void kvm_tracing_set_el1_configuration(u64 > >> trfcr_while_in_guest) {} > >> diff --git a/arch/arm64/include/asm/kvm_pmu.h > >> b/arch/arm64/include/asm/kvm_pmu.h > >> new file mode 100644 > >> index 000000000000..3a8f737504d2 > >> --- /dev/null > >> +++ b/arch/arm64/include/asm/kvm_pmu.h > >> @@ -0,0 +1,26 @@ > >> +/* SPDX-License-Identifier: GPL-2.0-only */ > >> + > >> +#ifndef __KVM_PMU_H > >> +#define __KVM_PMU_H > >> + > >> +void kvm_vcpu_pmu_resync_el0(void); > >> + > >> +#ifdef CONFIG_KVM > >> +void kvm_set_pmu_events(u64 set, struct perf_event_attr *attr); > >> +void kvm_clr_pmu_events(u64 clr); > >> +bool kvm_set_pmuserenr(u64 val); > >> +#else > >> +static inline void kvm_set_pmu_events(u64 set, struct > >> perf_event_attr *attr) {} > >> +static inline void kvm_clr_pmu_events(u64 clr) {} > >> +static inline bool kvm_set_pmuserenr(u64 val) > >> +{ > >> + return false; > >> +} > >> +#endif > >> + > >> +static inline bool kvm_pmu_counter_deferred(struct perf_event_attr > >> *attr) > >> +{ > >> + return (!has_vhe() && attr->exclude_host); > >> +} > >> + > >> +#endif > > > How does this solve your problem of using the HYP-calling macros? > > This code does not directly solve that problem. It makes a solution to > that problem possible because it breaks up the cyclical dependency by > getting asm/kvm_host.h out of asm/arm_pmuv3.h while still providing the > declarations to arm_pmuv3.c > > With a cyclical dependency the compiler gets confused if you try to use > anything from asm/kvm_host.h inside asm/arm_pmuv3.h (like HYP-calling > macros defined there for example). Again, I believe that inclusion > should not be there in the first place which is the motivation for this > patch. > > But since it is included, here's what happens if you try: > > When asm/kvm_host.h is included somewhere, it indirectly includes > asm/arm_pmuv3.h via the chain described in my commit message. > asm/arm_pmuv3.h is then effectively pasted into asm/kvm_host.h and due > to include guards is passed over this time, but this means that many > things in asm/kvm_host.h aren't defined yet so any symbols from > asm/kvm_host.h defined after the include of asm/arm_pmuv3.h are used > before their definition: boom, confusing compiler errors > > You might argue: just don't do that, but I think it's a terrible > developer experience when you can't use definitions from a file that is > currently included. I spent hours puzzling over errors before realizing > a cyclical dependency was the root cause and want to save other devs > from the same fate. Then do it correctly. Or don't do that. Nobody other than you gets confused by this, it seems. > > >> diff --git a/include/kvm/arm_pmu.h b/include/kvm/arm_pmu.h > >> index 147bd3ee4f7b..2c78b1b1a9bb 100644 > >> --- a/include/kvm/arm_pmu.h > >> +++ b/include/kvm/arm_pmu.h > >> @@ -74,7 +74,6 @@ int kvm_arm_pmu_v3_enable(struct kvm_vcpu *vcpu); > >> struct kvm_pmu_events *kvm_get_pmu_events(void); > >> void kvm_vcpu_pmu_restore_guest(struct kvm_vcpu *vcpu); > >> void kvm_vcpu_pmu_restore_host(struct kvm_vcpu *vcpu); > >> -void kvm_vcpu_pmu_resync_el0(void); > > >> #define kvm_vcpu_has_pmu(vcpu) \ > >> (vcpu_has_feature(vcpu, KVM_ARM_VCPU_PMU_V3)) > > >> base-commit: 2014c95afecee3e76ca4a56956a936e23283f05b > > > I'm absolutely not keen on *two* PMU-related include files. They both > > describe internal APIs, and don't see a good reasoning for this > > arbitrary split other than "it works better for me and I don't want to > > do more than strictly necessary". > > I understand the point which is why v1 tried not to introduce a new > header file and I was advised to make a new header file. > > > For example, include/kvm was only introduced to be able to share files > > between architectures, and with 32bit KVM/arm being long dead, this > > serves no purpose anymore. Moving these things out of the way would be > > a good start and would provide a better base for further change. > > Good to know. I avoided doing that because it would be a lot of change > noise and I wasn't sure such changes would be welcome. > > > So please present a rationale on what needs to go where and why based > > on their usage pattern rather than personal convenience, and then > > we'll look at a possible patch. But not the other way around. > > My rationale is fixing a cyclical dependency due to an inclusion of > asm/kvm_host.h where we both seem to agree it shouldn't be. Cyclical > dependencies are really bad and cause nasty surprises when something > that seems like it should obviously work doesn't. Fixing things like > this makes programming here more conveneint for everyone, not just > me. So I thought it was worth a separate patch. That's not a rationale. That's the umpteenth repetition of a circular argument. This "make it easy for everyone" is a total fallacy. If you really want to make it easy, split the include files using a clear definition of what goes where. That would *actually* help. > > Another possible solution that avoids moving anything around is to take > asm/kvm_host.h out of asm/arm_pmuv3.h and do > > #ifdef CONFIG_ARM64 > #include <linux/kvm_host.h> > #endif > > directly in arm_pmuv3.c which breaks the cycle while still providing the > correct declarations for arm_pmuv3.c (and admittedly many more than > necessary). > > I find this conditional inclusion ugly and possibly you will have an > objection to it, but let me know. No. I want a full solution, not a point hack. Since you are not willing to define things, let me do it for you. - Anything that is at the interface between the host PMU driver and KVM moves in its own include file. Nothing from kvm_host.h should be needed for this, and the driver code only includes that particular file. Make is standalone, so that it doesn't require contextual inclusions (see how your kvm_pmu.h doesn't include anything, and yet depends on perf_event_attr being defined as well as has_vhe()). - Anything that is internal to KVM moves to kvm_host.h. Eventually, this could move to a kvm_internal.h that is nobody else's business (just like we have a vgic.h that is not visible to anyone). - include/kvm/arm_pmu.h dies. And since I like putting my words into action, here's the result of a 10 minute rework: https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/log/?h=kvm-arm64/pmu-includes Not exactly rocket science. M.
diff --git a/arch/arm64/include/asm/arm_pmuv3.h b/arch/arm64/include/asm/arm_pmuv3.h index 8a777dec8d88..54dd27a7a19f 100644 --- a/arch/arm64/include/asm/arm_pmuv3.h +++ b/arch/arm64/include/asm/arm_pmuv3.h @@ -6,9 +6,8 @@ #ifndef __ASM_PMUV3_H #define __ASM_PMUV3_H -#include <asm/kvm_host.h> - #include <asm/cpufeature.h> +#include <asm/kvm_pmu.h> #include <asm/sysreg.h> #define RETURN_READ_PMEVCNTRN(n) \ diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h index 7cfa024de4e3..6d4a2e7ab310 100644 --- a/arch/arm64/include/asm/kvm_host.h +++ b/arch/arm64/include/asm/kvm_host.h @@ -1385,25 +1385,11 @@ void kvm_arch_vcpu_ctxflush_fp(struct kvm_vcpu *vcpu); void kvm_arch_vcpu_ctxsync_fp(struct kvm_vcpu *vcpu); void kvm_arch_vcpu_put_fp(struct kvm_vcpu *vcpu); -static inline bool kvm_pmu_counter_deferred(struct perf_event_attr *attr) -{ - return (!has_vhe() && attr->exclude_host); -} - #ifdef CONFIG_KVM -void kvm_set_pmu_events(u64 set, struct perf_event_attr *attr); -void kvm_clr_pmu_events(u64 clr); -bool kvm_set_pmuserenr(u64 val); void kvm_enable_trbe(void); void kvm_disable_trbe(void); void kvm_tracing_set_el1_configuration(u64 trfcr_while_in_guest); #else -static inline void kvm_set_pmu_events(u64 set, struct perf_event_attr *attr) {} -static inline void kvm_clr_pmu_events(u64 clr) {} -static inline bool kvm_set_pmuserenr(u64 val) -{ - return false; -} static inline void kvm_enable_trbe(void) {} static inline void kvm_disable_trbe(void) {} static inline void kvm_tracing_set_el1_configuration(u64 trfcr_while_in_guest) {} diff --git a/arch/arm64/include/asm/kvm_pmu.h b/arch/arm64/include/asm/kvm_pmu.h new file mode 100644 index 000000000000..3a8f737504d2 --- /dev/null +++ b/arch/arm64/include/asm/kvm_pmu.h @@ -0,0 +1,26 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#ifndef __KVM_PMU_H +#define __KVM_PMU_H + +void kvm_vcpu_pmu_resync_el0(void); + +#ifdef CONFIG_KVM +void kvm_set_pmu_events(u64 set, struct perf_event_attr *attr); +void kvm_clr_pmu_events(u64 clr); +bool kvm_set_pmuserenr(u64 val); +#else +static inline void kvm_set_pmu_events(u64 set, struct perf_event_attr *attr) {} +static inline void kvm_clr_pmu_events(u64 clr) {} +static inline bool kvm_set_pmuserenr(u64 val) +{ + return false; +} +#endif + +static inline bool kvm_pmu_counter_deferred(struct perf_event_attr *attr) +{ + return (!has_vhe() && attr->exclude_host); +} + +#endif diff --git a/include/kvm/arm_pmu.h b/include/kvm/arm_pmu.h index 147bd3ee4f7b..2c78b1b1a9bb 100644 --- a/include/kvm/arm_pmu.h +++ b/include/kvm/arm_pmu.h @@ -74,7 +74,6 @@ int kvm_arm_pmu_v3_enable(struct kvm_vcpu *vcpu); struct kvm_pmu_events *kvm_get_pmu_events(void); void kvm_vcpu_pmu_restore_guest(struct kvm_vcpu *vcpu); void kvm_vcpu_pmu_restore_host(struct kvm_vcpu *vcpu); -void kvm_vcpu_pmu_resync_el0(void); #define kvm_vcpu_has_pmu(vcpu) \ (vcpu_has_feature(vcpu, KVM_ARM_VCPU_PMU_V3))
asm/kvm_host.h includes asm/arm_pmu.h which includes perf/arm_pmuv3.h which includes asm/arm_pmuv3.h which includes asm/kvm_host.h This causes confusing compilation problems why trying to use anything defined in any of the headers in any other headers. Header guards is the only reason this cycle didn't create tons of redefinition warnings. The motivating example was figuring out it was impossible to use the hypercall macros kvm_call_hyp* from kvm_host.h in arm_pmuv3.h. The compiler will insist they aren't defined even though kvm_host.h is included. Many other examples are lurking which could confuse developers in the future. Break the cycle by taking asm/kvm_host.h out of asm/arm_pmuv3.h because asm/kvm_host.h is huge and we only need a few functions from it. Move the required declarations to a new header asm/kvm_pmu.h. Signed-off-by: Colton Lewis <coltonlewis@google.com> --- Possibly spinning more definitions out of asm/kvm_host.h would be a good idea, but I'm not interested in getting bogged down in which functions ideally belong where. This is sufficient to break the cyclical dependency and get rid of the compilation issues. Though I mention the one example I found, many other similar problems could confuse developers in the future. v2: * Make a new header instead of moving kvm functions into the dedicated pmuv3 header v1: https://lore.kernel.org/kvm/20250204195708.1703531-1-coltonlewis@google.com/ arch/arm64/include/asm/arm_pmuv3.h | 3 +-- arch/arm64/include/asm/kvm_host.h | 14 -------------- arch/arm64/include/asm/kvm_pmu.h | 26 ++++++++++++++++++++++++++ include/kvm/arm_pmu.h | 1 - 4 files changed, 27 insertions(+), 17 deletions(-) create mode 100644 arch/arm64/include/asm/kvm_pmu.h base-commit: 2014c95afecee3e76ca4a56956a936e23283f05b -- 2.48.1.362.g079036d154-goog