Re: [Xen-devel] [PATCH v5 15/28] xsplice: Add .xsplice.hooks functions and test-case

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 24.03.16 at 21:00, <konrad.wilk@xxxxxxxxxx> wrote:
> From: Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>
> 
> Add hook functions which run during patch apply and patch revert.
> Hook functions are used by xsplice payloads to manipulate data structures
> during patching, etc.

Since the added documentation here didn't enlighten me, I've gone
back to the design doc, and found a single trivial mentioning of hooks.
No example of what they would be useful for, nothing. Unless these
can be shown to be needed if any recent XSA fix would be converted
to an xSplice patch, I'd recommend dropping this for now.

> @@ -851,6 +878,11 @@ static int apply_payload(struct payload *data)
>  
>      arch_xsplice_patching_leave();
>  
> +    spin_debug_disable();
> +    for ( i = 0; i < data->n_load_funcs; i++ )
> +        data->load_funcs[i]();
> +    spin_debug_enable();

The spin debug disabling needs explanation. And shouldn't this be
done before arch_xsplice_patching_leave()? Or wait,
documentation above says "before payload is being applied", so it
would need to go even further up, and ...

> @@ -874,6 +906,11 @@ static int revert_payload(struct payload *data)
>  
>      arch_xsplice_patching_leave();
>  
> +    spin_debug_disable();
> +    for ( i = 0; i < data->n_unload_funcs; i++ )
> +        data->unload_funcs[i]();
> +    spin_debug_enable();

... it would be this one which may need to move up by just a few
lines.

> --- /dev/null
> +++ b/xen/include/xen/xsplice_patch.h
> @@ -0,0 +1,59 @@
> +/*
> + * Copyright (C) 2016 Citrix Systems R&D Ltd.
> + */
> +
> +#ifndef __XEN_XSPLICE_PATCH_H__
> +#define __XEN_XSPLICE_PATCH_H__
> +
> +/*
> + * The following definitions are to be used in patches. They are taken
> + * from kpatch.
> + */
> +typedef void (*xsplice_loadcall_t)(void);
> +typedef void (*xsplice_unloadcall_t)(void);

Plain function types please.

> +/* This definition is taken from Linux. */
> +#define __UNIQUE_ID(prefix) __PASTE(__PASTE(__UNIQUE_ID_, prefix), 
> __COUNTER__)
> +/*
> + * XSPLICE_IGNORE_SECTION macro
> + *
> + * This macro is for ignoring sections that may change as a side effect of
> + * another change or might be a non-bundlable section; that is one that does
> + * not honor -ffunction-section and create a one-to-one relation from 
> function
> + * symbol to section.
> + */
> +#define XSPLICE_IGNORE_SECTION(_sec) \
> +     char *__UNIQUE_ID(xsplice_ignore_section_) 
> __section(".xsplice.ignore.sections") = _sec;
> +
> +/*
> + * XSPLICE_IGNORE_FUNCTION macro
> + *
> + * This macro is for ignoring functions that may change as a side effect of a
> + * change in another function.
> + */
> +#define XSPLICE_IGNORE_FUNCTION(_fn) \
> +     void *__xsplice_ignore_func_##_fn 
> __section(".xsplice.ignore.functions") = _fn;

Despite mentioned in the commit message, all of the above seems
unrelated (and unclear in this context). Even more so that - afaict -
they're unusable as we don't seem to have any __PASTE().

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel