Re: [Xen-devel] REGRESSION: Xen 4.13 RC5 fails to bootstrap Dom0 on ARM

[Roman Shaposhnik <roman@zededa.com>]

Hi Julien,

On Sun, Dec 29, 2019 at 10:01 AM Julien Grall <julien@xen.org> wrote:
>
> Hi,
>
> On 21/12/2019 01:37, Roman Shaposhnik wrote:
> > On Thu, Dec 19, 2019 at 4:01 PM Stefano Stabellini
> > <sstabellini@kernel.org> wrote:
> >>
> >> On Thu, 19 Dec 2019, Julien Grall wrote:
> >>>>> In fact most of people on Arm are using GRUB rather than EFI directly as
> >>>>> this is more friendly to use.
> >>>>>
> >>>>> Regarding the devicetree, Xen and Linux will completely ignore the
> >>>>> memory nodes in Xen if using EFI. This because the EFI memory map will
> >>>>> give you an overview of the platform with the EFI regions included.
> >>>>
> >>>> Aha! So in that sense it is a bug in Xen after all, right? (that's what
> >>>> you're
> >>>> referring to when you say you now understand what needs to get fixed).
> >>>
> >>> Yes. The EFI memory map is a list of existing memory with a type associated to
> >>> it (Conventional, BootServiceCodes, MemoryMappedIO...).
> >>>
> >>> The OS/Hypervisor will have to go through them and check which regions are
> >>> usuable. Compare to Linux, Xen has limited itself to only a few types.
> >>>
> >>> However, I think we can be on a par with Linux here.
> >>
> >> I gave a look at the Linux implementation, the interesting bit is
> >> drivers/firmware/efi/arm-init.c:is_usable_memory as far as I can tell.
> >> I also gave a look at the Xen side, which is
> >> xen/arch/arm/efi/efi-boot.h:efi_process_memory_map_bootinfo. As guessed,
> >> the two are not quite the same.
> >>
> >> One of the main differences is that Linux uses as "System RAM" even
> >> regions that were marked as EFI_BOOT_SERVICES_CODE/DATA and
> >> EFI_LOADER_CODE/DATA because they will get freed anyway. Xen doesn't
> >> do that unless map_bs is set.
> >>
> >> I wrote a quick patch to implement the Linux behavior on Xen, only
> >> lightly tested. I can confirm that I see more memory this way. However,
> >> I am not sure we actually want to import the Linux behavior wholesale.
> >>
> >> Anyway, Roman, could you please let me know if this patch solves the
> >> issue?
> >
> > Tried the attached patch -- but it seems I can't boot at all with this. Xen
> > doesn't print anything on the console either.
>
> Thank you for trying the patch. Do you have earlyprintk enabled for the
> hikey board?

No (since I thought it wasn't possible on ARM :-)) but now that you
mentioned it,
I've found this:
     http://xenbits.xenproject.org/docs/4.13-testing/misc/arm/early-printk.txt
and I'd be more than happy to try (hopefully CONFIG_EARLY_PRINTK= hikey960
will do the trick).

> > To Julien's point -- should I reduce the # of types and try again?
>
>  From my understanding, the field Attribute is a series of flag telling
> what the region can support.
>
> So it would be possible to have other flags set at the same time as
> EFI_MEMORY_WC. However, the check in the patch below is an == equal and
> would potentially discard a lot of regions (if not all regions).
>
> In other words...
>
> >>
> >>
> >> diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
> >> index ca655ff003..ad18ff3669 100644
> >> --- a/xen/arch/arm/efi/efi-boot.h
> >> +++ b/xen/arch/arm/efi/efi-boot.h
> >> @@ -149,10 +149,14 @@ static EFI_STATUS __init efi_process_memory_map_bootinfo(EFI_MEMORY_DESCRIPTOR *
> >>
> >>       for ( Index = 0; Index < (mmap_size / desc_size); Index++ )
> >>       {
> >> -        if ( desc_ptr->Type == EfiConventionalMemory ||
> >> -             (!map_bs &&
> >> -              (desc_ptr->Type == EfiBootServicesCode ||
> >> -               desc_ptr->Type == EfiBootServicesData)) )
> >> +        if ( desc_ptr->Attribute == EFI_MEMORY_WB &&
>
> ... this should be desc_ptr->Attribute & EFI_MEMORY_WB.
>
> Can you give a spin with this change and see how far you can go?

Aha! That makes much more sense -- will give it a try tomorrow
(in conjunction with earlyprintk)

Thanks,
Roman.

> >> +             (desc_ptr->Type == EfiConventionalMemory ||
> >> +              desc_ptr->Type == EfiLoaderCode ||
> >> +              desc_ptr->Type == EfiLoaderData ||
> >> +              desc_ptr->Type == EfiACPIReclaimMemory ||
> >> +              desc_ptr->Type == EfiPersistentMemory ||
> >> +              desc_ptr->Type == EfiBootServicesCode ||
> >> +              desc_ptr->Type == EfiBootServicesData) )
> >>           {
> >>               if ( !meminfo_add_bank(&bootinfo.mem, desc_ptr) )
> >>               {
> >> diff --git a/xen/include/efi/efidef.h b/xen/include/efi/efidef.h
> >> index 86a7e111bf..f46207840f 100644
> >> --- a/xen/include/efi/efidef.h
> >> +++ b/xen/include/efi/efidef.h
> >> @@ -147,6 +147,7 @@ typedef enum {
> >>       EfiMemoryMappedIO,
> >>       EfiMemoryMappedIOPortSpace,
> >>       EfiPalCode,
> >> +    EfiPersistentMemory,
> >>       EfiMaxMemoryType
> >>   } EFI_MEMORY_TYPE;
> >>
>
> Cheers,
>
> --
> Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel