| Message ID | 1448650215-15218-11-git-send-email-marc.zyngier@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@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. > +#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? Apologies if this question is just outright ridiculous. -Christoffer -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@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) > >> +#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! ;-) 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. M.
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@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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@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. >> >> 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.
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.zyngier@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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/12/15 11:53, Christoffer Dall wrote: > 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.zyngier@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? No, and that's the whole point of this patch: the function pointers are loaded into registers as PC-relative constants (adrp+add), the selection being done by a mov or a nop. For example: ffffffc0007f1f60: 90000001 adrp x1, ffffffc0007f1000 ffffffc0007f1f64: 90000000 adrp x0, ffffffc0007f1000 ffffffc0007f1f68: 91036021 add x1, x1, #0xd8 ffffffc0007f1f6c: 910b0000 add x0, x0, #0x2c0 ffffffc0007f1f70: d503201f nop ffffffc0007f1f74: aa1303e0 mov x0, x19 ffffffc0007f1f78: d63f0020 blr x1 For the default condition (the above code), the CPU is likely to discard the second adrp+add (because of the mov x0, x19). For the alternate, the nop is replaced by a "mov x1, x0", which makes the first adrp+add irrelevant (it will be eliminated in the pipeline of any decent CPU). While this has a cost in terms of instruction footprint, the branch predictor is quickly going to find out where we're branching. We also avoid fetching both from the I and D sides, which could kill the branch predictor if not speculated in time. In the end, we get something that is a lot more predictable, even for simpler CPU designs. M.
On Wed, Dec 02, 2015 at 01:19:22PM +0000, Marc Zyngier wrote: > On 02/12/15 11:53, Christoffer Dall wrote: > > 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.zyngier@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? > > No, and that's the whole point of this patch: the function pointers are > loaded into registers as PC-relative constants (adrp+add), the selection > being done by a mov or a nop. For example: > > ffffffc0007f1f60: 90000001 adrp x1, ffffffc0007f1000 > ffffffc0007f1f64: 90000000 adrp x0, ffffffc0007f1000 > ffffffc0007f1f68: 91036021 add x1, x1, #0xd8 > ffffffc0007f1f6c: 910b0000 add x0, x0, #0x2c0 > ffffffc0007f1f70: d503201f nop > ffffffc0007f1f74: aa1303e0 mov x0, x19 > ffffffc0007f1f78: d63f0020 blr x1 right, looking at the disassembly was a good idea. > > For the default condition (the above code), the CPU is likely to discard > the second adrp+add (because of the mov x0, x19). For the alternate, the > nop is replaced by a "mov x1, x0", which makes the first adrp+add > irrelevant (it will be eliminated in the pipeline of any decent CPU). > > While this has a cost in terms of instruction footprint, the branch > predictor is quickly going to find out where we're branching. We also > avoid fetching both from the I and D sides, which could kill the branch > predictor if not speculated in time. In the end, we get something that > is a lot more predictable, even for simpler CPU designs. > consider me convinced. Thanks for the in-depth! -Christoffer -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. How about setting static-keys at hyp init time? Thanks, drew -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2 Dec 2015 16:34:56 -0600 Andrew Jones <drjones@redhat.com> 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. > > How about setting static-keys at hyp init time? That was an option I looked at. And while static keys would work to some extent, they also have some nasty side effects: - They create both a fast and a slow path. We don't want that - both path should be equally fast, or at least have as little overhead as possible - We do need code patching for some assembly code, and using static keys on top creates a parallel mechanism that makes it hard to follow/debug/maintain. You can view this alternative function call as a slightly different kind of static keys - one that can give you the capability to handle function calls instead of just jumping over code sequences. Both have their own merits. Thanks, M.
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. + */ +#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);
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. Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> --- arch/arm64/kvm/hyp/hyp.h | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)