Re: [PATCH 7/8] KVM: arm64: Mark some header functions as inline

[Marc Zyngier <maz@kernel.org>]

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.

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