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

[Daniel Kiper <daniel.kiper@xxxxxxxxxx>]

On Thu, Oct 23, 2014 at 10:55:29PM +0100, Andrew Cooper wrote:
> On 23/10/2014 19:04, konrad wilk wrote:

[...]

> > 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,

I have never said that it should be accepted as is or with bugs. I am
trying to workout the best solution which will improve Xen code and
add missing feature. The best possible Xen code quality is my goal too.

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

AIUI the problem is not in idea per se but in the lack of good
explanation of it? Am I right?

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

Taking into account above thing may we consider patches #5 (x86/boot/reloc: 
create
generic alloc and copy functions), #6 (x86: introduce MultiBoot Data (MBD) 
type),
#17 (x86/boot: use %ecx instead of %eax) and #18 (xen/x86: add multiboot2 
protocol
support) as a base for further EFI + multiboot2 development. If yes then I will
prepare relevant patches on top of them and post all things as whole series 
(with
relevant comments). When it be merged then we could back to discussion about
further boot_info development. I think this is better solution than one which
I sent you yesterday.

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

As I can see you caught the plot. Great!

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

OK.

> To summarise,
>
> I am not against either of the changes per se, but do still have

Great!

> unresolved issues which give me concerns against the series as a whole,
> in its current form.

OK, lat's back to discussion about that after merging EFI + multiboot2 stuff.

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

OK.

Daniel

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel