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.zyngier@xxxxxxx>
>>>> ---
>>>> 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.
>>
>> 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.
There is also a small number of cases where you *have* to patch
instructions (think VHE, for example). And having two different ways to
check for things is just asking for trouble in the long run.
M.
--
Jazz is not dead. It just smells funny...
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
