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

[David Woodhouse <dwmw2@infradead.org>]

[-- Attachment #1.1: Type: text/plain, Size: 6161 bytes --]

Apologies for delayed response; I was away last week and was frowned at
every time I so much as looked towards the laptop.


On Mon, 2019-08-12 at 11:41 +0200, Jan Beulich wrote:
> 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"?

Yes, I suppose I see that ambiguity. But ultimately I file it under the
category of "don't hack boot code while drunk".

I agree that to the reader of English (native or otherwise), that
sentence may have either meaning. 

But this isn't documentation; it's code comments in an assembler file.
Anyone who is actually going to make a meaningful contribution to the
boot code, might reasonably be expected to understand that "real mode
or using the pseudo-realmode segment selector" are grouped together,
and that they must use one or the other of those. At boot time.

This is not an attempt at a two-line tutorial on all the pitfalls of
touching the boot code/data; via bootsym() or otherwise. It's just a
pointer in the right direction.

But sure, I'll have a look at fixing it — if I don't feel that what I
come up with is too clumsy.

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

They end up being more different than this. It was my judgement that
attempting to create a more generic building block from which they
could all be derived would end up being less readable than just a
little bit of partial duplication. If you feel strongly otherwise, I
would welcome a follow-on patch to attempt to remedy that, if you
choose to send one.

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

I think I'd class that under 'don't hack boot code while drunk" too,
which is apparently the existing approach taken by this most of code
(except on the occasions when people have clearly done precisely that
☺).

The other .S files are *included* from head.S; some indirectly via
trampoline.S. Various macros are defined just once in head.S rather
than being in a header file or with mixed visibility. There is a
potential cleanup to be done there, but it is not one of the cleanups I
had elected to make because I see it as being of limited utility except
cosmetic. Again, if you feel strongly then I would welcome a follow-on
patch or series to move everything out and build each .S file as a
separate compilation unit.




[-- Attachment #1.2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5174 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

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