Re: [PATCH v2 5/5] xen/livepatch: group and document payload hooks

[Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>]

On Tue, Feb 27, 2024 at 11:25 AM Roger Pau Monne <roger.pau@xxxxxxxxxx> wrote:
>
> Group the payload hooks between the pre/post handlers, and the apply/revert
> replacements.  Also attempt to comment the context in which the hooks are
> executed.
>
> No functional change.
>
> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> ---
> Changes since v1:
>  - New in this version.
> ---
>  xen/include/xen/livepatch_payload.h | 37 ++++++++++++++++++++++-------
>  1 file changed, 29 insertions(+), 8 deletions(-)
>
> diff --git a/xen/include/xen/livepatch_payload.h 
> b/xen/include/xen/livepatch_payload.h
> index b9cd4f209670..472d6a4a63c1 100644
> --- a/xen/include/xen/livepatch_payload.h
> +++ b/xen/include/xen/livepatch_payload.h
> @@ -82,6 +82,8 @@ struct payload {
>   * collision.  Since multiple hooks can be registered, the
>   * .livepatch.hook.load section is a table of functions that will be
>   * executed in series by the livepatch infrastructure at patch load time.
> + *
> + * Note the load hook is executed in quiesced context.
>   */
>  #define LIVEPATCH_LOAD_HOOK(_fn) \
>      livepatch_loadcall_t *__weak \
> @@ -96,14 +98,20 @@ struct payload {
>       livepatch_unloadcall_t *__weak \
>          const livepatch_unload_data_##_fn 
> __section(".livepatch.hooks.unload") = _fn;
>
> +/*
> + * Pre/Post action hooks.
> + *
> + * This hooks are executed before or after the livepatch application. Pre 
> hooks
> + * can veto the application/revert of the livepatch.  They are not executed 
> in
> + * quiesced context.  All of pre and post hooks are considered vetoing, and
> + * hence filling any of those will block the usage of the REPLACE action.
> + *
> + * Each of the hooks below can only be set once per livepatch payload.
> + */
>  #define LIVEPATCH_PREAPPLY_HOOK(_fn) \
>      livepatch_precall_t *__attribute__((weak, used)) \
>          const livepatch_preapply_data_##_fn 
> __section(".livepatch.hooks.preapply") = _fn;
>
> -#define LIVEPATCH_APPLY_HOOK(_fn) \
> -    livepatch_actioncall_t *__attribute__((weak, used)) \
> -        const livepatch_apply_data_##_fn __section(".livepatch.hooks.apply") 
> = _fn;
> -
>  #define LIVEPATCH_POSTAPPLY_HOOK(_fn) \
>      livepatch_postcall_t *__attribute__((weak, used)) \
>          const livepatch_postapply_data_##_fn 
> __section(".livepatch.hooks.postapply") = _fn;
> @@ -112,14 +120,27 @@ struct payload {
>      livepatch_precall_t *__attribute__((weak, used)) \
>          const livepatch_prerevert_data_##_fn 
> __section(".livepatch.hooks.prerevert") = _fn;
>
> -#define LIVEPATCH_REVERT_HOOK(_fn) \
> -    livepatch_actioncall_t *__attribute__((weak, used)) \
> -        const livepatch_revert_data_##_fn 
> __section(".livepatch.hooks.revert") = _fn;
> -
>  #define LIVEPATCH_POSTREVERT_HOOK(_fn) \
>      livepatch_postcall_t *__attribute__((weak, used)) \
>          const livepatch_postrevert_data_##_fn 
> __section(".livepatch.hooks.postrevert") = _fn;
>
> +/*
> + * Action replacement hooks.
> + *
> + * The following hooks replace the hypervisor implementation for the 
> livepatch
> + * application and revert routines.  When filling the hooks below the native
> + * apply and revert routines will not be executed, so the provided hooks need
> + * to make sure the state of the payload after apply or revert is as expected
> + * by the livepatch logic.
> + */
> +#define LIVEPATCH_APPLY_HOOK(_fn) \
> +    livepatch_actioncall_t *__attribute__((weak, used)) \
> +        const livepatch_apply_data_##_fn __section(".livepatch.hooks.apply") 
> = _fn;
> +
> +#define LIVEPATCH_REVERT_HOOK(_fn) \
> +    livepatch_actioncall_t *__attribute__((weak, used)) \
> +        const livepatch_revert_data_##_fn 
> __section(".livepatch.hooks.revert") = _fn;
> +
>  #endif /* __XEN_LIVEPATCH_PAYLOAD_H__ */
>

LGTM, thanks.

Reviewed-by: Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>