[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH] x86/livepatch: enable livepatching assembly source files
On Wed, Apr 19, 2023 at 04:39:01PM +0200, Jan Beulich wrote: > On 19.04.2023 15:36, Roger Pau Monné wrote: > > On Wed, Apr 19, 2023 at 02:00:38PM +0200, Jan Beulich wrote: > >> On 19.04.2023 13:44, Roger Pau Monné wrote: > >>> On Wed, Apr 19, 2023 at 10:43:22AM +0200, Jan Beulich wrote: > >>>> On 19.04.2023 10:25, Roger Pau Monné wrote: > >>>>> On Wed, Apr 19, 2023 at 08:17:45AM +0200, Jan Beulich wrote: > >>>>>> On 18.04.2023 15:06, Roger Pau Monné wrote: > >>>>>>> On Tue, Apr 18, 2023 at 01:00:53PM +0200, Jan Beulich wrote: > >>>>>>>> On 18.04.2023 11:24, Roger Pau Monne wrote: > >>>>>>>>> --- a/xen/arch/x86/include/asm/config.h > >>>>>>>>> +++ b/xen/arch/x86/include/asm/config.h > >>>>>>>>> @@ -44,6 +44,20 @@ > >>>>>>>>> /* Linkage for x86 */ > >>>>>>>>> #ifdef __ASSEMBLY__ > >>>>>>>>> #define ALIGN .align 16,0x90 > >>>>>>>>> +#ifdef CONFIG_LIVEPATCH > >>>>>>>>> +#define START_LP(name) \ > >>>>>>>>> + jmp name; \ > >>>>>>>>> + .pushsection .text.name, "ax", @progbits; \ > >>>>>>>>> + name: > >>>>>>>>> +#define END_LP(name) \ > >>>>>>>>> + .size name, . - name; \ > >>>>>>>>> + .type name, @function; \ > >>>>>>>>> + .popsection > >>>>>>>>> +#else > >>>>>>>>> +#define START_LP(name) \ > >>>>>>>>> + name: > >>>>>>>>> +#define END_LP(name) > >>>>>>>>> +#endif > >>>>>>>>> #define ENTRY(name) \ > >>>>>>>>> .globl name; \ > >>>>>>>>> ALIGN; \ > >>>>>>>> > >>>>>>>> Couldn't END_LP() set type and size unconditionally? (But see also > >>>>>>>> below.) > >>>>>>> > >>>>>>> I see, so that we could also use it for debug purposes. I guess at > >>>>>>> that point it might be better to use {START,END}_FUNC() to note that > >>>>>>> the macros also have an effect beyond that of livepatching. > >>>>>>> > >>>>>>> Maybe also introduce a START_ENTRY() that replaces ENTRY()? Albeit I > >>>>>>> find START_ENTRY a weird name. > >>>>>> > >>>>>> So do I. {START,END}_FUNC() or whatever else are in principle fine, but > >>>>>> I take it that you're aware that we meanwhile have two or even three > >>>>>> concurring proposals on a general scheme of such annotations, and we > >>>>>> don't seem to be able to agree on one. (I guess I'll make a design > >>>>>> session proposal on this topic for Prague.) > >>>>> > >>>>> Oh, I wasn't aware we had other proposals, I've been away on an off > >>>>> quite a lot recently, and haven't been able to keep up with all > >>>>> xen-devel email. Do you have any references at hand? > >>>> > >>>> Andrew said he had posted something long ago, but I didn't recall and > >>>> hence have no reference. My posting from about a year ago is > >>>> https://lists.xen.org/archives/html/xen-devel/2022-04/msg00876.html > >>>> Subsequently Jane went kind of the Linux route: > >>>> https://lists.xen.org/archives/html/xen-devel/2022-08/msg00236.html > >>>> > >>>>>> One thing needs to be clear though: Macros doing things solely needed > >>>>>> for LP need to not have extra effects with it disabled, and such > >>>>>> macros also better wouldn't e.g. insert stray JMP when not really > >>>>>> needed. Hence I expect we still want (some) LP-specific macros besides > >>>>>> whatever we settle on as the generic ones. > >>>>> > >>>>> The stray jmp can be inserted only in the livepatch case, if we end up > >>>>> having to add it. > >>>>> > >>>>> Maybe we should just go with Linux names, so initially I would like to > >>>>> use: > >>>>> > >>>>> SYM_FUNC_START{_NOALIGN}(name) > >>>>> SYM_FUNC_START_LOCAL{_NOALIGN}(name) > >>>>> SYM_FUNC_END(name) > >>>> > >>>> As said in replies on the earlier threads, I think these are overly > >>>> verbose and come in overly many variations. > >>> > >>> Right, I would only introduce the ones above and as lonog as I have at > >>> least one user for them. I don't think there's much value in importing > >>> the file wholesale if we have no use case for a lot of the imported > >>> macros. > >>> > >>> The main issue with ENTRY() and ENDPROC() / ENDDATA() is that we still > >>> need a tag for local function-like entry point labels, would you then > >>> use PROC() for those? ENTRY_LOCAL()? > >>> > >>> I have to admit I prefer the FUNC_START{_LOCAL} for that purpose as I > >>> think it's clearer. I would agree on dropping the SYM_ prefix from > >>> the Linux ones if there's consensus. > >> > >> Okay, I'm glad we can agree on no SYM_. But what value does START have? > >> And why would the type be (re)specified via ..._END()? FUNC(), DATA(), > >> and END() ought to be all we need. > > > > Does it imply that we would then drop ENTRY()? (seems so, would just > > like to confirm). > > Yes. ENTRY() may not go away immediately, but I'd expect it to be > phased out. > > >> The type would be set by the entry > >> point macros, and the size by END(). To cover local vs global I could > >> live with _LOCAL suffixes, but personally would prefer e.g. LFUNC() > >> and GFUNC(). We could also limit ourselves to FUNC() plus DATA(), and > >> have (non-)global expressed by END() and e.g. LEND() or END_LOCAL(). > >> One less macro, but maybe slightly odd to have the .global directives > >> then at the end rather than at the beginning. > > > > Hm, yes, I do find it odd to have the .global at the end. FUNC and > > FUNC_LOCAL would be my preference, I do find {L,G}FUNC a bit > > confusing. > > Well, yes, I was expecting this to be the case. Hence why I said I could > live with _LOCAL suffixes, even if they aren't my preference. What we > may want to keep in mind is that sooner or later we may want to have > non-aligning variants of these. That'll again make for larger names, > unless we went with e.g. an optional 2nd parameter which, if absent, > means default alignment, while if present it would specify the alignment > (which then can be used to effectively specify no alignment). E.g. > > #define ALIGN(algn...) .balign algn > > #define GLOBAL(name) \ > .globl name; \ > .hidden name; \ > name: > > #define FUNC(name, algn...) \ > ALIGN(LAST(16, ## algn), 0x90); \ > GLOBAL(name); \ > .type name, @function > > with these helpers (and count_args() as we already have it), or ideally > something yet more simple (which right now I can't seem to be able to > think of): > > #define ARG1_(x, y...) (x) > #define ARG2_(x, y...) (y) > > #define LAST__(nr) ARG ## nr ## _ > #define LAST_(nr) LAST__(nr) > #define LAST(x, y...) LAST_(count_args(x, ## y))(x, ## y) Would seem acceptable to me. Would you like to make a proposal (likely updating your previous patch) along this lines? Thanks, Roger.
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |