Re: [Xen-devel] [PATCH v2 4/6] xen: add capability to load initrd outside of initial mapping

[Daniel Kiper <daniel.kiper@xxxxxxxxxx>]

On Thu, Feb 11, 2016 at 03:13:40PM +0100, Juergen Gross wrote:
> On 11/02/16 13:33, Daniel Kiper wrote:
> > On Thu, Feb 11, 2016 at 08:53:24AM +0100, Juergen Gross wrote:
> >> Modern pvops linux kernels support an initrd not covered by the initial
> >> mapping. This capability is flagged by an elf-note.
> >>
> >> In case the elf-note is set by the kernel don't place the initrd into
> >> the initial mapping. This will allow to load larger initrds and/or
> >> support domains with larger memory, as the initial mapping is limited
> >> to 2GB and it is containing the p2m list.
> >>
> >> Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> >> ---
> >>  grub-core/loader/i386/xen.c        | 56 
> >> ++++++++++++++++++++++++++++++--------
> >>  grub-core/loader/i386/xen_fileXX.c |  3 ++
> >>  include/grub/xen_file.h            |  1 +
> >>  3 files changed, 49 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/grub-core/loader/i386/xen.c b/grub-core/loader/i386/xen.c
> >> index 65cec27..0f41048 100644
> >> --- a/grub-core/loader/i386/xen.c
> >> +++ b/grub-core/loader/i386/xen.c
> >> @@ -307,15 +307,14 @@ grub_xen_pt_alloc (void)
> >>  }
> >>
> >>  static grub_err_t
> >> -grub_xen_boot (void)
> >> +grub_xen_alloc_end (void)
> >>  {
> >>    grub_err_t err;
> >> -  grub_uint64_t nr_pages;
> >> -  struct gnttab_set_version gnttab_setver;
> >> -  grub_size_t i;
> >> +  static int called = 0;
> >>
> >> -  if (grub_xen_n_allocated_shared_pages)
> >> -    return grub_error (GRUB_ERR_BUG, "active grants");
> >> +  if (called)
> >> +    return GRUB_ERR_NONE;
> >
> > Why?
>
> Did you look at the function? grub_xen_alloc_end() (some parts of the
> original grub_xen_boot()) is new and may be called multiple times. It
> was much easier to just call it and let it do what must be done only
> at the first time called.

OK, but this means that you can use this loader only once. If you make
a mistake in boot command line then you cannot fix it by calling this
loader again because grub_xen_alloc_end() will fail. Am I right?

> >> +  called = 1;
> >>
> >>    err = grub_xen_p2m_alloc ();
> >>    if (err)
> >> @@ -327,6 +326,24 @@ grub_xen_boot (void)
> >>    if (err)
> >>      return err;
> >>
> >> +  return GRUB_ERR_NONE;
> >> +}
> >> +
> >> +static grub_err_t
> >> +grub_xen_boot (void)
> >> +{
> >> +  grub_err_t err;
> >> +  grub_uint64_t nr_pages;
> >> +  struct gnttab_set_version gnttab_setver;
> >> +  grub_size_t i;
> >> +
> >> +  if (grub_xen_n_allocated_shared_pages)
> >> +    return grub_error (GRUB_ERR_BUG, "active grants");
> >
> > Why?
>
> That's how it has been before. git just decided to generate the diff
> that way.

Could you try --patience git option?

> >> +  err = grub_xen_alloc_end ();
> >> +  if (err)
> >> +    return err;
> >> +
> >>    err = set_mfns (console_pfn);
> >>    if (err)
> >>      return err;
> >> @@ -587,6 +604,13 @@ grub_cmd_initrd (grub_command_t cmd __attribute__ 
> >> ((unused)),
> >>        goto fail;
> >>      }
> >>
> >> +  if (xen_inf.unmapped_initrd)
> >> +    {
> >> +      err = grub_xen_alloc_end ();
> >> +      if (err)
> >> +        goto fail;
> >> +    }
> >> +
> >>    if (grub_initrd_init (argc, argv, &initrd_ctx))
> >>      goto fail;
> >>
> >> @@ -603,13 +627,22 @@ grub_cmd_initrd (grub_command_t cmd __attribute__ 
> >> ((unused)),
> >>    goto fail;
> >>      }
> >>
> >> -  next_start.mod_start = max_addr + xen_inf.virt_base;
> >> -  next_start.mod_len = size;
> >> -
> >> -  max_addr = ALIGN_UP (max_addr + size, PAGE_SIZE);
> >> +  if (xen_inf.unmapped_initrd)
> >> +    {
> >> +      next_start.flags |= SIF_MOD_START_PFN;
> >> +      next_start.mod_start = max_addr >> PAGE_SHIFT;
> >> +      next_start.mod_len = size;
> >> +    }
> >> +  else
> >> +    {
> >> +      next_start.mod_start = max_addr + xen_inf.virt_base;
> >> +      next_start.mod_len = size;
> >> +    }
> >>
> >>    grub_dprintf ("xen", "Initrd, addr=0x%x, size=0x%x\n",
> >> -          (unsigned) next_start.mod_start, (unsigned) size);
> >> +          (unsigned) (max_addr + xen_inf.virt_base), (unsigned) size);
> >> +
> >> +  max_addr = ALIGN_UP (max_addr + size, PAGE_SIZE);
> >>
> >>  fail:
> >>    grub_initrd_close (&initrd_ctx);
> >> @@ -660,6 +693,7 @@ grub_cmd_module (grub_command_t cmd __attribute__ 
> >> ((unused)),
> >>
> >>    if (!xen_module_info_page)
> >>      {
> >> +      xen_inf.unmapped_initrd = 0;
> >>        n_modules = 0;
> >>        max_addr = ALIGN_UP (max_addr, PAGE_SIZE);
> >>        modules_target_start = max_addr;
> >> diff --git a/grub-core/loader/i386/xen_fileXX.c 
> >> b/grub-core/loader/i386/xen_fileXX.c
> >> index 1ba5649..69fccd2 100644
> >> --- a/grub-core/loader/i386/xen_fileXX.c
> >> +++ b/grub-core/loader/i386/xen_fileXX.c
> >> @@ -253,6 +253,9 @@ parse_note (grub_elf_t elf, struct grub_xen_file_info 
> >> *xi,
> >>                                      descsz == 2 ? 2 : 3) == 0)
> >>        xi->arch = GRUB_XEN_FILE_I386;
> >>      break;
> >> +  case 16:
> >
> > Could you define this a constant and use it here?
>
> This would be the only case with a constant. All others are numeric
> as well.

Ugh... So, please, please fix it at first. It is not an excuse
if somebody before you made a mistake (let's call it in that way).
Or at least put comments what these values mean. However, I prefer
constants. Constants with comments (at least in header file) would
be perfect.

Daniel

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