grub-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v3 02/10] xen: reduce number of global variables in xen loade


From: Juergen Gross
Subject: Re: [PATCH v3 02/10] xen: reduce number of global variables in xen loader
Date: Thu, 18 Feb 2016 11:34:49 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.6.0

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 <address@hidden>
> 
> Just two nitpicks but in general...
> 
> Reviewed-by: Daniel Kiper <address@hidden>
> 
>> ---
>>  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.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 >> PAGE_SHIFT));
>> -      nr_pt_pages = get_pgtable_size (nr_pages, xen_inf.virt_base);
>> +      nr_pt_pages = get_pgtable_size (nr_pages, 
>> xen_state.xen_inf.virt_base);
>>        nr_need_pages =
>>      nr_info_pages + nr_pt_pages +
>>      ((ADDITIONAL_SIZE + STACK_SIZE) >> PAGE_SHIFT);
>> @@ -278,27 +287,28 @@ grub_xen_boot (void)
>>      }
>>
>>    grub_dprintf ("xen", "bootstrap domain %llx+%llx\n",
>> -            (unsigned long long) xen_inf.virt_base,
>> +            (unsigned long long) xen_state.xen_inf.virt_base,
>>              (unsigned long long) page2offset (nr_pages));
>>
>> -  err = grub_relocator_alloc_chunk_addr (relocator, &ch,
>> -                                     max_addr, page2offset (nr_pt_pages));
>> +  err = grub_relocator_alloc_chunk_addr (xen_state.relocator, &ch,
>> +                                     xen_state.max_addr,
>> +                                     page2offset (nr_pt_pages));
>>    if (err)
>>      return err;
>>
>>    generate_page_table (get_virtual_current_address (ch),
>> -                   max_addr >> PAGE_SHIFT, nr_pages,
>> -                   xen_inf.virt_base, new_mfn_list);
>> +                   xen_state.max_addr >> PAGE_SHIFT, nr_pages,
>> +                   xen_state.xen_inf.virt_base, new_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;
>> +  xen_state.max_addr += page2offset (nr_pt_pages);
>> +  state.stack = xen_state.max_addr + STACK_SIZE + 
>> xen_state.xen_inf.virt_base;
>> +  state.entry_point = xen_state.xen_inf.entry_point;
>>
>> -  next_start.nr_p2m_frames += nr_pt_pages;
>> -  next_start.nr_pt_frames = nr_pt_pages;
>> +  xen_state.next_start.nr_p2m_frames += nr_pt_pages;
>> +  xen_state.next_start.nr_pt_frames = nr_pt_pages;
>>    state.paging_size = nr_pt_pages;
>>
>> -  *nst = next_start;
>> +  *nst = xen_state.next_start;
>>
>>    grub_memset (&gnttab_setver, 0, sizeof (gnttab_setver));
>>
>> @@ -308,24 +318,20 @@ grub_xen_boot (void)
>>    for (i = 0; i < ARRAY_SIZE (grub_xen_shared_info->evtchn_pending); i++)
>>      grub_xen_shared_info->evtchn_pending[i] = 0;
>>
>> -  return grub_relocator_xen_boot (relocator, state, nr_pages,
>> -                              xen_inf.virt_base <
>> +  return grub_relocator_xen_boot (xen_state.relocator, state, nr_pages,
>> +                              xen_state.xen_inf.virt_base <
>>                                PAGE_SIZE ? page2offset (nr_pages) : 0,
>>                                nr_pages - 1,
>>                                page2offset (nr_pages - 1) +
>> -                              xen_inf.virt_base);
>> +                              xen_state.xen_inf.virt_base);
>>  }
>>
>>  static void
>>  grub_xen_reset (void)
>>  {
>> -  grub_memset (&next_start, 0, sizeof (next_start));
>> -  xen_module_info_page = NULL;
>> -  n_modules = 0;
>> +  grub_relocator_unload (xen_state.relocator);
>>
>> -  grub_relocator_unload (relocator);
>> -  relocator = NULL;
>> -  loaded = 0;
>> +  grub_memset (&xen_state, 0, sizeof (xen_state));
>>  }
>>
>>  static grub_err_t
>> @@ -409,17 +415,22 @@ grub_cmd_xen (grub_command_t cmd __attribute__ 
>> ((unused)),
>>    grub_file_t file;
>>    grub_elf_t elf;
>>    grub_err_t err;
>> +  void *kern_chunk_src;
>> +  grub_relocator_chunk_t ch;
>> +  grub_addr_t kern_start;
>> +  grub_addr_t kern_end;
>>
>>    if (argc == 0)
>>      return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("filename expected"));
>>
>> +  /* Call grub_loader_unset early to avoid it being called by 
>> grub_loader_set */
>>    grub_loader_unset ();
>>
>>    grub_xen_reset ();
>>
>>    grub_create_loader_cmdline (argc - 1, argv + 1,
>> -                          (char *) next_start.cmd_line,
>> -                          sizeof (next_start.cmd_line) - 1);
>> +                          (char *) xen_state.next_start.cmd_line,
>> +                          sizeof (xen_state.next_start.cmd_line) - 1);
>>
>>    file = grub_file_open (argv[0]);
>>    if (!file)
>> @@ -429,85 +440,82 @@ grub_cmd_xen (grub_command_t cmd __attribute__ 
>> ((unused)),
>>    if (!elf)
>>      goto fail;
>>
>> -  err = grub_xen_get_info (elf, &xen_inf);
>> +  err = grub_xen_get_info (elf, &xen_state.xen_inf);
>>    if (err)
>>      goto fail;
>>  #ifdef __x86_64__
>> -  if (xen_inf.arch != GRUB_XEN_FILE_X86_64)
>> +  if (xen_state.xen_inf.arch != GRUB_XEN_FILE_X86_64)
>>  #else
>> -  if (xen_inf.arch != GRUB_XEN_FILE_I386_PAE
>> -      && xen_inf.arch != GRUB_XEN_FILE_I386_PAE_BIMODE)
>> +  if (xen_state.xen_inf.arch != GRUB_XEN_FILE_I386_PAE
>> +      && xen_state.xen_inf.arch != GRUB_XEN_FILE_I386_PAE_BIMODE)
>>  #endif
>>      {
>>        grub_error (GRUB_ERR_BAD_OS, "incompatible architecture: %d",
>> -              xen_inf.arch);
>> +              xen_state.xen_inf.arch);
>>        goto fail;
>>      }
>>
>> -  if (xen_inf.virt_base & (PAGE_SIZE - 1))
>> +  if (xen_state.xen_inf.virt_base & (PAGE_SIZE - 1))
>>      {
>>        grub_error (GRUB_ERR_BAD_OS, "unaligned virt_base");
>>        goto fail;
>>      }
>>    grub_dprintf ("xen", "virt_base = %llx, entry = %llx\n",
>> -            (unsigned long long) xen_inf.virt_base,
>> -            (unsigned long long) xen_inf.entry_point);
>> +            (unsigned long long) xen_state.xen_inf.virt_base,
>> +            (unsigned long long) xen_state.xen_inf.entry_point);
>>
>> -  relocator = grub_relocator_new ();
>> -  if (!relocator)
>> +  xen_state.relocator = grub_relocator_new ();
>> +  if (!xen_state.relocator)
>>      goto fail;
>>
>> -  grub_relocator_chunk_t ch;
>> -  grub_addr_t kern_start = xen_inf.kern_start - xen_inf.paddr_offset;
>> -  grub_addr_t kern_end = xen_inf.kern_end - xen_inf.paddr_offset;
>> +  kern_start = xen_state.xen_inf.kern_start - 
>> xen_state.xen_inf.paddr_offset;
>> +  kern_end = xen_state.xen_inf.kern_end - xen_state.xen_inf.paddr_offset;
>>
>> -  if (xen_inf.has_hypercall_page)
>> +  if (xen_state.xen_inf.has_hypercall_page)
>>      {
>>        grub_dprintf ("xen", "hypercall page at 0x%llx\n",
>> -                (unsigned long long) xen_inf.hypercall_page);
>> -      if (xen_inf.hypercall_page - xen_inf.virt_base < kern_start)
>> -    kern_start = xen_inf.hypercall_page - xen_inf.virt_base;
>> -
>> -      if (xen_inf.hypercall_page - xen_inf.virt_base + PAGE_SIZE > kern_end)
>> -    kern_end = xen_inf.hypercall_page - xen_inf.virt_base + PAGE_SIZE;
>> +                (unsigned long long) xen_state.xen_inf.hypercall_page);
>> +      kern_start = grub_min (kern_start, xen_state.xen_inf.hypercall_page -
>> +                                     xen_state.xen_inf.virt_base);
>> +      kern_end = grub_max (kern_end, xen_state.xen_inf.hypercall_page -
>> +                                 xen_state.xen_inf.virt_base + PAGE_SIZE);
>>      }
>>
>> -  max_addr = ALIGN_UP (kern_end, PAGE_SIZE);
>> +  xen_state.max_addr = ALIGN_UP (kern_end, PAGE_SIZE);
>>
>> -  err = grub_relocator_alloc_chunk_addr (relocator, &ch, kern_start,
>> +  err = grub_relocator_alloc_chunk_addr (xen_state.relocator, &ch, 
>> kern_start,
>>                                       kern_end - kern_start);
>>    if (err)
>>      goto fail;
>>    kern_chunk_src = get_virtual_current_address (ch);
>>
>>    grub_dprintf ("xen", "paddr_offset = 0x%llx\n",
>> -            (unsigned long long) xen_inf.paddr_offset);
>> +            (unsigned long long) xen_state.xen_inf.paddr_offset);
>>    grub_dprintf ("xen", "kern_start = 0x%llx, kern_end = 0x%llx\n",
>> -            (unsigned long long) xen_inf.kern_start,
>> -            (unsigned long long) xen_inf.kern_end);
>> +            (unsigned long long) xen_state.xen_inf.kern_start,
>> +            (unsigned long long) xen_state.xen_inf.kern_end);
>>
>>    err = grub_elfXX_load (elf, argv[0],
>>                       (grub_uint8_t *) kern_chunk_src - kern_start
>> -                     - xen_inf.paddr_offset, 0, 0, 0);
>> +                     - xen_state.xen_inf.paddr_offset, 0, 0, 0);
>>
>> -  if (xen_inf.has_hypercall_page)
>> +  if (xen_state.xen_inf.has_hypercall_page)
>>      {
>>        unsigned i;
>>        for (i = 0; i < PAGE_SIZE / HYPERCALL_INTERFACE_SIZE; i++)
>>      set_hypercall_interface ((grub_uint8_t *) kern_chunk_src +
>>                               i * HYPERCALL_INTERFACE_SIZE +
>> -                             xen_inf.hypercall_page - xen_inf.virt_base -
>> -                             kern_start, i);
>> +                             xen_state.xen_inf.hypercall_page -
>> +                             xen_state.xen_inf.virt_base - kern_start, i);
>>      }
>>
>>    if (err)
>>      goto fail;
>>
>>    grub_dl_ref (my_mod);
>> -  loaded = 1;
>> +  xen_state.loaded = 1;
>>
>>    grub_loader_set (grub_xen_boot, grub_xen_unload, 0);
>> -  loaded = 1;
>>
>>    goto fail;
>>
>> @@ -540,14 +548,14 @@ grub_cmd_initrd (grub_command_t cmd __attribute__ 
>> ((unused)),
>>        goto fail;
>>      }
>>
>> -  if (!loaded)
>> +  if (!xen_state.loaded)
>>      {
>>        grub_error (GRUB_ERR_BAD_ARGUMENT,
>>                N_("you need to load the kernel first"));
>>        goto fail;
>>      }
>>
>> -  if (next_start.mod_start || next_start.mod_len)
>> +  if (xen_state.next_start.mod_start || xen_state.next_start.mod_len)
>>      {
>>        grub_error (GRUB_ERR_BAD_ARGUMENT, N_("initrd already loaded"));
>>        goto fail;
>> @@ -560,7 +568,8 @@ grub_cmd_initrd (grub_command_t cmd __attribute__ 
>> ((unused)),
>>
>>    if (size)
>>      {
>> -      err = grub_relocator_alloc_chunk_addr (relocator, &ch, max_addr, 
>> size);
>> +      err = grub_relocator_alloc_chunk_addr (xen_state.relocator, &ch,
>> +                                         xen_state.max_addr, size);
>>        if (err)
>>      goto fail;
>>
>> @@ -569,13 +578,14 @@ grub_cmd_initrd (grub_command_t cmd __attribute__ 
>> ((unused)),
>>      goto fail;
>>      }
>>
>> -  next_start.mod_start = max_addr + xen_inf.virt_base;
>> -  next_start.mod_len = size;
>> +  xen_state.next_start.mod_start =
>> +    xen_state.max_addr + xen_state.xen_inf.virt_base;
>> +  xen_state.next_start.mod_len = size;
>>
>> -  max_addr = ALIGN_UP (max_addr + size, PAGE_SIZE);
>> +  xen_state.max_addr = ALIGN_UP (xen_state.max_addr + size, PAGE_SIZE);
>>
>>    grub_dprintf ("xen", "Initrd, addr=0x%x, size=0x%x\n",
>> -            (unsigned) next_start.mod_start, (unsigned) size);
>> +            (unsigned) xen_state.next_start.mod_start, (unsigned) size);
>>
>>  fail:
>>    grub_initrd_close (&initrd_ctx);
>> @@ -607,45 +617,48 @@ grub_cmd_module (grub_command_t cmd __attribute__ 
>> ((unused)),
>>    if (argc == 0)
>>      return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("filename expected"));
>>
>> -  if (!loaded)
>> +  if (!xen_state.loaded)
>>      {
>>        return grub_error (GRUB_ERR_BAD_ARGUMENT,
>>                       N_("you need to load the kernel first"));
>>      }
>>
>> -  if ((next_start.mod_start || next_start.mod_len) && !xen_module_info_page)
>> +  if ((xen_state.next_start.mod_start || xen_state.next_start.mod_len) &&
>> +      !xen_state.module_info_page)
>>      {
>>        return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("initrd already 
>> loaded"));
>>      }
>>
>>    /* Leave one space for terminator.  */
>> -  if (n_modules >= MAX_MODULES - 1)
>> +  if (xen_state.n_modules >= MAX_MODULES - 1)
>>      {
>>        return grub_error (GRUB_ERR_BAD_ARGUMENT, "too many modules");
>>      }
>>
>> -  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.

> 
>> +      xen_state.next_start.flags |= SIF_MULTIBOOT_MOD;
>>
>> -      err = grub_relocator_alloc_chunk_addr (relocator, &ch,
>> -                                         max_addr, MAX_MODULES
>> +      err = grub_relocator_alloc_chunk_addr (xen_state.relocator, &ch,
>> +                                         xen_state.max_addr, MAX_MODULES
>>                                           *
>> -                                         sizeof (xen_module_info_page
>> +                                         sizeof (xen_state.module_info_page
>>                                                   [0]));
>>        if (err)
>>      return err;
>> -      xen_module_info_page = get_virtual_current_address (ch);
>> -      grub_memset (xen_module_info_page, 0, MAX_MODULES
>> -               * sizeof (xen_module_info_page[0]));
>> -      max_addr += MAX_MODULES * sizeof (xen_module_info_page[0]);
>> +      xen_state.module_info_page = get_virtual_current_address (ch);
>> +      grub_memset (xen_state.module_info_page, 0, MAX_MODULES
>> +               * sizeof (xen_state.module_info_page[0]));
>> +      xen_state.max_addr +=
>> +    MAX_MODULES * sizeof (xen_state.module_info_page[0]);
> 
> Ditto.

Yep. tab again.


Juergen



reply via email to

[Prev in Thread] Current Thread [Next in Thread]