Re: [Xen-devel] [PATCH v5 01/14] libxc: Rework extra module initialisation

[Anthony PERARD]

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, , name, 0, module->length) )
> > > > +goto err;
> > > > +dest = xc_dom_seg_to_ptr(dom, );
> > > > +if ( dest == NULL )
> > > > +{
> > > > +DOMPRINTF("%s: xc_dom_seg_to_ptr(dom, ) => 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

[Wei Liu]

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, , name, 0, module->length) )
> > > +goto err;
> > > +dest = xc_dom_seg_to_ptr(dom, );
> > > +if ( dest == NULL )
> > > +{
> > > +DOMPRINTF("%s: xc_dom_seg_to_ptr(dom, ) => 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

[Anthony PERARD]

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, , name, 0, module->length) )
> > +goto err;
> > +dest = xc_dom_seg_to_ptr(dom, );
> > +if ( dest == NULL )
> > +{
> > +DOMPRINTF("%s: xc_dom_seg_to_ptr(dom, ) => 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

[Wei Liu]

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, , name, 0, module->length) )
> +goto err;
> +dest = xc_dom_seg_to_ptr(dom, );
> +if ( dest == NULL )
> +{
> +DOMPRINTF("%s: xc_dom_seg_to_ptr(dom, ) => 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

[Anthony PERARD]

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, , name, 0, module->length) )
+goto err;
+dest = xc_dom_seg_to_ptr(dom, );
+if ( dest == NULL )
+{
+DOMPRINTF("%s: xc_dom_seg_to_ptr(dom, ) => 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;