Re: [Xen-devel] [PATCH 4/4] x86: add multiboot2 protocol support for EFI platforms

[Daniel Kiper <daniel.kiper@xxxxxxxxxx>]

On Mon, Jan 16, 2017 at 09:28:45AM -0500, Doug Goldstein wrote:
> On 1/16/17 9:11 AM, Daniel Kiper wrote:
> > On Mon, Jan 16, 2017 at 08:41:08AM -0500, Doug Goldstein wrote:
> >> On 1/16/17 7:50 AM, Daniel Kiper wrote:
> >>> On Mon, Jan 16, 2017 at 05:02:05AM -0700, Jan Beulich wrote:
> >>>>>>> On 13.01.17 at 20:21, <cardoe@xxxxxxxxxx> wrote:
> >>>>> Doug v1 - fix incorrect assembly (identified by Andrew Cooper)
> >>>>>         - fix issue where the trampoline size was left as 0 and the
> >>>>>           way the memory is allocated for the trampolines we would go to
> >>>>>           the end of an available section and then subtract off the size
> >>>>>           to decide where to place it. The end result was that we would
> >>>>>           always copy the trampolines and the 32-bit stack into some
> >>>>>           form of reserved memory after the conventional region we
> >>>>>           wanted to put things into. On some systems this did not
> >>>>>           manifest as a crash while on others it did. Reworked the
> >>>>>           changes to always reserve 64kb for both the stack and the size
> >>>>>           of the trampolines. Added an ASSERT to make sure we never blow
> >>>>>           through this size.
> >>>>
> >>>> Without having looked at the patch in detail, but knowing I did closely
> >>>> look at earlier versions (and iirc I was mostly fine with v10) the way
> >>>> the above is written would require me to either inter-diff the patches,
> >>>> or re-review the whole thing. For a large patch like this it would be
> >>>> rather helpful to be quite a bit more specific as to where exactly in the
> >>>> patch changes were made.
> >>>
> >>> I would prefer to not have this patch series applied because it will make 
> >>> me
> >>> more difficult to prepare v12. I hope that I will post it in about 2 
> >>> weeks.
> >>> Though I am going to take into account all comments posted by Doug for 
> >>> v11.
> >>>
> >>> Daniel
> >>>
> >>
> >> Why? They're the first 4 patches remaining of your series. It'll
> >> literally be the following commands:
> >>
> >> git fetch origin
> >> git rebase origin/staging
> >
> > If you changed something then probably this is not so simple.
> > Second, Jan asked me to reorder some patches in the series.
> > Last but not least, your patch series does not contain whole
> > functionality which is needed to properly load Xen using
> > multiboot2 protocol on most EFI platforms. So, as above.
> > Though I appreciate your feedback and I will take all
> > your changes into account.
> >
> > Daniel
> >
>
> Yes there are some code changes which is the point of me sending this.
> But the work for you is the same as having to rebase your series over
> the past few years. Its still a rebase. Its the same thing that everyone
> submitting patches has to do when they submit another version of their
> patches.

Yes, I know. It is always easy to say. And I do not know why you are in
such a hurry. Could not you wait 1-2 weeks?

> I believe you identified 1 machine that had an issue with the 1mb
> region. I've used a 12 machines to test this series and these 4 patches
> work on those machines. The relocation part of the series works on NONE
> of the machines I've tested with. So I would argue the opposite. I still
> haven't identified what's wrong with the relocation part of the series.

This is very interesting. As I know similar thing is used on many
machines for a year or two. And I have not received any complaints
until now. Though I do not say that it is perfect. If you find
something just drop me a line and I will fix it.

Daniel

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