diff mbox series

[v2,01/10] xen: Implement xen/alternative-call.h for use in common code

Message ID 20210712203233.20289-2-dpsmith@apertussolutions.com (mailing list archive)
State Superseded
Headers show
Series xsm: refactoring xsm hooks | expand

Commit Message

Daniel P. Smith July 12, 2021, 8:32 p.m. UTC 
From: Andrew Cooper <andrew.cooper3@citrix.com>

The alternative call infrastructure is x86-only for now, but the common iommu
code has a variant and more common code wants to use the infrastructure.

Introduce CONFIG_ALTERNATIVE_CALL and a conditional implemetnation so common
code can use the optimisation when available, without requiring all
architectures to implement no-op stubs.

Write some documentation, which was thus far entirely absent, covering the
requirements for an architecture to implement this optimsiation, and how to
use the infrastructure in general code.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
 xen/arch/x86/Kconfig               |  1 +
 xen/common/Kconfig                 |  3 ++
 xen/include/xen/alternative-call.h | 65 ++++++++++++++++++++++++++++++
 3 files changed, 69 insertions(+)
 create mode 100644 xen/include/xen/alternative-call.h

Comments

Andrew Cooper July 12, 2021, 11:48 p.m. UTC | #1 
On 12/07/2021 21:32, Daniel P. Smith wrote:
> diff --git a/xen/include/xen/alternative-call.h b/xen/include/xen/alternative-call.h
> new file mode 100644
> index 0000000000..11d1c26068
> --- /dev/null
> +++ b/xen/include/xen/alternative-call.h
> @@ -0,0 +1,65 @@
> +/* 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) struct foo_ops __alt_call_maybe_initconst foo_a_ops = { ... };
> + *     struct foo_ops __alt_call_maybe_initconst foo_b_ops = { ... };

It occurs to me after reviewing patch 2 that these want to be const
struct foo_ops __initconst ..., and there is no need for
__alt_call_maybe_initconst at all.

The only thing wanting a conditional annotation like this is the single
ops object, and it needs to be initdata (or hopefully ro_after_init in
the future).

~Andrew
Jan Beulich July 13, 2021, 6:28 a.m. UTC | #2 
On 13.07.2021 01:48, Andrew Cooper wrote:
> On 12/07/2021 21:32, Daniel P. Smith wrote:
>> diff --git a/xen/include/xen/alternative-call.h b/xen/include/xen/alternative-call.h
>> new file mode 100644
>> index 0000000000..11d1c26068
>> --- /dev/null
>> +++ b/xen/include/xen/alternative-call.h
>> @@ -0,0 +1,65 @@
>> +/* 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) struct foo_ops __alt_call_maybe_initconst foo_a_ops = { ... };
>> + *     struct foo_ops __alt_call_maybe_initconst foo_b_ops = { ... };
> 
> It occurs to me after reviewing patch 2 that these want to be const
> struct foo_ops __initconst ...,

__initconstrel then, I suppose.

> and there is no need for
> __alt_call_maybe_initconst at all.
> 
> The only thing wanting a conditional annotation like this is the single
> ops object, and it needs to be initdata (or hopefully ro_after_init in
> the future).

ro_after_init and initdata can't be alternatives of one another; ops
(until be gain ro_after_init) need to remain in .bss (or .data).

Jan
Andrew Cooper July 13, 2021, 8:36 a.m. UTC | #3 
On 13/07/2021 07:28, Jan Beulich wrote:
> On 13.07.2021 01:48, Andrew Cooper wrote:
>> On 12/07/2021 21:32, Daniel P. Smith wrote:
>>> diff --git a/xen/include/xen/alternative-call.h b/xen/include/xen/alternative-call.h
>>> new file mode 100644
>>> index 0000000000..11d1c26068
>>> --- /dev/null
>>> +++ b/xen/include/xen/alternative-call.h
>>> @@ -0,0 +1,65 @@
>>> +/* 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) struct foo_ops __alt_call_maybe_initconst foo_a_ops = { ... };
>>> + *     struct foo_ops __alt_call_maybe_initconst foo_b_ops = { ... };
>> It occurs to me after reviewing patch 2 that these want to be const
>> struct foo_ops __initconst ...,
> __initconstrel then, I suppose.
>
>> and there is no need for
>> __alt_call_maybe_initconst at all.
>>
>> The only thing wanting a conditional annotation like this is the single
>> ops object, and it needs to be initdata (or hopefully ro_after_init in
>> the future).
> ro_after_init and initdata can't be alternatives of one another; ops
> (until be gain ro_after_init) need to remain in .bss (or .data).

Once alternatives have run, the ops structure is no longer referenced by
anything at runtime, and can be reclaimed.

All the x86 examples are weird because we've either got extra data
fields which are referenced at runtime, or we've not accelerated all
function pointers.

In neither case does modifying an accelerated function pointer after the
fact do what the programmer expected, and conditionally initdata makes
this far more obvious.

~Andrew
Jan Beulich July 13, 2021, 8:53 a.m. UTC | #4 
On 13.07.2021 10:36, Andrew Cooper wrote:
> On 13/07/2021 07:28, Jan Beulich wrote:
>> On 13.07.2021 01:48, Andrew Cooper wrote:
>>> On 12/07/2021 21:32, Daniel P. Smith wrote:
>>>> diff --git a/xen/include/xen/alternative-call.h b/xen/include/xen/alternative-call.h
>>>> new file mode 100644
>>>> index 0000000000..11d1c26068
>>>> --- /dev/null
>>>> +++ b/xen/include/xen/alternative-call.h
>>>> @@ -0,0 +1,65 @@
>>>> +/* 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) struct foo_ops __alt_call_maybe_initconst foo_a_ops = { ... };
>>>> + *     struct foo_ops __alt_call_maybe_initconst foo_b_ops = { ... };
>>> It occurs to me after reviewing patch 2 that these want to be const
>>> struct foo_ops __initconst ...,
>> __initconstrel then, I suppose.
>>
>>> and there is no need for
>>> __alt_call_maybe_initconst at all.
>>>
>>> The only thing wanting a conditional annotation like this is the single
>>> ops object, and it needs to be initdata (or hopefully ro_after_init in
>>> the future).
>> ro_after_init and initdata can't be alternatives of one another; ops
>> (until be gain ro_after_init) need to remain in .bss (or .data).
> 
> Once alternatives have run, the ops structure is no longer referenced by
> anything at runtime, and can be reclaimed.

Oh, right - if all pointers have been consumed for patching, initdata
is of course fine.

Jan

> All the x86 examples are weird because we've either got extra data
> fields which are referenced at runtime, or we've not accelerated all
> function pointers.
> 
> In neither case does modifying an accelerated function pointer after the
> fact do what the programmer expected, and conditionally initdata makes
> this far more obvious.
> 
> ~Andrew
>
Jan Beulich July 14, 2021, 3:35 p.m. UTC | #5 
On 12.07.2021 22:32, Daniel P. Smith wrote:
> --- a/xen/arch/x86/Kconfig
> +++ b/xen/arch/x86/Kconfig
> @@ -9,6 +9,7 @@ config X86
>  	select ARCH_SUPPORTS_INT128
>  	select CORE_PARKING
>  	select HAS_ALTERNATIVE
> +	select ALTERNATIVE_CALL
>  	select HAS_COMPAT
>  	select HAS_CPUFREQ
>  	select HAS_EHCI

Please respect the (at least largely) alphabetical ordering here and ...

> --- a/xen/common/Kconfig
> +++ b/xen/common/Kconfig
> @@ -25,6 +25,9 @@ config GRANT_TABLE
>  config HAS_ALTERNATIVE
>  	bool
>  
> +config ALTERNATIVE_CALL
> +	bool
> +
>  config HAS_COMPAT
>  	bool

... maybe also here.

> --- /dev/null
> +++ b/xen/include/xen/alternative-call.h
> @@ -0,0 +1,65 @@
> +/* 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) struct foo_ops __alt_call_maybe_initconst foo_a_ops = { ... };
> + *     struct foo_ops __alt_call_maybe_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).
> + */

