| Message ID | 20210903190629.11917-2-dpsmith@apertussolutions.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xsm: refactoring xsm hooks | expand |
On 03.09.2021 21:06, Daniel P. Smith wrote: > --- /dev/null > +++ b/xen/include/xen/alternative-call.h > @@ -0,0 +1,63 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +#ifndef XEN_ALTERNATIVE_CALL > +#define XEN_ALTERNATIVE_CALL > + > +/* > + * Some subsystems in Xen may have multiple implementions, which can be > + * resolved to a single implementation at boot time. By default, this will > + * result in the use of function pointers. > + * > + * Some architectures may have mechanisms for dynamically modifying .text. > + * Using this mechnaism, function pointers can be converted to direct calls > + * which are typically more efficient at runtime. > + * > + * For architectures to support: > + * > + * - Implement alternative_{,v}call() in asm/alternative.h. Code generation > + * requirements are to emit a function pointer call at build time, and stash > + * enough metadata to simplify the call at boot once the implementation has > + * been resolved. > + * - Select ALTERNATIVE_CALL in Kconfig. > + * > + * To use: > + * > + * Consider the following simplified example. > + * > + * 1) struct foo_ops __alt_call_maybe_initdata ops; > + * > + * 2) const struct foo_ops __initconst foo_a_ops = { ... }; > + * const struct foo_ops __initconst foo_b_ops = { ... }; > + * > + * void foo_init(void) > + * { > + * ... > + * if ( use_impl_a ) > + * ops = *foo_a_ops; > + * else if ( use_impl_b ) > + * ops = *foo_b_ops; > + * ... > + * } > + * > + * 3) alternative_call(ops.bar, ...); > + * > + * There needs to a single ops object (1) which will eventually contain the > + * function pointers. This should be populated in foo's init() function (2) > + * by one of the available implementations. To call functions, use > + * alternative_{,v}call() referencing the main ops object (3). > + */ > + > +#ifdef CONFIG_ALTERNATIVE_CALL > + > +#include <asm/alternative.h> > + > +#define __alt_call_maybe_initdata __initdata My v3 comment here was: "I think it wants (needs) clarifying that this may only be used if the ops object is used exclusively in alternative_{,v}call() instances (besides the original assignments to it, of course)." I realize this was slightly too strict, as other uses from .init.* are of course also okay, but I continue to think that - in particular with the example using it - there should be a warning about this possible pitfall. Or am I merely unable to spot the wording change somewhere in the comment? Jan
On 06/09/2021 16:52, Jan Beulich wrote: > On 03.09.2021 21:06, Daniel P. Smith wrote: >> --- /dev/null >> +++ b/xen/include/xen/alternative-call.h >> @@ -0,0 +1,63 @@ >> +/* SPDX-License-Identifier: GPL-2.0 */ >> +#ifndef XEN_ALTERNATIVE_CALL >> +#define XEN_ALTERNATIVE_CALL >> + >> +/* >> + * Some subsystems in Xen may have multiple implementions, which can be >> + * resolved to a single implementation at boot time. By default, this will >> + * result in the use of function pointers. >> + * >> + * Some architectures may have mechanisms for dynamically modifying .text. >> + * Using this mechnaism, function pointers can be converted to direct calls >> + * which are typically more efficient at runtime. >> + * >> + * For architectures to support: >> + * >> + * - Implement alternative_{,v}call() in asm/alternative.h. Code generation >> + * requirements are to emit a function pointer call at build time, and stash >> + * enough metadata to simplify the call at boot once the implementation has >> + * been resolved. >> + * - Select ALTERNATIVE_CALL in Kconfig. >> + * >> + * To use: >> + * >> + * Consider the following simplified example. >> + * >> + * 1) struct foo_ops __alt_call_maybe_initdata ops; >> + * >> + * 2) const struct foo_ops __initconst foo_a_ops = { ... }; >> + * const struct foo_ops __initconst foo_b_ops = { ... }; >> + * >> + * void foo_init(void) >> + * { >> + * ... >> + * if ( use_impl_a ) >> + * ops = *foo_a_ops; >> + * else if ( use_impl_b ) >> + * ops = *foo_b_ops; >> + * ... >> + * } >> + * >> + * 3) alternative_call(ops.bar, ...); >> + * >> + * There needs to a single ops object (1) which will eventually contain the >> + * function pointers. This should be populated in foo's init() function (2) >> + * by one of the available implementations. To call functions, use >> + * alternative_{,v}call() referencing the main ops object (3). >> + */ >> + >> +#ifdef CONFIG_ALTERNATIVE_CALL >> + >> +#include <asm/alternative.h> >> + >> +#define __alt_call_maybe_initdata __initdata > My v3 comment here was: > > "I think it wants (needs) clarifying that this may only be used if > the ops object is used exclusively in alternative_{,v}call() > instances (besides the original assignments to it, of course)." > > I realize this was slightly too strict, as other uses from .init.* > are of course also okay, but I continue to think that - in > particular with the example using it - there should be a warning > about this possible pitfall. Or am I merely unable to spot the > wording change somewhere in the comment? Such a comment is utterly pointless. initdata has a well known meaning, and a comment warning about the effects of it is just teaching developers to suck eggs[1] ~Andrew [1] For the non-english speakers, https://en.wikipedia.org/wiki/Teaching_grandmother_to_suck_eggs
On 06.09.2021 18:22, Andrew Cooper wrote: > On 06/09/2021 16:52, Jan Beulich wrote: >> On 03.09.2021 21:06, Daniel P. Smith wrote: >>> --- /dev/null >>> +++ b/xen/include/xen/alternative-call.h >>> @@ -0,0 +1,63 @@ >>> +/* SPDX-License-Identifier: GPL-2.0 */ >>> +#ifndef XEN_ALTERNATIVE_CALL >>> +#define XEN_ALTERNATIVE_CALL >>> + >>> +/* >>> + * Some subsystems in Xen may have multiple implementions, which can be >>> + * resolved to a single implementation at boot time. By default, this will >>> + * result in the use of function pointers. >>> + * >>> + * Some architectures may have mechanisms for dynamically modifying .text. >>> + * Using this mechnaism, function pointers can be converted to direct calls >>> + * which are typically more efficient at runtime. >>> + * >>> + * For architectures to support: >>> + * >>> + * - Implement alternative_{,v}call() in asm/alternative.h. Code generation >>> + * requirements are to emit a function pointer call at build time, and stash >>> + * enough metadata to simplify the call at boot once the implementation has >>> + * been resolved. >>> + * - Select ALTERNATIVE_CALL in Kconfig. >>> + * >>> + * To use: >>> + * >>> + * Consider the following simplified example. >>> + * >>> + * 1) struct foo_ops __alt_call_maybe_initdata ops; >>> + * >>> + * 2) const struct foo_ops __initconst foo_a_ops = { ... }; >>> + * const struct foo_ops __initconst foo_b_ops = { ... }; >>> + * >>> + * void foo_init(void) >>> + * { >>> + * ... >>> + * if ( use_impl_a ) >>> + * ops = *foo_a_ops; >>> + * else if ( use_impl_b ) >>> + * ops = *foo_b_ops; >>> + * ... >>> + * } >>> + * >>> + * 3) alternative_call(ops.bar, ...); >>> + * >>> + * There needs to a single ops object (1) which will eventually contain the >>> + * function pointers. This should be populated in foo's init() function (2) >>> + * by one of the available implementations. To call functions, use >>> + * alternative_{,v}call() referencing the main ops object (3). >>> + */ >>> + >>> +#ifdef CONFIG_ALTERNATIVE_CALL >>> + >>> +#include <asm/alternative.h> >>> + >>> +#define __alt_call_maybe_initdata __initdata >> My v3 comment here was: >> >> "I think it wants (needs) clarifying that this may only be used if >> the ops object is used exclusively in alternative_{,v}call() >> instances (besides the original assignments to it, of course)." >> >> I realize this was slightly too strict, as other uses from .init.* >> are of course also okay, but I continue to think that - in >> particular with the example using it - there should be a warning >> about this possible pitfall. Or am I merely unable to spot the >> wording change somewhere in the comment? > > Such a comment is utterly pointless. initdata has a well known meaning, > and a comment warning about the effects of it is just teaching > developers to suck eggs[1] Well, okay then - at least the definition of __alt_call_maybe_initdata isn't far away from the comment. (What I'm not convinced of is that people knowing __initdata's meaning necessarily need to correctly infer __alt_call_maybe_initdata's.) Two other observations about the comment though, which I'd like to be taken care of (perhaps while committing): - __initconst wants to become __initconstrel. - foo_init(), seeing that there are section annotations elsewhere, wants to be marked __init. Then Acked-by: Jan Beulich <jbeulich@suse.com> Daniel, you having made changes (even if just minor ones) imo requires you S-o-b on the patch alongside Andrew's. Jan
On 9/7/21 2:00 AM, Jan Beulich wrote: > On 06.09.2021 18:22, Andrew Cooper wrote: >> On 06/09/2021 16:52, Jan Beulich wrote: >>> On 03.09.2021 21:06, Daniel P. Smith wrote: >>>> --- /dev/null >>>> +++ b/xen/include/xen/alternative-call.h >>>> @@ -0,0 +1,63 @@ >>>> +/* SPDX-License-Identifier: GPL-2.0 */ >>>> +#ifndef XEN_ALTERNATIVE_CALL >>>> +#define XEN_ALTERNATIVE_CALL >>>> + >>>> +/* >>>> + * Some subsystems in Xen may have multiple implementions, which can be >>>> + * resolved to a single implementation at boot time. By default, this will >>>> + * result in the use of function pointers. >>>> + * >>>> + * Some architectures may have mechanisms for dynamically modifying .text. >>>> + * Using this mechnaism, function pointers can be converted to direct calls >>>> + * which are typically more efficient at runtime. >>>> + * >>>> + * For architectures to support: >>>> + * >>>> + * - Implement alternative_{,v}call() in asm/alternative.h. Code generation >>>> + * requirements are to emit a function pointer call at build time, and stash >>>> + * enough metadata to simplify the call at boot once the implementation has >>>> + * been resolved. >>>> + * - Select ALTERNATIVE_CALL in Kconfig. >>>> + * >>>> + * To use: >>>> + * >>>> + * Consider the following simplified example. >>>> + * >>>> + * 1) struct foo_ops __alt_call_maybe_initdata ops; >>>> + * >>>> + * 2) const struct foo_ops __initconst foo_a_ops = { ... }; >>>> + * const struct foo_ops __initconst foo_b_ops = { ... }; >>>> + * >>>> + * void foo_init(void) >>>> + * { >>>> + * ... >>>> + * if ( use_impl_a ) >>>> + * ops = *foo_a_ops; >>>> + * else if ( use_impl_b ) >>>> + * ops = *foo_b_ops; >>>> + * ... >>>> + * } >>>> + * >>>> + * 3) alternative_call(ops.bar, ...); >>>> + * >>>> + * There needs to a single ops object (1) which will eventually contain the >>>> + * function pointers. This should be populated in foo's init() function (2) >>>> + * by one of the available implementations. To call functions, use >>>> + * alternative_{,v}call() referencing the main ops object (3). >>>> + */ >>>> + >>>> +#ifdef CONFIG_ALTERNATIVE_CALL >>>> + >>>> +#include <asm/alternative.h> >>>> + >>>> +#define __alt_call_maybe_initdata __initdata >>> My v3 comment here was: >>> >>> "I think it wants (needs) clarifying that this may only be used if >>> the ops object is used exclusively in alternative_{,v}call() >>> instances (besides the original assignments to it, of course)." >>> >>> I realize this was slightly too strict, as other uses from .init.* >>> are of course also okay, but I continue to think that - in >>> particular with the example using it - there should be a warning >>> about this possible pitfall. Or am I merely unable to spot the >>> wording change somewhere in the comment? >> >> Such a comment is utterly pointless. initdata has a well known meaning, >> and a comment warning about the effects of it is just teaching >> developers to suck eggs[1] > > Well, okay then - at least the definition of __alt_call_maybe_initdata > isn't far away from the comment. (What I'm not convinced of is that > people knowing __initdata's meaning necessarily need to correctly > infer __alt_call_maybe_initdata's.) > > Two other observations about the comment though, which I'd like to be > taken care of (perhaps while committing): > > - __initconst wants to become __initconstrel. > - foo_init(), seeing that there are section annotations elsewhere, > wants to be marked __init. > > Then > Acked-by: Jan Beulich <jbeulich@suse.com> > > Daniel, you having made changes (even if just minor ones) imo requires > you S-o-b on the patch alongside Andrew's. Ack, I realized after sending I didn't SoB it, my apologies on that. v/r, dps
diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig index 9b164db641..1f83518ee0 100644 --- a/xen/arch/x86/Kconfig +++ b/xen/arch/x86/Kconfig @@ -6,6 +6,7 @@ config X86 def_bool y select ACPI select ACPI_LEGACY_TABLES_LOOKUP + select ALTERNATIVE_CALL select ARCH_SUPPORTS_INT128 select CORE_PARKING select HAS_ALTERNATIVE diff --git a/xen/common/Kconfig b/xen/common/Kconfig index 0ddd18e11a..ac5491b1cc 100644 --- a/xen/common/Kconfig +++ b/xen/common/Kconfig @@ -22,6 +22,9 @@ config GRANT_TABLE If unsure, say Y. +config ALTERNATIVE_CALL + bool + config HAS_ALTERNATIVE bool diff --git a/xen/include/xen/alternative-call.h b/xen/include/xen/alternative-call.h new file mode 100644 index 0000000000..d10af90b1b --- /dev/null +++ b/xen/include/xen/alternative-call.h @@ -0,0 +1,63 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +#ifndef XEN_ALTERNATIVE_CALL +#define XEN_ALTERNATIVE_CALL + +/* + * Some subsystems in Xen may have multiple implementions, which can be + * resolved to a single implementation at boot time. By default, this will + * result in the use of function pointers. + * + * Some architectures may have mechanisms for dynamically modifying .text. + * Using this mechnaism, function pointers can be converted to direct calls + * which are typically more efficient at runtime. + * + * For architectures to support: + * + * - Implement alternative_{,v}call() in asm/alternative.h. Code generation + * requirements are to emit a function pointer call at build time, and stash + * enough metadata to simplify the call at boot once the implementation has + * been resolved. + * - Select ALTERNATIVE_CALL in Kconfig. + * + * To use: + * + * Consider the following simplified example. + * + * 1) struct foo_ops __alt_call_maybe_initdata ops; + * + * 2) const struct foo_ops __initconst foo_a_ops = { ... }; + * const struct foo_ops __initconst foo_b_ops = { ... }; + * + * void foo_init(void) + * { + * ... + * if ( use_impl_a ) + * ops = *foo_a_ops; + * else if ( use_impl_b ) + * ops = *foo_b_ops; + * ... + * } + * + * 3) alternative_call(ops.bar, ...); + * + * There needs to a single ops object (1) which will eventually contain the + * function pointers. This should be populated in foo's init() function (2) + * by one of the available implementations. To call functions, use + * alternative_{,v}call() referencing the main ops object (3). + */ + +#ifdef CONFIG_ALTERNATIVE_CALL + +#include <asm/alternative.h> + +#define __alt_call_maybe_initdata __initdata + +#else + +#define alternative_call(func, args...) (func)(args) +#define alternative_vcall(func, args...) (func)(args) + +#define __alt_call_maybe_initdata __read_mostly + +#endif /* !CONFIG_ALTERNATIVE_CALL */ +#endif /* XEN_ALTERNATIVE_CALL */