Re: [PATCH v2 1/5] xen/x86: Introduce MultiBoot Data (MBD) type

["Jan Beulich" <JBeulich@suse.com>]

>>> On 25.09.14 at 14:56, <daniel.kiper@oracle.com> wrote:
> On Thu, Sep 25, 2014 at 10:22:16AM +0100, Jan Beulich wrote:
>> >>> On 24.09.14 at 20:40, <andrew.cooper3@citrix.com> wrote:
>> > On 24/09/14 18:19, Daniel Kiper wrote:
>> >> +typedef unsigned char u8;
>> >> +typedef unsigned short u16;
>> >> +typedef unsigned long u32;
>> >> +typedef unsigned long long u64;
>> >> +
>> >> +#include "../../../include/xen/compiler.h"
>> >> +#include "../../../include/xen/multiboot.h"
>> >> +
>> >> +#include "../../../include/asm/mbd.h"
>> >
>> > How about playing with -I for this file in the Makefile to allow
>> > #include <xen/compiler.h> and so ?
>>
>> Including these here is bogus anyway, even if for the ones above it's
> 
> Hmmm... Why it is bogus?

Because the headers aren't intended for the environment this code
runs in.

>> perhaps acceptable. But expressing it to be bogus via the ../../../
>> prefix is quite desirable imo.
> 
> I have been thinking about that since I saw this first time. Why we could
> not use -I gcc option here? Could you enlighten me?

For aforementioned reason: Once we make it easy and seamless to
include further headers, the risk of depending on something this code
shouldn't depend on grows.

Apart from that you also need to deal with dependency tracking if
you include new headers here - please see the Makefile for how
this needs to be done (and which really should have got dropped
by 46fce9fd2b3 ["x86: get rid of BOOT_TRAMPOLINE"]). And yes,
the existing inclusion of multiboot.h is lacking dependency tracking
too (but as usual this is no excuse to continue with this bad practice).

Which points at another issue making it (right now at least) desirable
to don't make it a nobrainer to include further ones: Non-leaf ones
would likely cause missed dependencies as long as they need to be
tracked manually.

>> >> +
>> >> +/*
>> >> + * __HERE__ IS ENTRY POINT!!!
>> >
>> > I am still of the firm opinion that anyone capable of editing this file
>> > ought to know understand the _start symbol, making this comment useless.
>>
>> Indeed.
> 
> We know this right now. However, I spent some time to discover this at
> the beginning of this work. This file is full of magic so I think that
> this comment helps a bit to understand what is going on. So, please
> do me a favor and let's leave it here. If you wish I can use lowercase
> and remove underscore.

And remove the three exclamation marks and fix the grammar.
All in all - much preferred - just drop the comment.

> Additionally, I removed similar comment for
> __reloc() (as you requested) which does not make sense if it is
> prefixed with static.

Making a function referenced from inline assembly only static is likely
wrong anyway. No idea though why removing a similar comment
there would be an issue.

Jan