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/2014 19:04, konrad wilk wrote:
>
>> 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.)
>>
>
> And it _was_ discussed at the Xen hackathon multiple times - where it
> seems it was the perfect place to hash this out.

The majority of the hackathon revolved around what was needed to get EFI
multiboot2 to work.  At the time, there were no patches (if I recall
correctly), and most of the discussion was around what functionality we
needed out of grub to make it work nicely, and what Linux did for EFI
booting.

>
> Or are we saying that any big project in Xen MUST have a design
> document with it? If so we really need to document that somewhere.

I am not in a position to insist on anything.

I have, however, found design documents to be very useful starting
points to get appropriate architectural input from interested parties,
as well as getting agreement in principle for how to proceed.

I will certainly be submitting design documents for any major work I do,
and I hope others will follow suit.

>
> This is frustrating.

I agree.  With my XenServer hat on, we would love multiboot2 support in
Xen 4.5.

However, just throwing patches in would be completely remiss of the
maintainers.  This critical feedback is nothing personal; it reflects
the fact that, in my opinion, the patch series is not of a sufficient
quality to be accepted.  (Note that of the patches are Reviewed-by me,
which I consider to be good quality.)  The flip side of all of this is
that I expect the community to hold me and my patches to the same high
standards; after all, it is Xen which benefits as a result.

As I have indicated before, most of my concerns are caused by a lack of
understanding of *why* changes have been made the way they have.  After
the monolithic patch was split into these 18-or-so, the individual
changes became apparent.  This was good, but still doesn't help with the
*why*.

Take as a perfect example the MBD structure.  It took until this
argument thread for me to understand why it was a good idea in the
context of supporting multiboot1 and multiboot2.  Now that I understand
why, I consider it a good idea.  The problem is that the *why* should be
clearly explained in the patch commit message, (and possibly even in
brief beside the relevant code).  It should not take an argument of this
magnitude to gain a basic understanding of the reasoning behind the changes.

The underlying issue here is that the patch series is trying to do two
completely orthogonal things at once.

Supporting multiboot2 necessarily involves playing with the boot
assembly code, which very likely involves untangling the multiboot1-isms
while doing so.

Unifying the boot paths is completely independent, albeit related.  I
will agree that, in principle, swapping a rats nest of random global
variables for a dedicated collection is a good thing.  Even more so as
it allows the use of more common code on legacy and efi boot paths. 
Implementing this is purely an exercise in shuffling.

The problem is that still, these two end goals are mixed all the way
through this series.  It has taken me until this iteration to work out
that the two goals were completely orthogonal.  Again, this is down to
my lack of understanding of why the changes have been made in the way
they have, which is a fault of the quality of explanation presented with
the series.


To summarise,

I am not against either of the changes per se, but do still have
unresolved issues which give me concerns against the series as a whole,
in its current form.

I highly suggest that multiboot2 is perused independently, (and probably
ahead of) unification.  The first is a new functional piece, while the
second is a massive quantity of cleanup.  These are both good things
independently, but should not be mixed together, especially as they are
causing confusion of each other during review.

I hope I have not ranted for too long, and I hope I am not being
unreasonable (and if you think I am, please call me out on it;  I wont
take it personally)

~Andrew