Re: [Xen-devel] [PATCH v2 3/6] x86/boot: Split bootsym() into four types of relocations

[Jan Beulich <jbeulich@xxxxxxxx>]

On 09.08.2019 17:01, 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.
I'm not a native speaker, so my viewing this as ambiguous may be wrong,
but to me it reads as "Only usable at boot or in real mode or via
BOOT_PSEUDORM_DS" when aiui it ought to be "Only usable at boot AND (in
real mode OR via BOOT_PSEUDORM_DS)". In which case how about "Only usable
at boot from real mode or via BOOT_PSEUDORM_DS"?

> + * bootdatasym(): Boot-time BIOS-discovered data, relocated back up to Xen
> + *                image after discovery.
> + * trampsym():    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
This repeats the basically same sequence of things several times.
I've not peeked ahead yet to see in how far more differences would
appear later on, but I don't really expect much of a further
change. In which case it might be nice to reduce the redundancy
here (by introducing a single "base" macro from which the four
similar ones would be derived).

Furthermore, with the intended isolation, wouldn't it be better to
limit visibility of the individual macros, such that using the
wrong one will be easier noticeable? This would be helped by there
being such a single "base" macro, as permitted to use macros could
then be, if needed, defined and undefined perhaps even multiple
times (for the time being at least).

Jan

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