On Wed, Dec 02, 2015 at 09:47:43AM +0000, Marc Zyngier wrote:
> On 02/12/15 09:27, Christoffer Dall wrote:
> > 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.zyng...@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.
>
> I don't find that stupid. Asking that kind of stuff is useful to put
> things in perspective.
>
Thanks!
> >>
> >> 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?
>
> An if statement will involve fetching some configuration from memory.
> You can do that, but you are going to waste a cache line and memory
> bandwidth (both which are scarce resources) for something that never
> ever changes over the life of the system. These things tend to accumulate.
Sure, but won't you be fetching the function pointer from memory anyway?
-Christoffer
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
