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

["Daniel P. Smith" <dpsmith@xxxxxxxxxxxxxxxxxxxx>]

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@xxxxxxxx>
>
> 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