Re: [Xen-devel] [PATCH v3 2/5] x86/boot: Split bootsym() into four types of relocations

[Jan Beulich <jbeulich@xxxxxxxx>]

On 30.08.2019 18:12, David Woodhouse wrote:
> On Fri, 2019-08-30 at 17:10 +0200, Jan Beulich wrote:
>> On 21.08.2019 18:35, David Woodhouse wrote:
>>> --- a/xen/arch/x86/boot/trampoline.S
>>> +++ b/xen/arch/x86/boot/trampoline.S
>>> @@ -16,21 +16,62 @@
>>>   * not guaranteed to persist.
>>>   */
>>>  
>>> -/* NB. bootsym() is only usable in real mode, or via BOOT_PSEUDORM_DS. */
>>> +/*
>>> + * There are four sets of relocations:
>>> + *
>>> + * bootsym():     Boot-time code relocated to low memory and run only once.
>>> + *                Only usable at boot; in real mode or via 
>>> BOOT_PSEUDORM_DS.
>>> + * bootdatasym(): Boot-time BIOS-discovered data, relocated back up to Xen
>>> + *                image after discovery.
>>> + * trampsym():    Permanent trampoline code relocated into low memory for 
>>> AP
>>> + *                startup and wakeup.
>>> + * tramp32sym():  32-bit trampoline code which at boot can be used directly
>>> + *                from the Xen image in memory, but which will need to be
>>> + *                relocated into low (well, into *mapped*) memory in order
>>> + *                to be used for AP startup.
>>> + */
>>>  #undef bootsym
>>>  #define bootsym(s) ((s)-trampoline_start)
>>>  
>>>  #define bootsym_rel(sym, off, opnd...)     \
>>>          bootsym(sym),##opnd;               \
>>>  111:;                                      \
>>> -        .pushsection .trampoline_rel, "a"; \
>>> +        .pushsection .bootsym_rel, "a";    \
>>>          .long 111b - (off) - .;            \
>>>          .popsection
>>>  
>>>  #define bootsym_segrel(sym, off)           \
>>>          $0,$bootsym(sym);                  \
>>>  111:;                                      \
>>> -        .pushsection .trampoline_seg, "a"; \
>>> +        .pushsection .bootsym_seg, "a";    \
>>> +        .long 111b - (off) - .;            \
>>> +        .popsection
>>> +
>>> +#define bootdatasym(s) ((s)-trampoline_start)
>>> +#define bootdatasym_rel(sym, off, opnd...) \
>>> +        bootdatasym(sym),##opnd;           \
>>> +111:;                                      \
>>> +        .pushsection .bootdatasym_rel, "a";\
>>> +        .long 111b - (off) - .;            \
>>> +        .popsection
>>> +
>>> +#undef trampsym
>>> +#define trampsym(s) ((s)-trampoline_start)
>>> +
>>> +#define trampsym_rel(sym, off, opnd...)    \
>>> +        trampsym(sym),##opnd;              \
>>> +111:;                                      \
>>> +        .pushsection .trampsym_rel, "a";   \
>>> +        .long 111b - (off) - .;            \
>>> +        .popsection
>>> +
>>> +#undef tramp32sym
>>> +#define tramp32sym(s) ((s)-trampoline_start)
>>> +
>>> +#define tramp32sym_rel(sym, off, opnd...)  \
>>> +        tramp32sym(sym),##opnd;            \
>>> +111:;                                      \
>>> +        .pushsection .tramp32sym_rel, "a"; \
>>>          .long 111b - (off) - .;            \
>>>          .popsection
>>
>> After your reply to my comment regarding the redundancy here I've
>> checked (in your git branch) how things end up. Am I mistaken, or
>> are the trampsym and tramp32sym #define-s entirely identical
>> (except for the relocations section name)? Even between the others
>> there's little enough difference, so it continues to be unclear to
>> me why you think it's better to have four instances of about the
>> same (not entirely trivial) thing.
> 
> The distinction is that in a no-real-mode boot tramp32 is used in place
> in the Xen image at the physical address it happened to be loaded at,
> and then *again* later in the AP/wakeup path. In the latter case it
> needs to be moved to low memory (or we need to put the physical
> location into idle_pg_table which seemed to be harder, as discussed).
> 
> So tramp32 symbols get relocated *twice*, while the plain tramp symbols
> don't, but actually we could probably ditch the distinction and treat
> them all the same, which would reduce the four categories to three.
> 
> I'll take a look.
> 
> I suppose we could also combine bootsym and bootdatasym, and copy that
> *whole* section back up to the Xen image; both code and data. But I'm
> inclined to prefer keeping them separate and only copying the data back
> up.

My remark here was and is not so much about reducing the number of
instances of separate reloc macros/sections, but about reducing the
redundancy in their definition. At the very least this part

111:;                                      \
        .pushsection .bootdatasym_rel, "a";\
        .long 111b - (off) - .;            \
        .popsection

is identical between all of them, except for the section name, and
hence I'd prefer it to be spelled out just once, and the "actual"
macros then simply using the resulting (helper) macro.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel