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

[Doug Goldstein <cardoe@xxxxxxxxxx>]

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.

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.

-- 
Doug Goldstein

Attachment: signature.asc
Description: OpenPGP digital signature

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