Re: [Xen-devel] [PATCH v3 02/10] xen: reduce number of global variables in xen loader
On Fri, Feb 19, 2016 at 05:59:27AM +0100, Juergen Gross wrote:
> On 18/02/16 18:00, Lennart Sorensen wrote:
> > On Thu, Feb 18, 2016 at 11:34:49AM +0100, Juergen Gross wrote:
> >> On 18/02/16 11:22, Daniel Kiper wrote:
> >>> On Wed, Feb 17, 2016 at 06:19:29PM +0100, Juergen Gross wrote:
> >>>> The loader for xen paravirtualized environment is using lots of global
> >>>> variables. Reduce the number by making them either local or by putting
> >>>> them into a single state structure.
> >>>>
> >>>> Signed-off-by: Juergen Gross
> >>>
> >>> Just two nitpicks but in general...
> >>>
> >>> Reviewed-by: Daniel Kiper
> >>>
> >>>> ---
> >>>> grub-core/loader/i386/xen.c | 259
> >>>> +++-
> >>>> 1 file changed, 138 insertions(+), 121 deletions(-)
> >>>>
> >>>> diff --git a/grub-core/loader/i386/xen.c b/grub-core/loader/i386/xen.c
> >>>> index ff7c553..d5fe168 100644
> >>>> --- a/grub-core/loader/i386/xen.c
> >>>> +++ b/grub-core/loader/i386/xen.c
> >>>> @@ -42,16 +42,20 @@
> >>>>
> >>>> GRUB_MOD_LICENSE ("GPLv3+");
> >>>>
> >>>> -static struct grub_relocator *relocator = NULL;
> >>>> -static grub_uint64_t max_addr;
> >>>> +struct xen_loader_state {
> >>>> + struct grub_relocator *relocator;
> >>>> + struct start_info next_start;
> >>>> + struct grub_xen_file_info xen_inf;
> >>>> + grub_uint64_t max_addr;
> >>>> + struct xen_multiboot_mod_list *module_info_page;
> >>>> + grub_uint64_t modules_target_start;
> >>>> + grub_size_t n_modules;
> >>>> + int loaded;
> >>>> +};
> >>>> +
> >>>> +static struct xen_loader_state xen_state;
> >>>> +
> >>>> static grub_dl_t my_mod;
> >>>> -static int loaded = 0;
> >>>> -static struct start_info next_start;
> >>>> -static void *kern_chunk_src;
> >>>> -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;
> >>>>
> >>>> #define PAGE_SIZE 4096
> >>>> #define MAX_MODULES (PAGE_SIZE / sizeof (struct xen_multiboot_mod_list))
> >>>> @@ -225,50 +229,55 @@ grub_xen_boot (void)
> >>>>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? */
> >>>> + state.mfn_list = xen_state.max_addr;
> >>>> + xen_state.next_start.mfn_list =
> >>>> +xen_state.max_addr + xen_state.xen_inf.virt_base;
> >>>> + xen_state.next_start.first_p2m_pfn = xen_state.max_addr >> PAGE_SHIFT;
> >>>>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_relocator_alloc_chunk_addr (xen_state.relocator, &ch,
> >>>> + xen_state.max_addr, pgtsize);
> >>>> + xen_state.next_start.nr_p2m_frames = (pgtsize + PAGE_SIZE - 1) >>
> >>>> PAGE_SHIFT;
> >>>>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);
> >>>> + xen_state.max_addr = ALIGN_UP (xen_state.max_addr + pgtsize,
> >>>> PAGE_SIZE);
> >>>>
> >>>> - err = grub_relocator_alloc_chunk_addr (relocator, &ch,
> >>>> - max_addr, sizeof (n
Re: [Xen-devel] [PATCH v3 02/10] xen: reduce number of global variables in xen loader
On Thu, Feb 18, 2016 at 11:34:49AM +0100, Juergen Gross wrote:
> On 18/02/16 11:22, Daniel Kiper wrote:
> > On Wed, Feb 17, 2016 at 06:19:29PM +0100, Juergen Gross wrote:
> >> The loader for xen paravirtualized environment is using lots of global
> >> variables. Reduce the number by making them either local or by putting
> >> them into a single state structure.
> >>
> >> Signed-off-by: Juergen Gross
> >
> > Just two nitpicks but in general...
> >
> > Reviewed-by: Daniel Kiper
> >
> >> ---
> >> grub-core/loader/i386/xen.c | 259
> >> +++-
> >> 1 file changed, 138 insertions(+), 121 deletions(-)
> >>
> >> diff --git a/grub-core/loader/i386/xen.c b/grub-core/loader/i386/xen.c
> >> index ff7c553..d5fe168 100644
> >> --- a/grub-core/loader/i386/xen.c
> >> +++ b/grub-core/loader/i386/xen.c
> >> @@ -42,16 +42,20 @@
> >>
> >> GRUB_MOD_LICENSE ("GPLv3+");
> >>
> >> -static struct grub_relocator *relocator = NULL;
> >> -static grub_uint64_t max_addr;
> >> +struct xen_loader_state {
> >> + struct grub_relocator *relocator;
> >> + struct start_info next_start;
> >> + struct grub_xen_file_info xen_inf;
> >> + grub_uint64_t max_addr;
> >> + struct xen_multiboot_mod_list *module_info_page;
> >> + grub_uint64_t modules_target_start;
> >> + grub_size_t n_modules;
> >> + int loaded;
> >> +};
> >> +
> >> +static struct xen_loader_state xen_state;
> >> +
> >> static grub_dl_t my_mod;
> >> -static int loaded = 0;
> >> -static struct start_info next_start;
> >> -static void *kern_chunk_src;
> >> -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;
> >>
> >> #define PAGE_SIZE 4096
> >> #define MAX_MODULES (PAGE_SIZE / sizeof (struct xen_multiboot_mod_list))
> >> @@ -225,50 +229,55 @@ grub_xen_boot (void)
> >>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? */
> >> + state.mfn_list = xen_state.max_addr;
> >> + xen_state.next_start.mfn_list =
> >> +xen_state.max_addr + xen_state.xen_inf.virt_base;
> >> + xen_state.next_start.first_p2m_pfn = xen_state.max_addr >> PAGE_SHIFT;
> >>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_relocator_alloc_chunk_addr (xen_state.relocator, &ch,
> >> + xen_state.max_addr, pgtsize);
> >> + xen_state.next_start.nr_p2m_frames = (pgtsize + PAGE_SIZE - 1) >>
> >> PAGE_SHIFT;
> >>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);
> >> + xen_state.max_addr = ALIGN_UP (xen_state.max_addr + pgtsize, PAGE_SIZE);
> >>
> >> - err = grub_relocator_alloc_chunk_addr (relocator, &ch,
> >> - max_addr, sizeof (next_start));
> >> + err = grub_relocator_alloc_chunk_addr (xen_state.relocator, &ch,
> >> + xen_state.max_addr,
> >> + sizeof (xen_state.next_start));
> >>if (err)
> >> return err;
> >> - state.start_info = max_addr + xen_inf.virt_base;
> >> + state.start_info = xen_state.max_addr + xen_state.xen_inf.virt_base;
> >>nst = get_virtual_current_address (ch);
> >> - max_addr = ALIGN_UP (max_addr + sizeof (next_start), PAGE_SIZE);
> >> + xen_state.max_addr =
> >> +ALIGN_UP (xen_state.max_addr + sizeof (xen_state.next_start),
> >> PAGE_SIZE);
> >>
> >> - next_start.nr_pages = grub_xen_start_page_addr->nr_pages;
> >> - grub_memcpy (next_start.magic, grub_xen_start_page_addr->magic,
> >> - sizeof (next_start.magic));
> >> - next_start.store_mfn = grub_xen_start_page_addr->store_mfn;
> >> - next_start.store_evtchn = grub_xen_start_page_addr->store_evtchn;
> >> - next_start.console.domU = grub_xen_start_page_addr->console.domU;
> >> - next_start.shared_info = grub_xen_start_page_addr->shared_info;
> >> + xen_state.next_start.nr_pages = grub_xen_start_page_addr->nr_pages;
> >> + grub_memcpy (xen_state.next_start.magic,
> >> grub_xen_start_page_addr->magic,
> >> + sizeof (xen_state.next_start.magic));
> >> + xen_state.next_start.store_mfn = grub_xen_start_page_addr->store_mfn;
> >> + xen_state.next_start.store_evtchn =
> >> grub_xen_start_page_addr->store_evtchn;
> >> + xen_state.next_start.console.domU =
> >> grub_xen_start_page_addr->console.domU;
> >> + xen_state.next_start.sh
Re: [Xen-devel] [PATCH 05/18] efi: split efi_enabled to efi_platform and efi_loader
On Fri, Mar 27, 2015 at 02:19:44PM +, Jan Beulich wrote: > >>> On 27.03.15 at 15:09, wrote: > > On Fri, Mar 27, 2015 at 02:04:22PM +, Jan Beulich wrote: > >> >>> On 27.03.15 at 14:53, wrote: > >> > On 27/03/15 13:43, Jan Beulich wrote: > >> > On 27.03.15 at 14:32, wrote: > >> >>> On Fri, Feb 20, 2015 at 04:17:35PM +, Jan Beulich wrote: > >> >>> On 30.01.15 at 18:54, wrote: > >> > We need more fine grained knowledge about EFI environment and check > >> > for EFI platform and EFI loader separately to properly support > >> > multiboot2 protocol. > >> ... because of ... (i.e. I can't see from the description what the > >> separation is good for). Looking at the comments you placed > >> aside the variables doesn't help me either. > >> > >> > In general Xen loaded by this protocol uses > >> > memory mappings and loaded modules in simliar way to Xen loaded > >> > by multiboot (v1) protocol. Hence, split efi_enabled to efi_platform > >> > and efi_loader. > >> And if I'm guessing things right, then introducing efi_loader but > >> leaving efi_enabled alone (only converting where needed) would > >> >>> efi_enabled is not fortunate name for new usage. Currently it means > >> >>> that Xen binary have or does not have EFI support build in. However, > >> >>> if we build in multiboot2 protocol into xen.gz then it means that > >> >>> it can ran on legacy BIOS or EFI platform. So, I think that we > >> >>> should rename efi_enabled to efi_platform because it will mean > >> >>> that Xen runs on EFI platform or not. > >> >> I disagree here. > >> >> > >> >>> efi_loader is used to differentiate between EFI native loader > >> >>> and multiboot2 protocol. > >> >> And I agree here. > >> > > >> > I suppose "built with efi support" is known because of CONFIG_EFI or > >> > not, and doesn't need a variable. > >> > > >> > However, "booted legacy" vs "booted EFI" does need distinguishing at > >> > runtime, as either is possible. > >> > >> Right, but that's what efi_enabled is supposed to express after > >> the change; there's no need to express "built with EFI support". > >> It just so happens that right now, without all these changes, > >> built-with-EFI-support == runs-on-EFI. > > > > Then how about 'efi_booted' as the variable name. > > Why should we rename a variable that's perfectly suitable for the > purposes we have? Even more so that we don't just want to > express that we were booted from EFI, but also that we're running > in a respective environment, including using tables coming from > EFI and using runtime services (unless specifically disabled). If > anything we could follow Linux and make efi_enabled a bit mask. OK, so it isn't just to tell that you booted from EFI. -- Len Sorensen ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 05/18] efi: split efi_enabled to efi_platform and efi_loader
On Fri, Mar 27, 2015 at 02:04:22PM +, Jan Beulich wrote: > >>> On 27.03.15 at 14:53, wrote: > > On 27/03/15 13:43, Jan Beulich wrote: > > On 27.03.15 at 14:32, wrote: > >>> On Fri, Feb 20, 2015 at 04:17:35PM +, Jan Beulich wrote: > >>> On 30.01.15 at 18:54, wrote: > > We need more fine grained knowledge about EFI environment and check > > for EFI platform and EFI loader separately to properly support > > multiboot2 protocol. > ... because of ... (i.e. I can't see from the description what the > separation is good for). Looking at the comments you placed > aside the variables doesn't help me either. > > > In general Xen loaded by this protocol uses > > memory mappings and loaded modules in simliar way to Xen loaded > > by multiboot (v1) protocol. Hence, split efi_enabled to efi_platform > > and efi_loader. > And if I'm guessing things right, then introducing efi_loader but > leaving efi_enabled alone (only converting where needed) would > >>> efi_enabled is not fortunate name for new usage. Currently it means > >>> that Xen binary have or does not have EFI support build in. However, > >>> if we build in multiboot2 protocol into xen.gz then it means that > >>> it can ran on legacy BIOS or EFI platform. So, I think that we > >>> should rename efi_enabled to efi_platform because it will mean > >>> that Xen runs on EFI platform or not. > >> I disagree here. > >> > >>> efi_loader is used to differentiate between EFI native loader > >>> and multiboot2 protocol. > >> And I agree here. > > > > I suppose "built with efi support" is known because of CONFIG_EFI or > > not, and doesn't need a variable. > > > > However, "booted legacy" vs "booted EFI" does need distinguishing at > > runtime, as either is possible. > > Right, but that's what efi_enabled is supposed to express after > the change; there's no need to express "built with EFI support". > It just so happens that right now, without all these changes, > built-with-EFI-support == runs-on-EFI. Then how about 'efi_booted' as the variable name. -- Len Sorensen ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 1/5] multiboot2: Fix information request tag size calculation
On Fri, Jan 30, 2015 at 01:52:09PM -0700, Ben Hildred wrote:
> Why do you want the size of a pointer instead of the size of the structure?
Isn't *request_tag the dereferenced pointer, and hence is the size of
the structure, where as before it was the size of a pointer?
--
Len Sorensen
> On Fri, Jan 30, 2015 at 10:59 AM, Daniel Kiper
> wrote:
>
> > Signed-off-by: Daniel Kiper
> > ---
> > grub-core/loader/multiboot_mbi2.c |2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/grub-core/loader/multiboot_mbi2.c
> > b/grub-core/loader/multiboot_mbi2.c
> > index 6f74aee..d7c19bc 100644
> > --- a/grub-core/loader/multiboot_mbi2.c
> > +++ b/grub-core/loader/multiboot_mbi2.c
> > @@ -150,7 +150,7 @@ grub_multiboot_load (grub_file_t file, const char
> > *filename)
> > = (struct multiboot_header_tag_information_request *) tag;
> > if (request_tag->flags & MULTIBOOT_HEADER_TAG_OPTIONAL)
> > break;
> > - for (i = 0; i < (request_tag->size - sizeof (request_tag))
> > + for (i = 0; i < (request_tag->size - sizeof (*request_tag))
> > / sizeof (request_tag->requests[0]); i++)
> > switch (request_tag->requests[i])
> > {
> > --
> > 1.7.10.4
> >
> >
> > ___
> > Grub-devel mailing list
> > grub-de...@gnu.org
> > https://lists.gnu.org/mailman/listinfo/grub-devel
> >
>
>
>
> --
> --
> Ben Hildred
> Automation Support Services
> 303 815 6721
> ___
> Grub-devel mailing list
> grub-de...@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
