Re: [Xen-devel] [PATCH v3 02/10] xen: reduce number of global variables in xen loader

2016-02-19 Thread Lennart Sorensen 
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, , max_addr, 
>  pgtsize);
>  -  next_start.nr_p2m_frames = (pgtsize + PAGE_SIZE - 1) >> PAGE_SHIFT;
>  +  err = grub_relocator_alloc_chunk_addr (xen_state.relocator, ,
>  + 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, ,
>  - max_addr, sizeof (next_start));
>  +  err = grub_relocator_alloc_chunk_addr (xen_state.relocator, ,
>  + 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

Re: [Xen-devel] [PATCH v3 02/10] xen: reduce number of global variables in xen loader

2016-02-18 Thread Juergen Gross 
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, , max_addr, 
 pgtsize);
 -  next_start.nr_p2m_frames = (pgtsize + PAGE_SIZE - 1) >> PAGE_SHIFT;
 +  err = grub_relocator_alloc_chunk_addr (xen_state.relocator, ,
 +   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, ,
 -   max_addr, sizeof (next_start));
 +  err = grub_relocator_alloc_chunk_addr (xen_state.relocator, ,
 +   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 =

Re: [Xen-devel] [PATCH v3 02/10] xen: reduce number of global variables in xen loader

2016-02-18 Thread Daniel Kiper 
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(-)

[...]

> >> -  if (!xen_module_info_page)
> >> +  if (!xen_state.module_info_page)
> >>  {
> >> -  n_modules = 0;
> >> -  max_addr = ALIGN_UP (max_addr, PAGE_SIZE);
> >> -  modules_target_start = max_addr;
> >> -  next_start.mod_start = max_addr + xen_inf.virt_base;
> >> -  next_start.flags |= SIF_MULTIBOOT_MOD;
> >> +  xen_state.n_modules = 0;
> >> +  xen_state.max_addr = ALIGN_UP (xen_state.max_addr, PAGE_SIZE);
> >> +  xen_state.modules_target_start = xen_state.max_addr;
> >> +  xen_state.next_start.mod_start =
> >> +  xen_state.max_addr + xen_state.xen_inf.virt_base;
> >
> > Lost one space.
>
> Really? Common indentation style seams to be to use tabs where possible.
> And this is a tab.

Sorry, I have missed that.

Daniel

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

Re: [Xen-devel] [PATCH v3 02/10] xen: reduce number of global variables in xen loader

2016-02-18 Thread Lennart Sorensen 
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, , max_addr, 
> >> pgtsize);
> >> -  next_start.nr_p2m_frames = (pgtsize + PAGE_SIZE - 1) >> PAGE_SHIFT;
> >> +  err = grub_relocator_alloc_chunk_addr (xen_state.relocator, ,
> >> +   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, ,
> >> -   max_addr, sizeof (next_start));
> >> +  err = grub_relocator_alloc_chunk_addr (xen_state.relocator, ,
> >> +   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;

Re: [Xen-devel] [PATCH v3 02/10] xen: reduce number of global variables in xen loader

2016-02-18 Thread Juergen Gross 
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, , max_addr, pgtsize);
>> -  next_start.nr_p2m_frames = (pgtsize + PAGE_SIZE - 1) >> PAGE_SHIFT;
>> +  err = grub_relocator_alloc_chunk_addr (xen_state.relocator, ,
>> + 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, ,
>> - max_addr, sizeof (next_start));
>> +  err = grub_relocator_alloc_chunk_addr (xen_state.relocator, ,
>> + 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.shared_info = grub_xen_start_page_addr->shared_info;
>>
>> -  err = set_mfns (new_mfn_list, max_addr >> PAGE_SHIFT);
>> +  err = set_mfns (new_mfn_list, xen_state.max_addr >> PAGE_SHIFT);
>>if (err)
>>  return err;
>> -  max_addr += 2 *

Re: [Xen-devel] [PATCH v3 02/10] xen: reduce number of global variables in xen loader

2016-02-18 Thread Daniel Kiper 
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, , max_addr, pgtsize);
> -  next_start.nr_p2m_frames = (pgtsize + PAGE_SIZE - 1) >> PAGE_SHIFT;
> +  err = grub_relocator_alloc_chunk_addr (xen_state.relocator, ,
> +  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, ,
> -  max_addr, sizeof (next_start));
> +  err = grub_relocator_alloc_chunk_addr (xen_state.relocator, ,
> +  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.shared_info = grub_xen_start_page_addr->shared_info;
>
> -  err = set_mfns (new_mfn_list, max_addr >> PAGE_SHIFT);
> +  err = set_mfns (new_mfn_list, xen_state.max_addr >> PAGE_SHIFT);
>if (err)
>  return err;
> -  max_addr += 2 * PAGE_SIZE;
> +  xen_state.max_addr += 2 * PAGE_SIZE;
>
> -  next_start.pt_base = max_addr + xen_inf.virt_base;
> -  state.paging_start = max_addr >> PAGE_SHIFT;
> +

[Xen-devel] [PATCH v3 02/10] xen: reduce number of global variables in xen loader

2016-02-17 Thread Juergen Gross 
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 
---
 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, , max_addr, pgtsize);
-  next_start.nr_p2m_frames = (pgtsize + PAGE_SIZE - 1) >> PAGE_SHIFT;
+  err = grub_relocator_alloc_chunk_addr (xen_state.relocator, ,
+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, ,
-max_addr, sizeof (next_start));
+  err = grub_relocator_alloc_chunk_addr (xen_state.relocator, ,
+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.shared_info = grub_xen_start_page_addr->shared_info;
 
-  err = set_mfns (new_mfn_list, max_addr >> PAGE_SHIFT);
+  err = set_mfns (new_mfn_list, xen_state.max_addr >> PAGE_SHIFT);
   if (err)
 return err;
-  max_addr += 2 * PAGE_SIZE;
+  xen_state.max_addr += 2 * PAGE_SIZE;
 
-  next_start.pt_base = max_addr + xen_inf.virt_base;
-  state.paging_start = max_addr >> PAGE_SHIFT;
+  xen_state.next_start.pt_base =
+xen_state.max_addr + xen_state.xen_inf.virt_base;
+  state.paging_start = xen_state.max_addr >> PAGE_SHIFT;
 
-  nr_info_pages = max_addr >> PAGE_SHIFT;
+  nr_info_pages = xen_state.max_addr >> PAGE_SHIFT;
   nr_pages = nr_info_pages;
 
   while (1)
 {
   nr_pages = ALIGN_UP (nr_pages, (ALIGN_SIZE >>