* [PATCH v4 01/11] xen: make xen loader callable multiple times
[not found] <1456120999-5639-1-git-send-email-jgross@suse.com>
@ 2016-02-22 6:03 ` Juergen Gross
2016-02-22 6:03 ` [PATCH v4 02/11] xen: avoid memleaks on error Juergen Gross
` (12 subsequent siblings)
13 siblings, 0 replies; 33+ messages in thread
From: Juergen Gross @ 2016-02-22 6:03 UTC (permalink / raw)
To: grub-devel; +Cc: Juergen Gross, phcoder, daniel.kiper, mchang, xen-devel
The loader for xen paravirtualized environment isn't callable multiple
times as it won't free any memory in case of failure.
Call grub_relocator_unload() as other modules do it before allocating
a new relocator or when unloading the module.
Signed-off-by: Juergen Gross <jgross@suse.com>
---
V4: split patch into two patches
add comment
---
grub-core/loader/i386/xen.c | 27 +++++++++++++++++++--------
1 file changed, 19 insertions(+), 8 deletions(-)
diff --git a/grub-core/loader/i386/xen.c b/grub-core/loader/i386/xen.c
index c4d9689..179e89c 100644
--- a/grub-core/loader/i386/xen.c
+++ b/grub-core/loader/i386/xen.c
@@ -316,11 +316,23 @@ grub_xen_boot (void)
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 (relocator);
+ relocator = NULL;
+ loaded = 0;
+}
+
static grub_err_t
grub_xen_unload (void)
{
+ grub_xen_reset ();
grub_dl_unref (my_mod);
- loaded = 0;
return GRUB_ERR_NONE;
}
@@ -403,10 +415,7 @@ grub_cmd_xen (grub_command_t cmd __attribute__ ((unused)),
grub_loader_unset ();
- grub_memset (&next_start, 0, sizeof (next_start));
-
- xen_module_info_page = NULL;
- n_modules = 0;
+ grub_xen_reset ();
grub_create_loader_cmdline (argc - 1, argv + 1,
(char *) next_start.cmd_line,
@@ -503,16 +512,18 @@ grub_cmd_xen (grub_command_t cmd __attribute__ ((unused)),
goto fail;
fail:
+ /* grub_errno might be clobbered by further calls, save the error reason. */
+ err = grub_errno;
if (elf)
grub_elf_close (elf);
else if (file)
grub_file_close (file);
- if (grub_errno != GRUB_ERR_NONE)
- loaded = 0;
+ if (err != GRUB_ERR_NONE)
+ grub_xen_reset ();
- return grub_errno;
+ return err;
}
static grub_err_t
--
2.6.2
^ permalink raw reply related [flat|nested] 33+ messages in thread* [PATCH v4 02/11] xen: avoid memleaks on error
[not found] <1456120999-5639-1-git-send-email-jgross@suse.com>
2016-02-22 6:03 ` [PATCH v4 01/11] xen: make xen loader callable multiple times Juergen Gross
@ 2016-02-22 6:03 ` Juergen Gross
2016-02-22 6:03 ` [PATCH v4 03/11] xen: reduce number of global variables in xen loader Juergen Gross
` (11 subsequent siblings)
13 siblings, 0 replies; 33+ messages in thread
From: Juergen Gross @ 2016-02-22 6:03 UTC (permalink / raw)
To: grub-devel; +Cc: Juergen Gross, phcoder, daniel.kiper, mchang, xen-devel
When loading a Xen pv-kernel avoid memory leaks in case of errors.
Signed-off-by: Juergen Gross <jgross@suse.com>
---
grub-core/loader/i386/xen.c | 2 +-
grub-core/loader/i386/xen_fileXX.c | 17 +++++++++++------
2 files changed, 12 insertions(+), 7 deletions(-)
diff --git a/grub-core/loader/i386/xen.c b/grub-core/loader/i386/xen.c
index 179e89c..f45f70f 100644
--- a/grub-core/loader/i386/xen.c
+++ b/grub-core/loader/i386/xen.c
@@ -563,7 +563,7 @@ grub_cmd_initrd (grub_command_t cmd __attribute__ ((unused)),
{
err = grub_relocator_alloc_chunk_addr (relocator, &ch, max_addr, size);
if (err)
- return err;
+ goto fail;
if (grub_initrd_load (&initrd_ctx, argv,
get_virtual_current_address (ch)))
diff --git a/grub-core/loader/i386/xen_fileXX.c b/grub-core/loader/i386/xen_fileXX.c
index 1ba5649..5475819 100644
--- a/grub-core/loader/i386/xen_fileXX.c
+++ b/grub-core/loader/i386/xen_fileXX.c
@@ -35,7 +35,8 @@ parse_xen_guest (grub_elf_t elf, struct grub_xen_file_info *xi,
if (grub_file_read (elf->file, buf, sz) != (grub_ssize_t) sz)
{
if (grub_errno)
- return grub_errno;
+ goto out;
+ grub_free (buf);
return grub_error (GRUB_ERR_BAD_OS, N_("premature end of file %s"),
elf->file->name);
}
@@ -123,14 +124,14 @@ parse_xen_guest (grub_elf_t elf, struct grub_xen_file_info *xi,
{
xi->virt_base = grub_strtoull (ptr + sizeof ("VIRT_BASE=") - 1, &ptr, 16);
if (grub_errno)
- return grub_errno;
+ goto out;
continue;
}
if (grub_strncmp (ptr, "VIRT_ENTRY=", sizeof ("VIRT_ENTRY=") - 1) == 0)
{
xi->entry_point = grub_strtoull (ptr + sizeof ("VIRT_ENTRY=") - 1, &ptr, 16);
if (grub_errno)
- return grub_errno;
+ goto out;
continue;
}
if (grub_strncmp (ptr, "HYPERCALL_PAGE=", sizeof ("HYPERCALL_PAGE=") - 1) == 0)
@@ -138,7 +139,7 @@ parse_xen_guest (grub_elf_t elf, struct grub_xen_file_info *xi,
xi->hypercall_page = grub_strtoull (ptr + sizeof ("HYPERCALL_PAGE=") - 1, &ptr, 16);
xi->has_hypercall_page = 1;
if (grub_errno)
- return grub_errno;
+ goto out;
continue;
}
if (grub_strncmp (ptr, "ELF_PADDR_OFFSET=", sizeof ("ELF_PADDR_OFFSET=") - 1) == 0)
@@ -146,7 +147,7 @@ parse_xen_guest (grub_elf_t elf, struct grub_xen_file_info *xi,
xi->paddr_offset = grub_strtoull (ptr + sizeof ("ELF_PADDR_OFFSET=") - 1, &ptr, 16);
has_paddr = 1;
if (grub_errno)
- return grub_errno;
+ goto out;
continue;
}
}
@@ -154,7 +155,11 @@ parse_xen_guest (grub_elf_t elf, struct grub_xen_file_info *xi,
xi->hypercall_page = (xi->hypercall_page << 12) + xi->virt_base;
if (!has_paddr)
xi->paddr_offset = xi->virt_base;
- return GRUB_ERR_NONE;
+
+out:
+ grub_free (buf);
+
+ return grub_errno;
}
#pragma GCC diagnostic ignored "-Wcast-align"
--
2.6.2
^ permalink raw reply related [flat|nested] 33+ messages in thread* [PATCH v4 03/11] xen: reduce number of global variables in xen loader
[not found] <1456120999-5639-1-git-send-email-jgross@suse.com>
2016-02-22 6:03 ` [PATCH v4 01/11] xen: make xen loader callable multiple times Juergen Gross
2016-02-22 6:03 ` [PATCH v4 02/11] xen: avoid memleaks on error Juergen Gross
@ 2016-02-22 6:03 ` Juergen Gross
2016-02-22 6:03 ` [PATCH v4 04/11] xen: add elfnote.h to avoid using numbers instead of constants Juergen Gross
` (10 subsequent siblings)
13 siblings, 0 replies; 33+ messages in thread
From: Juergen Gross @ 2016-02-22 6:03 UTC (permalink / raw)
To: grub-devel; +Cc: Juergen Gross, phcoder, daniel.kiper, mchang, xen-devel
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>
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 f45f70f..691c22e 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;
@@ -541,14 +549,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;
@@ -561,7 +569,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;
@@ -570,13 +579,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);
@@ -608,45 +618,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;
+ 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]);
}
- max_addr = ALIGN_UP (max_addr, PAGE_SIZE);
+ xen_state.max_addr = ALIGN_UP (xen_state.max_addr, PAGE_SIZE);
if (nounzip)
grub_file_filter_disable_compression ();
@@ -657,20 +670,22 @@ grub_cmd_module (grub_command_t cmd __attribute__ ((unused)),
cmdline_len = grub_loader_cmdline_size (argc - 1, argv + 1);
- err = grub_relocator_alloc_chunk_addr (relocator, &ch,
- max_addr, cmdline_len);
+ err = grub_relocator_alloc_chunk_addr (xen_state.relocator, &ch,
+ xen_state.max_addr, cmdline_len);
if (err)
goto fail;
grub_create_loader_cmdline (argc - 1, argv + 1,
get_virtual_current_address (ch), cmdline_len);
- xen_module_info_page[n_modules].cmdline = max_addr - modules_target_start;
- max_addr = ALIGN_UP (max_addr + cmdline_len, PAGE_SIZE);
+ xen_state.module_info_page[xen_state.n_modules].cmdline =
+ xen_state.max_addr - xen_state.modules_target_start;
+ xen_state.max_addr = ALIGN_UP (xen_state.max_addr + cmdline_len, PAGE_SIZE);
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;
if (grub_file_read (file, get_virtual_current_address (ch), size)
@@ -682,15 +697,17 @@ grub_cmd_module (grub_command_t cmd __attribute__ ((unused)),
goto fail;
}
}
- next_start.mod_len = max_addr + size - modules_target_start;
- xen_module_info_page[n_modules].mod_start = max_addr - modules_target_start;
- xen_module_info_page[n_modules].mod_end =
- max_addr + size - modules_target_start;
+ xen_state.next_start.mod_len =
+ xen_state.max_addr + size - xen_state.modules_target_start;
+ xen_state.module_info_page[xen_state.n_modules].mod_start =
+ xen_state.max_addr - xen_state.modules_target_start;
+ xen_state.module_info_page[xen_state.n_modules].mod_end =
+ xen_state.max_addr + size - xen_state.modules_target_start;
- n_modules++;
+ xen_state.n_modules++;
grub_dprintf ("xen", "module, addr=0x%x, size=0x%x\n",
- (unsigned) max_addr, (unsigned) size);
- max_addr = ALIGN_UP (max_addr + size, PAGE_SIZE);
+ (unsigned) xen_state.max_addr, (unsigned) size);
+ xen_state.max_addr = ALIGN_UP (xen_state.max_addr + size, PAGE_SIZE);
fail:
--
2.6.2
^ permalink raw reply related [flat|nested] 33+ messages in thread* [PATCH v4 04/11] xen: add elfnote.h to avoid using numbers instead of constants
[not found] <1456120999-5639-1-git-send-email-jgross@suse.com>
` (2 preceding siblings ...)
2016-02-22 6:03 ` [PATCH v4 03/11] xen: reduce number of global variables in xen loader Juergen Gross
@ 2016-02-22 6:03 ` Juergen Gross
2016-02-22 6:03 ` [PATCH v4 05/11] xen: synchronize xen header Juergen Gross
` (9 subsequent siblings)
13 siblings, 0 replies; 33+ messages in thread
From: Juergen Gross @ 2016-02-22 6:03 UTC (permalink / raw)
To: grub-devel; +Cc: Juergen Gross, phcoder, daniel.kiper, mchang, xen-devel
Various features and parameters of a pv-kernel are specified via
elf notes in the kernel image. Those notes are part of the interface
between the Xen hypervisor and the kernel.
Instead of using num,bers in the code when interpreting the elf notes
make use of the header supplied by Xen for that purpose.
Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
---
grub-core/loader/i386/xen_fileXX.c | 19 +--
include/xen/elfnote.h | 281 +++++++++++++++++++++++++++++++++++++
2 files changed, 291 insertions(+), 9 deletions(-)
create mode 100644 include/xen/elfnote.h
diff --git a/grub-core/loader/i386/xen_fileXX.c b/grub-core/loader/i386/xen_fileXX.c
index 5475819..1f7f71d 100644
--- a/grub-core/loader/i386/xen_fileXX.c
+++ b/grub-core/loader/i386/xen_fileXX.c
@@ -18,6 +18,7 @@
#include <grub/xen_file.h>
#include <grub/misc.h>
+#include <xen/elfnote.h>
static grub_err_t
parse_xen_guest (grub_elf_t elf, struct grub_xen_file_info *xi,
@@ -201,35 +202,35 @@ parse_note (grub_elf_t elf, struct grub_xen_file_info *xi,
xi->has_note = 1;
switch (nh->n_type)
{
- case 1:
+ case XEN_ELFNOTE_ENTRY:
xi->entry_point = grub_le_to_cpu_addr (*(Elf_Addr *) desc);
break;
- case 2:
+ case XEN_ELFNOTE_HYPERCALL_PAGE:
xi->hypercall_page = grub_le_to_cpu_addr (*(Elf_Addr *) desc);
xi->has_hypercall_page = 1;
break;
- case 3:
+ case XEN_ELFNOTE_VIRT_BASE:
xi->virt_base = grub_le_to_cpu_addr (*(Elf_Addr *) desc);
break;
- case 4:
+ case XEN_ELFNOTE_PADDR_OFFSET:
xi->paddr_offset = grub_le_to_cpu_addr (*(Elf_Addr *) desc);
break;
- case 5:
+ case XEN_ELFNOTE_XEN_VERSION:
grub_dprintf ("xen", "xenversion = `%s'\n", (char *) desc);
break;
- case 6:
+ case XEN_ELFNOTE_GUEST_OS:
grub_dprintf ("xen", "name = `%s'\n", (char *) desc);
break;
- case 7:
+ case XEN_ELFNOTE_GUEST_VERSION:
grub_dprintf ("xen", "version = `%s'\n", (char *) desc);
break;
- case 8:
+ case XEN_ELFNOTE_LOADER:
if (descsz < 7
|| grub_memcmp (desc, "generic", descsz == 7 ? 7 : 8) != 0)
return grub_error (GRUB_ERR_BAD_OS, "invalid loader");
break;
/* PAE */
- case 9:
+ case XEN_ELFNOTE_PAE_MODE:
grub_dprintf ("xen", "pae = `%s', %d, %d\n", (char *) desc,
xi->arch, descsz);
if (xi->arch != GRUB_XEN_FILE_I386
diff --git a/include/xen/elfnote.h b/include/xen/elfnote.h
new file mode 100644
index 0000000..353985f
--- /dev/null
+++ b/include/xen/elfnote.h
@@ -0,0 +1,281 @@
+/******************************************************************************
+ * elfnote.h
+ *
+ * Definitions used for the Xen ELF notes.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to
+ * deal in the Software without restriction, including without limitation the
+ * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
+ * sell copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
+ * DEALINGS IN THE SOFTWARE.
+ *
+ * Copyright (c) 2006, Ian Campbell, XenSource Ltd.
+ */
+
+#ifndef __XEN_PUBLIC_ELFNOTE_H__
+#define __XEN_PUBLIC_ELFNOTE_H__
+
+/*
+ * `incontents 200 elfnotes ELF notes
+ *
+ * The notes should live in a PT_NOTE segment and have "Xen" in the
+ * name field.
+ *
+ * Numeric types are either 4 or 8 bytes depending on the content of
+ * the desc field.
+ *
+ * LEGACY indicated the fields in the legacy __xen_guest string which
+ * this a note type replaces.
+ *
+ * String values (for non-legacy) are NULL terminated ASCII, also known
+ * as ASCIZ type.
+ */
+
+/*
+ * NAME=VALUE pair (string).
+ */
+#define XEN_ELFNOTE_INFO 0
+
+/*
+ * The virtual address of the entry point (numeric).
+ *
+ * LEGACY: VIRT_ENTRY
+ */
+#define XEN_ELFNOTE_ENTRY 1
+
+/* The virtual address of the hypercall transfer page (numeric).
+ *
+ * LEGACY: HYPERCALL_PAGE. (n.b. legacy value is a physical page
+ * number not a virtual address)
+ */
+#define XEN_ELFNOTE_HYPERCALL_PAGE 2
+
+/* The virtual address where the kernel image should be mapped (numeric).
+ *
+ * Defaults to 0.
+ *
+ * LEGACY: VIRT_BASE
+ */
+#define XEN_ELFNOTE_VIRT_BASE 3
+
+/*
+ * The offset of the ELF paddr field from the actual required
+ * pseudo-physical address (numeric).
+ *
+ * This is used to maintain backwards compatibility with older kernels
+ * which wrote __PAGE_OFFSET into that field. This field defaults to 0
+ * if not present.
+ *
+ * LEGACY: ELF_PADDR_OFFSET. (n.b. legacy default is VIRT_BASE)
+ */
+#define XEN_ELFNOTE_PADDR_OFFSET 4
+
+/*
+ * The version of Xen that we work with (string).
+ *
+ * LEGACY: XEN_VER
+ */
+#define XEN_ELFNOTE_XEN_VERSION 5
+
+/*
+ * The name of the guest operating system (string).
+ *
+ * LEGACY: GUEST_OS
+ */
+#define XEN_ELFNOTE_GUEST_OS 6
+
+/*
+ * The version of the guest operating system (string).
+ *
+ * LEGACY: GUEST_VER
+ */
+#define XEN_ELFNOTE_GUEST_VERSION 7
+
+/*
+ * The loader type (string).
+ *
+ * LEGACY: LOADER
+ */
+#define XEN_ELFNOTE_LOADER 8
+
+/*
+ * The kernel supports PAE (x86/32 only, string = "yes", "no" or
+ * "bimodal").
+ *
+ * For compatibility with Xen 3.0.3 and earlier the "bimodal" setting
+ * may be given as "yes,bimodal" which will cause older Xen to treat
+ * this kernel as PAE.
+ *
+ * LEGACY: PAE (n.b. The legacy interface included a provision to
+ * indicate 'extended-cr3' support allowing L3 page tables to be
+ * placed above 4G. It is assumed that any kernel new enough to use
+ * these ELF notes will include this and therefore "yes" here is
+ * equivalent to "yes[entended-cr3]" in the __xen_guest interface.
+ */
+#define XEN_ELFNOTE_PAE_MODE 9
+
+/*
+ * The features supported/required by this kernel (string).
+ *
+ * The string must consist of a list of feature names (as given in
+ * features.h, without the "XENFEAT_" prefix) separated by '|'
+ * characters. If a feature is required for the kernel to function
+ * then the feature name must be preceded by a '!' character.
+ *
+ * LEGACY: FEATURES
+ */
+#define XEN_ELFNOTE_FEATURES 10
+
+/*
+ * The kernel requires the symbol table to be loaded (string = "yes" or "no")
+ * LEGACY: BSD_SYMTAB (n.b. The legacy treated the presence or absence
+ * of this string as a boolean flag rather than requiring "yes" or
+ * "no".
+ */
+#define XEN_ELFNOTE_BSD_SYMTAB 11
+
+/*
+ * The lowest address the hypervisor hole can begin at (numeric).
+ *
+ * This must not be set higher than HYPERVISOR_VIRT_START. Its presence
+ * also indicates to the hypervisor that the kernel can deal with the
+ * hole starting at a higher address.
+ */
+#define XEN_ELFNOTE_HV_START_LOW 12
+
+/*
+ * List of maddr_t-sized mask/value pairs describing how to recognize
+ * (non-present) L1 page table entries carrying valid MFNs (numeric).
+ */
+#define XEN_ELFNOTE_L1_MFN_VALID 13
+
+/*
+ * Whether or not the guest supports cooperative suspend cancellation.
+ * This is a numeric value.
+ *
+ * Default is 0
+ */
+#define XEN_ELFNOTE_SUSPEND_CANCEL 14
+
+/*
+ * The (non-default) location the initial phys-to-machine map should be
+ * placed at by the hypervisor (Dom0) or the tools (DomU).
+ * The kernel must be prepared for this mapping to be established using
+ * large pages, despite such otherwise not being available to guests.
+ * The kernel must also be able to handle the page table pages used for
+ * this mapping not being accessible through the initial mapping.
+ * (Only x86-64 supports this at present.)
+ */
+#define XEN_ELFNOTE_INIT_P2M 15
+
+/*
+ * Whether or not the guest can deal with being passed an initrd not
+ * mapped through its initial page tables.
+ */
+#define XEN_ELFNOTE_MOD_START_PFN 16
+
+/*
+ * The features supported by this kernel (numeric).
+ *
+ * Other than XEN_ELFNOTE_FEATURES on pre-4.2 Xen, this note allows a
+ * kernel to specify support for features that older hypervisors don't
+ * know about. The set of features 4.2 and newer hypervisors will
+ * consider supported by the kernel is the combination of the sets
+ * specified through this and the string note.
+ *
+ * LEGACY: FEATURES
+ */
+#define XEN_ELFNOTE_SUPPORTED_FEATURES 17
+
+/*
+ * Physical entry point into the kernel.
+ *
+ * 32bit entry point into the kernel. When requested to launch the
+ * guest kernel in a HVM container, Xen will use this entry point to
+ * launch the guest in 32bit protected mode with paging disabled.
+ * Ignored otherwise.
+ */
+#define XEN_ELFNOTE_PHYS32_ENTRY 18
+
+/*
+ * The number of the highest elfnote defined.
+ */
+#define XEN_ELFNOTE_MAX XEN_ELFNOTE_PHYS32_ENTRY
+
+/*
+ * System information exported through crash notes.
+ *
+ * The kexec / kdump code will create one XEN_ELFNOTE_CRASH_INFO
+ * note in case of a system crash. This note will contain various
+ * information about the system, see xen/include/xen/elfcore.h.
+ */
+#define XEN_ELFNOTE_CRASH_INFO 0x1000001
+
+/*
+ * System registers exported through crash notes.
+ *
+ * The kexec / kdump code will create one XEN_ELFNOTE_CRASH_REGS
+ * note per cpu in case of a system crash. This note is architecture
+ * specific and will contain registers not saved in the "CORE" note.
+ * See xen/include/xen/elfcore.h for more information.
+ */
+#define XEN_ELFNOTE_CRASH_REGS 0x1000002
+
+
+/*
+ * xen dump-core none note.
+ * xm dump-core code will create one XEN_ELFNOTE_DUMPCORE_NONE
+ * in its dump file to indicate that the file is xen dump-core
+ * file. This note doesn't have any other information.
+ * See tools/libxc/xc_core.h for more information.
+ */
+#define XEN_ELFNOTE_DUMPCORE_NONE 0x2000000
+
+/*
+ * xen dump-core header note.
+ * xm dump-core code will create one XEN_ELFNOTE_DUMPCORE_HEADER
+ * in its dump file.
+ * See tools/libxc/xc_core.h for more information.
+ */
+#define XEN_ELFNOTE_DUMPCORE_HEADER 0x2000001
+
+/*
+ * xen dump-core xen version note.
+ * xm dump-core code will create one XEN_ELFNOTE_DUMPCORE_XEN_VERSION
+ * in its dump file. It contains the xen version obtained via the
+ * XENVER hypercall.
+ * See tools/libxc/xc_core.h for more information.
+ */
+#define XEN_ELFNOTE_DUMPCORE_XEN_VERSION 0x2000002
+
+/*
+ * xen dump-core format version note.
+ * xm dump-core code will create one XEN_ELFNOTE_DUMPCORE_FORMAT_VERSION
+ * in its dump file. It contains a format version identifier.
+ * See tools/libxc/xc_core.h for more information.
+ */
+#define XEN_ELFNOTE_DUMPCORE_FORMAT_VERSION 0x2000003
+
+#endif /* __XEN_PUBLIC_ELFNOTE_H__ */
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
--
2.6.2
^ permalink raw reply related [flat|nested] 33+ messages in thread* [PATCH v4 05/11] xen: synchronize xen header
[not found] <1456120999-5639-1-git-send-email-jgross@suse.com>
` (3 preceding siblings ...)
2016-02-22 6:03 ` [PATCH v4 04/11] xen: add elfnote.h to avoid using numbers instead of constants Juergen Gross
@ 2016-02-22 6:03 ` Juergen Gross
2016-02-22 6:03 ` [PATCH v4 06/11] xen: factor out p2m list allocation into separate function Juergen Gross
` (8 subsequent siblings)
13 siblings, 0 replies; 33+ messages in thread
From: Juergen Gross @ 2016-02-22 6:03 UTC (permalink / raw)
To: grub-devel; +Cc: Juergen Gross, phcoder, daniel.kiper, mchang, xen-devel
Get actual version of include/xen/xen.h from the Xen repository in
order to be able to use constants defined there.
Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
---
include/xen/arch-x86/xen-x86_32.h | 22 +++----
include/xen/arch-x86/xen-x86_64.h | 8 +--
include/xen/xen.h | 125 +++++++++++++++++++++++++++-----------
3 files changed, 105 insertions(+), 50 deletions(-)
diff --git a/include/xen/arch-x86/xen-x86_32.h b/include/xen/arch-x86/xen-x86_32.h
index 1504191..7eca6cd 100644
--- a/include/xen/arch-x86/xen-x86_32.h
+++ b/include/xen/arch-x86/xen-x86_32.h
@@ -58,34 +58,31 @@
#define __HYPERVISOR_VIRT_START_PAE 0xF5800000
#define __MACH2PHYS_VIRT_START_PAE 0xF5800000
#define __MACH2PHYS_VIRT_END_PAE 0xF6800000
-#define HYPERVISOR_VIRT_START_PAE \
- mk_unsigned_long(__HYPERVISOR_VIRT_START_PAE)
-#define MACH2PHYS_VIRT_START_PAE \
- mk_unsigned_long(__MACH2PHYS_VIRT_START_PAE)
-#define MACH2PHYS_VIRT_END_PAE \
- mk_unsigned_long(__MACH2PHYS_VIRT_END_PAE)
+#define HYPERVISOR_VIRT_START_PAE xen_mk_ulong(__HYPERVISOR_VIRT_START_PAE)
+#define MACH2PHYS_VIRT_START_PAE xen_mk_ulong(__MACH2PHYS_VIRT_START_PAE)
+#define MACH2PHYS_VIRT_END_PAE xen_mk_ulong(__MACH2PHYS_VIRT_END_PAE)
/* Non-PAE bounds are obsolete. */
#define __HYPERVISOR_VIRT_START_NONPAE 0xFC000000
#define __MACH2PHYS_VIRT_START_NONPAE 0xFC000000
#define __MACH2PHYS_VIRT_END_NONPAE 0xFC400000
#define HYPERVISOR_VIRT_START_NONPAE \
- mk_unsigned_long(__HYPERVISOR_VIRT_START_NONPAE)
+ xen_mk_ulong(__HYPERVISOR_VIRT_START_NONPAE)
#define MACH2PHYS_VIRT_START_NONPAE \
- mk_unsigned_long(__MACH2PHYS_VIRT_START_NONPAE)
+ xen_mk_ulong(__MACH2PHYS_VIRT_START_NONPAE)
#define MACH2PHYS_VIRT_END_NONPAE \
- mk_unsigned_long(__MACH2PHYS_VIRT_END_NONPAE)
+ xen_mk_ulong(__MACH2PHYS_VIRT_END_NONPAE)
#define __HYPERVISOR_VIRT_START __HYPERVISOR_VIRT_START_PAE
#define __MACH2PHYS_VIRT_START __MACH2PHYS_VIRT_START_PAE
#define __MACH2PHYS_VIRT_END __MACH2PHYS_VIRT_END_PAE
#ifndef HYPERVISOR_VIRT_START
-#define HYPERVISOR_VIRT_START mk_unsigned_long(__HYPERVISOR_VIRT_START)
+#define HYPERVISOR_VIRT_START xen_mk_ulong(__HYPERVISOR_VIRT_START)
#endif
-#define MACH2PHYS_VIRT_START mk_unsigned_long(__MACH2PHYS_VIRT_START)
-#define MACH2PHYS_VIRT_END mk_unsigned_long(__MACH2PHYS_VIRT_END)
+#define MACH2PHYS_VIRT_START xen_mk_ulong(__MACH2PHYS_VIRT_START)
+#define MACH2PHYS_VIRT_END xen_mk_ulong(__MACH2PHYS_VIRT_END)
#define MACH2PHYS_NR_ENTRIES ((MACH2PHYS_VIRT_END-MACH2PHYS_VIRT_START)>>2)
#ifndef machine_to_phys_mapping
#define machine_to_phys_mapping ((unsigned long *)MACH2PHYS_VIRT_START)
@@ -104,6 +101,7 @@
do { if ( sizeof(hnd) == 8 ) *(uint64_t *)&(hnd) = 0; \
(hnd).p = val; \
} while ( 0 )
+#define int64_aligned_t int64_t __attribute__((aligned(8)))
#define uint64_aligned_t uint64_t __attribute__((aligned(8)))
#define __XEN_GUEST_HANDLE_64(name) __guest_handle_64_ ## name
#define XEN_GUEST_HANDLE_64(name) __XEN_GUEST_HANDLE_64(name)
diff --git a/include/xen/arch-x86/xen-x86_64.h b/include/xen/arch-x86/xen-x86_64.h
index 1c4e159..5e18613 100644
--- a/include/xen/arch-x86/xen-x86_64.h
+++ b/include/xen/arch-x86/xen-x86_64.h
@@ -76,12 +76,12 @@
#define __MACH2PHYS_VIRT_END 0xFFFF804000000000
#ifndef HYPERVISOR_VIRT_START
-#define HYPERVISOR_VIRT_START mk_unsigned_long(__HYPERVISOR_VIRT_START)
-#define HYPERVISOR_VIRT_END mk_unsigned_long(__HYPERVISOR_VIRT_END)
+#define HYPERVISOR_VIRT_START xen_mk_ulong(__HYPERVISOR_VIRT_START)
+#define HYPERVISOR_VIRT_END xen_mk_ulong(__HYPERVISOR_VIRT_END)
#endif
-#define MACH2PHYS_VIRT_START mk_unsigned_long(__MACH2PHYS_VIRT_START)
-#define MACH2PHYS_VIRT_END mk_unsigned_long(__MACH2PHYS_VIRT_END)
+#define MACH2PHYS_VIRT_START xen_mk_ulong(__MACH2PHYS_VIRT_START)
+#define MACH2PHYS_VIRT_END xen_mk_ulong(__MACH2PHYS_VIRT_END)
#define MACH2PHYS_NR_ENTRIES ((MACH2PHYS_VIRT_END-MACH2PHYS_VIRT_START)>>3)
#ifndef machine_to_phys_mapping
#define machine_to_phys_mapping ((unsigned long *)HYPERVISOR_VIRT_START)
diff --git a/include/xen/xen.h b/include/xen/xen.h
index a6a2092..6c9e42b 100644
--- a/include/xen/xen.h
+++ b/include/xen/xen.h
@@ -52,6 +52,19 @@ DEFINE_XEN_GUEST_HANDLE(void);
DEFINE_XEN_GUEST_HANDLE(uint64_t);
DEFINE_XEN_GUEST_HANDLE(xen_pfn_t);
DEFINE_XEN_GUEST_HANDLE(xen_ulong_t);
+
+/* Turn a plain number into a C unsigned (long) constant. */
+#define __xen_mk_uint(x) x ## U
+#define __xen_mk_ulong(x) x ## UL
+#define xen_mk_uint(x) __xen_mk_uint(x)
+#define xen_mk_ulong(x) __xen_mk_ulong(x)
+
+#else
+
+/* In assembly code we cannot use C numeric constant suffixes. */
+#define xen_mk_uint(x) x
+#define xen_mk_ulong(x) x
+
#endif
/*
@@ -101,6 +114,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_ulong_t);
#define __HYPERVISOR_kexec_op 37
#define __HYPERVISOR_tmem_op 38
#define __HYPERVISOR_xc_reserved_op 39 /* reserved for XenClient */
+#define __HYPERVISOR_xenpmu_op 40
/* Architecture-specific hypercall definitions. */
#define __HYPERVISOR_arch_0 48
@@ -160,6 +174,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_ulong_t);
#define VIRQ_MEM_EVENT 10 /* G. (DOM0) A memory event has occured */
#define VIRQ_XC_RESERVED 11 /* G. Reserved for XenClient */
#define VIRQ_ENOMEM 12 /* G. (DOM0) Low on heap memory */
+#define VIRQ_XENPMU 13 /* V. PMC interrupt */
/* Architecture-specific VIRQ definitions. */
#define VIRQ_ARCH_0 16
@@ -449,13 +464,13 @@ DEFINE_XEN_GUEST_HANDLE(mmuext_op_t);
/* When specifying UVMF_MULTI, also OR in a pointer to a CPU bitmap. */
/* UVMF_LOCAL is merely UVMF_MULTI with a NULL bitmap pointer. */
/* ` enum uvm_flags { */
-#define UVMF_NONE (0UL<<0) /* No flushing at all. */
-#define UVMF_TLB_FLUSH (1UL<<0) /* Flush entire TLB(s). */
-#define UVMF_INVLPG (2UL<<0) /* Flush only one entry. */
-#define UVMF_FLUSHTYPE_MASK (3UL<<0)
-#define UVMF_MULTI (0UL<<2) /* Flush subset of TLBs. */
-#define UVMF_LOCAL (0UL<<2) /* Flush local TLB. */
-#define UVMF_ALL (1UL<<2) /* Flush all TLBs. */
+#define UVMF_NONE (xen_mk_ulong(0)<<0) /* No flushing at all. */
+#define UVMF_TLB_FLUSH (xen_mk_ulong(1)<<0) /* Flush entire TLB(s). */
+#define UVMF_INVLPG (xen_mk_ulong(2)<<0) /* Flush only one entry. */
+#define UVMF_FLUSHTYPE_MASK (xen_mk_ulong(3)<<0)
+#define UVMF_MULTI (xen_mk_ulong(0)<<2) /* Flush subset of TLBs. */
+#define UVMF_LOCAL (xen_mk_ulong(0)<<2) /* Flush local TLB. */
+#define UVMF_ALL (xen_mk_ulong(1)<<2) /* Flush all TLBs. */
/* ` } */
/*
@@ -486,17 +501,27 @@ DEFINE_XEN_GUEST_HANDLE(mmuext_op_t);
/* x86/PAE guests: support PDPTs above 4GB. */
#define VMASST_TYPE_pae_extended_cr3 3
+/*
+ * x86/64 guests: strictly hide M2P from user mode.
+ * This allows the guest to control respective hypervisor behavior:
+ * - when not set, L4 tables get created with the respective slot blank,
+ * and whenever the L4 table gets used as a kernel one the missing
+ * mapping gets inserted,
+ * - when set, L4 tables get created with the respective slot initialized
+ * as before, and whenever the L4 table gets used as a user one the
+ * mapping gets zapped.
+ */
+#define VMASST_TYPE_m2p_strict 32
+
+#if __XEN_INTERFACE_VERSION__ < 0x00040600
#define MAX_VMASST_TYPE 3
-
-#ifndef __ASSEMBLY__
-
-typedef uint16_t domid_t;
+#endif
/* Domain ids >= DOMID_FIRST_RESERVED cannot be used for ordinary domains. */
-#define DOMID_FIRST_RESERVED (0x7FF0U)
+#define DOMID_FIRST_RESERVED xen_mk_uint(0x7FF0)
/* DOMID_SELF is used in certain contexts to refer to oneself. */
-#define DOMID_SELF (0x7FF0U)
+#define DOMID_SELF xen_mk_uint(0x7FF0)
/*
* DOMID_IO is used to restrict page-table updates to mapping I/O memory.
@@ -507,7 +532,7 @@ typedef uint16_t domid_t;
* This only makes sense in MMUEXT_SET_FOREIGNDOM, but in that context can
* be specified by any calling domain.
*/
-#define DOMID_IO (0x7FF1U)
+#define DOMID_IO xen_mk_uint(0x7FF1)
/*
* DOMID_XEN is used to allow privileged domains to map restricted parts of
@@ -515,17 +540,21 @@ typedef uint16_t domid_t;
* This only makes sense in MMUEXT_SET_FOREIGNDOM, and is only permitted if
* the caller is privileged.
*/
-#define DOMID_XEN (0x7FF2U)
+#define DOMID_XEN xen_mk_uint(0x7FF2)
/*
* DOMID_COW is used as the owner of sharable pages */
-#define DOMID_COW (0x7FF3U)
+#define DOMID_COW xen_mk_uint(0x7FF3)
/* DOMID_INVALID is used to identify pages with unknown owner. */
-#define DOMID_INVALID (0x7FF4U)
+#define DOMID_INVALID xen_mk_uint(0x7FF4)
/* Idle domain. */
-#define DOMID_IDLE (0x7FFFU)
+#define DOMID_IDLE xen_mk_uint(0x7FFF)
+
+#ifndef __ASSEMBLY__
+
+typedef uint16_t domid_t;
/*
* Send an array of these to HYPERVISOR_mmu_update().
@@ -682,6 +711,12 @@ struct shared_info {
uint32_t wc_version; /* Version counter: see vcpu_time_info_t. */
uint32_t wc_sec; /* Secs 00:00:00 UTC, Jan 1, 1970. */
uint32_t wc_nsec; /* Nsecs 00:00:00 UTC, Jan 1, 1970. */
+#if !defined(__i386__)
+ uint32_t wc_sec_hi;
+# define xen_wc_sec_hi wc_sec_hi
+#elif !defined(__XEN__) && !defined(__XEN_TOOLS__)
+# define xen_wc_sec_hi arch.wc_sec_hi
+#endif
struct arch_shared_info arch;
@@ -698,24 +733,27 @@ typedef struct shared_info shared_info_t;
* 3. This the order of bootstrap elements in the initial virtual region:
* a. relocated kernel image
* b. initial ram disk [mod_start, mod_len]
+ * (may be omitted)
* c. list of allocated page frames [mfn_list, nr_pages]
* (unless relocated due to XEN_ELFNOTE_INIT_P2M)
* d. start_info_t structure [register ESI (x86)]
- * e. bootstrap page tables [pt_base and CR3 (x86)]
- * f. bootstrap stack [register ESP (x86)]
+ * in case of dom0 this page contains the console info, too
+ * e. unless dom0: xenstore ring page
+ * f. unless dom0: console ring page
+ * g. bootstrap page tables [pt_base and CR3 (x86)]
+ * h. bootstrap stack [register ESP (x86)]
* 4. Bootstrap elements are packed together, but each is 4kB-aligned.
- * 5. The initial ram disk may be omitted.
- * 6. The list of page frames forms a contiguous 'pseudo-physical' memory
+ * 5. The list of page frames forms a contiguous 'pseudo-physical' memory
* layout for the domain. In particular, the bootstrap virtual-memory
* region is a 1:1 mapping to the first section of the pseudo-physical map.
- * 7. All bootstrap elements are mapped read-writable for the guest OS. The
+ * 6. All bootstrap elements are mapped read-writable for the guest OS. The
* only exception is the bootstrap page table, which is mapped read-only.
- * 8. There is guaranteed to be at least 512kB padding after the final
+ * 7. There is guaranteed to be at least 512kB padding after the final
* bootstrap element. If necessary, the bootstrap virtual region is
* extended by an extra 4MB to ensure this.
*
* Note: Prior to 25833:bb85bbccb1c9. ("x86/32-on-64 adjust Dom0 initial page
- * table layout") a bug caused the pt_base (3.e above) and cr3 to not point
+ * table layout") a bug caused the pt_base (3.g above) and cr3 to not point
* to the start of the guest page tables (it was offset by two pages).
* This only manifested itself on 32-on-64 dom0 kernels and not 32-on-64 domU
* or 64-bit kernels of any colour. The page tables for a 32-on-64 dom0 got
@@ -759,6 +797,29 @@ struct start_info {
};
typedef struct start_info start_info_t;
+/*
+ * Start of day structure passed to PVH guests in %ebx.
+ *
+ * NOTE: nothing will be loaded at physical address 0, so
+ * a 0 value in any of the address fields should be treated
+ * as not present.
+ */
+struct hvm_start_info {
+#define HVM_START_MAGIC_VALUE 0x336ec578
+ uint32_t magic; /* Contains the magic value 0x336ec578 */
+ /* ("xEn3" with the 0x80 bit of the "E" set).*/
+ uint32_t flags; /* SIF_xxx flags. */
+ uint32_t cmdline_paddr; /* Physical address of the command line. */
+ uint32_t nr_modules; /* Number of modules passed to the kernel. */
+ uint32_t modlist_paddr; /* Physical address of an array of */
+ /* hvm_modlist_entry. */
+};
+
+struct hvm_modlist_entry {
+ uint32_t paddr; /* Physical address of the module. */
+ uint32_t size; /* Size of the module in bytes. */
+};
+
/* New console union for dom0 introduced in 0x00030203. */
#if __XEN_INTERFACE_VERSION__ < 0x00030203
#define console_mfn console.domU.mfn
@@ -771,6 +832,8 @@ typedef struct start_info start_info_t;
#define SIF_INITDOMAIN (1<<1) /* Is this the initial control domain? */
#define SIF_MULTIBOOT_MOD (1<<2) /* Is mod_start a multiboot module? */
#define SIF_MOD_START_PFN (1<<3) /* Is mod_start a PFN? */
+#define SIF_VIRT_P2M_4TOOLS (1<<4) /* Do Xen tools understand a virt. mapped */
+ /* P->M making the 3 level tree obsolete? */
#define SIF_PM_MASK (0xFF<<8) /* reserve 1 byte for xen-pm options */
/*
@@ -851,25 +914,19 @@ typedef struct dom0_vga_console_info {
typedef uint8_t xen_domain_handle_t[16];
-/* Turn a plain number into a C unsigned long constant. */
-#define __mk_unsigned_long(x) x ## UL
-#define mk_unsigned_long(x) __mk_unsigned_long(x)
-
__DEFINE_XEN_GUEST_HANDLE(uint8, uint8_t);
__DEFINE_XEN_GUEST_HANDLE(uint16, uint16_t);
__DEFINE_XEN_GUEST_HANDLE(uint32, uint32_t);
__DEFINE_XEN_GUEST_HANDLE(uint64, uint64_t);
-#else /* __ASSEMBLY__ */
-
-/* In assembly code we cannot use C numeric constant suffixes. */
-#define mk_unsigned_long(x) x
-
#endif /* !__ASSEMBLY__ */
/* Default definitions for macros used by domctl/sysctl. */
#if defined(__XEN__) || defined(__XEN_TOOLS__)
+#ifndef int64_aligned_t
+#define int64_aligned_t int64_t
+#endif
#ifndef uint64_aligned_t
#define uint64_aligned_t uint64_t
#endif
--
2.6.2
^ permalink raw reply related [flat|nested] 33+ messages in thread* [PATCH v4 06/11] xen: factor out p2m list allocation into separate function
[not found] <1456120999-5639-1-git-send-email-jgross@suse.com>
` (4 preceding siblings ...)
2016-02-22 6:03 ` [PATCH v4 05/11] xen: synchronize xen header Juergen Gross
@ 2016-02-22 6:03 ` Juergen Gross
2016-02-22 6:03 ` [PATCH v4 07/11] xen: factor out allocation of special pages " Juergen Gross
` (7 subsequent siblings)
13 siblings, 0 replies; 33+ messages in thread
From: Juergen Gross @ 2016-02-22 6:03 UTC (permalink / raw)
To: grub-devel; +Cc: Juergen Gross, phcoder, daniel.kiper, mchang, xen-devel
Do the p2m list allocation of the to be loaded kernel in a separate
function. This will allow doing the p2m list allocation at different
times of the boot preparations depending on the features the kernel
is supporting.
While at this remove superfluous setting of first_p2m_pfn and
nr_p2m_frames as those are needed only in case of the p2m list not
being mapped by the initial kernel mapping.
Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
---
grub-core/loader/i386/xen.c | 87 ++++++++++++++++++++++++++-------------------
1 file changed, 50 insertions(+), 37 deletions(-)
diff --git a/grub-core/loader/i386/xen.c b/grub-core/loader/i386/xen.c
index 691c22e..a2fd1f6 100644
--- a/grub-core/loader/i386/xen.c
+++ b/grub-core/loader/i386/xen.c
@@ -44,8 +44,10 @@ GRUB_MOD_LICENSE ("GPLv3+");
struct xen_loader_state {
struct grub_relocator *relocator;
+ struct grub_relocator_xen_state state;
struct start_info next_start;
struct grub_xen_file_info xen_inf;
+ grub_xen_mfn_t *virt_mfn_list;
grub_uint64_t max_addr;
struct xen_multiboot_mod_list *module_info_page;
grub_uint64_t modules_target_start;
@@ -170,7 +172,7 @@ generate_page_table (grub_uint64_t *where, grub_uint64_t paging_start,
}
static grub_err_t
-set_mfns (grub_xen_mfn_t * new_mfn_list, grub_xen_mfn_t pfn)
+set_mfns (grub_xen_mfn_t pfn)
{
grub_xen_mfn_t i, t;
grub_xen_mfn_t cn_pfn = -1, st_pfn = -1;
@@ -179,32 +181,34 @@ set_mfns (grub_xen_mfn_t * new_mfn_list, grub_xen_mfn_t pfn)
for (i = 0; i < grub_xen_start_page_addr->nr_pages; i++)
{
- if (new_mfn_list[i] == grub_xen_start_page_addr->console.domU.mfn)
+ if (xen_state.virt_mfn_list[i] ==
+ grub_xen_start_page_addr->console.domU.mfn)
cn_pfn = i;
- if (new_mfn_list[i] == grub_xen_start_page_addr->store_mfn)
+ if (xen_state.virt_mfn_list[i] == grub_xen_start_page_addr->store_mfn)
st_pfn = i;
}
if (cn_pfn == (grub_xen_mfn_t)-1)
return grub_error (GRUB_ERR_BUG, "no console");
if (st_pfn == (grub_xen_mfn_t)-1)
return grub_error (GRUB_ERR_BUG, "no store");
- t = new_mfn_list[pfn];
- new_mfn_list[pfn] = new_mfn_list[cn_pfn];
- new_mfn_list[cn_pfn] = t;
- t = new_mfn_list[pfn + 1];
- new_mfn_list[pfn + 1] = new_mfn_list[st_pfn];
- new_mfn_list[st_pfn] = t;
+ t = xen_state.virt_mfn_list[pfn];
+ xen_state.virt_mfn_list[pfn] = xen_state.virt_mfn_list[cn_pfn];
+ xen_state.virt_mfn_list[cn_pfn] = t;
+ t = xen_state.virt_mfn_list[pfn + 1];
+ xen_state.virt_mfn_list[pfn + 1] = xen_state.virt_mfn_list[st_pfn];
+ xen_state.virt_mfn_list[st_pfn] = t;
- m2p_updates[0].ptr = page2offset (new_mfn_list[pfn]) | MMU_MACHPHYS_UPDATE;
+ m2p_updates[0].ptr =
+ page2offset (xen_state.virt_mfn_list[pfn]) | MMU_MACHPHYS_UPDATE;
m2p_updates[0].val = pfn;
m2p_updates[1].ptr =
- page2offset (new_mfn_list[pfn + 1]) | MMU_MACHPHYS_UPDATE;
+ page2offset (xen_state.virt_mfn_list[pfn + 1]) | MMU_MACHPHYS_UPDATE;
m2p_updates[1].val = pfn + 1;
m2p_updates[2].ptr =
- page2offset (new_mfn_list[cn_pfn]) | MMU_MACHPHYS_UPDATE;
+ page2offset (xen_state.virt_mfn_list[cn_pfn]) | MMU_MACHPHYS_UPDATE;
m2p_updates[2].val = cn_pfn;
m2p_updates[3].ptr =
- page2offset (new_mfn_list[st_pfn]) | MMU_MACHPHYS_UPDATE;
+ page2offset (xen_state.virt_mfn_list[st_pfn]) | MMU_MACHPHYS_UPDATE;
m2p_updates[3].val = st_pfn;
grub_xen_mmu_update (m2p_updates, 4, NULL, DOMID_SELF);
@@ -213,43 +217,52 @@ set_mfns (grub_xen_mfn_t * new_mfn_list, grub_xen_mfn_t pfn)
}
static grub_err_t
+grub_xen_p2m_alloc (void)
+{
+ grub_relocator_chunk_t ch;
+ grub_size_t p2msize;
+ grub_err_t err;
+
+ xen_state.state.mfn_list = xen_state.max_addr;
+ xen_state.next_start.mfn_list =
+ xen_state.max_addr + xen_state.xen_inf.virt_base;
+ p2msize = sizeof (grub_xen_mfn_t) * grub_xen_start_page_addr->nr_pages;
+ err = grub_relocator_alloc_chunk_addr (xen_state.relocator, &ch,
+ xen_state.max_addr, p2msize);
+ if (err)
+ return err;
+ xen_state.virt_mfn_list = get_virtual_current_address (ch);
+ grub_memcpy (xen_state.virt_mfn_list,
+ (void *) grub_xen_start_page_addr->mfn_list, p2msize);
+ xen_state.max_addr = ALIGN_UP (xen_state.max_addr + p2msize, PAGE_SIZE);
+
+ return GRUB_ERR_NONE;
+}
+
+static grub_err_t
grub_xen_boot (void)
{
- struct grub_relocator_xen_state state;
grub_relocator_chunk_t ch;
grub_err_t err;
- grub_size_t pgtsize;
struct start_info *nst;
grub_uint64_t nr_info_pages;
grub_uint64_t nr_pages, nr_pt_pages, nr_need_pages;
struct gnttab_set_version gnttab_setver;
- grub_xen_mfn_t *new_mfn_list;
grub_size_t i;
if (grub_xen_n_allocated_shared_pages)
return grub_error (GRUB_ERR_BUG, "active grants");
- 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 (xen_state.relocator, &ch,
- xen_state.max_addr, pgtsize);
- xen_state.next_start.nr_p2m_frames = (pgtsize + PAGE_SIZE - 1) >> PAGE_SHIFT;
+ err = grub_xen_p2m_alloc ();
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);
- xen_state.max_addr = ALIGN_UP (xen_state.max_addr + pgtsize, PAGE_SIZE);
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 = xen_state.max_addr + xen_state.xen_inf.virt_base;
+ xen_state.state.start_info = xen_state.max_addr + xen_state.xen_inf.virt_base;
nst = get_virtual_current_address (ch);
xen_state.max_addr =
ALIGN_UP (xen_state.max_addr + sizeof (xen_state.next_start), PAGE_SIZE);
@@ -262,14 +275,14 @@ grub_xen_boot (void)
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, xen_state.max_addr >> PAGE_SHIFT);
+ err = set_mfns (xen_state.max_addr >> PAGE_SHIFT);
if (err)
return err;
xen_state.max_addr += 2 * PAGE_SIZE;
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;
+ xen_state.state.paging_start = xen_state.max_addr >> PAGE_SHIFT;
nr_info_pages = xen_state.max_addr >> PAGE_SHIFT;
nr_pages = nr_info_pages;
@@ -298,15 +311,15 @@ grub_xen_boot (void)
generate_page_table (get_virtual_current_address (ch),
xen_state.max_addr >> PAGE_SHIFT, nr_pages,
- xen_state.xen_inf.virt_base, new_mfn_list);
+ xen_state.xen_inf.virt_base, xen_state.virt_mfn_list);
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;
+ xen_state.state.stack =
+ xen_state.max_addr + STACK_SIZE + xen_state.xen_inf.virt_base;
+ xen_state.state.entry_point = xen_state.xen_inf.entry_point;
- 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;
+ xen_state.state.paging_size = nr_pt_pages;
*nst = xen_state.next_start;
@@ -318,7 +331,7 @@ 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 (xen_state.relocator, state, nr_pages,
+ return grub_relocator_xen_boot (xen_state.relocator, xen_state.state, nr_pages,
xen_state.xen_inf.virt_base <
PAGE_SIZE ? page2offset (nr_pages) : 0,
nr_pages - 1,
--
2.6.2
^ permalink raw reply related [flat|nested] 33+ messages in thread* [PATCH v4 07/11] xen: factor out allocation of special pages into separate function
[not found] <1456120999-5639-1-git-send-email-jgross@suse.com>
` (5 preceding siblings ...)
2016-02-22 6:03 ` [PATCH v4 06/11] xen: factor out p2m list allocation into separate function Juergen Gross
@ 2016-02-22 6:03 ` Juergen Gross
2016-02-22 6:03 ` [PATCH v4 08/11] xen: factor out allocation of page tables " Juergen Gross
` (6 subsequent siblings)
13 siblings, 0 replies; 33+ messages in thread
From: Juergen Gross @ 2016-02-22 6:03 UTC (permalink / raw)
To: grub-devel; +Cc: Juergen Gross, phcoder, daniel.kiper, mchang, xen-devel
Do the allocation of special pages (start info, console and xenbus
ring buffers) in a separate function. This will allow to do the
allocation at different times of the boot preparations depending on
the features the kernel is supporting.
Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
---
grub-core/loader/i386/xen.c | 48 +++++++++++++++++++++++++++++----------------
1 file changed, 31 insertions(+), 17 deletions(-)
diff --git a/grub-core/loader/i386/xen.c b/grub-core/loader/i386/xen.c
index a2fd1f6..a80c0f8 100644
--- a/grub-core/loader/i386/xen.c
+++ b/grub-core/loader/i386/xen.c
@@ -48,6 +48,8 @@ struct xen_loader_state {
struct start_info next_start;
struct grub_xen_file_info xen_inf;
grub_xen_mfn_t *virt_mfn_list;
+ struct start_info *virt_start_info;
+ grub_xen_mfn_t console_pfn;
grub_uint64_t max_addr;
struct xen_multiboot_mod_list *module_info_page;
grub_uint64_t modules_target_start;
@@ -240,22 +242,10 @@ grub_xen_p2m_alloc (void)
}
static grub_err_t
-grub_xen_boot (void)
+grub_xen_special_alloc (void)
{
grub_relocator_chunk_t ch;
grub_err_t err;
- struct start_info *nst;
- grub_uint64_t nr_info_pages;
- grub_uint64_t nr_pages, nr_pt_pages, nr_need_pages;
- struct gnttab_set_version gnttab_setver;
- grub_size_t i;
-
- if (grub_xen_n_allocated_shared_pages)
- return grub_error (GRUB_ERR_BUG, "active grants");
-
- err = grub_xen_p2m_alloc ();
- if (err)
- return err;
err = grub_relocator_alloc_chunk_addr (xen_state.relocator, &ch,
xen_state.max_addr,
@@ -263,9 +253,11 @@ grub_xen_boot (void)
if (err)
return err;
xen_state.state.start_info = xen_state.max_addr + xen_state.xen_inf.virt_base;
- nst = get_virtual_current_address (ch);
+ xen_state.virt_start_info = get_virtual_current_address (ch);
xen_state.max_addr =
ALIGN_UP (xen_state.max_addr + sizeof (xen_state.next_start), PAGE_SIZE);
+ xen_state.console_pfn = xen_state.max_addr >> PAGE_SHIFT;
+ xen_state.max_addr += 2 * PAGE_SIZE;
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,
@@ -275,10 +267,28 @@ grub_xen_boot (void)
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 (xen_state.max_addr >> PAGE_SHIFT);
+ return GRUB_ERR_NONE;
+}
+
+static grub_err_t
+grub_xen_boot (void)
+{
+ grub_relocator_chunk_t ch;
+ grub_err_t err;
+ grub_uint64_t nr_info_pages;
+ grub_uint64_t nr_pages, nr_pt_pages, nr_need_pages;
+ struct gnttab_set_version gnttab_setver;
+ grub_size_t i;
+
+ if (grub_xen_n_allocated_shared_pages)
+ return grub_error (GRUB_ERR_BUG, "active grants");
+
+ err = grub_xen_p2m_alloc ();
+ if (err)
+ return err;
+ err = grub_xen_special_alloc ();
if (err)
return err;
- xen_state.max_addr += 2 * PAGE_SIZE;
xen_state.next_start.pt_base =
xen_state.max_addr + xen_state.xen_inf.virt_base;
@@ -309,6 +319,10 @@ grub_xen_boot (void)
if (err)
return err;
+ err = set_mfns (xen_state.console_pfn);
+ if (err)
+ return err;
+
generate_page_table (get_virtual_current_address (ch),
xen_state.max_addr >> PAGE_SHIFT, nr_pages,
xen_state.xen_inf.virt_base, xen_state.virt_mfn_list);
@@ -321,7 +335,7 @@ grub_xen_boot (void)
xen_state.next_start.nr_pt_frames = nr_pt_pages;
xen_state.state.paging_size = nr_pt_pages;
- *nst = xen_state.next_start;
+ *xen_state.virt_start_info = xen_state.next_start;
grub_memset (&gnttab_setver, 0, sizeof (gnttab_setver));
--
2.6.2
^ permalink raw reply related [flat|nested] 33+ messages in thread* [PATCH v4 08/11] xen: factor out allocation of page tables into separate function
[not found] <1456120999-5639-1-git-send-email-jgross@suse.com>
` (6 preceding siblings ...)
2016-02-22 6:03 ` [PATCH v4 07/11] xen: factor out allocation of special pages " Juergen Gross
@ 2016-02-22 6:03 ` Juergen Gross
2016-02-22 6:03 ` [PATCH v4 09/11] xen: add capability to load initrd outside of initial mapping Juergen Gross
` (5 subsequent siblings)
13 siblings, 0 replies; 33+ messages in thread
From: Juergen Gross @ 2016-02-22 6:03 UTC (permalink / raw)
To: grub-devel; +Cc: Juergen Gross, phcoder, daniel.kiper, mchang, xen-devel
Do the allocation of page tables in a separate function. This will
allow to do the allocation at different times of the boot preparations
depending on the features the kernel is supporting.
Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
---
grub-core/loader/i386/xen.c | 91 ++++++++++++++++++++++++++++-----------------
1 file changed, 56 insertions(+), 35 deletions(-)
diff --git a/grub-core/loader/i386/xen.c b/grub-core/loader/i386/xen.c
index a80c0f8..2e12763 100644
--- a/grub-core/loader/i386/xen.c
+++ b/grub-core/loader/i386/xen.c
@@ -51,6 +51,9 @@ struct xen_loader_state {
struct start_info *virt_start_info;
grub_xen_mfn_t console_pfn;
grub_uint64_t max_addr;
+ grub_uint64_t *virt_pgtable;
+ grub_uint64_t pgtbl_start;
+ grub_uint64_t pgtbl_end;
struct xen_multiboot_mod_list *module_info_page;
grub_uint64_t modules_target_start;
grub_size_t n_modules;
@@ -110,17 +113,17 @@ get_pgtable_size (grub_uint64_t total_pages, grub_uint64_t virt_base)
static void
generate_page_table (grub_uint64_t *where, grub_uint64_t paging_start,
- grub_uint64_t total_pages, grub_uint64_t virt_base,
- grub_xen_mfn_t *mfn_list)
+ grub_uint64_t paging_end, grub_uint64_t total_pages,
+ grub_uint64_t virt_base, grub_xen_mfn_t *mfn_list)
{
if (!virt_base)
- total_pages++;
+ paging_end++;
grub_uint64_t lx[NUMBER_OF_LEVELS], lxs[NUMBER_OF_LEVELS];
grub_uint64_t nlx, nls, sz = 0;
int l;
- nlx = total_pages;
+ nlx = paging_end;
nls = virt_base >> PAGE_SHIFT;
for (l = 0; l < NUMBER_OF_LEVELS; l++)
{
@@ -164,7 +167,7 @@ generate_page_table (grub_uint64_t *where, grub_uint64_t paging_start,
if (pr)
pg += POINTERS_PER_PAGE;
- for (j = 0; j < total_pages; j++)
+ for (j = 0; j < paging_end; j++)
{
if (j >= paging_start && j < lp)
pg[j + lxs[0]] = page2offset (mfn_list[j]) | 5;
@@ -271,24 +274,12 @@ grub_xen_special_alloc (void)
}
static grub_err_t
-grub_xen_boot (void)
+grub_xen_pt_alloc (void)
{
grub_relocator_chunk_t ch;
grub_err_t err;
grub_uint64_t nr_info_pages;
grub_uint64_t nr_pages, nr_pt_pages, nr_need_pages;
- struct gnttab_set_version gnttab_setver;
- grub_size_t i;
-
- if (grub_xen_n_allocated_shared_pages)
- return grub_error (GRUB_ERR_BUG, "active grants");
-
- err = grub_xen_p2m_alloc ();
- if (err)
- return err;
- err = grub_xen_special_alloc ();
- if (err)
- return err;
xen_state.next_start.pt_base =
xen_state.max_addr + xen_state.xen_inf.virt_base;
@@ -309,31 +300,61 @@ grub_xen_boot (void)
nr_pages = nr_need_pages;
}
- grub_dprintf ("xen", "bootstrap domain %llx+%llx\n",
- (unsigned long long) xen_state.xen_inf.virt_base,
- (unsigned long long) page2offset (nr_pages));
-
err = grub_relocator_alloc_chunk_addr (xen_state.relocator, &ch,
xen_state.max_addr,
page2offset (nr_pt_pages));
if (err)
return err;
- err = set_mfns (xen_state.console_pfn);
- if (err)
- return err;
-
- generate_page_table (get_virtual_current_address (ch),
- xen_state.max_addr >> PAGE_SHIFT, nr_pages,
- xen_state.xen_inf.virt_base, xen_state.virt_mfn_list);
-
+ xen_state.virt_pgtable = get_virtual_current_address (ch);
+ xen_state.pgtbl_start = xen_state.max_addr >> PAGE_SHIFT;
xen_state.max_addr += page2offset (nr_pt_pages);
xen_state.state.stack =
xen_state.max_addr + STACK_SIZE + xen_state.xen_inf.virt_base;
- xen_state.state.entry_point = xen_state.xen_inf.entry_point;
-
- xen_state.next_start.nr_pt_frames = nr_pt_pages;
xen_state.state.paging_size = nr_pt_pages;
+ xen_state.next_start.nr_pt_frames = nr_pt_pages;
+ xen_state.max_addr = page2offset (nr_pages);
+ xen_state.pgtbl_end = nr_pages;
+
+ return GRUB_ERR_NONE;
+}
+
+static grub_err_t
+grub_xen_boot (void)
+{
+ grub_err_t err;
+ grub_uint64_t nr_pages;
+ struct gnttab_set_version gnttab_setver;
+ grub_size_t i;
+
+ if (grub_xen_n_allocated_shared_pages)
+ return grub_error (GRUB_ERR_BUG, "active grants");
+
+ err = grub_xen_p2m_alloc ();
+ if (err)
+ return err;
+ err = grub_xen_special_alloc ();
+ if (err)
+ return err;
+ err = grub_xen_pt_alloc ();
+ if (err)
+ return err;
+
+ err = set_mfns (xen_state.console_pfn);
+ if (err)
+ return err;
+
+ nr_pages = xen_state.max_addr >> PAGE_SHIFT;
+
+ grub_dprintf ("xen", "bootstrap domain %llx+%llx\n",
+ (unsigned long long) xen_state.xen_inf.virt_base,
+ (unsigned long long) page2offset (nr_pages));
+
+ generate_page_table (xen_state.virt_pgtable, xen_state.pgtbl_start,
+ xen_state.pgtbl_end, nr_pages,
+ xen_state.xen_inf.virt_base, xen_state.virt_mfn_list);
+
+ xen_state.state.entry_point = xen_state.xen_inf.entry_point;
*xen_state.virt_start_info = xen_state.next_start;
@@ -348,8 +369,8 @@ grub_xen_boot (void)
return grub_relocator_xen_boot (xen_state.relocator, xen_state.state, nr_pages,
xen_state.xen_inf.virt_base <
PAGE_SIZE ? page2offset (nr_pages) : 0,
- nr_pages - 1,
- page2offset (nr_pages - 1) +
+ xen_state.pgtbl_end - 1,
+ page2offset (xen_state.pgtbl_end - 1) +
xen_state.xen_inf.virt_base);
}
--
2.6.2
^ permalink raw reply related [flat|nested] 33+ messages in thread* [PATCH v4 09/11] xen: add capability to load initrd outside of initial mapping
[not found] <1456120999-5639-1-git-send-email-jgross@suse.com>
` (7 preceding siblings ...)
2016-02-22 6:03 ` [PATCH v4 08/11] xen: factor out allocation of page tables " Juergen Gross
@ 2016-02-22 6:03 ` Juergen Gross
2016-02-22 6:03 ` [PATCH v4 10/11] xen: modify page table construction Juergen Gross
` (4 subsequent siblings)
13 siblings, 0 replies; 33+ messages in thread
From: Juergen Gross @ 2016-02-22 6:03 UTC (permalink / raw)
To: grub-devel; +Cc: Juergen Gross, phcoder, daniel.kiper, mchang, xen-devel
Modern pvops linux kernels support an initrd not covered by the initial
mapping. This capability is flagged by an elf-note.
In case the elf-note is set by the kernel don't place the initrd into
the initial mapping. This will allow to load larger initrds and/or
support domains with larger memory, as the initial mapping is limited
to 2GB and it is containing the p2m list.
Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
---
V4: rename grub_xen_alloc_end() to grub_xen_alloc_final()
---
grub-core/loader/i386/xen.c | 61 ++++++++++++++++++++++++++++++--------
grub-core/loader/i386/xen_fileXX.c | 3 ++
include/grub/xen_file.h | 1 +
3 files changed, 52 insertions(+), 13 deletions(-)
diff --git a/grub-core/loader/i386/xen.c b/grub-core/loader/i386/xen.c
index 2e12763..22a94ae 100644
--- a/grub-core/loader/i386/xen.c
+++ b/grub-core/loader/i386/xen.c
@@ -58,6 +58,7 @@ struct xen_loader_state {
grub_uint64_t modules_target_start;
grub_size_t n_modules;
int loaded;
+ int alloc_end_called;
};
static struct xen_loader_state xen_state;
@@ -320,6 +321,28 @@ grub_xen_pt_alloc (void)
}
static grub_err_t
+grub_xen_alloc_final (void)
+{
+ grub_err_t err;
+
+ if (xen_state.alloc_end_called)
+ return GRUB_ERR_NONE;
+ xen_state.alloc_end_called = 1;
+
+ err = grub_xen_p2m_alloc ();
+ if (err)
+ return err;
+ err = grub_xen_special_alloc ();
+ if (err)
+ return err;
+ err = grub_xen_pt_alloc ();
+ if (err)
+ return err;
+
+ return GRUB_ERR_NONE;
+}
+
+static grub_err_t
grub_xen_boot (void)
{
grub_err_t err;
@@ -330,13 +353,7 @@ grub_xen_boot (void)
if (grub_xen_n_allocated_shared_pages)
return grub_error (GRUB_ERR_BUG, "active grants");
- err = grub_xen_p2m_alloc ();
- if (err)
- return err;
- err = grub_xen_special_alloc ();
- if (err)
- return err;
- err = grub_xen_pt_alloc ();
+ err = grub_xen_alloc_final ();
if (err)
return err;
@@ -610,6 +627,13 @@ grub_cmd_initrd (grub_command_t cmd __attribute__ ((unused)),
goto fail;
}
+ if (xen_state.xen_inf.unmapped_initrd)
+ {
+ err = grub_xen_alloc_final ();
+ if (err)
+ goto fail;
+ }
+
if (grub_initrd_init (argc, argv, &initrd_ctx))
goto fail;
@@ -627,14 +651,24 @@ grub_cmd_initrd (grub_command_t cmd __attribute__ ((unused)),
goto fail;
}
- xen_state.next_start.mod_start =
- xen_state.max_addr + xen_state.xen_inf.virt_base;
- xen_state.next_start.mod_len = size;
-
- xen_state.max_addr = ALIGN_UP (xen_state.max_addr + size, PAGE_SIZE);
+ if (xen_state.xen_inf.unmapped_initrd)
+ {
+ xen_state.next_start.flags |= SIF_MOD_START_PFN;
+ xen_state.next_start.mod_start = xen_state.max_addr >> PAGE_SHIFT;
+ xen_state.next_start.mod_len = size;
+ }
+ else
+ {
+ xen_state.next_start.mod_start =
+ xen_state.max_addr + xen_state.xen_inf.virt_base;
+ xen_state.next_start.mod_len = size;
+ }
grub_dprintf ("xen", "Initrd, addr=0x%x, size=0x%x\n",
- (unsigned) xen_state.next_start.mod_start, (unsigned) size);
+ (unsigned) (xen_state.max_addr + xen_state.xen_inf.virt_base),
+ (unsigned) size);
+
+ xen_state.max_addr = ALIGN_UP (xen_state.max_addr + size, PAGE_SIZE);
fail:
grub_initrd_close (&initrd_ctx);
@@ -686,6 +720,7 @@ grub_cmd_module (grub_command_t cmd __attribute__ ((unused)),
if (!xen_state.module_info_page)
{
+ xen_state.xen_inf.unmapped_initrd = 0;
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;
diff --git a/grub-core/loader/i386/xen_fileXX.c b/grub-core/loader/i386/xen_fileXX.c
index 1f7f71d..d68634d 100644
--- a/grub-core/loader/i386/xen_fileXX.c
+++ b/grub-core/loader/i386/xen_fileXX.c
@@ -259,6 +259,9 @@ parse_note (grub_elf_t elf, struct grub_xen_file_info *xi,
descsz == 2 ? 2 : 3) == 0)
xi->arch = GRUB_XEN_FILE_I386;
break;
+ case XEN_ELFNOTE_MOD_START_PFN:
+ xi->unmapped_initrd = !!grub_le_to_cpu32(*(grub_uint32_t *) desc);
+ break;
default:
grub_dprintf ("xen", "unknown note type %d\n", nh->n_type);
break;
diff --git a/include/grub/xen_file.h b/include/grub/xen_file.h
index 4b2ccba..ed749fa 100644
--- a/include/grub/xen_file.h
+++ b/include/grub/xen_file.h
@@ -36,6 +36,7 @@ struct grub_xen_file_info
int has_note;
int has_xen_guest;
int extended_cr3;
+ int unmapped_initrd;
enum
{
GRUB_XEN_FILE_I386 = 1,
--
2.6.2
^ permalink raw reply related [flat|nested] 33+ messages in thread* [PATCH v4 10/11] xen: modify page table construction
[not found] <1456120999-5639-1-git-send-email-jgross@suse.com>
` (8 preceding siblings ...)
2016-02-22 6:03 ` [PATCH v4 09/11] xen: add capability to load initrd outside of initial mapping Juergen Gross
@ 2016-02-22 6:03 ` Juergen Gross
2016-02-22 9:17 ` Daniel Kiper
[not found] ` <20160222091748.GO3482@olila.local.net-space.pl>
2016-02-22 6:03 ` [PATCH v4 11/11] xen: add capability to load p2m list outside of kernel mapping Juergen Gross
` (3 subsequent siblings)
13 siblings, 2 replies; 33+ messages in thread
From: Juergen Gross @ 2016-02-22 6:03 UTC (permalink / raw)
To: grub-devel; +Cc: Juergen Gross, phcoder, daniel.kiper, mchang, xen-devel
Modify the page table construction to allow multiple virtual regions
to be mapped. This is done as preparation for removing the p2m list
from the initial kernel mapping in order to support huge pv domains.
This allows a cleaner approach for mapping the relocator page by
using this capability.
The interface to the assembler level of the relocator has to be changed
in order to be able to process multiple page table areas.
Signed-off-by: Juergen Gross <jgross@suse.com>
---
V4: align variables in assembly sources
use separate structure define as requested by Daniel Kiper
V3: use constants instead of numbers as requested by Daniel Kiper
add lots of comments to assembly code as requested by Daniel Kiper
---
grub-core/lib/i386/xen/relocator.S | 88 ++++++----
grub-core/lib/x86_64/xen/relocator.S | 135 ++++++---------
grub-core/lib/xen/relocator.c | 28 ++-
grub-core/loader/i386/xen.c | 325 ++++++++++++++++++++++++-----------
include/grub/i386/memory.h | 7 +
include/grub/xen/relocator.h | 6 +-
6 files changed, 359 insertions(+), 230 deletions(-)
diff --git a/grub-core/lib/i386/xen/relocator.S b/grub-core/lib/i386/xen/relocator.S
index 694a54c..f8d135a 100644
--- a/grub-core/lib/i386/xen/relocator.S
+++ b/grub-core/lib/i386/xen/relocator.S
@@ -16,6 +16,8 @@
* along with GRUB. If not, see <http://www.gnu.org/licenses/>.
*/
+#include <grub/i386/memory.h>
+#include <grub/i386/types.h>
#include <grub/symbol.h>
#include <grub/xen.h>
@@ -23,78 +25,86 @@
VARIABLE(grub_relocator_xen_remap_start)
LOCAL(base):
- /* mov imm32, %ebx */
+ /* Remap the remapper to it's new address. */
+ /* mov imm32, %ebx - %ebx: new virtual address of remapper */
.byte 0xbb
VARIABLE(grub_relocator_xen_remapper_virt)
.long 0
- /* mov imm32, %ecx */
+ /* mov imm32, %ecx - %ecx: low part of page table entry */
.byte 0xb9
VARIABLE(grub_relocator_xen_remapper_map)
.long 0
- /* mov imm32, %edx */
+ /* mov imm32, %edx - %edx: high part of page table entry */
.byte 0xba
VARIABLE(grub_relocator_xen_remapper_map_high)
.long 0
- movl %ebx, %ebp
+ movl %ebx, %ebp /* %ebx is clobbered by hypercall */
- movl $2, %esi
+ movl $UVMF_INVLPG, %esi /* esi: flags (inv. single entry) */
movl $__HYPERVISOR_update_va_mapping, %eax
int $0x82
movl %ebp, %ebx
addl $(LOCAL(cont) - LOCAL(base)), %ebx
- jmp *%ebx
+ jmp *%ebx /* Continue with new virtual address */
LOCAL(cont):
- xorl %eax, %eax
- movl %eax, %ebp
+ /* Modify mappings of new page tables to be read-only. */
+ /* mov imm32, %eax */
+ .byte 0xb8
+VARIABLE(grub_relocator_xen_paging_areas_addr)
+ .long 0
+ movl %eax, %ebx
1:
+ movl 0(%ebx), %ebp /* Get start pfn of the current area */
+ movl GRUB_TARGET_SIZEOF_LONG(%ebx), %ecx /* Get # of pg tables */
+ testl %ecx, %ecx /* 0 -> last area reached */
+ jz 3f
+ addl $(2 * GRUB_TARGET_SIZEOF_LONG), %ebx
+ movl %ebx, %esp /* Save current area pointer */
+2:
+ movl %ecx, %edi
/* mov imm32, %eax */
.byte 0xb8
VARIABLE(grub_relocator_xen_mfn_list)
.long 0
- movl %eax, %edi
- movl %ebp, %eax
- movl 0(%edi, %eax, 4), %ecx
-
- /* mov imm32, %ebx */
- .byte 0xbb
-VARIABLE(grub_relocator_xen_paging_start)
- .long 0
- shll $12, %eax
- addl %eax, %ebx
+ movl 0(%eax, %ebp, 4), %ecx /* mfn */
+ movl %ebp, %ebx
+ shll $PAGE_SHIFT, %ebx /* virtual address (1:1 mapping) */
movl %ecx, %edx
- shll $12, %ecx
- shrl $20, %edx
- orl $5, %ecx
- movl $2, %esi
+ shll $PAGE_SHIFT, %ecx /* prepare pte low part */
+ shrl $(32 - PAGE_SHIFT), %edx /* pte high part */
+ orl $(GRUB_PAGE_PRESENT | GRUB_PAGE_USER), %ecx /* pte low */
+ movl $UVMF_INVLPG, %esi
movl $__HYPERVISOR_update_va_mapping, %eax
- int $0x82
+ int $0x82 /* parameters: eax, ebx, ecx, edx, esi */
- incl %ebp
- /* mov imm32, %ecx */
- .byte 0xb9
-VARIABLE(grub_relocator_xen_paging_size)
- .long 0
- cmpl %ebp, %ecx
+ incl %ebp /* next pfn */
+ movl %edi, %ecx
- ja 1b
+ loop 2b
+ mov %esp, %ebx /* restore area poniter */
+ jmp 1b
+
+3:
+ /* Switch page tables: pin new L3 pt, load cr3, unpin old L3. */
/* mov imm32, %ebx */
.byte 0xbb
VARIABLE(grub_relocator_xen_mmu_op_addr)
.long 0
- movl $3, %ecx
- movl $0, %edx
- movl $0x7FF0, %esi
+ movl $3, %ecx /* 3 mmu ops */
+ movl $0, %edx /* pdone (not used) */
+ movl $DOMID_SELF, %esi
movl $__HYPERVISOR_mmuext_op, %eax
int $0x82
+ /* Continue in virtual kernel mapping. */
/* mov imm32, %eax */
.byte 0xb8
VARIABLE(grub_relocator_xen_remap_continue)
@@ -102,6 +112,10 @@ VARIABLE(grub_relocator_xen_remap_continue)
jmp *%eax
+ .p2align 2
+VARIABLE(grub_relocator_xen_paging_areas)
+ .long 0, 0, 0, 0, 0, 0, 0, 0
+
VARIABLE(grub_relocator_xen_mmu_op)
.space 256
@@ -109,6 +123,7 @@ VARIABLE(grub_relocator_xen_remap_end)
VARIABLE(grub_relocator_xen_start)
+ /* Unmap old remapper area. */
/* mov imm32, %eax */
.byte 0xb8
VARIABLE(grub_relocator_xen_remapper_virt2)
@@ -116,14 +131,14 @@ VARIABLE(grub_relocator_xen_remapper_virt2)
movl %eax, %edi
- xorl %ecx, %ecx
+ xorl %ecx, %ecx /* Invalid pte */
xorl %edx, %edx
- movl $2, %esi
+ movl $UVMF_INVLPG, %esi
movl $__HYPERVISOR_update_va_mapping, %eax
int $0x82
-
+ /* Prepare registers for starting kernel. */
/* mov imm32, %eax */
.byte 0xb8
VARIABLE(grub_relocator_xen_stack)
@@ -145,6 +160,7 @@ VARIABLE(grub_relocator_xen_start_info)
VARIABLE(grub_relocator_xen_entry_point)
.long 0
+ /* Now start the new kernel. */
jmp *%eax
VARIABLE(grub_relocator_xen_end)
diff --git a/grub-core/lib/x86_64/xen/relocator.S b/grub-core/lib/x86_64/xen/relocator.S
index 92e9e72..38ae157 100644
--- a/grub-core/lib/x86_64/xen/relocator.S
+++ b/grub-core/lib/x86_64/xen/relocator.S
@@ -16,95 +16,86 @@
* along with GRUB. If not, see <http://www.gnu.org/licenses/>.
*/
+#include <grub/x86_64/memory.h>
+#include <grub/x86_64/types.h>
#include <grub/symbol.h>
#include <grub/xen.h>
+/* Macro to load an imm64 value stored by the C-part into %rax: */
+#define MOV_IMM64_RAX(var) .byte 0x48, 0xb8; VARIABLE(var); .quad 0
+
.p2align 4 /* force 16-byte alignment */
VARIABLE(grub_relocator_xen_remap_start)
LOCAL(base):
- /* mov imm64, %rax */
- .byte 0x48
- .byte 0xb8
-VARIABLE(grub_relocator_xen_remapper_virt)
- .quad 0
+ /* Remap the remapper to it's new address. */
+ MOV_IMM64_RAX(grub_relocator_xen_remapper_virt)
- movq %rax, %rdi
- movq %rax, %rbx
+ movq %rax, %rdi /* %rdi: new virtual address of remapper */
+ movq %rax, %rbx /* Remember new virtual address */
- /* mov imm64, %rax */
- .byte 0x48
- .byte 0xb8
-VARIABLE(grub_relocator_xen_remapper_map)
- .quad 0
+ MOV_IMM64_RAX(grub_relocator_xen_remapper_map)
- movq %rax, %rsi
+ movq %rax, %rsi /* %rsi: page table entry */
- movq $2, %rdx
+ movq $UVMF_INVLPG, %rdx /* %rdx: flags (inv. single entry) */
movq $__HYPERVISOR_update_va_mapping, %rax
- syscall
+ syscall /* Do the remap operation */
addq $(LOCAL(cont) - LOCAL(base)), %rbx
- jmp *%rbx
+ jmp *%rbx /* Continue with new virtual address */
LOCAL(cont):
-
- /* mov imm64, %rcx */
- .byte 0x48
- .byte 0xb9
-VARIABLE(grub_relocator_xen_paging_size)
- .quad 0
+ /* Modify mappings of new page tables to be read-only. */
+ MOV_IMM64_RAX(grub_relocator_xen_mfn_list)
- /* mov imm64, %rax */
- .byte 0x48
- .byte 0xb8
-VARIABLE(grub_relocator_xen_paging_start)
- .quad 0
+ movq %rax, %rbx /* %rbx is the base of the p2m list */
+ leaq EXT_C(grub_relocator_xen_paging_areas) (%rip), %r8
- movq %rax, %r12
-
- /* mov imm64, %rax */
- .byte 0x48
- .byte 0xb8
-VARIABLE(grub_relocator_xen_mfn_list)
- .quad 0
-
- movq %rax, %rsi
1:
+ movq 0(%r8), %r12 /* Get start pfn of the current area */
+ movq GRUB_TARGET_SIZEOF_LONG(%r8), %rcx /* Get # of pg tables */
+ testq %rcx, %rcx /* 0 -> last area reached */
+ jz 3f
+2:
movq %r12, %rdi
- movq %rsi, %rbx
- movq 0(%rsi), %rsi
- shlq $12, %rsi
- orq $5, %rsi
- movq $2, %rdx
- movq %rcx, %r9
+ shlq $PAGE_SHIFT, %rdi /* virtual address (1:1 mapping) */
+ movq (%rbx, %r12, 8), %rsi /* mfn */
+ shlq $PAGE_SHIFT, %rsi
+ orq $(GRUB_PAGE_PRESENT | GRUB_PAGE_USER), %rsi /* Build pte */
+ movq $UVMF_INVLPG, %rdx
+ movq %rcx, %r9 /* %rcx clobbered by hypercall */
movq $__HYPERVISOR_update_va_mapping, %rax
syscall
movq %r9, %rcx
- addq $8, %rbx
- addq $4096, %r12
- movq %rbx, %rsi
+ incq %r12 /* next pfn */
- loop 1b
+ loop 2b
- leaq LOCAL(mmu_op) (%rip), %rdi
- movq $3, %rsi
- movq $0, %rdx
- movq $0x7FF0, %r10
+ addq $(2 * GRUB_TARGET_SIZEOF_LONG), %r8 /* next pg table area */
+ jmp 1b
+
+3:
+ /* Switch page tables: pin new L4 pt, load cr3, unpin old L4. */
+ leaq EXT_C(grub_relocator_xen_mmu_op) (%rip), %rdi
+ movq $3, %rsi /* 3 mmu ops */
+ movq $0, %rdx /* pdone (not used) */
+ movq $DOMID_SELF, %r10
movq $__HYPERVISOR_mmuext_op, %rax
syscall
- /* mov imm64, %rax */
- .byte 0x48
- .byte 0xb8
-VARIABLE(grub_relocator_xen_remap_continue)
- .quad 0
+ /* Continue in virtual kernel mapping. */
+ MOV_IMM64_RAX(grub_relocator_xen_remap_continue)
jmp *%rax
-LOCAL(mmu_op):
+ .p2align 3
+VARIABLE(grub_relocator_xen_paging_areas)
+ /* array of start, size pairs, size 0 is end marker */
+ .quad 0, 0, 0, 0, 0, 0, 0, 0
+
VARIABLE(grub_relocator_xen_mmu_op)
.space 256
@@ -112,46 +103,32 @@ VARIABLE(grub_relocator_xen_remap_end)
VARIABLE(grub_relocator_xen_start)
- /* mov imm64, %rax */
- .byte 0x48
- .byte 0xb8
-VARIABLE(grub_relocator_xen_remapper_virt2)
- .quad 0
+ /* Unmap old remapper area. */
+ MOV_IMM64_RAX(grub_relocator_xen_remapper_virt2)
movq %rax, %rdi
- xorq %rax, %rax
+ xorq %rax, %rax /* Invalid pte */
movq %rax, %rsi
- movq $2, %rdx
+ movq $UVMF_INVLPG, %rdx
movq $__HYPERVISOR_update_va_mapping, %rax
syscall
-
- /* mov imm64, %rax */
- .byte 0x48
- .byte 0xb8
-VARIABLE(grub_relocator_xen_stack)
- .quad 0
+ /* Prepare registers for starting kernel. */
+ MOV_IMM64_RAX(grub_relocator_xen_stack)
movq %rax, %rsp
- /* mov imm64, %rax */
- .byte 0x48
- .byte 0xb8
-VARIABLE(grub_relocator_xen_start_info)
- .quad 0
+ MOV_IMM64_RAX(grub_relocator_xen_start_info)
movq %rax, %rsi
cld
- /* mov imm64, %rax */
- .byte 0x48
- .byte 0xb8
-VARIABLE(grub_relocator_xen_entry_point)
- .quad 0
+ MOV_IMM64_RAX(grub_relocator_xen_entry_point)
+ /* Now start the new kernel. */
jmp *%rax
VARIABLE(grub_relocator_xen_end)
diff --git a/grub-core/lib/xen/relocator.c b/grub-core/lib/xen/relocator.c
index 8f427d3..a05b253 100644
--- a/grub-core/lib/xen/relocator.c
+++ b/grub-core/lib/xen/relocator.c
@@ -29,6 +29,11 @@
typedef grub_addr_t grub_xen_reg_t;
+struct grub_relocator_xen_paging_area {
+ grub_xen_reg_t start;
+ grub_xen_reg_t size;
+};
+
extern grub_uint8_t grub_relocator_xen_start;
extern grub_uint8_t grub_relocator_xen_end;
extern grub_uint8_t grub_relocator_xen_remap_start;
@@ -36,15 +41,16 @@ extern grub_uint8_t grub_relocator_xen_remap_end;
extern grub_xen_reg_t grub_relocator_xen_stack;
extern grub_xen_reg_t grub_relocator_xen_start_info;
extern grub_xen_reg_t grub_relocator_xen_entry_point;
-extern grub_xen_reg_t grub_relocator_xen_paging_start;
-extern grub_xen_reg_t grub_relocator_xen_paging_size;
extern grub_xen_reg_t grub_relocator_xen_remapper_virt;
extern grub_xen_reg_t grub_relocator_xen_remapper_virt2;
extern grub_xen_reg_t grub_relocator_xen_remapper_map;
extern grub_xen_reg_t grub_relocator_xen_mfn_list;
+extern struct grub_relocator_xen_paging_area
+ grub_relocator_xen_paging_areas[XEN_MAX_MAPPINGS];
extern grub_xen_reg_t grub_relocator_xen_remap_continue;
#ifdef __i386__
extern grub_xen_reg_t grub_relocator_xen_mmu_op_addr;
+extern grub_xen_reg_t grub_relocator_xen_paging_areas_addr;
extern grub_xen_reg_t grub_relocator_xen_remapper_map_high;
#endif
extern mmuext_op_t grub_relocator_xen_mmu_op[3];
@@ -61,6 +67,7 @@ grub_relocator_xen_boot (struct grub_relocator *rel,
{
grub_err_t err;
void *relst;
+ int i;
grub_relocator_chunk_t ch, ch_tramp;
grub_xen_mfn_t *mfn_list =
(grub_xen_mfn_t *) grub_xen_start_page_addr->mfn_list;
@@ -77,8 +84,11 @@ grub_relocator_xen_boot (struct grub_relocator *rel,
grub_relocator_xen_stack = state.stack;
grub_relocator_xen_start_info = state.start_info;
grub_relocator_xen_entry_point = state.entry_point;
- grub_relocator_xen_paging_start = state.paging_start << 12;
- grub_relocator_xen_paging_size = state.paging_size;
+ for (i = 0; i < XEN_MAX_MAPPINGS; i++)
+ {
+ grub_relocator_xen_paging_areas[i].start = state.paging_start[i];
+ grub_relocator_xen_paging_areas[i].size = state.paging_size[i];
+ }
grub_relocator_xen_remapper_virt = remapper_virt;
grub_relocator_xen_remapper_virt2 = remapper_virt;
grub_relocator_xen_remap_continue = trampoline_virt;
@@ -88,10 +98,12 @@ grub_relocator_xen_boot (struct grub_relocator *rel,
grub_relocator_xen_remapper_map_high = (mfn_list[remapper_pfn] >> 20);
grub_relocator_xen_mmu_op_addr = (char *) &grub_relocator_xen_mmu_op
- (char *) &grub_relocator_xen_remap_start + remapper_virt;
+ grub_relocator_xen_paging_areas_addr =
+ (char *) &grub_relocator_xen_paging_areas
+ - (char *) &grub_relocator_xen_remap_start + remapper_virt;
#endif
- grub_relocator_xen_mfn_list = state.mfn_list
- + state.paging_start * sizeof (grub_addr_t);
+ grub_relocator_xen_mfn_list = state.mfn_list;
grub_memset (grub_relocator_xen_mmu_op, 0,
sizeof (grub_relocator_xen_mmu_op));
@@ -100,9 +112,9 @@ grub_relocator_xen_boot (struct grub_relocator *rel,
#else
grub_relocator_xen_mmu_op[0].cmd = MMUEXT_PIN_L4_TABLE;
#endif
- grub_relocator_xen_mmu_op[0].arg1.mfn = mfn_list[state.paging_start];
+ grub_relocator_xen_mmu_op[0].arg1.mfn = mfn_list[state.paging_start[0]];
grub_relocator_xen_mmu_op[1].cmd = MMUEXT_NEW_BASEPTR;
- grub_relocator_xen_mmu_op[1].arg1.mfn = mfn_list[state.paging_start];
+ grub_relocator_xen_mmu_op[1].arg1.mfn = mfn_list[state.paging_start[0]];
grub_relocator_xen_mmu_op[2].cmd = MMUEXT_UNPIN_TABLE;
grub_relocator_xen_mmu_op[2].arg1.mfn =
mfn_list[grub_xen_start_page_addr->pt_base >> 12];
diff --git a/grub-core/loader/i386/xen.c b/grub-core/loader/i386/xen.c
index 22a94ae..4eadc47 100644
--- a/grub-core/loader/i386/xen.c
+++ b/grub-core/loader/i386/xen.c
@@ -39,9 +39,34 @@
#include <grub/xen.h>
#include <grub/xen_file.h>
#include <grub/linux.h>
+#include <grub/i386/memory.h>
GRUB_MOD_LICENSE ("GPLv3+");
+#ifdef __x86_64__
+#define NUMBER_OF_LEVELS 4
+#define INTERMEDIATE_OR (GRUB_PAGE_PRESENT | GRUB_PAGE_RW | GRUB_PAGE_USER)
+#define VIRT_MASK 0x0000ffffffffffffULL
+#else
+#define NUMBER_OF_LEVELS 3
+#define INTERMEDIATE_OR (GRUB_PAGE_PRESENT | GRUB_PAGE_RW)
+#define VIRT_MASK 0x00000000ffffffffULL
+#define HYPERVISOR_PUD_ADDRESS 0xc0000000ULL
+#endif
+
+struct grub_xen_mapping_lvl {
+ grub_uint64_t virt_start;
+ grub_uint64_t virt_end;
+ grub_uint64_t pfn_start;
+ grub_uint64_t n_pt_pages;
+};
+
+struct grub_xen_mapping {
+ grub_uint64_t *where;
+ struct grub_xen_mapping_lvl area;
+ struct grub_xen_mapping_lvl lvls[NUMBER_OF_LEVELS];
+};
+
struct xen_loader_state {
struct grub_relocator *relocator;
struct grub_relocator_xen_state state;
@@ -57,6 +82,9 @@ struct xen_loader_state {
struct xen_multiboot_mod_list *module_info_page;
grub_uint64_t modules_target_start;
grub_size_t n_modules;
+ struct grub_xen_mapping *map_reloc;
+ struct grub_xen_mapping mappings[XEN_MAX_MAPPINGS];
+ int n_mappings;
int loaded;
int alloc_end_called;
};
@@ -65,9 +93,8 @@ static struct xen_loader_state xen_state;
static grub_dl_t my_mod;
-#define PAGE_SIZE 4096
+#define PAGE_SIZE (1UL << PAGE_SHIFT)
#define MAX_MODULES (PAGE_SIZE / sizeof (struct xen_multiboot_mod_list))
-#define PAGE_SHIFT 12
#define STACK_SIZE 1048576
#define ADDITIONAL_SIZE (1 << 19)
#define ALIGN_SIZE (1 << 22)
@@ -80,100 +107,163 @@ page2offset (grub_uint64_t page)
return page << PAGE_SHIFT;
}
-#ifdef __x86_64__
-#define NUMBER_OF_LEVELS 4
-#define INTERMEDIATE_OR 7
-#else
-#define NUMBER_OF_LEVELS 3
-#define INTERMEDIATE_OR 3
+static grub_err_t
+get_pgtable_size (grub_uint64_t from, grub_uint64_t to, grub_uint64_t pfn)
+{
+ struct grub_xen_mapping *map, *map_cmp;
+ grub_uint64_t mask, bits;
+ int i, m;
+
+ if (xen_state.n_mappings == XEN_MAX_MAPPINGS)
+ return grub_error (GRUB_ERR_BUG, "too many mapped areas");
+
+ grub_dprintf ("xen", "get_pgtable_size %d from=%llx, to=%llx, pfn=%llx\n",
+ xen_state.n_mappings, (unsigned long long) from,
+ (unsigned long long) to, (unsigned long long) pfn);
+
+ map = xen_state.mappings + xen_state.n_mappings;
+ grub_memset (map, 0, sizeof (*map));
+
+ map->area.virt_start = from & VIRT_MASK;
+ map->area.virt_end = (to - 1) & VIRT_MASK;
+ map->area.n_pt_pages = 0;
+
+ for (i = NUMBER_OF_LEVELS - 1; i >= 0; i--)
+ {
+ map->lvls[i].pfn_start = pfn + map->area.n_pt_pages;
+ if (i == NUMBER_OF_LEVELS - 1)
+ {
+ if (xen_state.n_mappings == 0)
+ {
+ map->lvls[i].virt_start = 0;
+ map->lvls[i].virt_end = VIRT_MASK;
+ map->lvls[i].n_pt_pages = 1;
+ map->area.n_pt_pages++;
+ }
+ continue;
+ }
+
+ bits = PAGE_SHIFT + (i + 1) * LOG_POINTERS_PER_PAGE;
+ mask = (1ULL << bits) - 1;
+ map->lvls[i].virt_start = map->area.virt_start & ~mask;
+ map->lvls[i].virt_end = map->area.virt_end | mask;
+#ifdef __i386__
+ /* PAE wants last root directory present. */
+ if (i == 1 && to <= HYPERVISOR_PUD_ADDRESS && xen_state.n_mappings == 0)
+ map->lvls[i].virt_end = VIRT_MASK;
#endif
+ for (m = 0; m < xen_state.n_mappings; m++)
+ {
+ map_cmp = xen_state.mappings + m;
+ if (map_cmp->lvls[i].virt_start == map_cmp->lvls[i].virt_end)
+ continue;
+ if (map->lvls[i].virt_start >= map_cmp->lvls[i].virt_start &&
+ map->lvls[i].virt_end <= map_cmp->lvls[i].virt_end)
+ {
+ map->lvls[i].virt_start = 0;
+ map->lvls[i].virt_end = 0;
+ break;
+ }
+ if (map->lvls[i].virt_start >= map_cmp->lvls[i].virt_start &&
+ map->lvls[i].virt_start <= map_cmp->lvls[i].virt_end)
+ map->lvls[i].virt_start = map_cmp->lvls[i].virt_end + 1;
+ if (map->lvls[i].virt_end >= map_cmp->lvls[i].virt_start &&
+ map->lvls[i].virt_end <= map_cmp->lvls[i].virt_end)
+ map->lvls[i].virt_end = map_cmp->lvls[i].virt_start - 1;
+ }
+ if (map->lvls[i].virt_start < map->lvls[i].virt_end)
+ map->lvls[i].n_pt_pages =
+ ((map->lvls[i].virt_end - map->lvls[i].virt_start) >> bits) + 1;
+ map->area.n_pt_pages += map->lvls[i].n_pt_pages;
+ grub_dprintf ("xen", "get_pgtable_size level %d: virt %llx-%llx %d pts\n",
+ i, (unsigned long long) map->lvls[i].virt_start,
+ (unsigned long long) map->lvls[i].virt_end,
+ (int) map->lvls[i].n_pt_pages);
+ }
+
+ grub_dprintf ("xen", "get_pgtable_size return: %d page tables\n",
+ (int) map->area.n_pt_pages);
+
+ xen_state.state.paging_start[xen_state.n_mappings] = pfn;
+ xen_state.state.paging_size[xen_state.n_mappings] = map->area.n_pt_pages;
+
+ return GRUB_ERR_NONE;
+}
+
+static grub_uint64_t *
+get_pg_table_virt (int mapping, int level)
+{
+ grub_uint64_t pfn;
+ struct grub_xen_mapping *map;
+
+ map = xen_state.mappings + mapping;
+ pfn = map->lvls[level].pfn_start - map->lvls[NUMBER_OF_LEVELS - 1].pfn_start;
+ return map->where + pfn * POINTERS_PER_PAGE;
+}
static grub_uint64_t
-get_pgtable_size (grub_uint64_t total_pages, grub_uint64_t virt_base)
+get_pg_table_prot (int level, grub_uint64_t pfn)
{
- if (!virt_base)
- total_pages++;
- grub_uint64_t ret = 0;
- grub_uint64_t ll = total_pages;
- int i;
- for (i = 0; i < NUMBER_OF_LEVELS; i++)
+ int m;
+ grub_uint64_t pfn_s, pfn_e;
+
+ if (level > 0)
+ return INTERMEDIATE_OR;
+ for (m = 0; m < xen_state.n_mappings; m++)
{
- ll = (ll + POINTERS_PER_PAGE - 1) >> LOG_POINTERS_PER_PAGE;
- /* PAE wants all 4 root directories present. */
-#ifdef __i386__
- if (i == 1)
- ll = 4;
-#endif
- ret += ll;
+ pfn_s = xen_state.mappings[m].lvls[NUMBER_OF_LEVELS - 1].pfn_start;
+ pfn_e = xen_state.mappings[m].area.n_pt_pages + pfn_s;
+ if (pfn >= pfn_s && pfn < pfn_e)
+ return GRUB_PAGE_PRESENT | GRUB_PAGE_USER;
}
- for (i = 1; i < NUMBER_OF_LEVELS; i++)
- if (virt_base >> (PAGE_SHIFT + i * LOG_POINTERS_PER_PAGE))
- ret++;
- return ret;
+ return GRUB_PAGE_PRESENT | GRUB_PAGE_RW | GRUB_PAGE_USER;
}
static void
-generate_page_table (grub_uint64_t *where, grub_uint64_t paging_start,
- grub_uint64_t paging_end, grub_uint64_t total_pages,
- grub_uint64_t virt_base, grub_xen_mfn_t *mfn_list)
+generate_page_table (grub_xen_mfn_t *mfn_list)
{
- if (!virt_base)
- paging_end++;
+ int l, m1, m2;
+ long p, p_s, p_e;
+ grub_uint64_t start, end, pfn;
+ grub_uint64_t *pg;
+ struct grub_xen_mapping_lvl *lvl;
- grub_uint64_t lx[NUMBER_OF_LEVELS], lxs[NUMBER_OF_LEVELS];
- grub_uint64_t nlx, nls, sz = 0;
- int l;
+ for (m1 = 0; m1 < xen_state.n_mappings; m1++)
+ grub_memset (xen_state.mappings[m1].where, 0,
+ xen_state.mappings[m1].area.n_pt_pages * PAGE_SIZE);
- nlx = paging_end;
- nls = virt_base >> PAGE_SHIFT;
- for (l = 0; l < NUMBER_OF_LEVELS; l++)
+ for (l = NUMBER_OF_LEVELS - 1; l >= 0; l--)
{
- nlx = (nlx + POINTERS_PER_PAGE - 1) >> LOG_POINTERS_PER_PAGE;
- /* PAE wants all 4 root directories present. */
-#ifdef __i386__
- if (l == 1)
- nlx = 4;
-#endif
- lx[l] = nlx;
- sz += lx[l];
- lxs[l] = nls & (POINTERS_PER_PAGE - 1);
- if (nls && l != 0)
- sz++;
- nls >>= LOG_POINTERS_PER_PAGE;
- }
-
- grub_uint64_t lp;
- grub_uint64_t j;
- grub_uint64_t *pg = (grub_uint64_t *) where;
- int pr = 0;
-
- grub_memset (pg, 0, sz * PAGE_SIZE);
-
- lp = paging_start + lx[NUMBER_OF_LEVELS - 1];
- for (l = NUMBER_OF_LEVELS - 1; l >= 1; l--)
- {
- if (lxs[l] || pr)
- pg[0] = page2offset (mfn_list[lp++]) | INTERMEDIATE_OR;
- if (pr)
- pg += POINTERS_PER_PAGE;
- for (j = 0; j < lx[l - 1]; j++)
- pg[j + lxs[l]] = page2offset (mfn_list[lp++]) | INTERMEDIATE_OR;
- pg += lx[l] * POINTERS_PER_PAGE;
- if (lxs[l])
- pr = 1;
- }
-
- if (lxs[0] || pr)
- pg[0] = page2offset (mfn_list[total_pages]) | 5;
- if (pr)
- pg += POINTERS_PER_PAGE;
-
- for (j = 0; j < paging_end; j++)
- {
- if (j >= paging_start && j < lp)
- pg[j + lxs[0]] = page2offset (mfn_list[j]) | 5;
- else
- pg[j + lxs[0]] = page2offset (mfn_list[j]) | 7;
+ for (m1 = 0; m1 < xen_state.n_mappings; m1++)
+ {
+ start = xen_state.mappings[m1].lvls[l].virt_start;
+ end = xen_state.mappings[m1].lvls[l].virt_end;
+ pg = get_pg_table_virt(m1, l);
+ for (m2 = 0; m2 < xen_state.n_mappings; m2++)
+ {
+ lvl = (l > 0) ? xen_state.mappings[m2].lvls + l - 1
+ : &xen_state.mappings[m2].area;
+ if (l > 0 && lvl->n_pt_pages == 0)
+ continue;
+ if (lvl->virt_start >= end || lvl->virt_end <= start)
+ continue;
+ p_s = (grub_max (start, lvl->virt_start) - start) >>
+ (PAGE_SHIFT + l * LOG_POINTERS_PER_PAGE);
+ p_e = (grub_min (end, lvl->virt_end) - start) >>
+ (PAGE_SHIFT + l * LOG_POINTERS_PER_PAGE);
+ pfn = ((grub_max (start, lvl->virt_start) - lvl->virt_start) >>
+ (PAGE_SHIFT + l * LOG_POINTERS_PER_PAGE)) + lvl->pfn_start;
+ grub_dprintf ("xen", "write page table entries level %d pg %p "
+ "mapping %d/%d index %lx-%lx pfn %llx\n",
+ l, pg, m1, m2, p_s, p_e, (unsigned long long) pfn);
+ for (p = p_s; p <= p_e; p++)
+ {
+ pg[p] = page2offset (mfn_list[pfn]) |
+ get_pg_table_prot (l, pfn);
+ pfn++;
+ }
+ }
+ }
}
}
@@ -280,42 +370,68 @@ grub_xen_pt_alloc (void)
grub_relocator_chunk_t ch;
grub_err_t err;
grub_uint64_t nr_info_pages;
- grub_uint64_t nr_pages, nr_pt_pages, nr_need_pages;
+ grub_uint64_t nr_need_pages;
+ grub_uint64_t try_virt_end;
+ struct grub_xen_mapping *map;
+
+ map = xen_state.mappings + xen_state.n_mappings;
+ xen_state.map_reloc = map + 1;
xen_state.next_start.pt_base =
xen_state.max_addr + xen_state.xen_inf.virt_base;
- xen_state.state.paging_start = xen_state.max_addr >> PAGE_SHIFT;
-
nr_info_pages = xen_state.max_addr >> PAGE_SHIFT;
- nr_pages = nr_info_pages;
+ nr_need_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_state.xen_inf.virt_base);
- nr_need_pages =
- nr_info_pages + nr_pt_pages +
- ((ADDITIONAL_SIZE + STACK_SIZE) >> PAGE_SHIFT);
- if (nr_pages >= nr_need_pages)
+ try_virt_end = ALIGN_UP (xen_state.xen_inf.virt_base +
+ page2offset (nr_need_pages) +
+ ADDITIONAL_SIZE + STACK_SIZE, ALIGN_SIZE);
+ if (!xen_state.xen_inf.virt_base)
+ try_virt_end += PAGE_SIZE;
+
+ err = get_pgtable_size (xen_state.xen_inf.virt_base, try_virt_end,
+ nr_info_pages);
+ if (err)
+ return err;
+ xen_state.n_mappings++;
+
+ /* Map the relocator page either at virtual 0 or after end of area. */
+ nr_need_pages = nr_info_pages + map->area.n_pt_pages;
+ if (xen_state.xen_inf.virt_base)
+ err = get_pgtable_size (0, PAGE_SIZE, nr_need_pages);
+ else
+ err = get_pgtable_size (try_virt_end - PAGE_SIZE, try_virt_end,
+ nr_need_pages);
+ if (err)
+ return err;
+ nr_need_pages += xen_state.map_reloc->area.n_pt_pages;
+
+ if (xen_state.xen_inf.virt_base + page2offset (nr_need_pages) <=
+ try_virt_end)
break;
- nr_pages = nr_need_pages;
+
+ xen_state.n_mappings--;
}
+ xen_state.n_mappings++;
+ nr_need_pages = map->area.n_pt_pages + xen_state.map_reloc->area.n_pt_pages;
err = grub_relocator_alloc_chunk_addr (xen_state.relocator, &ch,
xen_state.max_addr,
- page2offset (nr_pt_pages));
+ page2offset (nr_need_pages));
if (err)
return err;
- xen_state.virt_pgtable = get_virtual_current_address (ch);
- xen_state.pgtbl_start = xen_state.max_addr >> PAGE_SHIFT;
- xen_state.max_addr += page2offset (nr_pt_pages);
+ map->where = get_virtual_current_address (ch);
+ map->area.pfn_start = 0;
+ xen_state.max_addr += page2offset (nr_need_pages);
xen_state.state.stack =
xen_state.max_addr + STACK_SIZE + xen_state.xen_inf.virt_base;
- xen_state.state.paging_size = nr_pt_pages;
- xen_state.next_start.nr_pt_frames = nr_pt_pages;
- xen_state.max_addr = page2offset (nr_pages);
- xen_state.pgtbl_end = nr_pages;
+ xen_state.next_start.nr_pt_frames = nr_need_pages;
+ xen_state.max_addr = try_virt_end - xen_state.xen_inf.virt_base;
+ xen_state.pgtbl_end = xen_state.max_addr >> PAGE_SHIFT;
+ xen_state.map_reloc->where = (grub_uint64_t *) ((char *) map->where +
+ page2offset (map->area.n_pt_pages));
return GRUB_ERR_NONE;
}
@@ -367,9 +483,8 @@ grub_xen_boot (void)
(unsigned long long) xen_state.xen_inf.virt_base,
(unsigned long long) page2offset (nr_pages));
- generate_page_table (xen_state.virt_pgtable, xen_state.pgtbl_start,
- xen_state.pgtbl_end, nr_pages,
- xen_state.xen_inf.virt_base, xen_state.virt_mfn_list);
+ xen_state.map_reloc->area.pfn_start = nr_pages;
+ generate_page_table (xen_state.virt_mfn_list);
xen_state.state.entry_point = xen_state.xen_inf.entry_point;
diff --git a/include/grub/i386/memory.h b/include/grub/i386/memory.h
index c9b1328..8bb6e1c 100644
--- a/include/grub/i386/memory.h
+++ b/include/grub/i386/memory.h
@@ -20,6 +20,8 @@
#ifndef GRUB_MEMORY_CPU_HEADER
#define GRUB_MEMORY_CPU_HEADER 1
+#define PAGE_SHIFT 12
+
/* The flag for protected mode. */
#define GRUB_MEMORY_CPU_CR0_PE_ON 0x1
#define GRUB_MEMORY_CPU_CR4_PAE_ON 0x00000020
@@ -31,6 +33,11 @@
#define GRUB_MEMORY_MACHINE_UPPER_START 0x100000 /* 1 MiB */
#define GRUB_MEMORY_MACHINE_LOWER_SIZE GRUB_MEMORY_MACHINE_UPPER_START
+/* Some PTE definitions. */
+#define GRUB_PAGE_PRESENT 0x00000001
+#define GRUB_PAGE_RW 0x00000002
+#define GRUB_PAGE_USER 0x00000004
+
#ifndef ASM_FILE
#define GRUB_MMAP_MALLOC_LOW 1
diff --git a/include/grub/xen/relocator.h b/include/grub/xen/relocator.h
index ae45dce..35a0ad9 100644
--- a/include/grub/xen/relocator.h
+++ b/include/grub/xen/relocator.h
@@ -23,11 +23,13 @@
#include <grub/err.h>
#include <grub/relocator.h>
+#define XEN_MAX_MAPPINGS 3
+
struct grub_relocator_xen_state
{
grub_addr_t start_info;
- grub_addr_t paging_start;
- grub_addr_t paging_size;
+ grub_addr_t paging_start[XEN_MAX_MAPPINGS];
+ grub_addr_t paging_size[XEN_MAX_MAPPINGS];
grub_addr_t mfn_list;
grub_addr_t stack;
grub_addr_t entry_point;
--
2.6.2
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [PATCH v4 10/11] xen: modify page table construction
2016-02-22 6:03 ` [PATCH v4 10/11] xen: modify page table construction Juergen Gross
@ 2016-02-22 9:17 ` Daniel Kiper
[not found] ` <20160222091748.GO3482@olila.local.net-space.pl>
1 sibling, 0 replies; 33+ messages in thread
From: Daniel Kiper @ 2016-02-22 9:17 UTC (permalink / raw)
To: Juergen Gross; +Cc: grub-devel, phcoder, mchang, xen-devel
On Mon, Feb 22, 2016 at 07:03:18AM +0100, Juergen Gross wrote:
> Modify the page table construction to allow multiple virtual regions
> to be mapped. This is done as preparation for removing the p2m list
> from the initial kernel mapping in order to support huge pv domains.
>
> This allows a cleaner approach for mapping the relocator page by
> using this capability.
>
> The interface to the assembler level of the relocator has to be changed
> in order to be able to process multiple page table areas.
>
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
> V4: align variables in assembly sources
> use separate structure define as requested by Daniel Kiper
>
> V3: use constants instead of numbers as requested by Daniel Kiper
> add lots of comments to assembly code as requested by Daniel Kiper
> ---
> grub-core/lib/i386/xen/relocator.S | 88 ++++++----
> grub-core/lib/x86_64/xen/relocator.S | 135 ++++++---------
> grub-core/lib/xen/relocator.c | 28 ++-
> grub-core/loader/i386/xen.c | 325 ++++++++++++++++++++++++-----------
> include/grub/i386/memory.h | 7 +
> include/grub/xen/relocator.h | 6 +-
> 6 files changed, 359 insertions(+), 230 deletions(-)
>
> diff --git a/grub-core/lib/i386/xen/relocator.S b/grub-core/lib/i386/xen/relocator.S
> index 694a54c..f8d135a 100644
> --- a/grub-core/lib/i386/xen/relocator.S
> +++ b/grub-core/lib/i386/xen/relocator.S
> @@ -16,6 +16,8 @@
> * along with GRUB. If not, see <http://www.gnu.org/licenses/>.
> */
>
> +#include <grub/i386/memory.h>
> +#include <grub/i386/types.h>
> #include <grub/symbol.h>
> #include <grub/xen.h>
>
> @@ -23,78 +25,86 @@
>
> VARIABLE(grub_relocator_xen_remap_start)
> LOCAL(base):
> - /* mov imm32, %ebx */
> + /* Remap the remapper to it's new address. */
> + /* mov imm32, %ebx - %ebx: new virtual address of remapper */
> .byte 0xbb
> VARIABLE(grub_relocator_xen_remapper_virt)
> .long 0
>
> - /* mov imm32, %ecx */
> + /* mov imm32, %ecx - %ecx: low part of page table entry */
> .byte 0xb9
> VARIABLE(grub_relocator_xen_remapper_map)
> .long 0
>
> - /* mov imm32, %edx */
> + /* mov imm32, %edx - %edx: high part of page table entry */
> .byte 0xba
> VARIABLE(grub_relocator_xen_remapper_map_high)
> .long 0
>
> - movl %ebx, %ebp
> + movl %ebx, %ebp /* %ebx is clobbered by hypercall */
>
> - movl $2, %esi
> + movl $UVMF_INVLPG, %esi /* esi: flags (inv. single entry) */
> movl $__HYPERVISOR_update_va_mapping, %eax
> int $0x82
>
> movl %ebp, %ebx
> addl $(LOCAL(cont) - LOCAL(base)), %ebx
>
> - jmp *%ebx
> + jmp *%ebx /* Continue with new virtual address */
>
> LOCAL(cont):
> - xorl %eax, %eax
> - movl %eax, %ebp
> + /* Modify mappings of new page tables to be read-only. */
> + /* mov imm32, %eax */
> + .byte 0xb8
> +VARIABLE(grub_relocator_xen_paging_areas_addr)
> + .long 0
> + movl %eax, %ebx
> 1:
> + movl 0(%ebx), %ebp /* Get start pfn of the current area */
> + movl GRUB_TARGET_SIZEOF_LONG(%ebx), %ecx /* Get # of pg tables */
> + testl %ecx, %ecx /* 0 -> last area reached */
> + jz 3f
> + addl $(2 * GRUB_TARGET_SIZEOF_LONG), %ebx
> + movl %ebx, %esp /* Save current area pointer */
>
> +2:
> + movl %ecx, %edi
> /* mov imm32, %eax */
> .byte 0xb8
> VARIABLE(grub_relocator_xen_mfn_list)
> .long 0
> - movl %eax, %edi
> - movl %ebp, %eax
> - movl 0(%edi, %eax, 4), %ecx
> -
> - /* mov imm32, %ebx */
> - .byte 0xbb
> -VARIABLE(grub_relocator_xen_paging_start)
> - .long 0
> - shll $12, %eax
> - addl %eax, %ebx
> + movl 0(%eax, %ebp, 4), %ecx /* mfn */
> + movl %ebp, %ebx
> + shll $PAGE_SHIFT, %ebx /* virtual address (1:1 mapping) */
> movl %ecx, %edx
> - shll $12, %ecx
> - shrl $20, %edx
> - orl $5, %ecx
> - movl $2, %esi
> + shll $PAGE_SHIFT, %ecx /* prepare pte low part */
> + shrl $(32 - PAGE_SHIFT), %edx /* pte high part */
> + orl $(GRUB_PAGE_PRESENT | GRUB_PAGE_USER), %ecx /* pte low */
> + movl $UVMF_INVLPG, %esi
> movl $__HYPERVISOR_update_va_mapping, %eax
> - int $0x82
> + int $0x82 /* parameters: eax, ebx, ecx, edx, esi */
>
> - incl %ebp
> - /* mov imm32, %ecx */
> - .byte 0xb9
> -VARIABLE(grub_relocator_xen_paging_size)
> - .long 0
> - cmpl %ebp, %ecx
> + incl %ebp /* next pfn */
> + movl %edi, %ecx
>
> - ja 1b
> + loop 2b
>
> + mov %esp, %ebx /* restore area poniter */
> + jmp 1b
> +
> +3:
> + /* Switch page tables: pin new L3 pt, load cr3, unpin old L3. */
> /* mov imm32, %ebx */
> .byte 0xbb
> VARIABLE(grub_relocator_xen_mmu_op_addr)
> .long 0
> - movl $3, %ecx
> - movl $0, %edx
> - movl $0x7FF0, %esi
> + movl $3, %ecx /* 3 mmu ops */
> + movl $0, %edx /* pdone (not used) */
> + movl $DOMID_SELF, %esi
> movl $__HYPERVISOR_mmuext_op, %eax
> int $0x82
>
> + /* Continue in virtual kernel mapping. */
> /* mov imm32, %eax */
> .byte 0xb8
> VARIABLE(grub_relocator_xen_remap_continue)
> @@ -102,6 +112,10 @@ VARIABLE(grub_relocator_xen_remap_continue)
>
> jmp *%eax
>
> + .p2align 2
I do not think this is needed but...
> +VARIABLE(grub_relocator_xen_paging_areas)
> + .long 0, 0, 0, 0, 0, 0, 0, 0
> +
> VARIABLE(grub_relocator_xen_mmu_op)
> .space 256
>
> @@ -109,6 +123,7 @@ VARIABLE(grub_relocator_xen_remap_end)
>
>
> VARIABLE(grub_relocator_xen_start)
> + /* Unmap old remapper area. */
> /* mov imm32, %eax */
> .byte 0xb8
> VARIABLE(grub_relocator_xen_remapper_virt2)
> @@ -116,14 +131,14 @@ VARIABLE(grub_relocator_xen_remapper_virt2)
>
> movl %eax, %edi
>
> - xorl %ecx, %ecx
> + xorl %ecx, %ecx /* Invalid pte */
> xorl %edx, %edx
>
> - movl $2, %esi
> + movl $UVMF_INVLPG, %esi
> movl $__HYPERVISOR_update_va_mapping, %eax
> int $0x82
>
> -
> + /* Prepare registers for starting kernel. */
> /* mov imm32, %eax */
> .byte 0xb8
> VARIABLE(grub_relocator_xen_stack)
> @@ -145,6 +160,7 @@ VARIABLE(grub_relocator_xen_start_info)
> VARIABLE(grub_relocator_xen_entry_point)
> .long 0
>
> + /* Now start the new kernel. */
> jmp *%eax
>
> VARIABLE(grub_relocator_xen_end)
> diff --git a/grub-core/lib/x86_64/xen/relocator.S b/grub-core/lib/x86_64/xen/relocator.S
> index 92e9e72..38ae157 100644
> --- a/grub-core/lib/x86_64/xen/relocator.S
> +++ b/grub-core/lib/x86_64/xen/relocator.S
> @@ -16,95 +16,86 @@
> * along with GRUB. If not, see <http://www.gnu.org/licenses/>.
> */
>
> +#include <grub/x86_64/memory.h>
> +#include <grub/x86_64/types.h>
> #include <grub/symbol.h>
> #include <grub/xen.h>
>
> +/* Macro to load an imm64 value stored by the C-part into %rax: */
> +#define MOV_IMM64_RAX(var) .byte 0x48, 0xb8; VARIABLE(var); .quad 0
> +
> .p2align 4 /* force 16-byte alignment */
>
> VARIABLE(grub_relocator_xen_remap_start)
> LOCAL(base):
> - /* mov imm64, %rax */
> - .byte 0x48
> - .byte 0xb8
> -VARIABLE(grub_relocator_xen_remapper_virt)
> - .quad 0
> + /* Remap the remapper to it's new address. */
> + MOV_IMM64_RAX(grub_relocator_xen_remapper_virt)
>
> - movq %rax, %rdi
> - movq %rax, %rbx
> + movq %rax, %rdi /* %rdi: new virtual address of remapper */
> + movq %rax, %rbx /* Remember new virtual address */
>
> - /* mov imm64, %rax */
> - .byte 0x48
> - .byte 0xb8
> -VARIABLE(grub_relocator_xen_remapper_map)
> - .quad 0
> + MOV_IMM64_RAX(grub_relocator_xen_remapper_map)
>
> - movq %rax, %rsi
> + movq %rax, %rsi /* %rsi: page table entry */
>
> - movq $2, %rdx
> + movq $UVMF_INVLPG, %rdx /* %rdx: flags (inv. single entry) */
> movq $__HYPERVISOR_update_va_mapping, %rax
> - syscall
> + syscall /* Do the remap operation */
>
> addq $(LOCAL(cont) - LOCAL(base)), %rbx
>
> - jmp *%rbx
> + jmp *%rbx /* Continue with new virtual address */
>
> LOCAL(cont):
> -
> - /* mov imm64, %rcx */
> - .byte 0x48
> - .byte 0xb9
> -VARIABLE(grub_relocator_xen_paging_size)
> - .quad 0
> + /* Modify mappings of new page tables to be read-only. */
> + MOV_IMM64_RAX(grub_relocator_xen_mfn_list)
>
> - /* mov imm64, %rax */
> - .byte 0x48
> - .byte 0xb8
> -VARIABLE(grub_relocator_xen_paging_start)
> - .quad 0
> + movq %rax, %rbx /* %rbx is the base of the p2m list */
> + leaq EXT_C(grub_relocator_xen_paging_areas) (%rip), %r8
>
> - movq %rax, %r12
> -
> - /* mov imm64, %rax */
> - .byte 0x48
> - .byte 0xb8
> -VARIABLE(grub_relocator_xen_mfn_list)
> - .quad 0
> -
> - movq %rax, %rsi
> 1:
> + movq 0(%r8), %r12 /* Get start pfn of the current area */
> + movq GRUB_TARGET_SIZEOF_LONG(%r8), %rcx /* Get # of pg tables */
> + testq %rcx, %rcx /* 0 -> last area reached */
> + jz 3f
> +2:
> movq %r12, %rdi
> - movq %rsi, %rbx
> - movq 0(%rsi), %rsi
> - shlq $12, %rsi
> - orq $5, %rsi
> - movq $2, %rdx
> - movq %rcx, %r9
> + shlq $PAGE_SHIFT, %rdi /* virtual address (1:1 mapping) */
> + movq (%rbx, %r12, 8), %rsi /* mfn */
> + shlq $PAGE_SHIFT, %rsi
> + orq $(GRUB_PAGE_PRESENT | GRUB_PAGE_USER), %rsi /* Build pte */
> + movq $UVMF_INVLPG, %rdx
> + movq %rcx, %r9 /* %rcx clobbered by hypercall */
> movq $__HYPERVISOR_update_va_mapping, %rax
> syscall
>
> movq %r9, %rcx
> - addq $8, %rbx
> - addq $4096, %r12
> - movq %rbx, %rsi
> + incq %r12 /* next pfn */
>
> - loop 1b
> + loop 2b
>
> - leaq LOCAL(mmu_op) (%rip), %rdi
> - movq $3, %rsi
> - movq $0, %rdx
> - movq $0x7FF0, %r10
> + addq $(2 * GRUB_TARGET_SIZEOF_LONG), %r8 /* next pg table area */
> + jmp 1b
> +
> +3:
> + /* Switch page tables: pin new L4 pt, load cr3, unpin old L4. */
> + leaq EXT_C(grub_relocator_xen_mmu_op) (%rip), %rdi
> + movq $3, %rsi /* 3 mmu ops */
> + movq $0, %rdx /* pdone (not used) */
> + movq $DOMID_SELF, %r10
> movq $__HYPERVISOR_mmuext_op, %rax
> syscall
>
> - /* mov imm64, %rax */
> - .byte 0x48
> - .byte 0xb8
> -VARIABLE(grub_relocator_xen_remap_continue)
> - .quad 0
> + /* Continue in virtual kernel mapping. */
> + MOV_IMM64_RAX(grub_relocator_xen_remap_continue)
>
> jmp *%rax
>
> -LOCAL(mmu_op):
> + .p2align 3
Ditto...
> +VARIABLE(grub_relocator_xen_paging_areas)
> + /* array of start, size pairs, size 0 is end marker */
> + .quad 0, 0, 0, 0, 0, 0, 0, 0
> +
> VARIABLE(grub_relocator_xen_mmu_op)
> .space 256
>
> @@ -112,46 +103,32 @@ VARIABLE(grub_relocator_xen_remap_end)
>
>
> VARIABLE(grub_relocator_xen_start)
> - /* mov imm64, %rax */
> - .byte 0x48
> - .byte 0xb8
> -VARIABLE(grub_relocator_xen_remapper_virt2)
> - .quad 0
> + /* Unmap old remapper area. */
> + MOV_IMM64_RAX(grub_relocator_xen_remapper_virt2)
>
> movq %rax, %rdi
>
> - xorq %rax, %rax
> + xorq %rax, %rax /* Invalid pte */
> movq %rax, %rsi
>
> - movq $2, %rdx
> + movq $UVMF_INVLPG, %rdx
> movq $__HYPERVISOR_update_va_mapping, %rax
> syscall
>
> -
> - /* mov imm64, %rax */
> - .byte 0x48
> - .byte 0xb8
> -VARIABLE(grub_relocator_xen_stack)
> - .quad 0
> + /* Prepare registers for starting kernel. */
> + MOV_IMM64_RAX(grub_relocator_xen_stack)
>
> movq %rax, %rsp
>
> - /* mov imm64, %rax */
> - .byte 0x48
> - .byte 0xb8
> -VARIABLE(grub_relocator_xen_start_info)
> - .quad 0
> + MOV_IMM64_RAX(grub_relocator_xen_start_info)
>
> movq %rax, %rsi
>
> cld
>
> - /* mov imm64, %rax */
> - .byte 0x48
> - .byte 0xb8
> -VARIABLE(grub_relocator_xen_entry_point)
> - .quad 0
> + MOV_IMM64_RAX(grub_relocator_xen_entry_point)
>
> + /* Now start the new kernel. */
> jmp *%rax
>
> VARIABLE(grub_relocator_xen_end)
> diff --git a/grub-core/lib/xen/relocator.c b/grub-core/lib/xen/relocator.c
> index 8f427d3..a05b253 100644
> --- a/grub-core/lib/xen/relocator.c
> +++ b/grub-core/lib/xen/relocator.c
> @@ -29,6 +29,11 @@
>
> typedef grub_addr_t grub_xen_reg_t;
>
> +struct grub_relocator_xen_paging_area {
> + grub_xen_reg_t start;
> + grub_xen_reg_t size;
> +};
> +
... this should have GRUB_PACKED because compiler may
add padding to align size member.
Daniel
^ permalink raw reply [flat|nested] 33+ messages in thread[parent not found: <20160222091748.GO3482@olila.local.net-space.pl>]
* Re: [PATCH v4 10/11] xen: modify page table construction
[not found] ` <20160222091748.GO3482@olila.local.net-space.pl>
@ 2016-02-22 9:29 ` Juergen Gross
[not found] ` <56CAD4E0.80304@suse.com>
1 sibling, 0 replies; 33+ messages in thread
From: Juergen Gross @ 2016-02-22 9:29 UTC (permalink / raw)
To: Daniel Kiper; +Cc: grub-devel, phcoder, mchang, xen-devel
On 22/02/16 10:17, Daniel Kiper wrote:
> On Mon, Feb 22, 2016 at 07:03:18AM +0100, Juergen Gross wrote:
>> Modify the page table construction to allow multiple virtual regions
>> to be mapped. This is done as preparation for removing the p2m list
>> from the initial kernel mapping in order to support huge pv domains.
>>
>> This allows a cleaner approach for mapping the relocator page by
>> using this capability.
>>
>> The interface to the assembler level of the relocator has to be changed
>> in order to be able to process multiple page table areas.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>> V4: align variables in assembly sources
>> use separate structure define as requested by Daniel Kiper
>>
>> V3: use constants instead of numbers as requested by Daniel Kiper
>> add lots of comments to assembly code as requested by Daniel Kiper
>> ---
>> grub-core/lib/i386/xen/relocator.S | 88 ++++++----
>> grub-core/lib/x86_64/xen/relocator.S | 135 ++++++---------
>> grub-core/lib/xen/relocator.c | 28 ++-
>> grub-core/loader/i386/xen.c | 325 ++++++++++++++++++++++++-----------
>> include/grub/i386/memory.h | 7 +
>> include/grub/xen/relocator.h | 6 +-
>> 6 files changed, 359 insertions(+), 230 deletions(-)
>>
>> diff --git a/grub-core/lib/i386/xen/relocator.S b/grub-core/lib/i386/xen/relocator.S
>> index 694a54c..f8d135a 100644
>> --- a/grub-core/lib/i386/xen/relocator.S
>> +++ b/grub-core/lib/i386/xen/relocator.S
>> @@ -102,6 +112,10 @@ VARIABLE(grub_relocator_xen_remap_continue)
>>
>> jmp *%eax
>>
>> + .p2align 2
>
> I do not think this is needed but...
Even if the processor does allow unaligned loads: it's cleaner to
avoid them.
>
>> diff --git a/grub-core/lib/x86_64/xen/relocator.S b/grub-core/lib/x86_64/xen/relocator.S
>> index 92e9e72..38ae157 100644
>> --- a/grub-core/lib/x86_64/xen/relocator.S
>> +++ b/grub-core/lib/x86_64/xen/relocator.S
>>
>> jmp *%rax
>>
>> -LOCAL(mmu_op):
>> + .p2align 3
>
> Ditto...
>
>> diff --git a/grub-core/lib/xen/relocator.c b/grub-core/lib/xen/relocator.c
>> index 8f427d3..a05b253 100644
>> --- a/grub-core/lib/xen/relocator.c
>> +++ b/grub-core/lib/xen/relocator.c
>> @@ -29,6 +29,11 @@
>>
>> typedef grub_addr_t grub_xen_reg_t;
>>
>> +struct grub_relocator_xen_paging_area {
>> + grub_xen_reg_t start;
>> + grub_xen_reg_t size;
>> +};
>> +
>
> ... this should have GRUB_PACKED because compiler may
> add padding to align size member.
Why would the compiler add padding to a structure containing two items
of the same type? I don't think the C standard would allow this.
grub_xen_reg_t is either unsigned (32 bit) or unsigned long (64 bit).
There is no way this could require any padding.
Juergen
^ permalink raw reply [flat|nested] 33+ messages in thread[parent not found: <56CAD4E0.80304@suse.com>]
* Re: [PATCH v4 10/11] xen: modify page table construction
[not found] ` <56CAD4E0.80304@suse.com>
@ 2016-02-22 12:18 ` Daniel Kiper
[not found] ` <20160222121858.GQ3482@olila.local.net-space.pl>
1 sibling, 0 replies; 33+ messages in thread
From: Daniel Kiper @ 2016-02-22 12:18 UTC (permalink / raw)
To: Juergen Gross; +Cc: grub-devel, phcoder, mchang, xen-devel
On Mon, Feb 22, 2016 at 10:29:04AM +0100, Juergen Gross wrote:
> On 22/02/16 10:17, Daniel Kiper wrote:
> > On Mon, Feb 22, 2016 at 07:03:18AM +0100, Juergen Gross wrote:
> >> Modify the page table construction to allow multiple virtual regions
> >> to be mapped. This is done as preparation for removing the p2m list
> >> from the initial kernel mapping in order to support huge pv domains.
> >>
> >> This allows a cleaner approach for mapping the relocator page by
> >> using this capability.
> >>
> >> The interface to the assembler level of the relocator has to be changed
> >> in order to be able to process multiple page table areas.
> >>
> >> Signed-off-by: Juergen Gross <jgross@suse.com>
> >> ---
> >> V4: align variables in assembly sources
> >> use separate structure define as requested by Daniel Kiper
> >>
> >> V3: use constants instead of numbers as requested by Daniel Kiper
> >> add lots of comments to assembly code as requested by Daniel Kiper
> >> ---
> >> grub-core/lib/i386/xen/relocator.S | 88 ++++++----
> >> grub-core/lib/x86_64/xen/relocator.S | 135 ++++++---------
> >> grub-core/lib/xen/relocator.c | 28 ++-
> >> grub-core/loader/i386/xen.c | 325 ++++++++++++++++++++++++-----------
> >> include/grub/i386/memory.h | 7 +
> >> include/grub/xen/relocator.h | 6 +-
> >> 6 files changed, 359 insertions(+), 230 deletions(-)
> >>
> >> diff --git a/grub-core/lib/i386/xen/relocator.S b/grub-core/lib/i386/xen/relocator.S
> >> index 694a54c..f8d135a 100644
> >> --- a/grub-core/lib/i386/xen/relocator.S
> >> +++ b/grub-core/lib/i386/xen/relocator.S
> >> @@ -102,6 +112,10 @@ VARIABLE(grub_relocator_xen_remap_continue)
> >>
> >> jmp *%eax
> >>
> >> + .p2align 2
> >
> > I do not think this is needed but...
>
> Even if the processor does allow unaligned loads: it's cleaner to
> avoid them.
I do not think that it pays here. However, if you are going
to leave .p2align please align other variables too.
> >> diff --git a/grub-core/lib/x86_64/xen/relocator.S b/grub-core/lib/x86_64/xen/relocator.S
> >> index 92e9e72..38ae157 100644
> >> --- a/grub-core/lib/x86_64/xen/relocator.S
> >> +++ b/grub-core/lib/x86_64/xen/relocator.S
> >>
> >> jmp *%rax
> >>
> >> -LOCAL(mmu_op):
> >> + .p2align 3
> >
> > Ditto...
> >
> >> diff --git a/grub-core/lib/xen/relocator.c b/grub-core/lib/xen/relocator.c
> >> index 8f427d3..a05b253 100644
> >> --- a/grub-core/lib/xen/relocator.c
> >> +++ b/grub-core/lib/xen/relocator.c
> >> @@ -29,6 +29,11 @@
> >>
> >> typedef grub_addr_t grub_xen_reg_t;
> >>
> >> +struct grub_relocator_xen_paging_area {
> >> + grub_xen_reg_t start;
> >> + grub_xen_reg_t size;
> >> +};
> >> +
> >
> > ... this should have GRUB_PACKED because compiler may
> > add padding to align size member.
>
> Why would the compiler add padding to a structure containing two items
> of the same type? I don't think the C standard would allow this.
>
> grub_xen_reg_t is either unsigned (32 bit) or unsigned long (64 bit).
> There is no way this could require any padding.
You are right but we should add this here just in case.
Daniel
^ permalink raw reply [flat|nested] 33+ messages in thread[parent not found: <20160222121858.GQ3482@olila.local.net-space.pl>]
* Re: [PATCH v4 10/11] xen: modify page table construction
[not found] ` <20160222121858.GQ3482@olila.local.net-space.pl>
@ 2016-02-22 12:30 ` Juergen Gross
[not found] ` <56CAFF66.1080709@suse.com>
1 sibling, 0 replies; 33+ messages in thread
From: Juergen Gross @ 2016-02-22 12:30 UTC (permalink / raw)
To: Daniel Kiper; +Cc: grub-devel, phcoder, mchang, xen-devel
On 22/02/16 13:18, Daniel Kiper wrote:
> On Mon, Feb 22, 2016 at 10:29:04AM +0100, Juergen Gross wrote:
>> On 22/02/16 10:17, Daniel Kiper wrote:
>>> On Mon, Feb 22, 2016 at 07:03:18AM +0100, Juergen Gross wrote:
>>>> Modify the page table construction to allow multiple virtual regions
>>>> to be mapped. This is done as preparation for removing the p2m list
>>>> from the initial kernel mapping in order to support huge pv domains.
>>>>
>>>> This allows a cleaner approach for mapping the relocator page by
>>>> using this capability.
>>>>
>>>> The interface to the assembler level of the relocator has to be changed
>>>> in order to be able to process multiple page table areas.
>>>>
>>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>>> ---
>>>> V4: align variables in assembly sources
>>>> use separate structure define as requested by Daniel Kiper
>>>>
>>>> V3: use constants instead of numbers as requested by Daniel Kiper
>>>> add lots of comments to assembly code as requested by Daniel Kiper
>>>> ---
>>>> grub-core/lib/i386/xen/relocator.S | 88 ++++++----
>>>> grub-core/lib/x86_64/xen/relocator.S | 135 ++++++---------
>>>> grub-core/lib/xen/relocator.c | 28 ++-
>>>> grub-core/loader/i386/xen.c | 325 ++++++++++++++++++++++++-----------
>>>> include/grub/i386/memory.h | 7 +
>>>> include/grub/xen/relocator.h | 6 +-
>>>> 6 files changed, 359 insertions(+), 230 deletions(-)
>>>>
>>>> diff --git a/grub-core/lib/i386/xen/relocator.S b/grub-core/lib/i386/xen/relocator.S
>>>> index 694a54c..f8d135a 100644
>>>> --- a/grub-core/lib/i386/xen/relocator.S
>>>> +++ b/grub-core/lib/i386/xen/relocator.S
>>>> @@ -102,6 +112,10 @@ VARIABLE(grub_relocator_xen_remap_continue)
>>>>
>>>> jmp *%eax
>>>>
>>>> + .p2align 2
>>>
>>> I do not think this is needed but...
>>
>> Even if the processor does allow unaligned loads: it's cleaner to
>> avoid them.
>
> I do not think that it pays here. However, if you are going
> to leave .p2align please align other variables too.
Which?
The only candidate would be grub_relocator_xen_mmu_op which already
is aligned due to above p2align. All other variables must be kept
as they are, because they are part of instructions and alignment would
break the code.
>
>>>> diff --git a/grub-core/lib/x86_64/xen/relocator.S b/grub-core/lib/x86_64/xen/relocator.S
>>>> index 92e9e72..38ae157 100644
>>>> --- a/grub-core/lib/x86_64/xen/relocator.S
>>>> +++ b/grub-core/lib/x86_64/xen/relocator.S
>>>>
>>>> jmp *%rax
>>>>
>>>> -LOCAL(mmu_op):
>>>> + .p2align 3
>>>
>>> Ditto...
>>>
>>>> diff --git a/grub-core/lib/xen/relocator.c b/grub-core/lib/xen/relocator.c
>>>> index 8f427d3..a05b253 100644
>>>> --- a/grub-core/lib/xen/relocator.c
>>>> +++ b/grub-core/lib/xen/relocator.c
>>>> @@ -29,6 +29,11 @@
>>>>
>>>> typedef grub_addr_t grub_xen_reg_t;
>>>>
>>>> +struct grub_relocator_xen_paging_area {
>>>> + grub_xen_reg_t start;
>>>> + grub_xen_reg_t size;
>>>> +};
>>>> +
>>>
>>> ... this should have GRUB_PACKED because compiler may
>>> add padding to align size member.
>>
>> Why would the compiler add padding to a structure containing two items
>> of the same type? I don't think the C standard would allow this.
>>
>> grub_xen_reg_t is either unsigned (32 bit) or unsigned long (64 bit).
>> There is no way this could require any padding.
>
> You are right but we should add this here just in case.
Sorry, I don't think this makes any sense. The C standard is very clear
in this case: a type requiring a special alignment has always a length
being a multiple of that alignment. Otherwise arrays wouldn't work.
Adding GRUB_PACKED would make the code less clear IMO. Finding such a
qualifier in some code I want to modify would make me search for the
reason for it which isn't existing.
Juergen
^ permalink raw reply [flat|nested] 33+ messages in thread[parent not found: <56CAFF66.1080709@suse.com>]
* Re: [PATCH v4 10/11] xen: modify page table construction
[not found] ` <56CAFF66.1080709@suse.com>
@ 2016-02-22 12:48 ` Daniel Kiper
[not found] ` <20160222124805.GS3482@olila.local.net-space.pl>
1 sibling, 0 replies; 33+ messages in thread
From: Daniel Kiper @ 2016-02-22 12:48 UTC (permalink / raw)
To: Juergen Gross; +Cc: grub-devel, phcoder, mchang, xen-devel
On Mon, Feb 22, 2016 at 01:30:30PM +0100, Juergen Gross wrote:
> On 22/02/16 13:18, Daniel Kiper wrote:
> > On Mon, Feb 22, 2016 at 10:29:04AM +0100, Juergen Gross wrote:
> >> On 22/02/16 10:17, Daniel Kiper wrote:
> >>> On Mon, Feb 22, 2016 at 07:03:18AM +0100, Juergen Gross wrote:
> >>>> Modify the page table construction to allow multiple virtual regions
> >>>> to be mapped. This is done as preparation for removing the p2m list
> >>>> from the initial kernel mapping in order to support huge pv domains.
> >>>>
> >>>> This allows a cleaner approach for mapping the relocator page by
> >>>> using this capability.
> >>>>
> >>>> The interface to the assembler level of the relocator has to be changed
> >>>> in order to be able to process multiple page table areas.
> >>>>
> >>>> Signed-off-by: Juergen Gross <jgross@suse.com>
> >>>> ---
> >>>> V4: align variables in assembly sources
> >>>> use separate structure define as requested by Daniel Kiper
> >>>>
> >>>> V3: use constants instead of numbers as requested by Daniel Kiper
> >>>> add lots of comments to assembly code as requested by Daniel Kiper
> >>>> ---
> >>>> grub-core/lib/i386/xen/relocator.S | 88 ++++++----
> >>>> grub-core/lib/x86_64/xen/relocator.S | 135 ++++++---------
> >>>> grub-core/lib/xen/relocator.c | 28 ++-
> >>>> grub-core/loader/i386/xen.c | 325 ++++++++++++++++++++++++-----------
> >>>> include/grub/i386/memory.h | 7 +
> >>>> include/grub/xen/relocator.h | 6 +-
> >>>> 6 files changed, 359 insertions(+), 230 deletions(-)
> >>>>
> >>>> diff --git a/grub-core/lib/i386/xen/relocator.S b/grub-core/lib/i386/xen/relocator.S
> >>>> index 694a54c..f8d135a 100644
> >>>> --- a/grub-core/lib/i386/xen/relocator.S
> >>>> +++ b/grub-core/lib/i386/xen/relocator.S
> >>>> @@ -102,6 +112,10 @@ VARIABLE(grub_relocator_xen_remap_continue)
> >>>>
> >>>> jmp *%eax
> >>>>
> >>>> + .p2align 2
> >>>
> >>> I do not think this is needed but...
> >>
> >> Even if the processor does allow unaligned loads: it's cleaner to
> >> avoid them.
> >
> > I do not think that it pays here. However, if you are going
> > to leave .p2align please align other variables too.
>
> Which?
>
> The only candidate would be grub_relocator_xen_mmu_op which already
> is aligned due to above p2align. All other variables must be kept
If that is all then OK. However, IMO, it does not pays here and there.
> as they are, because they are part of instructions and alignment would
> break the code.
I am aware of that.
> >>>> diff --git a/grub-core/lib/x86_64/xen/relocator.S b/grub-core/lib/x86_64/xen/relocator.S
> >>>> index 92e9e72..38ae157 100644
> >>>> --- a/grub-core/lib/x86_64/xen/relocator.S
> >>>> +++ b/grub-core/lib/x86_64/xen/relocator.S
> >>>>
> >>>> jmp *%rax
> >>>>
> >>>> -LOCAL(mmu_op):
> >>>> + .p2align 3
> >>>
> >>> Ditto...
> >>>
> >>>> diff --git a/grub-core/lib/xen/relocator.c b/grub-core/lib/xen/relocator.c
> >>>> index 8f427d3..a05b253 100644
> >>>> --- a/grub-core/lib/xen/relocator.c
> >>>> +++ b/grub-core/lib/xen/relocator.c
> >>>> @@ -29,6 +29,11 @@
> >>>>
> >>>> typedef grub_addr_t grub_xen_reg_t;
> >>>>
> >>>> +struct grub_relocator_xen_paging_area {
> >>>> + grub_xen_reg_t start;
> >>>> + grub_xen_reg_t size;
> >>>> +};
> >>>> +
> >>>
> >>> ... this should have GRUB_PACKED because compiler may
> >>> add padding to align size member.
> >>
> >> Why would the compiler add padding to a structure containing two items
> >> of the same type? I don't think the C standard would allow this.
> >>
> >> grub_xen_reg_t is either unsigned (32 bit) or unsigned long (64 bit).
> >> There is no way this could require any padding.
> >
> > You are right but we should add this here just in case.
>
> Sorry, I don't think this makes any sense. The C standard is very clear
> in this case: a type requiring a special alignment has always a length
> being a multiple of that alignment. Otherwise arrays wouldn't work.
Sorry, I am not sure what do you mean by that.
> Adding GRUB_PACKED would make the code less clear IMO. Finding such a
> qualifier in some code I want to modify would make me search for the
> reason for it which isn't existing.
If maintainers do not object I am not going to insist on that any longer.
Daniel
^ permalink raw reply [flat|nested] 33+ messages in thread[parent not found: <20160222124805.GS3482@olila.local.net-space.pl>]
* Re: [PATCH v4 10/11] xen: modify page table construction
[not found] ` <20160222124805.GS3482@olila.local.net-space.pl>
@ 2016-02-22 13:14 ` Juergen Gross
[not found] ` <56CB09AE.3040301@suse.com>
1 sibling, 0 replies; 33+ messages in thread
From: Juergen Gross @ 2016-02-22 13:14 UTC (permalink / raw)
To: Daniel Kiper; +Cc: grub-devel, phcoder, mchang, xen-devel
On 22/02/16 13:48, Daniel Kiper wrote:
> On Mon, Feb 22, 2016 at 01:30:30PM +0100, Juergen Gross wrote:
>> On 22/02/16 13:18, Daniel Kiper wrote:
>>> On Mon, Feb 22, 2016 at 10:29:04AM +0100, Juergen Gross wrote:
>>>> On 22/02/16 10:17, Daniel Kiper wrote:
>>>>> On Mon, Feb 22, 2016 at 07:03:18AM +0100, Juergen Gross wrote:
>>>>>> diff --git a/grub-core/lib/xen/relocator.c b/grub-core/lib/xen/relocator.c
>>>>>> index 8f427d3..a05b253 100644
>>>>>> --- a/grub-core/lib/xen/relocator.c
>>>>>> +++ b/grub-core/lib/xen/relocator.c
>>>>>> @@ -29,6 +29,11 @@
>>>>>>
>>>>>> typedef grub_addr_t grub_xen_reg_t;
>>>>>>
>>>>>> +struct grub_relocator_xen_paging_area {
>>>>>> + grub_xen_reg_t start;
>>>>>> + grub_xen_reg_t size;
>>>>>> +};
>>>>>> +
>>>>>
>>>>> ... this should have GRUB_PACKED because compiler may
>>>>> add padding to align size member.
>>>>
>>>> Why would the compiler add padding to a structure containing two items
>>>> of the same type? I don't think the C standard would allow this.
>>>>
>>>> grub_xen_reg_t is either unsigned (32 bit) or unsigned long (64 bit).
>>>> There is no way this could require any padding.
>>>
>>> You are right but we should add this here just in case.
>>
>> Sorry, I don't think this makes any sense. The C standard is very clear
>> in this case: a type requiring a special alignment has always a length
>> being a multiple of that alignment. Otherwise arrays wouldn't work.
>
> Sorry, I am not sure what do you mean by that.
The size of any C type (no matter whether it is an integral type like
"int" or a structure) has always the same alignment restriction as the
type itself. So a type requiring 8 byte alignment will always have a
size of a multiple of 8 bytes. This is mandatory for arrays to work, as
otherwise either the elements wouldn't be placed consecutively in memory
or the alignment restrictions wouldn't be obeyed for all elements.
For our case it means that two structure elements of the same type will
never require a padding between them, thus the annotation with "packed"
can't serve any purpose.
>
>> Adding GRUB_PACKED would make the code less clear IMO. Finding such a
>> qualifier in some code I want to modify would make me search for the
>> reason for it which isn't existing.
>
> If maintainers do not object I am not going to insist on that any longer.
Juergen
^ permalink raw reply [flat|nested] 33+ messages in thread[parent not found: <56CB09AE.3040301@suse.com>]
* Re: [PATCH v4 10/11] xen: modify page table construction
[not found] ` <56CB09AE.3040301@suse.com>
@ 2016-02-25 18:33 ` Andrei Borzenkov
[not found] ` <56CF4903.6020304@gmail.com>
1 sibling, 0 replies; 33+ messages in thread
From: Andrei Borzenkov @ 2016-02-25 18:33 UTC (permalink / raw)
To: The development of GNU GRUB, Daniel Kiper; +Cc: phcoder, mchang, xen-devel
22.02.2016 16:14, Juergen Gross пишет:
> On 22/02/16 13:48, Daniel Kiper wrote:
>> On Mon, Feb 22, 2016 at 01:30:30PM +0100, Juergen Gross wrote:
>>> On 22/02/16 13:18, Daniel Kiper wrote:
>>>> On Mon, Feb 22, 2016 at 10:29:04AM +0100, Juergen Gross wrote:
>>>>> On 22/02/16 10:17, Daniel Kiper wrote:
>>>>>> On Mon, Feb 22, 2016 at 07:03:18AM +0100, Juergen Gross wrote:
>>>>>>> diff --git a/grub-core/lib/xen/relocator.c b/grub-core/lib/xen/relocator.c
>>>>>>> index 8f427d3..a05b253 100644
>>>>>>> --- a/grub-core/lib/xen/relocator.c
>>>>>>> +++ b/grub-core/lib/xen/relocator.c
>>>>>>> @@ -29,6 +29,11 @@
>>>>>>>
>>>>>>> typedef grub_addr_t grub_xen_reg_t;
>>>>>>>
>>>>>>> +struct grub_relocator_xen_paging_area {
>>>>>>> + grub_xen_reg_t start;
>>>>>>> + grub_xen_reg_t size;
>>>>>>> +};
>>>>>>> +
>>>>>>
>>>>>> ... this should have GRUB_PACKED because compiler may
>>>>>> add padding to align size member.
>>>>>
>>>>> Why would the compiler add padding to a structure containing two items
>>>>> of the same type? I don't think the C standard would allow this.
>>>>>
>>>>> grub_xen_reg_t is either unsigned (32 bit) or unsigned long (64 bit).
>>>>> There is no way this could require any padding.
>>>>
>>>> You are right but we should add this here just in case.
>>>
>>> Sorry, I don't think this makes any sense. The C standard is very clear
>>> in this case: a type requiring a special alignment has always a length
>>> being a multiple of that alignment. Otherwise arrays wouldn't work.
>>
>> Sorry, I am not sure what do you mean by that.
>
> The size of any C type (no matter whether it is an integral type like
> "int" or a structure) has always the same alignment restriction as the
> type itself. So a type requiring 8 byte alignment will always have a
> size of a multiple of 8 bytes. This is mandatory for arrays to work, as
> otherwise either the elements wouldn't be placed consecutively in memory
> or the alignment restrictions wouldn't be obeyed for all elements.
>
I too not follow how it is relevant to this case. We talk about internal
padding between structure members, not between array elements.
> For our case it means that two structure elements of the same type will
> never require a padding between them, thus the annotation with "packed"
> can't serve any purpose.
>
Well, I am not aware of any requirement. Compiler may add arbitrary
padding between structure elements; it is only prohibited to add padding
at the beginning. Sure, it would be unusual, but never say "never" ...
also should Xen ever be ported to architecture where types are not
self-aligned it will become an issue.
>>
>>> Adding GRUB_PACKED would make the code less clear IMO. Finding such a
>>> qualifier in some code I want to modify would make me search for the
>>> reason for it which isn't existing.
>>
>> If maintainers do not object I am not going to insist on that any longer.
>
It seems inconsistent through the code, really. But I think in this case
it should be packed. It does not look like it is performance critical
and it ensures we always match assembler code.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 33+ messages in thread[parent not found: <56CF4903.6020304@gmail.com>]
* Re: [PATCH v4 10/11] xen: modify page table construction
[not found] ` <56CF4903.6020304@gmail.com>
@ 2016-02-29 9:13 ` Juergen Gross
[not found] ` <56D40BB6.6090904@suse.com>
1 sibling, 0 replies; 33+ messages in thread
From: Juergen Gross @ 2016-02-29 9:13 UTC (permalink / raw)
To: The development of GNU GRUB, Daniel Kiper; +Cc: phcoder, mchang, xen-devel
On 25/02/16 19:33, Andrei Borzenkov wrote:
> 22.02.2016 16:14, Juergen Gross пишет:
>> On 22/02/16 13:48, Daniel Kiper wrote:
>>> On Mon, Feb 22, 2016 at 01:30:30PM +0100, Juergen Gross wrote:
>>>> On 22/02/16 13:18, Daniel Kiper wrote:
>>>>> On Mon, Feb 22, 2016 at 10:29:04AM +0100, Juergen Gross wrote:
>>>>>> On 22/02/16 10:17, Daniel Kiper wrote:
>>>>>>> On Mon, Feb 22, 2016 at 07:03:18AM +0100, Juergen Gross wrote:
>>>>>>>> diff --git a/grub-core/lib/xen/relocator.c b/grub-core/lib/xen/relocator.c
>>>>>>>> index 8f427d3..a05b253 100644
>>>>>>>> --- a/grub-core/lib/xen/relocator.c
>>>>>>>> +++ b/grub-core/lib/xen/relocator.c
>>>>>>>> @@ -29,6 +29,11 @@
>>>>>>>>
>>>>>>>> typedef grub_addr_t grub_xen_reg_t;
>>>>>>>>
>>>>>>>> +struct grub_relocator_xen_paging_area {
>>>>>>>> + grub_xen_reg_t start;
>>>>>>>> + grub_xen_reg_t size;
>>>>>>>> +};
>>>>>>>> +
>>>>>>>
>>>>>>> ... this should have GRUB_PACKED because compiler may
>>>>>>> add padding to align size member.
>>>>>>
>>>>>> Why would the compiler add padding to a structure containing two items
>>>>>> of the same type? I don't think the C standard would allow this.
>>>>>>
>>>>>> grub_xen_reg_t is either unsigned (32 bit) or unsigned long (64 bit).
>>>>>> There is no way this could require any padding.
>>>>>
>>>>> You are right but we should add this here just in case.
>>>>
>>>> Sorry, I don't think this makes any sense. The C standard is very clear
>>>> in this case: a type requiring a special alignment has always a length
>>>> being a multiple of that alignment. Otherwise arrays wouldn't work.
>>>
>>> Sorry, I am not sure what do you mean by that.
>>
>> The size of any C type (no matter whether it is an integral type like
>> "int" or a structure) has always the same alignment restriction as the
>> type itself. So a type requiring 8 byte alignment will always have a
>> size of a multiple of 8 bytes. This is mandatory for arrays to work, as
>> otherwise either the elements wouldn't be placed consecutively in memory
>> or the alignment restrictions wouldn't be obeyed for all elements.
>>
>
> I too not follow how it is relevant to this case. We talk about internal
> padding between structure members, not between array elements.
>
>> For our case it means that two structure elements of the same type will
>> never require a padding between them, thus the annotation with "packed"
>> can't serve any purpose.
>>
>
> Well, I am not aware of any requirement. Compiler may add arbitrary
> padding between structure elements; it is only prohibited to add padding
> at the beginning. Sure, it would be unusual, but never say "never" ...
> also should Xen ever be ported to architecture where types are not
> self-aligned it will become an issue.
So you are telling me that _all_ interfaces between e.g. Linux, grub2,
Xen and all wire protocols not attributed with "packed" are just wrong?
Sorry, I don't think this is true.
And before starting this weird attributing I'd like to see e.g. the
structures in multiboot.h and multiboot2.h to be updated according to
this philosophy.
BTW: Linux kernel isn't using "packed" for network protocols and other
structures where the same reasoning would apply.
>>>> Adding GRUB_PACKED would make the code less clear IMO. Finding such a
>>>> qualifier in some code I want to modify would make me search for the
>>>> reason for it which isn't existing.
>>>
>>> If maintainers do not object I am not going to insist on that any longer.
>>
>
> It seems inconsistent through the code, really. But I think in this case
> it should be packed. It does not look like it is performance critical
> and it ensures we always match assembler code.
In case you still insist on it, I'll change it. But I'm far from
convinced this is a good move.
Juergen
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 33+ messages in thread[parent not found: <56D40BB6.6090904@suse.com>]
* Re: [PATCH v4 10/11] xen: modify page table construction
[not found] ` <56D40BB6.6090904@suse.com>
@ 2016-02-29 12:19 ` Juergen Gross
[not found] ` <56D4374F.5050209@suse.com>
1 sibling, 0 replies; 33+ messages in thread
From: Juergen Gross @ 2016-02-29 12:19 UTC (permalink / raw)
To: The development of GNU GRUB, Daniel Kiper; +Cc: phcoder, mchang, xen-devel
On 29/02/16 10:13, Juergen Gross wrote:
> On 25/02/16 19:33, Andrei Borzenkov wrote:
>> 22.02.2016 16:14, Juergen Gross пишет:
>>> On 22/02/16 13:48, Daniel Kiper wrote:
>>>> On Mon, Feb 22, 2016 at 01:30:30PM +0100, Juergen Gross wrote:
>>>>> On 22/02/16 13:18, Daniel Kiper wrote:
>>>>>> On Mon, Feb 22, 2016 at 10:29:04AM +0100, Juergen Gross wrote:
>>>>>>> On 22/02/16 10:17, Daniel Kiper wrote:
>>>>>>>> On Mon, Feb 22, 2016 at 07:03:18AM +0100, Juergen Gross wrote:
>>>>>>>>> diff --git a/grub-core/lib/xen/relocator.c b/grub-core/lib/xen/relocator.c
>>>>>>>>> index 8f427d3..a05b253 100644
>>>>>>>>> --- a/grub-core/lib/xen/relocator.c
>>>>>>>>> +++ b/grub-core/lib/xen/relocator.c
>>>>>>>>> @@ -29,6 +29,11 @@
>>>>>>>>>
>>>>>>>>> typedef grub_addr_t grub_xen_reg_t;
>>>>>>>>>
>>>>>>>>> +struct grub_relocator_xen_paging_area {
>>>>>>>>> + grub_xen_reg_t start;
>>>>>>>>> + grub_xen_reg_t size;
>>>>>>>>> +};
>>>>>>>>> +
>>>>>>>>
>>>>>>>> ... this should have GRUB_PACKED because compiler may
>>>>>>>> add padding to align size member.
>>>>>>>
>>>>>>> Why would the compiler add padding to a structure containing two items
>>>>>>> of the same type? I don't think the C standard would allow this.
>>>>>>>
>>>>>>> grub_xen_reg_t is either unsigned (32 bit) or unsigned long (64 bit).
>>>>>>> There is no way this could require any padding.
>>>>>>
>>>>>> You are right but we should add this here just in case.
>>>>>
>>>>> Sorry, I don't think this makes any sense. The C standard is very clear
>>>>> in this case: a type requiring a special alignment has always a length
>>>>> being a multiple of that alignment. Otherwise arrays wouldn't work.
>>>>
>>>> Sorry, I am not sure what do you mean by that.
>>>
>>> The size of any C type (no matter whether it is an integral type like
>>> "int" or a structure) has always the same alignment restriction as the
>>> type itself. So a type requiring 8 byte alignment will always have a
>>> size of a multiple of 8 bytes. This is mandatory for arrays to work, as
>>> otherwise either the elements wouldn't be placed consecutively in memory
>>> or the alignment restrictions wouldn't be obeyed for all elements.
>>>
>>
>> I too not follow how it is relevant to this case. We talk about internal
>> padding between structure members, not between array elements.
>>
>>> For our case it means that two structure elements of the same type will
>>> never require a padding between them, thus the annotation with "packed"
>>> can't serve any purpose.
>>>
>>
>> Well, I am not aware of any requirement. Compiler may add arbitrary
>> padding between structure elements; it is only prohibited to add padding
>> at the beginning. Sure, it would be unusual, but never say "never" ...
>> also should Xen ever be ported to architecture where types are not
>> self-aligned it will become an issue.
>
> So you are telling me that _all_ interfaces between e.g. Linux, grub2,
> Xen and all wire protocols not attributed with "packed" are just wrong?
>
> Sorry, I don't think this is true.
Okay, just found a reference: The x86 ABI states:
Aggregates and Unions
---------------------
Structures and unions assume the alignment of their most strictly
aligned component. Each member is assigned to the lowest available
offset with the appropriate alignment. The size of any object is always
a multiple of the object‘s alignment.
I don't think any x86 C-compiler will violate the x86 ABI.
Juergen
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 33+ messages in thread[parent not found: <56D4374F.5050209@suse.com>]
* Re: [PATCH v4 10/11] xen: modify page table construction
[not found] ` <56D4374F.5050209@suse.com>
@ 2016-03-01 3:52 ` Andrei Borzenkov
[not found] ` <56D511EA.4000503@gmail.com>
` (2 subsequent siblings)
3 siblings, 0 replies; 33+ messages in thread
From: Andrei Borzenkov @ 2016-03-01 3:52 UTC (permalink / raw)
To: The development of GNU GRUB, Daniel Kiper; +Cc: phcoder, mchang, xen-devel
29.02.2016 15:19, Juergen Gross пишет:
> On 29/02/16 10:13, Juergen Gross wrote:
>> On 25/02/16 19:33, Andrei Borzenkov wrote:
>>> 22.02.2016 16:14, Juergen Gross пишет:
>>>> On 22/02/16 13:48, Daniel Kiper wrote:
>>>>> On Mon, Feb 22, 2016 at 01:30:30PM +0100, Juergen Gross wrote:
>>>>>> On 22/02/16 13:18, Daniel Kiper wrote:
>>>>>>> On Mon, Feb 22, 2016 at 10:29:04AM +0100, Juergen Gross wrote:
>>>>>>>> On 22/02/16 10:17, Daniel Kiper wrote:
>>>>>>>>> On Mon, Feb 22, 2016 at 07:03:18AM +0100, Juergen Gross wrote:
>>>>>>>>>> diff --git a/grub-core/lib/xen/relocator.c b/grub-core/lib/xen/relocator.c
>>>>>>>>>> index 8f427d3..a05b253 100644
>>>>>>>>>> --- a/grub-core/lib/xen/relocator.c
>>>>>>>>>> +++ b/grub-core/lib/xen/relocator.c
>>>>>>>>>> @@ -29,6 +29,11 @@
>>>>>>>>>>
>>>>>>>>>> typedef grub_addr_t grub_xen_reg_t;
>>>>>>>>>>
>>>>>>>>>> +struct grub_relocator_xen_paging_area {
>>>>>>>>>> + grub_xen_reg_t start;
>>>>>>>>>> + grub_xen_reg_t size;
>>>>>>>>>> +};
>>>>>>>>>> +
>>>>>>>>>
>>>>>>>>> ... this should have GRUB_PACKED because compiler may
>>>>>>>>> add padding to align size member.
>>>>>>>>
>>>>>>>> Why would the compiler add padding to a structure containing two items
>>>>>>>> of the same type? I don't think the C standard would allow this.
>>>>>>>>
>>>>>>>> grub_xen_reg_t is either unsigned (32 bit) or unsigned long (64 bit).
>>>>>>>> There is no way this could require any padding.
>>>>>>>
>>>>>>> You are right but we should add this here just in case.
>>>>>>
>>>>>> Sorry, I don't think this makes any sense. The C standard is very clear
>>>>>> in this case: a type requiring a special alignment has always a length
>>>>>> being a multiple of that alignment. Otherwise arrays wouldn't work.
>>>>>
>>>>> Sorry, I am not sure what do you mean by that.
>>>>
>>>> The size of any C type (no matter whether it is an integral type like
>>>> "int" or a structure) has always the same alignment restriction as the
>>>> type itself. So a type requiring 8 byte alignment will always have a
>>>> size of a multiple of 8 bytes. This is mandatory for arrays to work, as
>>>> otherwise either the elements wouldn't be placed consecutively in memory
>>>> or the alignment restrictions wouldn't be obeyed for all elements.
>>>>
>>>
>>> I too not follow how it is relevant to this case. We talk about internal
>>> padding between structure members, not between array elements.
>>>
>>>> For our case it means that two structure elements of the same type will
>>>> never require a padding between them, thus the annotation with "packed"
>>>> can't serve any purpose.
>>>>
>>>
>>> Well, I am not aware of any requirement. Compiler may add arbitrary
>>> padding between structure elements; it is only prohibited to add padding
>>> at the beginning. Sure, it would be unusual, but never say "never" ...
>>> also should Xen ever be ported to architecture where types are not
>>> self-aligned it will become an issue.
>>
>> So you are telling me that _all_ interfaces between e.g. Linux, grub2,
>> Xen and all wire protocols not attributed with "packed" are just wrong?
>>
>> Sorry, I don't think this is true.
>
> Okay, just found a reference: The x86 ABI states:
>
> Aggregates and Unions
> ---------------------
> Structures and unions assume the alignment of their most strictly
> aligned component. Each member is assigned to the lowest available
> offset with the appropriate alignment. The size of any object is always
> a multiple of the object‘s alignment.
>
> I don't think any x86 C-compiler will violate the x86 ABI.
>
Thank you! I was not really objecting, more thinking loud, because I
missed such explicit statemrnt. Could you post link?
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 33+ messages in thread[parent not found: <56D511EA.4000503@gmail.com>]
* Re: [PATCH v4 10/11] xen: modify page table construction
[not found] ` <56D511EA.4000503@gmail.com>
@ 2016-03-01 5:12 ` Juergen Gross
0 siblings, 0 replies; 33+ messages in thread
From: Juergen Gross @ 2016-03-01 5:12 UTC (permalink / raw)
To: Andrei Borzenkov, The development of GNU GRUB, Daniel Kiper
Cc: phcoder, mchang, xen-devel
On 01/03/16 04:52, Andrei Borzenkov wrote:
> 29.02.2016 15:19, Juergen Gross пишет:
>> On 29/02/16 10:13, Juergen Gross wrote:
>>> On 25/02/16 19:33, Andrei Borzenkov wrote:
>>>> 22.02.2016 16:14, Juergen Gross пишет:
>>>>> On 22/02/16 13:48, Daniel Kiper wrote:
>>>>>> On Mon, Feb 22, 2016 at 01:30:30PM +0100, Juergen Gross wrote:
>>>>>>> On 22/02/16 13:18, Daniel Kiper wrote:
>>>>>>>> On Mon, Feb 22, 2016 at 10:29:04AM +0100, Juergen Gross wrote:
>>>>>>>>> On 22/02/16 10:17, Daniel Kiper wrote:
>>>>>>>>>> On Mon, Feb 22, 2016 at 07:03:18AM +0100, Juergen Gross wrote:
>>>>>>>>>>> diff --git a/grub-core/lib/xen/relocator.c b/grub-core/lib/xen/relocator.c
>>>>>>>>>>> index 8f427d3..a05b253 100644
>>>>>>>>>>> --- a/grub-core/lib/xen/relocator.c
>>>>>>>>>>> +++ b/grub-core/lib/xen/relocator.c
>>>>>>>>>>> @@ -29,6 +29,11 @@
>>>>>>>>>>>
>>>>>>>>>>> typedef grub_addr_t grub_xen_reg_t;
>>>>>>>>>>>
>>>>>>>>>>> +struct grub_relocator_xen_paging_area {
>>>>>>>>>>> + grub_xen_reg_t start;
>>>>>>>>>>> + grub_xen_reg_t size;
>>>>>>>>>>> +};
>>>>>>>>>>> +
>>>>>>>>>>
>>>>>>>>>> ... this should have GRUB_PACKED because compiler may
>>>>>>>>>> add padding to align size member.
>>>>>>>>>
>>>>>>>>> Why would the compiler add padding to a structure containing two items
>>>>>>>>> of the same type? I don't think the C standard would allow this.
>>>>>>>>>
>>>>>>>>> grub_xen_reg_t is either unsigned (32 bit) or unsigned long (64 bit).
>>>>>>>>> There is no way this could require any padding.
>>>>>>>>
>>>>>>>> You are right but we should add this here just in case.
>>>>>>>
>>>>>>> Sorry, I don't think this makes any sense. The C standard is very clear
>>>>>>> in this case: a type requiring a special alignment has always a length
>>>>>>> being a multiple of that alignment. Otherwise arrays wouldn't work.
>>>>>>
>>>>>> Sorry, I am not sure what do you mean by that.
>>>>>
>>>>> The size of any C type (no matter whether it is an integral type like
>>>>> "int" or a structure) has always the same alignment restriction as the
>>>>> type itself. So a type requiring 8 byte alignment will always have a
>>>>> size of a multiple of 8 bytes. This is mandatory for arrays to work, as
>>>>> otherwise either the elements wouldn't be placed consecutively in memory
>>>>> or the alignment restrictions wouldn't be obeyed for all elements.
>>>>>
>>>>
>>>> I too not follow how it is relevant to this case. We talk about internal
>>>> padding between structure members, not between array elements.
>>>>
>>>>> For our case it means that two structure elements of the same type will
>>>>> never require a padding between them, thus the annotation with "packed"
>>>>> can't serve any purpose.
>>>>>
>>>>
>>>> Well, I am not aware of any requirement. Compiler may add arbitrary
>>>> padding between structure elements; it is only prohibited to add padding
>>>> at the beginning. Sure, it would be unusual, but never say "never" ...
>>>> also should Xen ever be ported to architecture where types are not
>>>> self-aligned it will become an issue.
>>>
>>> So you are telling me that _all_ interfaces between e.g. Linux, grub2,
>>> Xen and all wire protocols not attributed with "packed" are just wrong?
>>>
>>> Sorry, I don't think this is true.
>>
>> Okay, just found a reference: The x86 ABI states:
>>
>> Aggregates and Unions
>> ---------------------
>> Structures and unions assume the alignment of their most strictly
>> aligned component. Each member is assigned to the lowest available
>> offset with the appropriate alignment. The size of any object is always
>> a multiple of the object‘s alignment.
>>
>> I don't think any x86 C-compiler will violate the x86 ABI.
>>
>
> Thank you! I was not really objecting, more thinking loud, because I
> missed such explicit statemrnt. Could you post link?
http://www.x86-64.org/documentation/abi.pdf
Chapter 3.1.2, page 13
Juergen
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v4 10/11] xen: modify page table construction
[not found] ` <56D4374F.5050209@suse.com>
2016-03-01 3:52 ` Andrei Borzenkov
[not found] ` <56D511EA.4000503@gmail.com>
@ 2016-03-02 9:12 ` Daniel Kiper
[not found] ` <20160302091248.GH3500@olila.local.net-space.pl>
3 siblings, 0 replies; 33+ messages in thread
From: Daniel Kiper @ 2016-03-02 9:12 UTC (permalink / raw)
To: Juergen Gross; +Cc: The development of GNU GRUB, mchang, xen-devel, phcoder
On Mon, Feb 29, 2016 at 01:19:27PM +0100, Juergen Gross wrote:
> On 29/02/16 10:13, Juergen Gross wrote:
> > On 25/02/16 19:33, Andrei Borzenkov wrote:
> >> 22.02.2016 16:14, Juergen Gross пишет:
> >>> On 22/02/16 13:48, Daniel Kiper wrote:
> >>>> On Mon, Feb 22, 2016 at 01:30:30PM +0100, Juergen Gross wrote:
> >>>>> On 22/02/16 13:18, Daniel Kiper wrote:
> >>>>>> On Mon, Feb 22, 2016 at 10:29:04AM +0100, Juergen Gross wrote:
> >>>>>>> On 22/02/16 10:17, Daniel Kiper wrote:
> >>>>>>>> On Mon, Feb 22, 2016 at 07:03:18AM +0100, Juergen Gross wrote:
> >>>>>>>>> diff --git a/grub-core/lib/xen/relocator.c b/grub-core/lib/xen/relocator.c
> >>>>>>>>> index 8f427d3..a05b253 100644
> >>>>>>>>> --- a/grub-core/lib/xen/relocator.c
> >>>>>>>>> +++ b/grub-core/lib/xen/relocator.c
> >>>>>>>>> @@ -29,6 +29,11 @@
> >>>>>>>>>
> >>>>>>>>> typedef grub_addr_t grub_xen_reg_t;
> >>>>>>>>>
> >>>>>>>>> +struct grub_relocator_xen_paging_area {
> >>>>>>>>> + grub_xen_reg_t start;
> >>>>>>>>> + grub_xen_reg_t size;
> >>>>>>>>> +};
> >>>>>>>>> +
> >>>>>>>>
> >>>>>>>> ... this should have GRUB_PACKED because compiler may
> >>>>>>>> add padding to align size member.
> >>>>>>>
> >>>>>>> Why would the compiler add padding to a structure containing two items
> >>>>>>> of the same type? I don't think the C standard would allow this.
> >>>>>>>
> >>>>>>> grub_xen_reg_t is either unsigned (32 bit) or unsigned long (64 bit).
> >>>>>>> There is no way this could require any padding.
> >>>>>>
> >>>>>> You are right but we should add this here just in case.
> >>>>>
> >>>>> Sorry, I don't think this makes any sense. The C standard is very clear
> >>>>> in this case: a type requiring a special alignment has always a length
> >>>>> being a multiple of that alignment. Otherwise arrays wouldn't work.
> >>>>
> >>>> Sorry, I am not sure what do you mean by that.
> >>>
> >>> The size of any C type (no matter whether it is an integral type like
> >>> "int" or a structure) has always the same alignment restriction as the
> >>> type itself. So a type requiring 8 byte alignment will always have a
> >>> size of a multiple of 8 bytes. This is mandatory for arrays to work, as
> >>> otherwise either the elements wouldn't be placed consecutively in memory
> >>> or the alignment restrictions wouldn't be obeyed for all elements.
> >>>
> >>
> >> I too not follow how it is relevant to this case. We talk about internal
> >> padding between structure members, not between array elements.
> >>
> >>> For our case it means that two structure elements of the same type will
> >>> never require a padding between them, thus the annotation with "packed"
> >>> can't serve any purpose.
> >>>
> >>
> >> Well, I am not aware of any requirement. Compiler may add arbitrary
> >> padding between structure elements; it is only prohibited to add padding
> >> at the beginning. Sure, it would be unusual, but never say "never" ...
> >> also should Xen ever be ported to architecture where types are not
> >> self-aligned it will become an issue.
> >
> > So you are telling me that _all_ interfaces between e.g. Linux, grub2,
> > Xen and all wire protocols not attributed with "packed" are just wrong?
> >
> > Sorry, I don't think this is true.
>
> Okay, just found a reference: The x86 ABI states:
>
> Aggregates and Unions
> ---------------------
> Structures and unions assume the alignment of their most strictly
> aligned component. Each member is assigned to the lowest available
> offset with the appropriate alignment. The size of any object is always
> a multiple of the object‘s alignment.
>
> I don't think any x86 C-compiler will violate the x86 ABI.
You just cited only part of paragraph. Here is full paragraph:
[...]
Aggregates and Unions
Structures and unions assume the alignment of their most strictly aligned component.
Each member is assigned to the lowest available offset with the appropriate
alignment. The size of any object is always a multiple of the object‘s alignment.
An array uses the same alignment as its elements, except that a local or global
array variable of length at least 16 bytes or a C99 variable-length array variable
always has alignment of at least 16 bytes.
Structure and union objects can require padding to meet size and alignment
constraints. The contents of any padding is undefined.
[...]
Well, this is a bit hard to understand, so, please look here
http://www.catb.org/esr/structure-packing/#_structure_alignment_and_padding
what can happen if struct has members with different sizes and you do
not use packed attribute.
Luckily you use struct members with the same sizes, so, everything works.
However, if you/somebody will try to change grub_relocator_xen_paging_area
layout and add a member with different size in the middle or the beginning
of struct then suddenly everything will stop working. So, I think that in
cases when you create an interface between C and assembly using struct you
should use packed attribute (or separate variables if it makes sense). Even
if it works without it right now. Please do it to save your and others time
for more useful things than debugging issues like lack of packed attr.
Additionally, I still think that you do not need alignment before
grub_relocator_xen_paging_area struct struct in assembly file.
Daniel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 33+ messages in thread[parent not found: <20160302091248.GH3500@olila.local.net-space.pl>]
* Re: [PATCH v4 10/11] xen: modify page table construction
[not found] ` <20160302091248.GH3500@olila.local.net-space.pl>
@ 2016-03-02 15:43 ` Juergen Gross
[not found] ` <56D70A0B.9000704@suse.com>
1 sibling, 0 replies; 33+ messages in thread
From: Juergen Gross @ 2016-03-02 15:43 UTC (permalink / raw)
To: Daniel Kiper; +Cc: The development of GNU GRUB, mchang, xen-devel, phcoder
On 02/03/16 10:12, Daniel Kiper wrote:
> On Mon, Feb 29, 2016 at 01:19:27PM +0100, Juergen Gross wrote:
>> On 29/02/16 10:13, Juergen Gross wrote:
>>> On 25/02/16 19:33, Andrei Borzenkov wrote:
>>>> 22.02.2016 16:14, Juergen Gross пишет:
>>>>> On 22/02/16 13:48, Daniel Kiper wrote:
>>>>>> On Mon, Feb 22, 2016 at 01:30:30PM +0100, Juergen Gross wrote:
>>>>>>> On 22/02/16 13:18, Daniel Kiper wrote:
>>>>>>>> On Mon, Feb 22, 2016 at 10:29:04AM +0100, Juergen Gross wrote:
>>>>>>>>> On 22/02/16 10:17, Daniel Kiper wrote:
>>>>>>>>>> On Mon, Feb 22, 2016 at 07:03:18AM +0100, Juergen Gross wrote:
>>>>>>>>>>> diff --git a/grub-core/lib/xen/relocator.c b/grub-core/lib/xen/relocator.c
>>>>>>>>>>> index 8f427d3..a05b253 100644
>>>>>>>>>>> --- a/grub-core/lib/xen/relocator.c
>>>>>>>>>>> +++ b/grub-core/lib/xen/relocator.c
>>>>>>>>>>> @@ -29,6 +29,11 @@
>>>>>>>>>>>
>>>>>>>>>>> typedef grub_addr_t grub_xen_reg_t;
>>>>>>>>>>>
>>>>>>>>>>> +struct grub_relocator_xen_paging_area {
>>>>>>>>>>> + grub_xen_reg_t start;
>>>>>>>>>>> + grub_xen_reg_t size;
>>>>>>>>>>> +};
>>>>>>>>>>> +
>>>>>>>>>>
>>>>>>>>>> ... this should have GRUB_PACKED because compiler may
>>>>>>>>>> add padding to align size member.
>>>>>>>>>
>>>>>>>>> Why would the compiler add padding to a structure containing two items
>>>>>>>>> of the same type? I don't think the C standard would allow this.
>>>>>>>>>
>>>>>>>>> grub_xen_reg_t is either unsigned (32 bit) or unsigned long (64 bit).
>>>>>>>>> There is no way this could require any padding.
>>>>>>>>
>>>>>>>> You are right but we should add this here just in case.
>>>>>>>
>>>>>>> Sorry, I don't think this makes any sense. The C standard is very clear
>>>>>>> in this case: a type requiring a special alignment has always a length
>>>>>>> being a multiple of that alignment. Otherwise arrays wouldn't work.
>>>>>>
>>>>>> Sorry, I am not sure what do you mean by that.
>>>>>
>>>>> The size of any C type (no matter whether it is an integral type like
>>>>> "int" or a structure) has always the same alignment restriction as the
>>>>> type itself. So a type requiring 8 byte alignment will always have a
>>>>> size of a multiple of 8 bytes. This is mandatory for arrays to work, as
>>>>> otherwise either the elements wouldn't be placed consecutively in memory
>>>>> or the alignment restrictions wouldn't be obeyed for all elements.
>>>>>
>>>>
>>>> I too not follow how it is relevant to this case. We talk about internal
>>>> padding between structure members, not between array elements.
>>>>
>>>>> For our case it means that two structure elements of the same type will
>>>>> never require a padding between them, thus the annotation with "packed"
>>>>> can't serve any purpose.
>>>>>
>>>>
>>>> Well, I am not aware of any requirement. Compiler may add arbitrary
>>>> padding between structure elements; it is only prohibited to add padding
>>>> at the beginning. Sure, it would be unusual, but never say "never" ...
>>>> also should Xen ever be ported to architecture where types are not
>>>> self-aligned it will become an issue.
>>>
>>> So you are telling me that _all_ interfaces between e.g. Linux, grub2,
>>> Xen and all wire protocols not attributed with "packed" are just wrong?
>>>
>>> Sorry, I don't think this is true.
>>
>> Okay, just found a reference: The x86 ABI states:
>>
>> Aggregates and Unions
>> ---------------------
>> Structures and unions assume the alignment of their most strictly
>> aligned component. Each member is assigned to the lowest available
>> offset with the appropriate alignment. The size of any object is always
>> a multiple of the object‘s alignment.
>>
>> I don't think any x86 C-compiler will violate the x86 ABI.
>
> You just cited only part of paragraph. Here is full paragraph:
>
> [...]
>
> Aggregates and Unions
>
> Structures and unions assume the alignment of their most strictly aligned component.
> Each member is assigned to the lowest available offset with the appropriate
> alignment. The size of any object is always a multiple of the object‘s alignment.
> An array uses the same alignment as its elements, except that a local or global
> array variable of length at least 16 bytes or a C99 variable-length array variable
> always has alignment of at least 16 bytes.
> Structure and union objects can require padding to meet size and alignment
> constraints. The contents of any padding is undefined.
>
> [...]
>
> Well, this is a bit hard to understand, so, please look here
> http://www.catb.org/esr/structure-packing/#_structure_alignment_and_padding
> what can happen if struct has members with different sizes and you do
> not use packed attribute.
>
> Luckily you use struct members with the same sizes, so, everything works.
This wasn't luck, it was on purpose. ;-)
> However, if you/somebody will try to change grub_relocator_xen_paging_area
> layout and add a member with different size in the middle or the beginning
> of struct then suddenly everything will stop working. So, I think that in
Of course. It doesn't matter whether the sizes are different or where
the new item is introduced. Every change of this structure needs to be
reflected in the assembly file.
> cases when you create an interface between C and assembly using struct you
> should use packed attribute (or separate variables if it makes sense). Even
> if it works without it right now. Please do it to save your and others time
> for more useful things than debugging issues like lack of packed attr.
I'm tired of this discussion. I'll add the packed attribute if you are
insisting on it, even if I still don't see the need.
> Additionally, I still think that you do not need alignment before
> grub_relocator_xen_paging_area struct struct in assembly file.
I don't need it. It's just "best practice" (why does the compiler
align variables?). I'll drop those as well, just to get the patch in.
Juergen
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 33+ messages in thread[parent not found: <56D70A0B.9000704@suse.com>]
* Re: [PATCH v4 10/11] xen: modify page table construction
[not found] ` <56D70A0B.9000704@suse.com>
@ 2016-03-02 16:04 ` Daniel Kiper
0 siblings, 0 replies; 33+ messages in thread
From: Daniel Kiper @ 2016-03-02 16:04 UTC (permalink / raw)
To: Juergen Gross; +Cc: The development of GNU GRUB, mchang, xen-devel, phcoder
On Wed, Mar 02, 2016 at 04:43:07PM +0100, Juergen Gross wrote:
> On 02/03/16 10:12, Daniel Kiper wrote:
> > On Mon, Feb 29, 2016 at 01:19:27PM +0100, Juergen Gross wrote:
> >> On 29/02/16 10:13, Juergen Gross wrote:
> >>> On 25/02/16 19:33, Andrei Borzenkov wrote:
> >>>> 22.02.2016 16:14, Juergen Gross пишет:
> >>>>> On 22/02/16 13:48, Daniel Kiper wrote:
> >>>>>> On Mon, Feb 22, 2016 at 01:30:30PM +0100, Juergen Gross wrote:
> >>>>>>> On 22/02/16 13:18, Daniel Kiper wrote:
> >>>>>>>> On Mon, Feb 22, 2016 at 10:29:04AM +0100, Juergen Gross wrote:
> >>>>>>>>> On 22/02/16 10:17, Daniel Kiper wrote:
> >>>>>>>>>> On Mon, Feb 22, 2016 at 07:03:18AM +0100, Juergen Gross wrote:
> >>>>>>>>>>> diff --git a/grub-core/lib/xen/relocator.c b/grub-core/lib/xen/relocator.c
> >>>>>>>>>>> index 8f427d3..a05b253 100644
> >>>>>>>>>>> --- a/grub-core/lib/xen/relocator.c
> >>>>>>>>>>> +++ b/grub-core/lib/xen/relocator.c
> >>>>>>>>>>> @@ -29,6 +29,11 @@
> >>>>>>>>>>>
> >>>>>>>>>>> typedef grub_addr_t grub_xen_reg_t;
> >>>>>>>>>>>
> >>>>>>>>>>> +struct grub_relocator_xen_paging_area {
> >>>>>>>>>>> + grub_xen_reg_t start;
> >>>>>>>>>>> + grub_xen_reg_t size;
> >>>>>>>>>>> +};
> >>>>>>>>>>> +
> >>>>>>>>>>
> >>>>>>>>>> ... this should have GRUB_PACKED because compiler may
> >>>>>>>>>> add padding to align size member.
> >>>>>>>>>
> >>>>>>>>> Why would the compiler add padding to a structure containing two items
> >>>>>>>>> of the same type? I don't think the C standard would allow this.
> >>>>>>>>>
> >>>>>>>>> grub_xen_reg_t is either unsigned (32 bit) or unsigned long (64 bit).
> >>>>>>>>> There is no way this could require any padding.
> >>>>>>>>
> >>>>>>>> You are right but we should add this here just in case.
> >>>>>>>
> >>>>>>> Sorry, I don't think this makes any sense. The C standard is very clear
> >>>>>>> in this case: a type requiring a special alignment has always a length
> >>>>>>> being a multiple of that alignment. Otherwise arrays wouldn't work.
> >>>>>>
> >>>>>> Sorry, I am not sure what do you mean by that.
> >>>>>
> >>>>> The size of any C type (no matter whether it is an integral type like
> >>>>> "int" or a structure) has always the same alignment restriction as the
> >>>>> type itself. So a type requiring 8 byte alignment will always have a
> >>>>> size of a multiple of 8 bytes. This is mandatory for arrays to work, as
> >>>>> otherwise either the elements wouldn't be placed consecutively in memory
> >>>>> or the alignment restrictions wouldn't be obeyed for all elements.
> >>>>>
> >>>>
> >>>> I too not follow how it is relevant to this case. We talk about internal
> >>>> padding between structure members, not between array elements.
> >>>>
> >>>>> For our case it means that two structure elements of the same type will
> >>>>> never require a padding between them, thus the annotation with "packed"
> >>>>> can't serve any purpose.
> >>>>>
> >>>>
> >>>> Well, I am not aware of any requirement. Compiler may add arbitrary
> >>>> padding between structure elements; it is only prohibited to add padding
> >>>> at the beginning. Sure, it would be unusual, but never say "never" ...
> >>>> also should Xen ever be ported to architecture where types are not
> >>>> self-aligned it will become an issue.
> >>>
> >>> So you are telling me that _all_ interfaces between e.g. Linux, grub2,
> >>> Xen and all wire protocols not attributed with "packed" are just wrong?
> >>>
> >>> Sorry, I don't think this is true.
> >>
> >> Okay, just found a reference: The x86 ABI states:
> >>
> >> Aggregates and Unions
> >> ---------------------
> >> Structures and unions assume the alignment of their most strictly
> >> aligned component. Each member is assigned to the lowest available
> >> offset with the appropriate alignment. The size of any object is always
> >> a multiple of the object‘s alignment.
> >>
> >> I don't think any x86 C-compiler will violate the x86 ABI.
> >
> > You just cited only part of paragraph. Here is full paragraph:
> >
> > [...]
> >
> > Aggregates and Unions
> >
> > Structures and unions assume the alignment of their most strictly aligned component.
> > Each member is assigned to the lowest available offset with the appropriate
> > alignment. The size of any object is always a multiple of the object‘s alignment.
> > An array uses the same alignment as its elements, except that a local or global
> > array variable of length at least 16 bytes or a C99 variable-length array variable
> > always has alignment of at least 16 bytes.
> > Structure and union objects can require padding to meet size and alignment
> > constraints. The contents of any padding is undefined.
> >
> > [...]
> >
> > Well, this is a bit hard to understand, so, please look here
> > http://www.catb.org/esr/structure-packing/#_structure_alignment_and_padding
> > what can happen if struct has members with different sizes and you do
> > not use packed attribute.
> >
> > Luckily you use struct members with the same sizes, so, everything works.
>
> This wasn't luck, it was on purpose. ;-)
>
> > However, if you/somebody will try to change grub_relocator_xen_paging_area
> > layout and add a member with different size in the middle or the beginning
> > of struct then suddenly everything will stop working. So, I think that in
>
> Of course. It doesn't matter whether the sizes are different or where
> the new item is introduced. Every change of this structure needs to be
> reflected in the assembly file.
>
> > cases when you create an interface between C and assembly using struct you
> > should use packed attribute (or separate variables if it makes sense). Even
> > if it works without it right now. Please do it to save your and others time
> > for more useful things than debugging issues like lack of packed attr.
>
> I'm tired of this discussion. I'll add the packed attribute if you are
> insisting on it, even if I still don't see the need.
>
> > Additionally, I still think that you do not need alignment before
> > grub_relocator_xen_paging_area struct struct in assembly file.
>
> I don't need it. It's just "best practice" (why does the compiler
> align variables?). I'll drop those as well, just to get the patch in.
Thanks a lot!
Daniel
PS Shortly I will send my GRUB2 patch series, so, you will have a joy of reviewing... :-)))
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH v4 11/11] xen: add capability to load p2m list outside of kernel mapping
[not found] <1456120999-5639-1-git-send-email-jgross@suse.com>
` (9 preceding siblings ...)
2016-02-22 6:03 ` [PATCH v4 10/11] xen: modify page table construction Juergen Gross
@ 2016-02-22 6:03 ` Juergen Gross
[not found] ` <1456120999-5639-2-git-send-email-jgross@suse.com>
` (2 subsequent siblings)
13 siblings, 0 replies; 33+ messages in thread
From: Juergen Gross @ 2016-02-22 6:03 UTC (permalink / raw)
To: grub-devel; +Cc: Juergen Gross, phcoder, daniel.kiper, mchang, xen-devel
Modern pvops linux kernels support a p2m list not covered by the
kernel mapping. This capability is flagged by an elf-note specifying
the virtual address the kernel is expecting the p2m list to be mapped
to.
In case the elf-note is set by the kernel don't place the p2m list
into the kernel mapping, but map it to the given address. This will
allow to support domains with larger memory, as the kernel mapping is
limited to 2GB and a domain with huge memory in the TB range will have
a p2m list larger than this.
Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
---
grub-core/loader/i386/xen.c | 53 +++++++++++++++++++++++++++++++-------
grub-core/loader/i386/xen_fileXX.c | 4 +++
include/grub/xen_file.h | 2 ++
3 files changed, 50 insertions(+), 9 deletions(-)
diff --git a/grub-core/loader/i386/xen.c b/grub-core/loader/i386/xen.c
index 4eadc47..7945c1b 100644
--- a/grub-core/loader/i386/xen.c
+++ b/grub-core/loader/i386/xen.c
@@ -316,21 +316,47 @@ static grub_err_t
grub_xen_p2m_alloc (void)
{
grub_relocator_chunk_t ch;
- grub_size_t p2msize;
+ grub_size_t p2msize, p2malloc;
grub_err_t err;
+ struct grub_xen_mapping *map;
+
+ map = xen_state.mappings + xen_state.n_mappings;
+ p2msize = ALIGN_UP (sizeof (grub_xen_mfn_t) *
+ grub_xen_start_page_addr->nr_pages, PAGE_SIZE);
+ if (xen_state.xen_inf.has_p2m_base)
+ {
+ err = get_pgtable_size (xen_state.xen_inf.p2m_base,
+ xen_state.xen_inf.p2m_base + p2msize,
+ (xen_state.max_addr + p2msize) >> PAGE_SHIFT);
+ if (err)
+ return err;
+
+ map->area.pfn_start = xen_state.max_addr >> PAGE_SHIFT;
+ p2malloc = p2msize + page2offset (map->area.n_pt_pages);
+ xen_state.n_mappings++;
+ xen_state.next_start.mfn_list = xen_state.xen_inf.p2m_base;
+ xen_state.next_start.first_p2m_pfn = map->area.pfn_start;
+ xen_state.next_start.nr_p2m_frames = p2malloc >> PAGE_SHIFT;
+ }
+ else
+ {
+ xen_state.next_start.mfn_list =
+ xen_state.max_addr + xen_state.xen_inf.virt_base;
+ p2malloc = p2msize;
+ }
xen_state.state.mfn_list = xen_state.max_addr;
- xen_state.next_start.mfn_list =
- xen_state.max_addr + xen_state.xen_inf.virt_base;
- p2msize = sizeof (grub_xen_mfn_t) * grub_xen_start_page_addr->nr_pages;
err = grub_relocator_alloc_chunk_addr (xen_state.relocator, &ch,
- xen_state.max_addr, p2msize);
+ xen_state.max_addr, p2malloc);
if (err)
return err;
xen_state.virt_mfn_list = get_virtual_current_address (ch);
+ if (xen_state.xen_inf.has_p2m_base)
+ map->where = (grub_uint64_t *) xen_state.virt_mfn_list +
+ p2msize / sizeof (grub_uint64_t);
grub_memcpy (xen_state.virt_mfn_list,
(void *) grub_xen_start_page_addr->mfn_list, p2msize);
- xen_state.max_addr = ALIGN_UP (xen_state.max_addr + p2msize, PAGE_SIZE);
+ xen_state.max_addr += p2malloc;
return GRUB_ERR_NONE;
}
@@ -445,9 +471,12 @@ grub_xen_alloc_final (void)
return GRUB_ERR_NONE;
xen_state.alloc_end_called = 1;
- err = grub_xen_p2m_alloc ();
- if (err)
- return err;
+ if (!xen_state.xen_inf.has_p2m_base)
+ {
+ err = grub_xen_p2m_alloc ();
+ if (err)
+ return err;
+ }
err = grub_xen_special_alloc ();
if (err)
return err;
@@ -472,6 +501,12 @@ grub_xen_boot (void)
err = grub_xen_alloc_final ();
if (err)
return err;
+ if (xen_state.xen_inf.has_p2m_base)
+ {
+ err = grub_xen_p2m_alloc ();
+ if (err)
+ return err;
+ }
err = set_mfns (xen_state.console_pfn);
if (err)
diff --git a/grub-core/loader/i386/xen_fileXX.c b/grub-core/loader/i386/xen_fileXX.c
index d68634d..c92b807 100644
--- a/grub-core/loader/i386/xen_fileXX.c
+++ b/grub-core/loader/i386/xen_fileXX.c
@@ -259,6 +259,10 @@ parse_note (grub_elf_t elf, struct grub_xen_file_info *xi,
descsz == 2 ? 2 : 3) == 0)
xi->arch = GRUB_XEN_FILE_I386;
break;
+ case XEN_ELFNOTE_INIT_P2M:
+ xi->p2m_base = grub_le_to_cpu_addr (*(Elf_Addr *) desc);
+ xi->has_p2m_base = 1;
+ break;
case XEN_ELFNOTE_MOD_START_PFN:
xi->unmapped_initrd = !!grub_le_to_cpu32(*(grub_uint32_t *) desc);
break;
diff --git a/include/grub/xen_file.h b/include/grub/xen_file.h
index ed749fa..6587999 100644
--- a/include/grub/xen_file.h
+++ b/include/grub/xen_file.h
@@ -32,9 +32,11 @@ struct grub_xen_file_info
grub_uint64_t entry_point;
grub_uint64_t hypercall_page;
grub_uint64_t paddr_offset;
+ grub_uint64_t p2m_base;
int has_hypercall_page;
int has_note;
int has_xen_guest;
+ int has_p2m_base;
int extended_cr3;
int unmapped_initrd;
enum
--
2.6.2
^ permalink raw reply related [flat|nested] 33+ messages in thread[parent not found: <1456120999-5639-2-git-send-email-jgross@suse.com>]
[parent not found: <1456120999-5639-3-git-send-email-jgross@suse.com>]
* Re: [PATCH v4 02/11] xen: avoid memleaks on error
[not found] ` <1456120999-5639-3-git-send-email-jgross@suse.com>
@ 2016-02-22 8:24 ` Daniel Kiper
[not found] ` <20160222082432.GM3482@olila.local.net-space.pl>
1 sibling, 0 replies; 33+ messages in thread
From: Daniel Kiper @ 2016-02-22 8:24 UTC (permalink / raw)
To: Juergen Gross; +Cc: grub-devel, phcoder, mchang, xen-devel
On Mon, Feb 22, 2016 at 07:03:10AM +0100, Juergen Gross wrote:
> When loading a Xen pv-kernel avoid memory leaks in case of errors.
>
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
> grub-core/loader/i386/xen.c | 2 +-
> grub-core/loader/i386/xen_fileXX.c | 17 +++++++++++------
> 2 files changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/grub-core/loader/i386/xen.c b/grub-core/loader/i386/xen.c
> index 179e89c..f45f70f 100644
> --- a/grub-core/loader/i386/xen.c
> +++ b/grub-core/loader/i386/xen.c
> @@ -563,7 +563,7 @@ grub_cmd_initrd (grub_command_t cmd __attribute__ ((unused)),
> {
> err = grub_relocator_alloc_chunk_addr (relocator, &ch, max_addr, size);
> if (err)
> - return err;
> + goto fail;
>
> if (grub_initrd_load (&initrd_ctx, argv,
> get_virtual_current_address (ch)))
> diff --git a/grub-core/loader/i386/xen_fileXX.c b/grub-core/loader/i386/xen_fileXX.c
> index 1ba5649..5475819 100644
> --- a/grub-core/loader/i386/xen_fileXX.c
> +++ b/grub-core/loader/i386/xen_fileXX.c
> @@ -35,7 +35,8 @@ parse_xen_guest (grub_elf_t elf, struct grub_xen_file_info *xi,
> if (grub_file_read (elf->file, buf, sz) != (grub_ssize_t) sz)
> {
> if (grub_errno)
> - return grub_errno;
> + goto out;
> + grub_free (buf);
> return grub_error (GRUB_ERR_BAD_OS, N_("premature end of file %s"),
> elf->file->name);
> }
> @@ -123,14 +124,14 @@ parse_xen_guest (grub_elf_t elf, struct grub_xen_file_info *xi,
> {
> xi->virt_base = grub_strtoull (ptr + sizeof ("VIRT_BASE=") - 1, &ptr, 16);
> if (grub_errno)
> - return grub_errno;
> + goto out;
> continue;
> }
> if (grub_strncmp (ptr, "VIRT_ENTRY=", sizeof ("VIRT_ENTRY=") - 1) == 0)
> {
> xi->entry_point = grub_strtoull (ptr + sizeof ("VIRT_ENTRY=") - 1, &ptr, 16);
> if (grub_errno)
> - return grub_errno;
> + goto out;
> continue;
> }
> if (grub_strncmp (ptr, "HYPERCALL_PAGE=", sizeof ("HYPERCALL_PAGE=") - 1) == 0)
> @@ -138,7 +139,7 @@ parse_xen_guest (grub_elf_t elf, struct grub_xen_file_info *xi,
> xi->hypercall_page = grub_strtoull (ptr + sizeof ("HYPERCALL_PAGE=") - 1, &ptr, 16);
> xi->has_hypercall_page = 1;
> if (grub_errno)
> - return grub_errno;
> + goto out;
> continue;
> }
> if (grub_strncmp (ptr, "ELF_PADDR_OFFSET=", sizeof ("ELF_PADDR_OFFSET=") - 1) == 0)
> @@ -146,7 +147,7 @@ parse_xen_guest (grub_elf_t elf, struct grub_xen_file_info *xi,
> xi->paddr_offset = grub_strtoull (ptr + sizeof ("ELF_PADDR_OFFSET=") - 1, &ptr, 16);
> has_paddr = 1;
> if (grub_errno)
> - return grub_errno;
> + goto out;
> continue;
> }
> }
> @@ -154,7 +155,11 @@ parse_xen_guest (grub_elf_t elf, struct grub_xen_file_info *xi,
> xi->hypercall_page = (xi->hypercall_page << 12) + xi->virt_base;
> if (!has_paddr)
> xi->paddr_offset = xi->virt_base;
> - return GRUB_ERR_NONE;
> +
> +out:
> + grub_free (buf);
> +
> + return grub_errno;
Are you sure that grub_errno is always set to GRUB_ERR_NONE
if any GRUB2 function finished successfully? Maybe you should
set initialize grub_errno with GRUB_ERR_NONE at the beginning
of parse_xen_guest()?
Daniel
^ permalink raw reply [flat|nested] 33+ messages in thread[parent not found: <20160222082432.GM3482@olila.local.net-space.pl>]
* Re: [PATCH v4 02/11] xen: avoid memleaks on error
[not found] ` <20160222082432.GM3482@olila.local.net-space.pl>
@ 2016-02-22 9:06 ` Juergen Gross
2016-02-25 17:38 ` Andrei Borzenkov
1 sibling, 0 replies; 33+ messages in thread
From: Juergen Gross @ 2016-02-22 9:06 UTC (permalink / raw)
To: Daniel Kiper; +Cc: grub-devel, phcoder, mchang, xen-devel
On 22/02/16 09:24, Daniel Kiper wrote:
> On Mon, Feb 22, 2016 at 07:03:10AM +0100, Juergen Gross wrote:
>> When loading a Xen pv-kernel avoid memory leaks in case of errors.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>> grub-core/loader/i386/xen.c | 2 +-
>> grub-core/loader/i386/xen_fileXX.c | 17 +++++++++++------
>> 2 files changed, 12 insertions(+), 7 deletions(-)
>>
>> diff --git a/grub-core/loader/i386/xen.c b/grub-core/loader/i386/xen.c
>> index 179e89c..f45f70f 100644
>> --- a/grub-core/loader/i386/xen.c
>> +++ b/grub-core/loader/i386/xen.c
>> @@ -563,7 +563,7 @@ grub_cmd_initrd (grub_command_t cmd __attribute__ ((unused)),
>> {
>> err = grub_relocator_alloc_chunk_addr (relocator, &ch, max_addr, size);
>> if (err)
>> - return err;
>> + goto fail;
>>
>> if (grub_initrd_load (&initrd_ctx, argv,
>> get_virtual_current_address (ch)))
>> diff --git a/grub-core/loader/i386/xen_fileXX.c b/grub-core/loader/i386/xen_fileXX.c
>> index 1ba5649..5475819 100644
>> --- a/grub-core/loader/i386/xen_fileXX.c
>> +++ b/grub-core/loader/i386/xen_fileXX.c
>> @@ -35,7 +35,8 @@ parse_xen_guest (grub_elf_t elf, struct grub_xen_file_info *xi,
>> if (grub_file_read (elf->file, buf, sz) != (grub_ssize_t) sz)
>> {
>> if (grub_errno)
>> - return grub_errno;
>> + goto out;
>> + grub_free (buf);
>> return grub_error (GRUB_ERR_BAD_OS, N_("premature end of file %s"),
>> elf->file->name);
>> }
>> @@ -123,14 +124,14 @@ parse_xen_guest (grub_elf_t elf, struct grub_xen_file_info *xi,
>> {
>> xi->virt_base = grub_strtoull (ptr + sizeof ("VIRT_BASE=") - 1, &ptr, 16);
>> if (grub_errno)
>> - return grub_errno;
>> + goto out;
>> continue;
>> }
>> if (grub_strncmp (ptr, "VIRT_ENTRY=", sizeof ("VIRT_ENTRY=") - 1) == 0)
>> {
>> xi->entry_point = grub_strtoull (ptr + sizeof ("VIRT_ENTRY=") - 1, &ptr, 16);
>> if (grub_errno)
>> - return grub_errno;
>> + goto out;
>> continue;
>> }
>> if (grub_strncmp (ptr, "HYPERCALL_PAGE=", sizeof ("HYPERCALL_PAGE=") - 1) == 0)
>> @@ -138,7 +139,7 @@ parse_xen_guest (grub_elf_t elf, struct grub_xen_file_info *xi,
>> xi->hypercall_page = grub_strtoull (ptr + sizeof ("HYPERCALL_PAGE=") - 1, &ptr, 16);
>> xi->has_hypercall_page = 1;
>> if (grub_errno)
>> - return grub_errno;
>> + goto out;
>> continue;
>> }
>> if (grub_strncmp (ptr, "ELF_PADDR_OFFSET=", sizeof ("ELF_PADDR_OFFSET=") - 1) == 0)
>> @@ -146,7 +147,7 @@ parse_xen_guest (grub_elf_t elf, struct grub_xen_file_info *xi,
>> xi->paddr_offset = grub_strtoull (ptr + sizeof ("ELF_PADDR_OFFSET=") - 1, &ptr, 16);
>> has_paddr = 1;
>> if (grub_errno)
>> - return grub_errno;
>> + goto out;
>> continue;
>> }
>> }
>> @@ -154,7 +155,11 @@ parse_xen_guest (grub_elf_t elf, struct grub_xen_file_info *xi,
>> xi->hypercall_page = (xi->hypercall_page << 12) + xi->virt_base;
>> if (!has_paddr)
>> xi->paddr_offset = xi->virt_base;
>> - return GRUB_ERR_NONE;
>> +
>> +out:
>> + grub_free (buf);
>> +
>> + return grub_errno;
>
> Are you sure that grub_errno is always set to GRUB_ERR_NONE
> if any GRUB2 function finished successfully? Maybe you should
> set initialize grub_errno with GRUB_ERR_NONE at the beginning
> of parse_xen_guest()?
Yes, probably a good idea.
Juergen
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH v4 02/11] xen: avoid memleaks on error
[not found] ` <20160222082432.GM3482@olila.local.net-space.pl>
2016-02-22 9:06 ` Juergen Gross
@ 2016-02-25 17:38 ` Andrei Borzenkov
1 sibling, 0 replies; 33+ messages in thread
From: Andrei Borzenkov @ 2016-02-25 17:38 UTC (permalink / raw)
To: The development of GNU GRUB, Juergen Gross; +Cc: phcoder, mchang, xen-devel
22.02.2016 11:24, Daniel Kiper пишет:
>
> Are you sure that grub_errno is always set to GRUB_ERR_NONE
> if any GRUB2 function finished successfully?
grub_errno is reset by command parser before command execution (or after
previous command finished actually). During command execution there is
no guarantee that this happens; sometimes grub_errno is explicitly reset
to suppress subsequent errors but in general errors are just passed through.
> Maybe you should
> set initialize grub_errno with GRUB_ERR_NONE at the beginning
> of parse_xen_guest()?
>
> Daniel
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@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
^ permalink raw reply [flat|nested] 33+ messages in thread
[parent not found: <1456120999-5639-10-git-send-email-jgross@suse.com>]
* Re: [PATCH v4 09/11] xen: add capability to load initrd outside of initial mapping
[not found] ` <1456120999-5639-10-git-send-email-jgross@suse.com>
@ 2016-02-22 8:42 ` Daniel Kiper
[not found] ` <20160222084209.GN3482@olila.local.net-space.pl>
1 sibling, 0 replies; 33+ messages in thread
From: Daniel Kiper @ 2016-02-22 8:42 UTC (permalink / raw)
To: Juergen Gross; +Cc: grub-devel, phcoder, mchang, xen-devel
On Mon, Feb 22, 2016 at 07:03:17AM +0100, Juergen Gross wrote:
> Modern pvops linux kernels support an initrd not covered by the initial
> mapping. This capability is flagged by an elf-note.
>
> In case the elf-note is set by the kernel don't place the initrd into
> the initial mapping. This will allow to load larger initrds and/or
> support domains with larger memory, as the initial mapping is limited
> to 2GB and it is containing the p2m list.
>
> Signed-off-by: Juergen Gross <jgross@suse.com>
> Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
> ---
> V4: rename grub_xen_alloc_end() to grub_xen_alloc_final()
> ---
> grub-core/loader/i386/xen.c | 61 ++++++++++++++++++++++++++++++--------
> grub-core/loader/i386/xen_fileXX.c | 3 ++
> include/grub/xen_file.h | 1 +
> 3 files changed, 52 insertions(+), 13 deletions(-)
>
> diff --git a/grub-core/loader/i386/xen.c b/grub-core/loader/i386/xen.c
> index 2e12763..22a94ae 100644
> --- a/grub-core/loader/i386/xen.c
> +++ b/grub-core/loader/i386/xen.c
> @@ -58,6 +58,7 @@ struct xen_loader_state {
> grub_uint64_t modules_target_start;
> grub_size_t n_modules;
> int loaded;
> + int alloc_end_called;
alloc_end_called -> alloc_final_called
> };
>
> static struct xen_loader_state xen_state;
> @@ -320,6 +321,28 @@ grub_xen_pt_alloc (void)
> }
>
> static grub_err_t
> +grub_xen_alloc_final (void)
> +{
> + grub_err_t err;
> +
> + if (xen_state.alloc_end_called)
> + return GRUB_ERR_NONE;
> + xen_state.alloc_end_called = 1;
This is not nice. What happens if grub_xen_p2m_alloc() allocate what
is needed and grub_xen_special_alloc() fails?
Maybe grub_xen_p2m_alloc(), grub_xen_special_alloc() and grub_xen_pt_alloc()
should check itself that required regions are allocated properly
(if (something != NULL)) and do nothing if yes.
Daniel
^ permalink raw reply [flat|nested] 33+ messages in thread[parent not found: <20160222084209.GN3482@olila.local.net-space.pl>]
* Re: [PATCH v4 09/11] xen: add capability to load initrd outside of initial mapping
[not found] ` <20160222084209.GN3482@olila.local.net-space.pl>
@ 2016-02-22 9:18 ` Juergen Gross
[not found] ` <56CAD26E.9090205@suse.com>
1 sibling, 0 replies; 33+ messages in thread
From: Juergen Gross @ 2016-02-22 9:18 UTC (permalink / raw)
To: Daniel Kiper; +Cc: grub-devel, phcoder, mchang, xen-devel
On 22/02/16 09:42, Daniel Kiper wrote:
> On Mon, Feb 22, 2016 at 07:03:17AM +0100, Juergen Gross wrote:
>> Modern pvops linux kernels support an initrd not covered by the initial
>> mapping. This capability is flagged by an elf-note.
>>
>> In case the elf-note is set by the kernel don't place the initrd into
>> the initial mapping. This will allow to load larger initrds and/or
>> support domains with larger memory, as the initial mapping is limited
>> to 2GB and it is containing the p2m list.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
>> ---
>> V4: rename grub_xen_alloc_end() to grub_xen_alloc_final()
>> ---
>> grub-core/loader/i386/xen.c | 61 ++++++++++++++++++++++++++++++--------
>> grub-core/loader/i386/xen_fileXX.c | 3 ++
>> include/grub/xen_file.h | 1 +
>> 3 files changed, 52 insertions(+), 13 deletions(-)
>>
>> diff --git a/grub-core/loader/i386/xen.c b/grub-core/loader/i386/xen.c
>> index 2e12763..22a94ae 100644
>> --- a/grub-core/loader/i386/xen.c
>> +++ b/grub-core/loader/i386/xen.c
>> @@ -58,6 +58,7 @@ struct xen_loader_state {
>> grub_uint64_t modules_target_start;
>> grub_size_t n_modules;
>> int loaded;
>> + int alloc_end_called;
>
> alloc_end_called -> alloc_final_called
>
>> };
>>
>> static struct xen_loader_state xen_state;
>> @@ -320,6 +321,28 @@ grub_xen_pt_alloc (void)
>> }
>>
>> static grub_err_t
>> +grub_xen_alloc_final (void)
>> +{
>> + grub_err_t err;
>> +
>> + if (xen_state.alloc_end_called)
>> + return GRUB_ERR_NONE;
>> + xen_state.alloc_end_called = 1;
>
> This is not nice. What happens if grub_xen_p2m_alloc() allocate what
> is needed and grub_xen_special_alloc() fails?
Then grub_xen_alloc_final will fail resulting in boot failure of the
new kernel. The memory allocated by the single functions will be freed
in case a new boot is attempted (patch 1 and 2).
> Maybe grub_xen_p2m_alloc(), grub_xen_special_alloc() and grub_xen_pt_alloc()
> should check itself that required regions are allocated properly
> (if (something != NULL)) and do nothing if yes.
Hmm, with grub_xen_reset() doing appropriate cleanups this would be an
option. I think I'll change it as you are suggesting.
Juergen
^ permalink raw reply [flat|nested] 33+ messages in thread[parent not found: <56CAD26E.9090205@suse.com>]
* Re: [PATCH v4 09/11] xen: add capability to load initrd outside of initial mapping
[not found] ` <56CAD26E.9090205@suse.com>
@ 2016-02-22 12:24 ` Daniel Kiper
[not found] ` <20160222122411.GR3482@olila.local.net-space.pl>
1 sibling, 0 replies; 33+ messages in thread
From: Daniel Kiper @ 2016-02-22 12:24 UTC (permalink / raw)
To: Juergen Gross; +Cc: grub-devel, phcoder, mchang, xen-devel
On Mon, Feb 22, 2016 at 10:18:38AM +0100, Juergen Gross wrote:
> On 22/02/16 09:42, Daniel Kiper wrote:
> > On Mon, Feb 22, 2016 at 07:03:17AM +0100, Juergen Gross wrote:
> >> Modern pvops linux kernels support an initrd not covered by the initial
> >> mapping. This capability is flagged by an elf-note.
> >>
> >> In case the elf-note is set by the kernel don't place the initrd into
> >> the initial mapping. This will allow to load larger initrds and/or
> >> support domains with larger memory, as the initial mapping is limited
> >> to 2GB and it is containing the p2m list.
> >>
> >> Signed-off-by: Juergen Gross <jgross@suse.com>
> >> Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
> >> ---
> >> V4: rename grub_xen_alloc_end() to grub_xen_alloc_final()
> >> ---
> >> grub-core/loader/i386/xen.c | 61 ++++++++++++++++++++++++++++++--------
> >> grub-core/loader/i386/xen_fileXX.c | 3 ++
> >> include/grub/xen_file.h | 1 +
> >> 3 files changed, 52 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/grub-core/loader/i386/xen.c b/grub-core/loader/i386/xen.c
> >> index 2e12763..22a94ae 100644
> >> --- a/grub-core/loader/i386/xen.c
> >> +++ b/grub-core/loader/i386/xen.c
> >> @@ -58,6 +58,7 @@ struct xen_loader_state {
> >> grub_uint64_t modules_target_start;
> >> grub_size_t n_modules;
> >> int loaded;
> >> + int alloc_end_called;
> >
> > alloc_end_called -> alloc_final_called
> >
> >> };
> >>
> >> static struct xen_loader_state xen_state;
> >> @@ -320,6 +321,28 @@ grub_xen_pt_alloc (void)
> >> }
> >>
> >> static grub_err_t
> >> +grub_xen_alloc_final (void)
> >> +{
> >> + grub_err_t err;
> >> +
> >> + if (xen_state.alloc_end_called)
> >> + return GRUB_ERR_NONE;
> >> + xen_state.alloc_end_called = 1;
> >
> > This is not nice. What happens if grub_xen_p2m_alloc() allocate what
> > is needed and grub_xen_special_alloc() fails?
>
> Then grub_xen_alloc_final will fail resulting in boot failure of the
> new kernel. The memory allocated by the single functions will be freed
> in case a new boot is attempted (patch 1 and 2).
>
> > Maybe grub_xen_p2m_alloc(), grub_xen_special_alloc() and grub_xen_pt_alloc()
> > should check itself that required regions are allocated properly
> > (if (something != NULL)) and do nothing if yes.
>
> Hmm, with grub_xen_reset() doing appropriate cleanups this would be an
> option. I think I'll change it as you are suggesting.
It looks that we can completely avoid that stuff. I think that
grub_xen_alloc_final() should be called conditionally in both
places/cases. That is all.
Daniel
^ permalink raw reply [flat|nested] 33+ messages in thread[parent not found: <20160222122411.GR3482@olila.local.net-space.pl>]
* Re: [PATCH v4 09/11] xen: add capability to load initrd outside of initial mapping
[not found] ` <20160222122411.GR3482@olila.local.net-space.pl>
@ 2016-02-22 13:26 ` Juergen Gross
0 siblings, 0 replies; 33+ messages in thread
From: Juergen Gross @ 2016-02-22 13:26 UTC (permalink / raw)
To: Daniel Kiper; +Cc: grub-devel, phcoder, mchang, xen-devel
On 22/02/16 13:24, Daniel Kiper wrote:
> On Mon, Feb 22, 2016 at 10:18:38AM +0100, Juergen Gross wrote:
>> On 22/02/16 09:42, Daniel Kiper wrote:
>>> On Mon, Feb 22, 2016 at 07:03:17AM +0100, Juergen Gross wrote:
>>>> Modern pvops linux kernels support an initrd not covered by the initial
>>>> mapping. This capability is flagged by an elf-note.
>>>>
>>>> In case the elf-note is set by the kernel don't place the initrd into
>>>> the initial mapping. This will allow to load larger initrds and/or
>>>> support domains with larger memory, as the initial mapping is limited
>>>> to 2GB and it is containing the p2m list.
>>>>
>>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>>> Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
>>>> ---
>>>> V4: rename grub_xen_alloc_end() to grub_xen_alloc_final()
>>>> ---
>>>> grub-core/loader/i386/xen.c | 61 ++++++++++++++++++++++++++++++--------
>>>> grub-core/loader/i386/xen_fileXX.c | 3 ++
>>>> include/grub/xen_file.h | 1 +
>>>> 3 files changed, 52 insertions(+), 13 deletions(-)
>>>>
>>>> diff --git a/grub-core/loader/i386/xen.c b/grub-core/loader/i386/xen.c
>>>> index 2e12763..22a94ae 100644
>>>> --- a/grub-core/loader/i386/xen.c
>>>> +++ b/grub-core/loader/i386/xen.c
>>>> @@ -58,6 +58,7 @@ struct xen_loader_state {
>>>> grub_uint64_t modules_target_start;
>>>> grub_size_t n_modules;
>>>> int loaded;
>>>> + int alloc_end_called;
>>>
>>> alloc_end_called -> alloc_final_called
>>>
>>>> };
>>>>
>>>> static struct xen_loader_state xen_state;
>>>> @@ -320,6 +321,28 @@ grub_xen_pt_alloc (void)
>>>> }
>>>>
>>>> static grub_err_t
>>>> +grub_xen_alloc_final (void)
>>>> +{
>>>> + grub_err_t err;
>>>> +
>>>> + if (xen_state.alloc_end_called)
>>>> + return GRUB_ERR_NONE;
>>>> + xen_state.alloc_end_called = 1;
>>>
>>> This is not nice. What happens if grub_xen_p2m_alloc() allocate what
>>> is needed and grub_xen_special_alloc() fails?
>>
>> Then grub_xen_alloc_final will fail resulting in boot failure of the
>> new kernel. The memory allocated by the single functions will be freed
>> in case a new boot is attempted (patch 1 and 2).
>>
>>> Maybe grub_xen_p2m_alloc(), grub_xen_special_alloc() and grub_xen_pt_alloc()
>>> should check itself that required regions are allocated properly
>>> (if (something != NULL)) and do nothing if yes.
>>
>> Hmm, with grub_xen_reset() doing appropriate cleanups this would be an
>> option. I think I'll change it as you are suggesting.
>
> It looks that we can completely avoid that stuff. I think that
> grub_xen_alloc_final() should be called conditionally in both
> places/cases. That is all.
Hmm. I think this is less clear, especially taking patch 11 into
account. And I wouldn't like to tie e.g. page table allocation to
something like "if (!xen_state.virt_start_info)" which would result
from that change, now that I've removed xen_state.alloc_end_called as
you suggested.
Juergen
^ permalink raw reply [flat|nested] 33+ messages in thread