| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <87frkrtr4z.wl-maz@kernel.org>
Date: Thu, 06 Feb 2025 08:27:08 +0000
From: Marc Zyngier <maz@...nel.org>
To: Colton Lewis <coltonlewis@...gle.com>
Cc: kvm@...r.kernel.org,
Catalin Marinas <catalin.marinas@....com>,
Will Deacon <will@...nel.org>,
Oliver Upton <oliver.upton@...ux.dev>,
Joey Gouly <joey.gouly@....com>,
Suzuki K Poulose <suzuki.poulose@....com>,
Zenghui Yu <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
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.
>
> 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.
> 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.
--
Without deviation from the norm, progress is not possible.
Powered by blists - more mailing lists