| Message ID | 1455177206-8974-4-git-send-email-jgross@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@suse.com> > --- > 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. > #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? Daniel
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@suse.com> >> --- >> 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). Juergen
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@suse.com> > >> --- > >> 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
On 11/02/16 18:14, Daniel Kiper wrote: > 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@suse.com> >>>> --- >>>> 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? Yes, you are right. Other loaders are calling grub_relocator_unload() to achieve this before allocating a new relocator or when unloading the module. Juergen
On 11.02.2016 13:53, 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@suse.com> >>> --- >>> 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). > If we don't it's a bug and not an excuse to continue doing bad things > Juergen > > . >
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; #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; + err = set_mfns (console_pfn); if (err) return err; - generate_page_table (get_virtual_current_address (ch), - max_addr >> PAGE_SHIFT, nr_pages, + nr_pages = max_addr >> PAGE_SHIFT; + + grub_dprintf ("xen", "bootstrap domain %llx+%llx\n", + (unsigned long long) xen_inf.virt_base, + (unsigned long long) page2offset (nr_pages)); + + generate_page_table (virt_pgtable, pgtbl_start, pgtbl_end, nr_pages, xen_inf.virt_base, virt_mfn_list); - max_addr += page2offset (nr_pt_pages); - state.stack = max_addr + STACK_SIZE + xen_inf.virt_base; state.entry_point = xen_inf.entry_point; - next_start.nr_pt_frames = nr_pt_pages; - state.paging_size = nr_pt_pages; - *virt_start_info = next_start; grub_memset (&gnttab_setver, 0, sizeof (gnttab_setver)); @@ -335,8 +355,8 @@ grub_xen_boot (void) return grub_relocator_xen_boot (relocator, state, nr_pages, xen_inf.virt_base < PAGE_SIZE ? page2offset (nr_pages) : 0, - nr_pages - 1, - page2offset (nr_pages - 1) + + pgtbl_end - 1, + page2offset (pgtbl_end - 1) + xen_inf.virt_base); }
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@suse.com> --- grub-core/loader/i386/xen.c | 82 ++++++++++++++++++++++++++++----------------- 1 file changed, 51 insertions(+), 31 deletions(-)