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

[Jan Beulich <jbeulich@xxxxxxxx>]

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