|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v3 02/10] xen: reduce number of global variables in xen loader
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 <jgross@xxxxxxxx>
>>>
>>> Just two nitpicks but in general...
>>>
>>> Reviewed-by: Daniel Kiper <daniel.kiper@xxxxxxxxxx>
>>>
>>>> ---
>>>> 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.
>
> But your line continuation is indented LESS than the line it is
> continuing, which is clearly NOT the style.
No. Tabs are 8 spaces and have been so in this file and in others as
well.
Juergen
BTW: any reason you omitted me from the to: and cc: list?
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |