On 29.05.2023 15:34, Roger Pau Monné wrote: > On Tue, May 23, 2023 at 01:30:51PM +0200, Jan Beulich wrote: >> Note that the FB-label in autogen_stubs() cannot be converted just yet: >> Such labels cannot be used with .type. We could further diverge from >> Linux'es model and avoid setting STT_NOTYPE explicitly (that's the type >> labels get by default anyway). >> >> Note that we can't use ALIGN() (in place of SYM_ALIGN()) as long as we >> still have ALIGN. > > FWIW, as I'm looking into using the newly added macros in order to add > annotations suitable for live-patching, I would need to switch some of > the LABEL usages into it's own functions, as it's not possible to > livepatch a function that has labels jumped into from code paths > outside of the function.
Hmm, I'm not sure what the best way is to overcome that restriction. I'm not convinced we want to arbitrarily name things "functions". >> --- a/xen/arch/x86/include/asm/asm_defns.h >> +++ b/xen/arch/x86/include/asm/asm_defns.h >> @@ -81,6 +81,45 @@ register unsigned long current_stack_poi >> >> #ifdef __ASSEMBLY__ >> >> +#define SYM_ALIGN(algn...) .balign algn >> + >> +#define SYM_L_GLOBAL(name) .globl name >> +#define SYM_L_WEAK(name) .weak name > > Won't this better be added when required? I can't spot any weak > symbols in assembly ATM, and you don't introduce any _WEAK macro > variants below. Well, Andrew specifically mentioned to desire to also have Linux'es support for weak symbols. Hence I decided to add it here despite (for now) being unused). I can certainly drop that again, but in particular if we wanted to use the scheme globally, I think we may want to make it "complete". >> +#define SYM_L_LOCAL(name) /* nothing */ >> + >> +#define SYM_T_FUNC STT_FUNC >> +#define SYM_T_DATA STT_OBJECT >> +#define SYM_T_NONE STT_NOTYPE >> + >> +#define SYM(name, typ, linkage, algn...) \ >> + .type name, SYM_T_ ## typ; \ >> + SYM_L_ ## linkage(name); \ >> + SYM_ALIGN(algn); \ >> + name: >> + >> +#define END(name) .size name, . - name >> + >> +#define ARG1_(x, y...) (x) >> +#define ARG2_(x, y...) ARG1_(y) >> + >> +#define LAST__(nr) ARG ## nr ## _ >> +#define LAST_(nr) LAST__(nr) >> +#define LAST(x, y...) LAST_(count_args(x, ## y))(x, ## y) > > I find LAST not very descriptive, won't it better be named OPTIONAL() > or similar? (and maybe placed in lib.h?) I don't think OPTIONAL describes the purpose. I truly mean "last" here. As to placing in lib.h - perhaps, but then we may want to have forms with more than 2 arguments right away (and it would be a little unclear how far up to go). >> + >> +#define FUNC(name, algn...) \ >> + SYM(name, FUNC, GLOBAL, LAST(16, ## algn), 0x90) > > A rant, should the alignment of functions use a different padding? > (ie: ret or ud2?) In order to prevent stray jumps falling in the > padding and fall trough into the next function. That would also > prevent the implicit fall trough used in some places. Yes, but that's a separate topic (for which iirc patches are pending as well, just of course not integrated with the work here. There's the slight risk of overlooking some "fall-through" case ... >> +#define LABEL(name, algn...) \ >> + SYM(name, NONE, GLOBAL, LAST(16, ## algn), 0x90) >> +#define DATA(name, algn...) \ >> + SYM(name, DATA, GLOBAL, LAST(0, ## algn), 0xff) >> + >> +#define FUNC_LOCAL(name, algn...) \ >> + SYM(name, FUNC, LOCAL, LAST(16, ## algn), 0x90) >> +#define LABEL_LOCAL(name, algn...) \ >> + SYM(name, NONE, LOCAL, LAST(16, ## algn), 0x90) > > Is there much value in adding local labels to the symbol table? > > AFAICT the main purpose of this macro is to be used to declare aligned > labels, and here avoid the ALIGN + label name pair, but could likely > drop the .type directive? Right, .type ... NOTYPE is kind of redundant, but it fits the model better here. >> --- a/xen/arch/x86/x86_64/compat/entry.S >> +++ b/xen/arch/x86/x86_64/compat/entry.S >> @@ -8,10 +8,11 @@ >> #include <asm/page.h> >> #include <asm/processor.h> >> #include <asm/desc.h> >> +#include <xen/lib.h> > > Shouldn't the inclusion of lib.h be in asm_defs.h, as that's where the > usage of count_args() resides? (I assume that's why lib.h is added > here). When the uses are in macros I'm always largely undecided, and I slightly tend towards the (in general, perhaps not overly relevant here) "less dependencies" solution. As in: Source files not using the macros which use count_args() also don't need libs.h then. >> @@ -66,24 +68,21 @@ compat_test_guest_events: >> call compat_create_bounce_frame >> jmp compat_test_all_events >> >> - ALIGN >> /* %rbx: struct vcpu */ >> -compat_process_softirqs: >> +LABEL_LOCAL(compat_process_softirqs) > > Shouldn't this be a local function rather than a local label? It's > fully isolated. I guess it would create issues with > compat_process_trap, as we would then require a jump from the > preceding compat_process_nmi. Alternatives are possible, but right now I consider this an inner label of compat_test_all_events. >> sti >> call do_softirq >> jmp compat_test_all_events >> >> - ALIGN >> /* %rbx: struct vcpu, %rdx: struct trap_bounce */ >> -.Lcompat_process_trapbounce: >> +LABEL_LOCAL(.Lcompat_process_trapbounce) > > It's my understanding that here the '.L' prefix is pointless, since > LABEL_LOCAL() will forcefully create a symbol for the label due to the > usage of .type? I don't think .type has this effect. There's certainly no such label in the symbol table of the object file I have as a result. Jan
