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

[Daniel Kiper <daniel.kiper@xxxxxxxxxx>]

On Mon, Feb 29, 2016 at 04:49:04PM +0100, Juergen Gross wrote:
> On 29/02/16 16:43, Daniel Kiper wrote:
> > On Mon, Feb 29, 2016 at 09:27:42AM +0100, Juergen Gross wrote:
> >> On 26/02/16 16:41, Daniel Kiper wrote:
> >>> On Fri, Feb 26, 2016 at 03:28:21PM +0100, Juergen Gross wrote:
> >>>> On 26/02/16 15:00, Daniel Kiper wrote:
> >>>>> 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.
> >>>>
> >>>> Sure it is. Patch 11 just adds allocation to grub_xen_alloc_final(), not
> >>>> after it.
> >>>
> >>> Excerpt from patch 11:
> >>>
> >>> @@ -474,6 +503,12 @@ grub_xen_boot (void)
> >>>    err = grub_xen_alloc_final ();
> >>>    if (err)
> >>>      return err;
> >>> +  if (xen_state.xen_inf.has_p2m_base)
> >>> +    {
> >>> +      err = grub_xen_p2m_alloc ();
> >>> +      if (err)
> >>> +        return err;
> >>> +    }
> >>>
> >>
> >> Hmph. No idea what I looked at when writing my previous reply.
> >>
> >>>>> So, maybe it should be called grub_xen_alloc_boot_data() or something 
> >>>>> like that.
> >>>
> >>> Then my previous comment is still valid.
> >>
> >> What about naming it grub_xen_alloc_kernel_mapping() ? This is what it
> >> really does: It is allocating all not yet allocated areas which are to
> >> be included in the initial kernel mapping.
> >
> > This function does three things:
> >   - allocate memory for p2m array,
> >   - allocate special pages (e.g. console),
> >   - allocate memory for page tables.
> >
> > Three of them are part of boot data described by struct start_info.
> > Well, there are also other required stuff to properly boot PV guest.
> > So, I can agree that grub_xen_alloc_boot_data() is not perfect but
> > I cannot find anything better. Additionally, IMO 
> > grub_xen_alloc_kernel_mapping()
> > is not right choice too. It suggests that this functions does anything
> > with kernel mapping. Yep, it does, however, this is only one third
> > of this function. So, I tend to use my proposal or anything better
> > which is short and quite well describes what is going in this
> > function as whole.
>
> Yeah. grub_xen_alloc_stuff() with a nice comment. ;-)

:-))) I thought about that one too!

> Okay, lets stick with grub_xen_alloc_boot_data() and add a comment
> explaining that the function will allocate everything not yet allocated
> and covered by the initial page tables.

LGTM. Granted!

Daniel

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