Re: [Xen-devel] [PATCH v2 3/6] xen: factor out allocation of page tables into separate function

[Daniel Kiper <daniel.kiper@xxxxxxxxxx>]

On Thu, Feb 11, 2016 at 01:53:51PM +0100, Juergen Gross wrote:
> On 11/02/16 13:27, Daniel Kiper wrote:
> > On Thu, Feb 11, 2016 at 08:53:23AM +0100, Juergen Gross wrote:
> >> Do the allocation of page tables in a separate function. This will
> >> allow to do the allocation at different times of the boot preparations
> >> depending on the features the kernel is supporting.
> >>
> >> Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> >> ---
> >>  grub-core/loader/i386/xen.c | 82 
> >> ++++++++++++++++++++++++++++-----------------
> >>  1 file changed, 51 insertions(+), 31 deletions(-)
> >>
> >> diff --git a/grub-core/loader/i386/xen.c b/grub-core/loader/i386/xen.c
> >> index e48cc3f..65cec27 100644
> >> --- a/grub-core/loader/i386/xen.c
> >> +++ b/grub-core/loader/i386/xen.c
> >> @@ -56,6 +56,9 @@ static struct grub_relocator_xen_state state;
> >>  static grub_xen_mfn_t *virt_mfn_list;
> >>  static struct start_info *virt_start_info;
> >>  static grub_xen_mfn_t console_pfn;
> >> +static grub_uint64_t *virt_pgtable;
> >> +static grub_uint64_t pgtbl_start;
> >> +static grub_uint64_t pgtbl_end;
> >
> > Same as in patches #1 and #2.
>
> Yep.
>
> >
> >>  #define PAGE_SIZE 4096
> >>  #define MAX_MODULES (PAGE_SIZE / sizeof (struct xen_multiboot_mod_list))
> >> @@ -106,17 +109,17 @@ get_pgtable_size (grub_uint64_t total_pages, 
> >> grub_uint64_t virt_base)
> >>
> >>  static void
> >>  generate_page_table (grub_uint64_t *where, grub_uint64_t paging_start,
> >> -               grub_uint64_t total_pages, grub_uint64_t virt_base,
> >> -               grub_xen_mfn_t *mfn_list)
> >> +               grub_uint64_t paging_end, grub_uint64_t total_pages,
> >> +               grub_uint64_t virt_base, grub_xen_mfn_t *mfn_list)
> >>  {
> >>    if (!virt_base)
> >> -    total_pages++;
> >> +    paging_end++;
> >>
> >>    grub_uint64_t lx[NUMBER_OF_LEVELS], lxs[NUMBER_OF_LEVELS];
> >>    grub_uint64_t nlx, nls, sz = 0;
> >>    int l;
> >>
> >> -  nlx = total_pages;
> >> +  nlx = paging_end;
> >>    nls = virt_base >> PAGE_SHIFT;
> >>    for (l = 0; l < NUMBER_OF_LEVELS; l++)
> >>      {
> >> @@ -160,7 +163,7 @@ generate_page_table (grub_uint64_t *where, 
> >> grub_uint64_t paging_start,
> >>    if (pr)
> >>      pg += POINTERS_PER_PAGE;
> >>
> >> -  for (j = 0; j < total_pages; j++)
> >> +  for (j = 0; j < paging_end; j++)
> >>      {
> >>        if (j >= paging_start && j < lp)
> >>    pg[j + lxs[0]] = page2offset (mfn_list[j]) | 5;
> >> @@ -261,24 +264,12 @@ grub_xen_special_alloc (void)
> >>  }
> >>
> >>  static grub_err_t
> >> -grub_xen_boot (void)
> >> +grub_xen_pt_alloc (void)
> >>  {
> >>    grub_relocator_chunk_t ch;
> >>    grub_err_t err;
> >>    grub_uint64_t nr_info_pages;
> >>    grub_uint64_t nr_pages, nr_pt_pages, nr_need_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");
> >> -
> >> -  err = grub_xen_p2m_alloc ();
> >> -  if (err)
> >> -    return err;
> >> -  err = grub_xen_special_alloc ();
> >> -  if (err)
> >> -    return err;
> >>
> >>    next_start.pt_base = max_addr + xen_inf.virt_base;
> >>    state.paging_start = max_addr >> PAGE_SHIFT;
> >> @@ -298,30 +289,59 @@ grub_xen_boot (void)
> >>        nr_pages = nr_need_pages;
> >>      }
> >>
> >> -  grub_dprintf ("xen", "bootstrap domain %llx+%llx\n",
> >> -          (unsigned long long) xen_inf.virt_base,
> >> -          (unsigned long long) page2offset (nr_pages));
> >> -
> >>    err = grub_relocator_alloc_chunk_addr (relocator, &ch,
> >>                                     max_addr, page2offset (nr_pt_pages));
> >>    if (err)
> >>      return err;
> >>
> >> +  virt_pgtable = get_virtual_current_address (ch);
> >> +  pgtbl_start = max_addr >> PAGE_SHIFT;
> >> +  max_addr += page2offset (nr_pt_pages);
> >> +  state.stack = max_addr + STACK_SIZE + xen_inf.virt_base;
> >> +  state.paging_size = nr_pt_pages;
> >> +  next_start.nr_pt_frames = nr_pt_pages;
> >> +  max_addr = page2offset (nr_pages);
> >> +  pgtbl_end = nr_pages;
> >> +
> >> +  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");
> >> +
> >> +  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;
> >
> > Should not we free memory allocated by grub_xen_p2m_alloc() and
> > grub_xen_special_alloc() if grub_xen_pt_alloc() failed?
>
> Hmm, why? If I don't miss anything freeing memory in case of error isn't
> done anywhere (at least not in this source file).

It seems that memory is freed by higher level functions when you
try to use the same loader or other one. Otherwise it would be clear
memory leak. Could you double check it?

Daniel

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