| Message ID | 1455177206-8974-2-git-send-email-jgross@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Feb 11, 2016 at 08:53:21AM +0100, Juergen Gross wrote: > Do the p2m list allocation of the to be loaded kernel in a separate > function. This will allow doing the p2m list allocation at different > times of the boot preparations depending on the features the kernel > is supporting. > > While at this remove superfluous setting of first_p2m_pfn and > nr_p2m_frames as those are needed only in case of the p2m list not > being mapped by the initial kernel mapping. > > Signed-off-by: Juergen Gross <jgross@suse.com> > --- > grub-core/loader/i386/xen.c | 70 ++++++++++++++++++++++++++------------------- > 1 file changed, 40 insertions(+), 30 deletions(-) > > diff --git a/grub-core/loader/i386/xen.c b/grub-core/loader/i386/xen.c > index c4d9689..42ed7c7 100644 > --- a/grub-core/loader/i386/xen.c > +++ b/grub-core/loader/i386/xen.c > @@ -52,6 +52,8 @@ static struct grub_xen_file_info xen_inf; > static struct xen_multiboot_mod_list *xen_module_info_page; > static grub_uint64_t modules_target_start; > static grub_size_t n_modules; > +static struct grub_relocator_xen_state state; > +static grub_xen_mfn_t *virt_mfn_list; Do we strongly need this as globals? I suppose that both of them could be local to grub_xen_boot. > #define PAGE_SIZE 4096 > #define MAX_MODULES (PAGE_SIZE / sizeof (struct xen_multiboot_mod_list)) > @@ -166,7 +168,7 @@ generate_page_table (grub_uint64_t *where, grub_uint64_t paging_start, > } > > static grub_err_t > -set_mfns (grub_xen_mfn_t * new_mfn_list, grub_xen_mfn_t pfn) > +set_mfns (grub_xen_mfn_t pfn) > { > grub_xen_mfn_t i, t; > grub_xen_mfn_t cn_pfn = -1, st_pfn = -1; > @@ -175,32 +177,32 @@ set_mfns (grub_xen_mfn_t * new_mfn_list, grub_xen_mfn_t pfn) > > for (i = 0; i < grub_xen_start_page_addr->nr_pages; i++) > { > - if (new_mfn_list[i] == grub_xen_start_page_addr->console.domU.mfn) > + if (virt_mfn_list[i] == grub_xen_start_page_addr->console.domU.mfn) > cn_pfn = i; > - if (new_mfn_list[i] == grub_xen_start_page_addr->store_mfn) > + if (virt_mfn_list[i] == grub_xen_start_page_addr->store_mfn) > st_pfn = i; > } > if (cn_pfn == (grub_xen_mfn_t)-1) > return grub_error (GRUB_ERR_BUG, "no console"); > if (st_pfn == (grub_xen_mfn_t)-1) > return grub_error (GRUB_ERR_BUG, "no store"); > - t = new_mfn_list[pfn]; > - new_mfn_list[pfn] = new_mfn_list[cn_pfn]; > - new_mfn_list[cn_pfn] = t; > - t = new_mfn_list[pfn + 1]; > - new_mfn_list[pfn + 1] = new_mfn_list[st_pfn]; > - new_mfn_list[st_pfn] = t; > - > - m2p_updates[0].ptr = page2offset (new_mfn_list[pfn]) | MMU_MACHPHYS_UPDATE; > + t = virt_mfn_list[pfn]; > + virt_mfn_list[pfn] = virt_mfn_list[cn_pfn]; > + virt_mfn_list[cn_pfn] = t; > + t = virt_mfn_list[pfn + 1]; > + virt_mfn_list[pfn + 1] = virt_mfn_list[st_pfn]; > + virt_mfn_list[st_pfn] = t; > + > + m2p_updates[0].ptr = page2offset (virt_mfn_list[pfn]) | MMU_MACHPHYS_UPDATE; > m2p_updates[0].val = pfn; > m2p_updates[1].ptr = > - page2offset (new_mfn_list[pfn + 1]) | MMU_MACHPHYS_UPDATE; > + page2offset (virt_mfn_list[pfn + 1]) | MMU_MACHPHYS_UPDATE; > m2p_updates[1].val = pfn + 1; > m2p_updates[2].ptr = > - page2offset (new_mfn_list[cn_pfn]) | MMU_MACHPHYS_UPDATE; > + page2offset (virt_mfn_list[cn_pfn]) | MMU_MACHPHYS_UPDATE; > m2p_updates[2].val = cn_pfn; > m2p_updates[3].ptr = > - page2offset (new_mfn_list[st_pfn]) | MMU_MACHPHYS_UPDATE; > + page2offset (virt_mfn_list[st_pfn]) | MMU_MACHPHYS_UPDATE; > m2p_updates[3].val = st_pfn; > > grub_xen_mmu_update (m2p_updates, 4, NULL, DOMID_SELF); > @@ -209,34 +211,43 @@ set_mfns (grub_xen_mfn_t * new_mfn_list, grub_xen_mfn_t pfn) > } > > static grub_err_t > +grub_xen_p2m_alloc (void) > +{ > + grub_relocator_chunk_t ch; > + grub_size_t p2msize; > + grub_err_t err; > + > + state.mfn_list = max_addr; > + next_start.mfn_list = max_addr + xen_inf.virt_base; > + p2msize = sizeof (grub_xen_mfn_t) * grub_xen_start_page_addr->nr_pages; > + err = grub_relocator_alloc_chunk_addr (relocator, &ch, max_addr, p2msize); Hmmm.... Where relocator is defined? > + if (err) > + return err; > + virt_mfn_list = get_virtual_current_address (ch); > + grub_memcpy (virt_mfn_list, > + (void *) grub_xen_start_page_addr->mfn_list, p2msize); > + max_addr = ALIGN_UP (max_addr + p2msize, PAGE_SIZE); > + > + return GRUB_ERR_NONE; > +} > + > +static grub_err_t > grub_xen_boot (void) > { > - struct grub_relocator_xen_state state; > grub_relocator_chunk_t ch; > grub_err_t err; > - grub_size_t pgtsize; > struct start_info *nst; > grub_uint64_t nr_info_pages; > grub_uint64_t nr_pages, nr_pt_pages, nr_need_pages; > struct gnttab_set_version gnttab_setver; > - grub_xen_mfn_t *new_mfn_list; > grub_size_t i; > > if (grub_xen_n_allocated_shared_pages) > return grub_error (GRUB_ERR_BUG, "active grants"); > > - state.mfn_list = max_addr; > - next_start.mfn_list = max_addr + xen_inf.virt_base; > - next_start.first_p2m_pfn = max_addr >> PAGE_SHIFT; /* Is this right? */ > - pgtsize = sizeof (grub_xen_mfn_t) * grub_xen_start_page_addr->nr_pages; > - err = grub_relocator_alloc_chunk_addr (relocator, &ch, max_addr, pgtsize); > - next_start.nr_p2m_frames = (pgtsize + PAGE_SIZE - 1) >> PAGE_SHIFT; > + err = grub_xen_p2m_alloc (); > if (err) > return err; > - new_mfn_list = get_virtual_current_address (ch); > - grub_memcpy (new_mfn_list, > - (void *) grub_xen_start_page_addr->mfn_list, pgtsize); > - max_addr = ALIGN_UP (max_addr + pgtsize, PAGE_SIZE); > > err = grub_relocator_alloc_chunk_addr (relocator, &ch, Ditto. I think that this could be passed as a grub_xen_boot() argument. Otherwise this appear from nowhere and reading/understanding is more difficult. Daniel
On 11/02/16 13:19, Daniel Kiper wrote: > On Thu, Feb 11, 2016 at 08:53:21AM +0100, Juergen Gross wrote: >> Do the p2m list allocation of the to be loaded kernel in a separate >> function. This will allow doing the p2m list allocation at different >> times of the boot preparations depending on the features the kernel >> is supporting. >> >> While at this remove superfluous setting of first_p2m_pfn and >> nr_p2m_frames as those are needed only in case of the p2m list not >> being mapped by the initial kernel mapping. >> >> Signed-off-by: Juergen Gross <jgross@suse.com> >> --- >> grub-core/loader/i386/xen.c | 70 ++++++++++++++++++++++++++------------------- >> 1 file changed, 40 insertions(+), 30 deletions(-) >> >> diff --git a/grub-core/loader/i386/xen.c b/grub-core/loader/i386/xen.c >> index c4d9689..42ed7c7 100644 >> --- a/grub-core/loader/i386/xen.c >> +++ b/grub-core/loader/i386/xen.c >> @@ -52,6 +52,8 @@ static struct grub_xen_file_info xen_inf; >> static struct xen_multiboot_mod_list *xen_module_info_page; >> static grub_uint64_t modules_target_start; >> static grub_size_t n_modules; >> +static struct grub_relocator_xen_state state; >> +static grub_xen_mfn_t *virt_mfn_list; > > Do we strongly need this as globals? I suppose that > both of them could be local to grub_xen_boot. This would require passing the state pointer to many other functions. Same applies to virt_mfn_list. I just followed the style used in the source already: variables used in multiple functions are mostly defined globally (there are even some which are used in one function only). > >> #define PAGE_SIZE 4096 >> #define MAX_MODULES (PAGE_SIZE / sizeof (struct xen_multiboot_mod_list)) >> @@ -166,7 +168,7 @@ generate_page_table (grub_uint64_t *where, grub_uint64_t paging_start, >> } >> >> static grub_err_t >> -set_mfns (grub_xen_mfn_t * new_mfn_list, grub_xen_mfn_t pfn) >> +set_mfns (grub_xen_mfn_t pfn) >> { >> grub_xen_mfn_t i, t; >> grub_xen_mfn_t cn_pfn = -1, st_pfn = -1; >> @@ -175,32 +177,32 @@ set_mfns (grub_xen_mfn_t * new_mfn_list, grub_xen_mfn_t pfn) >> >> for (i = 0; i < grub_xen_start_page_addr->nr_pages; i++) >> { >> - if (new_mfn_list[i] == grub_xen_start_page_addr->console.domU.mfn) >> + if (virt_mfn_list[i] == grub_xen_start_page_addr->console.domU.mfn) >> cn_pfn = i; >> - if (new_mfn_list[i] == grub_xen_start_page_addr->store_mfn) >> + if (virt_mfn_list[i] == grub_xen_start_page_addr->store_mfn) >> st_pfn = i; >> } >> if (cn_pfn == (grub_xen_mfn_t)-1) >> return grub_error (GRUB_ERR_BUG, "no console"); >> if (st_pfn == (grub_xen_mfn_t)-1) >> return grub_error (GRUB_ERR_BUG, "no store"); >> - t = new_mfn_list[pfn]; >> - new_mfn_list[pfn] = new_mfn_list[cn_pfn]; >> - new_mfn_list[cn_pfn] = t; >> - t = new_mfn_list[pfn + 1]; >> - new_mfn_list[pfn + 1] = new_mfn_list[st_pfn]; >> - new_mfn_list[st_pfn] = t; >> - >> - m2p_updates[0].ptr = page2offset (new_mfn_list[pfn]) | MMU_MACHPHYS_UPDATE; >> + t = virt_mfn_list[pfn]; >> + virt_mfn_list[pfn] = virt_mfn_list[cn_pfn]; >> + virt_mfn_list[cn_pfn] = t; >> + t = virt_mfn_list[pfn + 1]; >> + virt_mfn_list[pfn + 1] = virt_mfn_list[st_pfn]; >> + virt_mfn_list[st_pfn] = t; >> + >> + m2p_updates[0].ptr = page2offset (virt_mfn_list[pfn]) | MMU_MACHPHYS_UPDATE; >> m2p_updates[0].val = pfn; >> m2p_updates[1].ptr = >> - page2offset (new_mfn_list[pfn + 1]) | MMU_MACHPHYS_UPDATE; >> + page2offset (virt_mfn_list[pfn + 1]) | MMU_MACHPHYS_UPDATE; >> m2p_updates[1].val = pfn + 1; >> m2p_updates[2].ptr = >> - page2offset (new_mfn_list[cn_pfn]) | MMU_MACHPHYS_UPDATE; >> + page2offset (virt_mfn_list[cn_pfn]) | MMU_MACHPHYS_UPDATE; >> m2p_updates[2].val = cn_pfn; >> m2p_updates[3].ptr = >> - page2offset (new_mfn_list[st_pfn]) | MMU_MACHPHYS_UPDATE; >> + page2offset (virt_mfn_list[st_pfn]) | MMU_MACHPHYS_UPDATE; >> m2p_updates[3].val = st_pfn; >> >> grub_xen_mmu_update (m2p_updates, 4, NULL, DOMID_SELF); >> @@ -209,34 +211,43 @@ set_mfns (grub_xen_mfn_t * new_mfn_list, grub_xen_mfn_t pfn) >> } >> >> static grub_err_t >> +grub_xen_p2m_alloc (void) >> +{ >> + grub_relocator_chunk_t ch; >> + grub_size_t p2msize; >> + grub_err_t err; >> + >> + state.mfn_list = max_addr; >> + next_start.mfn_list = max_addr + xen_inf.virt_base; >> + p2msize = sizeof (grub_xen_mfn_t) * grub_xen_start_page_addr->nr_pages; >> + err = grub_relocator_alloc_chunk_addr (relocator, &ch, max_addr, p2msize); > > Hmmm.... Where relocator is defined? First global variable in this source. > >> + if (err) >> + return err; >> + virt_mfn_list = get_virtual_current_address (ch); >> + grub_memcpy (virt_mfn_list, >> + (void *) grub_xen_start_page_addr->mfn_list, p2msize); >> + max_addr = ALIGN_UP (max_addr + p2msize, PAGE_SIZE); >> + >> + return GRUB_ERR_NONE; >> +} >> + >> +static grub_err_t >> grub_xen_boot (void) >> { >> - struct grub_relocator_xen_state state; >> grub_relocator_chunk_t ch; >> grub_err_t err; >> - grub_size_t pgtsize; >> struct start_info *nst; >> grub_uint64_t nr_info_pages; >> grub_uint64_t nr_pages, nr_pt_pages, nr_need_pages; >> struct gnttab_set_version gnttab_setver; >> - grub_xen_mfn_t *new_mfn_list; >> grub_size_t i; >> >> if (grub_xen_n_allocated_shared_pages) >> return grub_error (GRUB_ERR_BUG, "active grants"); >> >> - state.mfn_list = max_addr; >> - next_start.mfn_list = max_addr + xen_inf.virt_base; >> - next_start.first_p2m_pfn = max_addr >> PAGE_SHIFT; /* Is this right? */ >> - pgtsize = sizeof (grub_xen_mfn_t) * grub_xen_start_page_addr->nr_pages; >> - err = grub_relocator_alloc_chunk_addr (relocator, &ch, max_addr, pgtsize); >> - next_start.nr_p2m_frames = (pgtsize + PAGE_SIZE - 1) >> PAGE_SHIFT; >> + err = grub_xen_p2m_alloc (); >> if (err) >> return err; >> - new_mfn_list = get_virtual_current_address (ch); >> - grub_memcpy (new_mfn_list, >> - (void *) grub_xen_start_page_addr->mfn_list, pgtsize); >> - max_addr = ALIGN_UP (max_addr + pgtsize, PAGE_SIZE); >> >> err = grub_relocator_alloc_chunk_addr (relocator, &ch, > > Ditto. I think that this could be passed as a grub_xen_boot() argument. > Otherwise this appear from nowhere and reading/understanding is more > difficult. Huh? How that? grub_xen_boot() can't have any parameters (see prototype of grub_loader_set() which is used with grub_xen_boot() as parameter). BTW: I didn't introduce this kind of usage of relocator. Juergen
On Thu, Feb 11, 2016 at 01:38:10PM +0100, Juergen Gross wrote: > On 11/02/16 13:19, Daniel Kiper wrote: > > On Thu, Feb 11, 2016 at 08:53:21AM +0100, Juergen Gross wrote: > >> Do the p2m list allocation of the to be loaded kernel in a separate > >> function. This will allow doing the p2m list allocation at different > >> times of the boot preparations depending on the features the kernel > >> is supporting. > >> > >> While at this remove superfluous setting of first_p2m_pfn and > >> nr_p2m_frames as those are needed only in case of the p2m list not > >> being mapped by the initial kernel mapping. > >> > >> Signed-off-by: Juergen Gross <jgross@suse.com> > >> --- > >> grub-core/loader/i386/xen.c | 70 ++++++++++++++++++++++++++------------------- > >> 1 file changed, 40 insertions(+), 30 deletions(-) > >> > >> diff --git a/grub-core/loader/i386/xen.c b/grub-core/loader/i386/xen.c > >> index c4d9689..42ed7c7 100644 > >> --- a/grub-core/loader/i386/xen.c > >> +++ b/grub-core/loader/i386/xen.c > >> @@ -52,6 +52,8 @@ static struct grub_xen_file_info xen_inf; > >> static struct xen_multiboot_mod_list *xen_module_info_page; > >> static grub_uint64_t modules_target_start; > >> static grub_size_t n_modules; > >> +static struct grub_relocator_xen_state state; > >> +static grub_xen_mfn_t *virt_mfn_list; > > > > Do we strongly need this as globals? I suppose that > > both of them could be local to grub_xen_boot. > > This would require passing the state pointer to many other functions. > Same applies to virt_mfn_list. > > I just followed the style used in the source already: variables used in > multiple functions are mostly defined globally (there are even some > which are used in one function only). Well, I do not like that style but if maintainer do not object I will do not complain more here about that. > >> #define PAGE_SIZE 4096 > >> #define MAX_MODULES (PAGE_SIZE / sizeof (struct xen_multiboot_mod_list)) > >> @@ -166,7 +168,7 @@ generate_page_table (grub_uint64_t *where, grub_uint64_t paging_start, > >> } > >> > >> static grub_err_t > >> -set_mfns (grub_xen_mfn_t * new_mfn_list, grub_xen_mfn_t pfn) > >> +set_mfns (grub_xen_mfn_t pfn) > >> { > >> grub_xen_mfn_t i, t; > >> grub_xen_mfn_t cn_pfn = -1, st_pfn = -1; > >> @@ -175,32 +177,32 @@ set_mfns (grub_xen_mfn_t * new_mfn_list, grub_xen_mfn_t pfn) > >> > >> for (i = 0; i < grub_xen_start_page_addr->nr_pages; i++) > >> { > >> - if (new_mfn_list[i] == grub_xen_start_page_addr->console.domU.mfn) > >> + if (virt_mfn_list[i] == grub_xen_start_page_addr->console.domU.mfn) > >> cn_pfn = i; > >> - if (new_mfn_list[i] == grub_xen_start_page_addr->store_mfn) > >> + if (virt_mfn_list[i] == grub_xen_start_page_addr->store_mfn) > >> st_pfn = i; > >> } > >> if (cn_pfn == (grub_xen_mfn_t)-1) > >> return grub_error (GRUB_ERR_BUG, "no console"); > >> if (st_pfn == (grub_xen_mfn_t)-1) > >> return grub_error (GRUB_ERR_BUG, "no store"); > >> - t = new_mfn_list[pfn]; > >> - new_mfn_list[pfn] = new_mfn_list[cn_pfn]; > >> - new_mfn_list[cn_pfn] = t; > >> - t = new_mfn_list[pfn + 1]; > >> - new_mfn_list[pfn + 1] = new_mfn_list[st_pfn]; > >> - new_mfn_list[st_pfn] = t; > >> - > >> - m2p_updates[0].ptr = page2offset (new_mfn_list[pfn]) | MMU_MACHPHYS_UPDATE; > >> + t = virt_mfn_list[pfn]; > >> + virt_mfn_list[pfn] = virt_mfn_list[cn_pfn]; > >> + virt_mfn_list[cn_pfn] = t; > >> + t = virt_mfn_list[pfn + 1]; > >> + virt_mfn_list[pfn + 1] = virt_mfn_list[st_pfn]; > >> + virt_mfn_list[st_pfn] = t; > >> + > >> + m2p_updates[0].ptr = page2offset (virt_mfn_list[pfn]) | MMU_MACHPHYS_UPDATE; > >> m2p_updates[0].val = pfn; > >> m2p_updates[1].ptr = > >> - page2offset (new_mfn_list[pfn + 1]) | MMU_MACHPHYS_UPDATE; > >> + page2offset (virt_mfn_list[pfn + 1]) | MMU_MACHPHYS_UPDATE; > >> m2p_updates[1].val = pfn + 1; > >> m2p_updates[2].ptr = > >> - page2offset (new_mfn_list[cn_pfn]) | MMU_MACHPHYS_UPDATE; > >> + page2offset (virt_mfn_list[cn_pfn]) | MMU_MACHPHYS_UPDATE; > >> m2p_updates[2].val = cn_pfn; > >> m2p_updates[3].ptr = > >> - page2offset (new_mfn_list[st_pfn]) | MMU_MACHPHYS_UPDATE; > >> + page2offset (virt_mfn_list[st_pfn]) | MMU_MACHPHYS_UPDATE; > >> m2p_updates[3].val = st_pfn; > >> > >> grub_xen_mmu_update (m2p_updates, 4, NULL, DOMID_SELF); > >> @@ -209,34 +211,43 @@ set_mfns (grub_xen_mfn_t * new_mfn_list, grub_xen_mfn_t pfn) > >> } > >> > >> static grub_err_t > >> +grub_xen_p2m_alloc (void) > >> +{ > >> + grub_relocator_chunk_t ch; > >> + grub_size_t p2msize; > >> + grub_err_t err; > >> + > >> + state.mfn_list = max_addr; > >> + next_start.mfn_list = max_addr + xen_inf.virt_base; > >> + p2msize = sizeof (grub_xen_mfn_t) * grub_xen_start_page_addr->nr_pages; > >> + err = grub_relocator_alloc_chunk_addr (relocator, &ch, max_addr, p2msize); > > > > Hmmm.... Where relocator is defined? > > First global variable in this source. > > > > >> + if (err) > >> + return err; > >> + virt_mfn_list = get_virtual_current_address (ch); > >> + grub_memcpy (virt_mfn_list, > >> + (void *) grub_xen_start_page_addr->mfn_list, p2msize); > >> + max_addr = ALIGN_UP (max_addr + p2msize, PAGE_SIZE); > >> + > >> + return GRUB_ERR_NONE; > >> +} > >> + > >> +static grub_err_t > >> grub_xen_boot (void) > >> { > >> - struct grub_relocator_xen_state state; > >> grub_relocator_chunk_t ch; > >> grub_err_t err; > >> - grub_size_t pgtsize; > >> struct start_info *nst; > >> grub_uint64_t nr_info_pages; > >> grub_uint64_t nr_pages, nr_pt_pages, nr_need_pages; > >> struct gnttab_set_version gnttab_setver; > >> - grub_xen_mfn_t *new_mfn_list; > >> grub_size_t i; > >> > >> if (grub_xen_n_allocated_shared_pages) > >> return grub_error (GRUB_ERR_BUG, "active grants"); > >> > >> - state.mfn_list = max_addr; > >> - next_start.mfn_list = max_addr + xen_inf.virt_base; > >> - next_start.first_p2m_pfn = max_addr >> PAGE_SHIFT; /* Is this right? */ > >> - pgtsize = sizeof (grub_xen_mfn_t) * grub_xen_start_page_addr->nr_pages; > >> - err = grub_relocator_alloc_chunk_addr (relocator, &ch, max_addr, pgtsize); > >> - next_start.nr_p2m_frames = (pgtsize + PAGE_SIZE - 1) >> PAGE_SHIFT; > >> + err = grub_xen_p2m_alloc (); > >> if (err) > >> return err; > >> - new_mfn_list = get_virtual_current_address (ch); > >> - grub_memcpy (new_mfn_list, > >> - (void *) grub_xen_start_page_addr->mfn_list, pgtsize); > >> - max_addr = ALIGN_UP (max_addr + pgtsize, PAGE_SIZE); > >> > >> err = grub_relocator_alloc_chunk_addr (relocator, &ch, > > > > Ditto. I think that this could be passed as a grub_xen_boot() argument. > > Otherwise this appear from nowhere and reading/understanding is more > > difficult. > > Huh? How that? grub_xen_boot() can't have any parameters (see prototype > of grub_loader_set() which is used with grub_xen_boot() as parameter). > > BTW: I didn't introduce this kind of usage of relocator. OK, I have checked other loaders and it looks that they use relocator in the same way. Let's leave it as is. However, this does not waive my concerns expressed above in regards to other global variables. Daniel
On 11/02/16 18:09, Daniel Kiper wrote: > On Thu, Feb 11, 2016 at 01:38:10PM +0100, Juergen Gross wrote: >> On 11/02/16 13:19, Daniel Kiper wrote: >>> On Thu, Feb 11, 2016 at 08:53:21AM +0100, Juergen Gross wrote: >>>> Do the p2m list allocation of the to be loaded kernel in a separate >>>> function. This will allow doing the p2m list allocation at different >>>> times of the boot preparations depending on the features the kernel >>>> is supporting. >>>> >>>> While at this remove superfluous setting of first_p2m_pfn and >>>> nr_p2m_frames as those are needed only in case of the p2m list not >>>> being mapped by the initial kernel mapping. >>>> >>>> Signed-off-by: Juergen Gross <jgross@suse.com> >>>> --- >>>> grub-core/loader/i386/xen.c | 70 ++++++++++++++++++++++++++------------------- >>>> 1 file changed, 40 insertions(+), 30 deletions(-) >>>> >>>> diff --git a/grub-core/loader/i386/xen.c b/grub-core/loader/i386/xen.c >>>> index c4d9689..42ed7c7 100644 >>>> --- a/grub-core/loader/i386/xen.c >>>> +++ b/grub-core/loader/i386/xen.c >>>> @@ -52,6 +52,8 @@ static struct grub_xen_file_info xen_inf; >>>> static struct xen_multiboot_mod_list *xen_module_info_page; >>>> static grub_uint64_t modules_target_start; >>>> static grub_size_t n_modules; >>>> +static struct grub_relocator_xen_state state; >>>> +static grub_xen_mfn_t *virt_mfn_list; >>> >>> Do we strongly need this as globals? I suppose that >>> both of them could be local to grub_xen_boot. >> >> This would require passing the state pointer to many other functions. >> Same applies to virt_mfn_list. >> >> I just followed the style used in the source already: variables used in >> multiple functions are mostly defined globally (there are even some >> which are used in one function only). > > Well, I do not like that style but if maintainer do not object I will > do not complain more here about that. > >>>> #define PAGE_SIZE 4096 >>>> #define MAX_MODULES (PAGE_SIZE / sizeof (struct xen_multiboot_mod_list)) >>>> @@ -166,7 +168,7 @@ generate_page_table (grub_uint64_t *where, grub_uint64_t paging_start, >>>> } >>>> >>>> static grub_err_t >>>> -set_mfns (grub_xen_mfn_t * new_mfn_list, grub_xen_mfn_t pfn) >>>> +set_mfns (grub_xen_mfn_t pfn) >>>> { >>>> grub_xen_mfn_t i, t; >>>> grub_xen_mfn_t cn_pfn = -1, st_pfn = -1; >>>> @@ -175,32 +177,32 @@ set_mfns (grub_xen_mfn_t * new_mfn_list, grub_xen_mfn_t pfn) >>>> >>>> for (i = 0; i < grub_xen_start_page_addr->nr_pages; i++) >>>> { >>>> - if (new_mfn_list[i] == grub_xen_start_page_addr->console.domU.mfn) >>>> + if (virt_mfn_list[i] == grub_xen_start_page_addr->console.domU.mfn) >>>> cn_pfn = i; >>>> - if (new_mfn_list[i] == grub_xen_start_page_addr->store_mfn) >>>> + if (virt_mfn_list[i] == grub_xen_start_page_addr->store_mfn) >>>> st_pfn = i; >>>> } >>>> if (cn_pfn == (grub_xen_mfn_t)-1) >>>> return grub_error (GRUB_ERR_BUG, "no console"); >>>> if (st_pfn == (grub_xen_mfn_t)-1) >>>> return grub_error (GRUB_ERR_BUG, "no store"); >>>> - t = new_mfn_list[pfn]; >>>> - new_mfn_list[pfn] = new_mfn_list[cn_pfn]; >>>> - new_mfn_list[cn_pfn] = t; >>>> - t = new_mfn_list[pfn + 1]; >>>> - new_mfn_list[pfn + 1] = new_mfn_list[st_pfn]; >>>> - new_mfn_list[st_pfn] = t; >>>> - >>>> - m2p_updates[0].ptr = page2offset (new_mfn_list[pfn]) | MMU_MACHPHYS_UPDATE; >>>> + t = virt_mfn_list[pfn]; >>>> + virt_mfn_list[pfn] = virt_mfn_list[cn_pfn]; >>>> + virt_mfn_list[cn_pfn] = t; >>>> + t = virt_mfn_list[pfn + 1]; >>>> + virt_mfn_list[pfn + 1] = virt_mfn_list[st_pfn]; >>>> + virt_mfn_list[st_pfn] = t; >>>> + >>>> + m2p_updates[0].ptr = page2offset (virt_mfn_list[pfn]) | MMU_MACHPHYS_UPDATE; >>>> m2p_updates[0].val = pfn; >>>> m2p_updates[1].ptr = >>>> - page2offset (new_mfn_list[pfn + 1]) | MMU_MACHPHYS_UPDATE; >>>> + page2offset (virt_mfn_list[pfn + 1]) | MMU_MACHPHYS_UPDATE; >>>> m2p_updates[1].val = pfn + 1; >>>> m2p_updates[2].ptr = >>>> - page2offset (new_mfn_list[cn_pfn]) | MMU_MACHPHYS_UPDATE; >>>> + page2offset (virt_mfn_list[cn_pfn]) | MMU_MACHPHYS_UPDATE; >>>> m2p_updates[2].val = cn_pfn; >>>> m2p_updates[3].ptr = >>>> - page2offset (new_mfn_list[st_pfn]) | MMU_MACHPHYS_UPDATE; >>>> + page2offset (virt_mfn_list[st_pfn]) | MMU_MACHPHYS_UPDATE; >>>> m2p_updates[3].val = st_pfn; >>>> >>>> grub_xen_mmu_update (m2p_updates, 4, NULL, DOMID_SELF); >>>> @@ -209,34 +211,43 @@ set_mfns (grub_xen_mfn_t * new_mfn_list, grub_xen_mfn_t pfn) >>>> } >>>> >>>> static grub_err_t >>>> +grub_xen_p2m_alloc (void) >>>> +{ >>>> + grub_relocator_chunk_t ch; >>>> + grub_size_t p2msize; >>>> + grub_err_t err; >>>> + >>>> + state.mfn_list = max_addr; >>>> + next_start.mfn_list = max_addr + xen_inf.virt_base; >>>> + p2msize = sizeof (grub_xen_mfn_t) * grub_xen_start_page_addr->nr_pages; >>>> + err = grub_relocator_alloc_chunk_addr (relocator, &ch, max_addr, p2msize); >>> >>> Hmmm.... Where relocator is defined? >> >> First global variable in this source. >> >>> >>>> + if (err) >>>> + return err; >>>> + virt_mfn_list = get_virtual_current_address (ch); >>>> + grub_memcpy (virt_mfn_list, >>>> + (void *) grub_xen_start_page_addr->mfn_list, p2msize); >>>> + max_addr = ALIGN_UP (max_addr + p2msize, PAGE_SIZE); >>>> + >>>> + return GRUB_ERR_NONE; >>>> +} >>>> + >>>> +static grub_err_t >>>> grub_xen_boot (void) >>>> { >>>> - struct grub_relocator_xen_state state; >>>> grub_relocator_chunk_t ch; >>>> grub_err_t err; >>>> - grub_size_t pgtsize; >>>> struct start_info *nst; >>>> grub_uint64_t nr_info_pages; >>>> grub_uint64_t nr_pages, nr_pt_pages, nr_need_pages; >>>> struct gnttab_set_version gnttab_setver; >>>> - grub_xen_mfn_t *new_mfn_list; >>>> grub_size_t i; >>>> >>>> if (grub_xen_n_allocated_shared_pages) >>>> return grub_error (GRUB_ERR_BUG, "active grants"); >>>> >>>> - state.mfn_list = max_addr; >>>> - next_start.mfn_list = max_addr + xen_inf.virt_base; >>>> - next_start.first_p2m_pfn = max_addr >> PAGE_SHIFT; /* Is this right? */ >>>> - pgtsize = sizeof (grub_xen_mfn_t) * grub_xen_start_page_addr->nr_pages; >>>> - err = grub_relocator_alloc_chunk_addr (relocator, &ch, max_addr, pgtsize); >>>> - next_start.nr_p2m_frames = (pgtsize + PAGE_SIZE - 1) >> PAGE_SHIFT; >>>> + err = grub_xen_p2m_alloc (); >>>> if (err) >>>> return err; >>>> - new_mfn_list = get_virtual_current_address (ch); >>>> - grub_memcpy (new_mfn_list, >>>> - (void *) grub_xen_start_page_addr->mfn_list, pgtsize); >>>> - max_addr = ALIGN_UP (max_addr + pgtsize, PAGE_SIZE); >>>> >>>> err = grub_relocator_alloc_chunk_addr (relocator, &ch, >>> >>> Ditto. I think that this could be passed as a grub_xen_boot() argument. >>> Otherwise this appear from nowhere and reading/understanding is more >>> difficult. >> >> Huh? How that? grub_xen_boot() can't have any parameters (see prototype >> of grub_loader_set() which is used with grub_xen_boot() as parameter). >> >> BTW: I didn't introduce this kind of usage of relocator. > > OK, I have checked other loaders and it looks that they use relocator in > the same way. Let's leave it as is. However, this does not waive my > concerns expressed above in regards to other global variables. See my reply to patch 4. I'll change my series. Juergen
diff --git a/grub-core/loader/i386/xen.c b/grub-core/loader/i386/xen.c index c4d9689..42ed7c7 100644 --- a/grub-core/loader/i386/xen.c +++ b/grub-core/loader/i386/xen.c @@ -52,6 +52,8 @@ static struct grub_xen_file_info xen_inf; static struct xen_multiboot_mod_list *xen_module_info_page; static grub_uint64_t modules_target_start; static grub_size_t n_modules; +static struct grub_relocator_xen_state state; +static grub_xen_mfn_t *virt_mfn_list; #define PAGE_SIZE 4096 #define MAX_MODULES (PAGE_SIZE / sizeof (struct xen_multiboot_mod_list)) @@ -166,7 +168,7 @@ generate_page_table (grub_uint64_t *where, grub_uint64_t paging_start, } static grub_err_t -set_mfns (grub_xen_mfn_t * new_mfn_list, grub_xen_mfn_t pfn) +set_mfns (grub_xen_mfn_t pfn) { grub_xen_mfn_t i, t; grub_xen_mfn_t cn_pfn = -1, st_pfn = -1; @@ -175,32 +177,32 @@ set_mfns (grub_xen_mfn_t * new_mfn_list, grub_xen_mfn_t pfn) for (i = 0; i < grub_xen_start_page_addr->nr_pages; i++) { - if (new_mfn_list[i] == grub_xen_start_page_addr->console.domU.mfn) + if (virt_mfn_list[i] == grub_xen_start_page_addr->console.domU.mfn) cn_pfn = i; - if (new_mfn_list[i] == grub_xen_start_page_addr->store_mfn) + if (virt_mfn_list[i] == grub_xen_start_page_addr->store_mfn) st_pfn = i; } if (cn_pfn == (grub_xen_mfn_t)-1) return grub_error (GRUB_ERR_BUG, "no console"); if (st_pfn == (grub_xen_mfn_t)-1) return grub_error (GRUB_ERR_BUG, "no store"); - t = new_mfn_list[pfn]; - new_mfn_list[pfn] = new_mfn_list[cn_pfn]; - new_mfn_list[cn_pfn] = t; - t = new_mfn_list[pfn + 1]; - new_mfn_list[pfn + 1] = new_mfn_list[st_pfn]; - new_mfn_list[st_pfn] = t; - - m2p_updates[0].ptr = page2offset (new_mfn_list[pfn]) | MMU_MACHPHYS_UPDATE; + t = virt_mfn_list[pfn]; + virt_mfn_list[pfn] = virt_mfn_list[cn_pfn]; + virt_mfn_list[cn_pfn] = t; + t = virt_mfn_list[pfn + 1]; + virt_mfn_list[pfn + 1] = virt_mfn_list[st_pfn]; + virt_mfn_list[st_pfn] = t; + + m2p_updates[0].ptr = page2offset (virt_mfn_list[pfn]) | MMU_MACHPHYS_UPDATE; m2p_updates[0].val = pfn; m2p_updates[1].ptr = - page2offset (new_mfn_list[pfn + 1]) | MMU_MACHPHYS_UPDATE; + page2offset (virt_mfn_list[pfn + 1]) | MMU_MACHPHYS_UPDATE; m2p_updates[1].val = pfn + 1; m2p_updates[2].ptr = - page2offset (new_mfn_list[cn_pfn]) | MMU_MACHPHYS_UPDATE; + page2offset (virt_mfn_list[cn_pfn]) | MMU_MACHPHYS_UPDATE; m2p_updates[2].val = cn_pfn; m2p_updates[3].ptr = - page2offset (new_mfn_list[st_pfn]) | MMU_MACHPHYS_UPDATE; + page2offset (virt_mfn_list[st_pfn]) | MMU_MACHPHYS_UPDATE; m2p_updates[3].val = st_pfn; grub_xen_mmu_update (m2p_updates, 4, NULL, DOMID_SELF); @@ -209,34 +211,43 @@ set_mfns (grub_xen_mfn_t * new_mfn_list, grub_xen_mfn_t pfn) } static grub_err_t +grub_xen_p2m_alloc (void) +{ + grub_relocator_chunk_t ch; + grub_size_t p2msize; + grub_err_t err; + + state.mfn_list = max_addr; + next_start.mfn_list = max_addr + xen_inf.virt_base; + p2msize = sizeof (grub_xen_mfn_t) * grub_xen_start_page_addr->nr_pages; + err = grub_relocator_alloc_chunk_addr (relocator, &ch, max_addr, p2msize); + if (err) + return err; + virt_mfn_list = get_virtual_current_address (ch); + grub_memcpy (virt_mfn_list, + (void *) grub_xen_start_page_addr->mfn_list, p2msize); + max_addr = ALIGN_UP (max_addr + p2msize, PAGE_SIZE); + + return GRUB_ERR_NONE; +} + +static grub_err_t grub_xen_boot (void) { - struct grub_relocator_xen_state state; grub_relocator_chunk_t ch; grub_err_t err; - grub_size_t pgtsize; struct start_info *nst; grub_uint64_t nr_info_pages; grub_uint64_t nr_pages, nr_pt_pages, nr_need_pages; struct gnttab_set_version gnttab_setver; - grub_xen_mfn_t *new_mfn_list; grub_size_t i; if (grub_xen_n_allocated_shared_pages) return grub_error (GRUB_ERR_BUG, "active grants"); - state.mfn_list = max_addr; - next_start.mfn_list = max_addr + xen_inf.virt_base; - next_start.first_p2m_pfn = max_addr >> PAGE_SHIFT; /* Is this right? */ - pgtsize = sizeof (grub_xen_mfn_t) * grub_xen_start_page_addr->nr_pages; - err = grub_relocator_alloc_chunk_addr (relocator, &ch, max_addr, pgtsize); - next_start.nr_p2m_frames = (pgtsize + PAGE_SIZE - 1) >> PAGE_SHIFT; + err = grub_xen_p2m_alloc (); if (err) return err; - new_mfn_list = get_virtual_current_address (ch); - grub_memcpy (new_mfn_list, - (void *) grub_xen_start_page_addr->mfn_list, pgtsize); - max_addr = ALIGN_UP (max_addr + pgtsize, PAGE_SIZE); err = grub_relocator_alloc_chunk_addr (relocator, &ch, max_addr, sizeof (next_start)); @@ -254,7 +265,7 @@ grub_xen_boot (void) next_start.console.domU = grub_xen_start_page_addr->console.domU; next_start.shared_info = grub_xen_start_page_addr->shared_info; - err = set_mfns (new_mfn_list, max_addr >> PAGE_SHIFT); + err = set_mfns (max_addr >> PAGE_SHIFT); if (err) return err; max_addr += 2 * PAGE_SIZE; @@ -288,13 +299,12 @@ grub_xen_boot (void) generate_page_table (get_virtual_current_address (ch), max_addr >> PAGE_SHIFT, nr_pages, - xen_inf.virt_base, new_mfn_list); + 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_p2m_frames += nr_pt_pages; next_start.nr_pt_frames = nr_pt_pages; state.paging_size = nr_pt_pages;
Do the p2m list allocation of the to be loaded kernel in a separate function. This will allow doing the p2m list allocation at different times of the boot preparations depending on the features the kernel is supporting. While at this remove superfluous setting of first_p2m_pfn and nr_p2m_frames as those are needed only in case of the p2m list not being mapped by the initial kernel mapping. Signed-off-by: Juergen Gross <jgross@suse.com> --- grub-core/loader/i386/xen.c | 70 ++++++++++++++++++++++++++------------------- 1 file changed, 40 insertions(+), 30 deletions(-)