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

[Konrad Rzeszutek Wilk <konrad@xxxxxxxxxx>]

On Fri, Apr 01, 2016 at 09:50:31AM -0600, Jan Beulich wrote:
> >>> 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.

I do like this for the test-case uses (and regression testing).

The normal use-case is to modify structures values where we have to
be delicate about it and can't just replace the value. As in we
may have to recompute the value.

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

Done.
> 
> > +/* 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().

/me nods. I will remove them.
> 
> Jan
> 

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