[PATCH v2 10/21] arm64: KVM: Add patchable function selector

[Christoffer Dall]

On Tue, Dec 01, 2015 at 06:51:00PM +0000, Marc Zyngier wrote:
> On 01/12/15 15:39, Christoffer Dall wrote:
> > On Fri, Nov 27, 2015 at 06:50:04PM +0000, Marc Zyngier wrote:
> >> KVM so far relies on code patching, and is likely to use it more
> >> in the future. The main issue is that our alternative system works
> >> at the instruction level, while we'd like to have alternatives at
> >> the function level.
> >>
> >> In order to cope with this, add the "hyp_alternate_select" macro that
> >> outputs a brief sequence of code that in turn can be patched, allowing
> >> al alternative function to be selected.
> > 
> > s/al/an/ ?
> > 
> >>
> >> Signed-off-by: Marc Zyngier <marc.zyngier at arm.com>
> >> ---
> >>  arch/arm64/kvm/hyp/hyp.h | 16 ++++++++++++++++
> >>  1 file changed, 16 insertions(+)
> >>
> >> diff --git a/arch/arm64/kvm/hyp/hyp.h b/arch/arm64/kvm/hyp/hyp.h
> >> index 7ac8e11..f0427ee 100644
> >> --- a/arch/arm64/kvm/hyp/hyp.h
> >> +++ b/arch/arm64/kvm/hyp/hyp.h
> >> @@ -27,6 +27,22 @@
> >>  
> >>  #define kern_hyp_va(v) (typeof(v))((unsigned long)v & HYP_PAGE_OFFSET_MASK)
> >>  
> >> +/*
> >> + * Generates patchable code sequences that are used to switch between
> >> + * two implementations of a function, depending on the availability of
> >> + * a feature.
> >> + */
> > 
> > This looks right to me, but I'm a bit unclear what the types of this is
> > and how to use it.
> > 
> > Are orig and alt function pointers and cond is a CONFIG_FOO ?  fname is
> > a symbol, which is defined as a prototype somewhere and then implemented
> > here, or?
> > 
> > Perhaps a Usage: part of the docs would be helpful.
> 
> How about:
> 
> @fname: a symbol name that will be defined as a function returning a
> function pointer whose type will match @orig and @alt
> @orig: A pointer to the default function, as returned by @fname when
> @cond doesn't hold
> @alt: A pointer to the alternate function, as returned by @fname when
> @cond holds
> @cond: a CPU feature (as described in asm/cpufeature.h)

looks good.

> 
> > 
> >> +#define hyp_alternate_select(fname, orig, alt, cond)			\
> >> +typeof(orig) * __hyp_text fname(void)					\
> >> +{									\
> >> +	typeof(alt) *val = orig;					\
> >> +	asm volatile(ALTERNATIVE("nop		\n",			\
> >> +				 "mov	%0, %1	\n",			\
> >> +				 cond)					\
> >> +		     : "+r" (val) : "r" (alt));				\
> >> +	return val;							\
> >> +}
> >> +
> >>  void __vgic_v2_save_state(struct kvm_vcpu *vcpu);
> >>  void __vgic_v2_restore_state(struct kvm_vcpu *vcpu);
> >>  
> >> -- 
> >> 2.1.4
> >>
> > 
> > I haven't thought much about how all of this is implemented, but from my
> > point of views the ideal situation would be something like:
> > 
> > void foo(int a, int b)
> > {
> > 	ALTERNATIVE_IF_NOT CONFIG_BAR
> > 	foo_legacy(a, b);
> > 	ALTERNATIVE_ELSE
> > 	foo_new(a, b);
> > 	ALTERNATIVE_END
> > }
> > 
> > I realize this may be impossible because the C code could implement all
> > sort of fun stuff around the actual function calls, but would there be
> > some way to annotate the functions and find the actual branch statement
> > and change the target?
> 
> The main issue is that C doesn't give you any access to the branch
> function itself, except for the asm-goto statements. It also makes it
> very hard to preserve the return type. For your idea to work, we'd need
> some support in the compiler itself. I'm sure that it is doable, just
> not by me! ;-)

Not by me either, I'm just asking stupid questions - as always.

> 
> This is why I've ended up creating something that returns a function
> *pointer*, because that's something that exists in the language (no new
> concept). I simply made sure I could return it at minimal cost.
> 

I don't have a problem with this either.  I'm curious though, how much
of a performance improvement (and why) we get from doing this as opposed
to a simple if-statement?

Thanks,
-Christoffer