[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

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: 
generic alloc and copy functions), #6 (x86: introduce MultiBoot Data (MBD) 
#17 (x86/boot: use %ecx instead of %eax) and #18 (xen/x86: add multiboot2 
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 
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.


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

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.



Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.