From: "Lennart Sorensen" <lsorense@csclub.uwaterloo.ca>
To: Juergen Gross <jgross@suse.com>
Cc: The development of GNU GRUB <grub-devel@gnu.org>,
Daniel Kiper <daniel.kiper@oracle.com>,
xen-devel@lists.xen.org, mchang@suse.com, phcoder@gmail.com
Subject: Re: [PATCH v3 02/10] xen: reduce number of global variables in xen loader
Date: Fri, 19 Feb 2016 11:20:58 -0500 [thread overview]
Message-ID: <20160219162058.GC10673__35940.2366019369$1455898932$gmane$org@csclub.uwaterloo.ca> (raw)
In-Reply-To: <56C6A12F.5030106@suse.com>
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 <jgross@suse.com>
> >>>
> >>> Just two nitpicks but in general...
> >>>
> >>> Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
> >>>
> >>>> ---
> >>>> 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;
Now that we have 8 characters worth of quoting and patch indentation,
it looks perfect again. :)
> >>> 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.
Oh right. The horrible GNU coding style of 2 space indentation but tab
replacing 8 spaces, which then gets messed up and looks weird when you
start quoting it in email. It would be hard to come up with a worse
indentation style than the GNU one.
> BTW: any reason you omitted me from the to: and cc: list?
I just hit reply-all, so no idea why it would have been dropped.
--
Len Sorensen
next prev parent reply other threads:[~2016-02-19 16:20 UTC|newest]
Thread overview: 54+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-02-17 17:19 [PATCH v3 00/10] grub-xen: support booting huge pv-domains Juergen Gross
2016-02-17 17:19 ` [PATCH v3 01/10] xen: make xen loader callable multiple times Juergen Gross
2016-02-18 10:12 ` Daniel Kiper
2016-02-18 10:32 ` Juergen Gross
2016-02-18 16:58 ` Daniel Kiper
2016-02-19 5:21 ` Juergen Gross
2016-02-19 5:21 ` Juergen Gross
2016-02-18 16:58 ` Daniel Kiper
2016-02-17 17:19 ` [PATCH v3 02/10] xen: reduce number of global variables in xen loader Juergen Gross
2016-02-18 10:22 ` Daniel Kiper
2016-02-18 10:34 ` Juergen Gross
2016-02-18 17:00 ` Lennart Sorensen
2016-02-19 4:59 ` [Xen-devel] " Juergen Gross
2016-02-19 16:20 ` Lennart Sorensen
2016-02-19 16:20 ` Lennart Sorensen [this message]
2016-02-19 4:59 ` Juergen Gross
2016-02-18 17:00 ` Lennart Sorensen
2016-02-18 17:13 ` Daniel Kiper
2016-02-18 17:13 ` Daniel Kiper
2016-02-18 10:22 ` Daniel Kiper
2016-02-17 17:19 ` [PATCH v3 03/10] xen: add elfnote.h to avoid using numbers instead of constants Juergen Gross
2016-02-18 10:30 ` Daniel Kiper
2016-02-18 10:35 ` Juergen Gross
2016-02-18 17:15 ` Daniel Kiper
2016-02-18 17:15 ` Daniel Kiper
2016-02-17 17:19 ` Juergen Gross
2016-02-17 17:19 ` [PATCH v3 04/10] xen: synchronize xen header Juergen Gross
2016-02-18 10:33 ` Daniel Kiper
2016-02-17 17:19 ` [PATCH v3 05/10] xen: factor out p2m list allocation into separate function Juergen Gross
2016-02-17 17:19 ` Juergen Gross
2016-02-18 10:39 ` Daniel Kiper
2016-02-18 10:39 ` Daniel Kiper
2016-02-17 17:19 ` [PATCH v3 06/10] xen: factor out allocation of special pages " Juergen Gross
2016-02-18 10:42 ` Daniel Kiper
2016-02-18 10:42 ` Daniel Kiper
2016-02-17 17:19 ` [PATCH v3 07/10] xen: factor out allocation of page tables " Juergen Gross
2016-02-18 10:50 ` Daniel Kiper
2016-02-18 10:50 ` Daniel Kiper
2016-02-17 17:19 ` [PATCH v3 08/10] xen: add capability to load initrd outside of initial mapping Juergen Gross
2016-02-17 17:19 ` Juergen Gross
2016-02-18 11:18 ` Daniel Kiper
2016-02-18 11:18 ` Daniel Kiper
2016-02-18 12:43 ` Juergen Gross
2016-02-18 17:20 ` Daniel Kiper
2016-02-18 17:20 ` Daniel Kiper
2016-02-18 12:43 ` Juergen Gross
2016-02-17 17:19 ` [PATCH v3 09/10] xen: modify page table construction Juergen Gross
2016-02-18 16:40 ` Daniel Kiper
2016-02-19 5:20 ` Juergen Gross
2016-02-19 5:20 ` Juergen Gross
2016-02-18 16:40 ` Daniel Kiper
2016-02-17 17:19 ` [PATCH v3 10/10] xen: add capability to load p2m list outside of kernel mapping Juergen Gross
2016-02-18 16:44 ` Daniel Kiper
2016-02-18 16:44 ` Daniel Kiper
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='20160219162058.GC10673__35940.2366019369$1455898932$gmane$org@csclub.uwaterloo.ca' \
--to=lsorense@csclub.uwaterloo.ca \
--cc=daniel.kiper@oracle.com \
--cc=grub-devel@gnu.org \
--cc=jgross@suse.com \
--cc=mchang@suse.com \
--cc=phcoder@gmail.com \
--cc=xen-devel@lists.xen.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.