Re: [PATCH v2 1/2] x86: annotate entry points with type and size

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Tue, May 30, 2023 at 10:06:27AM +0200, Jan Beulich wrote:
> 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".

Any external entry point in the middle of a function-like block will
prevent it from being live patched.

If you want I can try to do a pass on top of your patch and see how
that would end up looking.  I'm attempting to think about other
solutions, but every other solution seems quite horrible.

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

OK, as long as we know it's unused.

> >> +#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).

Hm, I would be fine with adding that version with just 2 arguments, as
it's better to have the helper in a generic place IMO.

> >> +
> >> +#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 ...

Oh, OK, wasn't aware patches are floating for this already, just came
across it while reviewing.

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

I tend to prefer headers to be self contained, as it overall leads to
a clearer set of includes in source files.  It's not obvious why
entry.S needs lib.h unless the asm_macros.h usage is taken into
account.

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

I was expecting .type to force the creation of a symbol, so the '.L'
prefix does prevent the symbol from being created even if .type is
specified.

Shouldn't the assembler complain that we are attempting to set a type
for a not present symbol?

Thanks, Roger.