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

[Jan Beulich <jbeulich@xxxxxxxx>]

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