Re: [Xen-devel] [PATCH v5 01/14] libxc: Rework extra module initialisation
On Fri, Jul 08, 2016 at 12:29:36PM +0100, Wei Liu wrote: > On Fri, Jul 08, 2016 at 11:52:08AM +0100, Anthony PERARD wrote: > > On Thu, Jul 07, 2016 at 03:55:23PM +0100, Wei Liu wrote: > > > On Wed, Jun 22, 2016 at 06:15:32PM +0100, Anthony PERARD wrote: > > > > diff --git a/tools/libxc/xc_dom_hvmloader.c > > > > b/tools/libxc/xc_dom_hvmloader.c > > > > index 330d5e8..da8b995 100644 > > > > --- a/tools/libxc/xc_dom_hvmloader.c > > > > +++ b/tools/libxc/xc_dom_hvmloader.c > > > > @@ -129,98 +129,52 @@ static elf_errorstatus > > > > xc_dom_parse_hvm_kernel(struct xc_dom_image *dom) > > > > return rc; > > > > } > > > > > > > > -static int modules_init(struct xc_dom_image *dom, > > > > -uint64_t vend, struct elf_binary *elf, > > > > -uint64_t *mstart_out, uint64_t *mend_out) > > > > +static int module_init_one(struct xc_dom_image *dom, > > > > + struct xc_hvm_firmware_module *module, > > > > + char *name) > > > > { > > > > -#define MODULE_ALIGN 1UL << 7 > > > > -#define MB_ALIGN 1UL << 20 > > > > -#define MKALIGN(x, a) (((uint64_t)(x) + (a) - 1) & ~(uint64_t)((a) - > > > > 1)) > > > > -uint64_t total_len = 0, offset1 = 0; > > > > +struct xc_dom_seg seg; > > > > +void *dest; > > > > > > > > -if ( dom->acpi_module.length == 0 && dom->smbios_module.length == > > > > 0 ) > > > > -return 0; > > > > - > > > > -/* Find the total length for the firmware modules with a > > > > reasonable large > > > > - * alignment size to align each the modules. > > > > - */ > > > > -total_len = MKALIGN(dom->acpi_module.length, MODULE_ALIGN); > > > > -offset1 = total_len; > > > > -total_len += MKALIGN(dom->smbios_module.length, MODULE_ALIGN); > > > > - > > > > -/* Want to place the modules 1Mb+change behind the loader image. */ > > > > -*mstart_out = MKALIGN(elf->pend, MB_ALIGN) + (MB_ALIGN); > > > > -*mend_out = *mstart_out + total_len; > > > > - > > > > -if ( *mend_out > vend ) > > > > -return -1; > > > > - > > > > -if ( dom->acpi_module.length != 0 ) > > > > -dom->acpi_module.guest_addr_out = *mstart_out; > > > > -if ( dom->smbios_module.length != 0 ) > > > > -dom->smbios_module.guest_addr_out = *mstart_out + offset1; > > > > +if ( module->length ) > > > > +{ > > > > +if ( xc_dom_alloc_segment(dom, &seg, name, 0, module->length) ) > > > > +goto err; > > > > +dest = xc_dom_seg_to_ptr(dom, &seg); > > > > +if ( dest == NULL ) > > > > +{ > > > > +DOMPRINTF("%s: xc_dom_seg_to_ptr(dom, &seg) => NULL", > > > > + __FUNCTION__); > > > > +goto err; > > > > +} > > > > +memcpy(dest, module->data, module->length); > > > > +module->guest_addr_out = seg.vstart; > > > > +if ( module->guest_addr_out > UINT32_MAX || > > > > + module->guest_addr_out + module->length > UINT32_MAX ) > > > > +{ > > > > +DOMPRINTF("%s: Module %s would be loaded abrove 4GB", > > > > + __FUNCTION__, name); > > > > +goto err; > > > > +} > > > > > > One question: > > > > > > Can this check also account for MMIO hole below 4G? Maybe use > > > dom->mmio_size? > > > > Yes, I guess I can check against dom->mmio_start. Should I also check > > that mmio_start have reasonable value? (<4G, and not 0x0) Or is > > mmio_start is already supposed to have a good value? > > > > mmio_start should already have a sane value here -- or at least I hope > so. The sanity of mmio_start should be checked where it is assigned. Ok, I'll use dom->mmio_start instead of UINT32_MAX, and probably add an assert() on the values expected in mmio_start. Thanks, -- Anthony PERARD ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v5 01/14] libxc: Rework extra module initialisation
On Fri, Jul 08, 2016 at 11:52:08AM +0100, Anthony PERARD wrote: > On Thu, Jul 07, 2016 at 03:55:23PM +0100, Wei Liu wrote: > > On Wed, Jun 22, 2016 at 06:15:32PM +0100, Anthony PERARD wrote: > > > This patch use xc_dom_alloc_segment() to allocate the memory space for the > > > ACPI modules and the SMBIOS modules. This is to replace the arbitrary > > > placement of 1MB (+ extra for MB alignement) after the hvmloader image. > > > > > > This patch can help if one add extra ACPI table and hvmloader contain > > > OVMF (OVMF is a 2MB binary), as in that case the extra ACPI table could > > > easily be loaded past the address 4MB, but hvmloader use a range of > > > memory from 4MB to 10MB to perform tests and in the process, clears the > > > memory, before loading the modules. > > > > > > Signed-off-by: Anthony PERARD > > > --- > > > Changes in V5: > > > - rewrite patch description > > > > > > Changes in V4: > > > - check that guest_addr_out have a reasonable value. > > > > > > New patch in V3. > > > --- > > > tools/libxc/xc_dom_hvmloader.c | 131 > > > - > > > 1 file changed, 38 insertions(+), 93 deletions(-) > > > > > > diff --git a/tools/libxc/xc_dom_hvmloader.c > > > b/tools/libxc/xc_dom_hvmloader.c > > > index 330d5e8..da8b995 100644 > > > --- a/tools/libxc/xc_dom_hvmloader.c > > > +++ b/tools/libxc/xc_dom_hvmloader.c > > > @@ -129,98 +129,52 @@ static elf_errorstatus > > > xc_dom_parse_hvm_kernel(struct xc_dom_image *dom) > > > return rc; > > > } > > > > > > -static int modules_init(struct xc_dom_image *dom, > > > -uint64_t vend, struct elf_binary *elf, > > > -uint64_t *mstart_out, uint64_t *mend_out) > > > +static int module_init_one(struct xc_dom_image *dom, > > > + struct xc_hvm_firmware_module *module, > > > + char *name) > > > { > > > -#define MODULE_ALIGN 1UL << 7 > > > -#define MB_ALIGN 1UL << 20 > > > -#define MKALIGN(x, a) (((uint64_t)(x) + (a) - 1) & ~(uint64_t)((a) - 1)) > > > -uint64_t total_len = 0, offset1 = 0; > > > +struct xc_dom_seg seg; > > > +void *dest; > > > > > > -if ( dom->acpi_module.length == 0 && dom->smbios_module.length == 0 ) > > > -return 0; > > > - > > > -/* Find the total length for the firmware modules with a reasonable > > > large > > > - * alignment size to align each the modules. > > > - */ > > > -total_len = MKALIGN(dom->acpi_module.length, MODULE_ALIGN); > > > -offset1 = total_len; > > > -total_len += MKALIGN(dom->smbios_module.length, MODULE_ALIGN); > > > - > > > -/* Want to place the modules 1Mb+change behind the loader image. */ > > > -*mstart_out = MKALIGN(elf->pend, MB_ALIGN) + (MB_ALIGN); > > > -*mend_out = *mstart_out + total_len; > > > - > > > -if ( *mend_out > vend ) > > > -return -1; > > > - > > > -if ( dom->acpi_module.length != 0 ) > > > -dom->acpi_module.guest_addr_out = *mstart_out; > > > -if ( dom->smbios_module.length != 0 ) > > > -dom->smbios_module.guest_addr_out = *mstart_out + offset1; > > > +if ( module->length ) > > > +{ > > > +if ( xc_dom_alloc_segment(dom, &seg, name, 0, module->length) ) > > > +goto err; > > > +dest = xc_dom_seg_to_ptr(dom, &seg); > > > +if ( dest == NULL ) > > > +{ > > > +DOMPRINTF("%s: xc_dom_seg_to_ptr(dom, &seg) => NULL", > > > + __FUNCTION__); > > > +goto err; > > > +} > > > +memcpy(dest, module->data, module->length); > > > +module->guest_addr_out = seg.vstart; > > > +if ( module->guest_addr_out > UINT32_MAX || > > > + module->guest_addr_out + module->length > UINT32_MAX ) > > > +{ > > > +DOMPRINTF("%s: Module %s would be loaded abrove 4GB", > > > + __FUNCTION__, name); > > > +goto err; > > > +} > > > > One question: > > > > Can this check also account for MMIO hole below 4G? Maybe use > > dom->mmio_size? > > Yes, I guess I can check against dom->mmio_start. Should I also check > that mmio_start have reasonable value? (<4G, and not 0x0) Or is > mmio_start is already supposed to have a good value? > mmio_start should already have a sane value here -- or at least I hope so. The sanity of mmio_start should be checked where it is assigned. Wei. > > Other than this, this patch looks good. > > Thanks, > > -- > Anthony PERARD ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v5 01/14] libxc: Rework extra module initialisation
On Thu, Jul 07, 2016 at 03:55:23PM +0100, Wei Liu wrote: > On Wed, Jun 22, 2016 at 06:15:32PM +0100, Anthony PERARD wrote: > > This patch use xc_dom_alloc_segment() to allocate the memory space for the > > ACPI modules and the SMBIOS modules. This is to replace the arbitrary > > placement of 1MB (+ extra for MB alignement) after the hvmloader image. > > > > This patch can help if one add extra ACPI table and hvmloader contain > > OVMF (OVMF is a 2MB binary), as in that case the extra ACPI table could > > easily be loaded past the address 4MB, but hvmloader use a range of > > memory from 4MB to 10MB to perform tests and in the process, clears the > > memory, before loading the modules. > > > > Signed-off-by: Anthony PERARD > > --- > > Changes in V5: > > - rewrite patch description > > > > Changes in V4: > > - check that guest_addr_out have a reasonable value. > > > > New patch in V3. > > --- > > tools/libxc/xc_dom_hvmloader.c | 131 > > - > > 1 file changed, 38 insertions(+), 93 deletions(-) > > > > diff --git a/tools/libxc/xc_dom_hvmloader.c b/tools/libxc/xc_dom_hvmloader.c > > index 330d5e8..da8b995 100644 > > --- a/tools/libxc/xc_dom_hvmloader.c > > +++ b/tools/libxc/xc_dom_hvmloader.c > > @@ -129,98 +129,52 @@ static elf_errorstatus xc_dom_parse_hvm_kernel(struct > > xc_dom_image *dom) > > return rc; > > } > > > > -static int modules_init(struct xc_dom_image *dom, > > -uint64_t vend, struct elf_binary *elf, > > -uint64_t *mstart_out, uint64_t *mend_out) > > +static int module_init_one(struct xc_dom_image *dom, > > + struct xc_hvm_firmware_module *module, > > + char *name) > > { > > -#define MODULE_ALIGN 1UL << 7 > > -#define MB_ALIGN 1UL << 20 > > -#define MKALIGN(x, a) (((uint64_t)(x) + (a) - 1) & ~(uint64_t)((a) - 1)) > > -uint64_t total_len = 0, offset1 = 0; > > +struct xc_dom_seg seg; > > +void *dest; > > > > -if ( dom->acpi_module.length == 0 && dom->smbios_module.length == 0 ) > > -return 0; > > - > > -/* Find the total length for the firmware modules with a reasonable > > large > > - * alignment size to align each the modules. > > - */ > > -total_len = MKALIGN(dom->acpi_module.length, MODULE_ALIGN); > > -offset1 = total_len; > > -total_len += MKALIGN(dom->smbios_module.length, MODULE_ALIGN); > > - > > -/* Want to place the modules 1Mb+change behind the loader image. */ > > -*mstart_out = MKALIGN(elf->pend, MB_ALIGN) + (MB_ALIGN); > > -*mend_out = *mstart_out + total_len; > > - > > -if ( *mend_out > vend ) > > -return -1; > > - > > -if ( dom->acpi_module.length != 0 ) > > -dom->acpi_module.guest_addr_out = *mstart_out; > > -if ( dom->smbios_module.length != 0 ) > > -dom->smbios_module.guest_addr_out = *mstart_out + offset1; > > +if ( module->length ) > > +{ > > +if ( xc_dom_alloc_segment(dom, &seg, name, 0, module->length) ) > > +goto err; > > +dest = xc_dom_seg_to_ptr(dom, &seg); > > +if ( dest == NULL ) > > +{ > > +DOMPRINTF("%s: xc_dom_seg_to_ptr(dom, &seg) => NULL", > > + __FUNCTION__); > > +goto err; > > +} > > +memcpy(dest, module->data, module->length); > > +module->guest_addr_out = seg.vstart; > > +if ( module->guest_addr_out > UINT32_MAX || > > + module->guest_addr_out + module->length > UINT32_MAX ) > > +{ > > +DOMPRINTF("%s: Module %s would be loaded abrove 4GB", > > + __FUNCTION__, name); > > +goto err; > > +} > > One question: > > Can this check also account for MMIO hole below 4G? Maybe use > dom->mmio_size? Yes, I guess I can check against dom->mmio_start. Should I also check that mmio_start have reasonable value? (<4G, and not 0x0) Or is mmio_start is already supposed to have a good value? > Other than this, this patch looks good. Thanks, -- Anthony PERARD ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v5 01/14] libxc: Rework extra module initialisation
On Wed, Jun 22, 2016 at 06:15:32PM +0100, Anthony PERARD wrote: > This patch use xc_dom_alloc_segment() to allocate the memory space for the > ACPI modules and the SMBIOS modules. This is to replace the arbitrary > placement of 1MB (+ extra for MB alignement) after the hvmloader image. > > This patch can help if one add extra ACPI table and hvmloader contain > OVMF (OVMF is a 2MB binary), as in that case the extra ACPI table could > easily be loaded past the address 4MB, but hvmloader use a range of > memory from 4MB to 10MB to perform tests and in the process, clears the > memory, before loading the modules. > > Signed-off-by: Anthony PERARD > --- > Changes in V5: > - rewrite patch description > > Changes in V4: > - check that guest_addr_out have a reasonable value. > > New patch in V3. > --- > tools/libxc/xc_dom_hvmloader.c | 131 > - > 1 file changed, 38 insertions(+), 93 deletions(-) > > diff --git a/tools/libxc/xc_dom_hvmloader.c b/tools/libxc/xc_dom_hvmloader.c > index 330d5e8..da8b995 100644 > --- a/tools/libxc/xc_dom_hvmloader.c > +++ b/tools/libxc/xc_dom_hvmloader.c > @@ -129,98 +129,52 @@ static elf_errorstatus xc_dom_parse_hvm_kernel(struct > xc_dom_image *dom) > return rc; > } > > -static int modules_init(struct xc_dom_image *dom, > -uint64_t vend, struct elf_binary *elf, > -uint64_t *mstart_out, uint64_t *mend_out) > +static int module_init_one(struct xc_dom_image *dom, > + struct xc_hvm_firmware_module *module, > + char *name) > { > -#define MODULE_ALIGN 1UL << 7 > -#define MB_ALIGN 1UL << 20 > -#define MKALIGN(x, a) (((uint64_t)(x) + (a) - 1) & ~(uint64_t)((a) - 1)) > -uint64_t total_len = 0, offset1 = 0; > +struct xc_dom_seg seg; > +void *dest; > > -if ( dom->acpi_module.length == 0 && dom->smbios_module.length == 0 ) > -return 0; > - > -/* Find the total length for the firmware modules with a reasonable large > - * alignment size to align each the modules. > - */ > -total_len = MKALIGN(dom->acpi_module.length, MODULE_ALIGN); > -offset1 = total_len; > -total_len += MKALIGN(dom->smbios_module.length, MODULE_ALIGN); > - > -/* Want to place the modules 1Mb+change behind the loader image. */ > -*mstart_out = MKALIGN(elf->pend, MB_ALIGN) + (MB_ALIGN); > -*mend_out = *mstart_out + total_len; > - > -if ( *mend_out > vend ) > -return -1; > - > -if ( dom->acpi_module.length != 0 ) > -dom->acpi_module.guest_addr_out = *mstart_out; > -if ( dom->smbios_module.length != 0 ) > -dom->smbios_module.guest_addr_out = *mstart_out + offset1; > +if ( module->length ) > +{ > +if ( xc_dom_alloc_segment(dom, &seg, name, 0, module->length) ) > +goto err; > +dest = xc_dom_seg_to_ptr(dom, &seg); > +if ( dest == NULL ) > +{ > +DOMPRINTF("%s: xc_dom_seg_to_ptr(dom, &seg) => NULL", > + __FUNCTION__); > +goto err; > +} > +memcpy(dest, module->data, module->length); > +module->guest_addr_out = seg.vstart; > +if ( module->guest_addr_out > UINT32_MAX || > + module->guest_addr_out + module->length > UINT32_MAX ) > +{ > +DOMPRINTF("%s: Module %s would be loaded abrove 4GB", > + __FUNCTION__, name); > +goto err; > +} One question: Can this check also account for MMIO hole below 4G? Maybe use dom->mmio_size? Other than this, this patch looks good. Wei. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH v5 01/14] libxc: Rework extra module initialisation
This patch use xc_dom_alloc_segment() to allocate the memory space for the ACPI modules and the SMBIOS modules. This is to replace the arbitrary placement of 1MB (+ extra for MB alignement) after the hvmloader image. This patch can help if one add extra ACPI table and hvmloader contain OVMF (OVMF is a 2MB binary), as in that case the extra ACPI table could easily be loaded past the address 4MB, but hvmloader use a range of memory from 4MB to 10MB to perform tests and in the process, clears the memory, before loading the modules. Signed-off-by: Anthony PERARD --- Changes in V5: - rewrite patch description Changes in V4: - check that guest_addr_out have a reasonable value. New patch in V3. --- tools/libxc/xc_dom_hvmloader.c | 131 - 1 file changed, 38 insertions(+), 93 deletions(-) diff --git a/tools/libxc/xc_dom_hvmloader.c b/tools/libxc/xc_dom_hvmloader.c index 330d5e8..da8b995 100644 --- a/tools/libxc/xc_dom_hvmloader.c +++ b/tools/libxc/xc_dom_hvmloader.c @@ -129,98 +129,52 @@ static elf_errorstatus xc_dom_parse_hvm_kernel(struct xc_dom_image *dom) return rc; } -static int modules_init(struct xc_dom_image *dom, -uint64_t vend, struct elf_binary *elf, -uint64_t *mstart_out, uint64_t *mend_out) +static int module_init_one(struct xc_dom_image *dom, + struct xc_hvm_firmware_module *module, + char *name) { -#define MODULE_ALIGN 1UL << 7 -#define MB_ALIGN 1UL << 20 -#define MKALIGN(x, a) (((uint64_t)(x) + (a) - 1) & ~(uint64_t)((a) - 1)) -uint64_t total_len = 0, offset1 = 0; +struct xc_dom_seg seg; +void *dest; -if ( dom->acpi_module.length == 0 && dom->smbios_module.length == 0 ) -return 0; - -/* Find the total length for the firmware modules with a reasonable large - * alignment size to align each the modules. - */ -total_len = MKALIGN(dom->acpi_module.length, MODULE_ALIGN); -offset1 = total_len; -total_len += MKALIGN(dom->smbios_module.length, MODULE_ALIGN); - -/* Want to place the modules 1Mb+change behind the loader image. */ -*mstart_out = MKALIGN(elf->pend, MB_ALIGN) + (MB_ALIGN); -*mend_out = *mstart_out + total_len; - -if ( *mend_out > vend ) -return -1; - -if ( dom->acpi_module.length != 0 ) -dom->acpi_module.guest_addr_out = *mstart_out; -if ( dom->smbios_module.length != 0 ) -dom->smbios_module.guest_addr_out = *mstart_out + offset1; +if ( module->length ) +{ +if ( xc_dom_alloc_segment(dom, &seg, name, 0, module->length) ) +goto err; +dest = xc_dom_seg_to_ptr(dom, &seg); +if ( dest == NULL ) +{ +DOMPRINTF("%s: xc_dom_seg_to_ptr(dom, &seg) => NULL", + __FUNCTION__); +goto err; +} +memcpy(dest, module->data, module->length); +module->guest_addr_out = seg.vstart; +if ( module->guest_addr_out > UINT32_MAX || + module->guest_addr_out + module->length > UINT32_MAX ) +{ +DOMPRINTF("%s: Module %s would be loaded abrove 4GB", + __FUNCTION__, name); +goto err; +} +} return 0; +err: +return -1; } -static int loadmodules(struct xc_dom_image *dom, - uint64_t mstart, uint64_t mend, - uint32_t domid) +static int modules_init(struct xc_dom_image *dom) { -privcmd_mmap_entry_t *entries = NULL; -unsigned long pfn_start; -unsigned long pfn_end; -size_t pages; -uint32_t i; -uint8_t *dest; -int rc = -1; -xc_interface *xch = dom->xch; - -if ( mstart == 0 || mend == 0 ) -return 0; - -pfn_start = (unsigned long)(mstart >> PAGE_SHIFT); -pfn_end = (unsigned long)((mend + PAGE_SIZE - 1) >> PAGE_SHIFT); -pages = pfn_end - pfn_start; - -/* Map address space for module list. */ -entries = calloc(pages, sizeof(privcmd_mmap_entry_t)); -if ( entries == NULL ) -goto error_out; - -for ( i = 0; i < pages; i++ ) -entries[i].mfn = (mstart >> PAGE_SHIFT) + i; - -dest = xc_map_foreign_ranges( -xch, domid, pages << PAGE_SHIFT, PROT_READ | PROT_WRITE, 1 << PAGE_SHIFT, -entries, pages); -if ( dest == NULL ) -goto error_out; - -/* Zero the range so padding is clear between modules */ -memset(dest, 0, pages << PAGE_SHIFT); - -/* Load modules into range */ -if ( dom->acpi_module.length != 0 ) -{ -memcpy(dest, - dom->acpi_module.data, - dom->acpi_module.length); -} -if ( dom->smbios_module.length != 0 ) -{ -memcpy(dest + (dom->smbios_module.guest_addr_out - mstart), - dom->smbios_module.data, - dom->smbios_module.length); -} - -munmap(dest, pages << PAGE_SHIFT); -rc = 0; +int rc; - error_out: