Hi!

Good news: one that type was fixed Xen booted just fine and detected
all the available 2G of memory.

I am attaching the log if anyone needs to sleuth through it.

Thanks,
Roman.


On Mon, Dec 30, 2019 at 9:14 PM Roman Shaposhnik <roman@zededa.com> wrote:
>
> 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