Message-ID: <86lduit1if.wl-maz@kernel.org>
Date: Fri, 07 Feb 2025 11:52:56 +0000
From: Marc Zyngier <maz@...nel.org>
To: Colton Lewis <coltonlewis@...gle.com>
Cc: kvm@...r.kernel.org,
	catalin.marinas@....com,
	will@...nel.org,
	oliver.upton@...ux.dev,
	joey.gouly@....com,
	suzuki.poulose@....com,
	yuzenghui@...wei.com,
	linux-arm-kernel@...ts.infradead.org,
	linux-kernel@...r.kernel.org,
	kvmarm@...ts.linux.dev
Subject: Re: [PATCH v2] KVM: arm64: Remove cyclical dependency in arm_pmuv3.h

On Thu, 06 Feb 2025 19:45:51 +0000,
Colton Lewis <coltonlewis@...gle.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@...nel.org> writes:
> 
> > Colton,
> 
> > On Thu, 06 Feb 2025 00:17:44 +0000,
> > Colton Lewis <coltonlewis@...gle.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@...gle.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.

-- 
Without deviation from the norm, progress is not possible.

Powered by blists - more mailing lists