| Message ID | 20250204152100.705610-8-mark.rutland@arm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | KVM: arm64: FPSIMD/SVE/SME fixes | expand |
On Tue, Feb 04, 2025 at 03:20:59PM +0000, Mark Rutland wrote: > The shared hyp swtich header has a number of static functions which > might not be used by all files that include the header, and when unused > they will provoke compiler warnings, e.g. s/swtich/switch/ Reviewed-by: Mark Brown <broonie@kernel.org>
On Tue, Feb 04, 2025 at 03:20:59PM +0000, Mark Rutland wrote: > The shared hyp swtich header has a number of static functions which > might not be used by all files that include the header, and when unused > they will provoke compiler warnings, e.g. With at least LLVM 18 we still have some issues with unused statics arising from the aliased function definitions: In file included from arch/arm64/kvm/hyp/nvhe/hyp-main.c:8: ./arch/arm64/kvm/hyp/include/hyp/switch.h:699:13: warning: unused function 'kvm_hyp_handle_iabt_low' [-Wunused-function] 699 | static bool kvm_hyp_handle_iabt_low(struct kvm_vcpu *vcpu, u64 *exit_code) | ^~~~~~~~~~~~~~~~~~~~~~~ ./arch/arm64/kvm/hyp/include/hyp/switch.h:701:13: warning: unused function 'kvm_hyp_handle_watchpt_low' [-Wunused-function] 701 | static bool kvm_hyp_handle_watchpt_low(struct kvm_vcpu *vcpu, u64 *exit_code) | ^~~~~~~~~~~~~~~~~~~~~~~~~~ The simplest thing would be to expand the alises into simple wrapper functions but that doesn't feel amazing, I don't know what people's taste is there?
On Wed, Feb 05, 2025 at 09:50:30PM +0000, Mark Brown wrote: > On Tue, Feb 04, 2025 at 03:20:59PM +0000, Mark Rutland wrote: > > The shared hyp swtich header has a number of static functions which > > might not be used by all files that include the header, and when unused > > they will provoke compiler warnings, e.g. > > With at least LLVM 18 we still have some issues with unused statics > arising from the aliased function definitions: > > In file included from arch/arm64/kvm/hyp/nvhe/hyp-main.c:8: > ./arch/arm64/kvm/hyp/include/hyp/switch.h:699:13: warning: unused function 'kvm_hyp_handle_iabt_low' [-Wunused-function] > 699 | static bool kvm_hyp_handle_iabt_low(struct kvm_vcpu *vcpu, u64 *exit_code) > | ^~~~~~~~~~~~~~~~~~~~~~~ > ./arch/arm64/kvm/hyp/include/hyp/switch.h:701:13: warning: unused function 'kvm_hyp_handle_watchpt_low' [-Wunused-function] > 701 | static bool kvm_hyp_handle_watchpt_low(struct kvm_vcpu *vcpu, u64 *exit_code) > | ^~~~~~~~~~~~~~~~~~~~~~~~~~ > > The simplest thing would be to expand the alises into simple wrapper > functions but that doesn't feel amazing, I don't know what people's > taste is there? Adding 'inline' seems to work, which seems simpler? That said, I'm going to go with the below, adding 'inline' to kvm_hyp_handle_memory_fault() and using CPP defines to alias the function names: | static inline bool kvm_hyp_handle_memory_fault(struct kvm_vcpu *vcpu, | u64 *exit_code) | { | if (!__populate_fault_info(vcpu)) | return true; | | return false; | } | #define kvm_hyp_handle_iabt_low kvm_hyp_handle_memory_fault | #define kvm_hyp_handle_watchpt_low kvm_hyp_handle_memory_fault I think that's clearer, and it's more alisnged with how we usually alias function names in headers. Other than these two cases, __alias() is only used in C files to create a sesparate exprted symbol, and it's odd to use it in a header anyhow. Marc, please should if you'd prefer otherwise. Mark.
[resending as I corrupted MarcZ's email when moving from CC into TO] On Wed, Feb 05, 2025 at 09:50:30PM +0000, Mark Brown wrote: > On Tue, Feb 04, 2025 at 03:20:59PM +0000, Mark Rutland wrote: > > The shared hyp swtich header has a number of static functions which > > might not be used by all files that include the header, and when unused > > they will provoke compiler warnings, e.g. > > With at least LLVM 18 we still have some issues with unused statics > arising from the aliased function definitions: > > In file included from arch/arm64/kvm/hyp/nvhe/hyp-main.c:8: > ./arch/arm64/kvm/hyp/include/hyp/switch.h:699:13: warning: unused function 'kvm_hyp_handle_iabt_low' [-Wunused-function] > 699 | static bool kvm_hyp_handle_iabt_low(struct kvm_vcpu *vcpu, u64 *exit_code) > | ^~~~~~~~~~~~~~~~~~~~~~~ > ./arch/arm64/kvm/hyp/include/hyp/switch.h:701:13: warning: unused function 'kvm_hyp_handle_watchpt_low' [-Wunused-function] > 701 | static bool kvm_hyp_handle_watchpt_low(struct kvm_vcpu *vcpu, u64 *exit_code) > | ^~~~~~~~~~~~~~~~~~~~~~~~~~ > > The simplest thing would be to expand the alises into simple wrapper > functions but that doesn't feel amazing, I don't know what people's > taste is there? Adding 'inline' seems to work, which seems simpler? That said, I'm going to go with the below, adding 'inline' to kvm_hyp_handle_memory_fault() and using CPP defines to alias the function names: | static inline bool kvm_hyp_handle_memory_fault(struct kvm_vcpu *vcpu, | u64 *exit_code) | { | if (!__populate_fault_info(vcpu)) | return true; | | return false; | } | #define kvm_hyp_handle_iabt_low kvm_hyp_handle_memory_fault | #define kvm_hyp_handle_watchpt_low kvm_hyp_handle_memory_fault I think that's clearer, and it's more alisnged with how we usually alias function names in headers. Other than these two cases, __alias() is only used in C files to create a sesparate exprted symbol, and it's odd to use it in a header anyhow. Marc, please should if you'd prefer otherwise. Mark.
On Thu, 06 Feb 2025 10:03:46 +0000, Mark Rutland <mark.rutland@arm.com> wrote: > > [resending as I corrupted MarcZ's email when moving from CC into TO] > > On Wed, Feb 05, 2025 at 09:50:30PM +0000, Mark Brown wrote: > > On Tue, Feb 04, 2025 at 03:20:59PM +0000, Mark Rutland wrote: > > > The shared hyp swtich header has a number of static functions which > > > might not be used by all files that include the header, and when unused > > > they will provoke compiler warnings, e.g. > > > > With at least LLVM 18 we still have some issues with unused statics > > arising from the aliased function definitions: > > > > In file included from arch/arm64/kvm/hyp/nvhe/hyp-main.c:8: > > ./arch/arm64/kvm/hyp/include/hyp/switch.h:699:13: warning: unused function 'kvm_hyp_handle_iabt_low' [-Wunused-function] > > 699 | static bool kvm_hyp_handle_iabt_low(struct kvm_vcpu *vcpu, u64 *exit_code) > > | ^~~~~~~~~~~~~~~~~~~~~~~ > > ./arch/arm64/kvm/hyp/include/hyp/switch.h:701:13: warning: unused function 'kvm_hyp_handle_watchpt_low' [-Wunused-function] > > 701 | static bool kvm_hyp_handle_watchpt_low(struct kvm_vcpu *vcpu, u64 *exit_code) > > | ^~~~~~~~~~~~~~~~~~~~~~~~~~ > > > > The simplest thing would be to expand the alises into simple wrapper > > functions but that doesn't feel amazing, I don't know what people's > > taste is there? > > Adding 'inline' seems to work, which seems simpler? > > That said, I'm going to go with the below, adding 'inline' to > kvm_hyp_handle_memory_fault() and using CPP defines to alias the > function names: > > | static inline bool kvm_hyp_handle_memory_fault(struct kvm_vcpu *vcpu, > | u64 *exit_code) > | { > | if (!__populate_fault_info(vcpu)) > | return true; > | > | return false; > | } > | #define kvm_hyp_handle_iabt_low kvm_hyp_handle_memory_fault > | #define kvm_hyp_handle_watchpt_low kvm_hyp_handle_memory_fault > > I think that's clearer, and it's more alisnged with how we usually alias > function names in headers. Other than these two cases, __alias() is only > used in C files to create a sesparate exprted symbol, and it's odd to > use it in a header anyhow. > > Marc, please should if you'd prefer otherwise. Nah, that's fine by me. My only issue was with marking functions as inline, and yet storing pointers to these functions. But it looks like the compiler (GCC 12.2 in my case) is doing a good job noticing the weird pattern, and generating only one function, even if we store multiple pointers. Thanks, M.
On Thu, Feb 06, 2025 at 10:42:29AM +0000, Marc Zyngier wrote: > On Thu, 06 Feb 2025 10:03:46 +0000, > Mark Rutland <mark.rutland@arm.com> wrote: > > That said, I'm going to go with the below, adding 'inline' to > > kvm_hyp_handle_memory_fault() and using CPP defines to alias the > > function names: > > > > | static inline bool kvm_hyp_handle_memory_fault(struct kvm_vcpu *vcpu, > > | u64 *exit_code) > > | { > > | if (!__populate_fault_info(vcpu)) > > | return true; > > | > > | return false; > > | } > > | #define kvm_hyp_handle_iabt_low kvm_hyp_handle_memory_fault > > | #define kvm_hyp_handle_watchpt_low kvm_hyp_handle_memory_fault > > > > I think that's clearer, and it's more alisnged with how we usually alias > > function names in headers. Other than these two cases, __alias() is only > > used in C files to create a sesparate exprted symbol, and it's odd to > > use it in a header anyhow. > > > > Marc, please should if you'd prefer otherwise. > > Nah, that's fine by me. > > My only issue was with marking functions as inline, and yet storing > pointers to these functions. But it looks like the compiler (GCC 12.2 > in my case) is doing a good job noticing the weird pattern, and > generating only one function, even if we store multiple pointers. That's fair -- I'm fairly certain that we do this elsewhere too, but I can switch to __maybe_unused if we're worried that might bite us in future? Mark.
On Thu, 06 Feb 2025 10:55:21 +0000, Mark Rutland <mark.rutland@arm.com> wrote: > > On Thu, Feb 06, 2025 at 10:42:29AM +0000, Marc Zyngier wrote: > > On Thu, 06 Feb 2025 10:03:46 +0000, > > Mark Rutland <mark.rutland@arm.com> wrote: > > > That said, I'm going to go with the below, adding 'inline' to > > > kvm_hyp_handle_memory_fault() and using CPP defines to alias the > > > function names: > > > > > > | static inline bool kvm_hyp_handle_memory_fault(struct kvm_vcpu *vcpu, > > > | u64 *exit_code) > > > | { > > > | if (!__populate_fault_info(vcpu)) > > > | return true; > > > | > > > | return false; > > > | } > > > | #define kvm_hyp_handle_iabt_low kvm_hyp_handle_memory_fault > > > | #define kvm_hyp_handle_watchpt_low kvm_hyp_handle_memory_fault > > > > > > I think that's clearer, and it's more alisnged with how we usually alias > > > function names in headers. Other than these two cases, __alias() is only > > > used in C files to create a sesparate exprted symbol, and it's odd to > > > use it in a header anyhow. > > > > > > Marc, please should if you'd prefer otherwise. > > > > Nah, that's fine by me. > > > > My only issue was with marking functions as inline, and yet storing > > pointers to these functions. But it looks like the compiler (GCC 12.2 > > in my case) is doing a good job noticing the weird pattern, and > > generating only one function, even if we store multiple pointers. > > That's fair -- I'm fairly certain that we do this elsewhere too, but I > can switch to __maybe_unused if we're worried that might bite us in > future? Sure, that'd be equally fine. Thanks, M.
On Thu, Feb 06, 2025 at 12:28:42PM +0000, Marc Zyngier wrote: > On Thu, 06 Feb 2025 10:55:21 +0000, > Mark Rutland <mark.rutland@arm.com> wrote: > > > > On Thu, Feb 06, 2025 at 10:42:29AM +0000, Marc Zyngier wrote: > > > On Thu, 06 Feb 2025 10:03:46 +0000, > > > Mark Rutland <mark.rutland@arm.com> wrote: > > > > That said, I'm going to go with the below, adding 'inline' to > > > > kvm_hyp_handle_memory_fault() and using CPP defines to alias the > > > > function names: > > > > > > > > | static inline bool kvm_hyp_handle_memory_fault(struct kvm_vcpu *vcpu, > > > > | u64 *exit_code) > > > > | { > > > > | if (!__populate_fault_info(vcpu)) > > > > | return true; > > > > | > > > > | return false; > > > > | } > > > > | #define kvm_hyp_handle_iabt_low kvm_hyp_handle_memory_fault > > > > | #define kvm_hyp_handle_watchpt_low kvm_hyp_handle_memory_fault > > > > > > > > I think that's clearer, and it's more alisnged with how we usually alias > > > > function names in headers. Other than these two cases, __alias() is only > > > > used in C files to create a sesparate exprted symbol, and it's odd to > > > > use it in a header anyhow. > > > > > > > > Marc, please should if you'd prefer otherwise. > > > > > > Nah, that's fine by me. > > > > > > My only issue was with marking functions as inline, and yet storing > > > pointers to these functions. But it looks like the compiler (GCC 12.2 > > > in my case) is doing a good job noticing the weird pattern, and > > > generating only one function, even if we store multiple pointers. > > > > That's fair -- I'm fairly certain that we do this elsewhere too, but I > > can switch to __maybe_unused if we're worried that might bite us in > > future? > > Sure, that'd be equally fine. Looking around, we don't seem to do that elsewhere for functions in headers, so I'll stick with inline for now unless anyone has a strong opinion. I'll send out v2 shortly. Mark.
diff --git a/arch/arm64/kvm/hyp/include/hyp/switch.h b/arch/arm64/kvm/hyp/include/hyp/switch.h index 46df5c2eeaf57..3e34ade7e675a 100644 --- a/arch/arm64/kvm/hyp/include/hyp/switch.h +++ b/arch/arm64/kvm/hyp/include/hyp/switch.h @@ -326,7 +326,7 @@ static inline bool __populate_fault_info(struct kvm_vcpu *vcpu) return __get_fault_info(vcpu->arch.fault.esr_el2, &vcpu->arch.fault); } -static bool kvm_hyp_handle_mops(struct kvm_vcpu *vcpu, u64 *exit_code) +static inline bool kvm_hyp_handle_mops(struct kvm_vcpu *vcpu, u64 *exit_code) { *vcpu_pc(vcpu) = read_sysreg_el2(SYS_ELR); arm64_mops_reset_regs(vcpu_gp_regs(vcpu), vcpu->arch.fault.esr_el2); @@ -404,7 +404,7 @@ static void kvm_hyp_save_fpsimd_host(struct kvm_vcpu *vcpu) * If FP/SIMD is not implemented, handle the trap and inject an undefined * instruction exception to the guest. Similarly for trapped SVE accesses. */ -static bool kvm_hyp_handle_fpsimd(struct kvm_vcpu *vcpu, u64 *exit_code) +static inline bool kvm_hyp_handle_fpsimd(struct kvm_vcpu *vcpu, u64 *exit_code) { bool sve_guest; u8 esr_ec; @@ -608,7 +608,7 @@ static bool handle_ampere1_tcr(struct kvm_vcpu *vcpu) return true; } -static bool kvm_hyp_handle_sysreg(struct kvm_vcpu *vcpu, u64 *exit_code) +static inline bool kvm_hyp_handle_sysreg(struct kvm_vcpu *vcpu, u64 *exit_code) { if (cpus_have_final_cap(ARM64_WORKAROUND_CAVIUM_TX2_219_TVM) && handle_tx2_tvm(vcpu)) @@ -628,7 +628,7 @@ static bool kvm_hyp_handle_sysreg(struct kvm_vcpu *vcpu, u64 *exit_code) return false; } -static bool kvm_hyp_handle_cp15_32(struct kvm_vcpu *vcpu, u64 *exit_code) +static inline bool kvm_hyp_handle_cp15_32(struct kvm_vcpu *vcpu, u64 *exit_code) { if (static_branch_unlikely(&vgic_v3_cpuif_trap) && __vgic_v3_perform_cpuif_access(vcpu) == 1) @@ -649,7 +649,7 @@ static bool kvm_hyp_handle_iabt_low(struct kvm_vcpu *vcpu, u64 *exit_code) static bool kvm_hyp_handle_watchpt_low(struct kvm_vcpu *vcpu, u64 *exit_code) __alias(kvm_hyp_handle_memory_fault); -static bool kvm_hyp_handle_dabt_low(struct kvm_vcpu *vcpu, u64 *exit_code) +static inline bool kvm_hyp_handle_dabt_low(struct kvm_vcpu *vcpu, u64 *exit_code) { if (kvm_hyp_handle_memory_fault(vcpu, exit_code)) return true;
The shared hyp swtich header has a number of static functions which might not be used by all files that include the header, and when unused they will provoke compiler warnings, e.g. | In file included from arch/arm64/kvm/hyp/nvhe/hyp-main.c:8: | ./arch/arm64/kvm/hyp/include/hyp/switch.h:703:13: warning: 'kvm_hyp_handle_dabt_low' defined but not used [-Wunused-function] | 703 | static bool kvm_hyp_handle_dabt_low(struct kvm_vcpu *vcpu, u64 *exit_code) | | ^~~~~~~~~~~~~~~~~~~~~~~ | ./arch/arm64/kvm/hyp/include/hyp/switch.h:682:13: warning: 'kvm_hyp_handle_cp15_32' defined but not used [-Wunused-function] | 682 | static bool kvm_hyp_handle_cp15_32(struct kvm_vcpu *vcpu, u64 *exit_code) | | ^~~~~~~~~~~~~~~~~~~~~~ | ./arch/arm64/kvm/hyp/include/hyp/switch.h:662:13: warning: 'kvm_hyp_handle_sysreg' defined but not used [-Wunused-function] | 662 | static bool kvm_hyp_handle_sysreg(struct kvm_vcpu *vcpu, u64 *exit_code) | | ^~~~~~~~~~~~~~~~~~~~~ | ./arch/arm64/kvm/hyp/include/hyp/switch.h:458:13: warning: 'kvm_hyp_handle_fpsimd' defined but not used [-Wunused-function] | 458 | static bool kvm_hyp_handle_fpsimd(struct kvm_vcpu *vcpu, u64 *exit_code) | | ^~~~~~~~~~~~~~~~~~~~~ | ./arch/arm64/kvm/hyp/include/hyp/switch.h:329:13: warning: 'kvm_hyp_handle_mops' defined but not used [-Wunused-function] | 329 | static bool kvm_hyp_handle_mops(struct kvm_vcpu *vcpu, u64 *exit_code) | | ^~~~~~~~~~~~~~~~~~~ Mark these functions as 'inline' to suppress this warning. This shouldn't result in any functional change. Signed-off-by: Mark Rutland <mark.rutland@arm.com> Cc: Catalin Marinas <catalin.marinas@arm.com> Cc: Fuad Tabba <tabba@google.com> Cc: Marc Zyngier <maz@kernel.org> Cc: Mark Brown <broonie@kernel.org> Cc: Oliver Upton <oliver.upton@linux.dev> Cc: Will Deacon <will@kernel.org> --- arch/arm64/kvm/hyp/include/hyp/switch.h | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)