From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
To: Anthony PERARD <anthony.perard@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>,
Stefano Stabellini <stefano.stabellini@eu.citrix.com>,
Wei Liu <wei.liu2@citrix.com>,
xen-devel@lists.xen.org
Subject: Re: [PATCH v4 01/14] libxc: Rework extra module initialisation
Date: Tue, 15 Mar 2016 20:06:44 -0400 [thread overview]
Message-ID: <20160316000644.GG29495@char.us.oracle.com> (raw)
In-Reply-To: <1457978150-27201-2-git-send-email-anthony.perard@citrix.com>
On Mon, Mar 14, 2016 at 05:55:36PM +0000, Anthony PERARD wrote:
> This patch use xc_dom_alloc_segment() to allocate the memory space for the
> ACPI modules and the SMBIOS modules. This is to replace the arbitrary
> placement of 1MB after the hvmloader image.
s/of/at/ ?
Or are you rather staying that the memory for ACPI modules and such
is offset by one 1MB after hvmloader image?
In other words:
/-----------\ <- 0
| |
| hvmloader |
| binary |
+-----------| <- 800kB
| |
| empty |
+-----------+ <- 1.8MB
| ACPI DATA |
| |
+-----------+ <- 2.8MB
| |
| ..... |
+-----------| <- 4MB
| scratch |
| space for |
| hvmloader |
\-----------/ <- 8MB
Is what can happen now? But prior to this you had:
/-----------\ <- 0
| |
| hvmloader |
| binary |
+-----------| <- 800kB
| |
| empty |
+-----------+ <- 1MB
| ACPI DATA |
| |
+-----------+ <- 2MB
| |
| ..... |
+-----------| <- 4MB
| scratch |
| space for |
| hvmloader |
\-----------/ <- 8MB
?
>
> In later patches, while trying to load a firmware such as OVMF, the later
.. what later patches? These ones? You may want to mention which ones.
> could easily be loaded past the address 4MB (OVMF is a 2MB binary), but
Why would it be loaded past 4MB? Why not in the space right after ACPI
DSDT and such?
> hvmloader use a range of memory from 4MB to 8MB to perform tests and in the
> process, clear the memory, before loading the modules.
s/clear/clears/
>
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
> Changes in V4:
> - check that guest_addr_out have a reasonable value.
>
> New patch in V3.
> ---
> tools/libxc/xc_dom_hvmloader.c | 131 ++++++++++++-----------------------------
> 1 file changed, 38 insertions(+), 93 deletions(-)
>
> diff --git a/tools/libxc/xc_dom_hvmloader.c b/tools/libxc/xc_dom_hvmloader.c
> index 330d5e8..7cf5854 100644
> --- a/tools/libxc/xc_dom_hvmloader.c
> +++ b/tools/libxc/xc_dom_hvmloader.c
> @@ -129,98 +129,52 @@ static elf_errorstatus xc_dom_parse_hvm_kernel(struct xc_dom_image *dom)
> return rc;
> }
>
> -static int modules_init(struct xc_dom_image *dom,
> - uint64_t vend, struct elf_binary *elf,
> - uint64_t *mstart_out, uint64_t *mend_out)
> +static int module_init_one(struct xc_dom_image *dom,
> + struct xc_hvm_firmware_module *module,
> + char *name)
> {
> -#define MODULE_ALIGN 1UL << 7
> -#define MB_ALIGN 1UL << 20
> -#define MKALIGN(x, a) (((uint64_t)(x) + (a) - 1) & ~(uint64_t)((a) - 1))
> - uint64_t total_len = 0, offset1 = 0;
> + struct xc_dom_seg seg;
> + void *dest;
>
> - if ( dom->acpi_module.length == 0 && dom->smbios_module.length == 0 )
> - return 0;
> -
> - /* Find the total length for the firmware modules with a reasonable large
> - * alignment size to align each the modules.
> - */
> - total_len = MKALIGN(dom->acpi_module.length, MODULE_ALIGN);
> - offset1 = total_len;
> - total_len += MKALIGN(dom->smbios_module.length, MODULE_ALIGN);
> -
> - /* Want to place the modules 1Mb+change behind the loader image. */
> - *mstart_out = MKALIGN(elf->pend, MB_ALIGN) + (MB_ALIGN);
> - *mend_out = *mstart_out + total_len;
> -
> - if ( *mend_out > vend )
> - return -1;
> -
> - if ( dom->acpi_module.length != 0 )
> - dom->acpi_module.guest_addr_out = *mstart_out;
> - if ( dom->smbios_module.length != 0 )
> - dom->smbios_module.guest_addr_out = *mstart_out + offset1;
> + if ( module->length )
> + {
> + if ( xc_dom_alloc_segment(dom, &seg, name, 0, module->length) )
> + goto err;
> + dest = xc_dom_seg_to_ptr(dom, &seg);
> + if ( dest == NULL )
> + {
> + DOMPRINTF("%s: xc_dom_seg_to_ptr(dom, &seg) => NULL",
> + __FUNCTION__);
> + goto err;
> + }
> + memcpy(dest, module->data, module->length);
> + module->guest_addr_out = seg.vstart;
> + if ( module->guest_addr_out > UINT32_MAX ||
> + module->guest_addr_out + module->length > UINT32_MAX )
> + {
> + DOMPRINTF("%s: Module %s would be loaded abrove 4GB",
> + __FUNCTION__, name);
> + goto err;
> + }
> + }
>
> return 0;
> +err:
> + return -1;
> }
>
> -static int loadmodules(struct xc_dom_image *dom,
> - uint64_t mstart, uint64_t mend,
> - uint32_t domid)
> +static int modules_init(struct xc_dom_image *dom)
> {
> - privcmd_mmap_entry_t *entries = NULL;
> - unsigned long pfn_start;
> - unsigned long pfn_end;
> - size_t pages;
> - uint32_t i;
> - uint8_t *dest;
> - int rc = -1;
> - xc_interface *xch = dom->xch;
> -
> - if ( mstart == 0 || mend == 0 )
> - return 0;
> -
> - pfn_start = (unsigned long)(mstart >> PAGE_SHIFT);
> - pfn_end = (unsigned long)((mend + PAGE_SIZE - 1) >> PAGE_SHIFT);
> - pages = pfn_end - pfn_start;
> -
> - /* Map address space for module list. */
> - entries = calloc(pages, sizeof(privcmd_mmap_entry_t));
> - if ( entries == NULL )
> - goto error_out;
> -
> - for ( i = 0; i < pages; i++ )
> - entries[i].mfn = (mstart >> PAGE_SHIFT) + i;
> -
> - dest = xc_map_foreign_ranges(
> - xch, domid, pages << PAGE_SHIFT, PROT_READ | PROT_WRITE, 1 << PAGE_SHIFT,
> - entries, pages);
> - if ( dest == NULL )
> - goto error_out;
> -
> - /* Zero the range so padding is clear between modules */
> - memset(dest, 0, pages << PAGE_SHIFT);
> -
> - /* Load modules into range */
> - if ( dom->acpi_module.length != 0 )
> - {
> - memcpy(dest,
> - dom->acpi_module.data,
> - dom->acpi_module.length);
> - }
> - if ( dom->smbios_module.length != 0 )
> - {
> - memcpy(dest + (dom->smbios_module.guest_addr_out - mstart),
> - dom->smbios_module.data,
> - dom->smbios_module.length);
> - }
> -
> - munmap(dest, pages << PAGE_SHIFT);
> - rc = 0;
> + int rc;
>
> - error_out:
> - free(entries);
> + rc = module_init_one(dom, &dom->acpi_module, "acpi module");
> + if ( rc ) goto err;
> + rc = module_init_one(dom, &dom->smbios_module, "smbios module");
> + if ( rc ) goto err;
You can't be that code thought. That is some nice deletion of code.
Now is this safe for migration?
I presume so since this is all E820_RESERVED righT? No funny business
there?
>
> - return rc;
> + return 0;
> +err:
> + return -1;
> }
>
> static elf_errorstatus xc_dom_load_hvm_kernel(struct xc_dom_image *dom)
> @@ -229,7 +183,6 @@ static elf_errorstatus xc_dom_load_hvm_kernel(struct xc_dom_image *dom)
> privcmd_mmap_entry_t *entries = NULL;
> size_t pages = (elf->pend - elf->pstart + PAGE_SIZE - 1) >> PAGE_SHIFT;
> elf_errorstatus rc;
> - uint64_t m_start = 0, m_end = 0;
> int i;
>
> /* Map address space for initial elf image. */
> @@ -262,15 +215,7 @@ static elf_errorstatus xc_dom_load_hvm_kernel(struct xc_dom_image *dom)
>
> munmap(elf->dest_base, elf->dest_size);
>
> - rc = modules_init(dom, dom->total_pages << PAGE_SHIFT, elf, &m_start,
> - &m_end);
> - if ( rc != 0 )
> - {
> - DOMPRINTF("%s: insufficient space to load modules.", __func__);
> - goto error;
> - }
> -
> - rc = loadmodules(dom, m_start, m_end, dom->guest_domid);
> + rc = modules_init(dom);
> if ( rc != 0 )
> {
> DOMPRINTF("%s: unable to load modules.", __func__);
> --
> Anthony PERARD
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
next prev parent reply other threads:[~2016-03-16 0:06 UTC|newest]
Thread overview: 73+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-03-14 17:55 [PATCH v4 00/14] Load BIOS via toolstack instead of been embedded in hvmloader Anthony PERARD
2016-03-14 17:55 ` [PATCH v4 01/14] libxc: Rework extra module initialisation Anthony PERARD
2016-03-16 0:06 ` Konrad Rzeszutek Wilk [this message]
2016-03-17 16:24 ` Anthony PERARD
2016-03-14 17:55 ` [PATCH v4 02/14] libxc: Prepare a start info structure for hvmloader Anthony PERARD
2016-03-16 0:18 ` Konrad Rzeszutek Wilk
2016-03-16 18:01 ` Boris Ostrovsky
2016-03-17 16:48 ` Anthony PERARD
2016-03-17 16:28 ` Anthony PERARD
2016-03-14 17:55 ` [PATCH v4 03/14] configure: #define SEABIOS_PATH and OVMF_PATH Anthony PERARD
2016-03-16 0:20 ` Konrad Rzeszutek Wilk
2016-04-08 13:38 ` Wei Liu
2016-03-14 17:55 ` [PATCH v4 04/14] firmware/makefile: install BIOS blob Anthony PERARD
2016-03-16 0:26 ` Konrad Rzeszutek Wilk
2016-03-16 8:54 ` Dario Faggioli
2016-03-16 8:56 ` Konrad Rzeszutek Wilk
2016-03-17 16:58 ` Anthony PERARD
2016-03-17 17:37 ` Doug Goldstein
2016-03-17 18:33 ` Anthony PERARD
2016-03-18 21:11 ` Jim Fehlig
2016-03-19 0:43 ` Doug Goldstein
2016-04-18 14:31 ` Doug Goldstein
2016-04-19 13:11 ` Stefano Stabellini
2016-03-14 17:55 ` [PATCH v4 05/14] libxl: Load guest BIOS from file Anthony PERARD
2016-03-16 0:53 ` Konrad Rzeszutek Wilk
2016-03-16 9:27 ` Dario Faggioli
2016-03-17 17:24 ` Anthony PERARD
2016-03-14 17:55 ` [PATCH v4 06/14] xen: Move the hvm_start_info C representation from libxc to public/xen.h Anthony PERARD
2016-03-15 8:09 ` Jan Beulich
2016-03-16 0:59 ` Konrad Rzeszutek Wilk
2016-03-16 1:00 ` Konrad Rzeszutek Wilk
2016-03-16 8:32 ` Jan Beulich
2016-03-21 17:04 ` Roger Pau Monné
2016-03-21 17:21 ` Jan Beulich
2016-03-14 17:55 ` [PATCH v4 07/14] hvmloader: Grab the hvm_start_info pointer Anthony PERARD
2016-03-16 1:07 ` Konrad Rzeszutek Wilk
2016-04-05 12:43 ` Jan Beulich
2016-03-14 17:55 ` [PATCH v4 08/14] hvmloader: Locate the BIOS blob Anthony PERARD
2016-03-16 1:14 ` Konrad Rzeszutek Wilk
2016-03-17 17:46 ` Anthony PERARD
2016-03-17 17:57 ` Konrad Rzeszutek Wilk
2016-03-18 7:34 ` Jan Beulich
2016-04-05 12:59 ` Jan Beulich
2016-04-05 14:05 ` Roger Pau Monné
2016-04-05 14:23 ` Jan Beulich
2016-04-07 15:10 ` Anthony PERARD
2016-04-07 15:30 ` Jan Beulich
2016-03-14 17:55 ` [PATCH v4 09/14] hvmloader: Check modules whereabouts in perform_tests Anthony PERARD
2016-03-16 1:23 ` Konrad Rzeszutek Wilk
2016-03-17 18:08 ` Anthony PERARD
2016-04-05 13:07 ` Jan Beulich
2016-03-14 17:55 ` [PATCH v4 10/14] hvmloader: Load SeaBIOS from hvm_start_info modules Anthony PERARD
2016-03-16 1:27 ` Konrad Rzeszutek Wilk
2016-03-16 1:27 ` Konrad Rzeszutek Wilk
2016-04-05 13:11 ` Jan Beulich
2016-03-14 17:55 ` [PATCH v4 11/14] hvmloader: Load OVMF from modules Anthony PERARD
2016-03-16 1:36 ` Konrad Rzeszutek Wilk
2016-04-05 13:16 ` Jan Beulich
2016-03-14 17:55 ` [PATCH v4 12/14] hvmloader: Specific bios_load function required Anthony PERARD
2016-03-16 1:38 ` Konrad Rzeszutek Wilk
2016-03-17 18:25 ` Anthony PERARD
2016-03-14 17:55 ` [PATCH v4 13/14] hvmloader: Always build-in SeaBIOS and OVMF loader Anthony PERARD
2016-03-14 17:55 ` [PATCH v4 14/14] configure: do not depend on SEABIOS_PATH or OVMF_PATH Anthony PERARD
2016-03-16 1:40 ` Konrad Rzeszutek Wilk
2016-04-08 13:38 ` Wei Liu
2016-03-24 17:55 ` [PATCH v4 00/14] Load BIOS via toolstack instead of been embedded in hvmloader Jim Fehlig
2016-03-30 17:22 ` Jim Fehlig
2016-03-31 7:20 ` Jan Beulich
[not found] ` <56FCEBCD02000078000E19BF@suse.com>
2016-03-31 14:36 ` Jim Fehlig
2016-03-31 16:49 ` George Dunlap
2016-04-01 9:12 ` George Dunlap
2016-04-01 14:24 ` Konrad Rzeszutek Wilk
2016-04-01 20:06 ` Jim Fehlig
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20160316000644.GG29495@char.us.oracle.com \
--to=konrad.wilk@oracle.com \
--cc=anthony.perard@citrix.com \
--cc=ian.jackson@eu.citrix.com \
--cc=stefano.stabellini@eu.citrix.com \
--cc=wei.liu2@citrix.com \
--cc=xen-devel@lists.xen.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).