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 <jbeul...@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
