Re: [Xen-devel] [PATCH v5 09/11] xen: add capability to load initrd outside of initial mapping

[Daniel Kiper <daniel.kiper@xxxxxxxxxx>]

On Thu, Feb 25, 2016 at 04:33:46PM +0100, Juergen Gross wrote:
> On 25/02/16 13:47, Daniel Kiper wrote:
> > On Thu, Feb 25, 2016 at 12:33:35PM +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>
> >> ---
> >> V5: let call grub_xen_alloc_final() all subfunctions unconditionally
> >>     and let them decide whether they need to do anything
> >> V4: rename grub_xen_alloc_end() to grub_xen_alloc_final()
> >> ---
> >>  grub-core/loader/i386/xen.c        | 65 
> >> ++++++++++++++++++++++++++++++--------
> >>  grub-core/loader/i386/xen_fileXX.c |  3 ++
> >>  include/grub/xen_file.h            |  1 +
> >>  3 files changed, 56 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/grub-core/loader/i386/xen.c b/grub-core/loader/i386/xen.c
> >> index 2e12763..466f0c0 100644
> >> --- a/grub-core/loader/i386/xen.c
> >> +++ b/grub-core/loader/i386/xen.c
> >> @@ -228,6 +228,9 @@ grub_xen_p2m_alloc (void)
> >>    grub_size_t p2msize;
> >>    grub_err_t err;
> >>
> >> +  if (xen_state.virt_mfn_list)
> >> +    return GRUB_ERR_NONE;
> >> +
> >>    xen_state.state.mfn_list = xen_state.max_addr;
> >>    xen_state.next_start.mfn_list =
> >>      xen_state.max_addr + xen_state.xen_inf.virt_base;
> >> @@ -250,6 +253,9 @@ grub_xen_special_alloc (void)
> >>    grub_relocator_chunk_t ch;
> >>    grub_err_t err;
> >>
> >> +  if (xen_state.virt_start_info)
> >> +    return GRUB_ERR_NONE;
> >> +
> >>    err = grub_relocator_alloc_chunk_addr (xen_state.relocator, &ch,
> >>                                     xen_state.max_addr,
> >>                                     sizeof (xen_state.next_start));
> >> @@ -281,6 +287,9 @@ grub_xen_pt_alloc (void)
> >>    grub_uint64_t nr_info_pages;
> >>    grub_uint64_t nr_pages, nr_pt_pages, nr_need_pages;
> >>
> >> +  if (xen_state.virt_pgtable)
> >> +    return GRUB_ERR_NONE;
> >> +
> >>    xen_state.next_start.pt_base =
> >>      xen_state.max_addr + xen_state.xen_inf.virt_base;
> >>    xen_state.state.paging_start = xen_state.max_addr >> PAGE_SHIFT;
> >> @@ -320,6 +329,24 @@ grub_xen_pt_alloc (void)
> >>  }
> >>
> >>  static grub_err_t
> >> +grub_xen_alloc_final (void)
> >> +{
> >> +  grub_err_t err;
> >> +
> >> +  err = grub_xen_p2m_alloc ();
> >> +  if (err)
> >> +    return err;
> >> +  err = grub_xen_special_alloc ();
> >> +  if (err)
> >> +    return err;
> >> +  err = grub_xen_pt_alloc ();
> >> +  if (err)
> >> +    return err;
> >
> > Could you move grub_xen_p2m_alloc() here? This way grub_xen_alloc_final()
> > will be real final in patch 11 and you do not need an extra condition
> > around grub_xen_p2m_alloc().
>
> No. Page tables must be allocated after p2m unless p2m is outside of
> initial kernel mapping (patch 11).

OK, I was afraid of that. However, then grub_xen_alloc_final() is not final.
So, maybe it should be called grub_xen_alloc_boot_data() or something like that.

> >> +  return GRUB_ERR_NONE;
> >> +}
> >> +
> >> +static grub_err_t
> >>  grub_xen_boot (void)
> >>  {
> >>    grub_err_t err;
> >> @@ -330,13 +357,7 @@ grub_xen_boot (void)
> >>    if (grub_xen_n_allocated_shared_pages)
> >>      return grub_error (GRUB_ERR_BUG, "active grants");
> >>
> >> -  err = grub_xen_p2m_alloc ();
> >> -  if (err)
> >> -    return err;
> >> -  err = grub_xen_special_alloc ();
> >> -  if (err)
> >> -    return err;
> >> -  err = grub_xen_pt_alloc ();
> >> +  err = grub_xen_alloc_final ();
> >
> > if (!xen_state.xen_inf.unmapped_initrd)
> >   {
> >     err = grub_xen_alloc_final ();
> >     if (err)
> >       goto fail;
> >   }
> >
> > This way you avoid checks in grub_xen_p2m_alloc(),
> > grub_xen_special_alloc() and grub_xen_pt_alloc().
>
> No. In case there is no initrd, but the kernel does support an unmapped
> initrd, I would omit allocating special pages, p2m and page tables.

Ups... I have missed that.

> >>    if (err)
> >>      return err;
> >>
> >> @@ -610,6 +631,13 @@ grub_cmd_initrd (grub_command_t cmd __attribute__ 
> >> ((unused)),
> >>        goto fail;
> >>      }
> >>
> >> +  if (xen_state.xen_inf.unmapped_initrd)
> >> +    {
> >> +      err = grub_xen_alloc_final ();
> >> +      if (err)
> >> +  goto fail;
> >> +    }
> >> +
> >>    if (grub_initrd_init (argc, argv, &initrd_ctx))
> >>      goto fail;
> >>
> >> @@ -627,14 +655,24 @@ grub_cmd_initrd (grub_command_t cmd __attribute__ 
> >> ((unused)),
> >>    goto fail;
> >>      }
> >>
> >> -  xen_state.next_start.mod_start =
> >> -    xen_state.max_addr + xen_state.xen_inf.virt_base;
> >> -  xen_state.next_start.mod_len = size;
> >
> > Leave "xen_state.next_start.mod_len = size;" as is here and...
> >
> >> -
> >> -  xen_state.max_addr = ALIGN_UP (xen_state.max_addr + size, PAGE_SIZE);
> >> +  if (xen_state.xen_inf.unmapped_initrd)
> >> +    {
> >> +      xen_state.next_start.flags |= SIF_MOD_START_PFN;
> >> +      xen_state.next_start.mod_start = xen_state.max_addr >> PAGE_SHIFT;
> >> +      xen_state.next_start.mod_len = size;
> >
> > ... do not add it here...
> >
> >> +    }
> >> +  else
> >> +    {
> >> +      xen_state.next_start.mod_start =
> >> +  xen_state.max_addr + xen_state.xen_inf.virt_base;
> >> +      xen_state.next_start.mod_len = size;
> >
> > ... and here...
> >
> >> +    }
> >
> > ... and you do not need curly brackets then.
>
> I still do, as in the "if" part I still have two statements.

Yep, but else stuff does not need one.

> >>    grub_dprintf ("xen", "Initrd, addr=0x%x, size=0x%x\n",
> >> -          (unsigned) xen_state.next_start.mod_start, (unsigned) size);
> >> +          (unsigned) (xen_state.max_addr + xen_state.xen_inf.virt_base),
> >> +          (unsigned) size);
> >
> > This should be printed only if !xen_state.xen_inf.unmapped_initrd.
>
> Really? Why? We still have an initrd.
>
> > Why do not use xen_state.next_start.mod_start here?
>
> It is the pfn in case of unmapped_initrd.

Errr... I have looked at wrong line. Sorry about that.

Daniel

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