Re: [Xen-devel] [PATCH for-xen-4.5 v3 09/16] x86: Move legacy BIOS memory map stuff to boot_info

[Daniel Kiper <daniel.kiper@xxxxxxxxxx>]

On Fri, Oct 10, 2014 at 03:02:43PM +0100, Stefano Stabellini wrote:
> On Wed, 8 Oct 2014, Daniel Kiper wrote:
> > Signed-off-by: Daniel Kiper <daniel.kiper@xxxxxxxxxx>

[...]

> > diff --git a/xen/common/efi/runtime.c b/xen/common/efi/runtime.c
> > index ee2ee2d..865d0bc 100644
> > --- a/xen/common/efi/runtime.c
> > +++ b/xen/common/efi/runtime.c
> > @@ -7,6 +7,7 @@
> >  #include <xen/time.h>
> >  #ifndef CONFIG_ARM /* TODO - disabled until implemented on ARM */
> >  #include <asm/boot_info.h>
> > +#include <asm/e820.h>
> >  #endif
> >
> >  DEFINE_XEN_GUEST_HANDLE(CHAR16);
> > @@ -53,9 +54,15 @@ struct efi __read_mostly efi = {
> >  const struct efi_pci_rom *__read_mostly efi_pci_roms;
> >
> >  #ifndef CONFIG_ARM /* TODO - disabled until implemented on ARM */
> > +extern struct e820entry e820map[];
> > +
> >  boot_info_t __read_mostly boot_info_efi = {
> >      .boot_loader_name = "EFI",
> >      .cmdline = NULL,
> > +    .mmap_type = "EFI",
> > +    .mem_upper = 0,
> > +    .e820map_nr = 0,
> > +    .e820map = e820map,
> >      .warn_msg = NULL,
> >      .err_msg = NULL
> >  };
>
> This is not going in the right direction of getting rid of arch-specific
> concepts from xen/common/efi/runtime.c.

I thought about that a bit and decided to do this in that way right now.
However, I agree that finally (after introducing boot_info into ARM) there
should be an arch_boot_info_init() function which will initialize all
architecture specific stuff here.

> > diff --git a/xen/include/asm-x86/boot_info.h 
> > b/xen/include/asm-x86/boot_info.h
> > index d74ed67..640a420 100644
> > --- a/xen/include/asm-x86/boot_info.h
> > +++ b/xen/include/asm-x86/boot_info.h
> > @@ -18,6 +18,10 @@
> >  #ifndef __BOOT_INFO_H__
> >  #define __BOOT_INFO_H__
> >
> > +#include <xen/types.h>
> > +
> > +#include <asm/e820.h>
> > +
> >  /*
> >   * Define boot_info type. It will be used to define variable which in turn
> >   * will store data collected by bootloader and preloader. This way it will
> > @@ -35,6 +39,24 @@ typedef struct {
> >      /* Xen command line. */
> >      char *cmdline;
> >
> > +    /* Memory map type (source of memory map). */
> > +    char *mmap_type;
>
> I realize that you inherited mmap_type from existing code, but shouldn't
> this be an enum?

This is a boot loader name. I will improve the comment here.

Daniel

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