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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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