May be worth adding a sentence about the section annotations, the
more that (as you did point out in an earlier reply) there are
pre-existing cases differing from the general goal here?

> +#ifdef CONFIG_ALTERNATIVE_CALL
> +
> +#include <asm/alternative.h>
> +
> +#define __alt_call_maybe_initdata  __initdata
> +#define __alt_call_maybe_initconst __initconst

As indicated elsewhere, I think this wants to be __initconstrel,
as the function pointers to place in the structures will
definitely involve relocations. Otoh, given your initial reply,
__alt_call_maybe_initdata may be all that's actually needed.

> +#else
> +
> +#define alternative_call(func, args...)  (func)(args)
> +#define alternative_vcall(func, args...) (func)(args)
> +
> +#define __alt_call_maybe_initdata
> +#define __alt_call_maybe_initconst
> +
> +#endif /* !CONFIG_ALTERNATIVE_CALL */
> +#endif /* XEN_ALTERNATIVE_CALL */

I'm surprised you have no "Local variables:" footer comment here.
Not that I need it for anything, but I thought you and others are
quite interested in it to be there.

Jan
diff mbox series

Patch

diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
index 9b164db641..c91cdd83dc 100644
--- a/xen/arch/x86/Kconfig
+++ b/xen/arch/x86/Kconfig
@@ -9,6 +9,7 @@  config X86
 	select ARCH_SUPPORTS_INT128
 	select CORE_PARKING
 	select HAS_ALTERNATIVE
+	select ALTERNATIVE_CALL
 	select HAS_COMPAT
 	select HAS_CPUFREQ
 	select HAS_EHCI
diff --git a/xen/common/Kconfig b/xen/common/Kconfig
index 0ddd18e11a..1594ce4e73 100644
--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -25,6 +25,9 @@  config GRANT_TABLE
 config HAS_ALTERNATIVE
 	bool
 
+config ALTERNATIVE_CALL
+	bool
+
 config HAS_COMPAT
 	bool
 
diff --git a/xen/include/xen/alternative-call.h b/xen/include/xen/alternative-call.h
new file mode 100644
index 0000000000..11d1c26068
--- /dev/null
+++ b/xen/include/xen/alternative-call.h
@@ -0,0 +1,65 @@ 
+/* 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) struct foo_ops __alt_call_maybe_initconst foo_a_ops = { ... };
+ *     struct foo_ops __alt_call_maybe_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
+#define __alt_call_maybe_initconst __initconst
+
+#else
+
+#define alternative_call(func, args...)  (func)(args)
+#define alternative_vcall(func, args...) (func)(args)
+
+#define __alt_call_maybe_initdata
+#define __alt_call_maybe_initconst
+
+#endif /* !CONFIG_ALTERNATIVE_CALL */
+#endif /* XEN_ALTERNATIVE_CALL */