Re: [PATCH for-xen-4.5 v4 00/18] xen: Break multiboot (v1) dependency and add multiboot2 support

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 23/10/14 15:57, Daniel Kiper wrote:
> On Thu, Oct 23, 2014 at 12:08:32PM +0100, Andrew Cooper wrote:
>> On 23/10/14 11:19, Jan Beulich wrote:
>>>>>> On 17.10.14 at 17:49, <daniel.kiper@oracle.com> wrote:
>>>> What is your main concern here (beside that it is big patch series)?
>>> The size alone is not the reason. As already said, I'm not convinced
>>> that the approach you use is ultimately the right one (and Andrew
>>> seems to agree, at least to some degree). And then, looking at your
>>> track record, apart from some kexec (and even older tool stack)
>>> work there hasn't been much you've been doing earlier particularly
>>> with the kind of fragile initial boot code. Hence I think the only
>>> viable route for you to get MB2 code merged is to just add it
>>> _without_ largely re-writing all sorts of other code. Once that
>>> happened we can then go and see whether the consolidation you're
>>> thinking of is (a) the right approach and (b) you're the one to carry
>>> it out.
>>>
>>> Jan
>>>
>> To add to this a little.  I still can't see a reasonable justification
>> for the new "multiboot data" type.  I think it would be perfectly
>> reasonable keep the multiboot1/2 distinction up until the boot_info
>> stage, and cast the pointer based on multiboot magic.

Answering out-of-order:


> I understand that gains are not clearly visible at this stage of development
> because there is lack of relevant multiboot2 + EFI code on top of this patches.

This is entirely the problem.  Acceptance of big changes like this are
first and formost based on the benefits of the changes.

For a change like this, the benefits should be completely obvious to the
reviewers, and this is not the case.  At this point, there are large
numbers of changes all over the boot code.  You have asserted that they
cause an improvement, but have not succeeded is persuading either Jan or
myself.

This is not to say that there are not good reasons, but they are not
obvious.  Furthermore, they have remained not obvious all the way
through review.

> multiboot1 and multiboot2 are completely different things. If we do what
> you suggest then at least we will be forced to complicate assembly code
> parsing cmdline in xen/arch/x86/boot/cmdline.S.

This is a good reason, but it shouldn't become apparent from an email
thread like this.  It should ideally be in the design document
accompanying the series.

> We could also stay with
> multiboot1 stuff but then we will be forced to put all stuff which we are
> able to put in multiboot1. However, we are not able to put at least EFI_HANDLE
> and EFI_SYSTEM_TABLE from multiboot2 (which are a must on EFI; maybe we
> want other data from multiboot2 protocol too) in multiboot1 struct because
> there is not designed place for that. It means that we must add another
> global variables which will appear from nowhere in Xen code (as usual
> in case of global variables).

Globals are fine.

>
> Guys, if you wish I can do what Jan suggest. However, personally I think
> that it will complicate things more which are already complicated enough.
> I can understand that Xen was tied to multiboot1 protocol at early stage of
> development. Even I am able to understand that this thing was not changed
> during introduction of EFI code. However, I think that right know, when we
> introduce third boot protocol, mutliboot2, there is not longer any excuse to
> have it build in early and in main Xen code especially. So, IMO we should
> do all cleanups and fixes first and then put all multiboot2 + EFI stuff
> on top of it. If we reverse the order then I do not believe that cleanup
> will be done in foreseeable future.

One deep problem is that the series is trying to do two independent at
the same time, both in the same area of code.

On the one hand, you have "support multiboot2", whose end goal is
obvious and unquestionably a good thing.  On the other hand, you have
"unify all boot paths", and are using it as a prerequisite to "support
multiboot2" when it really is not a prerequisite.

Furthermore, there is still an open question of whether "unify all boot
paths" is indeed a clever idea in the first place.  Again, this is
ideally something which should have been argued over / decided upon as
part of the design review.  (People on the list might notice a
reoccurring theme there, when it comes to doing large chunks of work.)

If you were to cut the problem in half and just do the multiboot2
support, I suspect you would find it much easier to get the multiboot2
series accepted.  At that point, with the code in-tree, you are in a far
better position to make an argument for unifying the boot paths.

~Andrew