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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 30.05.2023 15:21, Roger Pau Monné wrote:
> 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.

Is there actually any particular reason for this restriction? As long
as old and new code has the same external entry points, redirecting
all old ones to their new counterparts would seem feasible.

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

Right, but splitting functions into piecemeal fragments isn't going
to be very nice either.

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

I've added a sentence to this effect to the description.

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

I'll think about this some more.

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

Well, those don't cover padding yet, but they deal with straight-line
speculation past RET or JMP.

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

But .L symbols are still normal symbols to gas, just that it knows to not
emit them to the symbol table (unless there's a need, e.g. through a use
in a relocation that cannot be expressed as section-relative one). It
could flag the pointless use, but then it may get this wrong if in the
end the symbol does need emitting.

Jan