* [PATCH v4 00/14] Load BIOS via toolstack instead of been embedded in hvmloader.
@ 2016-03-14 17:55 Anthony PERARD
2016-03-14 17:55 ` [PATCH v4 01/14] libxc: Rework extra module initialisation Anthony PERARD
` (15 more replies)
0 siblings, 16 replies; 73+ messages in thread
From: Anthony PERARD @ 2016-03-14 17:55 UTC (permalink / raw)
To: xen-devel
Cc: Wei Liu, Stefano Stabellini, Andrew Cooper, Ian Jackson,
Jan Beulich, Anthony PERARD
Hi all,
Few changes in V4:
I leave the ACPI alone and only load the BIOS via libxl now.
Detail of changes in each patches.
I've look at loading the BIOS via the toolstack instead of having them embedded
in the hvmloader binary. After this patch series, hvmloader compilation would
be indenpendant from SeaBIOS and OVMF compilation.
Here is a general view of the few step to load the BIOS:
- libxl load the BIOS blob into it's memory and add it to struct
xc_hvm_build_args.bios_module
- libxc load the blob into the guest memory and fill the struct
hvm_start_info and store a name for each module into the module cmdline.
- hvmloader read the addresses from the hvm_start_info, find out which
module to load and copy the blob to the right place.
A git tree can be found here:
git://xenbits.xen.org/people/aperard/xen-unstable.git
tag: hvmloader-with-separated-bios-v4
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Regards,
Anthony PERARD (14):
libxc: Rework extra module initialisation
libxc: Prepare a start info structure for hvmloader
configure: #define SEABIOS_PATH and OVMF_PATH
firmware/makefile: install BIOS blob ...
libxl: Load guest BIOS from file
xen: Move the hvm_start_info C representation from libxc to
public/xen.h
hvmloader: Grab the hvm_start_info pointer
hvmloader: Locate the BIOS blob
hvmloader: Check modules whereabouts in perform_tests
hvmloader: Load SeaBIOS from hvm_start_info modules
hvmloader: Load OVMF from modules
hvmloader: Specific bios_load function required
hvmloader: Always build-in SeaBIOS and OVMF loader
configure: do not depend on SEABIOS_PATH or OVMF_PATH ...
docs/man/xl.cfg.pod.5 | 9 +++
tools/configure.ac | 12 +++-
tools/firmware/Makefile | 13 ++--
tools/firmware/hvmloader/Makefile | 39 +---------
tools/firmware/hvmloader/config.h | 2 +-
tools/firmware/hvmloader/hvmloader.c | 63 ++++++++++++++---
tools/firmware/hvmloader/ovmf.c | 33 ++++-----
tools/firmware/hvmloader/rombios.c | 3 +-
tools/firmware/hvmloader/seabios.c | 24 ++++---
tools/firmware/hvmloader/tests.c | 20 ++++++
tools/firmware/hvmloader/util.h | 5 ++
tools/libxc/include/xc_dom.h | 34 +--------
tools/libxc/xc_dom_hvmloader.c | 133 +++++++++++------------------------
tools/libxc/xc_dom_x86.c | 132 +++++++++++++++++++++++-----------
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 ++-
xen/include/public/xen.h | 33 ++++++++-
21 files changed, 391 insertions(+), 253 deletions(-)
--
Anthony PERARD
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 73+ messages in thread
* [PATCH v4 01/14] libxc: Rework extra module initialisation
2016-03-14 17:55 [PATCH v4 00/14] Load BIOS via toolstack instead of been embedded in hvmloader Anthony PERARD
@ 2016-03-14 17:55 ` Anthony PERARD
2016-03-16 0:06 ` Konrad Rzeszutek Wilk
2016-03-14 17:55 ` [PATCH v4 02/14] libxc: Prepare a start info structure for hvmloader Anthony PERARD
` (14 subsequent siblings)
15 siblings, 1 reply; 73+ messages in thread
From: Anthony PERARD @ 2016-03-14 17:55 UTC (permalink / raw)
To: xen-devel; +Cc: Anthony PERARD, Wei Liu, Ian Jackson, Stefano Stabellini
This patch use xc_dom_alloc_segment() to allocate the memory space for the
ACPI modules and the SMBIOS modules. This is to replace the arbitrary
placement of 1MB after the hvmloader image.
In later patches, while trying to load a firmware such as OVMF, the later
could easily be loaded past the address 4MB (OVMF is a 2MB binary), but
hvmloader use a range of memory from 4MB to 8MB to perform tests and in the
process, clear the memory, before loading the modules.
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
Changes in V4:
- check that guest_addr_out have a reasonable value.
New patch in V3.
---
tools/libxc/xc_dom_hvmloader.c | 131 ++++++++++++-----------------------------
1 file changed, 38 insertions(+), 93 deletions(-)
diff --git a/tools/libxc/xc_dom_hvmloader.c b/tools/libxc/xc_dom_hvmloader.c
index 330d5e8..7cf5854 100644
--- a/tools/libxc/xc_dom_hvmloader.c
+++ b/tools/libxc/xc_dom_hvmloader.c
@@ -129,98 +129,52 @@ static elf_errorstatus xc_dom_parse_hvm_kernel(struct xc_dom_image *dom)
return rc;
}
-static int modules_init(struct xc_dom_image *dom,
- uint64_t vend, struct elf_binary *elf,
- uint64_t *mstart_out, uint64_t *mend_out)
+static int module_init_one(struct xc_dom_image *dom,
+ struct xc_hvm_firmware_module *module,
+ char *name)
{
-#define MODULE_ALIGN 1UL << 7
-#define MB_ALIGN 1UL << 20
-#define MKALIGN(x, a) (((uint64_t)(x) + (a) - 1) & ~(uint64_t)((a) - 1))
- uint64_t total_len = 0, offset1 = 0;
+ struct xc_dom_seg seg;
+ void *dest;
- if ( dom->acpi_module.length == 0 && dom->smbios_module.length == 0 )
- return 0;
-
- /* Find the total length for the firmware modules with a reasonable large
- * alignment size to align each the modules.
- */
- total_len = MKALIGN(dom->acpi_module.length, MODULE_ALIGN);
- offset1 = total_len;
- total_len += MKALIGN(dom->smbios_module.length, MODULE_ALIGN);
-
- /* Want to place the modules 1Mb+change behind the loader image. */
- *mstart_out = MKALIGN(elf->pend, MB_ALIGN) + (MB_ALIGN);
- *mend_out = *mstart_out + total_len;
-
- if ( *mend_out > vend )
- return -1;
-
- if ( dom->acpi_module.length != 0 )
- dom->acpi_module.guest_addr_out = *mstart_out;
- if ( dom->smbios_module.length != 0 )
- dom->smbios_module.guest_addr_out = *mstart_out + offset1;
+ if ( module->length )
+ {
+ if ( xc_dom_alloc_segment(dom, &seg, name, 0, module->length) )
+ goto err;
+ dest = xc_dom_seg_to_ptr(dom, &seg);
+ if ( dest == NULL )
+ {
+ DOMPRINTF("%s: xc_dom_seg_to_ptr(dom, &seg) => NULL",
+ __FUNCTION__);
+ goto err;
+ }
+ memcpy(dest, module->data, module->length);
+ module->guest_addr_out = seg.vstart;
+ if ( module->guest_addr_out > UINT32_MAX ||
+ module->guest_addr_out + module->length > UINT32_MAX )
+ {
+ DOMPRINTF("%s: Module %s would be loaded abrove 4GB",
+ __FUNCTION__, name);
+ goto err;
+ }
+ }
return 0;
+err:
+ return -1;
}
-static int loadmodules(struct xc_dom_image *dom,
- uint64_t mstart, uint64_t mend,
- uint32_t domid)
+static int modules_init(struct xc_dom_image *dom)
{
- privcmd_mmap_entry_t *entries = NULL;
- unsigned long pfn_start;
- unsigned long pfn_end;
- size_t pages;
- uint32_t i;
- uint8_t *dest;
- int rc = -1;
- xc_interface *xch = dom->xch;
-
- if ( mstart == 0 || mend == 0 )
- return 0;
-
- pfn_start = (unsigned long)(mstart >> PAGE_SHIFT);
- pfn_end = (unsigned long)((mend + PAGE_SIZE - 1) >> PAGE_SHIFT);
- pages = pfn_end - pfn_start;
-
- /* Map address space for module list. */
- entries = calloc(pages, sizeof(privcmd_mmap_entry_t));
- if ( entries == NULL )
- goto error_out;
-
- for ( i = 0; i < pages; i++ )
- entries[i].mfn = (mstart >> PAGE_SHIFT) + i;
-
- dest = xc_map_foreign_ranges(
- xch, domid, pages << PAGE_SHIFT, PROT_READ | PROT_WRITE, 1 << PAGE_SHIFT,
- entries, pages);
- if ( dest == NULL )
- goto error_out;
-
- /* Zero the range so padding is clear between modules */
- memset(dest, 0, pages << PAGE_SHIFT);
-
- /* Load modules into range */
- if ( dom->acpi_module.length != 0 )
- {
- memcpy(dest,
- dom->acpi_module.data,
- dom->acpi_module.length);
- }
- if ( dom->smbios_module.length != 0 )
- {
- memcpy(dest + (dom->smbios_module.guest_addr_out - mstart),
- dom->smbios_module.data,
- dom->smbios_module.length);
- }
-
- munmap(dest, pages << PAGE_SHIFT);
- rc = 0;
+ int rc;
- error_out:
- free(entries);
+ rc = module_init_one(dom, &dom->acpi_module, "acpi module");
+ if ( rc ) goto err;
+ rc = module_init_one(dom, &dom->smbios_module, "smbios module");
+ if ( rc ) goto err;
- 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] 73+ messages in thread
* [PATCH v4 02/14] libxc: Prepare a start info structure for hvmloader
2016-03-14 17:55 [PATCH v4 00/14] Load BIOS via toolstack instead of been embedded in hvmloader Anthony PERARD
2016-03-14 17:55 ` [PATCH v4 01/14] libxc: Rework extra module initialisation Anthony PERARD
@ 2016-03-14 17:55 ` Anthony PERARD
2016-03-16 0:18 ` Konrad Rzeszutek Wilk
2016-03-14 17:55 ` [PATCH v4 03/14] configure: #define SEABIOS_PATH and OVMF_PATH Anthony PERARD
` (13 subsequent siblings)
15 siblings, 1 reply; 73+ messages in thread
From: Anthony PERARD @ 2016-03-14 17:55 UTC (permalink / raw)
To: xen-devel; +Cc: Anthony PERARD, Wei Liu, Ian Jackson, Stefano Stabellini
... and load BIOS into guest memory.
This adds a new firmware module, bios_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>
---
Change 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.
Change 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 | 2 +
tools/libxc/xc_dom_x86.c | 132 ++++++++++++++++++++++++++++-------------
xen/include/public/xen.h | 2 +-
4 files changed, 96 insertions(+), 43 deletions(-)
diff --git a/tools/libxc/include/xc_dom.h b/tools/libxc/include/xc_dom.h
index 6ebe946..93f894c 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 passed to HVMLOADER */
+ struct xc_hvm_firmware_module bios_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 7cf5854..606351a 100644
--- a/tools/libxc/xc_dom_hvmloader.c
+++ b/tools/libxc/xc_dom_hvmloader.c
@@ -167,6 +167,8 @@ static int modules_init(struct xc_dom_image *dom)
{
int rc;
+ rc = module_init_one(dom, &dom->bios_module, "bios 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 bdec40a..9c56d55 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,26 @@ 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 */
+ start_info_size +=
+ HVMLOADER_MODULE_NAME_SIZE * HVMLOADER_MODULE_MAX_COUNT;
+ }
+
+ 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;
+ }
+
+ if ( dom->device_model )
+ {
/*
* Allocate and clear additional ioreq server pages. The default
* server will use the IOREQ and BUFIOREQ special pages above.
@@ -1696,39 +1707,68 @@ static int alloc_pgtables_hvm(struct xc_dom_image *dom)
return 0;
}
+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;
+ 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((char*)(modlist + HVMLOADER_MODULE_MAX_COUNT)
+ + HVMLOADER_MODULE_NAME_SIZE * index,
+ name, HVMLOADER_MODULE_NAME_SIZE);
+ modlist[index].cmdline_paddr =
+ (dom->start_info_seg.pfn << PAGE_SHIFT) +
+ ((uintptr_t)modlist - (uintptr_t)start_info) +
+ sizeof(struct hvm_modlist_entry) * HVMLOADER_MODULE_MAX_COUNT +
+ 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;
+ void *start_page;
+ 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_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 = start_page;
+ start_info = start_page;
+ modlist = start_page + sizeof(*start_info) + dom->cmdline_size;
+ if ( !dom->device_model )
+ {
if ( dom->cmdline )
{
char *cmdline = start_page + sizeof(*start_info);
@@ -1740,22 +1780,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->bios_module, "bios",
+ 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_page, 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 64ba7ab..1cfec5c 100644
--- a/xen/include/public/xen.h
+++ b/xen/include/public/xen.h
@@ -798,7 +798,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] 73+ messages in thread
* [PATCH v4 03/14] configure: #define SEABIOS_PATH and OVMF_PATH
2016-03-14 17:55 [PATCH v4 00/14] Load BIOS via toolstack instead of been embedded in hvmloader Anthony PERARD
2016-03-14 17:55 ` [PATCH v4 01/14] libxc: Rework extra module initialisation Anthony PERARD
2016-03-14 17:55 ` [PATCH v4 02/14] libxc: Prepare a start info structure for hvmloader Anthony PERARD
@ 2016-03-14 17:55 ` Anthony PERARD
2016-03-16 0:20 ` Konrad Rzeszutek Wilk
2016-04-08 13:38 ` Wei Liu
2016-03-14 17:55 ` [PATCH v4 04/14] firmware/makefile: install BIOS blob Anthony PERARD
` (12 subsequent siblings)
15 siblings, 2 replies; 73+ messages in thread
From: Anthony PERARD @ 2016-03-14 17:55 UTC (permalink / raw)
To: xen-devel; +Cc: Anthony PERARD, Wei Liu, Ian Jackson, Stefano Stabellini
Those paths are to be used by libxl, in order to load the firmware in
memory. If a system path is not define via --with-system-seabios or
--with-system-ovmf, then this default to the Xen firmware directory.
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
Please, run ./autogen.sh on this patch.
---
tools/configure.ac | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/tools/configure.ac b/tools/configure.ac
index 5b5dda4..7e5452e 100644
--- a/tools/configure.ac
+++ b/tools/configure.ac
@@ -218,6 +218,9 @@ AC_ARG_WITH([system-seabios],
esac
],[])
AC_SUBST(seabios_path)
+AC_DEFINE_UNQUOTED([SEABIOS_PATH],
+ ["${seabios_path:-$XENFIRMWAREDIR/seabios.bin}"],
+ [SeaBIOS path])
AC_ARG_WITH([system-ovmf],
AS_HELP_STRING([--with-system-ovmf@<:@=PATH@:>@],
@@ -229,6 +232,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] 73+ messages in thread
* [PATCH v4 04/14] firmware/makefile: install BIOS blob ...
2016-03-14 17:55 [PATCH v4 00/14] Load BIOS via toolstack instead of been embedded in hvmloader Anthony PERARD
` (2 preceding siblings ...)
2016-03-14 17:55 ` [PATCH v4 03/14] configure: #define SEABIOS_PATH and OVMF_PATH Anthony PERARD
@ 2016-03-14 17:55 ` Anthony PERARD
2016-03-16 0:26 ` Konrad Rzeszutek Wilk
` (2 more replies)
2016-03-14 17:55 ` [PATCH v4 05/14] libxl: Load guest BIOS from file Anthony PERARD
` (11 subsequent siblings)
15 siblings, 3 replies; 73+ messages in thread
From: Anthony PERARD @ 2016-03-14 17:55 UTC (permalink / raw)
To: xen-devel; +Cc: Anthony PERARD, Wei Liu, Ian Jackson, Stefano Stabellini
... into the firmware directory, along with hvmloader.
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
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 | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/tools/firmware/Makefile b/tools/firmware/Makefile
index 6cc86ce..6a37758 100644
--- a/tools/firmware/Makefile
+++ b/tools/firmware/Makefile
@@ -19,6 +19,9 @@ SUBDIRS-y += hvmloader
LD32BIT-$(CONFIG_FreeBSD) := LD32BIT_FLAG=-melf_i386_fbsd
+SEABIOS_ROM := seabios-dir/out/bios.bin
+OVMF_ROM := ovmf-dir/ovmf.bin
+
ovmf-dir:
GIT=$(GIT) $(XEN_ROOT)/scripts/git-checkout.sh $(OVMF_UPSTREAM_URL) $(OVMF_UPSTREAM_REVISION) ovmf-dir
cp ovmf-makefile ovmf-dir/Makefile;
@@ -45,6 +48,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_ROM) $(INST_DIR)/seabios.bin
+endif
+endif
+ifeq ($(CONFIG_OVMF),y)
+ifeq ($(OVMF_PATH),)
+ $(INSTALL_DATA) $(OVMF_ROM) $(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] 73+ messages in thread
* [PATCH v4 05/14] libxl: Load guest BIOS from file
2016-03-14 17:55 [PATCH v4 00/14] Load BIOS via toolstack instead of been embedded in hvmloader Anthony PERARD
` (3 preceding siblings ...)
2016-03-14 17:55 ` [PATCH v4 04/14] firmware/makefile: install BIOS blob Anthony PERARD
@ 2016-03-14 17:55 ` Anthony PERARD
2016-03-16 0:53 ` Konrad Rzeszutek Wilk
2016-03-14 17:55 ` [PATCH v4 06/14] xen: Move the hvm_start_info C representation from libxc to public/xen.h Anthony PERARD
` (10 subsequent siblings)
15 siblings, 1 reply; 73+ messages in thread
From: Anthony PERARD @ 2016-03-14 17:55 UTC (permalink / raw)
To: xen-devel; +Cc: Anthony PERARD, Wei Liu, Stefano Stabellini
The path to the BIOS blob can be override by the xl's bios_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>
---
Change in V4:
- updating man page to have bios_override described.
- return ERROR_INVAL in libxl__load_hvm_firmware_module when the file is
empty.
Change 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 | 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 b/docs/man/xl.cfg.pod.5
index 56b1117..165915b 100644
--- a/docs/man/xl.cfg.pod.5
+++ b/docs/man/xl.cfg.pod.5
@@ -1121,6 +1121,15 @@ Requires device_model_version=qemu-xen.
=back
+=item B<bios_override="PATH">
+
+Override the path to the blob to be used as BIOS. The blob provided here MUST
+be consistent with the `bios` which you have specified. You should not normally
+need to specify this option.
+
+This options does not have any effect if using bios="rombios" or
+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 f9e3ef5..d06c6c5 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -884,6 +884,14 @@ void libxl_mac_copy(libxl_ctx *ctx, libxl_mac *dst, libxl_mac *src);
#define LIBXL_HAVE_CHECKPOINTED_STREAM 1
/*
+ * LIBXL_HAVE_BUILDINFO_HVM_BIOS_FIRMWARE
+ *
+ * libxl_domain_build_info has u.hvm.bios_firmware field which can be use
+ * to provide a different bios blob (like SeaBIOS or OVMF).
+ */
+#define LIBXL_HAVE_BUILDINFO_HVM_BIOS_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 b825b98..c112cc5 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 e;
+
+ LOG(DEBUG, "Loading %s: %s", what, filename);
+ e = libxl_read_file_contents(CTX, filename, &data, &datalen);
+ if (e) {
+ /*
+ * Print a message only on ENOENT, other error are logged by the
+ * function libxl_read_file_contents().
+ */
+ if (e == ENOENT)
+ LOGEV(ERROR, e, "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.bios_firmware) {
+ bios_filename = info->u.hvm.bios_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->bios_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 005fe53..af3ba9a 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -2265,6 +2265,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 632c009..3978fd9 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -493,6 +493,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
("timer_mode", libxl_timer_mode),
("nested_hvm", libxl_defbool),
("altp2m", libxl_defbool),
+ ("bios_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 990d3c9..08ceede 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -1505,12 +1505,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_override",
+ &b_info->u.hvm.bios_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.bios_firmware)
+ fprintf(stderr, "WARNING: "
+ "bios_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] 73+ messages in thread
* [PATCH v4 06/14] xen: Move the hvm_start_info C representation from libxc to public/xen.h
2016-03-14 17:55 [PATCH v4 00/14] Load BIOS via toolstack instead of been embedded in hvmloader Anthony PERARD
` (4 preceding siblings ...)
2016-03-14 17:55 ` [PATCH v4 05/14] libxl: Load guest BIOS from file Anthony PERARD
@ 2016-03-14 17:55 ` Anthony PERARD
2016-03-15 8:09 ` Jan Beulich
2016-03-14 17:55 ` [PATCH v4 07/14] hvmloader: Grab the hvm_start_info pointer Anthony PERARD
` (9 subsequent siblings)
15 siblings, 1 reply; 73+ messages in thread
From: Anthony PERARD @ 2016-03-14 17:55 UTC (permalink / raw)
To: xen-devel; +Cc: Anthony PERARD, Wei Liu, Ian Jackson, Stefano Stabellini
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>
---
New in V4.
---
tools/libxc/include/xc_dom.h | 31 -------------------------------
xen/include/public/xen.h | 31 +++++++++++++++++++++++++++++++
2 files changed, 31 insertions(+), 31 deletions(-)
diff --git a/tools/libxc/include/xc_dom.h b/tools/libxc/include/xc_dom.h
index 93f894c..567016c 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 cmdline_paddr; /* Physical address of the command line. */
- uint32_t nr_modules; /* Number of modules passed to the kernel. */
- uint32_t modlist_paddr; /* Physical address of an array of */
- /* hvm_modlist_entry. */
- uint32_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 1cfec5c..c270cf1 100644
--- a/xen/include/public/xen.h
+++ b/xen/include/public/xen.h
@@ -841,6 +841,37 @@ 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.
+ *
+ * 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 cmdline_paddr; /* Physical address of the command line. */
+ uint32_t nr_modules; /* Number of modules passed to the kernel. */
+ uint32_t modlist_paddr; /* Physical address of an array of */
+ /* hvm_modlist_entry. */
+ uint32_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 */
+
/* 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] 73+ messages in thread
* [PATCH v4 07/14] hvmloader: Grab the hvm_start_info pointer
2016-03-14 17:55 [PATCH v4 00/14] Load BIOS via toolstack instead of been embedded in hvmloader Anthony PERARD
` (5 preceding siblings ...)
2016-03-14 17:55 ` [PATCH v4 06/14] xen: Move the hvm_start_info C representation from libxc to public/xen.h Anthony PERARD
@ 2016-03-14 17:55 ` Anthony PERARD
2016-03-16 1:07 ` Konrad Rzeszutek Wilk
2016-04-05 12:43 ` Jan Beulich
2016-03-14 17:55 ` [PATCH v4 08/14] hvmloader: Locate the BIOS blob Anthony PERARD
` (8 subsequent siblings)
15 siblings, 2 replies; 73+ messages in thread
From: Anthony PERARD @ 2016-03-14 17:55 UTC (permalink / raw)
To: xen-devel
Cc: Keir Fraser, Stefano Stabellini, Andrew Cooper, Ian Jackson,
Jan Beulich, Anthony PERARD, Wei Liu
Signed-off-by: Anthony PERARD <anthony.perard@citrix.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] 73+ messages in thread
* [PATCH v4 08/14] hvmloader: Locate the BIOS blob
2016-03-14 17:55 [PATCH v4 00/14] Load BIOS via toolstack instead of been embedded in hvmloader Anthony PERARD
` (6 preceding siblings ...)
2016-03-14 17:55 ` [PATCH v4 07/14] hvmloader: Grab the hvm_start_info pointer Anthony PERARD
@ 2016-03-14 17:55 ` Anthony PERARD
2016-03-16 1:14 ` Konrad Rzeszutek Wilk
2016-04-05 12:59 ` Jan Beulich
2016-03-14 17:55 ` [PATCH v4 09/14] hvmloader: Check modules whereabouts in perform_tests Anthony PERARD
` (7 subsequent siblings)
15 siblings, 2 replies; 73+ messages in thread
From: Anthony PERARD @ 2016-03-14 17:55 UTC (permalink / raw)
To: xen-devel
Cc: Keir Fraser, Stefano Stabellini, Andrew Cooper, Ian Jackson,
Jan Beulich, Anthony PERARD, Wei Liu
The BIOS can be found an entry called "bios" 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. It is going to be used by the next few
patches.
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
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 | 42 ++++++++++++++++++++++++++++++++++--
tools/firmware/hvmloader/ovmf.c | 3 ++-
tools/firmware/hvmloader/rombios.c | 3 ++-
tools/firmware/hvmloader/util.h | 2 ++
5 files changed, 47 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..460efb9 100644
--- a/tools/firmware/hvmloader/hvmloader.c
+++ b/tools/firmware/hvmloader/hvmloader.c
@@ -253,10 +253,40 @@ 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 *)info->modlist_paddr;
+ unsigned int i;
+
+ if ( !modlist )
+ return NULL;
+
+ for ( i = 0; i < info->nr_modules; i++ )
+ {
+ uint32_t module_name = modlist[i].cmdline_paddr;
+
+ BUG_ON(!modlist[i].cmdline_paddr ||
+ modlist[i].cmdline_paddr > UINT_MAX);
+
+ if ( !strcmp(name, (char*)module_name) )
+ {
+ BUG_ON(!modlist[i].paddr || modlist[i].paddr > UINT_MAX ||
+ modlist[i].size > UINT_MAX);
+ 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 +322,16 @@ 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, "bios");
+ 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, 0, 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] 73+ messages in thread
* [PATCH v4 09/14] hvmloader: Check modules whereabouts in perform_tests
2016-03-14 17:55 [PATCH v4 00/14] Load BIOS via toolstack instead of been embedded in hvmloader Anthony PERARD
` (7 preceding siblings ...)
2016-03-14 17:55 ` [PATCH v4 08/14] hvmloader: Locate the BIOS blob Anthony PERARD
@ 2016-03-14 17:55 ` Anthony PERARD
2016-03-16 1:23 ` Konrad Rzeszutek Wilk
2016-04-05 13:07 ` Jan Beulich
2016-03-14 17:55 ` [PATCH v4 10/14] hvmloader: Load SeaBIOS from hvm_start_info modules Anthony PERARD
` (6 subsequent siblings)
15 siblings, 2 replies; 73+ messages in thread
From: Anthony PERARD @ 2016-03-14 17:55 UTC (permalink / raw)
To: xen-devel
Cc: Keir Fraser, Stefano Stabellini, Andrew Cooper, Ian Jackson,
Jan Beulich, Anthony PERARD, Wei Liu
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 v4:
- move the check into the perform_test() function.
- skip tests instead of using BUG.
New in V3
---
tools/firmware/hvmloader/tests.c | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/tools/firmware/hvmloader/tests.c b/tools/firmware/hvmloader/tests.c
index fea3ad3..7996206 100644
--- a/tools/firmware/hvmloader/tests.c
+++ b/tools/firmware/hvmloader/tests.c
@@ -210,6 +210,26 @@ void perform_tests(void)
return;
}
+ /* Check that tests does not use memory where modules are stored */
+ if ( ((uint32_t)hvm_start_info + sizeof(struct hvm_start_info)) > 4 << 20
+ && (uint32_t)hvm_start_info < 8 << 20 )
+ {
+ printf("Skipping tests due to memory used by hvm_start_info\n");
+ return;
+ }
+ for ( unsigned i = 0; i < hvm_start_info->nr_modules; i++ )
+ {
+ const struct hvm_modlist_entry *modlist =
+ (struct hvm_modlist_entry *)hvm_start_info->modlist_paddr;
+ if ( modlist[i].paddr
+ && modlist[i].paddr + modlist[i].size > 4ul << 20
+ && modlist[i].paddr < 8ul << 20 )
+ {
+ printf("Skipping tests due to memory used by a module\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] 73+ messages in thread
* [PATCH v4 10/14] hvmloader: Load SeaBIOS from hvm_start_info modules
2016-03-14 17:55 [PATCH v4 00/14] Load BIOS via toolstack instead of been embedded in hvmloader Anthony PERARD
` (8 preceding siblings ...)
2016-03-14 17:55 ` [PATCH v4 09/14] hvmloader: Check modules whereabouts in perform_tests Anthony PERARD
@ 2016-03-14 17:55 ` Anthony PERARD
2016-03-16 1:27 ` Konrad Rzeszutek Wilk
2016-04-05 13:11 ` Jan Beulich
2016-03-14 17:55 ` [PATCH v4 11/14] hvmloader: Load OVMF from modules Anthony PERARD
` (5 subsequent siblings)
15 siblings, 2 replies; 73+ messages in thread
From: Anthony PERARD @ 2016-03-14 17:55 UTC (permalink / raw)
To: xen-devel
Cc: Keir Fraser, Stefano Stabellini, Andrew Cooper, Ian Jackson,
Jan Beulich, Anthony PERARD, Wei Liu
... 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 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 | 24 ++++++++++++++----------
2 files changed, 15 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..5d0a491 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,29 @@ static void seabios_setup_e820(void)
struct e820entry *e820 = scratch_alloc(sizeof(struct e820entry)*16, 0);
info->e820 = (uint32_t)e820;
+ BUG_ON(seabios_config.bios_address < 0xc0000 || seabios_config.bios_address >= 0x100000);
/* 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] 73+ messages in thread
* [PATCH v4 11/14] hvmloader: Load OVMF from modules
2016-03-14 17:55 [PATCH v4 00/14] Load BIOS via toolstack instead of been embedded in hvmloader Anthony PERARD
` (9 preceding siblings ...)
2016-03-14 17:55 ` [PATCH v4 10/14] hvmloader: Load SeaBIOS from hvm_start_info modules Anthony PERARD
@ 2016-03-14 17:55 ` Anthony PERARD
2016-03-16 1:36 ` Konrad Rzeszutek Wilk
2016-04-05 13:16 ` Jan Beulich
2016-03-14 17:55 ` [PATCH v4 12/14] hvmloader: Specific bios_load function required Anthony PERARD
` (4 subsequent siblings)
15 siblings, 2 replies; 73+ messages in thread
From: Anthony PERARD @ 2016-03-14 17:55 UTC (permalink / raw)
To: xen-devel
Cc: Keir Fraser, Stefano Stabellini, Andrew Cooper, Ian Jackson,
Jan Beulich, Anthony PERARD, Wei Liu
... and do not include the OVMF ROM into hvmloader anymore.
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
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 | 30 +++++++++++++-----------------
2 files changed, 14 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..6607e57 100644
--- a/tools/firmware/hvmloader/ovmf.c
+++ b/tools/firmware/hvmloader/ovmf.c
@@ -34,17 +34,10 @@
#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 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 +90,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 = 0x100000000ULL
+ - ((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 +151,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] 73+ messages in thread
* [PATCH v4 12/14] hvmloader: Specific bios_load function required
2016-03-14 17:55 [PATCH v4 00/14] Load BIOS via toolstack instead of been embedded in hvmloader Anthony PERARD
` (10 preceding siblings ...)
2016-03-14 17:55 ` [PATCH v4 11/14] hvmloader: Load OVMF from modules Anthony PERARD
@ 2016-03-14 17:55 ` Anthony PERARD
2016-03-16 1:38 ` Konrad Rzeszutek Wilk
2016-03-14 17:55 ` [PATCH v4 13/14] hvmloader: Always build-in SeaBIOS and OVMF loader Anthony PERARD
` (3 subsequent siblings)
15 siblings, 1 reply; 73+ messages in thread
From: Anthony PERARD @ 2016-03-14 17:55 UTC (permalink / raw)
To: xen-devel
Cc: Keir Fraser, Stefano Stabellini, Andrew Cooper, Ian Jackson,
Jan Beulich, Anthony PERARD, Wei Liu
All BIOS 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>
---
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 460efb9..bb2a309 100644
--- a/tools/firmware/hvmloader/hvmloader.c
+++ b/tools/firmware/hvmloader/hvmloader.c
@@ -323,21 +323,25 @@ int main(void)
printf("Loading %s ...\n", bios->name);
bios_module = get_module_entry(hvm_start_info, "bios");
- 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, 0, 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] 73+ messages in thread
* [PATCH v4 13/14] hvmloader: Always build-in SeaBIOS and OVMF loader
2016-03-14 17:55 [PATCH v4 00/14] Load BIOS via toolstack instead of been embedded in hvmloader Anthony PERARD
` (11 preceding siblings ...)
2016-03-14 17:55 ` [PATCH v4 12/14] hvmloader: Specific bios_load function required Anthony PERARD
@ 2016-03-14 17:55 ` Anthony PERARD
2016-03-14 17:55 ` [PATCH v4 14/14] configure: do not depend on SEABIOS_PATH or OVMF_PATH Anthony PERARD
` (2 subsequent siblings)
15 siblings, 0 replies; 73+ messages in thread
From: Anthony PERARD @ 2016-03-14 17:55 UTC (permalink / raw)
To: xen-devel
Cc: Keir Fraser, Stefano Stabellini, Andrew Cooper, Ian Jackson,
Jan Beulich, Anthony PERARD, Wei Liu
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 bb2a309..7631de2 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] 73+ messages in thread
* [PATCH v4 14/14] configure: do not depend on SEABIOS_PATH or OVMF_PATH ...
2016-03-14 17:55 [PATCH v4 00/14] Load BIOS via toolstack instead of been embedded in hvmloader Anthony PERARD
` (12 preceding siblings ...)
2016-03-14 17:55 ` [PATCH v4 13/14] hvmloader: Always build-in SeaBIOS and OVMF loader Anthony PERARD
@ 2016-03-14 17:55 ` Anthony PERARD
2016-03-16 1:40 ` Konrad Rzeszutek Wilk
2016-04-08 13:38 ` Wei Liu
2016-03-24 17:55 ` [PATCH v4 00/14] Load BIOS via toolstack instead of been embedded in hvmloader Jim Fehlig
2016-03-30 17:22 ` Jim Fehlig
15 siblings, 2 replies; 73+ messages in thread
From: Anthony PERARD @ 2016-03-14 17:55 UTC (permalink / raw)
To: xen-devel; +Cc: Anthony PERARD, Wei Liu, Ian Jackson, Stefano Stabellini
... to compile SeaBIOS and OVMF. Only depends 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>
---
Change in V4:
- change subject prefix
Please, run ./autogen.sh on this patch.
---
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 7e5452e..ef306ae 100644
--- a/tools/configure.ac
+++ b/tools/configure.ac
@@ -212,12 +212,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/seabios.bin}"],
[SeaBIOS path])
@@ -226,12 +227,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 6a37758..4975cb4 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
@@ -49,15 +45,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_ROM) $(INST_DIR)/seabios.bin
endif
-endif
ifeq ($(CONFIG_OVMF),y)
-ifeq ($(OVMF_PATH),)
$(INSTALL_DATA) $(OVMF_ROM) $(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] 73+ messages in thread
* Re: [PATCH v4 06/14] xen: Move the hvm_start_info C representation from libxc to public/xen.h
2016-03-14 17:55 ` [PATCH v4 06/14] xen: Move the hvm_start_info C representation from libxc to public/xen.h Anthony PERARD
@ 2016-03-15 8:09 ` Jan Beulich
2016-03-16 0:59 ` Konrad Rzeszutek Wilk
2016-03-21 17:04 ` Roger Pau Monné
0 siblings, 2 replies; 73+ messages in thread
From: Jan Beulich @ 2016-03-15 8:09 UTC (permalink / raw)
To: Anthony PERARD, Roger Pau Monne
Cc: Ian Jackson, xen-devel, Wei Liu, Stefano Stabellini
>>> On 14.03.16 at 18:55, <anthony.perard@citrix.com> wrote:
> --- a/xen/include/public/xen.h
> +++ b/xen/include/public/xen.h
> @@ -841,6 +841,37 @@ 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.
> + *
> + * 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 cmdline_paddr; /* Physical address of the command line. */
> + uint32_t nr_modules; /* Number of modules passed to the kernel. */
> + uint32_t modlist_paddr; /* Physical address of an array of */
> + /* hvm_modlist_entry. */
> + uint32_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 */
This needs extra care: Either the packed attributes need to be
dropped (Roger - why did they get put there in the first place?),
or their use needs to be shielded from compilers not understanding
them.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH v4 01/14] libxc: Rework extra module initialisation
2016-03-14 17:55 ` [PATCH v4 01/14] libxc: Rework extra module initialisation Anthony PERARD
@ 2016-03-16 0:06 ` Konrad Rzeszutek Wilk
2016-03-17 16:24 ` Anthony PERARD
0 siblings, 1 reply; 73+ messages in thread
From: Konrad Rzeszutek Wilk @ 2016-03-16 0:06 UTC (permalink / raw)
To: Anthony PERARD; +Cc: Ian Jackson, Stefano Stabellini, Wei Liu, xen-devel
On Mon, Mar 14, 2016 at 05:55:36PM +0000, Anthony PERARD wrote:
> This patch use xc_dom_alloc_segment() to allocate the memory space for the
> ACPI modules and the SMBIOS modules. This is to replace the arbitrary
> placement of 1MB after the hvmloader image.
s/of/at/ ?
Or are you rather staying that the memory for ACPI modules and such
is offset by one 1MB after hvmloader image?
In other words:
/-----------\ <- 0
| |
| hvmloader |
| binary |
+-----------| <- 800kB
| |
| empty |
+-----------+ <- 1.8MB
| ACPI DATA |
| |
+-----------+ <- 2.8MB
| |
| ..... |
+-----------| <- 4MB
| scratch |
| space for |
| hvmloader |
\-----------/ <- 8MB
Is what can happen now? But prior to this you had:
/-----------\ <- 0
| |
| hvmloader |
| binary |
+-----------| <- 800kB
| |
| empty |
+-----------+ <- 1MB
| ACPI DATA |
| |
+-----------+ <- 2MB
| |
| ..... |
+-----------| <- 4MB
| scratch |
| space for |
| hvmloader |
\-----------/ <- 8MB
?
>
> In later patches, while trying to load a firmware such as OVMF, the later
.. what later patches? These ones? You may want to mention which ones.
> could easily be loaded past the address 4MB (OVMF is a 2MB binary), but
Why would it be loaded past 4MB? Why not in the space right after ACPI
DSDT and such?
> hvmloader use a range of memory from 4MB to 8MB to perform tests and in the
> process, clear the memory, before loading the modules.
s/clear/clears/
>
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
> Changes in V4:
> - check that guest_addr_out have a reasonable value.
>
> New patch in V3.
> ---
> tools/libxc/xc_dom_hvmloader.c | 131 ++++++++++++-----------------------------
> 1 file changed, 38 insertions(+), 93 deletions(-)
>
> diff --git a/tools/libxc/xc_dom_hvmloader.c b/tools/libxc/xc_dom_hvmloader.c
> index 330d5e8..7cf5854 100644
> --- a/tools/libxc/xc_dom_hvmloader.c
> +++ b/tools/libxc/xc_dom_hvmloader.c
> @@ -129,98 +129,52 @@ static elf_errorstatus xc_dom_parse_hvm_kernel(struct xc_dom_image *dom)
> return rc;
> }
>
> -static int modules_init(struct xc_dom_image *dom,
> - uint64_t vend, struct elf_binary *elf,
> - uint64_t *mstart_out, uint64_t *mend_out)
> +static int module_init_one(struct xc_dom_image *dom,
> + struct xc_hvm_firmware_module *module,
> + char *name)
> {
> -#define MODULE_ALIGN 1UL << 7
> -#define MB_ALIGN 1UL << 20
> -#define MKALIGN(x, a) (((uint64_t)(x) + (a) - 1) & ~(uint64_t)((a) - 1))
> - uint64_t total_len = 0, offset1 = 0;
> + struct xc_dom_seg seg;
> + void *dest;
>
> - if ( dom->acpi_module.length == 0 && dom->smbios_module.length == 0 )
> - return 0;
> -
> - /* Find the total length for the firmware modules with a reasonable large
> - * alignment size to align each the modules.
> - */
> - total_len = MKALIGN(dom->acpi_module.length, MODULE_ALIGN);
> - offset1 = total_len;
> - total_len += MKALIGN(dom->smbios_module.length, MODULE_ALIGN);
> -
> - /* Want to place the modules 1Mb+change behind the loader image. */
> - *mstart_out = MKALIGN(elf->pend, MB_ALIGN) + (MB_ALIGN);
> - *mend_out = *mstart_out + total_len;
> -
> - if ( *mend_out > vend )
> - return -1;
> -
> - if ( dom->acpi_module.length != 0 )
> - dom->acpi_module.guest_addr_out = *mstart_out;
> - if ( dom->smbios_module.length != 0 )
> - dom->smbios_module.guest_addr_out = *mstart_out + offset1;
> + if ( module->length )
> + {
> + if ( xc_dom_alloc_segment(dom, &seg, name, 0, module->length) )
> + goto err;
> + dest = xc_dom_seg_to_ptr(dom, &seg);
> + if ( dest == NULL )
> + {
> + DOMPRINTF("%s: xc_dom_seg_to_ptr(dom, &seg) => NULL",
> + __FUNCTION__);
> + goto err;
> + }
> + memcpy(dest, module->data, module->length);
> + module->guest_addr_out = seg.vstart;
> + if ( module->guest_addr_out > UINT32_MAX ||
> + module->guest_addr_out + module->length > UINT32_MAX )
> + {
> + DOMPRINTF("%s: Module %s would be loaded abrove 4GB",
> + __FUNCTION__, name);
> + goto err;
> + }
> + }
>
> return 0;
> +err:
> + return -1;
> }
>
> -static int loadmodules(struct xc_dom_image *dom,
> - uint64_t mstart, uint64_t mend,
> - uint32_t domid)
> +static int modules_init(struct xc_dom_image *dom)
> {
> - privcmd_mmap_entry_t *entries = NULL;
> - unsigned long pfn_start;
> - unsigned long pfn_end;
> - size_t pages;
> - uint32_t i;
> - uint8_t *dest;
> - int rc = -1;
> - xc_interface *xch = dom->xch;
> -
> - if ( mstart == 0 || mend == 0 )
> - return 0;
> -
> - pfn_start = (unsigned long)(mstart >> PAGE_SHIFT);
> - pfn_end = (unsigned long)((mend + PAGE_SIZE - 1) >> PAGE_SHIFT);
> - pages = pfn_end - pfn_start;
> -
> - /* Map address space for module list. */
> - entries = calloc(pages, sizeof(privcmd_mmap_entry_t));
> - if ( entries == NULL )
> - goto error_out;
> -
> - for ( i = 0; i < pages; i++ )
> - entries[i].mfn = (mstart >> PAGE_SHIFT) + i;
> -
> - dest = xc_map_foreign_ranges(
> - xch, domid, pages << PAGE_SHIFT, PROT_READ | PROT_WRITE, 1 << PAGE_SHIFT,
> - entries, pages);
> - if ( dest == NULL )
> - goto error_out;
> -
> - /* Zero the range so padding is clear between modules */
> - memset(dest, 0, pages << PAGE_SHIFT);
> -
> - /* Load modules into range */
> - if ( dom->acpi_module.length != 0 )
> - {
> - memcpy(dest,
> - dom->acpi_module.data,
> - dom->acpi_module.length);
> - }
> - if ( dom->smbios_module.length != 0 )
> - {
> - memcpy(dest + (dom->smbios_module.guest_addr_out - mstart),
> - dom->smbios_module.data,
> - dom->smbios_module.length);
> - }
> -
> - munmap(dest, pages << PAGE_SHIFT);
> - rc = 0;
> + int rc;
>
> - error_out:
> - free(entries);
> + rc = module_init_one(dom, &dom->acpi_module, "acpi module");
> + if ( rc ) goto err;
> + rc = module_init_one(dom, &dom->smbios_module, "smbios module");
> + if ( rc ) goto err;
You can't be that code thought. That is some nice deletion of code.
Now is this safe for migration?
I presume so since this is all E820_RESERVED righT? No funny business
there?
>
> - return rc;
> + return 0;
> +err:
> + return -1;
> }
>
> static elf_errorstatus xc_dom_load_hvm_kernel(struct xc_dom_image *dom)
> @@ -229,7 +183,6 @@ static elf_errorstatus xc_dom_load_hvm_kernel(struct xc_dom_image *dom)
> privcmd_mmap_entry_t *entries = NULL;
> size_t pages = (elf->pend - elf->pstart + PAGE_SIZE - 1) >> PAGE_SHIFT;
> elf_errorstatus rc;
> - uint64_t m_start = 0, m_end = 0;
> int i;
>
> /* Map address space for initial elf image. */
> @@ -262,15 +215,7 @@ static elf_errorstatus xc_dom_load_hvm_kernel(struct xc_dom_image *dom)
>
> munmap(elf->dest_base, elf->dest_size);
>
> - rc = modules_init(dom, dom->total_pages << PAGE_SHIFT, elf, &m_start,
> - &m_end);
> - if ( rc != 0 )
> - {
> - DOMPRINTF("%s: insufficient space to load modules.", __func__);
> - goto error;
> - }
> -
> - rc = loadmodules(dom, m_start, m_end, dom->guest_domid);
> + rc = modules_init(dom);
> if ( rc != 0 )
> {
> DOMPRINTF("%s: unable to load modules.", __func__);
> --
> Anthony PERARD
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH v4 02/14] libxc: Prepare a start info structure for hvmloader
2016-03-14 17:55 ` [PATCH v4 02/14] libxc: Prepare a start info structure for hvmloader Anthony PERARD
@ 2016-03-16 0:18 ` Konrad Rzeszutek Wilk
2016-03-16 18:01 ` Boris Ostrovsky
2016-03-17 16:28 ` Anthony PERARD
0 siblings, 2 replies; 73+ messages in thread
From: Konrad Rzeszutek Wilk @ 2016-03-16 0:18 UTC (permalink / raw)
To: Anthony PERARD, boris.ostrovsky, roger.pau
Cc: Ian Jackson, Stefano Stabellini, Wei Liu, xen-devel
On Mon, Mar 14, 2016 at 05:55:37PM +0000, Anthony PERARD wrote:
> ... and load BIOS into guest memory.
>
> This adds a new firmware module, bios_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.
CC-ing Boris and Roger since this impact the PVH code.
And therefore not snipping the code so they can see it in its full
glory.
>
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
> Change 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.
>
> Change 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 | 2 +
> tools/libxc/xc_dom_x86.c | 132 ++++++++++++++++++++++++++++-------------
> xen/include/public/xen.h | 2 +-
> 4 files changed, 96 insertions(+), 43 deletions(-)
>
> diff --git a/tools/libxc/include/xc_dom.h b/tools/libxc/include/xc_dom.h
> index 6ebe946..93f894c 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 passed to HVMLOADER */
> + struct xc_hvm_firmware_module bios_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 7cf5854..606351a 100644
> --- a/tools/libxc/xc_dom_hvmloader.c
> +++ b/tools/libxc/xc_dom_hvmloader.c
> @@ -167,6 +167,8 @@ static int modules_init(struct xc_dom_image *dom)
> {
> int rc;
>
> + rc = module_init_one(dom, &dom->bios_module, "bios 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 bdec40a..9c56d55 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,26 @@ 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 */
> + start_info_size +=
> + HVMLOADER_MODULE_NAME_SIZE * HVMLOADER_MODULE_MAX_COUNT;
What about \0 ? Ah, the strncpy we use adds \0 byte. But it would be nice
to mention that somewhere. Perhaps mention:
The HVMLOADER_MODULE_NAME_SIZE accounts for NUL byte?
> + }
> +
> + 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;
> + }
> +
> + if ( dom->device_model )
> + {
> /*
> * Allocate and clear additional ioreq server pages. The default
> * server will use the IOREQ and BUFIOREQ special pages above.
> @@ -1696,39 +1707,68 @@ static int alloc_pgtables_hvm(struct xc_dom_image *dom)
> return 0;
> }
>
> +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;
> + 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((char*)(modlist + HVMLOADER_MODULE_MAX_COUNT)
> + + HVMLOADER_MODULE_NAME_SIZE * index,
> + name, HVMLOADER_MODULE_NAME_SIZE);
> + modlist[index].cmdline_paddr =
> + (dom->start_info_seg.pfn << PAGE_SHIFT) +
> + ((uintptr_t)modlist - (uintptr_t)start_info) +
> + sizeof(struct hvm_modlist_entry) * HVMLOADER_MODULE_MAX_COUNT +
> + HVMLOADER_MODULE_NAME_SIZE * index;
That looks right, but boy it takes a bit of thinking to
make sure it is right. Perhaps put a comment outlining where
it ought to be (so folks reading the first time can feel
OK they got it right?)
> +
> + 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;
> + void *start_page;
> + 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_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 = start_page;
> + start_info = start_page;
> + modlist = start_page + sizeof(*start_info) + dom->cmdline_size;
>
> + if ( !dom->device_model )
> + {
> if ( dom->cmdline )
> {
> char *cmdline = start_page + sizeof(*start_info);
> @@ -1740,22 +1780,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->bios_module, "bios",
> + 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_page, 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 64ba7ab..1cfec5c 100644
> --- a/xen/include/public/xen.h
> +++ b/xen/include/public/xen.h
> @@ -798,7 +798,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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH v4 03/14] configure: #define SEABIOS_PATH and OVMF_PATH
2016-03-14 17:55 ` [PATCH v4 03/14] configure: #define SEABIOS_PATH and OVMF_PATH Anthony PERARD
@ 2016-03-16 0:20 ` Konrad Rzeszutek Wilk
2016-04-08 13:38 ` Wei Liu
1 sibling, 0 replies; 73+ messages in thread
From: Konrad Rzeszutek Wilk @ 2016-03-16 0:20 UTC (permalink / raw)
To: Anthony PERARD; +Cc: Ian Jackson, Stefano Stabellini, Wei Liu, xen-devel
On Mon, Mar 14, 2016 at 05:55:38PM +0000, Anthony PERARD wrote:
> Those paths are to be used by libxl, in order to load the firmware in
> memory. If a system path is not define via --with-system-seabios or
s/define/defined/
> --with-system-ovmf, then this default to the Xen firmware directory.
s/this//
>
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>
> ---
> Please, run ./autogen.sh on this patch.
> ---
> tools/configure.ac | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/tools/configure.ac b/tools/configure.ac
> index 5b5dda4..7e5452e 100644
> --- a/tools/configure.ac
> +++ b/tools/configure.ac
> @@ -218,6 +218,9 @@ AC_ARG_WITH([system-seabios],
> esac
> ],[])
> AC_SUBST(seabios_path)
> +AC_DEFINE_UNQUOTED([SEABIOS_PATH],
> + ["${seabios_path:-$XENFIRMWAREDIR/seabios.bin}"],
> + [SeaBIOS path])
>
> AC_ARG_WITH([system-ovmf],
> AS_HELP_STRING([--with-system-ovmf@<:@=PATH@:>@],
> @@ -229,6 +232,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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH v4 04/14] firmware/makefile: install BIOS blob ...
2016-03-14 17:55 ` [PATCH v4 04/14] firmware/makefile: install BIOS blob Anthony PERARD
@ 2016-03-16 0:26 ` Konrad Rzeszutek Wilk
2016-03-16 8:54 ` Dario Faggioli
2016-03-17 16:58 ` Anthony PERARD
2016-03-17 17:37 ` Doug Goldstein
2016-04-18 14:31 ` Doug Goldstein
2 siblings, 2 replies; 73+ messages in thread
From: Konrad Rzeszutek Wilk @ 2016-03-16 0:26 UTC (permalink / raw)
To: Anthony PERARD; +Cc: Ian Jackson, Stefano Stabellini, Wei Liu, xen-devel
On Mon, Mar 14, 2016 at 05:55:39PM +0000, Anthony PERARD wrote:
> ... into the firmware directory, along with hvmloader.
>
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
> 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 | 13 +++++++++++++
> 1 file changed, 13 insertions(+)
>
> diff --git a/tools/firmware/Makefile b/tools/firmware/Makefile
> index 6cc86ce..6a37758 100644
> --- a/tools/firmware/Makefile
> +++ b/tools/firmware/Makefile
> @@ -19,6 +19,9 @@ SUBDIRS-y += hvmloader
>
> LD32BIT-$(CONFIG_FreeBSD) := LD32BIT_FLAG=-melf_i386_fbsd
>
> +SEABIOS_ROM := seabios-dir/out/bios.bin
> +OVMF_ROM := ovmf-dir/ovmf.bin
These will set the variables..
> +
> ovmf-dir:
> GIT=$(GIT) $(XEN_ROOT)/scripts/git-checkout.sh $(OVMF_UPSTREAM_URL) $(OVMF_UPSTREAM_REVISION) ovmf-dir
> cp ovmf-makefile ovmf-dir/Makefile;
> @@ -45,6 +48,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),)
But here you check them?
Or should the setting of OVMF_ROM and SEABIOS_ROM be ?=
> + $(INSTALL_DATA) $(SEABIOS_ROM) $(INST_DIR)/seabios.bin
> +endif
> +endif
> +ifeq ($(CONFIG_OVMF),y)
> +ifeq ($(OVMF_PATH),)
> + $(INSTALL_DATA) $(OVMF_ROM) $(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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH v4 05/14] libxl: Load guest BIOS from file
2016-03-14 17:55 ` [PATCH v4 05/14] libxl: Load guest BIOS from file Anthony PERARD
@ 2016-03-16 0:53 ` Konrad Rzeszutek Wilk
2016-03-16 9:27 ` Dario Faggioli
0 siblings, 1 reply; 73+ messages in thread
From: Konrad Rzeszutek Wilk @ 2016-03-16 0:53 UTC (permalink / raw)
To: Anthony PERARD; +Cc: Stefano Stabellini, Wei Liu, xen-devel
On Mon, Mar 14, 2016 at 05:55:40PM +0000, Anthony PERARD wrote:
> The path to the BIOS blob can be override by the xl's bios_override option,
s/override/overriden/
> 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>
>
> ---
> Change in V4:
> - updating man page to have bios_override described.
> - return ERROR_INVAL in libxl__load_hvm_firmware_module when the file is
> empty.
>
> Change 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 | 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 b/docs/man/xl.cfg.pod.5
> index 56b1117..165915b 100644
> --- a/docs/man/xl.cfg.pod.5
> +++ b/docs/man/xl.cfg.pod.5
> @@ -1121,6 +1121,15 @@ Requires device_model_version=qemu-xen.
>
> =back
>
> +=item B<bios_override="PATH">
Perhaps 'bios_override_path' ?
> +
> +Override the path to the blob to be used as BIOS. The blob provided here MUST
Perhaps:
Override the path to search for the B<bios=>?
Or is this the full path including the name?? In which case should it
mention that B<bios=> is overriden?
> +be consistent with the `bios` which you have specified. You should not normally
> +need to specify this option.
> +
> +This options does not have any effect if using bios="rombios" or
B<bios="rombios"> ?
> +device_model_version="qemu-xen-traditional".
And here too?
> +
> =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 f9e3ef5..d06c6c5 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -884,6 +884,14 @@ void libxl_mac_copy(libxl_ctx *ctx, libxl_mac *dst, libxl_mac *src);
> #define LIBXL_HAVE_CHECKPOINTED_STREAM 1
>
> /*
> + * LIBXL_HAVE_BUILDINFO_HVM_BIOS_FIRMWARE
> + *
> + * libxl_domain_build_info has u.hvm.bios_firmware field which can be use
> + * to provide a different bios blob (like SeaBIOS or OVMF).
> + */
> +#define LIBXL_HAVE_BUILDINFO_HVM_BIOS_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 b825b98..c112cc5 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 e;
That is interesting.
The CODING_STYLE in tools/libxl says:
36 int rc; /* a libxl error code - and not anything else */
37 int r; /* the return value from a system call (or libxc call) */
38 bool ok; /* the success return value from a boolean function */
And libxl_read_file_contents is quite weird. It does return an normal
errno value, so .. one could say it should be 'r'? But the existing users
of this are either 'e','ret' or 'rc'. 'rc' is not good, and 'ret'
is 'rc' is clearly wrong.
How confusing.
Ian, Wei, maybe you could clarify please?
> +
> + LOG(DEBUG, "Loading %s: %s", what, filename);
> + e = libxl_read_file_contents(CTX, filename, &data, &datalen);
> + if (e) {
> + /*
> + * Print a message only on ENOENT, other error are logged by the
s/error/errors/
> + * function libxl_read_file_contents().
> + */
> + if (e == ENOENT)
> + LOGEV(ERROR, e, "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.bios_firmware) {
> + bios_filename = info->u.hvm.bios_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->bios_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 005fe53..af3ba9a 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -2265,6 +2265,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 632c009..3978fd9 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -493,6 +493,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
> ("timer_mode", libxl_timer_mode),
> ("nested_hvm", libxl_defbool),
> ("altp2m", libxl_defbool),
> + ("bios_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 990d3c9..08ceede 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -1505,12 +1505,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_override",
> + &b_info->u.hvm.bios_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.bios_firmware)
> + fprintf(stderr, "WARNING: "
> + "bios_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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH v4 06/14] xen: Move the hvm_start_info C representation from libxc to public/xen.h
2016-03-15 8:09 ` Jan Beulich
@ 2016-03-16 0:59 ` Konrad Rzeszutek Wilk
2016-03-16 1:00 ` Konrad Rzeszutek Wilk
2016-03-16 8:32 ` Jan Beulich
2016-03-21 17:04 ` Roger Pau Monné
1 sibling, 2 replies; 73+ messages in thread
From: Konrad Rzeszutek Wilk @ 2016-03-16 0:59 UTC (permalink / raw)
To: Jan Beulich
Cc: Wei Liu, Stefano Stabellini, Ian Jackson, xen-devel,
Anthony PERARD, Roger Pau Monne
On Tue, Mar 15, 2016 at 02:09:46AM -0600, Jan Beulich wrote:
> >>> On 14.03.16 at 18:55, <anthony.perard@citrix.com> wrote:
> > --- a/xen/include/public/xen.h
> > +++ b/xen/include/public/xen.h
> > @@ -841,6 +841,37 @@ 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.
> > + *
> > + * 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 cmdline_paddr; /* Physical address of the command line. */
> > + uint32_t nr_modules; /* Number of modules passed to the kernel. */
> > + uint32_t modlist_paddr; /* Physical address of an array of */
> > + /* hvm_modlist_entry. */
> > + uint32_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 */
>
> This needs extra care: Either the packed attributes need to be
> dropped (Roger - why did they get put there in the first place?),
Presumarily b/c on 32-bit and 64-bit the size of the structure would
be different otherwise...
> or their use needs to be shielded from compilers not understanding
> them.
.. which begs the question - How come we didn't make the structure 64-bit nice?
As in add another uint32_t to it? And then we can ditch the packed?
>
> Jan
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH v4 06/14] xen: Move the hvm_start_info C representation from libxc to public/xen.h
2016-03-16 0:59 ` Konrad Rzeszutek Wilk
@ 2016-03-16 1:00 ` Konrad Rzeszutek Wilk
2016-03-16 8:32 ` Jan Beulich
1 sibling, 0 replies; 73+ messages in thread
From: Konrad Rzeszutek Wilk @ 2016-03-16 1:00 UTC (permalink / raw)
To: Jan Beulich, boris.ostrovsky
Cc: Wei Liu, Stefano Stabellini, Ian Jackson, xen-devel,
Anthony PERARD, Roger Pau Monne
On Tue, Mar 15, 2016 at 08:59:54PM -0400, Konrad Rzeszutek Wilk wrote:
> On Tue, Mar 15, 2016 at 02:09:46AM -0600, Jan Beulich wrote:
Adding Boris to the To:
> > >>> On 14.03.16 at 18:55, <anthony.perard@citrix.com> wrote:
> > > --- a/xen/include/public/xen.h
> > > +++ b/xen/include/public/xen.h
> > > @@ -841,6 +841,37 @@ 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.
> > > + *
> > > + * 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 cmdline_paddr; /* Physical address of the command line. */
> > > + uint32_t nr_modules; /* Number of modules passed to the kernel. */
> > > + uint32_t modlist_paddr; /* Physical address of an array of */
> > > + /* hvm_modlist_entry. */
> > > + uint32_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 */
> >
> > This needs extra care: Either the packed attributes need to be
> > dropped (Roger - why did they get put there in the first place?),
>
> Presumarily b/c on 32-bit and 64-bit the size of the structure would
> be different otherwise...
>
> > or their use needs to be shielded from compilers not understanding
> > them.
>
> .. which begs the question - How come we didn't make the structure 64-bit nice?
> As in add another uint32_t to it? And then we can ditch the packed?
>
>
>
> >
> > Jan
> >
> >
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xen.org
> > http://lists.xen.org/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH v4 07/14] hvmloader: Grab the hvm_start_info pointer
2016-03-14 17:55 ` [PATCH v4 07/14] hvmloader: Grab the hvm_start_info pointer Anthony PERARD
@ 2016-03-16 1:07 ` Konrad Rzeszutek Wilk
2016-04-05 12:43 ` Jan Beulich
1 sibling, 0 replies; 73+ messages in thread
From: Konrad Rzeszutek Wilk @ 2016-03-16 1:07 UTC (permalink / raw)
To: Anthony PERARD
Cc: Keir Fraser, Stefano Stabellini, Andrew Cooper, Ian Jackson,
xen-devel, Jan Beulich, Wei Liu
On Mon, Mar 14, 2016 at 05:55:42PM +0000, Anthony PERARD wrote:
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
>
Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH v4 08/14] hvmloader: Locate the BIOS blob
2016-03-14 17:55 ` [PATCH v4 08/14] hvmloader: Locate the BIOS blob Anthony PERARD
@ 2016-03-16 1:14 ` Konrad Rzeszutek Wilk
2016-03-17 17:46 ` Anthony PERARD
2016-04-05 12:59 ` Jan Beulich
1 sibling, 1 reply; 73+ messages in thread
From: Konrad Rzeszutek Wilk @ 2016-03-16 1:14 UTC (permalink / raw)
To: Anthony PERARD
Cc: Keir Fraser, Stefano Stabellini, Andrew Cooper, Ian Jackson,
xen-devel, Jan Beulich, Wei Liu
On Mon, Mar 14, 2016 at 05:55:43PM +0000, Anthony PERARD wrote:
> The BIOS can be found an entry called "bios" of the modlist of the
s/BIOS/BIOS blob/
> hvm_start_info struct.
>
> The found BIOS blob is not loaded by this patch, but only passed as
> argument to bios_load() function. It is going to be used by the next few
> patches.
Please list the 'few patches' by name as this patch and others may not
always be committed in order.
And it also helps in future to figure out what those other ones are
when debugging or backporting.
See below.
>
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
>
> ---
> 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 | 42 ++++++++++++++++++++++++++++++++++--
> tools/firmware/hvmloader/ovmf.c | 3 ++-
> tools/firmware/hvmloader/rombios.c | 3 ++-
> tools/firmware/hvmloader/util.h | 2 ++
> 5 files changed, 47 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..460efb9 100644
> --- a/tools/firmware/hvmloader/hvmloader.c
> +++ b/tools/firmware/hvmloader/hvmloader.c
> @@ -253,10 +253,40 @@ 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 *)info->modlist_paddr;
> + unsigned int i;
> +
> + if ( !modlist )
> + return NULL;
> +
> + for ( i = 0; i < info->nr_modules; i++ )
> + {
> + uint32_t module_name = modlist[i].cmdline_paddr;
> +
> + BUG_ON(!modlist[i].cmdline_paddr ||
Having an zero value in cmdline_paddr is OK.
The spec says:
803 * NOTE: nothing will be loaded at physical address 0, so a 0 value in any
804 * of the address fields should be treated as not present.
805 *
> + modlist[i].cmdline_paddr > UINT_MAX);
> +
> + if ( !strcmp(name, (char*)module_name) )
Which could result in looking up a value at 0 address.
Perhaps change your code to:
if ( !modlist[i].cmdline_paddr )
continue;
?
> + {
> + BUG_ON(!modlist[i].paddr || modlist[i].paddr > UINT_MAX ||
> + modlist[i].size > UINT_MAX);
To be fair all of those values are in spec..Perhaps you should mention
that the libxc code would never put those there and neermind the spec?
> + 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 +322,16 @@ 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, "bios");
> + 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, 0, 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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH v4 09/14] hvmloader: Check modules whereabouts in perform_tests
2016-03-14 17:55 ` [PATCH v4 09/14] hvmloader: Check modules whereabouts in perform_tests Anthony PERARD
@ 2016-03-16 1:23 ` Konrad Rzeszutek Wilk
2016-03-17 18:08 ` Anthony PERARD
2016-04-05 13:07 ` Jan Beulich
1 sibling, 1 reply; 73+ messages in thread
From: Konrad Rzeszutek Wilk @ 2016-03-16 1:23 UTC (permalink / raw)
To: Anthony PERARD
Cc: Keir Fraser, Stefano Stabellini, Andrew Cooper, Ian Jackson,
xen-devel, Jan Beulich, Wei Liu
On Mon, Mar 14, 2016 at 05:55:44PM +0000, Anthony PERARD wrote:
> As perform_tests() is going to clear memory past 4MB, we check that the
> memory can be use or we skip the tests.
I get the reason you want this - but if we have giant binary blobs
and something else screws up what the tests are testing for- we
don't have a regression test anymore.
.. Oh my looking at rep_io_test and trying to make it more dynamic
is tricky but should be managable?
I presume that is the test that blows everything out of the sky?
And also the placement of the page tables?
>
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
> 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 | 20 ++++++++++++++++++++
> 1 file changed, 20 insertions(+)
>
> diff --git a/tools/firmware/hvmloader/tests.c b/tools/firmware/hvmloader/tests.c
> index fea3ad3..7996206 100644
> --- a/tools/firmware/hvmloader/tests.c
> +++ b/tools/firmware/hvmloader/tests.c
> @@ -210,6 +210,26 @@ void perform_tests(void)
> return;
> }
>
> + /* Check that tests does not use memory where modules are stored */
> + if ( ((uint32_t)hvm_start_info + sizeof(struct hvm_start_info)) > 4 << 20
> + && (uint32_t)hvm_start_info < 8 << 20 )
> + {
> + printf("Skipping tests due to memory used by hvm_start_info\n");
> + return;
> + }
> + for ( unsigned i = 0; i < hvm_start_info->nr_modules; i++ )
> + {
> + const struct hvm_modlist_entry *modlist =
> + (struct hvm_modlist_entry *)hvm_start_info->modlist_paddr;
> + if ( modlist[i].paddr
> + && modlist[i].paddr + modlist[i].size > 4ul << 20
> + && modlist[i].paddr < 8ul << 20 )
> + {
> + printf("Skipping tests due to memory used by a module\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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH v4 10/14] hvmloader: Load SeaBIOS from hvm_start_info modules
2016-03-14 17:55 ` [PATCH v4 10/14] hvmloader: Load SeaBIOS from hvm_start_info modules Anthony PERARD
@ 2016-03-16 1:27 ` Konrad Rzeszutek Wilk
2016-03-16 1:27 ` Konrad Rzeszutek Wilk
2016-04-05 13:11 ` Jan Beulich
1 sibling, 1 reply; 73+ messages in thread
From: Konrad Rzeszutek Wilk @ 2016-03-16 1:27 UTC (permalink / raw)
To: Anthony PERARD
Cc: Keir Fraser, Stefano Stabellini, Andrew Cooper, Ian Jackson,
xen-devel, Jan Beulich, Wei Liu
> diff --git a/tools/firmware/hvmloader/seabios.c b/tools/firmware/hvmloader/seabios.c
> index c6b3d9f..5d0a491 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,29 @@ static void seabios_setup_e820(void)
> struct e820entry *e820 = scratch_alloc(sizeof(struct e820entry)*16, 0);
> info->e820 = (uint32_t)e820;
>
> + BUG_ON(seabios_config.bios_address < 0xc0000 || seabios_config.bios_address >= 0x100000);
s/0x100000/HVMLOADER_PHYSICAL_ADDRESS/
s/0xc0000/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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH v4 10/14] hvmloader: Load SeaBIOS from hvm_start_info modules
2016-03-16 1:27 ` Konrad Rzeszutek Wilk
@ 2016-03-16 1:27 ` Konrad Rzeszutek Wilk
0 siblings, 0 replies; 73+ messages in thread
From: Konrad Rzeszutek Wilk @ 2016-03-16 1:27 UTC (permalink / raw)
To: Anthony PERARD
Cc: Keir Fraser, Stefano Stabellini, Andrew Cooper, Ian Jackson,
xen-devel, Jan Beulich, Wei Liu
On Tue, Mar 15, 2016 at 09:27:19PM -0400, Konrad Rzeszutek Wilk wrote:
> > diff --git a/tools/firmware/hvmloader/seabios.c b/tools/firmware/hvmloader/seabios.c
> > index c6b3d9f..5d0a491 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,29 @@ static void seabios_setup_e820(void)
> > struct e820entry *e820 = scratch_alloc(sizeof(struct e820entry)*16, 0);
> > info->e820 = (uint32_t)e820;
> >
> > + BUG_ON(seabios_config.bios_address < 0xc0000 || seabios_config.bios_address >= 0x100000);
>
> s/0x100000/HVMLOADER_PHYSICAL_ADDRESS/
> s/0xc0000/0x000C0000/
That was suppose to be VGABIOS_PHYSICAL_ADDRESS
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH v4 11/14] hvmloader: Load OVMF from modules
2016-03-14 17:55 ` [PATCH v4 11/14] hvmloader: Load OVMF from modules Anthony PERARD
@ 2016-03-16 1:36 ` Konrad Rzeszutek Wilk
2016-04-05 13:16 ` Jan Beulich
1 sibling, 0 replies; 73+ messages in thread
From: Konrad Rzeszutek Wilk @ 2016-03-16 1:36 UTC (permalink / raw)
To: Anthony PERARD
Cc: Keir Fraser, Stefano Stabellini, Andrew Cooper, Ian Jackson,
xen-devel, Jan Beulich, Wei Liu
On Mon, Mar 14, 2016 at 05:55:46PM +0000, Anthony PERARD wrote:
> ... and do not include the OVMF ROM into hvmloader anymore.
>
Yeey..
> diff --git a/tools/firmware/hvmloader/ovmf.c b/tools/firmware/hvmloader/ovmf.c
> index 858a2d4..6607e57 100644
> --- a/tools/firmware/hvmloader/ovmf.c
> +++ b/tools/firmware/hvmloader/ovmf.c
> @@ -34,17 +34,10 @@
> #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 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 +90,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 = 0x100000000ULL
Perhaps still leave an macro for 0x100000000ULL?
> + - ((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. */
s/overlaps/overlap/
> + 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 +151,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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH v4 12/14] hvmloader: Specific bios_load function required
2016-03-14 17:55 ` [PATCH v4 12/14] hvmloader: Specific bios_load function required Anthony PERARD
@ 2016-03-16 1:38 ` Konrad Rzeszutek Wilk
2016-03-17 18:25 ` Anthony PERARD
0 siblings, 1 reply; 73+ messages in thread
From: Konrad Rzeszutek Wilk @ 2016-03-16 1:38 UTC (permalink / raw)
To: Anthony PERARD
Cc: Keir Fraser, Stefano Stabellini, Andrew Cooper, Ian Jackson,
xen-devel, Jan Beulich, Wei Liu
On Mon, Mar 14, 2016 at 05:55:47PM +0000, Anthony PERARD wrote:
The title says:
" Specific bios_load function required "
But I am not sure what you mean?
> All BIOS but ROMBIOS needs to be loaded via modules.
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>
> ---
> 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 460efb9..bb2a309 100644
> --- a/tools/firmware/hvmloader/hvmloader.c
> +++ b/tools/firmware/hvmloader/hvmloader.c
> @@ -323,21 +323,25 @@ int main(void)
>
> printf("Loading %s ...\n", bios->name);
> bios_module = get_module_entry(hvm_start_info, "bios");
> - 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, 0, 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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH v4 14/14] configure: do not depend on SEABIOS_PATH or OVMF_PATH ...
2016-03-14 17:55 ` [PATCH v4 14/14] configure: do not depend on SEABIOS_PATH or OVMF_PATH Anthony PERARD
@ 2016-03-16 1:40 ` Konrad Rzeszutek Wilk
2016-04-08 13:38 ` Wei Liu
1 sibling, 0 replies; 73+ messages in thread
From: Konrad Rzeszutek Wilk @ 2016-03-16 1:40 UTC (permalink / raw)
To: Anthony PERARD; +Cc: Ian Jackson, Stefano Stabellini, Wei Liu, xen-devel
On Mon, Mar 14, 2016 at 05:55:49PM +0000, Anthony PERARD wrote:
> ... to compile SeaBIOS and OVMF. Only depends on CONFIG_*.
s/depends/depend/
>
> 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>
>
> ---
> Change in V4:
> - change subject prefix
>
> Please, run ./autogen.sh on this patch.
> ---
> 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 7e5452e..ef306ae 100644
> --- a/tools/configure.ac
> +++ b/tools/configure.ac
> @@ -212,12 +212,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/seabios.bin}"],
> [SeaBIOS path])
> @@ -226,12 +227,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 6a37758..4975cb4 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
> @@ -49,15 +45,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_ROM) $(INST_DIR)/seabios.bin
> endif
> -endif
> ifeq ($(CONFIG_OVMF),y)
> -ifeq ($(OVMF_PATH),)
> $(INSTALL_DATA) $(OVMF_ROM) $(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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH v4 06/14] xen: Move the hvm_start_info C representation from libxc to public/xen.h
2016-03-16 0:59 ` Konrad Rzeszutek Wilk
2016-03-16 1:00 ` Konrad Rzeszutek Wilk
@ 2016-03-16 8:32 ` Jan Beulich
1 sibling, 0 replies; 73+ messages in thread
From: Jan Beulich @ 2016-03-16 8:32 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk
Cc: Wei Liu, Stefano Stabellini, Ian Jackson, xen-devel,
Anthony PERARD, Roger Pau Monne
>>> On 16.03.16 at 01:59, <konrad.wilk@oracle.com> wrote:
> On Tue, Mar 15, 2016 at 02:09:46AM -0600, Jan Beulich wrote:
>> >>> On 14.03.16 at 18:55, <anthony.perard@citrix.com> wrote:
>> > --- a/xen/include/public/xen.h
>> > +++ b/xen/include/public/xen.h
>> > @@ -841,6 +841,37 @@ 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.
>> > + *
>> > + * 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 cmdline_paddr; /* Physical address of the command line. */
>> > + uint32_t nr_modules; /* Number of modules passed to the kernel. */
>> > + uint32_t modlist_paddr; /* Physical address of an array of */
>> > + /* hvm_modlist_entry. */
>> > + uint32_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 */
>>
>> This needs extra care: Either the packed attributes need to be
>> dropped (Roger - why did they get put there in the first place?),
>
> Presumarily b/c on 32-bit and 64-bit the size of the structure would
> be different otherwise...
And that difference in size would result from what?
>> or their use needs to be shielded from compilers not understanding
>> them.
>
> .. which begs the question - How come we didn't make the structure 64-bit
> nice?
> As in add another uint32_t to it? And then we can ditch the packed?
Certainly not the odd number of uint32_t-s in there. If there was
a mix of uint32_t and uint64_t, I could certainly see any issue...
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH v4 04/14] firmware/makefile: install BIOS blob ...
2016-03-16 0:26 ` Konrad Rzeszutek Wilk
@ 2016-03-16 8:54 ` Dario Faggioli
2016-03-16 8:56 ` Konrad Rzeszutek Wilk
2016-03-17 16:58 ` Anthony PERARD
1 sibling, 1 reply; 73+ messages in thread
From: Dario Faggioli @ 2016-03-16 8:54 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk, Anthony PERARD
Cc: Wei Liu, xen-devel, Ian Jackson, Stefano Stabellini
[-- Attachment #1.1: Type: text/plain, Size: 1572 bytes --]
On Tue, 2016-03-15 at 20:26 -0400, Konrad Rzeszutek Wilk wrote:
> On Mon, Mar 14, 2016 at 05:55:39PM +0000, Anthony PERARD wrote:
> >
> > diff --git a/tools/firmware/Makefile b/tools/firmware/Makefile
> > index 6cc86ce..6a37758 100644
> > --- a/tools/firmware/Makefile
> > +++ b/tools/firmware/Makefile
> > @@ -19,6 +19,9 @@ SUBDIRS-y += hvmloader
> >
> > LD32BIT-$(CONFIG_FreeBSD) := LD32BIT_FLAG=-melf_i386_fbsd
> >
> > +SEABIOS_ROM := seabios-dir/out/bios.bin
> > +OVMF_ROM := ovmf-dir/ovmf.bin
> These will set the variables..
> >
> > +
> > ovmf-dir:
> > GIT=$(GIT) $(XEN_ROOT)/scripts/git-checkout.sh
> > $(OVMF_UPSTREAM_URL) $(OVMF_UPSTREAM_REVISION) ovmf-dir
> > cp ovmf-makefile ovmf-dir/Makefile;
> > @@ -45,6 +48,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),)
> But here you check them?
>
> Or should the setting of OVMF_ROM and SEABIOS_ROM be ?=
>
I don't speak too much Makefile, TBH (so apologies if I'm talking
nonsense), but it looks to me that what is set above and what is
checked here are indeed _not_the_same_ variables.. isn't that the case?
Regards,
Dario
--
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)
[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
[-- Attachment #2: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH v4 04/14] firmware/makefile: install BIOS blob ...
2016-03-16 8:54 ` Dario Faggioli
@ 2016-03-16 8:56 ` Konrad Rzeszutek Wilk
0 siblings, 0 replies; 73+ messages in thread
From: Konrad Rzeszutek Wilk @ 2016-03-16 8:56 UTC (permalink / raw)
To: Dario Faggioli
Cc: Anthony PERARD, Wei Liu, xen-devel, Ian Jackson, Stefano Stabellini
On Wed, Mar 16, 2016 at 09:54:44AM +0100, Dario Faggioli wrote:
> On Tue, 2016-03-15 at 20:26 -0400, Konrad Rzeszutek Wilk wrote:
> > On Mon, Mar 14, 2016 at 05:55:39PM +0000, Anthony PERARD wrote:
> > >
> > > diff --git a/tools/firmware/Makefile b/tools/firmware/Makefile
> > > index 6cc86ce..6a37758 100644
> > > --- a/tools/firmware/Makefile
> > > +++ b/tools/firmware/Makefile
> > > @@ -19,6 +19,9 @@ SUBDIRS-y += hvmloader
> > >
> > > LD32BIT-$(CONFIG_FreeBSD) := LD32BIT_FLAG=-melf_i386_fbsd
> > >
> > > +SEABIOS_ROM := seabios-dir/out/bios.bin
> > > +OVMF_ROM := ovmf-dir/ovmf.bin
> > These will set the variables..
> > >
> > > +
> > > ovmf-dir:
> > > GIT=$(GIT) $(XEN_ROOT)/scripts/git-checkout.sh
> > > $(OVMF_UPSTREAM_URL) $(OVMF_UPSTREAM_REVISION) ovmf-dir
> > > cp ovmf-makefile ovmf-dir/Makefile;
> > > @@ -45,6 +48,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),)
> > But here you check them?
> >
> > Or should the setting of OVMF_ROM and SEABIOS_ROM be ?=
> >
> I don't speak too much Makefile, TBH (so apologies if I'm talking
> nonsense), but it looks to me that what is set above and what is
> checked here are indeed _not_the_same_ variables.. isn't that the case?
<facepalm>
You are right.
Ignore my comment.
Thanks!
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH v4 05/14] libxl: Load guest BIOS from file
2016-03-16 0:53 ` Konrad Rzeszutek Wilk
@ 2016-03-16 9:27 ` Dario Faggioli
2016-03-17 17:24 ` Anthony PERARD
0 siblings, 1 reply; 73+ messages in thread
From: Dario Faggioli @ 2016-03-16 9:27 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk, Anthony PERARD
Cc: xen-devel, Wei Liu, Stefano Stabellini
[-- Attachment #1.1: Type: text/plain, Size: 3068 bytes --]
On Tue, 2016-03-15 at 20:53 -0400, Konrad Rzeszutek Wilk wrote:
> On Mon, Mar 14, 2016 at 05:55:40PM +0000, Anthony PERARD wrote:
> >
> > --- a/docs/man/xl.cfg.pod.5
> > +++ b/docs/man/xl.cfg.pod.5
> > @@ -1121,6 +1121,15 @@ Requires device_model_version=qemu-xen.
> >
> > =back
> >
> > +=item B<bios_override="PATH">
> Perhaps 'bios_override_path' ?
>
Or 'bios_path_override' ?
Wow, a French, an Italian and a Polish arguing about best looking
English-ish words... This must be a (bad) joke! :-P :-P
> > +
> > +Override the path to the blob to be used as BIOS. The blob
> > provided here MUST
> Perhaps:
>
> Override the path to search for the B<bios=>?
>
AFAIUI, B<bios=> does not contain an exact file name, but the
indication of what kind of BIOS we want. Therefore, referencing it
directly like this may be confusing (IMHO).
So, maybe something like: "Override the path at which to look for the
BIOS binary. Such binary MUST be consistent with what has been
specified in B<bios=>."
> Or is this the full path including the name?? In which case should it
> mention that B<bios=> is overriden?
>
If that is the case, indeed.
> > --- 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 e;
> That is interesting.
>
> The CODING_STYLE in tools/libxl says:
>
> 36 int rc; /* a libxl error code - and not anything else
> */
> 37 int r; /* the return value from a system call (or libxc
> call) */
> 38 bool ok; /* the success return value from a boolean function
> */
>
> And libxl_read_file_contents is quite weird. It does return an normal
> errno value, so .. one could say it should be 'r'? But the existing
> users
> of this are either 'e','ret' or 'rc'. 'rc' is not good, and 'ret'
> is 'rc' is clearly wrong.
>
> How confusing.
>
> Ian, Wei, maybe you could clarify please?
>
Hehe... I'd also go for 'r', and call the (or at least some of the)
existing users not codying style compliant, but it's indeed a funny
case, and we should hear from maintainers what they prefer. :-)
Regards,
Dario
--
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)
[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
[-- Attachment #2: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH v4 02/14] libxc: Prepare a start info structure for hvmloader
2016-03-16 0:18 ` Konrad Rzeszutek Wilk
@ 2016-03-16 18:01 ` Boris Ostrovsky
2016-03-17 16:48 ` Anthony PERARD
2016-03-17 16:28 ` Anthony PERARD
1 sibling, 1 reply; 73+ messages in thread
From: Boris Ostrovsky @ 2016-03-16 18:01 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk, Anthony PERARD, roger.pau
Cc: Ian Jackson, Stefano Stabellini, Wei Liu, xen-devel
On 03/15/2016 08:18 PM, Konrad Rzeszutek Wilk wrote:
> On Mon, Mar 14, 2016 at 05:55:37PM +0000, Anthony PERARD wrote:
>
>> @@ -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,26 @@ 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 */
>> + start_info_size +=
>> + HVMLOADER_MODULE_NAME_SIZE * HVMLOADER_MODULE_MAX_COUNT;
> What about \0 ? Ah, the strncpy we use adds \0 byte. But it would be nice
> to mention that somewhere. Perhaps mention:
>
> The HVMLOADER_MODULE_NAME_SIZE accounts for NUL byte?
>
>> + }
>> +
>> + 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;
>> + }
>> +
>> + if ( dom->device_model )
>> + {
Can you fold this into the 'else' clause above and move
xc_dom_alloc_segment() down?
>> +
>> 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;
>> + void *start_page;
>> + 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_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 = start_page;
>> + start_info = start_page;
>> + modlist = start_page + sizeof(*start_info) + dom->cmdline_size;
I think we can drop start_page and use start_info only. They are the
same, aren't they?
-boris
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH v4 01/14] libxc: Rework extra module initialisation
2016-03-16 0:06 ` Konrad Rzeszutek Wilk
@ 2016-03-17 16:24 ` Anthony PERARD
0 siblings, 0 replies; 73+ messages in thread
From: Anthony PERARD @ 2016-03-17 16:24 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk; +Cc: Ian Jackson, Stefano Stabellini, Wei Liu, xen-devel
On Tue, Mar 15, 2016 at 08:06:44PM -0400, Konrad Rzeszutek Wilk wrote:
> On Mon, Mar 14, 2016 at 05:55:36PM +0000, Anthony PERARD wrote:
> > This patch use xc_dom_alloc_segment() to allocate the memory space for the
> > ACPI modules and the SMBIOS modules. This is to replace the arbitrary
> > placement of 1MB after the hvmloader image.
>
> s/of/at/ ?
>
> Or are you rather staying that the memory for ACPI modules and such
> is offset by one 1MB after hvmloader image?
I was trying to translate this in to english:
*mstart_out = MKALIGN(elf->pend, MB_ALIGN) + (MB_ALIGN);
which came with that comment:
/* Want to place the modules 1Mb+change behind the loader image. */
[...]
Actually, the current situation before this patch would be, with every bios
compiled in, and with the use of acpi_firmware:
/-----------\ <- 0MB
| other |
| stuff |
+-----------+ <- 1MB
| hvmloader |
| binary |
+-----------| <- 3.8MB
| empty |
+-----------+ <- 4MB <- begining of memory used by perform_tests()
| ... |
+-----------+ <- 5MB
| ACPI DATA |
+-----------+ <- 5.4MB
| ... |
\-----------/ <- 8MB <- end of memory used by perform_tests()
I just tried to to use the options acpi_firmware, and that the result.
When hvmloader tried to load the extra acpi data, I have this:
Loading SeaBIOS ...
Creating MP tables ...
Loading ACPI ...
*** HVMLoader bug at util.c:460
*** HVMLoader crashed.
That the BUG_ON in mem_alloc(). I think it's trying to allocate memory of
size 0.
> > In later patches, while trying to load a firmware such as OVMF, the later
>
> .. what later patches? These ones? You may want to mention which ones.
Ok, I will. That would be "hvmloader: Load OVMF from modules" and previous
ones of the patch series.
> > could easily be loaded past the address 4MB (OVMF is a 2MB binary), but
>
> Why would it be loaded past 4MB? Why not in the space right after ACPI
> DSDT and such?
> > hvmloader use a range of memory from 4MB to 8MB to perform tests and in the
> > process, clear the memory, before loading the modules.
>
> s/clear/clears/
>
> >
> > Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> > ---
> > Changes in V4:
> > - check that guest_addr_out have a reasonable value.
> >
> > New patch in V3.
> > ---
> > tools/libxc/xc_dom_hvmloader.c | 131 ++++++++++++-----------------------------
> > 1 file changed, 38 insertions(+), 93 deletions(-)
> >
> > diff --git a/tools/libxc/xc_dom_hvmloader.c b/tools/libxc/xc_dom_hvmloader.c
> > index 330d5e8..7cf5854 100644
> > --- a/tools/libxc/xc_dom_hvmloader.c
> > +++ b/tools/libxc/xc_dom_hvmloader.c
[...]
> >
> > - error_out:
> > - free(entries);
> > + rc = module_init_one(dom, &dom->acpi_module, "acpi module");
> > + if ( rc ) goto err;
> > + rc = module_init_one(dom, &dom->smbios_module, "smbios module");
> > + if ( rc ) goto err;
>
> You can't be that code thought. That is some nice deletion of code.
>
> Now is this safe for migration?
> I presume so since this is all E820_RESERVED righT? No funny business
> there?
The acpi module is moved to another location by hvmloader. I guess that the
same for the smbios module. So I think it's safe for migration, and it
does not change anything once hvmloader has finished.
--
Anthony PERARD
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH v4 02/14] libxc: Prepare a start info structure for hvmloader
2016-03-16 0:18 ` Konrad Rzeszutek Wilk
2016-03-16 18:01 ` Boris Ostrovsky
@ 2016-03-17 16:28 ` Anthony PERARD
1 sibling, 0 replies; 73+ messages in thread
From: Anthony PERARD @ 2016-03-17 16:28 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk
Cc: Wei Liu, Stefano Stabellini, Ian Jackson, xen-devel,
boris.ostrovsky, roger.pau
> > diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
> > index bdec40a..9c56d55 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,26 @@ 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 */
> > + start_info_size +=
> > + HVMLOADER_MODULE_NAME_SIZE * HVMLOADER_MODULE_MAX_COUNT;
>
> What about \0 ? Ah, the strncpy we use adds \0 byte. But it would be nice
> to mention that somewhere. Perhaps mention:
>
> The HVMLOADER_MODULE_NAME_SIZE accounts for NUL byte?
Yes, I can add a comment about it.
> > + }
> > +
> > + 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;
> > + }
> > +
> > + if ( dom->device_model )
> > + {
> > /*
> > * Allocate and clear additional ioreq server pages. The default
> > * server will use the IOREQ and BUFIOREQ special pages above.
> > @@ -1696,39 +1707,68 @@ static int alloc_pgtables_hvm(struct xc_dom_image *dom)
> > return 0;
> > }
> >
> > +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;
> > + 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((char*)(modlist + HVMLOADER_MODULE_MAX_COUNT)
> > + + HVMLOADER_MODULE_NAME_SIZE * index,
> > + name, HVMLOADER_MODULE_NAME_SIZE);
> > + modlist[index].cmdline_paddr =
> > + (dom->start_info_seg.pfn << PAGE_SHIFT) +
> > + ((uintptr_t)modlist - (uintptr_t)start_info) +
> > + sizeof(struct hvm_modlist_entry) * HVMLOADER_MODULE_MAX_COUNT +
> > + HVMLOADER_MODULE_NAME_SIZE * index;
>
> That looks right, but boy it takes a bit of thinking to
> make sure it is right. Perhaps put a comment outlining where
> it ought to be (so folks reading the first time can feel
> OK they got it right?)
Yes, I can add comments, and maybe try to simplify it a bit.
--
Anthony PERARD
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH v4 02/14] libxc: Prepare a start info structure for hvmloader
2016-03-16 18:01 ` Boris Ostrovsky
@ 2016-03-17 16:48 ` Anthony PERARD
0 siblings, 0 replies; 73+ messages in thread
From: Anthony PERARD @ 2016-03-17 16:48 UTC (permalink / raw)
To: Boris Ostrovsky
Cc: Wei Liu, Stefano Stabellini, Ian Jackson, xen-devel, roger.pau
On Wed, Mar 16, 2016 at 02:01:38PM -0400, Boris Ostrovsky wrote:
> On 03/15/2016 08:18 PM, Konrad Rzeszutek Wilk wrote:
> >On Mon, Mar 14, 2016 at 05:55:37PM +0000, Anthony PERARD wrote:
> >
>
>
> >>@@ -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,26 @@ 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 */
> >>+ start_info_size +=
> >>+ HVMLOADER_MODULE_NAME_SIZE * HVMLOADER_MODULE_MAX_COUNT;
> >What about \0 ? Ah, the strncpy we use adds \0 byte. But it would be nice
> >to mention that somewhere. Perhaps mention:
> >
> >The HVMLOADER_MODULE_NAME_SIZE accounts for NUL byte?
> >
> >>+ }
> >>+
> >>+ 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;
> >>+ }
> >>+
> >>+ if ( dom->device_model )
> >>+ {
>
>
> Can you fold this into the 'else' clause above and move
> xc_dom_alloc_segment() down?
Yes, I will.
> >>+
> >> 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;
> >>+ void *start_page;
> >>+ 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_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 = start_page;
> >>+ start_info = start_page;
> >>+ modlist = start_page + sizeof(*start_info) + dom->cmdline_size;
>
> I think we can drop start_page and use start_info only. They are the same,
> aren't they?
Yes, there are pointer to the same part of memory, with different types.
By droping start_page, I could replace few "start_page+sizeof(*start_info)"
by "start_info + 1" :), and have to use a cast for the line abrove.
--
Anthony PERARD
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH v4 04/14] firmware/makefile: install BIOS blob ...
2016-03-16 0:26 ` Konrad Rzeszutek Wilk
2016-03-16 8:54 ` Dario Faggioli
@ 2016-03-17 16:58 ` Anthony PERARD
1 sibling, 0 replies; 73+ messages in thread
From: Anthony PERARD @ 2016-03-17 16:58 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk; +Cc: Ian Jackson, Stefano Stabellini, Wei Liu, xen-devel
On Tue, Mar 15, 2016 at 08:26:01PM -0400, Konrad Rzeszutek Wilk wrote:
> On Mon, Mar 14, 2016 at 05:55:39PM +0000, Anthony PERARD wrote:
> > ... into the firmware directory, along with hvmloader.
> >
> > Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> > ---
> > 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 | 13 +++++++++++++
> > 1 file changed, 13 insertions(+)
> >
> > diff --git a/tools/firmware/Makefile b/tools/firmware/Makefile
> > index 6cc86ce..6a37758 100644
> > --- a/tools/firmware/Makefile
> > +++ b/tools/firmware/Makefile
> > @@ -19,6 +19,9 @@ SUBDIRS-y += hvmloader
> >
> > LD32BIT-$(CONFIG_FreeBSD) := LD32BIT_FLAG=-melf_i386_fbsd
> >
> > +SEABIOS_ROM := seabios-dir/out/bios.bin
> > +OVMF_ROM := ovmf-dir/ovmf.bin
>
> These will set the variables..
> > +
> > ovmf-dir:
> > GIT=$(GIT) $(XEN_ROOT)/scripts/git-checkout.sh $(OVMF_UPSTREAM_URL) $(OVMF_UPSTREAM_REVISION) ovmf-dir
> > cp ovmf-makefile ovmf-dir/Makefile;
> > @@ -45,6 +48,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),)
>
> But here you check them?
What do you mean by check them?
> Or should the setting of OVMF_ROM and SEABIOS_ROM be ?=
I don't see any reason to change the location of SEABIOS_ROM, it describe
the location to the one we built. I've mostly copy those from
hvmloader/Makefile.
But thinking about it, there is no more reason to have variables, I'll
remove SEABIOS_ROM and OVMF_ROM and hardcode it below.
So that would give for seabios:
$(INSTALL_DATA) seabios-dir/out/bios.bin $(INST_DIR)/seabios.bin
Would that be better?
> > + $(INSTALL_DATA) $(SEABIOS_ROM) $(INST_DIR)/seabios.bin
> > +endif
> > +endif
> > +ifeq ($(CONFIG_OVMF),y)
> > +ifeq ($(OVMF_PATH),)
> > + $(INSTALL_DATA) $(OVMF_ROM) $(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
--
Anthony PERARD
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH v4 05/14] libxl: Load guest BIOS from file
2016-03-16 9:27 ` Dario Faggioli
@ 2016-03-17 17:24 ` Anthony PERARD
0 siblings, 0 replies; 73+ messages in thread
From: Anthony PERARD @ 2016-03-17 17:24 UTC (permalink / raw)
To: Dario Faggioli; +Cc: xen-devel, Stefano Stabellini, Wei Liu
On Wed, Mar 16, 2016 at 10:27:25AM +0100, Dario Faggioli wrote:
> On Tue, 2016-03-15 at 20:53 -0400, Konrad Rzeszutek Wilk wrote:
> > On Mon, Mar 14, 2016 at 05:55:40PM +0000, Anthony PERARD wrote:
> > >
> > > --- a/docs/man/xl.cfg.pod.5
> > > +++ b/docs/man/xl.cfg.pod.5
> > > @@ -1121,6 +1121,15 @@ Requires device_model_version=qemu-xen.
> > >
> > > =back
> > >
> > > +=item B<bios_override="PATH">
> > Perhaps 'bios_override_path' ?
> >
> Or 'bios_path_override' ?
>
> Wow, a French, an Italian and a Polish arguing about best looking
> English-ish words... This must be a (bad) joke! :-P :-P
:)
To be honest, I just try to do the same as what is done for
"device_model_override". I even copied over the description, replacing
"binary" by "blob".
Maybe bios_path_override would be a good name, would this name can be
describe as "override bios_path"? At least that would not make someone
beleave that the bios= config option itself is overridden.
> > > +
> > > +Override the path to the blob to be used as BIOS. The blob
> > > provided here MUST
> > Perhaps:
> >
> > Override the path to search for the B<bios=>?
> >
> AFAIUI, B<bios=> does not contain an exact file name, but the
> indication of what kind of BIOS we want. Therefore, referencing it
> directly like this may be confusing (IMHO).
That's right, value can only be "rombios" or "seabios" or "ovmf".
> So, maybe something like: "Override the path at which to look for the
> BIOS binary. Such binary MUST be consistent with what has been
> specified in B<bios=>."
>
> > Or is this the full path including the name?? In which case should it
> > mention that B<bios=> is overriden?
> >
> If that is the case, indeed.
Yes this new config option is the full path. But it does not override
bios=.
If someone start a guest with this:
bios="seabios"
bios_override="/usr/share/ovmf/ovmf_x64.bin"
it's going to fail.
--
Anthony PERARD
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH v4 04/14] firmware/makefile: install BIOS blob ...
2016-03-14 17:55 ` [PATCH v4 04/14] firmware/makefile: install BIOS blob Anthony PERARD
2016-03-16 0:26 ` Konrad Rzeszutek Wilk
@ 2016-03-17 17:37 ` Doug Goldstein
2016-03-17 18:33 ` Anthony PERARD
2016-04-18 14:31 ` Doug Goldstein
2 siblings, 1 reply; 73+ messages in thread
From: Doug Goldstein @ 2016-03-17 17:37 UTC (permalink / raw)
To: Anthony PERARD, xen-devel; +Cc: Ian Jackson, Wei Liu, Stefano Stabellini
[-- Attachment #1.1.1: Type: text/plain, Size: 1737 bytes --]
On 3/14/16 12:55 PM, Anthony PERARD wrote:
> ... into the firmware directory, along with hvmloader.
>
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
> 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 | 13 +++++++++++++
> 1 file changed, 13 insertions(+)
>
> diff --git a/tools/firmware/Makefile b/tools/firmware/Makefile
> index 6cc86ce..6a37758 100644
> --- a/tools/firmware/Makefile
> +++ b/tools/firmware/Makefile
> @@ -19,6 +19,9 @@ SUBDIRS-y += hvmloader
>
> LD32BIT-$(CONFIG_FreeBSD) := LD32BIT_FLAG=-melf_i386_fbsd
>
> +SEABIOS_ROM := seabios-dir/out/bios.bin
> +OVMF_ROM := ovmf-dir/ovmf.bin
> +
> ovmf-dir:
> GIT=$(GIT) $(XEN_ROOT)/scripts/git-checkout.sh $(OVMF_UPSTREAM_URL) $(OVMF_UPSTREAM_REVISION) ovmf-dir
> cp ovmf-makefile ovmf-dir/Makefile;
> @@ -45,6 +48,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_ROM) $(INST_DIR)/seabios.bin
Why install this as "seabios.bin" when the default is "bios.bin". Most
distro's packages for SeaBIOS install it as "bios.bin"
> +endif
> +endif
> +ifeq ($(CONFIG_OVMF),y)
> +ifeq ($(OVMF_PATH),)
> + $(INSTALL_DATA) $(OVMF_ROM) $(INST_DIR)/ovmf.bin
> +endif
> +endif
>
> .PHONY: clean
> clean: subdirs-clean
>
--
Doug Goldstein
[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 959 bytes --]
[-- Attachment #2: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH v4 08/14] hvmloader: Locate the BIOS blob
2016-03-16 1:14 ` Konrad Rzeszutek Wilk
@ 2016-03-17 17:46 ` Anthony PERARD
2016-03-17 17:57 ` Konrad Rzeszutek Wilk
2016-03-18 7:34 ` Jan Beulich
0 siblings, 2 replies; 73+ messages in thread
From: Anthony PERARD @ 2016-03-17 17:46 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk
Cc: Keir Fraser, Stefano Stabellini, Andrew Cooper, Ian Jackson,
xen-devel, Jan Beulich, Wei Liu
On Tue, Mar 15, 2016 at 09:14:17PM -0400, Konrad Rzeszutek Wilk wrote:
> On Mon, Mar 14, 2016 at 05:55:43PM +0000, Anthony PERARD wrote:
> > The BIOS can be found an entry called "bios" of the modlist of the
>
> s/BIOS/BIOS blob/
> > hvm_start_info struct.
> >
> > The found BIOS blob is not loaded by this patch, but only passed as
> > argument to bios_load() function. It is going to be used by the next few
> > patches.
>
> Please list the 'few patches' by name as this patch and others may not
> always be committed in order.
Yes, will do.
> And it also helps in future to figure out what those other ones are
> when debugging or backporting.
>
> See below.
> >
> > Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> >
> > ---
> > 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 | 42 ++++++++++++++++++++++++++++++++++--
> > tools/firmware/hvmloader/ovmf.c | 3 ++-
> > tools/firmware/hvmloader/rombios.c | 3 ++-
> > tools/firmware/hvmloader/util.h | 2 ++
> > 5 files changed, 47 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..460efb9 100644
> > --- a/tools/firmware/hvmloader/hvmloader.c
> > +++ b/tools/firmware/hvmloader/hvmloader.c
> > @@ -253,10 +253,40 @@ 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 *)info->modlist_paddr;
> > + unsigned int i;
> > +
> > + if ( !modlist )
> > + return NULL;
> > +
> > + for ( i = 0; i < info->nr_modules; i++ )
> > + {
> > + uint32_t module_name = modlist[i].cmdline_paddr;
> > +
> > + BUG_ON(!modlist[i].cmdline_paddr ||
>
> Having an zero value in cmdline_paddr is OK.
>
> The spec says:
>
> 803 * NOTE: nothing will be loaded at physical address 0, so a 0 value in any
> 804 * of the address fields should be treated as not present.
> 805 *
>
> > + modlist[i].cmdline_paddr > UINT_MAX);
> > +
> > + if ( !strcmp(name, (char*)module_name) )
>
> Which could result in looking up a value at 0 address.
>
> Perhaps change your code to:
> if ( !modlist[i].cmdline_paddr )
> continue;
> ?
I'll do that. I'm using to many BUG_ON in this code.
> > + {
> > + BUG_ON(!modlist[i].paddr || modlist[i].paddr > UINT_MAX ||
> > + modlist[i].size > UINT_MAX);
>
> To be fair all of those values are in spec..Perhaps you should mention
> that the libxc code would never put those there and neermind the spec?
Yes, there is just this mention in the spec: "However Xen will always try
to place all modules below the 4GiB boundary."
But it actualy would be a bug if the blob would be abrove 4GiB, since
hvmloader can not read that far, right? I can add a comment.
--
Anthony PERARD
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH v4 08/14] hvmloader: Locate the BIOS blob
2016-03-17 17:46 ` Anthony PERARD
@ 2016-03-17 17:57 ` Konrad Rzeszutek Wilk
2016-03-18 7:34 ` Jan Beulich
1 sibling, 0 replies; 73+ messages in thread
From: Konrad Rzeszutek Wilk @ 2016-03-17 17:57 UTC (permalink / raw)
To: Anthony PERARD
Cc: Keir Fraser, Stefano Stabellini, Andrew Cooper, Ian Jackson,
xen-devel, Jan Beulich, Wei Liu
> I'll do that. I'm using to many BUG_ON in this code.
>
> > > + {
> > > + BUG_ON(!modlist[i].paddr || modlist[i].paddr > UINT_MAX ||
> > > + modlist[i].size > UINT_MAX);
> >
> > To be fair all of those values are in spec..Perhaps you should mention
> > that the libxc code would never put those there and neermind the spec?
>
> Yes, there is just this mention in the spec: "However Xen will always try
> to place all modules below the 4GiB boundary."
>
> But it actualy would be a bug if the blob would be abrove 4GiB, since
> hvmloader can not read that far, right? I can add a comment.
Well no. It says that the hypervisor will 'try', not that it 'MUST'.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH v4 09/14] hvmloader: Check modules whereabouts in perform_tests
2016-03-16 1:23 ` Konrad Rzeszutek Wilk
@ 2016-03-17 18:08 ` Anthony PERARD
0 siblings, 0 replies; 73+ messages in thread
From: Anthony PERARD @ 2016-03-17 18:08 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk
Cc: Keir Fraser, Stefano Stabellini, Andrew Cooper, Ian Jackson,
xen-devel, Jan Beulich, Wei Liu
On Tue, Mar 15, 2016 at 09:23:37PM -0400, Konrad Rzeszutek Wilk wrote:
> On Mon, Mar 14, 2016 at 05:55:44PM +0000, Anthony PERARD wrote:
> > As perform_tests() is going to clear memory past 4MB, we check that the
> > memory can be use or we skip the tests.
>
> I get the reason you want this - but if we have giant binary blobs
> and something else screws up what the tests are testing for- we
> don't have a regression test anymore.
>
> .. Oh my looking at rep_io_test and trying to make it more dynamic
> is tricky but should be managable?
Maybe, it have been mention that the tests could be more dynamic. But
they could also be moved to Andrew's Xen Test Framework.
For now, the test are unlikely to be skipped. On my system, in the worse
case, with rombios compiled in, and trying to boot with ovmf, the test are
not skiped. So with seabios it should be fine for some time.
Still, I'm not sure what to do about it.
> I presume that is the test that blows everything out of the sky?
Yes, the test.
> And also the placement of the page tables?
:(, I did not take the page tables into account will writing the check.
--
Anthony PERARD
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH v4 12/14] hvmloader: Specific bios_load function required
2016-03-16 1:38 ` Konrad Rzeszutek Wilk
@ 2016-03-17 18:25 ` Anthony PERARD
0 siblings, 0 replies; 73+ messages in thread
From: Anthony PERARD @ 2016-03-17 18:25 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk
Cc: Keir Fraser, Stefano Stabellini, Andrew Cooper, Ian Jackson,
xen-devel, Jan Beulich, Wei Liu
On Tue, Mar 15, 2016 at 09:38:18PM -0400, Konrad Rzeszutek Wilk wrote:
> On Mon, Mar 14, 2016 at 05:55:47PM +0000, Anthony PERARD wrote:
>
> The title says:
>
> " Specific bios_load function required "
>
> But I am not sure what you mean?
I meant that bios_config->bios_load() need to be defined and can not be
NULL anymore.
What about: "bios->bios_load() now needs to be defined"?
> > All BIOS but ROMBIOS needs to be loaded via modules.
>
> All BIOSes but ROMBIOS needs to be loaded via modules?
OK.
> >
> > ROMBIOS is handled as a special case.
> >
> > Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> > Acked-by: Jan Beulich <jbeulich@suse.com>
> > ---
> > 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 460efb9..bb2a309 100644
> > --- a/tools/firmware/hvmloader/hvmloader.c
> > +++ b/tools/firmware/hvmloader/hvmloader.c
> > @@ -323,21 +323,25 @@ int main(void)
> >
> > printf("Loading %s ...\n", bios->name);
> > bios_module = get_module_entry(hvm_start_info, "bios");
> > - 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, 0, 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 [flat|nested] 73+ messages in thread
* Re: [PATCH v4 04/14] firmware/makefile: install BIOS blob ...
2016-03-17 17:37 ` Doug Goldstein
@ 2016-03-17 18:33 ` Anthony PERARD
2016-03-18 21:11 ` Jim Fehlig
2016-03-19 0:43 ` Doug Goldstein
0 siblings, 2 replies; 73+ messages in thread
From: Anthony PERARD @ 2016-03-17 18:33 UTC (permalink / raw)
To: Doug Goldstein; +Cc: Ian Jackson, Stefano Stabellini, Wei Liu, xen-devel
On Thu, Mar 17, 2016 at 12:37:36PM -0500, Doug Goldstein wrote:
> On 3/14/16 12:55 PM, Anthony PERARD wrote:
> > ... into the firmware directory, along with hvmloader.
> >
> > Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> > ---
> > 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 | 13 +++++++++++++
> > 1 file changed, 13 insertions(+)
> >
> > diff --git a/tools/firmware/Makefile b/tools/firmware/Makefile
> > index 6cc86ce..6a37758 100644
> > --- a/tools/firmware/Makefile
> > +++ b/tools/firmware/Makefile
> > @@ -19,6 +19,9 @@ SUBDIRS-y += hvmloader
> >
> > LD32BIT-$(CONFIG_FreeBSD) := LD32BIT_FLAG=-melf_i386_fbsd
> >
> > +SEABIOS_ROM := seabios-dir/out/bios.bin
> > +OVMF_ROM := ovmf-dir/ovmf.bin
> > +
> > ovmf-dir:
> > GIT=$(GIT) $(XEN_ROOT)/scripts/git-checkout.sh $(OVMF_UPSTREAM_URL) $(OVMF_UPSTREAM_REVISION) ovmf-dir
> > cp ovmf-makefile ovmf-dir/Makefile;
> > @@ -45,6 +48,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_ROM) $(INST_DIR)/seabios.bin
>
> Why install this as "seabios.bin" when the default is "bios.bin". Most
> distro's packages for SeaBIOS install it as "bios.bin"
No reason. I guess it's fine to keep the same name ("bios.bin"). My distro
install it as "bios-256k.bin", with "bios.bin" been the small version I
guess.
--
Anthony PERARD
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH v4 08/14] hvmloader: Locate the BIOS blob
2016-03-17 17:46 ` Anthony PERARD
2016-03-17 17:57 ` Konrad Rzeszutek Wilk
@ 2016-03-18 7:34 ` Jan Beulich
1 sibling, 0 replies; 73+ messages in thread
From: Jan Beulich @ 2016-03-18 7:34 UTC (permalink / raw)
To: Anthony PERARD, Konrad Rzeszutek Wilk
Cc: Keir Fraser, Stefano Stabellini, Andrew Cooper, Ian Jackson,
xen-devel, Wei Liu
>>> On 17.03.16 at 18:46, <anthony.perard@citrix.com> wrote:
> On Tue, Mar 15, 2016 at 09:14:17PM -0400, Konrad Rzeszutek Wilk wrote:
>> On Mon, Mar 14, 2016 at 05:55:43PM +0000, Anthony PERARD wrote:
>> > The BIOS can be found an entry called "bios" of the modlist of the
>>
>> s/BIOS/BIOS blob/
>> > hvm_start_info struct.
>> >
>> > The found BIOS blob is not loaded by this patch, but only passed as
>> > argument to bios_load() function. It is going to be used by the next few
>> > patches.
>>
>> Please list the 'few patches' by name as this patch and others may not
>> always be committed in order.
>
> Yes, will do.
Or maybe just s/next few/subsequent/ or some such? Listing
future dependencies in a too explicit manner may also not be
a good idea - who knows if their titles change by the time they
go in?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH v4 04/14] firmware/makefile: install BIOS blob ...
2016-03-17 18:33 ` Anthony PERARD
@ 2016-03-18 21:11 ` Jim Fehlig
2016-03-19 0:43 ` Doug Goldstein
1 sibling, 0 replies; 73+ messages in thread
From: Jim Fehlig @ 2016-03-18 21:11 UTC (permalink / raw)
To: Anthony PERARD, Doug Goldstein
Cc: Wei Liu, xen-devel, Ian Jackson, Stefano Stabellini
On 03/17/2016 12:33 PM, Anthony PERARD wrote:
> On Thu, Mar 17, 2016 at 12:37:36PM -0500, Doug Goldstein wrote:
>> On 3/14/16 12:55 PM, Anthony PERARD wrote:
>>> ... into the firmware directory, along with hvmloader.
>>>
>>> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
>>> ---
>>> 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 | 13 +++++++++++++
>>> 1 file changed, 13 insertions(+)
>>>
>>> diff --git a/tools/firmware/Makefile b/tools/firmware/Makefile
>>> index 6cc86ce..6a37758 100644
>>> --- a/tools/firmware/Makefile
>>> +++ b/tools/firmware/Makefile
>>> @@ -19,6 +19,9 @@ SUBDIRS-y += hvmloader
>>>
>>> LD32BIT-$(CONFIG_FreeBSD) := LD32BIT_FLAG=-melf_i386_fbsd
>>>
>>> +SEABIOS_ROM := seabios-dir/out/bios.bin
>>> +OVMF_ROM := ovmf-dir/ovmf.bin
>>> +
>>> ovmf-dir:
>>> GIT=$(GIT) $(XEN_ROOT)/scripts/git-checkout.sh $(OVMF_UPSTREAM_URL) $(OVMF_UPSTREAM_REVISION) ovmf-dir
>>> cp ovmf-makefile ovmf-dir/Makefile;
>>> @@ -45,6 +48,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_ROM) $(INST_DIR)/seabios.bin
>> Why install this as "seabios.bin" when the default is "bios.bin". Most
>> distro's packages for SeaBIOS install it as "bios.bin"
> No reason. I guess it's fine to keep the same name ("bios.bin"). My distro
> install it as "bios-256k.bin", with "bios.bin" been the small version I
> guess.
Same with SUSE's SeaBIOS package, which contains bios.bin (sz 131072) and
bios-256k.bin (sz 262144).
Regards,
Jim
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH v4 04/14] firmware/makefile: install BIOS blob ...
2016-03-17 18:33 ` Anthony PERARD
2016-03-18 21:11 ` Jim Fehlig
@ 2016-03-19 0:43 ` Doug Goldstein
1 sibling, 0 replies; 73+ messages in thread
From: Doug Goldstein @ 2016-03-19 0:43 UTC (permalink / raw)
To: Anthony PERARD; +Cc: Ian Jackson, Stefano Stabellini, Wei Liu, xen-devel
[-- Attachment #1.1.1: Type: text/plain, Size: 2221 bytes --]
On 3/17/16 1:33 PM, Anthony PERARD wrote:
> On Thu, Mar 17, 2016 at 12:37:36PM -0500, Doug Goldstein wrote:
>> On 3/14/16 12:55 PM, Anthony PERARD wrote:
>>> ... into the firmware directory, along with hvmloader.
>>>
>>> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
>>> ---
>>> 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 | 13 +++++++++++++
>>> 1 file changed, 13 insertions(+)
>>>
>>> diff --git a/tools/firmware/Makefile b/tools/firmware/Makefile
>>> index 6cc86ce..6a37758 100644
>>> --- a/tools/firmware/Makefile
>>> +++ b/tools/firmware/Makefile
>>> @@ -19,6 +19,9 @@ SUBDIRS-y += hvmloader
>>>
>>> LD32BIT-$(CONFIG_FreeBSD) := LD32BIT_FLAG=-melf_i386_fbsd
>>>
>>> +SEABIOS_ROM := seabios-dir/out/bios.bin
>>> +OVMF_ROM := ovmf-dir/ovmf.bin
>>> +
>>> ovmf-dir:
>>> GIT=$(GIT) $(XEN_ROOT)/scripts/git-checkout.sh $(OVMF_UPSTREAM_URL) $(OVMF_UPSTREAM_REVISION) ovmf-dir
>>> cp ovmf-makefile ovmf-dir/Makefile;
>>> @@ -45,6 +48,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_ROM) $(INST_DIR)/seabios.bin
>>
>> Why install this as "seabios.bin" when the default is "bios.bin". Most
>> distro's packages for SeaBIOS install it as "bios.bin"
>
> No reason. I guess it's fine to keep the same name ("bios.bin"). My distro
> install it as "bios-256k.bin", with "bios.bin" been the small version I
> guess.
>
>
Right. All the distros should be doing that now for modern versions of
SeaBIOS. When this work gets packaged up in a distro they'll likely use
one of those two versions instead of using the one that Xen builds, like
they mostly do with QEMU. I'd have to check how Xen builds SeaBIOS to
know which it should point to.
--
Doug Goldstein
[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 959 bytes --]
[-- Attachment #2: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH v4 06/14] xen: Move the hvm_start_info C representation from libxc to public/xen.h
2016-03-15 8:09 ` Jan Beulich
2016-03-16 0:59 ` Konrad Rzeszutek Wilk
@ 2016-03-21 17:04 ` Roger Pau Monné
2016-03-21 17:21 ` Jan Beulich
1 sibling, 1 reply; 73+ messages in thread
From: Roger Pau Monné @ 2016-03-21 17:04 UTC (permalink / raw)
To: Jan Beulich
Cc: Wei Liu, Stefano Stabellini, Ian Jackson, xen-devel,
Anthony PERARD, Roger Pau Monne
Hello,
On Tue, 15 Mar 2016, Jan Beulich wrote:
> >>> On 14.03.16 at 18:55, <anthony.perard@citrix.com> wrote:
> > --- a/xen/include/public/xen.h
> > +++ b/xen/include/public/xen.h
> > @@ -841,6 +841,37 @@ 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.
> > + *
> > + * 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.
> > + */
^ Rationale on why the packed attribute was added.
> > +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 cmdline_paddr; /* Physical address of the command line. */
> > + uint32_t nr_modules; /* Number of modules passed to the kernel. */
> > + uint32_t modlist_paddr; /* Physical address of an array of */
> > + /* hvm_modlist_entry. */
> > + uint32_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 */
>
> This needs extra care: Either the packed attributes need to be
> dropped (Roger - why did they get put there in the first place?),
> or their use needs to be shielded from compilers not understanding
> them.
Hello,
The rationale behind the packed attribute is in the highlighted comment
above.
I would really like to avoid placing this in public headers, or else
people will think this is the definition of the payload and will forget
that this is just a C representation of it, but the definition is in a
comment just above. I want to avoid the issues we have already seen by the
usage of C structures as definitions of the placement of payloads in
memory.
If this really has to be there, please guard it with:
#if defined(__XEN__) || defined(__XEN_TOOLS__)
So only the Xen kernel/tools can use it.
Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH v4 06/14] xen: Move the hvm_start_info C representation from libxc to public/xen.h
2016-03-21 17:04 ` Roger Pau Monné
@ 2016-03-21 17:21 ` Jan Beulich
0 siblings, 0 replies; 73+ messages in thread
From: Jan Beulich @ 2016-03-21 17:21 UTC (permalink / raw)
To: Roger Pau Monne
Cc: Anthony PERARD, Ian Jackson, xen-devel, Wei Liu, Stefano Stabellini
>>> On 21.03.16 at 18:04, <roger.pau@citrix.com> wrote:
> On Tue, 15 Mar 2016, Jan Beulich wrote:
>> >>> On 14.03.16 at 18:55, <anthony.perard@citrix.com> wrote:
>> > --- a/xen/include/public/xen.h
>> > +++ b/xen/include/public/xen.h
>> > @@ -841,6 +841,37 @@ 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.
>> > + *
>> > + * 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.
>> > + */
>
> ^ Rationale on why the packed attribute was added.
Well, I admit to have overlooked this comment, but I don't see
how the packed attribute helps enforce anything. Hence, Anthony,
I think together with the attribute that part of the comment should
be removed.
> I would really like to avoid placing this in public headers, or else
> people will think this is the definition of the payload and will forget
> that this is just a C representation of it, but the definition is in a
> comment just above. I want to avoid the issues we have already seen by the
> usage of C structures as definitions of the placement of payloads in
> memory.
>
> If this really has to be there, please guard it with:
>
> #if defined(__XEN__) || defined(__XEN_TOOLS__)
>
> So only the Xen kernel/tools can use it.
And hvmloader can't. Not a good idea imo.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH v4 00/14] Load BIOS via toolstack instead of been embedded in hvmloader.
2016-03-14 17:55 [PATCH v4 00/14] Load BIOS via toolstack instead of been embedded in hvmloader Anthony PERARD
` (13 preceding siblings ...)
2016-03-14 17:55 ` [PATCH v4 14/14] configure: do not depend on SEABIOS_PATH or OVMF_PATH Anthony PERARD
@ 2016-03-24 17:55 ` Jim Fehlig
2016-03-30 17:22 ` Jim Fehlig
15 siblings, 0 replies; 73+ messages in thread
From: Jim Fehlig @ 2016-03-24 17:55 UTC (permalink / raw)
To: Anthony PERARD
Cc: Wei Liu, Stefano Stabellini, Andrew Cooper, Ian Jackson,
xen-devel, Jan Beulich
Anthony PERARD wrote:
> Hi all,
>
> Few changes in V4:
> I leave the ACPI alone and only load the BIOS via libxl now.
>
> Detail of changes in each patches.
>
> I've look at loading the BIOS via the toolstack instead of having them embedded
> in the hvmloader binary. After this patch series, hvmloader compilation would
> be indenpendant from SeaBIOS and OVMF compilation.
>
> Here is a general view of the few step to load the BIOS:
> - libxl load the BIOS blob into it's memory and add it to struct
> xc_hvm_build_args.bios_module
> - libxc load the blob into the guest memory and fill the struct
> hvm_start_info and store a name for each module into the module cmdline.
> - hvmloader read the addresses from the hvm_start_info, find out which
> module to load and copy the blob to the right place.
>
> A git tree can be found here:
> git://xenbits.xen.org/people/aperard/xen-unstable.git
> tag: hvmloader-with-separated-bios-v4
>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: Jan Beulich <jbeulich@suse.com>
I've been testing this series in conjunction with my downstream work to use
distro-provided qemu, SeaBIOS, and OVMF instead of the Xen built ones. I've
tested many combinations of these components, e.g. distro qemu and SeaBIOS,
qemu-xen with distro SeaBIOS, distro qemu with Xen SeaBIOS, etc., and haven't
noticed any problems. Therefore...
Tested-by: Jim Fehlig <jfehlig@suse.com>
Regards,
Jim
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH v4 00/14] Load BIOS via toolstack instead of been embedded in hvmloader.
2016-03-14 17:55 [PATCH v4 00/14] Load BIOS via toolstack instead of been embedded in hvmloader Anthony PERARD
` (14 preceding siblings ...)
2016-03-24 17:55 ` [PATCH v4 00/14] Load BIOS via toolstack instead of been embedded in hvmloader Jim Fehlig
@ 2016-03-30 17:22 ` Jim Fehlig
2016-03-31 7:20 ` Jan Beulich
[not found] ` <56FCEBCD02000078000E19BF@suse.com>
15 siblings, 2 replies; 73+ messages in thread
From: Jim Fehlig @ 2016-03-30 17:22 UTC (permalink / raw)
To: Anthony PERARD
Cc: Wei Liu, Stefano Stabellini, Andrew Cooper, Ian Jackson,
xen-devel, Jan Beulich
Anthony PERARD wrote:
> Hi all,
>
> Few changes in V4:
> I leave the ACPI alone and only load the BIOS via libxl now.
>
> Detail of changes in each patches.
>
> I've look at loading the BIOS via the toolstack instead of having them embedded
> in the hvmloader binary. After this patch series, hvmloader compilation would
> be indenpendant from SeaBIOS and OVMF compilation.
Hi Anthony, Wei,
Any chance this series will make 4.7? AFAICT, there were no major issues in the
reviews. I didn't review all the patches in detail, but have done quite a bit of
testing with this series and haven't noticed any problems (beyond the current
upstream ovmf breakage with Xen).
Regards,
Jim
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH v4 00/14] Load BIOS via toolstack instead of been embedded in hvmloader.
2016-03-30 17:22 ` Jim Fehlig
@ 2016-03-31 7:20 ` Jan Beulich
[not found] ` <56FCEBCD02000078000E19BF@suse.com>
1 sibling, 0 replies; 73+ messages in thread
From: Jan Beulich @ 2016-03-31 7:20 UTC (permalink / raw)
To: Jim Fehlig
Cc: Wei Liu, Stefano Stabellini, Andrew Cooper, Ian Jackson,
xen-devel, Anthony PERARD
>>> On 30.03.16 at 19:22, <JFEHLIG@suse.com> wrote:
> Anthony PERARD wrote:
>> Hi all,
>>
>> Few changes in V4:
>> I leave the ACPI alone and only load the BIOS via libxl now.
>>
>> Detail of changes in each patches.
>>
>> I've look at loading the BIOS via the toolstack instead of having them
> embedded
>> in the hvmloader binary. After this patch series, hvmloader compilation
> would
>> be indenpendant from SeaBIOS and OVMF compilation.
>
> Hi Anthony, Wei,
>
> Any chance this series will make 4.7? AFAICT, there were no major issues in
> the
> reviews. I didn't review all the patches in detail, but have done quite a
> bit of
> testing with this series and haven't noticed any problems (beyond the
> current
> upstream ovmf breakage with Xen).
I'm afraid the primary issue here is reviewing bandwidth: While
the hvmloader parts are on my list of things to look at, this
clearly comes only after bug fixes, Andrew's CPUID and Konrad's
xSplice work in priority, and I already lack the time to provide
timely feedback on those two.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH v4 00/14] Load BIOS via toolstack instead of been embedded in hvmloader.
[not found] ` <56FCEBCD02000078000E19BF@suse.com>
@ 2016-03-31 14:36 ` Jim Fehlig
2016-03-31 16:49 ` George Dunlap
0 siblings, 1 reply; 73+ messages in thread
From: Jim Fehlig @ 2016-03-31 14:36 UTC (permalink / raw)
To: Jan Beulich
Cc: Wei Liu, Stefano Stabellini, Andrew Cooper, Ian Jackson,
xen-devel, Anthony PERARD
Jan Beulich wrote:
>>>> On 30.03.16 at 19:22, <JFEHLIG@suse.com> wrote:
>> Anthony PERARD wrote:
>>> Hi all,
>>>
>>> Few changes in V4:
>>> I leave the ACPI alone and only load the BIOS via libxl now.
>>>
>>> Detail of changes in each patches.
>>>
>>> I've look at loading the BIOS via the toolstack instead of having them
>> embedded
>>> in the hvmloader binary. After this patch series, hvmloader compilation
>> would
>>> be indenpendant from SeaBIOS and OVMF compilation.
>> Hi Anthony, Wei,
>>
>> Any chance this series will make 4.7? AFAICT, there were no major issues in
>> the
>> reviews. I didn't review all the patches in detail, but have done quite a
>> bit of
>> testing with this series and haven't noticed any problems (beyond the
>> current
>> upstream ovmf breakage with Xen).
>
> I'm afraid the primary issue here is reviewing bandwidth: While
> the hvmloader parts are on my list of things to look at
And these are precisely the patches I just skimmed over, since I'm not familiar
with hvmloader.
Jim
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH v4 00/14] Load BIOS via toolstack instead of been embedded in hvmloader.
2016-03-31 14:36 ` Jim Fehlig
@ 2016-03-31 16:49 ` George Dunlap
2016-04-01 9:12 ` George Dunlap
0 siblings, 1 reply; 73+ messages in thread
From: George Dunlap @ 2016-03-31 16:49 UTC (permalink / raw)
To: Jim Fehlig
Cc: Wei Liu, Stefano Stabellini, Andrew Cooper, Ian Jackson,
xen-devel, Jan Beulich, Anthony PERARD
On Thu, Mar 31, 2016 at 3:36 PM, Jim Fehlig <jfehlig@suse.com> wrote:
> Jan Beulich wrote:
>>>>> On 30.03.16 at 19:22, <JFEHLIG@suse.com> wrote:
>>> Anthony PERARD wrote:
>>>> Hi all,
>>>>
>>>> Few changes in V4:
>>>> I leave the ACPI alone and only load the BIOS via libxl now.
>>>>
>>>> Detail of changes in each patches.
>>>>
>>>> I've look at loading the BIOS via the toolstack instead of having them
>>> embedded
>>>> in the hvmloader binary. After this patch series, hvmloader compilation
>>> would
>>>> be indenpendant from SeaBIOS and OVMF compilation.
>>> Hi Anthony, Wei,
>>>
>>> Any chance this series will make 4.7? AFAICT, there were no major issues in
>>> the
>>> reviews. I didn't review all the patches in detail, but have done quite a
>>> bit of
>>> testing with this series and haven't noticed any problems (beyond the
>>> current
>>> upstream ovmf breakage with Xen).
>>
>> I'm afraid the primary issue here is reviewing bandwidth: While
>> the hvmloader parts are on my list of things to look at
>
> And these are precisely the patches I just skimmed over, since I'm not familiar
> with hvmloader.
Hmm, I probably could have reviewed it if I'd known a bit earlier.
Unfortunately I don't work for Citrix tomorrow.
-George
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH v4 00/14] Load BIOS via toolstack instead of been embedded in hvmloader.
2016-03-31 16:49 ` George Dunlap
@ 2016-04-01 9:12 ` George Dunlap
2016-04-01 14:24 ` Konrad Rzeszutek Wilk
2016-04-01 20:06 ` Jim Fehlig
0 siblings, 2 replies; 73+ messages in thread
From: George Dunlap @ 2016-04-01 9:12 UTC (permalink / raw)
To: Jim Fehlig
Cc: Wei Liu, Stefano Stabellini, Andrew Cooper, Ian Jackson,
xen-devel, Jan Beulich, Anthony PERARD
On Thu, Mar 31, 2016 at 5:49 PM, George Dunlap <dunlapg@umich.edu> wrote:
> On Thu, Mar 31, 2016 at 3:36 PM, Jim Fehlig <jfehlig@suse.com> wrote:
>> Jan Beulich wrote:
>>>>>> On 30.03.16 at 19:22, <JFEHLIG@suse.com> wrote:
>>>> Anthony PERARD wrote:
>>>>> Hi all,
>>>>>
>>>>> Few changes in V4:
>>>>> I leave the ACPI alone and only load the BIOS via libxl now.
>>>>>
>>>>> Detail of changes in each patches.
>>>>>
>>>>> I've look at loading the BIOS via the toolstack instead of having them
>>>> embedded
>>>>> in the hvmloader binary. After this patch series, hvmloader compilation
>>>> would
>>>>> be indenpendant from SeaBIOS and OVMF compilation.
>>>> Hi Anthony, Wei,
>>>>
>>>> Any chance this series will make 4.7? AFAICT, there were no major issues in
>>>> the
>>>> reviews. I didn't review all the patches in detail, but have done quite a
>>>> bit of
>>>> testing with this series and haven't noticed any problems (beyond the
>>>> current
>>>> upstream ovmf breakage with Xen).
>>>
>>> I'm afraid the primary issue here is reviewing bandwidth: While
>>> the hvmloader parts are on my list of things to look at
>>
>> And these are precisely the patches I just skimmed over, since I'm not familiar
>> with hvmloader.
>
> Hmm, I probably could have reviewed it if I'd known a bit earlier.
> Unfortunately I don't work for Citrix tomorrow.
Since there's obviously been a failure to communicate here, allow me
to expand on this. Last March (or was it March 2014? I forget now) I
asked for permission to cut back to 80% time at Citrix; so since then
I've been working M-Th, and Friday was "work for myself" day where I
mainly pursued personal projects.
And now that it's been publicly announced, this is actually my *last*
"work for myself" Friday -- as of next week I'm back to 100%.
So no, you haven't gotten rid of me just yet; on the contrary, you'll
soon get 25% more. :-)
-George
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH v4 00/14] Load BIOS via toolstack instead of been embedded in hvmloader.
2016-04-01 9:12 ` George Dunlap
@ 2016-04-01 14:24 ` Konrad Rzeszutek Wilk
2016-04-01 20:06 ` Jim Fehlig
1 sibling, 0 replies; 73+ messages in thread
From: Konrad Rzeszutek Wilk @ 2016-04-01 14:24 UTC (permalink / raw)
To: George Dunlap
Cc: Wei Liu, Stefano Stabellini, Andrew Cooper, Ian Jackson,
xen-devel, Jim Fehlig, Jan Beulich, Anthony PERARD
On Fri, Apr 01, 2016 at 10:12:54AM +0100, George Dunlap wrote:
> On Thu, Mar 31, 2016 at 5:49 PM, George Dunlap <dunlapg@umich.edu> wrote:
> > On Thu, Mar 31, 2016 at 3:36 PM, Jim Fehlig <jfehlig@suse.com> wrote:
> >> Jan Beulich wrote:
> >>>>>> On 30.03.16 at 19:22, <JFEHLIG@suse.com> wrote:
> >>>> Anthony PERARD wrote:
> >>>>> Hi all,
> >>>>>
> >>>>> Few changes in V4:
> >>>>> I leave the ACPI alone and only load the BIOS via libxl now.
> >>>>>
> >>>>> Detail of changes in each patches.
> >>>>>
> >>>>> I've look at loading the BIOS via the toolstack instead of having them
> >>>> embedded
> >>>>> in the hvmloader binary. After this patch series, hvmloader compilation
> >>>> would
> >>>>> be indenpendant from SeaBIOS and OVMF compilation.
> >>>> Hi Anthony, Wei,
> >>>>
> >>>> Any chance this series will make 4.7? AFAICT, there were no major issues in
> >>>> the
> >>>> reviews. I didn't review all the patches in detail, but have done quite a
> >>>> bit of
> >>>> testing with this series and haven't noticed any problems (beyond the
> >>>> current
> >>>> upstream ovmf breakage with Xen).
> >>>
> >>> I'm afraid the primary issue here is reviewing bandwidth: While
> >>> the hvmloader parts are on my list of things to look at
> >>
> >> And these are precisely the patches I just skimmed over, since I'm not familiar
> >> with hvmloader.
> >
> > Hmm, I probably could have reviewed it if I'd known a bit earlier.
> > Unfortunately I don't work for Citrix tomorrow.
>
> Since there's obviously been a failure to communicate here, allow me
> to expand on this. Last March (or was it March 2014? I forget now) I
> asked for permission to cut back to 80% time at Citrix; so since then
> I've been working M-Th, and Friday was "work for myself" day where I
> mainly pursued personal projects.
>
> And now that it's been publicly announced, this is actually my *last*
> "work for myself" Friday -- as of next week I'm back to 100%.
>
> So no, you haven't gotten rid of me just yet; on the contrary, you'll
> soon get 25% more. :-)
<Wheew>
You scared me George!
Wait, today is April 1st!?
Um?
Hmmmm...
Well, I am going to be an optimist and assume this is not a joke!
Welcome back to 100%!
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH v4 00/14] Load BIOS via toolstack instead of been embedded in hvmloader.
2016-04-01 9:12 ` George Dunlap
2016-04-01 14:24 ` Konrad Rzeszutek Wilk
@ 2016-04-01 20:06 ` Jim Fehlig
1 sibling, 0 replies; 73+ messages in thread
From: Jim Fehlig @ 2016-04-01 20:06 UTC (permalink / raw)
To: George Dunlap
Cc: Wei Liu, Stefano Stabellini, Andrew Cooper, Ian Jackson,
xen-devel, Jan Beulich, Anthony PERARD
On 04/01/2016 03:12 AM, George Dunlap wrote:
> So no, you haven't gotten rid of me just yet; on the contrary, you'll
> soon get 25% more. :-)
Looking forward to it :-).
Regards,
Jim
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH v4 07/14] hvmloader: Grab the hvm_start_info pointer
2016-03-14 17:55 ` [PATCH v4 07/14] hvmloader: Grab the hvm_start_info pointer Anthony PERARD
2016-03-16 1:07 ` Konrad Rzeszutek Wilk
@ 2016-04-05 12:43 ` Jan Beulich
1 sibling, 0 replies; 73+ messages in thread
From: Jan Beulich @ 2016-04-05 12:43 UTC (permalink / raw)
To: Anthony PERARD
Cc: Keir Fraser, Stefano Stabellini, Andrew Cooper, Ian Jackson,
xen-devel, Wei Liu
>>> On 14.03.16 at 18:55, <anthony.perard@citrix.com> wrote:
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH v4 08/14] hvmloader: Locate the BIOS blob
2016-03-14 17:55 ` [PATCH v4 08/14] hvmloader: Locate the BIOS blob Anthony PERARD
2016-03-16 1:14 ` Konrad Rzeszutek Wilk
@ 2016-04-05 12:59 ` Jan Beulich
2016-04-05 14:05 ` Roger Pau Monné
2016-04-07 15:10 ` Anthony PERARD
1 sibling, 2 replies; 73+ messages in thread
From: Jan Beulich @ 2016-04-05 12:59 UTC (permalink / raw)
To: Anthony PERARD, Roger Pau Monne
Cc: Keir Fraser, Stefano Stabellini, Andrew Cooper, Ian Jackson,
xen-devel, Wei Liu
>>> On 14.03.16 at 18:55, <anthony.perard@citrix.com> wrote:
> --- a/tools/firmware/hvmloader/hvmloader.c
> +++ b/tools/firmware/hvmloader/hvmloader.c
> @@ -253,10 +253,40 @@ 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 *)info->modlist_paddr;
This cast puzzles me (as at the first glance I would expect it to
cause a compiler warning): Roger, how come cmdline_paddr,
modlist_paddr, and rsdp_paddr are 32-bit quantities? While on
x86 that _may_ be fine, what about other architectures we may
want to run Xen on?
> + unsigned int i;
> +
> + if ( !modlist )
> + return NULL;
> +
> + for ( i = 0; i < info->nr_modules; i++ )
> + {
> + uint32_t module_name = modlist[i].cmdline_paddr;
> +
> + BUG_ON(!modlist[i].cmdline_paddr ||
> + modlist[i].cmdline_paddr > UINT_MAX);
While you can't use the local variable for the latter check, you can
for the former.
> @@ -292,8 +322,16 @@ 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, "bios");
Isn't "bios" a bit vague, as there could be multiple (system, video,
add-on card)?
> + if ( bios_module && bios->bios_load )
> + {
> + uint32_t paddr = bios_module->paddr;
> + bios->bios_load(bios, (void*)paddr, bios_module->size);
Blank line between declaration(s) and statement(s) please.
> + }
> + else if ( bios->bios_load )
> + {
> + bios->bios_load(bios, 0, 0);
bios->bios_load(bios, NULL, 0);
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH v4 09/14] hvmloader: Check modules whereabouts in perform_tests
2016-03-14 17:55 ` [PATCH v4 09/14] hvmloader: Check modules whereabouts in perform_tests Anthony PERARD
2016-03-16 1:23 ` Konrad Rzeszutek Wilk
@ 2016-04-05 13:07 ` Jan Beulich
1 sibling, 0 replies; 73+ messages in thread
From: Jan Beulich @ 2016-04-05 13:07 UTC (permalink / raw)
To: Anthony PERARD
Cc: Keir Fraser, Stefano Stabellini, Andrew Cooper, Ian Jackson,
xen-devel, Wei Liu
>>> On 14.03.16 at 18:55, <anthony.perard@citrix.com> wrote:
> 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 v4:
> - move the check into the perform_test() function.
> - skip tests instead of using BUG.
>
> New in V3
> ---
> tools/firmware/hvmloader/tests.c | 20 ++++++++++++++++++++
> 1 file changed, 20 insertions(+)
>
> diff --git a/tools/firmware/hvmloader/tests.c
> b/tools/firmware/hvmloader/tests.c
> index fea3ad3..7996206 100644
> --- a/tools/firmware/hvmloader/tests.c
> +++ b/tools/firmware/hvmloader/tests.c
> @@ -210,6 +210,26 @@ void perform_tests(void)
> return;
> }
>
> + /* Check that tests does not use memory where modules are stored */
> + if ( ((uint32_t)hvm_start_info + sizeof(struct hvm_start_info)) > 4 << 20
if ( (uint32_t)(hvm_start_info + 1) > (4 << 20)
> + && (uint32_t)hvm_start_info < 8 << 20 )
Missing parentheses around operands of <<. Also commonly the
&& would belong on the previous line.
> + {
> + printf("Skipping tests due to memory used by hvm_start_info\n");
> + return;
> + }
> + for ( unsigned i = 0; i < hvm_start_info->nr_modules; i++ )
Bogus shadowing of an outer scope variable. And we prefer not to
use this C99 declaration style anyway.
> + {
> + const struct hvm_modlist_entry *modlist =
> + (struct hvm_modlist_entry *)hvm_start_info->modlist_paddr;
> + if ( modlist[i].paddr
Blank line ...
> + && modlist[i].paddr + modlist[i].size > 4ul << 20
> + && modlist[i].paddr < 8ul << 20 )
Parentheses ... Also, why the ul suffixed here but not above?
Please be consistent.
> + {
> + printf("Skipping tests due to memory used by a module\n");
How about at least printing the module index as well?
> + return;
> + }
> + }
How about cmdline_paddr (both variant) and rsdp_paddr?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH v4 10/14] hvmloader: Load SeaBIOS from hvm_start_info modules
2016-03-14 17:55 ` [PATCH v4 10/14] hvmloader: Load SeaBIOS from hvm_start_info modules Anthony PERARD
2016-03-16 1:27 ` Konrad Rzeszutek Wilk
@ 2016-04-05 13:11 ` Jan Beulich
1 sibling, 0 replies; 73+ messages in thread
From: Jan Beulich @ 2016-04-05 13:11 UTC (permalink / raw)
To: Anthony PERARD
Cc: Keir Fraser, Stefano Stabellini, Andrew Cooper, Ian Jackson,
xen-devel, Wei Liu
>>> On 14.03.16 at 18:55, <anthony.perard@citrix.com> wrote:
> ... 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 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 | 24 ++++++++++++++----------
> 2 files changed, 15 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..5d0a491 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,29 @@ static void seabios_setup_e820(void)
> struct e820entry *e820 = scratch_alloc(sizeof(struct e820entry)*16, 0);
> info->e820 = (uint32_t)e820;
>
> + BUG_ON(seabios_config.bios_address < 0xc0000 || seabios_config.bios_address >= 0x100000);
The right side could benefit from also taking seabios_config.image_size
into account. Or wait - the right side could go away, as being redundant
with ...
> /* 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);
... this.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH v4 11/14] hvmloader: Load OVMF from modules
2016-03-14 17:55 ` [PATCH v4 11/14] hvmloader: Load OVMF from modules Anthony PERARD
2016-03-16 1:36 ` Konrad Rzeszutek Wilk
@ 2016-04-05 13:16 ` Jan Beulich
1 sibling, 0 replies; 73+ messages in thread
From: Jan Beulich @ 2016-04-05 13:16 UTC (permalink / raw)
To: Anthony PERARD
Cc: Keir Fraser, Stefano Stabellini, Andrew Cooper, Ian Jackson,
xen-devel, Wei Liu
>>> On 14.03.16 at 18:55, <anthony.perard@citrix.com> wrote:
> ... 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>
with some cosmetics left to be done:
> /* 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,
Please take the opportunity and remove the stray blanks here ...
> /* Copy FD. */
> - memcpy((void *) OVMF_BEGIN, config->image, OVMF_SIZE);
> + memcpy((void *) ovmf_config.bios_address, bios_addr, bios_length);
... and here (between cast's closing paren and casted expression).
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH v4 08/14] hvmloader: Locate the BIOS blob
2016-04-05 12:59 ` Jan Beulich
@ 2016-04-05 14:05 ` Roger Pau Monné
2016-04-05 14:23 ` Jan Beulich
2016-04-07 15:10 ` Anthony PERARD
1 sibling, 1 reply; 73+ messages in thread
From: Roger Pau Monné @ 2016-04-05 14:05 UTC (permalink / raw)
To: Jan Beulich
Cc: Wei Liu, Stefano Stabellini, Andrew Cooper, Ian Jackson,
xen-devel, Anthony PERARD, Keir Fraser, Roger Pau Monne
On Tue, 5 Apr 2016, Jan Beulich wrote:
> >>> On 14.03.16 at 18:55, <anthony.perard@citrix.com> wrote:
> > --- a/tools/firmware/hvmloader/hvmloader.c
> > +++ b/tools/firmware/hvmloader/hvmloader.c
> > @@ -253,10 +253,40 @@ 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 *)info->modlist_paddr;
>
> This cast puzzles me (as at the first glance I would expect it to
> cause a compiler warning): Roger, how come cmdline_paddr,
> modlist_paddr, and rsdp_paddr are 32-bit quantities? While on
> x86 that _may_ be fine, what about other architectures we may
> want to run Xen on?
I've always considered this protocol x86 specific TBH, and since guests
are started in 32bit mode I have always considered mandatory to have all
this information available below the 4GiB boundary.
I don't mind changing the sizes to be 64bits, it's a fairly easy change
that could be done before the release.
Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH v4 08/14] hvmloader: Locate the BIOS blob
2016-04-05 14:05 ` Roger Pau Monné
@ 2016-04-05 14:23 ` Jan Beulich
0 siblings, 0 replies; 73+ messages in thread
From: Jan Beulich @ 2016-04-05 14:23 UTC (permalink / raw)
To: Roger Pau Monne
Cc: Wei Liu, StefanoStabellini, Andrew Cooper, Ian Jackson,
xen-devel, Anthony PERARD, Keir Fraser
>>> On 05.04.16 at 16:05, <roger.pau@citrix.com> wrote:
> On Tue, 5 Apr 2016, Jan Beulich wrote:
>> >>> On 14.03.16 at 18:55, <anthony.perard@citrix.com> wrote:
>> > --- a/tools/firmware/hvmloader/hvmloader.c
>> > +++ b/tools/firmware/hvmloader/hvmloader.c
>> > @@ -253,10 +253,40 @@ 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 *)info->modlist_paddr;
>>
>> This cast puzzles me (as at the first glance I would expect it to
>> cause a compiler warning): Roger, how come cmdline_paddr,
>> modlist_paddr, and rsdp_paddr are 32-bit quantities? While on
>> x86 that _may_ be fine, what about other architectures we may
>> want to run Xen on?
>
> I've always considered this protocol x86 specific TBH, and since guests
> are started in 32bit mode I have always considered mandatory to have all
> this information available below the 4GiB boundary.
I could live with that, but then this should be made explicit, rather
than depending on only x86 defining XEN_HAVE_PV_GUEST_ENTRY.
And if that was to be done, then the other 64-bit address changes
likely could be undone again, too. We really (just) need to settle on
the intended scope of the interface, and then it should be made
internally consistent.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH v4 08/14] hvmloader: Locate the BIOS blob
2016-04-05 12:59 ` Jan Beulich
2016-04-05 14:05 ` Roger Pau Monné
@ 2016-04-07 15:10 ` Anthony PERARD
2016-04-07 15:30 ` Jan Beulich
1 sibling, 1 reply; 73+ messages in thread
From: Anthony PERARD @ 2016-04-07 15:10 UTC (permalink / raw)
To: Jan Beulich
Cc: Wei Liu, Stefano Stabellini, Andrew Cooper, Ian Jackson,
xen-devel, Keir Fraser, Roger Pau Monne
On Tue, Apr 05, 2016 at 06:59:03AM -0600, Jan Beulich wrote:
> >>> On 14.03.16 at 18:55, <anthony.perard@citrix.com> wrote:
> > --- a/tools/firmware/hvmloader/hvmloader.c
> > +++ b/tools/firmware/hvmloader/hvmloader.c
> > @@ -292,8 +322,16 @@ 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, "bios");
>
> Isn't "bios" a bit vague, as there could be multiple (system, video,
> add-on card)?
Maybe a bit, also this can be use to load OVMF which is not technically a
BIOS. I guess a better name would be "firmware" or "system firmware".
On the other hand, the name "bios" is used in few places, in hvmloader, in
libxl (to choose between seabios/ovmf).
--
Anthony PERARD
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH v4 08/14] hvmloader: Locate the BIOS blob
2016-04-07 15:10 ` Anthony PERARD
@ 2016-04-07 15:30 ` Jan Beulich
0 siblings, 0 replies; 73+ messages in thread
From: Jan Beulich @ 2016-04-07 15:30 UTC (permalink / raw)
To: Anthony PERARD
Cc: Wei Liu, Stefano Stabellini, Andrew Cooper, Ian Jackson,
xen-devel, Keir Fraser, Roger Pau Monne
>>> On 07.04.16 at 17:10, <anthony.perard@citrix.com> wrote:
> On Tue, Apr 05, 2016 at 06:59:03AM -0600, Jan Beulich wrote:
>> >>> On 14.03.16 at 18:55, <anthony.perard@citrix.com> wrote:
>> > --- a/tools/firmware/hvmloader/hvmloader.c
>> > +++ b/tools/firmware/hvmloader/hvmloader.c
>> > @@ -292,8 +322,16 @@ 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, "bios");
>>
>> Isn't "bios" a bit vague, as there could be multiple (system, video,
>> add-on card)?
>
> Maybe a bit, also this can be use to load OVMF which is not technically a
> BIOS. I guess a better name would be "firmware" or "system firmware".
>
> On the other hand, the name "bios" is used in few places, in hvmloader, in
> libxl (to choose between seabios/ovmf).
Perhaps that's a result of how that code evolved? I'm not heavily
objecting to using "bios" here, but I think this being a newly
introduced name it could as well be made more correct.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH v4 03/14] configure: #define SEABIOS_PATH and OVMF_PATH
2016-03-14 17:55 ` [PATCH v4 03/14] configure: #define SEABIOS_PATH and OVMF_PATH Anthony PERARD
2016-03-16 0:20 ` Konrad Rzeszutek Wilk
@ 2016-04-08 13:38 ` Wei Liu
1 sibling, 0 replies; 73+ messages in thread
From: Wei Liu @ 2016-04-08 13:38 UTC (permalink / raw)
To: Anthony PERARD; +Cc: Wei Liu, Stefano Stabellini, Ian Jackson, xen-devel
On Mon, Mar 14, 2016 at 05:55:38PM +0000, Anthony PERARD wrote:
> Those paths are to be used by libxl, in order to load the firmware in
> memory. If a system path is not define via --with-system-seabios or
> --with-system-ovmf, then this default to the Xen firmware directory.
>
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
>
Acked-by: Wei Liu <wei.liu2@citrix.com>
> ---
> Please, run ./autogen.sh on this patch.
> ---
> tools/configure.ac | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/tools/configure.ac b/tools/configure.ac
> index 5b5dda4..7e5452e 100644
> --- a/tools/configure.ac
> +++ b/tools/configure.ac
> @@ -218,6 +218,9 @@ AC_ARG_WITH([system-seabios],
> esac
> ],[])
> AC_SUBST(seabios_path)
> +AC_DEFINE_UNQUOTED([SEABIOS_PATH],
> + ["${seabios_path:-$XENFIRMWAREDIR/seabios.bin}"],
> + [SeaBIOS path])
>
> AC_ARG_WITH([system-ovmf],
> AS_HELP_STRING([--with-system-ovmf@<:@=PATH@:>@],
> @@ -229,6 +232,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 [flat|nested] 73+ messages in thread
* Re: [PATCH v4 14/14] configure: do not depend on SEABIOS_PATH or OVMF_PATH ...
2016-03-14 17:55 ` [PATCH v4 14/14] configure: do not depend on SEABIOS_PATH or OVMF_PATH Anthony PERARD
2016-03-16 1:40 ` Konrad Rzeszutek Wilk
@ 2016-04-08 13:38 ` Wei Liu
1 sibling, 0 replies; 73+ messages in thread
From: Wei Liu @ 2016-04-08 13:38 UTC (permalink / raw)
To: Anthony PERARD; +Cc: Wei Liu, Stefano Stabellini, Ian Jackson, xen-devel
On Mon, Mar 14, 2016 at 05:55:49PM +0000, Anthony PERARD wrote:
> ... to compile SeaBIOS and OVMF. Only depends 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>
>
Acked-by: Wei Liu <wei.liu2@citrix.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH v4 04/14] firmware/makefile: install BIOS blob ...
2016-03-14 17:55 ` [PATCH v4 04/14] firmware/makefile: install BIOS blob Anthony PERARD
2016-03-16 0:26 ` Konrad Rzeszutek Wilk
2016-03-17 17:37 ` Doug Goldstein
@ 2016-04-18 14:31 ` Doug Goldstein
2016-04-19 13:11 ` Stefano Stabellini
2 siblings, 1 reply; 73+ messages in thread
From: Doug Goldstein @ 2016-04-18 14:31 UTC (permalink / raw)
To: Anthony PERARD, xen-devel; +Cc: Ian Jackson, Wei Liu, Stefano Stabellini
[-- Attachment #1.1.1: Type: text/plain, Size: 2250 bytes --]
On 3/14/16 5:55 PM, Anthony PERARD wrote:
> ... into the firmware directory, along with hvmloader.
>
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
> 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 | 13 +++++++++++++
> 1 file changed, 13 insertions(+)
>
> diff --git a/tools/firmware/Makefile b/tools/firmware/Makefile
> index 6cc86ce..6a37758 100644
> --- a/tools/firmware/Makefile
> +++ b/tools/firmware/Makefile
> @@ -19,6 +19,9 @@ SUBDIRS-y += hvmloader
>
> LD32BIT-$(CONFIG_FreeBSD) := LD32BIT_FLAG=-melf_i386_fbsd
>
> +SEABIOS_ROM := seabios-dir/out/bios.bin
> +OVMF_ROM := ovmf-dir/ovmf.bin
> +
> ovmf-dir:
> GIT=$(GIT) $(XEN_ROOT)/scripts/git-checkout.sh $(OVMF_UPSTREAM_URL) $(OVMF_UPSTREAM_REVISION) ovmf-dir
> cp ovmf-makefile ovmf-dir/Makefile;
> @@ -45,6 +48,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_ROM) $(INST_DIR)/seabios.bin
> +endif
> +endif
> +ifeq ($(CONFIG_OVMF),y)
> +ifeq ($(OVMF_PATH),)
> + $(INSTALL_DATA) $(OVMF_ROM) $(INST_DIR)/ovmf.bin
> +endif
> +endif
>
> .PHONY: clean
> clean: subdirs-clean
>
So I'm going to toss this out there but what if we don't install these
at all? We talked about reducing the scope that the Xen Security team
had to maintain. What if we just state that SeaBIOS and/or OVMF are
dependencies? All the downstream distros don't use the pre-built
binaries from Xen and build it themselves. For plain Xen users we just
add that to the list of dependencies.
I think SeaBIOS and OVMF are a lot more low risk than something like
QEMU since they have a very clear target so they're a lot more likely to
remain stable. SeaBIOS also has a fairly low level of churn, especially
on stable branches.
Just a thought.
--
Doug Goldstein
[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 959 bytes --]
[-- Attachment #2: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH v4 04/14] firmware/makefile: install BIOS blob ...
2016-04-18 14:31 ` Doug Goldstein
@ 2016-04-19 13:11 ` Stefano Stabellini
0 siblings, 0 replies; 73+ messages in thread
From: Stefano Stabellini @ 2016-04-19 13:11 UTC (permalink / raw)
To: Doug Goldstein
Cc: Anthony PERARD, Wei Liu, Stefano Stabellini, Ian Jackson, xen-devel
On Mon, 18 Apr 2016, Doug Goldstein wrote:
> On 3/14/16 5:55 PM, Anthony PERARD wrote:
> > ... into the firmware directory, along with hvmloader.
> >
> > Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> > ---
> > 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 | 13 +++++++++++++
> > 1 file changed, 13 insertions(+)
> >
> > diff --git a/tools/firmware/Makefile b/tools/firmware/Makefile
> > index 6cc86ce..6a37758 100644
> > --- a/tools/firmware/Makefile
> > +++ b/tools/firmware/Makefile
> > @@ -19,6 +19,9 @@ SUBDIRS-y += hvmloader
> >
> > LD32BIT-$(CONFIG_FreeBSD) := LD32BIT_FLAG=-melf_i386_fbsd
> >
> > +SEABIOS_ROM := seabios-dir/out/bios.bin
> > +OVMF_ROM := ovmf-dir/ovmf.bin
> > +
> > ovmf-dir:
> > GIT=$(GIT) $(XEN_ROOT)/scripts/git-checkout.sh $(OVMF_UPSTREAM_URL) $(OVMF_UPSTREAM_REVISION) ovmf-dir
> > cp ovmf-makefile ovmf-dir/Makefile;
> > @@ -45,6 +48,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_ROM) $(INST_DIR)/seabios.bin
> > +endif
> > +endif
> > +ifeq ($(CONFIG_OVMF),y)
> > +ifeq ($(OVMF_PATH),)
> > + $(INSTALL_DATA) $(OVMF_ROM) $(INST_DIR)/ovmf.bin
> > +endif
> > +endif
> >
> > .PHONY: clean
> > clean: subdirs-clean
> >
>
> So I'm going to toss this out there but what if we don't install these
> at all? We talked about reducing the scope that the Xen Security team
> had to maintain. What if we just state that SeaBIOS and/or OVMF are
> dependencies? All the downstream distros don't use the pre-built
> binaries from Xen and build it themselves. For plain Xen users we just
> add that to the list of dependencies.
>
> I think SeaBIOS and OVMF are a lot more low risk than something like
> QEMU since they have a very clear target so they're a lot more likely to
> remain stable. SeaBIOS also has a fairly low level of churn, especially
> on stable branches.
Just to add to your argument, Raisin already supports both SeaBIOS and
OVMF. That's another way for users to build them if they want to.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 73+ messages in thread
end of thread, other threads:[~2016-04-19 13:11 UTC | newest]
Thread overview: 73+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-03-14 17:55 [PATCH v4 00/14] Load BIOS via toolstack instead of been embedded in hvmloader Anthony PERARD
2016-03-14 17:55 ` [PATCH v4 01/14] libxc: Rework extra module initialisation Anthony PERARD
2016-03-16 0:06 ` Konrad Rzeszutek Wilk
2016-03-17 16:24 ` Anthony PERARD
2016-03-14 17:55 ` [PATCH v4 02/14] libxc: Prepare a start info structure for hvmloader Anthony PERARD
2016-03-16 0:18 ` Konrad Rzeszutek Wilk
2016-03-16 18:01 ` Boris Ostrovsky
2016-03-17 16:48 ` Anthony PERARD
2016-03-17 16:28 ` Anthony PERARD
2016-03-14 17:55 ` [PATCH v4 03/14] configure: #define SEABIOS_PATH and OVMF_PATH Anthony PERARD
2016-03-16 0:20 ` Konrad Rzeszutek Wilk
2016-04-08 13:38 ` Wei Liu
2016-03-14 17:55 ` [PATCH v4 04/14] firmware/makefile: install BIOS blob Anthony PERARD
2016-03-16 0:26 ` Konrad Rzeszutek Wilk
2016-03-16 8:54 ` Dario Faggioli
2016-03-16 8:56 ` Konrad Rzeszutek Wilk
2016-03-17 16:58 ` Anthony PERARD
2016-03-17 17:37 ` Doug Goldstein
2016-03-17 18:33 ` Anthony PERARD
2016-03-18 21:11 ` Jim Fehlig
2016-03-19 0:43 ` Doug Goldstein
2016-04-18 14:31 ` Doug Goldstein
2016-04-19 13:11 ` Stefano Stabellini
2016-03-14 17:55 ` [PATCH v4 05/14] libxl: Load guest BIOS from file Anthony PERARD
2016-03-16 0:53 ` Konrad Rzeszutek Wilk
2016-03-16 9:27 ` Dario Faggioli
2016-03-17 17:24 ` Anthony PERARD
2016-03-14 17:55 ` [PATCH v4 06/14] xen: Move the hvm_start_info C representation from libxc to public/xen.h Anthony PERARD
2016-03-15 8:09 ` Jan Beulich
2016-03-16 0:59 ` Konrad Rzeszutek Wilk
2016-03-16 1:00 ` Konrad Rzeszutek Wilk
2016-03-16 8:32 ` Jan Beulich
2016-03-21 17:04 ` Roger Pau Monné
2016-03-21 17:21 ` Jan Beulich
2016-03-14 17:55 ` [PATCH v4 07/14] hvmloader: Grab the hvm_start_info pointer Anthony PERARD
2016-03-16 1:07 ` Konrad Rzeszutek Wilk
2016-04-05 12:43 ` Jan Beulich
2016-03-14 17:55 ` [PATCH v4 08/14] hvmloader: Locate the BIOS blob Anthony PERARD
2016-03-16 1:14 ` Konrad Rzeszutek Wilk
2016-03-17 17:46 ` Anthony PERARD
2016-03-17 17:57 ` Konrad Rzeszutek Wilk
2016-03-18 7:34 ` Jan Beulich
2016-04-05 12:59 ` Jan Beulich
2016-04-05 14:05 ` Roger Pau Monné
2016-04-05 14:23 ` Jan Beulich
2016-04-07 15:10 ` Anthony PERARD
2016-04-07 15:30 ` Jan Beulich
2016-03-14 17:55 ` [PATCH v4 09/14] hvmloader: Check modules whereabouts in perform_tests Anthony PERARD
2016-03-16 1:23 ` Konrad Rzeszutek Wilk
2016-03-17 18:08 ` Anthony PERARD
2016-04-05 13:07 ` Jan Beulich
2016-03-14 17:55 ` [PATCH v4 10/14] hvmloader: Load SeaBIOS from hvm_start_info modules Anthony PERARD
2016-03-16 1:27 ` Konrad Rzeszutek Wilk
2016-03-16 1:27 ` Konrad Rzeszutek Wilk
2016-04-05 13:11 ` Jan Beulich
2016-03-14 17:55 ` [PATCH v4 11/14] hvmloader: Load OVMF from modules Anthony PERARD
2016-03-16 1:36 ` Konrad Rzeszutek Wilk
2016-04-05 13:16 ` Jan Beulich
2016-03-14 17:55 ` [PATCH v4 12/14] hvmloader: Specific bios_load function required Anthony PERARD
2016-03-16 1:38 ` Konrad Rzeszutek Wilk
2016-03-17 18:25 ` Anthony PERARD
2016-03-14 17:55 ` [PATCH v4 13/14] hvmloader: Always build-in SeaBIOS and OVMF loader Anthony PERARD
2016-03-14 17:55 ` [PATCH v4 14/14] configure: do not depend on SEABIOS_PATH or OVMF_PATH Anthony PERARD
2016-03-16 1:40 ` Konrad Rzeszutek Wilk
2016-04-08 13:38 ` Wei Liu
2016-03-24 17:55 ` [PATCH v4 00/14] Load BIOS via toolstack instead of been embedded in hvmloader Jim Fehlig
2016-03-30 17:22 ` Jim Fehlig
2016-03-31 7:20 ` Jan Beulich
[not found] ` <56FCEBCD02000078000E19BF@suse.com>
2016-03-31 14:36 ` Jim Fehlig
2016-03-31 16:49 ` George Dunlap
2016-04-01 9:12 ` George Dunlap
2016-04-01 14:24 ` Konrad Rzeszutek Wilk
2016-04-01 20:06 ` Jim Fehlig
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).