* [PATCH v5 01/14] libxc: Rework extra module initialisation
2016-06-22 17:15 [PATCH v5 00/14] Load BIOS via toolstack instead of been embedded in hvmloader Anthony PERARD
@ 2016-06-22 17:15 ` Anthony PERARD
2016-07-07 14:55 ` Wei Liu
2016-06-22 17:15 ` [PATCH v5 02/14] libxc: Prepare a start info structure for hvmloader Anthony PERARD
` (12 subsequent siblings)
13 siblings, 1 reply; 43+ messages in thread
From: Anthony PERARD @ 2016-06-22 17:15 UTC (permalink / raw)
To: xen-devel; +Cc: Anthony PERARD, Wei Liu, Ian Jackson
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 (+ extra for MB alignement) after the hvmloader image.
This patch can help if one add extra ACPI table and hvmloader contain
OVMF (OVMF is a 2MB binary), as in that case the extra ACPI table could
easily be loaded past the address 4MB, but hvmloader use a range of
memory from 4MB to 10MB to perform tests and in the process, clears the
memory, before loading the modules.
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
Changes in V5:
- rewrite patch description
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..da8b995 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;
- 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
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH v5 01/14] libxc: Rework extra module initialisation
2016-06-22 17:15 ` [PATCH v5 01/14] libxc: Rework extra module initialisation Anthony PERARD
@ 2016-07-07 14:55 ` Wei Liu
2016-07-08 10:52 ` Anthony PERARD
0 siblings, 1 reply; 43+ messages in thread
From: Wei Liu @ 2016-07-07 14:55 UTC (permalink / raw)
To: Anthony PERARD; +Cc: Wei Liu, Ian Jackson, xen-devel
On Wed, Jun 22, 2016 at 06:15:32PM +0100, 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 (+ extra for MB alignement) after the hvmloader image.
>
> This patch can help if one add extra ACPI table and hvmloader contain
> OVMF (OVMF is a 2MB binary), as in that case the extra ACPI table could
> easily be loaded past the address 4MB, but hvmloader use a range of
> memory from 4MB to 10MB to perform tests and in the process, clears the
> memory, before loading the modules.
>
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
> Changes in V5:
> - rewrite patch description
>
> 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..da8b995 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;
> + }
One question:
Can this check also account for MMIO hole below 4G? Maybe use
dom->mmio_size?
Other than this, this patch looks good.
Wei.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v5 01/14] libxc: Rework extra module initialisation
2016-07-07 14:55 ` Wei Liu
@ 2016-07-08 10:52 ` Anthony PERARD
2016-07-08 11:29 ` Wei Liu
0 siblings, 1 reply; 43+ messages in thread
From: Anthony PERARD @ 2016-07-08 10:52 UTC (permalink / raw)
To: Wei Liu; +Cc: Ian Jackson, xen-devel
On Thu, Jul 07, 2016 at 03:55:23PM +0100, Wei Liu wrote:
> On Wed, Jun 22, 2016 at 06:15:32PM +0100, 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 (+ extra for MB alignement) after the hvmloader image.
> >
> > This patch can help if one add extra ACPI table and hvmloader contain
> > OVMF (OVMF is a 2MB binary), as in that case the extra ACPI table could
> > easily be loaded past the address 4MB, but hvmloader use a range of
> > memory from 4MB to 10MB to perform tests and in the process, clears the
> > memory, before loading the modules.
> >
> > Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> > ---
> > Changes in V5:
> > - rewrite patch description
> >
> > 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..da8b995 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;
> > + }
>
> One question:
>
> Can this check also account for MMIO hole below 4G? Maybe use
> dom->mmio_size?
Yes, I guess I can check against dom->mmio_start. Should I also check
that mmio_start have reasonable value? (<4G, and not 0x0) Or is
mmio_start is already supposed to have a good value?
> Other than this, this patch looks good.
Thanks,
--
Anthony PERARD
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v5 01/14] libxc: Rework extra module initialisation
2016-07-08 10:52 ` Anthony PERARD
@ 2016-07-08 11:29 ` Wei Liu
2016-07-08 13:26 ` Anthony PERARD
0 siblings, 1 reply; 43+ messages in thread
From: Wei Liu @ 2016-07-08 11:29 UTC (permalink / raw)
To: Anthony PERARD; +Cc: Ian Jackson, Wei Liu, xen-devel
On Fri, Jul 08, 2016 at 11:52:08AM +0100, Anthony PERARD wrote:
> On Thu, Jul 07, 2016 at 03:55:23PM +0100, Wei Liu wrote:
> > On Wed, Jun 22, 2016 at 06:15:32PM +0100, 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 (+ extra for MB alignement) after the hvmloader image.
> > >
> > > This patch can help if one add extra ACPI table and hvmloader contain
> > > OVMF (OVMF is a 2MB binary), as in that case the extra ACPI table could
> > > easily be loaded past the address 4MB, but hvmloader use a range of
> > > memory from 4MB to 10MB to perform tests and in the process, clears the
> > > memory, before loading the modules.
> > >
> > > Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> > > ---
> > > Changes in V5:
> > > - rewrite patch description
> > >
> > > 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..da8b995 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;
> > > + }
> >
> > One question:
> >
> > Can this check also account for MMIO hole below 4G? Maybe use
> > dom->mmio_size?
>
> Yes, I guess I can check against dom->mmio_start. Should I also check
> that mmio_start have reasonable value? (<4G, and not 0x0) Or is
> mmio_start is already supposed to have a good value?
>
mmio_start should already have a sane value here -- or at least I hope
so. The sanity of mmio_start should be checked where it is assigned.
Wei.
> > Other than this, this patch looks good.
>
> Thanks,
>
> --
> Anthony PERARD
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v5 01/14] libxc: Rework extra module initialisation
2016-07-08 11:29 ` Wei Liu
@ 2016-07-08 13:26 ` Anthony PERARD
0 siblings, 0 replies; 43+ messages in thread
From: Anthony PERARD @ 2016-07-08 13:26 UTC (permalink / raw)
To: Wei Liu; +Cc: Ian Jackson, xen-devel
On Fri, Jul 08, 2016 at 12:29:36PM +0100, Wei Liu wrote:
> On Fri, Jul 08, 2016 at 11:52:08AM +0100, Anthony PERARD wrote:
> > On Thu, Jul 07, 2016 at 03:55:23PM +0100, Wei Liu wrote:
> > > On Wed, Jun 22, 2016 at 06:15:32PM +0100, Anthony PERARD wrote:
> > > > diff --git a/tools/libxc/xc_dom_hvmloader.c b/tools/libxc/xc_dom_hvmloader.c
> > > > index 330d5e8..da8b995 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;
> > > > + }
> > >
> > > One question:
> > >
> > > Can this check also account for MMIO hole below 4G? Maybe use
> > > dom->mmio_size?
> >
> > Yes, I guess I can check against dom->mmio_start. Should I also check
> > that mmio_start have reasonable value? (<4G, and not 0x0) Or is
> > mmio_start is already supposed to have a good value?
> >
>
> mmio_start should already have a sane value here -- or at least I hope
> so. The sanity of mmio_start should be checked where it is assigned.
Ok, I'll use dom->mmio_start instead of UINT32_MAX, and probably add an
assert() on the values expected in mmio_start.
Thanks,
--
Anthony PERARD
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v5 02/14] libxc: Prepare a start info structure for hvmloader
2016-06-22 17:15 [PATCH v5 00/14] Load BIOS via toolstack instead of been embedded in hvmloader Anthony PERARD
2016-06-22 17:15 ` [PATCH v5 01/14] libxc: Rework extra module initialisation Anthony PERARD
@ 2016-06-22 17:15 ` Anthony PERARD
2016-06-23 14:44 ` Boris Ostrovsky
2016-07-07 14:55 ` Wei Liu
2016-06-22 17:15 ` [PATCH v5 03/14] configure: #define SEABIOS_PATH and OVMF_PATH Anthony PERARD
` (11 subsequent siblings)
13 siblings, 2 replies; 43+ messages in thread
From: Anthony PERARD @ 2016-06-22 17:15 UTC (permalink / raw)
To: xen-devel
Cc: Anthony PERARD, Wei Liu, boris.ostrovsky, Ian Jackson, roger.pau
... and load BIOS/UEFI firmware into guest memory.
This adds a new firmware module, system_firmware_module. It is loaded in
the guest memory and final location is provided to hvmloader via the
hvm_start_info struct.
This patch create the hvm_start_info struct for HVM guest that have a
device model, so this is now common code with HVM guest without device
model.
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
CC: boris.ostrovsky@oracle.com
CC: roger.pau@citrix.com
Changes in V5:
- in alloc_magic_pages_hvm, check dom->device_model only once instead of
twice (fold second if into previous else)
- rework add_module_to_list to make it easier to read
- also comment about the intended memory layout of start_info and the
modules
- in bootlate_hvm(), drop start_page and use start_info as they point to
the same address
- rename xc_dom_image.bios_module to xc_dom_image.system_firmware_module
- rename module name to "firmware" (was "bios")
Changes in V4:
- change title to suggest the change of beavior
- remove code to load acpi tables (dsdt)
- Update public/xen.h about hvm_start_info available on other HVM guest
in %ebx.
Changes in V3:
- rename acpi_table_module to full_acpi_module.
- factorise module loading, using new function to load existing optinal
module, this should not change anything
- should now use the same code to loads modules as for HVMlite VMs.
this avoid duplication of code.
- no more generic cmdline with a list of modules, each module have its name
in the module specific cmdline.
- scope change for common code between hvmlite and hvmloader
---
tools/libxc/include/xc_dom.h | 3 +
tools/libxc/xc_dom_hvmloader.c | 3 +
tools/libxc/xc_dom_x86.c | 152 +++++++++++++++++++++++++++++------------
xen/include/public/xen.h | 2 +-
4 files changed, 116 insertions(+), 44 deletions(-)
diff --git a/tools/libxc/include/xc_dom.h b/tools/libxc/include/xc_dom.h
index 6cb10c4..0629971 100644
--- a/tools/libxc/include/xc_dom.h
+++ b/tools/libxc/include/xc_dom.h
@@ -209,6 +209,9 @@ struct xc_dom_image {
/* If unset disables the setup of the IOREQ pages. */
bool device_model;
+ /* BIOS/Firmware passed to HVMLOADER */
+ struct xc_hvm_firmware_module system_firmware_module;
+
/* Extra ACPI tables passed to HVMLOADER */
struct xc_hvm_firmware_module acpi_module;
diff --git a/tools/libxc/xc_dom_hvmloader.c b/tools/libxc/xc_dom_hvmloader.c
index da8b995..cf2d57c 100644
--- a/tools/libxc/xc_dom_hvmloader.c
+++ b/tools/libxc/xc_dom_hvmloader.c
@@ -167,6 +167,9 @@ static int modules_init(struct xc_dom_image *dom)
{
int rc;
+ rc = module_init_one(dom, &dom->system_firmware_module,
+ "System Firmware module");
+ if ( rc ) goto err;
rc = module_init_one(dom, &dom->acpi_module, "ACPI module");
if ( rc ) goto err;
rc = module_init_one(dom, &dom->smbios_module, "SMBIOS module");
diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
index 021f8a8..f017fbd 100644
--- a/tools/libxc/xc_dom_x86.c
+++ b/tools/libxc/xc_dom_x86.c
@@ -69,6 +69,9 @@
#define round_up(addr, mask) ((addr) | (mask))
#define round_pg_up(addr) (((addr) + PAGE_SIZE_X86 - 1) & ~(PAGE_SIZE_X86 - 1))
+#define HVMLOADER_MODULE_MAX_COUNT 1
+#define HVMLOADER_MODULE_NAME_SIZE 10
+
struct xc_dom_params {
unsigned levels;
xen_vaddr_t vaddr_mask;
@@ -590,6 +593,7 @@ static int alloc_magic_pages_hvm(struct xc_dom_image *dom)
xen_pfn_t special_array[X86_HVM_NR_SPECIAL_PAGES];
xen_pfn_t ioreq_server_array[NR_IOREQ_SERVER_PAGES];
xc_interface *xch = dom->xch;
+ size_t start_info_size = sizeof(struct hvm_start_info);
/* Allocate and clear special pages. */
for ( i = 0; i < X86_HVM_NR_SPECIAL_PAGES; i++ )
@@ -624,8 +628,6 @@ static int alloc_magic_pages_hvm(struct xc_dom_image *dom)
if ( !dom->device_model )
{
- size_t start_info_size = sizeof(struct hvm_start_info);
-
if ( dom->cmdline )
{
dom->cmdline_size = ROUNDUP(strlen(dom->cmdline) + 1, 8);
@@ -635,17 +637,18 @@ static int alloc_magic_pages_hvm(struct xc_dom_image *dom)
/* Limited to one module. */
if ( dom->ramdisk_blob )
start_info_size += sizeof(struct hvm_modlist_entry);
-
- rc = xc_dom_alloc_segment(dom, &dom->start_info_seg,
- "HVMlite start info", 0, start_info_size);
- if ( rc != 0 )
- {
- DOMPRINTF("Unable to reserve memory for the start info");
- goto out;
- }
}
else
{
+ start_info_size +=
+ sizeof(struct hvm_modlist_entry) * HVMLOADER_MODULE_MAX_COUNT;
+ /*
+ * Add extra space to write modules name.
+ * The HVMLOADER_MODULE_NAME_SIZE accounts for NUL byte.
+ */
+ start_info_size +=
+ HVMLOADER_MODULE_NAME_SIZE * HVMLOADER_MODULE_MAX_COUNT;
+
/*
* Allocate and clear additional ioreq server pages. The default
* server will use the IOREQ and BUFIOREQ special pages above.
@@ -672,6 +675,14 @@ static int alloc_magic_pages_hvm(struct xc_dom_image *dom)
NR_IOREQ_SERVER_PAGES);
}
+ rc = xc_dom_alloc_segment(dom, &dom->start_info_seg,
+ "HVMlite start info", 0, start_info_size);
+ if ( rc != 0 )
+ {
+ DOMPRINTF("Unable to reserve memory for the start info");
+ goto out;
+ }
+
/*
* Identity-map page table is required for running with CR0.PG=0 when
* using Intel EPT. Create a 32-bit non-PAE page directory of superpages.
@@ -1689,42 +1700,89 @@ static int alloc_pgtables_hvm(struct xc_dom_image *dom)
return 0;
}
+/*
+ * The memory layout of the start_info page and the modules, and where the
+ * addresses are stored:
+ *
+ * /----------------------------------\
+ * | struct hvm_start_info |
+ * +----------------------------------+ <- start_info->modlist_paddr
+ * | struct hvm_modlist_entry[0] |
+ * +----------------------------------+
+ * | struct hvm_modlist_entry[1] |
+ * +----------------------------------+ <- modlist[0].cmdline_paddr
+ * | cmdline of module 0 |
+ * | char[HVMLOADER_MODULE_NAME_SIZE] |
+ * +----------------------------------+ <- modlist[1].cmdline_paddr
+ * | cmdline of module 1 |
+ * +----------------------------------+
+ */
+static void add_module_to_list(struct xc_dom_image *dom,
+ struct xc_hvm_firmware_module *module,
+ const char *name,
+ struct hvm_modlist_entry *modlist,
+ struct hvm_start_info *start_info)
+{
+ uint32_t index = start_info->nr_modules;
+ void *modules_cmdline_start = modlist + HVMLOADER_MODULE_MAX_COUNT;
+ uint64_t modlist_paddr = (dom->start_info_seg.pfn << PAGE_SHIFT) +
+ ((uintptr_t)modlist - (uintptr_t)start_info);
+ uint64_t modules_cmdline_paddr = modlist_paddr +
+ sizeof(struct hvm_modlist_entry) * HVMLOADER_MODULE_MAX_COUNT;
+
+ if ( module->length == 0 )
+ return;
+
+ assert(start_info->nr_modules < HVMLOADER_MODULE_MAX_COUNT);
+ assert(strnlen(name, HVMLOADER_MODULE_NAME_SIZE)
+ < HVMLOADER_MODULE_NAME_SIZE);
+
+ modlist[index].paddr = module->guest_addr_out;
+ modlist[index].size = module->length;
+
+ strncpy(modules_cmdline_start + HVMLOADER_MODULE_NAME_SIZE * index,
+ name, HVMLOADER_MODULE_NAME_SIZE);
+ modlist[index].cmdline_paddr =
+ modules_cmdline_paddr + HVMLOADER_MODULE_NAME_SIZE * index;
+
+ start_info->nr_modules++;
+}
+
static int bootlate_hvm(struct xc_dom_image *dom)
{
uint32_t domid = dom->guest_domid;
xc_interface *xch = dom->xch;
+ struct hvm_start_info *start_info;
+ size_t start_info_size;
+ struct hvm_modlist_entry *modlist;
- if ( !dom->device_model )
- {
- struct hvm_start_info *start_info;
- size_t start_info_size;
- void *start_page;
-
- start_info_size = sizeof(*start_info) + dom->cmdline_size;
- if ( dom->ramdisk_blob )
- start_info_size += sizeof(struct hvm_modlist_entry);
+ start_info_size = sizeof(*start_info) + dom->cmdline_size;
+ if ( dom->ramdisk_blob )
+ start_info_size += sizeof(struct hvm_modlist_entry);
- if ( start_info_size >
- dom->start_info_seg.pages << XC_DOM_PAGE_SHIFT(dom) )
- {
- DOMPRINTF("Trying to map beyond start_info_seg");
- return -1;
- }
+ if ( start_info_size >
+ dom->start_info_seg.pages << XC_DOM_PAGE_SHIFT(dom) )
+ {
+ DOMPRINTF("Trying to map beyond start_info_seg");
+ return -1;
+ }
- start_page = xc_map_foreign_range(xch, domid, start_info_size,
- PROT_READ | PROT_WRITE,
- dom->start_info_seg.pfn);
- if ( start_page == NULL )
- {
- DOMPRINTF("Unable to map HVM start info page");
- return -1;
- }
+ start_info = xc_map_foreign_range(xch, domid, start_info_size,
+ PROT_READ | PROT_WRITE,
+ dom->start_info_seg.pfn);
+ if ( start_info == NULL )
+ {
+ DOMPRINTF("Unable to map HVM start info page");
+ return -1;
+ }
- start_info = start_page;
+ modlist = (void*)(start_info + 1) + dom->cmdline_size;
+ if ( !dom->device_model )
+ {
if ( dom->cmdline )
{
- char *cmdline = start_page + sizeof(*start_info);
+ char *cmdline = (void*)(start_info + 1);
strncpy(cmdline, dom->cmdline, dom->cmdline_size);
start_info->cmdline_paddr = (dom->start_info_seg.pfn << PAGE_SHIFT) +
@@ -1733,22 +1791,30 @@ static int bootlate_hvm(struct xc_dom_image *dom)
if ( dom->ramdisk_blob )
{
- struct hvm_modlist_entry *modlist =
- start_page + sizeof(*start_info) + dom->cmdline_size;
modlist[0].paddr = dom->ramdisk_seg.vstart - dom->parms.virt_base;
modlist[0].size = dom->ramdisk_seg.vend - dom->ramdisk_seg.vstart;
- start_info->modlist_paddr = (dom->start_info_seg.pfn << PAGE_SHIFT) +
- ((uintptr_t)modlist - (uintptr_t)start_info);
start_info->nr_modules = 1;
}
-
- start_info->magic = XEN_HVM_START_MAGIC_VALUE;
-
- munmap(start_page, start_info_size);
}
else
{
+ add_module_to_list(dom, &dom->system_firmware_module, "firmware",
+ modlist, start_info);
+ }
+
+ if ( start_info->nr_modules )
+ {
+ start_info->modlist_paddr = (dom->start_info_seg.pfn << PAGE_SHIFT) +
+ ((uintptr_t)modlist - (uintptr_t)start_info);
+ }
+
+ start_info->magic = XEN_HVM_START_MAGIC_VALUE;
+
+ munmap(start_info, start_info_size);
+
+ if ( dom->device_model )
+ {
void *hvm_info_page;
if ( (hvm_info_page = xc_map_foreign_range(
diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h
index 37bbb22..d9ddee7 100644
--- a/xen/include/public/xen.h
+++ b/xen/include/public/xen.h
@@ -814,7 +814,7 @@ struct start_info {
typedef struct start_info start_info_t;
/*
- * Start of day structure passed to PVH guests in %ebx.
+ * Start of day structure passed to PVH guests and to HVM 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.
--
Anthony PERARD
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH v5 02/14] libxc: Prepare a start info structure for hvmloader
2016-06-22 17:15 ` [PATCH v5 02/14] libxc: Prepare a start info structure for hvmloader Anthony PERARD
@ 2016-06-23 14:44 ` Boris Ostrovsky
2016-06-23 16:52 ` Anthony PERARD
2016-07-07 14:55 ` Wei Liu
1 sibling, 1 reply; 43+ messages in thread
From: Boris Ostrovsky @ 2016-06-23 14:44 UTC (permalink / raw)
To: Anthony PERARD, xen-devel; +Cc: Wei Liu, Ian Jackson, roger.pau
On 06/22/2016 01:15 PM, Anthony PERARD wrote:
> ... and load BIOS/UEFI firmware into guest memory.
>
> This adds a new firmware module, system_firmware_module. It is loaded in
> the guest memory and final location is provided to hvmloader via the
> hvm_start_info struct.
>
> This patch create the hvm_start_info struct for HVM guest that have a
> device model, so this is now common code with HVM guest without device
> model.
>
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
> CC: boris.ostrovsky@oracle.com
> CC: roger.pau@citrix.com
>
> Changes in V5:
> - in alloc_magic_pages_hvm, check dom->device_model only once instead of
> twice (fold second if into previous else)
> - rework add_module_to_list to make it easier to read
> - also comment about the intended memory layout of start_info and the
> modules
> - in bootlate_hvm(), drop start_page and use start_info as they point to
> the same address
> - rename xc_dom_image.bios_module to xc_dom_image.system_firmware_module
> - rename module name to "firmware" (was "bios")
>
> Changes in V4:
> - change title to suggest the change of beavior
> - remove code to load acpi tables (dsdt)
> - Update public/xen.h about hvm_start_info available on other HVM guest
> in %ebx.
>
> Changes in V3:
> - rename acpi_table_module to full_acpi_module.
> - factorise module loading, using new function to load existing optinal
> module, this should not change anything
> - should now use the same code to loads modules as for HVMlite VMs.
> this avoid duplication of code.
> - no more generic cmdline with a list of modules, each module have its name
> in the module specific cmdline.
> - scope change for common code between hvmlite and hvmloader
> ---
> tools/libxc/include/xc_dom.h | 3 +
> tools/libxc/xc_dom_hvmloader.c | 3 +
> tools/libxc/xc_dom_x86.c | 152 +++++++++++++++++++++++++++++------------
> xen/include/public/xen.h | 2 +-
> 4 files changed, 116 insertions(+), 44 deletions(-)
>
> diff --git a/tools/libxc/include/xc_dom.h b/tools/libxc/include/xc_dom.h
> index 6cb10c4..0629971 100644
> --- a/tools/libxc/include/xc_dom.h
> +++ b/tools/libxc/include/xc_dom.h
> @@ -209,6 +209,9 @@ struct xc_dom_image {
> /* If unset disables the setup of the IOREQ pages. */
> bool device_model;
>
> + /* BIOS/Firmware passed to HVMLOADER */
> + struct xc_hvm_firmware_module system_firmware_module;
> +
> /* Extra ACPI tables passed to HVMLOADER */
> struct xc_hvm_firmware_module acpi_module;
>
> diff --git a/tools/libxc/xc_dom_hvmloader.c b/tools/libxc/xc_dom_hvmloader.c
> index da8b995..cf2d57c 100644
> --- a/tools/libxc/xc_dom_hvmloader.c
> +++ b/tools/libxc/xc_dom_hvmloader.c
> @@ -167,6 +167,9 @@ static int modules_init(struct xc_dom_image *dom)
> {
> int rc;
>
> + rc = module_init_one(dom, &dom->system_firmware_module,
> + "System Firmware module");
> + if ( rc ) goto err;
> rc = module_init_one(dom, &dom->acpi_module, "ACPI module");
> if ( rc ) goto err;
> rc = module_init_one(dom, &dom->smbios_module, "SMBIOS module");
> diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
> index 021f8a8..f017fbd 100644
> --- a/tools/libxc/xc_dom_x86.c
> +++ b/tools/libxc/xc_dom_x86.c
> @@ -69,6 +69,9 @@
> #define round_up(addr, mask) ((addr) | (mask))
> #define round_pg_up(addr) (((addr) + PAGE_SIZE_X86 - 1) & ~(PAGE_SIZE_X86 - 1))
>
> +#define HVMLOADER_MODULE_MAX_COUNT 1
> +#define HVMLOADER_MODULE_NAME_SIZE 10
> +
> struct xc_dom_params {
> unsigned levels;
> xen_vaddr_t vaddr_mask;
> @@ -590,6 +593,7 @@ static int alloc_magic_pages_hvm(struct xc_dom_image *dom)
> xen_pfn_t special_array[X86_HVM_NR_SPECIAL_PAGES];
> xen_pfn_t ioreq_server_array[NR_IOREQ_SERVER_PAGES];
> xc_interface *xch = dom->xch;
> + size_t start_info_size = sizeof(struct hvm_start_info);
>
> /* Allocate and clear special pages. */
> for ( i = 0; i < X86_HVM_NR_SPECIAL_PAGES; i++ )
> @@ -624,8 +628,6 @@ static int alloc_magic_pages_hvm(struct xc_dom_image *dom)
>
> if ( !dom->device_model )
> {
> - size_t start_info_size = sizeof(struct hvm_start_info);
> -
> if ( dom->cmdline )
> {
> dom->cmdline_size = ROUNDUP(strlen(dom->cmdline) + 1, 8);
> @@ -635,17 +637,18 @@ static int alloc_magic_pages_hvm(struct xc_dom_image *dom)
> /* Limited to one module. */
> if ( dom->ramdisk_blob )
> start_info_size += sizeof(struct hvm_modlist_entry);
> -
> - rc = xc_dom_alloc_segment(dom, &dom->start_info_seg,
> - "HVMlite start info", 0, start_info_size);
> - if ( rc != 0 )
> - {
> - DOMPRINTF("Unable to reserve memory for the start info");
> - goto out;
> - }
> }
> else
> {
> + start_info_size +=
> + sizeof(struct hvm_modlist_entry) * HVMLOADER_MODULE_MAX_COUNT;
> + /*
> + * Add extra space to write modules name.
> + * The HVMLOADER_MODULE_NAME_SIZE accounts for NUL byte.
> + */
> + start_info_size +=
> + HVMLOADER_MODULE_NAME_SIZE * HVMLOADER_MODULE_MAX_COUNT;
> +
> /*
> * Allocate and clear additional ioreq server pages. The default
> * server will use the IOREQ and BUFIOREQ special pages above.
> @@ -672,6 +675,14 @@ static int alloc_magic_pages_hvm(struct xc_dom_image *dom)
> NR_IOREQ_SERVER_PAGES);
> }
>
> + rc = xc_dom_alloc_segment(dom, &dom->start_info_seg,
> + "HVMlite start info", 0, start_info_size);
> + if ( rc != 0 )
> + {
> + DOMPRINTF("Unable to reserve memory for the start info");
> + goto out;
> + }
> +
> /*
> * Identity-map page table is required for running with CR0.PG=0 when
> * using Intel EPT. Create a 32-bit non-PAE page directory of superpages.
> @@ -1689,42 +1700,89 @@ static int alloc_pgtables_hvm(struct xc_dom_image *dom)
> return 0;
> }
>
> +/*
> + * The memory layout of the start_info page and the modules, and where the
> + * addresses are stored:
> + *
> + * /----------------------------------\
> + * | struct hvm_start_info |
> + * +----------------------------------+ <- start_info->modlist_paddr
> + * | struct hvm_modlist_entry[0] |
> + * +----------------------------------+
> + * | struct hvm_modlist_entry[1] |
> + * +----------------------------------+ <- modlist[0].cmdline_paddr
> + * | cmdline of module 0 |
> + * | char[HVMLOADER_MODULE_NAME_SIZE] |
> + * +----------------------------------+ <- modlist[1].cmdline_paddr
> + * | cmdline of module 1 |
> + * +----------------------------------+
> + */
Should this go to public/xen.h?
> +static void add_module_to_list(struct xc_dom_image *dom,
> + struct xc_hvm_firmware_module *module,
> + const char *name,
> + struct hvm_modlist_entry *modlist,
> + struct hvm_start_info *start_info)
> +{
> + uint32_t index = start_info->nr_modules;
> + void *modules_cmdline_start = modlist + HVMLOADER_MODULE_MAX_COUNT;
> + uint64_t modlist_paddr = (dom->start_info_seg.pfn << PAGE_SHIFT) +
> + ((uintptr_t)modlist - (uintptr_t)start_info);
> + uint64_t modules_cmdline_paddr = modlist_paddr +
> + sizeof(struct hvm_modlist_entry) * HVMLOADER_MODULE_MAX_COUNT;
> +
> + if ( module->length == 0 )
> + return;
> +
> + assert(start_info->nr_modules < HVMLOADER_MODULE_MAX_COUNT);
> + assert(strnlen(name, HVMLOADER_MODULE_NAME_SIZE)
> + < HVMLOADER_MODULE_NAME_SIZE);
> +
> + modlist[index].paddr = module->guest_addr_out;
> + modlist[index].size = module->length;
> +
> + strncpy(modules_cmdline_start + HVMLOADER_MODULE_NAME_SIZE * index,
> + name, HVMLOADER_MODULE_NAME_SIZE);
> + modlist[index].cmdline_paddr =
> + modules_cmdline_paddr + HVMLOADER_MODULE_NAME_SIZE * index;
> +
> + start_info->nr_modules++;
> +}
> +
> static int bootlate_hvm(struct xc_dom_image *dom)
> {
> uint32_t domid = dom->guest_domid;
> xc_interface *xch = dom->xch;
> + struct hvm_start_info *start_info;
> + size_t start_info_size;
> + struct hvm_modlist_entry *modlist;
>
> - if ( !dom->device_model )
> - {
> - struct hvm_start_info *start_info;
> - size_t start_info_size;
> - void *start_page;
> -
> - start_info_size = sizeof(*start_info) + dom->cmdline_size;
> - if ( dom->ramdisk_blob )
> - start_info_size += sizeof(struct hvm_modlist_entry);
> + start_info_size = sizeof(*start_info) + dom->cmdline_size;
> + if ( dom->ramdisk_blob )
> + start_info_size += sizeof(struct hvm_modlist_entry);
>
> - if ( start_info_size >
> - dom->start_info_seg.pages << XC_DOM_PAGE_SHIFT(dom) )
> - {
> - DOMPRINTF("Trying to map beyond start_info_seg");
> - return -1;
> - }
> + if ( start_info_size >
> + dom->start_info_seg.pages << XC_DOM_PAGE_SHIFT(dom) )
> + {
> + DOMPRINTF("Trying to map beyond start_info_seg");
> + return -1;
> + }
>
> - start_page = xc_map_foreign_range(xch, domid, start_info_size,
> - PROT_READ | PROT_WRITE,
> - dom->start_info_seg.pfn);
> - if ( start_page == NULL )
> - {
> - DOMPRINTF("Unable to map HVM start info page");
> - return -1;
> - }
> + start_info = xc_map_foreign_range(xch, domid, start_info_size,
> + PROT_READ | PROT_WRITE,
> + dom->start_info_seg.pfn);
> + if ( start_info == NULL )
> + {
> + DOMPRINTF("Unable to map HVM start info page");
> + return -1;
> + }
>
> - start_info = start_page;
> + modlist = (void*)(start_info + 1) + dom->cmdline_size;
>
> + if ( !dom->device_model )
> + {
> if ( dom->cmdline )
> {
> - char *cmdline = start_page + sizeof(*start_info);
> + char *cmdline = (void*)(start_info + 1);
>
> strncpy(cmdline, dom->cmdline, dom->cmdline_size);
> start_info->cmdline_paddr = (dom->start_info_seg.pfn << PAGE_SHIFT) +
> @@ -1733,22 +1791,30 @@ static int bootlate_hvm(struct xc_dom_image *dom)
>
> if ( dom->ramdisk_blob )
> {
> - struct hvm_modlist_entry *modlist =
> - start_page + sizeof(*start_info) + dom->cmdline_size;
>
> modlist[0].paddr = dom->ramdisk_seg.vstart - dom->parms.virt_base;
> modlist[0].size = dom->ramdisk_seg.vend - dom->ramdisk_seg.vstart;
> - start_info->modlist_paddr = (dom->start_info_seg.pfn << PAGE_SHIFT) +
> - ((uintptr_t)modlist - (uintptr_t)start_info);
> start_info->nr_modules = 1;
> }
> -
> - start_info->magic = XEN_HVM_START_MAGIC_VALUE;
> -
> - munmap(start_page, start_info_size);
> }
> else
> {
> + add_module_to_list(dom, &dom->system_firmware_module, "firmware",
> + modlist, start_info);
> + }
Is it possible to add PVH's ramdisk via this routine as well?
-boris
> +
> + if ( start_info->nr_modules )
> + {
> + start_info->modlist_paddr = (dom->start_info_seg.pfn << PAGE_SHIFT) +
> + ((uintptr_t)modlist - (uintptr_t)start_info);
> + }
> +
> + start_info->magic = XEN_HVM_START_MAGIC_VALUE;
> +
> + munmap(start_info, start_info_size);
> +
> + if ( dom->device_model )
> + {
> void *hvm_info_page;
>
> if ( (hvm_info_page = xc_map_foreign_range(
> diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h
> index 37bbb22..d9ddee7 100644
> --- a/xen/include/public/xen.h
> +++ b/xen/include/public/xen.h
> @@ -814,7 +814,7 @@ struct start_info {
> typedef struct start_info start_info_t;
>
> /*
> - * Start of day structure passed to PVH guests in %ebx.
> + * Start of day structure passed to PVH guests and to HVM 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.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v5 02/14] libxc: Prepare a start info structure for hvmloader
2016-06-23 14:44 ` Boris Ostrovsky
@ 2016-06-23 16:52 ` Anthony PERARD
0 siblings, 0 replies; 43+ messages in thread
From: Anthony PERARD @ 2016-06-23 16:52 UTC (permalink / raw)
To: Boris Ostrovsky; +Cc: Wei Liu, roger.pau, Ian Jackson, xen-devel
On Thu, Jun 23, 2016 at 10:44:26AM -0400, Boris Ostrovsky wrote:
> On 06/22/2016 01:15 PM, Anthony PERARD wrote:
> > +/*
> > + * The memory layout of the start_info page and the modules, and where the
> > + * addresses are stored:
> > + *
> > + * /----------------------------------\
> > + * | struct hvm_start_info |
> > + * +----------------------------------+ <- start_info->modlist_paddr
> > + * | struct hvm_modlist_entry[0] |
> > + * +----------------------------------+
> > + * | struct hvm_modlist_entry[1] |
> > + * +----------------------------------+ <- modlist[0].cmdline_paddr
> > + * | cmdline of module 0 |
> > + * | char[HVMLOADER_MODULE_NAME_SIZE] |
> > + * +----------------------------------+ <- modlist[1].cmdline_paddr
> > + * | cmdline of module 1 |
> > + * +----------------------------------+
> > + */
>
> Should this go to public/xen.h?
No, it should not. This is to describe how the memory is allocated
and used by this function. The different calculation may be a bit
complicated to follow.
> > +static void add_module_to_list(struct xc_dom_image *dom,
> > + struct xc_hvm_firmware_module *module,
> > + const char *name,
> > + struct hvm_modlist_entry *modlist,
> > + struct hvm_start_info *start_info)
> > +{
> > + uint32_t index = start_info->nr_modules;
> > + void *modules_cmdline_start = modlist + HVMLOADER_MODULE_MAX_COUNT;
> > + uint64_t modlist_paddr = (dom->start_info_seg.pfn << PAGE_SHIFT) +
> > + ((uintptr_t)modlist - (uintptr_t)start_info);
> > + uint64_t modules_cmdline_paddr = modlist_paddr +
> > + sizeof(struct hvm_modlist_entry) * HVMLOADER_MODULE_MAX_COUNT;
> > +
> > + if ( module->length == 0 )
> > + return;
> > +
> > + assert(start_info->nr_modules < HVMLOADER_MODULE_MAX_COUNT);
> > + assert(strnlen(name, HVMLOADER_MODULE_NAME_SIZE)
> > + < HVMLOADER_MODULE_NAME_SIZE);
> > +
> > + modlist[index].paddr = module->guest_addr_out;
> > + modlist[index].size = module->length;
> > +
> > + strncpy(modules_cmdline_start + HVMLOADER_MODULE_NAME_SIZE * index,
> > + name, HVMLOADER_MODULE_NAME_SIZE);
> > + modlist[index].cmdline_paddr =
> > + modules_cmdline_paddr + HVMLOADER_MODULE_NAME_SIZE * index;
> > +
> > + start_info->nr_modules++;
> > +}
> > +
> > static int bootlate_hvm(struct xc_dom_image *dom)
> > {
> > uint32_t domid = dom->guest_domid;
> > xc_interface *xch = dom->xch;
> > + struct hvm_start_info *start_info;
> > + size_t start_info_size;
> > + struct hvm_modlist_entry *modlist;
> >
> > - if ( !dom->device_model )
> > - {
> > - struct hvm_start_info *start_info;
> > - size_t start_info_size;
> > - void *start_page;
> > -
> > - start_info_size = sizeof(*start_info) + dom->cmdline_size;
> > - if ( dom->ramdisk_blob )
> > - start_info_size += sizeof(struct hvm_modlist_entry);
> > + start_info_size = sizeof(*start_info) + dom->cmdline_size;
> > + if ( dom->ramdisk_blob )
> > + start_info_size += sizeof(struct hvm_modlist_entry);
> >
> > - if ( start_info_size >
> > - dom->start_info_seg.pages << XC_DOM_PAGE_SHIFT(dom) )
> > - {
> > - DOMPRINTF("Trying to map beyond start_info_seg");
> > - return -1;
> > - }
> > + if ( start_info_size >
> > + dom->start_info_seg.pages << XC_DOM_PAGE_SHIFT(dom) )
> > + {
> > + DOMPRINTF("Trying to map beyond start_info_seg");
> > + return -1;
> > + }
> >
> > - start_page = xc_map_foreign_range(xch, domid, start_info_size,
> > - PROT_READ | PROT_WRITE,
> > - dom->start_info_seg.pfn);
> > - if ( start_page == NULL )
> > - {
> > - DOMPRINTF("Unable to map HVM start info page");
> > - return -1;
> > - }
> > + start_info = xc_map_foreign_range(xch, domid, start_info_size,
> > + PROT_READ | PROT_WRITE,
> > + dom->start_info_seg.pfn);
> > + if ( start_info == NULL )
> > + {
> > + DOMPRINTF("Unable to map HVM start info page");
> > + return -1;
> > + }
> >
> > - start_info = start_page;
> > + modlist = (void*)(start_info + 1) + dom->cmdline_size;
> >
> > + if ( !dom->device_model )
> > + {
> > if ( dom->cmdline )
> > {
> > - char *cmdline = start_page + sizeof(*start_info);
> > + char *cmdline = (void*)(start_info + 1);
> >
> > strncpy(cmdline, dom->cmdline, dom->cmdline_size);
> > start_info->cmdline_paddr = (dom->start_info_seg.pfn << PAGE_SHIFT) +
> > @@ -1733,22 +1791,30 @@ static int bootlate_hvm(struct xc_dom_image *dom)
> >
> > if ( dom->ramdisk_blob )
> > {
> > - struct hvm_modlist_entry *modlist =
> > - start_page + sizeof(*start_info) + dom->cmdline_size;
> >
> > modlist[0].paddr = dom->ramdisk_seg.vstart - dom->parms.virt_base;
> > modlist[0].size = dom->ramdisk_seg.vend - dom->ramdisk_seg.vstart;
> > - start_info->modlist_paddr = (dom->start_info_seg.pfn << PAGE_SHIFT) +
> > - ((uintptr_t)modlist - (uintptr_t)start_info);
> > start_info->nr_modules = 1;
> > }
> > -
> > - start_info->magic = XEN_HVM_START_MAGIC_VALUE;
> > -
> > - munmap(start_page, start_info_size);
> > }
> > else
> > {
> > + add_module_to_list(dom, &dom->system_firmware_module, "firmware",
> > + modlist, start_info);
> > + }
>
> Is it possible to add PVH's ramdisk via this routine as well?
I guest that could be possible with some change to add_module_to_list,
or with maybe with two different function, one that takes
xc_hvm_firmware_module and another that takes xc_dom_seg. I think I'll
leave this refactoring for another day.
--
Anthony PERARD
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v5 02/14] libxc: Prepare a start info structure for hvmloader
2016-06-22 17:15 ` [PATCH v5 02/14] libxc: Prepare a start info structure for hvmloader Anthony PERARD
2016-06-23 14:44 ` Boris Ostrovsky
@ 2016-07-07 14:55 ` Wei Liu
2016-07-08 10:55 ` Anthony PERARD
1 sibling, 1 reply; 43+ messages in thread
From: Wei Liu @ 2016-07-07 14:55 UTC (permalink / raw)
To: Anthony PERARD
Cc: Wei Liu, boris.ostrovsky, roger.pau, Ian Jackson, xen-devel
On Wed, Jun 22, 2016 at 06:15:33PM +0100, Anthony PERARD wrote:
[...]
> * Allocate and clear additional ioreq server pages. The default
> * server will use the IOREQ and BUFIOREQ special pages above.
> @@ -672,6 +675,14 @@ static int alloc_magic_pages_hvm(struct xc_dom_image *dom)
> NR_IOREQ_SERVER_PAGES);
> }
>
> + rc = xc_dom_alloc_segment(dom, &dom->start_info_seg,
> + "HVMlite start info", 0, start_info_size);
Should we just call this "HVM start info"? It is not restricted to
hvmlite anymore.
Wei.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v5 02/14] libxc: Prepare a start info structure for hvmloader
2016-07-07 14:55 ` Wei Liu
@ 2016-07-08 10:55 ` Anthony PERARD
0 siblings, 0 replies; 43+ messages in thread
From: Anthony PERARD @ 2016-07-08 10:55 UTC (permalink / raw)
To: Wei Liu; +Cc: boris.ostrovsky, roger.pau, Ian Jackson, xen-devel
On Thu, Jul 07, 2016 at 03:55:29PM +0100, Wei Liu wrote:
> On Wed, Jun 22, 2016 at 06:15:33PM +0100, Anthony PERARD wrote:
> [...]
> > * Allocate and clear additional ioreq server pages. The default
> > * server will use the IOREQ and BUFIOREQ special pages above.
> > @@ -672,6 +675,14 @@ static int alloc_magic_pages_hvm(struct xc_dom_image *dom)
> > NR_IOREQ_SERVER_PAGES);
> > }
> >
> > + rc = xc_dom_alloc_segment(dom, &dom->start_info_seg,
> > + "HVMlite start info", 0, start_info_size);
>
> Should we just call this "HVM start info"? It is not restricted to
> hvmlite anymore.
Yes, I think that would make more sense. I'll change that.
--
Anthony PERARD
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v5 03/14] configure: #define SEABIOS_PATH and OVMF_PATH
2016-06-22 17:15 [PATCH v5 00/14] Load BIOS via toolstack instead of been embedded in hvmloader Anthony PERARD
2016-06-22 17:15 ` [PATCH v5 01/14] libxc: Rework extra module initialisation Anthony PERARD
2016-06-22 17:15 ` [PATCH v5 02/14] libxc: Prepare a start info structure for hvmloader Anthony PERARD
@ 2016-06-22 17:15 ` Anthony PERARD
2016-06-22 17:15 ` [PATCH v5 04/14] firmware/makefile: install BIOS blob Anthony PERARD
` (10 subsequent siblings)
13 siblings, 0 replies; 43+ messages in thread
From: Anthony PERARD @ 2016-06-22 17:15 UTC (permalink / raw)
To: xen-devel; +Cc: Anthony PERARD, Wei Liu, Ian Jackson
Those paths are to be used by libxl, in order to load the firmware in
memory. If a system path is not defined via --with-system-seabios or
--with-system-ovmf, then default to the Xen firmware directory.
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Acked-by: Wei Liu <wei.liu2@citrix.com>
---
Please, run ./autogen.sh on this patch.
Change in V5:
- rename seabios.bin to bios.bin.
---
tools/configure.ac | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/tools/configure.ac b/tools/configure.ac
index 8704927..4f75fae 100644
--- a/tools/configure.ac
+++ b/tools/configure.ac
@@ -220,6 +220,9 @@ AC_ARG_WITH([system-seabios],
esac
],[])
AC_SUBST(seabios_path)
+AC_DEFINE_UNQUOTED([SEABIOS_PATH],
+ ["${seabios_path:-$XENFIRMWAREDIR/bios.bin}"],
+ [SeaBIOS path])
AC_ARG_WITH([system-ovmf],
AS_HELP_STRING([--with-system-ovmf@<:@=PATH@:>@],
@@ -231,6 +234,9 @@ AC_ARG_WITH([system-ovmf],
esac
],[])
AC_SUBST(ovmf_path)
+AC_DEFINE_UNQUOTED([OVMF_PATH],
+ ["${ovmf_path:-$XENFIRMWAREDIR/ovmf.bin}"],
+ [OVMF path])
AC_ARG_WITH([extra-qemuu-configure-args],
AS_HELP_STRING([--with-extra-qemuu-configure-args@<:@="--ARG1 ..."@:>@],
--
Anthony PERARD
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 43+ messages in thread* [PATCH v5 04/14] firmware/makefile: install BIOS blob ...
2016-06-22 17:15 [PATCH v5 00/14] Load BIOS via toolstack instead of been embedded in hvmloader Anthony PERARD
` (2 preceding siblings ...)
2016-06-22 17:15 ` [PATCH v5 03/14] configure: #define SEABIOS_PATH and OVMF_PATH Anthony PERARD
@ 2016-06-22 17:15 ` Anthony PERARD
2016-07-07 14:55 ` Wei Liu
2016-06-22 17:15 ` [PATCH v5 05/14] libxl: Load guest BIOS from file Anthony PERARD
` (9 subsequent siblings)
13 siblings, 1 reply; 43+ messages in thread
From: Anthony PERARD @ 2016-06-22 17:15 UTC (permalink / raw)
To: xen-devel; +Cc: Anthony PERARD, Wei Liu, Ian Jackson
... into the firmware directory, along with hvmloader.
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
Change in V5:
- remove use of "variable" for SEABIOS_ROM and OVMF_ROM location
there are static location
- install seabios as bios.bin instead of seabios.bin
Change in V4:
- remove install of acpi dsdt table
Change in V3:
- do not check if ROMs file exist before installing, they should exist
- change rules for dsdt_anycpu_qemu_xen.c in oder to generate both .c and
.aml files without changing temporarly the other dsdt_*.c rules.
---
tools/firmware/Makefile | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/tools/firmware/Makefile b/tools/firmware/Makefile
index 6cc86ce..82b1f6b 100644
--- a/tools/firmware/Makefile
+++ b/tools/firmware/Makefile
@@ -45,6 +45,16 @@ endif
install: all
[ -d $(INST_DIR) ] || $(INSTALL_DIR) $(INST_DIR)
[ ! -e $(TARGET) ] || $(INSTALL_DATA) $(TARGET) $(INST_DIR)
+ifeq ($(CONFIG_SEABIOS),y)
+ifeq ($(SEABIOS_PATH),)
+ $(INSTALL_DATA) seabios-dir/out/bios.bin $(INST_DIR)/bios.bin
+endif
+endif
+ifeq ($(CONFIG_OVMF),y)
+ifeq ($(OVMF_PATH),)
+ $(INSTALL_DATA) ovmf-dir/ovmf.bin $(INST_DIR)/ovmf.bin
+endif
+endif
.PHONY: clean
clean: subdirs-clean
--
Anthony PERARD
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 43+ messages in thread* [PATCH v5 05/14] libxl: Load guest BIOS from file
2016-06-22 17:15 [PATCH v5 00/14] Load BIOS via toolstack instead of been embedded in hvmloader Anthony PERARD
` (3 preceding siblings ...)
2016-06-22 17:15 ` [PATCH v5 04/14] firmware/makefile: install BIOS blob Anthony PERARD
@ 2016-06-22 17:15 ` Anthony PERARD
2016-06-24 7:23 ` Jan Beulich
2016-07-07 14:55 ` Wei Liu
2016-06-22 17:15 ` [PATCH v5 06/14] xen: Move the hvm_start_info C representation from libxc to public/xen.h Anthony PERARD
` (8 subsequent siblings)
13 siblings, 2 replies; 43+ messages in thread
From: Anthony PERARD @ 2016-06-22 17:15 UTC (permalink / raw)
To: xen-devel; +Cc: Anthony PERARD, Wei Liu, Ian Jackson
The path to the BIOS blob can be overriden by the xl's
bios_path_override option, or provided by u.hvm.bios_firmware in the
domain_build_info struct by other libxl user.
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
Changes in V5:
- man page, use B<> to highlight config option in description.
- rename config option from `bios_override` to `bios_path_override`
- store libxl_read_file_contents() return value into r instead of e
(just renamed the variable)
- rename domain_build_info.u.hvm.bios_firmware to system_firmware
Changes in V4:
- updating man page to have bios_override described.
- return ERROR_INVAL in libxl__load_hvm_firmware_module when the file is
empty.
Changes in V3:
- move seabios_path and ovmf_path to libxl_path.c (with renaming)
- fix some coding style
- warn for empty file
- remove rombios stuff (will still be built-in hvmloader)
- rename field bios_filename in domain_build_info to bios_firmware to
follow naming of acpi and smbios.
- log an error after libxl_read_file_contents() only when it return ENOENT
- return an error on empty file.
- added #define LIBXL_HAVE_BUILDINFO_HVM_BIOS_FIRMWARE
---
docs/man/xl.cfg.pod.5.in | 9 +++++++
tools/libxl/libxl.h | 8 +++++++
tools/libxl/libxl_dom.c | 57 ++++++++++++++++++++++++++++++++++++++++++++
tools/libxl/libxl_internal.h | 2 ++
tools/libxl/libxl_paths.c | 10 ++++++++
tools/libxl/libxl_types.idl | 1 +
tools/libxl/xl_cmdimpl.c | 11 ++++++---
7 files changed, 95 insertions(+), 3 deletions(-)
diff --git a/docs/man/xl.cfg.pod.5.in b/docs/man/xl.cfg.pod.5.in
index 3bb27d0..a685b83 100644
--- a/docs/man/xl.cfg.pod.5.in
+++ b/docs/man/xl.cfg.pod.5.in
@@ -1212,6 +1212,15 @@ Requires device_model_version=qemu-xen.
=back
+=item B<bios_path_override="PATH">
+
+Override the path to the blob to be used as BIOS. The blob provided here MUST
+be consistent with the B<bios=> which you have specified. You should not
+normally need to specify this option.
+
+This options does not have any effect if using B<bios="rombios"> or
+B<device_model_version="qemu-xen-traditional">.
+
=item B<pae=BOOLEAN>
Hide or expose the IA32 Physical Address Extensions. These extensions
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 2c0f868..2b1f678 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -928,6 +928,14 @@ void libxl_mac_copy(libxl_ctx *ctx, libxl_mac *dst, libxl_mac *src);
#define LIBXL_HAVE_CHECKPOINTED_STREAM 1
/*
+ * LIBXL_HAVE_BUILDINFO_HVM_SYSTEM_FIRMWARE
+ *
+ * libxl_domain_build_info has u.hvm.system_firmware field which can be use
+ * to provide a different firmware blob (like SeaBIOS or OVMF).
+ */
+#define LIBXL_HAVE_BUILDINFO_HVM_SYSTEM_FIRMWARE
+
+/*
* ERROR_REMUS_XXX error code only exists from Xen 4.5, Xen 4.6 and it
* is changed to ERROR_CHECKPOINT_XXX in Xen 4.7
*/
diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index ec29060..c341a29 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -862,6 +862,38 @@ err:
return ret;
}
+static int libxl__load_hvm_firmware_module(libxl__gc *gc,
+ const char *filename,
+ const char *what,
+ struct xc_hvm_firmware_module *m)
+{
+ int datalen = 0;
+ void *data = NULL;
+ int r;
+
+ LOG(DEBUG, "Loading %s: %s", what, filename);
+ r = libxl_read_file_contents(CTX, filename, &data, &datalen);
+ if (r) {
+ /*
+ * Print a message only on ENOENT, other errors are logged by the
+ * function libxl_read_file_contents().
+ */
+ if (r == ENOENT)
+ LOGEV(ERROR, r, "failed to read %s file", what);
+ return ERROR_FAIL;
+ }
+ libxl__ptr_add(gc, data);
+ if (datalen) {
+ /* Only accept non-empty files */
+ m->data = data;
+ m->length = datalen;
+ } else {
+ LOG(ERROR, "file %s for %s is empty", filename, what);
+ return ERROR_INVAL;
+ }
+ return 0;
+}
+
static int libxl__domain_firmware(libxl__gc *gc,
libxl_domain_build_info *info,
struct xc_dom_image *dom)
@@ -871,6 +903,7 @@ static int libxl__domain_firmware(libxl__gc *gc,
int e, rc;
int datalen = 0;
void *data;
+ const char *bios_filename = NULL;
if (info->u.hvm.firmware)
firmware = info->u.hvm.firmware;
@@ -914,6 +947,30 @@ static int libxl__domain_firmware(libxl__gc *gc,
goto out;
}
+ if (info->device_model_version == LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN) {
+ if (info->u.hvm.system_firmware) {
+ bios_filename = info->u.hvm.system_firmware;
+ } else {
+ switch (info->u.hvm.bios) {
+ case LIBXL_BIOS_TYPE_SEABIOS:
+ bios_filename = libxl__seabios_path();
+ break;
+ case LIBXL_BIOS_TYPE_OVMF:
+ bios_filename = libxl__ovmf_path();
+ break;
+ case LIBXL_BIOS_TYPE_ROMBIOS:
+ default:
+ abort();
+ }
+ }
+ }
+
+ if (bios_filename) {
+ rc = libxl__load_hvm_firmware_module(gc, bios_filename, "BIOS",
+ &dom->system_firmware_module);
+ if (rc) goto out;
+ }
+
if (info->u.hvm.smbios_firmware) {
data = NULL;
e = libxl_read_file_contents(ctx, info->u.hvm.smbios_firmware,
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index e7ab85d..e6a199b 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -2317,6 +2317,8 @@ _hidden const char *libxl__xen_config_dir_path(void);
_hidden const char *libxl__xen_script_dir_path(void);
_hidden const char *libxl__lock_dir_path(void);
_hidden const char *libxl__run_dir_path(void);
+_hidden const char *libxl__seabios_path(void);
+_hidden const char *libxl__ovmf_path(void);
/*----- subprocess execution with timeout -----*/
diff --git a/tools/libxl/libxl_paths.c b/tools/libxl/libxl_paths.c
index 9b7b0d5..6972b90 100644
--- a/tools/libxl/libxl_paths.c
+++ b/tools/libxl/libxl_paths.c
@@ -35,6 +35,16 @@ const char *libxl__run_dir_path(void)
return XEN_RUN_DIR;
}
+const char *libxl__seabios_path(void)
+{
+ return SEABIOS_PATH;
+}
+
+const char *libxl__ovmf_path(void)
+{
+ return OVMF_PATH;
+}
+
/*
* Local variables:
* mode: C
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index ef614be..98bfc3a 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -513,6 +513,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
("timer_mode", libxl_timer_mode),
("nested_hvm", libxl_defbool),
("altp2m", libxl_defbool),
+ ("system_firmware", string),
("smbios_firmware", string),
("acpi_firmware", string),
("hdtype", libxl_hdtype),
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 6459eec..18da7ab 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -1562,12 +1562,17 @@ static void parse_config_data(const char *config_source,
xlu_cfg_replace_string (config, "firmware_override",
&b_info->u.hvm.firmware, 0);
- if (!xlu_cfg_get_string(config, "bios", &buf, 0) &&
- libxl_bios_type_from_string(buf, &b_info->u.hvm.bios)) {
+ xlu_cfg_replace_string (config, "bios_path_override",
+ &b_info->u.hvm.system_firmware, 0);
+ if (!xlu_cfg_get_string(config, "bios", &buf, 0)) {
+ if (libxl_bios_type_from_string(buf, &b_info->u.hvm.bios)) {
fprintf(stderr, "ERROR: invalid value \"%s\" for \"bios\"\n",
buf);
exit (1);
- }
+ }
+ } else if (b_info->u.hvm.system_firmware)
+ fprintf(stderr, "WARNING: "
+ "bios_path_override given without specific bios name\n");
xlu_cfg_get_defbool(config, "pae", &b_info->u.hvm.pae, 0);
xlu_cfg_get_defbool(config, "apic", &b_info->u.hvm.apic, 0);
--
Anthony PERARD
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH v5 05/14] libxl: Load guest BIOS from file
2016-06-22 17:15 ` [PATCH v5 05/14] libxl: Load guest BIOS from file Anthony PERARD
@ 2016-06-24 7:23 ` Jan Beulich
2016-06-24 14:20 ` Anthony PERARD
2016-07-07 14:55 ` Wei Liu
1 sibling, 1 reply; 43+ messages in thread
From: Jan Beulich @ 2016-06-24 7:23 UTC (permalink / raw)
To: Anthony PERARD; +Cc: Ian Jackson, Jim Fehlig, Wei Liu, xen-devel
>>> On 22.06.16 at 19:15, <anthony.perard@citrix.com> wrote:
> --- a/tools/libxl/libxl_paths.c
> +++ b/tools/libxl/libxl_paths.c
> @@ -35,6 +35,16 @@ const char *libxl__run_dir_path(void)
> return XEN_RUN_DIR;
> }
>
> +const char *libxl__seabios_path(void)
> +{
> + return SEABIOS_PATH;
> +}
> +
> +const char *libxl__ovmf_path(void)
> +{
> + return OVMF_PATH;
> +}
With an earlier version of this series pulled into one of our branches,
I've run into a problem with this: The paths you return here are the
configured paths, and that's intended. Yet it breaks running the
tools out of the build area (i.e. without any "make install"), which so
far has been working fine (as apparently in all other relevant cases
where paths are needed, relative ones are being used), and which
I much prefer over the hassle of scattering around half a dozen of
different Xen tools versions in custom directories under, say,
/usr/local. I guess if I'm the only one using this, I'll have to find my
own local solution for this, but of course I'd prefer for this currently
working case not to get broken.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v5 05/14] libxl: Load guest BIOS from file
2016-06-24 7:23 ` Jan Beulich
@ 2016-06-24 14:20 ` Anthony PERARD
0 siblings, 0 replies; 43+ messages in thread
From: Anthony PERARD @ 2016-06-24 14:20 UTC (permalink / raw)
To: Jan Beulich; +Cc: Ian Jackson, Jim Fehlig, Wei Liu, xen-devel
On Fri, Jun 24, 2016 at 01:23:19AM -0600, Jan Beulich wrote:
> >>> On 22.06.16 at 19:15, <anthony.perard@citrix.com> wrote:
> > --- a/tools/libxl/libxl_paths.c
> > +++ b/tools/libxl/libxl_paths.c
> > @@ -35,6 +35,16 @@ const char *libxl__run_dir_path(void)
> > return XEN_RUN_DIR;
> > }
> >
> > +const char *libxl__seabios_path(void)
> > +{
> > + return SEABIOS_PATH;
> > +}
> > +
> > +const char *libxl__ovmf_path(void)
> > +{
> > + return OVMF_PATH;
> > +}
>
> With an earlier version of this series pulled into one of our branches,
> I've run into a problem with this: The paths you return here are the
> configured paths, and that's intended. Yet it breaks running the
> tools out of the build area (i.e. without any "make install"), which so
> far has been working fine (as apparently in all other relevant cases
> where paths are needed, relative ones are being used), and which
I'm not sure that true about the relative paths, I think most, if not
all are full path and happen to match what is already install on the
system. I've tried to run xl from the build dir (and also dist dir) and
with a different --prefix, xl can not find qemu and if I run xl, this
time configure with the same --prefix and I remove hvmloader from my
system, xl can not find hvmloader.
You could copy both firmware manually to be in the same directory as
hvmloader. A better solution would be to use all the _override xl
config, I've added `bios_path_override' so one can supply a different
bios/firmware to libxl.
To be honest, I don't know which relative path to use since it depend on
where is xl executed from. And I don't think the path to the firmwares
(like hvmloader) can be change at execution time unless change for each
of them.
Is that answer your question?
> I much prefer over the hassle of scattering around half a dozen of
> different Xen tools versions in custom directories under, say,
> /usr/local. I guess if I'm the only one using this, I'll have to find my
> own local solution for this, but of course I'd prefer for this currently
> working case not to get broken.
>
> Jan
>
--
Anthony PERARD
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v5 05/14] libxl: Load guest BIOS from file
2016-06-22 17:15 ` [PATCH v5 05/14] libxl: Load guest BIOS from file Anthony PERARD
2016-06-24 7:23 ` Jan Beulich
@ 2016-07-07 14:55 ` Wei Liu
1 sibling, 0 replies; 43+ messages in thread
From: Wei Liu @ 2016-07-07 14:55 UTC (permalink / raw)
To: Anthony PERARD; +Cc: Wei Liu, Ian Jackson, xen-devel
On Wed, Jun 22, 2016 at 06:15:36PM +0100, Anthony PERARD wrote:
> The path to the BIOS blob can be overriden by the xl's
> bios_path_override option, or provided by u.hvm.bios_firmware in the
> domain_build_info struct by other libxl user.
>
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
>
> ---
> Changes in V5:
> - man page, use B<> to highlight config option in description.
> - rename config option from `bios_override` to `bios_path_override`
> - store libxl_read_file_contents() return value into r instead of e
> (just renamed the variable)
> - rename domain_build_info.u.hvm.bios_firmware to system_firmware
>
> Changes in V4:
> - updating man page to have bios_override described.
> - return ERROR_INVAL in libxl__load_hvm_firmware_module when the file is
> empty.
>
> Changes in V3:
> - move seabios_path and ovmf_path to libxl_path.c (with renaming)
> - fix some coding style
> - warn for empty file
> - remove rombios stuff (will still be built-in hvmloader)
> - rename field bios_filename in domain_build_info to bios_firmware to
> follow naming of acpi and smbios.
> - log an error after libxl_read_file_contents() only when it return ENOENT
> - return an error on empty file.
> - added #define LIBXL_HAVE_BUILDINFO_HVM_BIOS_FIRMWARE
> ---
> docs/man/xl.cfg.pod.5.in | 9 +++++++
> tools/libxl/libxl.h | 8 +++++++
> tools/libxl/libxl_dom.c | 57 ++++++++++++++++++++++++++++++++++++++++++++
> tools/libxl/libxl_internal.h | 2 ++
> tools/libxl/libxl_paths.c | 10 ++++++++
> tools/libxl/libxl_types.idl | 1 +
> tools/libxl/xl_cmdimpl.c | 11 ++++++---
> 7 files changed, 95 insertions(+), 3 deletions(-)
>
> diff --git a/docs/man/xl.cfg.pod.5.in b/docs/man/xl.cfg.pod.5.in
> index 3bb27d0..a685b83 100644
> --- a/docs/man/xl.cfg.pod.5.in
> +++ b/docs/man/xl.cfg.pod.5.in
> @@ -1212,6 +1212,15 @@ Requires device_model_version=qemu-xen.
>
> =back
>
> +=item B<bios_path_override="PATH">
> +
> +Override the path to the blob to be used as BIOS. The blob provided here MUST
> +be consistent with the B<bios=> which you have specified. You should not
> +normally need to specify this option.
> +
> +This options does not have any effect if using B<bios="rombios"> or
> +B<device_model_version="qemu-xen-traditional">.
> +
> =item B<pae=BOOLEAN>
>
> Hide or expose the IA32 Physical Address Extensions. These extensions
> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index 2c0f868..2b1f678 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -928,6 +928,14 @@ void libxl_mac_copy(libxl_ctx *ctx, libxl_mac *dst, libxl_mac *src);
> #define LIBXL_HAVE_CHECKPOINTED_STREAM 1
>
> /*
> + * LIBXL_HAVE_BUILDINFO_HVM_SYSTEM_FIRMWARE
> + *
> + * libxl_domain_build_info has u.hvm.system_firmware field which can be use
> + * to provide a different firmware blob (like SeaBIOS or OVMF).
> + */
> +#define LIBXL_HAVE_BUILDINFO_HVM_SYSTEM_FIRMWARE
> +
> +/*
> * ERROR_REMUS_XXX error code only exists from Xen 4.5, Xen 4.6 and it
> * is changed to ERROR_CHECKPOINT_XXX in Xen 4.7
> */
> diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
> index ec29060..c341a29 100644
> --- a/tools/libxl/libxl_dom.c
> +++ b/tools/libxl/libxl_dom.c
> @@ -862,6 +862,38 @@ err:
> return ret;
> }
>
> +static int libxl__load_hvm_firmware_module(libxl__gc *gc,
> + const char *filename,
> + const char *what,
> + struct xc_hvm_firmware_module *m)
> +{
> + int datalen = 0;
> + void *data = NULL;
> + int r;
> +
> + LOG(DEBUG, "Loading %s: %s", what, filename);
> + r = libxl_read_file_contents(CTX, filename, &data, &datalen);
> + if (r) {
> + /*
> + * Print a message only on ENOENT, other errors are logged by the
> + * function libxl_read_file_contents().
> + */
> + if (r == ENOENT)
> + LOGEV(ERROR, r, "failed to read %s file", what);
> + return ERROR_FAIL;
> + }
> + libxl__ptr_add(gc, data);
> + if (datalen) {
> + /* Only accept non-empty files */
> + m->data = data;
> + m->length = datalen;
> + } else {
> + LOG(ERROR, "file %s for %s is empty", filename, what);
> + return ERROR_INVAL;
> + }
> + return 0;
> +}
> +
Please use goto style error handling to be consistent with other code.
Wei.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v5 06/14] xen: Move the hvm_start_info C representation from libxc to public/xen.h
2016-06-22 17:15 [PATCH v5 00/14] Load BIOS via toolstack instead of been embedded in hvmloader Anthony PERARD
` (4 preceding siblings ...)
2016-06-22 17:15 ` [PATCH v5 05/14] libxl: Load guest BIOS from file Anthony PERARD
@ 2016-06-22 17:15 ` Anthony PERARD
2016-07-07 14:55 ` Wei Liu
2016-07-07 15:02 ` Andrew Cooper
2016-06-22 17:15 ` [PATCH v5 07/14] hvmloader: Grab the hvm_start_info pointer Anthony PERARD
` (7 subsequent siblings)
13 siblings, 2 replies; 43+ messages in thread
From: Anthony PERARD @ 2016-06-22 17:15 UTC (permalink / raw)
To: xen-devel; +Cc: Anthony PERARD, Wei Liu, Ian Jackson
Instead of having several representation of hvm_start_info in C, define
it in public/xen.h so both libxc and hvmloader can use it.
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
Change in V5:
- remove packed attribute.
New in V4.
---
tools/libxc/include/xc_dom.h | 31 -------------------------------
xen/include/public/xen.h | 27 +++++++++++++++++++++++++++
2 files changed, 27 insertions(+), 31 deletions(-)
diff --git a/tools/libxc/include/xc_dom.h b/tools/libxc/include/xc_dom.h
index 0629971..de7dca9 100644
--- a/tools/libxc/include/xc_dom.h
+++ b/tools/libxc/include/xc_dom.h
@@ -219,37 +219,6 @@ struct xc_dom_image {
struct xc_hvm_firmware_module smbios_module;
};
-#if defined(__i386__) || defined(__x86_64__)
-/* C representation of the x86/HVM start info layout.
- *
- * The canonical definition of this layout resides in public/xen.h, this
- * is just a way to represent the layout described there using C types.
- *
- * NB: the packed attribute is not really needed, but it helps us enforce
- * the fact this this is just a representation, and it might indeed
- * be required in the future if there are alignment changes.
- */
-struct hvm_start_info {
- uint32_t magic; /* Contains the magic value 0x336ec578 */
- /* ("xEn3" with the 0x80 bit of the "E" set).*/
- uint32_t version; /* Version of this structure. */
- uint32_t flags; /* SIF_xxx flags. */
- uint32_t nr_modules; /* Number of modules passed to the kernel. */
- uint64_t modlist_paddr; /* Physical address of an array of */
- /* hvm_modlist_entry. */
- uint64_t cmdline_paddr; /* Physical address of the command line. */
- uint64_t rsdp_paddr; /* Physical address of the RSDP ACPI data */
- /* structure. */
-} __attribute__((packed));
-
-struct hvm_modlist_entry {
- uint64_t paddr; /* Physical address of the module. */
- uint64_t size; /* Size of the module in bytes. */
- uint64_t cmdline_paddr; /* Physical address of the command line. */
- uint64_t reserved;
-} __attribute__((packed));
-#endif /* x86 */
-
/* --- pluggable kernel loader ------------------------------------- */
struct xc_dom_loader {
diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h
index d9ddee7..defde97 100644
--- a/xen/include/public/xen.h
+++ b/xen/include/public/xen.h
@@ -859,6 +859,33 @@ typedef struct start_info start_info_t;
*/
#define XEN_HVM_START_MAGIC_VALUE 0x336ec578
+#if defined(__i386__) || defined(__x86_64__)
+/* C representation of the x86/HVM start info layout.
+ *
+ * The canonical definition of this layout is abrove, this is just a way to
+ * represent the layout described there using C types.
+ */
+struct hvm_start_info {
+ uint32_t magic; /* Contains the magic value 0x336ec578 */
+ /* ("xEn3" with the 0x80 bit of the "E" set).*/
+ uint32_t version; /* Version of this structure. */
+ uint32_t flags; /* SIF_xxx flags. */
+ uint32_t nr_modules; /* Number of modules passed to the kernel. */
+ uint64_t modlist_paddr; /* Physical address of an array of */
+ /* hvm_modlist_entry. */
+ uint64_t cmdline_paddr; /* Physical address of the command line. */
+ uint64_t rsdp_paddr; /* Physical address of the RSDP ACPI data */
+ /* structure. */
+};
+
+struct hvm_modlist_entry {
+ uint64_t paddr; /* Physical address of the module. */
+ uint64_t size; /* Size of the module in bytes. */
+ uint64_t cmdline_paddr; /* Physical address of the command line. */
+ uint64_t reserved;
+};
+#endif /* x86 */
+
/* New console union for dom0 introduced in 0x00030203. */
#if __XEN_INTERFACE_VERSION__ < 0x00030203
#define console_mfn console.domU.mfn
--
Anthony PERARD
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH v5 06/14] xen: Move the hvm_start_info C representation from libxc to public/xen.h
2016-06-22 17:15 ` [PATCH v5 06/14] xen: Move the hvm_start_info C representation from libxc to public/xen.h Anthony PERARD
@ 2016-07-07 14:55 ` Wei Liu
2016-07-07 15:07 ` Jan Beulich
2016-07-07 15:02 ` Andrew Cooper
1 sibling, 1 reply; 43+ messages in thread
From: Wei Liu @ 2016-07-07 14:55 UTC (permalink / raw)
To: Anthony PERARD
Cc: Wei Liu, Andrew Cooper, Ian Jackson, Jan Beulich, xen-devel
Cc HV maintainers
I'm of course fine with moving this structure somewhere else.
On Wed, Jun 22, 2016 at 06:15:37PM +0100, Anthony PERARD wrote:
> diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h
I suspect it would make more sense to move it to public/arch-x86/xen.h.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v5 06/14] xen: Move the hvm_start_info C representation from libxc to public/xen.h
2016-07-07 14:55 ` Wei Liu
@ 2016-07-07 15:07 ` Jan Beulich
2016-07-07 15:28 ` Wei Liu
0 siblings, 1 reply; 43+ messages in thread
From: Jan Beulich @ 2016-07-07 15:07 UTC (permalink / raw)
To: Anthony PERARD, WeiLiu; +Cc: Andrew Cooper, Ian Jackson, xen-devel
>>> On 07.07.16 at 16:55, <wei.liu2@citrix.com> wrote:
> Cc HV maintainers
>
> I'm of course fine with moving this structure somewhere else.
>
> On Wed, Jun 22, 2016 at 06:15:37PM +0100, Anthony PERARD wrote:
>
>> diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h
>
>
> I suspect it would make more sense to move it to public/arch-x86/xen.h.
The question is whether we really mean this to remain x86 specific:
The way it's now there's nothing inherently x86-ish there. But if
that's the plan (which the conditional around it supports) then the
suggested alternative resting place seems appropriate to me.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v5 06/14] xen: Move the hvm_start_info C representation from libxc to public/xen.h
2016-07-07 15:07 ` Jan Beulich
@ 2016-07-07 15:28 ` Wei Liu
2016-07-08 9:53 ` Julien Grall
0 siblings, 1 reply; 43+ messages in thread
From: Wei Liu @ 2016-07-07 15:28 UTC (permalink / raw)
To: Jan Beulich
Cc: Stefano Stabellini, WeiLiu, Andrew Cooper, Ian Jackson,
xen-devel, Julien Grall, Anthony PERARD
On Thu, Jul 07, 2016 at 09:07:59AM -0600, Jan Beulich wrote:
> >>> On 07.07.16 at 16:55, <wei.liu2@citrix.com> wrote:
> > Cc HV maintainers
> >
> > I'm of course fine with moving this structure somewhere else.
> >
> > On Wed, Jun 22, 2016 at 06:15:37PM +0100, Anthony PERARD wrote:
> >
> >> diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h
> >
> >
> > I suspect it would make more sense to move it to public/arch-x86/xen.h.
>
> The question is whether we really mean this to remain x86 specific:
> The way it's now there's nothing inherently x86-ish there. But if
> that's the plan (which the conditional around it supports) then the
> suggested alternative resting place seems appropriate to me.
>
For one, ARM doesn't distinguish PV vs HVM vs PVH (yet). Calling it HVM
for ARM would be wrong IMHO.
I've CC'ed Stefano and Julien to let them express their opinions.
Wei.
> Jan
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v5 06/14] xen: Move the hvm_start_info C representation from libxc to public/xen.h
2016-07-07 15:28 ` Wei Liu
@ 2016-07-08 9:53 ` Julien Grall
0 siblings, 0 replies; 43+ messages in thread
From: Julien Grall @ 2016-07-08 9:53 UTC (permalink / raw)
To: Wei Liu, Jan Beulich
Cc: Anthony PERARD, Andrew Cooper, Stefano Stabellini, Ian Jackson,
xen-devel
Hi Wei,
On 07/07/16 16:28, Wei Liu wrote:
> On Thu, Jul 07, 2016 at 09:07:59AM -0600, Jan Beulich wrote:
>>>>> On 07.07.16 at 16:55, <wei.liu2@citrix.com> wrote:
>>> Cc HV maintainers
>>>
>>> I'm of course fine with moving this structure somewhere else.
>>>
>>> On Wed, Jun 22, 2016 at 06:15:37PM +0100, Anthony PERARD wrote:
>>>
>>>> diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h
>>>
>>>
>>> I suspect it would make more sense to move it to public/arch-x86/xen.h.
>>
>> The question is whether we really mean this to remain x86 specific:
>> The way it's now there's nothing inherently x86-ish there. But if
>> that's the plan (which the conditional around it supports) then the
>> suggested alternative resting place seems appropriate to me.
>>
>
> For one, ARM doesn't distinguish PV vs HVM vs PVH (yet). Calling it HVM
> for ARM would be wrong IMHO.
Correct, if you would have to do a comparison with x86 it would be PVH.
However, this structure looks useful only if we lack a way to describe
those parameters in the firmware. This is not the case for ARM as this
could be described easily by the device tree with the generic bindings.
Regards,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v5 06/14] xen: Move the hvm_start_info C representation from libxc to public/xen.h
2016-06-22 17:15 ` [PATCH v5 06/14] xen: Move the hvm_start_info C representation from libxc to public/xen.h Anthony PERARD
2016-07-07 14:55 ` Wei Liu
@ 2016-07-07 15:02 ` Andrew Cooper
2016-07-07 15:09 ` Jan Beulich
1 sibling, 1 reply; 43+ messages in thread
From: Andrew Cooper @ 2016-07-07 15:02 UTC (permalink / raw)
To: Anthony PERARD, xen-devel; +Cc: Ian Jackson, Wei Liu
On 22/06/16 18:15, Anthony PERARD wrote:
> Instead of having several representation of hvm_start_info in C, define
> it in public/xen.h so both libxc and hvmloader can use it.
>
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
> Change in V5:
> - remove packed attribute.
>
> New in V4.
> ---
> tools/libxc/include/xc_dom.h | 31 -------------------------------
> xen/include/public/xen.h | 27 +++++++++++++++++++++++++++
+1 to the public interface, but I would recommend introducing a new file
public/hvm/start_info.h rather than extending the general dumping ground
which is xen.h
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v5 06/14] xen: Move the hvm_start_info C representation from libxc to public/xen.h
2016-07-07 15:02 ` Andrew Cooper
@ 2016-07-07 15:09 ` Jan Beulich
2016-07-07 15:12 ` Andrew Cooper
0 siblings, 1 reply; 43+ messages in thread
From: Jan Beulich @ 2016-07-07 15:09 UTC (permalink / raw)
To: Andrew Cooper, Anthony PERARD; +Cc: Ian Jackson, Wei Liu, xen-devel
>>> On 07.07.16 at 17:02, <andrew.cooper3@citrix.com> wrote:
> On 22/06/16 18:15, Anthony PERARD wrote:
>> Instead of having several representation of hvm_start_info in C, define
>> it in public/xen.h so both libxc and hvmloader can use it.
>>
>> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
>> ---
>> Change in V5:
>> - remove packed attribute.
>>
>> New in V4.
>> ---
>> tools/libxc/include/xc_dom.h | 31 -------------------------------
>> xen/include/public/xen.h | 27 +++++++++++++++++++++++++++
>
> +1 to the public interface, but I would recommend introducing a new file
> public/hvm/start_info.h rather than extending the general dumping ground
> which is xen.h
But then - see my other reply - perhaps public/arch-x86/hvm/start_info.h?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v5 06/14] xen: Move the hvm_start_info C representation from libxc to public/xen.h
2016-07-07 15:09 ` Jan Beulich
@ 2016-07-07 15:12 ` Andrew Cooper
0 siblings, 0 replies; 43+ messages in thread
From: Andrew Cooper @ 2016-07-07 15:12 UTC (permalink / raw)
To: Jan Beulich, Anthony PERARD; +Cc: Ian Jackson, Wei Liu, xen-devel
On 07/07/16 16:09, Jan Beulich wrote:
>>>> On 07.07.16 at 17:02, <andrew.cooper3@citrix.com> wrote:
>> On 22/06/16 18:15, Anthony PERARD wrote:
>>> Instead of having several representation of hvm_start_info in C, define
>>> it in public/xen.h so both libxc and hvmloader can use it.
>>>
>>> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
>>> ---
>>> Change in V5:
>>> - remove packed attribute.
>>>
>>> New in V4.
>>> ---
>>> tools/libxc/include/xc_dom.h | 31 -------------------------------
>>> xen/include/public/xen.h | 27 +++++++++++++++++++++++++++
>> +1 to the public interface, but I would recommend introducing a new file
>> public/hvm/start_info.h rather than extending the general dumping ground
>> which is xen.h
> But then - see my other reply - perhaps public/arch-x86/hvm/start_info.h?
Fine by me.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v5 07/14] hvmloader: Grab the hvm_start_info pointer
2016-06-22 17:15 [PATCH v5 00/14] Load BIOS via toolstack instead of been embedded in hvmloader Anthony PERARD
` (5 preceding siblings ...)
2016-06-22 17:15 ` [PATCH v5 06/14] xen: Move the hvm_start_info C representation from libxc to public/xen.h Anthony PERARD
@ 2016-06-22 17:15 ` Anthony PERARD
2016-06-22 17:15 ` [PATCH v5 08/14] hvmloader: Locate the BIOS blob Anthony PERARD
` (6 subsequent siblings)
13 siblings, 0 replies; 43+ messages in thread
From: Anthony PERARD @ 2016-06-22 17:15 UTC (permalink / raw)
To: xen-devel
Cc: Anthony PERARD, Andrew Cooper, Ian Jackson, Wei Liu, Jan Beulich
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
---
Change in V4:
- remove struct hvm_info_start redefinition, as it's moved to
public/xen.h in a previous patch.
Change in V3:
- remove cmdline parser
- load hvm_start_info pointer earlier, before calling main().
- Add struct hvm_start_info definition to hvmloader.
---
tools/firmware/hvmloader/hvmloader.c | 5 +++++
tools/firmware/hvmloader/util.h | 3 +++
2 files changed, 8 insertions(+)
diff --git a/tools/firmware/hvmloader/hvmloader.c b/tools/firmware/hvmloader/hvmloader.c
index 716d03c..c45f367 100644
--- a/tools/firmware/hvmloader/hvmloader.c
+++ b/tools/firmware/hvmloader/hvmloader.c
@@ -29,6 +29,8 @@
#include <xen/version.h>
#include <xen/hvm/params.h>
+const struct hvm_start_info *hvm_start_info;
+
asm (
" .text \n"
" .globl _start \n"
@@ -46,6 +48,8 @@ asm (
" ljmp $"STR(SEL_CODE32)",$1f \n"
"1: movl $stack_top,%esp \n"
" movl %esp,%ebp \n"
+ /* store HVM start info ptr */
+ " mov %ebx, hvm_start_info \n"
" call main \n"
/* Relocate real-mode trampoline to 0x0. */
" mov $trampoline_start,%esi \n"
@@ -258,6 +262,7 @@ int main(void)
memset((void *)HYPERCALL_PHYSICAL_ADDRESS, 0xc3 /* RET */, PAGE_SIZE);
printf("HVM Loader\n");
+ BUG_ON(hvm_start_info->magic != XEN_HVM_START_MAGIC_VALUE);
init_hypercalls();
diff --git a/tools/firmware/hvmloader/util.h b/tools/firmware/hvmloader/util.h
index 3126817..9808016 100644
--- a/tools/firmware/hvmloader/util.h
+++ b/tools/firmware/hvmloader/util.h
@@ -158,6 +158,9 @@ static inline void cpu_relax(void)
struct hvm_info_table *get_hvm_info_table(void) __attribute__ ((const));
#define hvm_info (get_hvm_info_table())
+/* HVM start info */
+extern const struct hvm_start_info *hvm_start_info;
+
/* String and memory functions */
int strcmp(const char *cs, const char *ct);
int strncmp(const char *s1, const char *s2, uint32_t n);
--
Anthony PERARD
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 43+ messages in thread* [PATCH v5 08/14] hvmloader: Locate the BIOS blob
2016-06-22 17:15 [PATCH v5 00/14] Load BIOS via toolstack instead of been embedded in hvmloader Anthony PERARD
` (6 preceding siblings ...)
2016-06-22 17:15 ` [PATCH v5 07/14] hvmloader: Grab the hvm_start_info pointer Anthony PERARD
@ 2016-06-22 17:15 ` Anthony PERARD
2016-06-24 7:33 ` Jan Beulich
2016-06-22 17:15 ` [PATCH v5 09/14] hvmloader: Check modules whereabouts in perform_tests Anthony PERARD
` (5 subsequent siblings)
13 siblings, 1 reply; 43+ messages in thread
From: Anthony PERARD @ 2016-06-22 17:15 UTC (permalink / raw)
To: xen-devel
Cc: Anthony PERARD, Andrew Cooper, Ian Jackson, Wei Liu, Jan Beulich
The BIOS blob can be found an entry called "firmware" of the modlist of
the hvm_start_info struct.
The found BIOS blob is not loaded by this patch, but only passed as
argument to bios_load() function.
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
Changes in V5:
- don't BUG() on module's paddr having value 0, and just skip.
- fix some coding style
- rename module name to "firmware" (was "bios")
- less use of BUG_ON in get_module_entry() and skip entries instead.
Only BUG() if the module which match name is not accessible.
Changes in V4:
- add more BUG_ON into get_module_entry(). Check that modules paddr and
size are 32bits.
Changes in V3:
- fix some codying style
- use module.cmdline to look for a module name instead of the main cmdline
from hvm_start_info.
---
tools/firmware/hvmloader/config.h | 2 +-
tools/firmware/hvmloader/hvmloader.c | 54 ++++++++++++++++++++++++++++++++++--
tools/firmware/hvmloader/ovmf.c | 3 +-
tools/firmware/hvmloader/rombios.c | 3 +-
tools/firmware/hvmloader/util.h | 2 ++
5 files changed, 59 insertions(+), 5 deletions(-)
diff --git a/tools/firmware/hvmloader/config.h b/tools/firmware/hvmloader/config.h
index b838cf9..4c6d8ad 100644
--- a/tools/firmware/hvmloader/config.h
+++ b/tools/firmware/hvmloader/config.h
@@ -22,7 +22,7 @@ struct bios_config {
/* ROMS */
void (*load_roms)(void);
- void (*bios_load)(const struct bios_config *config);
+ void (*bios_load)(const struct bios_config *config, void *addr, uint32_t size);
void (*bios_info_setup)(void);
void (*bios_info_finish)(void);
diff --git a/tools/firmware/hvmloader/hvmloader.c b/tools/firmware/hvmloader/hvmloader.c
index c45f367..7fdc847 100644
--- a/tools/firmware/hvmloader/hvmloader.c
+++ b/tools/firmware/hvmloader/hvmloader.c
@@ -253,10 +253,51 @@ static void acpi_enable_sci(void)
BUG_ON(!(pm1a_cnt_val & ACPI_PM1C_SCI_EN));
}
+const struct hvm_modlist_entry *get_module_entry(
+ const struct hvm_start_info *info,
+ const char *name)
+{
+ const struct hvm_modlist_entry *modlist =
+ (struct hvm_modlist_entry *)(uint32_t)info->modlist_paddr;
+ unsigned int i;
+
+ if ( !modlist || info->modlist_paddr > UINT_MAX)
+ return NULL;
+
+ for ( i = 0; i < info->nr_modules; i++ )
+ {
+ uint32_t module_name = modlist[i].cmdline_paddr;
+
+ /* Skip if the module or its cmdline is missing. */
+ if ( !module_name || !modlist[i].paddr )
+ continue;
+
+ /* Skip if the cmdline can not be read. */
+ if ( modlist[i].cmdline_paddr > UINT_MAX )
+ continue;
+
+ if ( !strcmp(name, (char*)module_name) )
+ {
+ if ( modlist[i].paddr > UINT_MAX || modlist[i].size > UINT_MAX ||
+ (modlist[i].paddr + modlist[i].size) > UINT_MAX )
+ {
+ printf("Can not load \"%s\" from 0x"PRIllx" (0x"PRIllx")\n",
+ name, PRIllx_arg(modlist[i].paddr),
+ PRIllx_arg(modlist[i].size));
+ BUG();
+ }
+ return &modlist[i];
+ }
+ }
+
+ return NULL;
+}
+
int main(void)
{
const struct bios_config *bios;
int acpi_enabled;
+ const struct hvm_modlist_entry *bios_module;
/* Initialise hypercall stubs with RET, rendering them no-ops. */
memset((void *)HYPERCALL_PHYSICAL_ADDRESS, 0xc3 /* RET */, PAGE_SIZE);
@@ -292,8 +333,17 @@ int main(void)
}
printf("Loading %s ...\n", bios->name);
- if ( bios->bios_load )
- bios->bios_load(bios);
+ bios_module = get_module_entry(hvm_start_info, "firmware");
+ if ( bios_module && bios->bios_load )
+ {
+ uint32_t paddr = bios_module->paddr;
+
+ bios->bios_load(bios, (void*)paddr, bios_module->size);
+ }
+ else if ( bios->bios_load )
+ {
+ bios->bios_load(bios, NULL, 0);
+ }
else
{
BUG_ON(bios->bios_address + bios->image_size >
diff --git a/tools/firmware/hvmloader/ovmf.c b/tools/firmware/hvmloader/ovmf.c
index db9fa7a..858a2d4 100644
--- a/tools/firmware/hvmloader/ovmf.c
+++ b/tools/firmware/hvmloader/ovmf.c
@@ -93,7 +93,8 @@ static void ovmf_finish_bios_info(void)
info->checksum = -checksum;
}
-static void ovmf_load(const struct bios_config *config)
+static void ovmf_load(const struct bios_config *config,
+ void *bios_addr, uint32_t bios_length)
{
xen_pfn_t mfn;
uint64_t addr = OVMF_BEGIN;
diff --git a/tools/firmware/hvmloader/rombios.c b/tools/firmware/hvmloader/rombios.c
index 1f15b94..2ded844 100644
--- a/tools/firmware/hvmloader/rombios.c
+++ b/tools/firmware/hvmloader/rombios.c
@@ -121,7 +121,8 @@ static void rombios_load_roms(void)
option_rom_phys_addr + option_rom_sz - 1);
}
-static void rombios_load(const struct bios_config *config)
+static void rombios_load(const struct bios_config *config,
+ void *unused_addr, uint32_t unused_size)
{
uint32_t bioshigh;
struct rombios_info *info;
diff --git a/tools/firmware/hvmloader/util.h b/tools/firmware/hvmloader/util.h
index 9808016..003dc71 100644
--- a/tools/firmware/hvmloader/util.h
+++ b/tools/firmware/hvmloader/util.h
@@ -34,6 +34,8 @@ enum {
#undef NULL
#define NULL ((void*)0)
+#define UINT_MAX (~0U)
+
void __assert_failed(char *assertion, char *file, int line)
__attribute__((noreturn));
#define ASSERT(p) \
--
Anthony PERARD
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH v5 08/14] hvmloader: Locate the BIOS blob
2016-06-22 17:15 ` [PATCH v5 08/14] hvmloader: Locate the BIOS blob Anthony PERARD
@ 2016-06-24 7:33 ` Jan Beulich
2016-06-24 17:02 ` Anthony PERARD
0 siblings, 1 reply; 43+ messages in thread
From: Jan Beulich @ 2016-06-24 7:33 UTC (permalink / raw)
To: Anthony PERARD; +Cc: Andrew Cooper, Wei Liu, Ian Jackson, xen-devel
>>> On 22.06.16 at 19:15, <anthony.perard@citrix.com> wrote:
> --- a/tools/firmware/hvmloader/hvmloader.c
> +++ b/tools/firmware/hvmloader/hvmloader.c
> @@ -253,10 +253,51 @@ static void acpi_enable_sci(void)
> BUG_ON(!(pm1a_cnt_val & ACPI_PM1C_SCI_EN));
> }
>
> +const struct hvm_modlist_entry *get_module_entry(
> + const struct hvm_start_info *info,
> + const char *name)
> +{
> + const struct hvm_modlist_entry *modlist =
> + (struct hvm_modlist_entry *)(uint32_t)info->modlist_paddr;
> + unsigned int i;
> +
> + if ( !modlist || info->modlist_paddr > UINT_MAX)
> + return NULL;
How about info->modlist_paddr + info->nr_modules * sizeof()?
You check for overflow below, but not here. I think you should
either consistently rely on there being something right below 4Gb
which makes this impossible (and then say so in a comment), or
do full checks everywhere.
> + for ( i = 0; i < info->nr_modules; i++ )
> + {
> + uint32_t module_name = modlist[i].cmdline_paddr;
> +
> + /* Skip if the module or its cmdline is missing. */
> + if ( !module_name || !modlist[i].paddr )
> + continue;
> +
> + /* Skip if the cmdline can not be read. */
> + if ( modlist[i].cmdline_paddr > UINT_MAX )
> + continue;
Similarly here.
> + if ( !strcmp(name, (char*)module_name) )
Stray cast.
> + {
> + if ( modlist[i].paddr > UINT_MAX || modlist[i].size > UINT_MAX ||
> + (modlist[i].paddr + modlist[i].size) > UINT_MAX )
I think the last one could be >=.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v5 08/14] hvmloader: Locate the BIOS blob
2016-06-24 7:33 ` Jan Beulich
@ 2016-06-24 17:02 ` Anthony PERARD
2016-06-27 7:13 ` Jan Beulich
0 siblings, 1 reply; 43+ messages in thread
From: Anthony PERARD @ 2016-06-24 17:02 UTC (permalink / raw)
To: Jan Beulich; +Cc: Andrew Cooper, Wei Liu, Ian Jackson, xen-devel
On Fri, Jun 24, 2016 at 01:33:45AM -0600, Jan Beulich wrote:
> >>> On 22.06.16 at 19:15, <anthony.perard@citrix.com> wrote:
> > --- a/tools/firmware/hvmloader/hvmloader.c
> > +++ b/tools/firmware/hvmloader/hvmloader.c
> > @@ -253,10 +253,51 @@ static void acpi_enable_sci(void)
> > BUG_ON(!(pm1a_cnt_val & ACPI_PM1C_SCI_EN));
> > }
> >
> > +const struct hvm_modlist_entry *get_module_entry(
> > + const struct hvm_start_info *info,
> > + const char *name)
> > +{
> > + const struct hvm_modlist_entry *modlist =
> > + (struct hvm_modlist_entry *)(uint32_t)info->modlist_paddr;
> > + unsigned int i;
> > +
> > + if ( !modlist || info->modlist_paddr > UINT_MAX)
> > + return NULL;
>
> How about info->modlist_paddr + info->nr_modules * sizeof()?
> You check for overflow below, but not here. I think you should
> either consistently rely on there being something right below 4Gb
> which makes this impossible (and then say so in a comment), or
> do full checks everywhere.
I'll do the full checks.
> > + for ( i = 0; i < info->nr_modules; i++ )
> > + {
> > + uint32_t module_name = modlist[i].cmdline_paddr;
> > +
> > + /* Skip if the module or its cmdline is missing. */
> > + if ( !module_name || !modlist[i].paddr )
> > + continue;
> > +
> > + /* Skip if the cmdline can not be read. */
> > + if ( modlist[i].cmdline_paddr > UINT_MAX )
> > + continue;
>
> Similarly here.
Here, I don't know the size of the cmdline and I don't think calling an
extra strlen() would be usefull. I think that the strcmp() below is going to
be enough for the top bondary check.
Or I could use the size of name.
> > + if ( !strcmp(name, (char*)module_name) )
>
> Stray cast.
Yes. I'll change the type of module_name and remove the cast here.
> > + {
> > + if ( modlist[i].paddr > UINT_MAX || modlist[i].size > UINT_MAX ||
> > + (modlist[i].paddr + modlist[i].size) > UINT_MAX )
>
> I think the last one could be >=.
I think it's valid if addr+size == UINT_MAX. That would means the last
byte of the module would be at 0xFFFFFFFE.
--
Anthony PERARD
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v5 08/14] hvmloader: Locate the BIOS blob
2016-06-24 17:02 ` Anthony PERARD
@ 2016-06-27 7:13 ` Jan Beulich
2016-06-30 15:04 ` Anthony PERARD
0 siblings, 1 reply; 43+ messages in thread
From: Jan Beulich @ 2016-06-27 7:13 UTC (permalink / raw)
To: Anthony PERARD; +Cc: Andrew Cooper, Wei Liu, Ian Jackson, xen-devel
>>> On 24.06.16 at 19:02, <anthony.perard@citrix.com> wrote:
> On Fri, Jun 24, 2016 at 01:33:45AM -0600, Jan Beulich wrote:
>> >>> On 22.06.16 at 19:15, <anthony.perard@citrix.com> wrote:
>> > --- a/tools/firmware/hvmloader/hvmloader.c
>> > +++ b/tools/firmware/hvmloader/hvmloader.c
>> > @@ -253,10 +253,51 @@ static void acpi_enable_sci(void)
>> > BUG_ON(!(pm1a_cnt_val & ACPI_PM1C_SCI_EN));
>> > }
>> >
>> > +const struct hvm_modlist_entry *get_module_entry(
>> > + const struct hvm_start_info *info,
>> > + const char *name)
>> > +{
>> > + const struct hvm_modlist_entry *modlist =
>> > + (struct hvm_modlist_entry *)(uint32_t)info->modlist_paddr;
>> > + unsigned int i;
>> > +
>> > + if ( !modlist || info->modlist_paddr > UINT_MAX)
>> > + return NULL;
>>
>> How about info->modlist_paddr + info->nr_modules * sizeof()?
>> You check for overflow below, but not here. I think you should
>> either consistently rely on there being something right below 4Gb
>> which makes this impossible (and then say so in a comment), or
>> do full checks everywhere.
>
> I'll do the full checks.
>
>> > + for ( i = 0; i < info->nr_modules; i++ )
>> > + {
>> > + uint32_t module_name = modlist[i].cmdline_paddr;
>> > +
>> > + /* Skip if the module or its cmdline is missing. */
>> > + if ( !module_name || !modlist[i].paddr )
>> > + continue;
>> > +
>> > + /* Skip if the cmdline can not be read. */
>> > + if ( modlist[i].cmdline_paddr > UINT_MAX )
>> > + continue;
>>
>> Similarly here.
>
> Here, I don't know the size of the cmdline and I don't think calling an
> extra strlen() would be usefull. I think that the strcmp() below is going to
> be enough for the top bondary check.
No - once you reach the 4Gb boundary, the compare would continue
at address zero. That's not what you want.
> Or I could use the size of name.
Size of name?
>> > + {
>> > + if ( modlist[i].paddr > UINT_MAX || modlist[i].size > UINT_MAX ||
>> > + (modlist[i].paddr + modlist[i].size) > UINT_MAX )
>>
>> I think the last one could be >=.
>
> I think it's valid if addr+size == UINT_MAX. That would means the last
> byte of the module would be at 0xFFFFFFFE.
It's even valid when addr+size == UINT_MAX+1 (without value
wrapping of course), as the last valid byte (i.e. the last one
hvmloader can address easily) is at 0xffffffff.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v5 08/14] hvmloader: Locate the BIOS blob
2016-06-27 7:13 ` Jan Beulich
@ 2016-06-30 15:04 ` Anthony PERARD
2016-07-01 6:40 ` Jan Beulich
0 siblings, 1 reply; 43+ messages in thread
From: Anthony PERARD @ 2016-06-30 15:04 UTC (permalink / raw)
To: Jan Beulich; +Cc: Andrew Cooper, Wei Liu, Ian Jackson, xen-devel
On Mon, Jun 27, 2016 at 01:13:43AM -0600, Jan Beulich wrote:
> >>> On 24.06.16 at 19:02, <anthony.perard@citrix.com> wrote:
> > On Fri, Jun 24, 2016 at 01:33:45AM -0600, Jan Beulich wrote:
> >> >>> On 22.06.16 at 19:15, <anthony.perard@citrix.com> wrote:
> >> > --- a/tools/firmware/hvmloader/hvmloader.c
> >> > +++ b/tools/firmware/hvmloader/hvmloader.c
> >> > @@ -253,10 +253,51 @@ static void acpi_enable_sci(void)
> >> > BUG_ON(!(pm1a_cnt_val & ACPI_PM1C_SCI_EN));
> >> > }
> >> >
> >> > +const struct hvm_modlist_entry *get_module_entry(
> >> > + const struct hvm_start_info *info,
> >> > + const char *name)
> >> > +{
> >> > + const struct hvm_modlist_entry *modlist =
> >> > + (struct hvm_modlist_entry *)(uint32_t)info->modlist_paddr;
> >> > + unsigned int i;
> >> > +
> >> > + if ( !modlist || info->modlist_paddr > UINT_MAX)
> >> > + return NULL;
> >>
> >> How about info->modlist_paddr + info->nr_modules * sizeof()?
> >> You check for overflow below, but not here. I think you should
> >> either consistently rely on there being something right below 4Gb
> >> which makes this impossible (and then say so in a comment), or
> >> do full checks everywhere.
> >
> > I'll do the full checks.
> >
> >> > + for ( i = 0; i < info->nr_modules; i++ )
> >> > + {
> >> > + uint32_t module_name = modlist[i].cmdline_paddr;
> >> > +
> >> > + /* Skip if the module or its cmdline is missing. */
> >> > + if ( !module_name || !modlist[i].paddr )
> >> > + continue;
> >> > +
> >> > + /* Skip if the cmdline can not be read. */
> >> > + if ( modlist[i].cmdline_paddr > UINT_MAX )
> >> > + continue;
> >>
> >> Similarly here.
> >
> > Here, I don't know the size of the cmdline and I don't think calling an
> > extra strlen() would be usefull. I think that the strcmp() below is going to
> > be enough for the top bondary check.
>
> No - once you reach the 4Gb boundary, the compare would continue
> at address zero. That's not what you want.
> > Or I could use the size of name.
>
> Size of name?
The function get_module_entry() takes an argument called "name", I think
I was proposing to use that, strlen(name).
So, I'm going to add this condition:
(cmdline_paddr + strlen(name) > UINTPTR_MAX)
name is the string we are going to compare cmdline to. I think that
will be enough to do a full check of the module cmdline.
> >> > + {
> >> > + if ( modlist[i].paddr > UINT_MAX || modlist[i].size > UINT_MAX ||
> >> > + (modlist[i].paddr + modlist[i].size) > UINT_MAX )
> >>
> >> I think the last one could be >=.
> >
> > I think it's valid if addr+size == UINT_MAX. That would means the last
> > byte of the module would be at 0xFFFFFFFE.
>
> It's even valid when addr+size == UINT_MAX+1 (without value
> wrapping of course), as the last valid byte (i.e. the last one
> hvmloader can address easily) is at 0xffffffff.
I'll do (addr+size-1 > UINTPTR_MAX) which should return true when the
module cross the 4GB boundary.
--
Anthony PERARD
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v5 08/14] hvmloader: Locate the BIOS blob
2016-06-30 15:04 ` Anthony PERARD
@ 2016-07-01 6:40 ` Jan Beulich
0 siblings, 0 replies; 43+ messages in thread
From: Jan Beulich @ 2016-07-01 6:40 UTC (permalink / raw)
To: Anthony PERARD; +Cc: Andrew Cooper, Wei Liu, Ian Jackson, xen-devel
>>> On 30.06.16 at 17:04, <anthony.perard@citrix.com> wrote:
> On Mon, Jun 27, 2016 at 01:13:43AM -0600, Jan Beulich wrote:
>> >>> On 24.06.16 at 19:02, <anthony.perard@citrix.com> wrote:
>> > On Fri, Jun 24, 2016 at 01:33:45AM -0600, Jan Beulich wrote:
>> >> >>> On 22.06.16 at 19:15, <anthony.perard@citrix.com> wrote:
>> >> > + for ( i = 0; i < info->nr_modules; i++ )
>> >> > + {
>> >> > + uint32_t module_name = modlist[i].cmdline_paddr;
>> >> > +
>> >> > + /* Skip if the module or its cmdline is missing. */
>> >> > + if ( !module_name || !modlist[i].paddr )
>> >> > + continue;
>> >> > +
>> >> > + /* Skip if the cmdline can not be read. */
>> >> > + if ( modlist[i].cmdline_paddr > UINT_MAX )
>> >> > + continue;
>> >>
>> >> Similarly here.
>> >
>> > Here, I don't know the size of the cmdline and I don't think calling an
>> > extra strlen() would be usefull. I think that the strcmp() below is going to
>> > be enough for the top bondary check.
>>
>> No - once you reach the 4Gb boundary, the compare would continue
>> at address zero. That's not what you want.
>> > Or I could use the size of name.
>>
>> Size of name?
>
> The function get_module_entry() takes an argument called "name", I think
> I was proposing to use that, strlen(name).
>
> So, I'm going to add this condition:
> (cmdline_paddr + strlen(name) > UINTPTR_MAX)
> name is the string we are going to compare cmdline to. I think that
> will be enough to do a full check of the module cmdline.
Makes sense.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v5 09/14] hvmloader: Check modules whereabouts in perform_tests
2016-06-22 17:15 [PATCH v5 00/14] Load BIOS via toolstack instead of been embedded in hvmloader Anthony PERARD
` (7 preceding siblings ...)
2016-06-22 17:15 ` [PATCH v5 08/14] hvmloader: Locate the BIOS blob Anthony PERARD
@ 2016-06-22 17:15 ` Anthony PERARD
2016-06-24 7:44 ` Jan Beulich
2016-06-22 17:15 ` [PATCH v5 10/14] hvmloader: Load SeaBIOS from hvm_start_info modules Anthony PERARD
` (4 subsequent siblings)
13 siblings, 1 reply; 43+ messages in thread
From: Anthony PERARD @ 2016-06-22 17:15 UTC (permalink / raw)
To: xen-devel
Cc: Anthony PERARD, Andrew Cooper, Ian Jackson, Wei Liu, Jan Beulich
As perform_tests() is going to clear memory past 4MB, we check that the
memory can be use or we skip the tests.
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
Changes in V5:
- also account for the pages table
- fix coding style
- also check modules cmdline and main cmdline
and modlist_paddr
- make use of check_overlap.
Changes in v4:
- move the check into the perform_test() function.
- skip tests instead of using BUG.
New in V3
---
tools/firmware/hvmloader/tests.c | 53 ++++++++++++++++++++++++++++++++++++++++
1 file changed, 53 insertions(+)
diff --git a/tools/firmware/hvmloader/tests.c b/tools/firmware/hvmloader/tests.c
index fea3ad3..bf3aa01 100644
--- a/tools/firmware/hvmloader/tests.c
+++ b/tools/firmware/hvmloader/tests.c
@@ -20,6 +20,7 @@
*/
#include "util.h"
+#include "config.h"
#define TEST_FAIL 0
#define TEST_PASS 1
@@ -189,6 +190,15 @@ static int shadow_gs_test(void)
return (ebx == 2) ? TEST_PASS : TEST_FAIL;
}
+static bool check_test_overlap(uint64_t start, uint64_t size)
+{
+ if (start)
+ return check_overlap(start, size,
+ 4ul << 20,
+ (PT_START + 4 * PAGE_SIZE) - (4ul << 20));
+ return false;
+}
+
void perform_tests(void)
{
int i, passed, skipped;
@@ -210,6 +220,49 @@ void perform_tests(void)
return;
}
+ /* Check that tests does not use memory where modules are stored */
+ if ( check_test_overlap((uint32_t)hvm_start_info, sizeof(hvm_start_info)) )
+ {
+ printf("Skipping tests due to memory used by hvm_start_info\n");
+ return;
+ }
+ if ( check_test_overlap(hvm_start_info->modlist_paddr,
+ hvm_start_info->nr_modules *
+ sizeof(struct hvm_modlist_entry)) )
+ {
+ printf("Skipping tests due to memory used by"
+ " hvm_start_info->modlist\n");
+ return;
+ }
+ for ( i = 0; i < hvm_start_info->nr_modules; i++ )
+ {
+ const struct hvm_modlist_entry *modlist =
+ (struct hvm_modlist_entry *)(uint32_t)hvm_start_info->modlist_paddr;
+ uint64_t cmdline_paddr = modlist[i].cmdline_paddr;
+
+ if ( check_test_overlap(modlist[i].paddr, modlist[i].size) )
+ {
+ printf("Skipping tests due to memory used by module[%d]\n", i);
+ return;
+ }
+ if ( cmdline_paddr && cmdline_paddr < UINT_MAX &&
+ check_test_overlap(cmdline_paddr,
+ strlen((char*)(uint32_t)cmdline_paddr)) )
+ {
+ printf("Skipping tests due to memory used by"
+ " module[%d]'s cmdline\n", i);
+ return;
+ }
+ }
+ if ( hvm_start_info->cmdline_paddr &&
+ hvm_start_info->cmdline_paddr < UINT_MAX &&
+ check_test_overlap(hvm_start_info->cmdline_paddr,
+ strlen((char*)(uint32_t)hvm_start_info->cmdline_paddr)) )
+ {
+ printf("Skipping tests due to memory used by the hvm_start_info->cmdline\n");
+ return;
+ }
+
passed = skipped = 0;
for ( i = 0; tests[i].test; i++ )
{
--
Anthony PERARD
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH v5 09/14] hvmloader: Check modules whereabouts in perform_tests
2016-06-22 17:15 ` [PATCH v5 09/14] hvmloader: Check modules whereabouts in perform_tests Anthony PERARD
@ 2016-06-24 7:44 ` Jan Beulich
2016-06-24 17:14 ` Anthony PERARD
0 siblings, 1 reply; 43+ messages in thread
From: Jan Beulich @ 2016-06-24 7:44 UTC (permalink / raw)
To: Anthony PERARD; +Cc: Andrew Cooper, Wei Liu, Ian Jackson, xen-devel
>>> On 22.06.16 at 19:15, <anthony.perard@citrix.com> wrote:
> --- a/tools/firmware/hvmloader/tests.c
> +++ b/tools/firmware/hvmloader/tests.c
> @@ -20,6 +20,7 @@
> */
>
> #include "util.h"
> +#include "config.h"
>
> #define TEST_FAIL 0
> #define TEST_PASS 1
> @@ -189,6 +190,15 @@ static int shadow_gs_test(void)
> return (ebx == 2) ? TEST_PASS : TEST_FAIL;
> }
>
> +static bool check_test_overlap(uint64_t start, uint64_t size)
> +{
> + if (start)
Missing blanks.
> + return check_overlap(start, size,
> + 4ul << 20,
> + (PT_START + 4 * PAGE_SIZE) - (4ul << 20));
Can the 4ul << 20 and the upper bound be made into descriptive
#define-s please, also to be used wherever (if anywhere) those
boundary are currently of relevance (but at least side by side with
the respective comment at the top of the file)?
> @@ -210,6 +220,49 @@ void perform_tests(void)
> return;
> }
>
> + /* Check that tests does not use memory where modules are stored */
> + if ( check_test_overlap((uint32_t)hvm_start_info, sizeof(hvm_start_info)) )
sizeof(*hvm_start_info) perhaps?
> + {
> + printf("Skipping tests due to memory used by hvm_start_info\n");
> + return;
> + }
> + if ( check_test_overlap(hvm_start_info->modlist_paddr,
> + hvm_start_info->nr_modules *
> + sizeof(struct hvm_modlist_entry)) )
> + {
> + printf("Skipping tests due to memory used by"
> + " hvm_start_info->modlist\n");
> + return;
> + }
> + for ( i = 0; i < hvm_start_info->nr_modules; i++ )
> + {
> + const struct hvm_modlist_entry *modlist =
> + (struct hvm_modlist_entry *)(uint32_t)hvm_start_info->modlist_paddr;
To cut done on the length of such lines, perhaps better to use void *
in casts like this?
> + uint64_t cmdline_paddr = modlist[i].cmdline_paddr;
> +
> + if ( check_test_overlap(modlist[i].paddr, modlist[i].size) )
> + {
> + printf("Skipping tests due to memory used by module[%d]\n", i);
> + return;
> + }
> + if ( cmdline_paddr && cmdline_paddr < UINT_MAX &&
> + check_test_overlap(cmdline_paddr,
> + strlen((char*)(uint32_t)cmdline_paddr)) )
As said on the previous patch - cmdline_paddr being below 4Gb
doesn't necessarily mean the string not crossing that boundary.
Depending on the resolution for that other patch this may need
adjustment.
Also, thinking about it again, the use of UINT_MAX for bounding
pointers is unfortunate: I think this would better be UINTPTR_MAX.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v5 09/14] hvmloader: Check modules whereabouts in perform_tests
2016-06-24 7:44 ` Jan Beulich
@ 2016-06-24 17:14 ` Anthony PERARD
2016-06-27 7:20 ` Jan Beulich
0 siblings, 1 reply; 43+ messages in thread
From: Anthony PERARD @ 2016-06-24 17:14 UTC (permalink / raw)
To: Jan Beulich; +Cc: Andrew Cooper, Wei Liu, Ian Jackson, xen-devel
On Fri, Jun 24, 2016 at 01:44:30AM -0600, Jan Beulich wrote:
> >>> On 22.06.16 at 19:15, <anthony.perard@citrix.com> wrote:
> > + uint64_t cmdline_paddr = modlist[i].cmdline_paddr;
> > +
> > + if ( check_test_overlap(modlist[i].paddr, modlist[i].size) )
> > + {
> > + printf("Skipping tests due to memory used by module[%d]\n", i);
> > + return;
> > + }
> > + if ( cmdline_paddr && cmdline_paddr < UINT_MAX &&
> > + check_test_overlap(cmdline_paddr,
> > + strlen((char*)(uint32_t)cmdline_paddr)) )
>
> As said on the previous patch - cmdline_paddr being below 4Gb
> doesn't necessarily mean the string not crossing that boundary.
> Depending on the resolution for that other patch this may need
> adjustment.
I'm not sure how I could handle that, here.
> Also, thinking about it again, the use of UINT_MAX for bounding
> pointers is unfortunate: I think this would better be UINTPTR_MAX.
Well, I do cast every addr to uint32_t, and I had to define UINT_MAX in
the previous patch (hvmloader: Locate the BIOS blob)(I should probably
add a comment about it in the patch description).
I could try to add UINTPTR_MAX instead.
--
Anthony PERARD
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v5 09/14] hvmloader: Check modules whereabouts in perform_tests
2016-06-24 17:14 ` Anthony PERARD
@ 2016-06-27 7:20 ` Jan Beulich
0 siblings, 0 replies; 43+ messages in thread
From: Jan Beulich @ 2016-06-27 7:20 UTC (permalink / raw)
To: Anthony PERARD; +Cc: Andrew Cooper, Wei Liu, Ian Jackson, xen-devel
>>> On 24.06.16 at 19:14, <anthony.perard@citrix.com> wrote:
> On Fri, Jun 24, 2016 at 01:44:30AM -0600, Jan Beulich wrote:
>> >>> On 22.06.16 at 19:15, <anthony.perard@citrix.com> wrote:
>> > + uint64_t cmdline_paddr = modlist[i].cmdline_paddr;
>> > +
>> > + if ( check_test_overlap(modlist[i].paddr, modlist[i].size) )
>> > + {
>> > + printf("Skipping tests due to memory used by module[%d]\n",
> i);
>> > + return;
>> > + }
>> > + if ( cmdline_paddr && cmdline_paddr < UINT_MAX &&
>> > + check_test_overlap(cmdline_paddr,
>> > + strlen((char*)(uint32_t)cmdline_paddr)) )
>>
>> As said on the previous patch - cmdline_paddr being below 4Gb
>> doesn't necessarily mean the string not crossing that boundary.
>> Depending on the resolution for that other patch this may need
>> adjustment.
>
> I'm not sure how I could handle that, here.
Either determine the length of the string first, or have a special
variant of strcmp() which returns false when the address would
wrap without having reached the end of the string.
But again - maybe all this goes too far, and we should instead
opt for the simpler route (and easier to grok code) assuming
that no item would ever cross the 4Gb boundary. I'm sure
making this a requirement even for PVH (which might not actually
have anything right below 4Gb, especially when there is no ACPI
enabled, and hence no tables to be put anywhere) wouldn't
pose a severe limitation to the party setting up the domain: After
all such code has to be prepared for stuff to need placement
below 4Gb (apart from ACPI tables this could also be PCI device
MMIO BAR assignment ranges) anyway.
>> Also, thinking about it again, the use of UINT_MAX for bounding
>> pointers is unfortunate: I think this would better be UINTPTR_MAX.
>
> Well, I do cast every addr to uint32_t, and I had to define UINT_MAX in
> the previous patch (hvmloader: Locate the BIOS blob)(I should probably
> add a comment about it in the patch description).
>
> I could try to add UINTPTR_MAX instead.
That would seem more natural, thanks. And in fact I question the
use of uint32_t (instead of unsigned long or even better uintptr_t)
for such casts and variable types.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v5 10/14] hvmloader: Load SeaBIOS from hvm_start_info modules
2016-06-22 17:15 [PATCH v5 00/14] Load BIOS via toolstack instead of been embedded in hvmloader Anthony PERARD
` (8 preceding siblings ...)
2016-06-22 17:15 ` [PATCH v5 09/14] hvmloader: Check modules whereabouts in perform_tests Anthony PERARD
@ 2016-06-22 17:15 ` Anthony PERARD
2016-06-24 7:46 ` Jan Beulich
2016-06-22 17:15 ` [PATCH v5 11/14] hvmloader: Load OVMF from modules Anthony PERARD
` (3 subsequent siblings)
13 siblings, 1 reply; 43+ messages in thread
From: Anthony PERARD @ 2016-06-22 17:15 UTC (permalink / raw)
To: xen-devel
Cc: Anthony PERARD, Andrew Cooper, Ian Jackson, Wei Liu, Jan Beulich
... and do not include the SeaBIOS ROM into hvmloader anymore.
This also fix the dependency on roms.inc, hvmloader.o does not include it.
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
Change in V5:
- update BUG_ON in seabios_setup_e820().
Change in V4:
- check that seabios_config.bios_address have a probably good value
instead of checking only if it's set.
Change in V3:
- change makefile to not include seabios roms into roms.inc.
- update the struct bios_config with the location of the bios blob.
---
tools/firmware/hvmloader/Makefile | 15 +--------------
tools/firmware/hvmloader/seabios.c | 25 +++++++++++++++----------
2 files changed, 16 insertions(+), 24 deletions(-)
diff --git a/tools/firmware/hvmloader/Makefile b/tools/firmware/hvmloader/Makefile
index f2f4791..ed0bfad 100644
--- a/tools/firmware/hvmloader/Makefile
+++ b/tools/firmware/hvmloader/Makefile
@@ -45,7 +45,6 @@ CIRRUSVGA_DEBUG ?= n
OVMF_DIR := ../ovmf-dir
ROMBIOS_DIR := ../rombios
-SEABIOS_DIR := ../seabios-dir
ifeq ($(CONFIG_ROMBIOS),y)
STDVGA_ROM := ../vgabios/VGABIOS-lgpl-latest.bin
@@ -80,19 +79,13 @@ endif
ifeq ($(CONFIG_SEABIOS),y)
OBJS += seabios.o
CFLAGS += -DENABLE_SEABIOS
-ifeq ($(SEABIOS_PATH),)
- SEABIOS_ROM := $(SEABIOS_DIR)/out/bios.bin
-else
- SEABIOS_ROM := $(SEABIOS_PATH)
-endif
-ROMS += $(SEABIOS_ROM)
endif
.PHONY: all
all: subdirs-all
$(MAKE) hvmloader
-ovmf.o rombios.o seabios.o hvmloader.o: roms.inc
+ovmf.o rombios.o: roms.inc
smbios.o: CFLAGS += -D__SMBIOS_DATE__="\"$(SMBIOS_REL_DATE)\""
hvmloader: $(OBJS) acpi/acpi.a
@@ -109,12 +102,6 @@ ifneq ($(ROMBIOS_ROM),)
echo "#endif" >> $@.new
endif
-ifneq ($(SEABIOS_ROM),)
- echo "#ifdef ROM_INCLUDE_SEABIOS" >> $@.new
- sh ./mkhex seabios $(SEABIOS_ROM) >> $@.new
- echo "#endif" >> $@.new
-endif
-
ifneq ($(OVMF_ROM),)
echo "#ifdef ROM_INCLUDE_OVMF" >> $@.new
sh ./mkhex ovmf $(OVMF_ROM) >> $@.new
diff --git a/tools/firmware/hvmloader/seabios.c b/tools/firmware/hvmloader/seabios.c
index c6b3d9f..c5d2b34 100644
--- a/tools/firmware/hvmloader/seabios.c
+++ b/tools/firmware/hvmloader/seabios.c
@@ -27,9 +27,6 @@
#include "smbios_types.h"
#include "acpi/acpi2_0.h"
-#define ROM_INCLUDE_SEABIOS
-#include "roms.inc"
-
extern unsigned char dsdt_anycpu_qemu_xen[];
extern int dsdt_anycpu_qemu_xen_len;
@@ -127,22 +124,30 @@ static void seabios_setup_e820(void)
struct e820entry *e820 = scratch_alloc(sizeof(struct e820entry)*16, 0);
info->e820 = (uint32_t)e820;
+ /* Upper boundary already checked by seabios_load(). */
+ BUG_ON(seabios_config.bios_address < 0x000c0000);
/* SeaBIOS reserves memory in e820 as necessary so no low reservation. */
- info->e820_nr = build_e820_table(e820, 0, 0x100000-sizeof(seabios));
+ info->e820_nr = build_e820_table(e820, 0, seabios_config.bios_address);
dump_e820_table(e820, info->e820_nr);
}
-struct bios_config seabios_config = {
- .name = "SeaBIOS",
+static void seabios_load(const struct bios_config *bios,
+ void *bios_addr, uint32_t bios_length)
+{
+ unsigned int bios_dest = 0x100000 - bios_length;
- .image = seabios,
- .image_size = sizeof(seabios),
+ BUG_ON(bios_dest + bios_length > HVMLOADER_PHYSICAL_ADDRESS);
+ memcpy((void *)bios_dest, bios_addr, bios_length);
+ seabios_config.bios_address = bios_dest;
+ seabios_config.image_size = bios_length;
+}
- .bios_address = 0x100000 - sizeof(seabios),
+struct bios_config seabios_config = {
+ .name = "SeaBIOS",
.load_roms = NULL,
- .bios_load = NULL,
+ .bios_load = seabios_load,
.bios_info_setup = seabios_setup_bios_info,
.bios_info_finish = seabios_finish_bios_info,
--
Anthony PERARD
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 43+ messages in thread* [PATCH v5 11/14] hvmloader: Load OVMF from modules
2016-06-22 17:15 [PATCH v5 00/14] Load BIOS via toolstack instead of been embedded in hvmloader Anthony PERARD
` (9 preceding siblings ...)
2016-06-22 17:15 ` [PATCH v5 10/14] hvmloader: Load SeaBIOS from hvm_start_info modules Anthony PERARD
@ 2016-06-22 17:15 ` Anthony PERARD
2016-06-22 17:15 ` [PATCH v5 12/14] hvmloader: bios->bios_load() now needs to be defined Anthony PERARD
` (2 subsequent siblings)
13 siblings, 0 replies; 43+ messages in thread
From: Anthony PERARD @ 2016-06-22 17:15 UTC (permalink / raw)
To: xen-devel
Cc: Anthony PERARD, Andrew Cooper, Ian Jackson, Wei Liu, Jan Beulich
... and do not include the OVMF ROM into hvmloader anymore.
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
---
Change in V5:
- define OVMF_END macro
- fix some cast coding style
Change in V4:
- check if source and dest of ovmf binary does not overlaps
Change in V3:
- change makefile to not include ovmf rom into roms.inc.
- update the struct bios_config with bios destination
---
tools/firmware/hvmloader/Makefile | 15 +--------------
tools/firmware/hvmloader/ovmf.c | 31 ++++++++++++++-----------------
2 files changed, 15 insertions(+), 31 deletions(-)
diff --git a/tools/firmware/hvmloader/Makefile b/tools/firmware/hvmloader/Makefile
index ed0bfad..dee1c6b 100644
--- a/tools/firmware/hvmloader/Makefile
+++ b/tools/firmware/hvmloader/Makefile
@@ -43,7 +43,6 @@ endif
CIRRUSVGA_DEBUG ?= n
-OVMF_DIR := ../ovmf-dir
ROMBIOS_DIR := ../rombios
ifeq ($(CONFIG_ROMBIOS),y)
@@ -61,12 +60,6 @@ ROMS :=
ifeq ($(CONFIG_OVMF),y)
OBJS += ovmf.o
CFLAGS += -DENABLE_OVMF
-ifeq ($(OVMF_PATH),)
- OVMF_ROM := $(OVMF_DIR)/ovmf.bin
-else
- OVMF_ROM := $(OVMF_PATH)
-endif
-ROMS += $(OVMF_ROM)
endif
ifeq ($(CONFIG_ROMBIOS),y)
@@ -85,7 +78,7 @@ endif
all: subdirs-all
$(MAKE) hvmloader
-ovmf.o rombios.o: roms.inc
+rombios.o: roms.inc
smbios.o: CFLAGS += -D__SMBIOS_DATE__="\"$(SMBIOS_REL_DATE)\""
hvmloader: $(OBJS) acpi/acpi.a
@@ -102,12 +95,6 @@ ifneq ($(ROMBIOS_ROM),)
echo "#endif" >> $@.new
endif
-ifneq ($(OVMF_ROM),)
- echo "#ifdef ROM_INCLUDE_OVMF" >> $@.new
- sh ./mkhex ovmf $(OVMF_ROM) >> $@.new
- echo "#endif" >> $@.new
-endif
-
ifneq ($(STDVGA_ROM),)
echo "#ifdef ROM_INCLUDE_VGABIOS" >> $@.new
sh ./mkhex vgabios_stdvga $(STDVGA_ROM) >> $@.new
diff --git a/tools/firmware/hvmloader/ovmf.c b/tools/firmware/hvmloader/ovmf.c
index 858a2d4..59d0e45 100644
--- a/tools/firmware/hvmloader/ovmf.c
+++ b/tools/firmware/hvmloader/ovmf.c
@@ -34,17 +34,11 @@
#include <xen/hvm/ioreq.h>
#include <xen/memory.h>
-#define ROM_INCLUDE_OVMF
-#include "roms.inc"
-
-#define OVMF_SIZE (sizeof(ovmf))
#define OVMF_MAXOFFSET 0x000FFFFFULL
-#define OVMF_BEGIN (0x100000000ULL - ((OVMF_SIZE + OVMF_MAXOFFSET) & ~OVMF_MAXOFFSET))
-#define OVMF_END (OVMF_BEGIN + OVMF_SIZE)
+#define OVMF_END 0x100000000ULL
#define LOWCHUNK_BEGIN 0x000F0000
#define LOWCHUNK_SIZE 0x00010000
#define LOWCHUNK_MAXOFFSET 0x0000FFFF
-#define LOWCHUNK_END (OVMF_BEGIN + OVMF_SIZE)
#define OVMF_INFO_PHYSICAL_ADDRESS 0x00001000
extern unsigned char dsdt_anycpu_qemu_xen[];
@@ -97,24 +91,31 @@ static void ovmf_load(const struct bios_config *config,
void *bios_addr, uint32_t bios_length)
{
xen_pfn_t mfn;
- uint64_t addr = OVMF_BEGIN;
+ uint64_t addr = OVMF_END
+ - ((bios_length + OVMF_MAXOFFSET) & ~OVMF_MAXOFFSET);
+ uint64_t ovmf_end = addr + bios_length;
+
+ ovmf_config.bios_address = addr;
+ ovmf_config.image_size = bios_length;
/* Copy low-reset vector portion. */
- memcpy((void *) LOWCHUNK_BEGIN, (uint8_t *) config->image
- + OVMF_SIZE
- - LOWCHUNK_SIZE,
+ memcpy((void *)LOWCHUNK_BEGIN,
+ (uint8_t *)bios_addr + bios_length - LOWCHUNK_SIZE,
LOWCHUNK_SIZE);
/* Ensure we have backing page prior to moving FD. */
- while ( (addr >> PAGE_SHIFT) != (OVMF_END >> PAGE_SHIFT) )
+ while ( (addr >> PAGE_SHIFT) != (ovmf_end >> PAGE_SHIFT) )
{
mfn = (uint32_t) (addr >> PAGE_SHIFT);
addr += PAGE_SIZE;
mem_hole_populate_ram(mfn, 1);
}
+ /* Check that source and destination does not overlaps. */
+ BUG_ON(addr + bios_length > (unsigned)bios_addr &&
+ addr < (unsigned)bios_addr + bios_length);
/* Copy FD. */
- memcpy((void *) OVMF_BEGIN, config->image, OVMF_SIZE);
+ memcpy((void *)ovmf_config.bios_address, bios_addr, bios_length);
}
static void ovmf_acpi_build_tables(void)
@@ -151,10 +152,6 @@ static void ovmf_setup_e820(void)
struct bios_config ovmf_config = {
.name = "OVMF",
- .image = ovmf,
- .image_size = sizeof(ovmf),
-
- .bios_address = OVMF_BEGIN,
.bios_load = ovmf_load,
.load_roms = 0,
--
Anthony PERARD
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 43+ messages in thread* [PATCH v5 12/14] hvmloader: bios->bios_load() now needs to be defined
2016-06-22 17:15 [PATCH v5 00/14] Load BIOS via toolstack instead of been embedded in hvmloader Anthony PERARD
` (10 preceding siblings ...)
2016-06-22 17:15 ` [PATCH v5 11/14] hvmloader: Load OVMF from modules Anthony PERARD
@ 2016-06-22 17:15 ` Anthony PERARD
2016-06-22 17:15 ` [PATCH v5 13/14] hvmloader: Always build-in SeaBIOS and OVMF loader Anthony PERARD
2016-06-22 17:15 ` [PATCH v5 14/14] configure: do not depend on SEABIOS_PATH or OVMF_PATH Anthony PERARD
13 siblings, 0 replies; 43+ messages in thread
From: Anthony PERARD @ 2016-06-22 17:15 UTC (permalink / raw)
To: xen-devel
Cc: Anthony PERARD, Andrew Cooper, Ian Jackson, Wei Liu, Jan Beulich
All BIOSes but ROMBIOS needs to be loaded via modules.
ROMBIOS is handled as a special case.
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
---
Change in V5:
- rename patch, was:
"hvmloader: Specific bios_load function required"
No change in V4.
Change in V3:
- reprint Main BIOS in bios map with now available information from bios
modules.
- handle rombios, and keep its built-in ROMs.
---
tools/firmware/hvmloader/hvmloader.c | 16 ++++++++++------
1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/tools/firmware/hvmloader/hvmloader.c b/tools/firmware/hvmloader/hvmloader.c
index 7fdc847..eacfe72 100644
--- a/tools/firmware/hvmloader/hvmloader.c
+++ b/tools/firmware/hvmloader/hvmloader.c
@@ -334,22 +334,26 @@ int main(void)
printf("Loading %s ...\n", bios->name);
bios_module = get_module_entry(hvm_start_info, "firmware");
- if ( bios_module && bios->bios_load )
+ if ( bios_module )
{
uint32_t paddr = bios_module->paddr;
bios->bios_load(bios, (void*)paddr, bios_module->size);
}
- else if ( bios->bios_load )
+#ifdef ENABLE_ROMBIOS
+ else if ( bios == &rombios_config )
{
bios->bios_load(bios, NULL, 0);
}
+#endif
else
{
- BUG_ON(bios->bios_address + bios->image_size >
- HVMLOADER_PHYSICAL_ADDRESS);
- memcpy((void *)bios->bios_address, bios->image,
- bios->image_size);
+ /*
+ * If there is no BIOS module supplied and if there is no embeded BIOS
+ * image, then we failed. Only rombios might have an embedded bios blob.
+ */
+ printf("no BIOS ROM image found\n");
+ BUG();
}
if ( (hvm_info->nr_vcpus > 1) || hvm_info->apic_mode )
--
Anthony PERARD
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 43+ messages in thread* [PATCH v5 13/14] hvmloader: Always build-in SeaBIOS and OVMF loader
2016-06-22 17:15 [PATCH v5 00/14] Load BIOS via toolstack instead of been embedded in hvmloader Anthony PERARD
` (11 preceding siblings ...)
2016-06-22 17:15 ` [PATCH v5 12/14] hvmloader: bios->bios_load() now needs to be defined Anthony PERARD
@ 2016-06-22 17:15 ` Anthony PERARD
2016-06-22 17:15 ` [PATCH v5 14/14] configure: do not depend on SEABIOS_PATH or OVMF_PATH Anthony PERARD
13 siblings, 0 replies; 43+ messages in thread
From: Anthony PERARD @ 2016-06-22 17:15 UTC (permalink / raw)
To: xen-devel
Cc: Anthony PERARD, Andrew Cooper, Ian Jackson, Wei Liu, Jan Beulich
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
---
tools/firmware/hvmloader/Makefile | 11 +----------
tools/firmware/hvmloader/hvmloader.c | 4 ----
2 files changed, 1 insertion(+), 14 deletions(-)
diff --git a/tools/firmware/hvmloader/Makefile b/tools/firmware/hvmloader/Makefile
index dee1c6b..9f7357f 100644
--- a/tools/firmware/hvmloader/Makefile
+++ b/tools/firmware/hvmloader/Makefile
@@ -37,6 +37,7 @@ OBJS = hvmloader.o mp_tables.o util.o smbios.o
OBJS += smp.o cacheattr.o xenbus.o vnuma.o
OBJS += e820.o pci.o pir.o ctype.o
OBJS += hvm_param.o
+OBJS += ovmf.o seabios.o
ifeq ($(debug),y)
OBJS += tests.o
endif
@@ -57,11 +58,6 @@ endif
ROMS :=
-ifeq ($(CONFIG_OVMF),y)
-OBJS += ovmf.o
-CFLAGS += -DENABLE_OVMF
-endif
-
ifeq ($(CONFIG_ROMBIOS),y)
OBJS += optionroms.o 32bitbios_support.o rombios.o
CFLAGS += -DENABLE_ROMBIOS
@@ -69,11 +65,6 @@ ROMBIOS_ROM := $(ROMBIOS_DIR)/BIOS-bochs-latest
ROMS += $(ROMBIOS_ROM) $(STDVGA_ROM) $(CIRRUSVGA_ROM) $(ETHERBOOT_ROMS)
endif
-ifeq ($(CONFIG_SEABIOS),y)
-OBJS += seabios.o
-CFLAGS += -DENABLE_SEABIOS
-endif
-
.PHONY: all
all: subdirs-all
$(MAKE) hvmloader
diff --git a/tools/firmware/hvmloader/hvmloader.c b/tools/firmware/hvmloader/hvmloader.c
index eacfe72..e0625fb 100644
--- a/tools/firmware/hvmloader/hvmloader.c
+++ b/tools/firmware/hvmloader/hvmloader.c
@@ -209,12 +209,8 @@ struct bios_info {
#ifdef ENABLE_ROMBIOS
{ "rombios", &rombios_config, },
#endif
-#ifdef ENABLE_SEABIOS
{ "seabios", &seabios_config, },
-#endif
-#ifdef ENABLE_OVMF
{ "ovmf", &ovmf_config, },
-#endif
{ NULL, NULL }
};
--
Anthony PERARD
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 43+ messages in thread* [PATCH v5 14/14] configure: do not depend on SEABIOS_PATH or OVMF_PATH ...
2016-06-22 17:15 [PATCH v5 00/14] Load BIOS via toolstack instead of been embedded in hvmloader Anthony PERARD
` (12 preceding siblings ...)
2016-06-22 17:15 ` [PATCH v5 13/14] hvmloader: Always build-in SeaBIOS and OVMF loader Anthony PERARD
@ 2016-06-22 17:15 ` Anthony PERARD
13 siblings, 0 replies; 43+ messages in thread
From: Anthony PERARD @ 2016-06-22 17:15 UTC (permalink / raw)
To: xen-devel; +Cc: Anthony PERARD, Wei Liu, Ian Jackson
... to compile SeaBIOS and OVMF. Only depend on CONFIG_*.
If --with-system-* configure option is used, then set *_CONFIG=n to not
compile SEABIOS and OVMF.
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Acked-by: Wei Liu <wei.liu2@citrix.com>
---
Please, run ./autogen.sh on this patch.
No change in V5.
Change in V4:
- change subject prefix
---
tools/configure.ac | 6 ++++--
tools/firmware/Makefile | 8 --------
2 files changed, 4 insertions(+), 10 deletions(-)
diff --git a/tools/configure.ac b/tools/configure.ac
index 4f75fae..19279a6 100644
--- a/tools/configure.ac
+++ b/tools/configure.ac
@@ -214,12 +214,13 @@ AC_ARG_WITH([system-seabios],
AS_HELP_STRING([--with-system-seabios@<:@=PATH@:>@],
[Use system supplied seabios PATH instead of building and installing
our own version]),[
+ # Disable compilation of SeaBIOS.
+ seabios=n
case $withval in
no) seabios_path= ;;
*) seabios_path=$withval ;;
esac
],[])
-AC_SUBST(seabios_path)
AC_DEFINE_UNQUOTED([SEABIOS_PATH],
["${seabios_path:-$XENFIRMWAREDIR/bios.bin}"],
[SeaBIOS path])
@@ -228,12 +229,13 @@ AC_ARG_WITH([system-ovmf],
AS_HELP_STRING([--with-system-ovmf@<:@=PATH@:>@],
[Use system supplied OVMF PATH instead of building and installing
our own version]),[
+ # Disable compilation of OVMF.
+ ovmf=n
case $withval in
no) ovmf_path= ;;
*) ovmf_path=$withval ;;
esac
],[])
-AC_SUBST(ovmf_path)
AC_DEFINE_UNQUOTED([OVMF_PATH],
["${ovmf_path:-$XENFIRMWAREDIR/ovmf.bin}"],
[OVMF path])
diff --git a/tools/firmware/Makefile b/tools/firmware/Makefile
index 82b1f6b..cf09ad2 100644
--- a/tools/firmware/Makefile
+++ b/tools/firmware/Makefile
@@ -6,12 +6,8 @@ TARGET := hvmloader/hvmloader
INST_DIR := $(DESTDIR)$(XENFIRMWAREDIR)
SUBDIRS-y :=
-ifeq ($(OVMF_PATH),)
SUBDIRS-$(CONFIG_OVMF) += ovmf-dir
-endif
-ifeq ($(SEABIOS_PATH),)
SUBDIRS-$(CONFIG_SEABIOS) += seabios-dir
-endif
SUBDIRS-$(CONFIG_ROMBIOS) += rombios
SUBDIRS-$(CONFIG_ROMBIOS) += vgabios
SUBDIRS-$(CONFIG_ROMBIOS) += etherboot
@@ -46,15 +42,11 @@ install: all
[ -d $(INST_DIR) ] || $(INSTALL_DIR) $(INST_DIR)
[ ! -e $(TARGET) ] || $(INSTALL_DATA) $(TARGET) $(INST_DIR)
ifeq ($(CONFIG_SEABIOS),y)
-ifeq ($(SEABIOS_PATH),)
$(INSTALL_DATA) seabios-dir/out/bios.bin $(INST_DIR)/bios.bin
endif
-endif
ifeq ($(CONFIG_OVMF),y)
-ifeq ($(OVMF_PATH),)
$(INSTALL_DATA) ovmf-dir/ovmf.bin $(INST_DIR)/ovmf.bin
endif
-endif
.PHONY: clean
clean: subdirs-clean
--
Anthony PERARD
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 43+ messages in thread