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

[Daniel Kiper <daniel.kiper@xxxxxxxxxx>]

On Wed, Jan 11, 2017 at 01:50:56PM -0600, Doug Goldstein wrote:
> On 1/11/17 1:08 PM, Daniel Kiper wrote:
> > On Mon, Jan 09, 2017 at 07:37:59PM -0600, Doug Goldstein wrote:
> >> On 12/5/16 4:25 PM, Daniel Kiper wrote:

[...]

> >>>      /* Populate E820 table and check trampoline area availability. */
> >>>      e = e820map - 1;
> >>> @@ -168,7 +170,8 @@ static void __init 
> >>> efi_arch_process_memory_map(EFI_SYSTEM_TABLE *SystemTable,
> >>>              /* fall through */
> >>>          case EfiConventionalMemory:
> >>>              if ( !trampoline_phys && desc->PhysicalStart + len <= 
> >>> 0x100000 &&
> >>> -                 len >= cfg.size && desc->PhysicalStart + len > cfg.addr 
> >>> )
> >>> +                 len >= cfg.size + extra_mem &&
> >>> +                 desc->PhysicalStart + len > cfg.addr )
> >>>                  cfg.addr = (desc->PhysicalStart + len - cfg.size) & 
> >>> PAGE_MASK;
> >>
> >> So this is where the current series blows up and fails on real hardware.
> >> No where in the EFI + MB2 code path is cfg.size ever initialized. Its
> >> only initialized in the straight EFI case. The result is that cfg.addr
> >> is set to the section immediately following this. Took a bit to
> >> trackdown because I checked for memory overlaps with where Xen was
> >> loaded and where it was relocated to but forgot to check for overlaps
> >> with the trampoline code. This is the address where the trampoline jumps
> >> are copied.
> >>
> >> Personally I'd like to see an ASSERT added or the code swizzled around
> >> in such a way that its not possible to get into a bad state. But this is
> >> probably another patch series.
> >
> > Nice catch! Thanks a lot. I think that we should initialize cfg.size = 64 
> > << 10
> > in efi_multiboot2(). It looks like real fix. extra_mem stuff is bogus.
>
> Except in head.S you start the stack 64kb after the end of the
> trampoline size. So cfg.size = (64 << 10); won't work. It needs to be
> the size of the trampoline + 64k.

I will double check it and drop you a line. Probably in 1-2 weeks.
Now I am tidying up my backlog after vacation.

> >>> +    efi_tables();
> >>> +    setup_efi_pci();
> >>> +    efi_variables();
> >>
> >> This is probably where you missed the call to "efi_arch_memory_setup();"
> >> that gave me hell above.
> >
> > This does not work in MB2 case.
>
> You're looking at the oldest email. I've have further follow ups that
> point that out and I've included a patch to fix the issues.

I have tried to reply to all your emails.

> >> So as an aside, IMHO this is where the series should end and the next
> >> set of patches should be a follow on.
> >
> > Hmmm... Why? If you do not apply rest of patches then MB2 does not
> > work on all EFI platforms.
> >
> > Daniel
> >
>
> Q: How do you eat an elephant?
> A: One bite at a time.
>
> The point is we have 0 MB2 support presently. We can add it in
> incremental hunks. Otherwise we get a patch series that's been floating

I think that nothing prevents maintainers to apply my patch series partially.
And many prerequisite patches went that way. However, I think that they should
not apply this patch without the rest (and more importantly patch series should
not end at this patch). If we do that we will provide MB2 functionality which
is broken and confuse users. However, if you think that it make sense please
convince maintainers to do that. Though I will not support such request.

> around for around 3 years and missed at least 2 releases where it should

Do not forget that it required a lot of changes in MB2 protocol, GRUB2 (changes
are in git repository and will come into next release), etc. And I have carried
almost all of that stuff myself. Please, believe me this is not easy task.

> have gotten in. We've only got several weeks before the 4.9 window
> closes as well.

Window closes at the end of the march. I am going to release new version
in 1-2 weeks after clearing my backlog. I hope that this patch series
will go into 4.9. At least I will do all my best to achieve that goal.

Daniel

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