Re: [PATCH v6 1/5] x86/boot: create a C bundle for 32 bit boot code and use it

["Roger Pau Monné" <roger.pau@citrix.com>]

On Fri, Oct 18, 2024 at 02:45:59PM +0100, Frediano Ziglio wrote:
> On Fri, Oct 18, 2024 at 1:59 PM Roger Pau Monné <roger.pau@citrix.com> wrote:
> >
> > On Fri, Oct 18, 2024 at 01:48:27PM +0100, Frediano Ziglio wrote:
> > > On Fri, Oct 18, 2024 at 12:41 PM Roger Pau Monné <roger.pau@citrix.com> wrote:
> > > >
> > > > On Thu, Oct 17, 2024 at 02:31:19PM +0100, Frediano Ziglio wrote:
> > > > > +#define DECLARE_IMPORT(name) name = . + (__LINE__ * MULT)
> > > > > +
> > > > > +ENTRY(dummy_start)
> > > > >
> > > > >  SECTIONS
> > > > >  {
> > > > >    /* Merge code and data into one section. */
> > > > > -  .text : {
> > > > > +  .text TEXT_START : {
> > > > > +        /* Silence linker warning, we are not going to use it */
> > > > > +        dummy_start = .;
> > > > > +
> > > > > +        /* Declare below any symbol name needed.
> > > > > +         * Each symbol should be on its own line.
> > > > > +         * It looks like a tedious work but we make sure the things we use.
> > > > > +         * Potentially they should be all variables. */
> > > >
> > > > The style is wrong for the opening and closing comment delimiters.
> > > >
> > > > I think it would be best if this was written in a more natural style.
> > > >
> > > > /*
> > > >  * Any symbols used should be declared below, this ensures which
> > > >  * symbols are visible to the 32bit C boot code.
> > > >  */
> > > >
> > >
> > > But why to remove the "Potentially they should be all variables.".
> > > Surely something not written is more clear than something written, but
> > > on the other way it carries no information.
> >
> > I'm not sure I understand why this is helpful: either they are
> > mandated to be only variables, and hence the "potentially" is wrong, or
> > they are not, in which case I don't see why spelling a desire for they
> > to be only variables is helpful if it's not a strict requirement.
> >
> 
> As normal, rules often have exceptions. Most of the functions (so
> code) in Xen is 64 bit, so you don't want to use them. However, saying
> you have a function in head.S written in assembly for 32 bit (or any
> other functions written for 32 bit), you want the possibility to call
> it. For instance you could export from head.S the function to output
> to serial in the future.
> 
> About variables... are all variables fine to be accessed from these
> functions? Probably yes if they have no pointers in them. If they have
> pointers... that's another matter. Does the pointer have relocation?
> Is it going to be used at the final defined program location or only
> during initialization? To make an example, you could override a NULL
> pointer (that is, without relocation) to a current symbol, if this
> pointer is used after Xen is moved into its final position it will
> become invalid. If, on the other hand, the pointer had relocation
> potentially it will be automatically be relocated.

IMO comments are meant to clarify parts of the code.  A comment that
uses a conditional like "Potentially" introduces more ambiguity than
it removes, unless the restriction is stated in the comment itself.

I think you either you expand the comment to mention exactly which
kind of symbols can be declared, or you make the comment a more
restrictive one to avoid the ambiguity: "Symbols declared below should
all be variables."

Thanks, Roger.