[PATCH v3 0/4] x86/PVHv2: Add memory map pointer to hvm_start_info struct

[PATCH v3 0/4] x86/PVHv2: Add memory map pointer to hvm_start_info struct

[Maran Wilson]

Here is the patch series for updating the canonical definition of the
hvm_start_info struct corresponding to the discussion happening on the
linux-kernel and kvm mailing lists regarding Qemu/KVM use of the PVH
entry point:

   KVM: x86: Allow Qemu/KVM to use PVH entry point
   https://lkml.org/lkml/2018/2/28/1121

Patch 1 contains all the changes to the hvm_start_info struct and
patches 2-4 modify Xen to use the new memory map fields of the structure.

Changes since v2:
 * Better definition of the memory map types including addition of new
   symbols and tightening up the comments as suggested.
 * Added a couple of BUILD_BUG_ON() statements to the c code in patch #4
   to document and verify the relationship between these memory types
   and e820 types.

Changes since v1:
 * Made updates to code comments as suggested by Jan and Roger, including
   better definition of the memory map type field.   
 * Boris provided additional patches to populate the new fields in the
   hvm_start_info struct as Jan (and later Roger also) had requested.


Boris Ostrovsky (3):
  libxl: Move libxl__arch_domain_construct_memmap() earlier
  libxl: Store PVH guest's e820 map in xc_dom_image
  libxc: Pass e820 map to PVH guest via hvm_start_info

Maran Wilson (1):
  x86/PVHv2: Add memory map pointer to hvm_start_info struct

 tools/libxc/include/xc_dom.h                 |  8 +++-
 tools/libxc/xc_dom_x86.c                     | 30 ++++++++++++-
 tools/libxl/libxl_create.c                   |  2 +-
 tools/libxl/libxl_dom.c                      | 12 +++--
 tools/libxl/libxl_internal.h                 |  1 +
 tools/libxl/libxl_x86.c                      |  9 ++++
 xen/include/public/arch-x86/hvm/start_info.h | 66 +++++++++++++++++++++++++++-
 7 files changed, 121 insertions(+), 7 deletions(-)

-- 
1.8.3.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v3 1/4] x86/PVHv2: Add memory map pointer to hvm_start_info struct

[Maran Wilson]

The start info structure that is defined as part of the x86/HVM direct boot
ABI and used for starting Xen PVH guests would be more versatile if it also
included a way to pass information about the memory map to the guest. This
would allow KVM guests to share the same entry point.

Signed-off-by: Maran Wilson <maran.wilson@oracle.com>
---
 xen/include/public/arch-x86/hvm/start_info.h | 66 +++++++++++++++++++++++++++-
 1 file changed, 65 insertions(+), 1 deletion(-)

diff --git a/xen/include/public/arch-x86/hvm/start_info.h b/xen/include/public/arch-x86/hvm/start_info.h
index 6484159..f2e8ba6 100644
--- a/xen/include/public/arch-x86/hvm/start_info.h
+++ b/xen/include/public/arch-x86/hvm/start_info.h
@@ -33,8 +33,9 @@
  *    | magic          | Contains the magic value XEN_HVM_START_MAGIC_VALUE
  *    |                | ("xEn3" with the 0x80 bit of the "E" set).
  *  4 +----------------+
- *    | version        | Version of this structure. Current version is 0. New
+ *    | version        | Version of this structure. Current version is 1. New
  *    |                | versions are guaranteed to be backwards-compatible.
+ *    |                | For PV guests only 0 allowed, for PVH 0 or 1 allowed.
  *  8 +----------------+
  *    | flags          | SIF_xxx flags.
  * 12 +----------------+
@@ -48,6 +49,15 @@
  * 32 +----------------+
  *    | rsdp_paddr     | Physical address of the RSDP ACPI data structure.
  * 40 +----------------+
+ *    | memmap_paddr   | Physical address of the (optional) memory map. Only
+ *    |                | present in version 1 and newer of the structure.
+ * 48 +----------------+
+ *    | memmap_entries | Number of entries in the memory map table. Zero
+ *    |                | if there is no memory map being provided. Only
+ *    |                | present in version 1 and newer of the structure.
+ * 52 +----------------+
+ *    | reserved       | Version 1 and newer only.
+ * 56 +----------------+
  *
  * The layout of each entry in the module structure is the following:
  *
@@ -62,14 +72,53 @@
  *    | reserved       |
  * 32 +----------------+
  *
+ * The layout of each entry in the memory map table is as follows:
+ *
+ *  0 +----------------+
+ *    | addr           | Base address
+ *  8 +----------------+
+ *    | size           | Size of mapping in bytes
+ * 16 +----------------+
+ *    | type           | Type of mapping as defined between the hypervisor
+ *    |                | and guest it's starting. See XEN_HVM_MEMMAP_TYPE_*
+ *    |                | values below.
+ * 20 +----------------|
+ *    | reserved       |
+ * 24 +----------------+
+ *
  * The address and sizes are always a 64bit little endian unsigned integer.
  *
  * NB: Xen on x86 will always try to place all the data below the 4GiB
  * boundary.
+ *
+ * Version numbers of the hvm_start_info structure have evolved like this:
+ *
+ * Version 0:  Initial implementation.
+ *
+ * Version 1:  Added the memmap_paddr/memmap_entries fields (plus 4 bytes of
+ *             padding) to the end of the hvm_start_info struct. These new
+ *             fields can be used to pass a memory map to the guest. The
+ *             memory map is optional and so guests that understand version 1
+ *             of the structure must check that memmap_entries is non-zero
+ *             before trying to read the memory map.
  */
 #define XEN_HVM_START_MAGIC_VALUE 0x336ec578
 
 /*
+ * The values used in the type field of the memory map table entries are
+ * defined below and match the Address Range Types as defined in the "System
+ * Address Map Interfaces" section of the ACPI Specification. Please refer to
+ * section 15 in version 6.2 of the ACPI spec: http://uefi.org/specifications
+ */
+#define XEN_HVM_MEMMAP_TYPE_RAM       1
+#define XEN_HVM_MEMMAP_TYPE_RESERVED  2
+#define XEN_HVM_MEMMAP_TYPE_ACPI      3
+#define XEN_HVM_MEMMAP_TYPE_NVS       4
+#define XEN_HVM_MEMMAP_TYPE_UNUSABLE  5
+#define XEN_HVM_MEMMAP_TYPE_DISABLED  6
+#define XEN_HVM_MEMMAP_TYPE_PMEM      7
+
+/*
  * C representation of the x86/HVM start info layout.
  *
  * The canonical definition of this layout is above, this is just a way to
@@ -86,6 +135,14 @@ struct hvm_start_info {
     uint64_t cmdline_paddr;     /* Physical address of the command line.     */
     uint64_t rsdp_paddr;        /* Physical address of the RSDP ACPI data    */
                                 /* structure.                                */
+    uint64_t memmap_paddr;	/* Physical address of an array of           */
+				/* hvm_memmap_table_entry. Only present in   */
+				/* version 1 and newer of the structure      */
+    uint32_t memmap_entries;	/* Number of entries in the memmap table.    */
+				/* Only present in version 1 and newer of    */
+				/* the structure. Value will be zero if      */
+				/* there is no memory map being provided.    */
+    uint32_t reserved;		/* Must be zero for Version 1.		     */
 };
 
 struct hvm_modlist_entry {
@@ -95,4 +152,11 @@ struct hvm_modlist_entry {
     uint64_t reserved;
 };
 
+struct hvm_memmap_table_entry {
+    uint64_t addr;		/* Base address of the memory region         */
+    uint64_t size;		/* Size of the memory region in bytes        */
+    uint32_t type;		/* Mapping type                              */
+    uint32_t reserved;		/* Must be zero for Version 1.		     */
+};
+
 #endif /* __XEN_PUBLIC_ARCH_X86_HVM_START_INFO_H__ */
-- 
1.8.3.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v3 2/4] libxl: Move libxl__arch_domain_construct_memmap() earlier

[Maran Wilson]

From: Boris Ostrovsky <boris.ostrovsky@oracle.com>

Since hvm_start_info has now been expanded to include PVH guest's
memory map (i.e. e820) we need to know size of this map by the time we
create dom->start_info_seg in alloc_magic_pages_hvm().

To do so we have to call libxl__arch_domain_construct_memmap() earlier,
before xc_dom_build_image().

Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
---
 tools/libxl/libxl_create.c   |  2 +-
 tools/libxl/libxl_dom.c      | 12 +++++++++---
 tools/libxl/libxl_internal.h |  1 +
 tools/libxl/libxl_x86.c      |  3 +++
 4 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index c498135..5dce3df 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -488,7 +488,7 @@ int libxl__domain_build(libxl__gc *gc,
 
         break;
     case LIBXL_DOMAIN_TYPE_PV:
-        ret = libxl__build_pv(gc, domid, info, state);
+        ret = libxl__build_pv(gc, domid, d_config, info, state);
         if (ret)
             goto out;
 
diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index 2e29b52..917b45e 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -698,6 +698,7 @@ static int set_vnuma_info(libxl__gc *gc, uint32_t domid,
 }
 
 static int libxl__build_dom(libxl__gc *gc, uint32_t domid,
+             libxl_domain_config *d_config,
              libxl_domain_build_info *info, libxl__domain_build_state *state,
              struct xc_dom_image *dom)
 {
@@ -737,6 +738,11 @@ static int libxl__build_dom(libxl__gc *gc, uint32_t domid,
         LOGE(ERROR, "libxl__arch_domain_finalise_hw_description failed");
         goto out;
     }
+    ret = libxl__arch_domain_construct_memmap(gc, d_config, domid, dom);
+    if (ret != 0) {
+        LOG(ERROR, "setting domain memory map failed");
+        goto out;
+    }
     if ( (ret = xc_dom_build_image(dom)) != 0 ) {
         LOGE(ERROR, "xc_dom_build_image failed");
         goto out;
@@ -758,7 +764,7 @@ out:
     return ret != 0 ? ERROR_FAIL : 0;
 }
 
-int libxl__build_pv(libxl__gc *gc, uint32_t domid,
+int libxl__build_pv(libxl__gc *gc, uint32_t domid, libxl_domain_config *d_config,
              libxl_domain_build_info *info, libxl__domain_build_state *state)
 {
     libxl_ctx *ctx = libxl__gc_owner(gc);
@@ -847,7 +853,7 @@ int libxl__build_pv(libxl__gc *gc, uint32_t domid,
             dom->vnode_to_pnode[i] = info->vnuma_nodes[i].pnode;
     }
 
-    ret = libxl__build_dom(gc, domid, info, state, dom);
+    ret = libxl__build_dom(gc, domid, d_config, info, state, dom);
     if (ret != 0)
         goto out;
 
@@ -1293,7 +1299,7 @@ int libxl__build_hvm(libxl__gc *gc, uint32_t domid,
             dom->vnode_to_pnode[i] = info->vnuma_nodes[i].pnode;
     }
 
-    rc = libxl__build_dom(gc, domid, info, state, dom);
+    rc = libxl__build_dom(gc, domid, d_config, info, state, dom);
     if (rc != 0)
         goto out;
 
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 506687f..914df23 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1159,6 +1159,7 @@ _hidden int libxl__build_post(libxl__gc *gc, uint32_t domid,
                char **vms_ents, char **local_ents);
 
 _hidden int libxl__build_pv(libxl__gc *gc, uint32_t domid,
+                            libxl_domain_config *const d_config,
              libxl_domain_build_info *info, libxl__domain_build_state *state);
 _hidden int libxl__build_hvm(libxl__gc *gc, uint32_t domid,
               libxl_domain_config *d_config,
diff --git a/tools/libxl/libxl_x86.c b/tools/libxl/libxl_x86.c
index d82013f..3331cc5 100644
--- a/tools/libxl/libxl_x86.c
+++ b/tools/libxl/libxl_x86.c
@@ -525,6 +525,9 @@ int libxl__arch_domain_construct_memmap(libxl__gc *gc,
     uint32_t lowmem_start = dom->device_model ? GUEST_LOW_MEM_START_DEFAULT : 0;
     unsigned page_size = XC_DOM_PAGE_SIZE(dom);
 
+    if (d_config->b_info.type == LIBXL_DOMAIN_TYPE_PV)
+        return 0;
+
     /* Add all rdm entries. */
     for (i = 0; i < d_config->num_rdms; i++)
         if (d_config->rdms[i].policy != LIBXL_RDM_RESERVE_POLICY_INVALID)
-- 
1.8.3.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v3 3/4] libxl: Store PVH guest's e820 map in xc_dom_image

[Maran Wilson]

From: Boris Ostrovsky <boris.ostrovsky@oracle.com>

We will later copy it to hvm_start_info.

(Also remove stale comment claming that xc_dom_image.start_info_seg is
only used for HVMlite guests)

Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
---
 tools/libxc/include/xc_dom.h | 8 +++++++-
 tools/libxl/libxl_x86.c      | 6 ++++++
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/tools/libxc/include/xc_dom.h b/tools/libxc/include/xc_dom.h
index 491cad8..6ef68f9 100644
--- a/tools/libxc/include/xc_dom.h
+++ b/tools/libxc/include/xc_dom.h
@@ -99,7 +99,7 @@ struct xc_dom_image {
     struct xc_dom_seg p2m_seg;
     struct xc_dom_seg pgtables_seg;
     struct xc_dom_seg devicetree_seg;
-    struct xc_dom_seg start_info_seg; /* HVMlite only */
+    struct xc_dom_seg start_info_seg;
     xen_pfn_t start_info_pfn;
     xen_pfn_t console_pfn;
     xen_pfn_t xenstore_pfn;
@@ -224,6 +224,12 @@ struct xc_dom_image {
     /* Extra SMBIOS structures passed to HVMLOADER */
     struct xc_hvm_firmware_module smbios_module;
 
+    /* PVH guests */
+#if defined(__i386__) || defined(__x86_64__)
+    struct e820entry *e820;
+    unsigned int e820_entries;
+#endif
+
     xen_pfn_t vuart_gfn;
 };
 
diff --git a/tools/libxl/libxl_x86.c b/tools/libxl/libxl_x86.c
index 3331cc5..0de278f 100644
--- a/tools/libxl/libxl_x86.c
+++ b/tools/libxl/libxl_x86.c
@@ -600,6 +600,12 @@ int libxl__arch_domain_construct_memmap(libxl__gc *gc,
         goto out;
     }
 
+    if (d_config->b_info.type == LIBXL_DOMAIN_TYPE_PVH) {
+        dom->e820 = libxl__malloc(gc, sizeof(struct e820entry) * e820_entries);
+        dom->e820_entries = e820_entries;
+        memcpy(dom->e820,  e820, e820_entries * sizeof(*(dom->e820)));
+    }
+
 out:
     return rc;
 }
-- 
1.8.3.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v3 4/4] libxc: Pass e820 map to PVH guest via hvm_start_info

[Maran Wilson]

From: Boris Ostrovsky <boris.ostrovsky@oracle.com>

Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Signed-off-by: Maran Wilson <maran.wilson@oracle.com>
---
 tools/libxc/xc_dom_x86.c | 30 +++++++++++++++++++++++++++++-
 1 file changed, 29 insertions(+), 1 deletion(-)

diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
index 0b65dab..b46bd1d 100644
--- a/tools/libxc/xc_dom_x86.c
+++ b/tools/libxc/xc_dom_x86.c
@@ -35,6 +35,8 @@
 #include <xen/arch-x86/hvm/start_info.h>
 #include <xen/io/protocols.h>
 
+#include <xen-tools/libs.h>
+
 #include "xg_private.h"
 #include "xc_dom.h"
 #include "xenctrl.h"
@@ -640,6 +642,8 @@ static int alloc_magic_pages_hvm(struct xc_dom_image *dom)
             dom->cmdline_size = ROUNDUP(strlen(dom->cmdline) + 1, 8);
             start_info_size += dom->cmdline_size;
         }
+
+        start_info_size += dom->e820_entries * sizeof(*(dom->e820));
     }
     else
     {
@@ -1666,7 +1670,7 @@ 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;
+    size_t start_info_size, modsize;
     struct hvm_modlist_entry *modlist;
     unsigned int i;
 
@@ -1693,6 +1697,8 @@ static int bootlate_hvm(struct xc_dom_image *dom)
 
     if ( !dom->device_model )
     {
+        struct hvm_memmap_table_entry *memmap;
+
         if ( dom->cmdline )
         {
             char *cmdline = (void*)(start_info + 1);
@@ -1718,6 +1724,27 @@ static int bootlate_hvm(struct xc_dom_image *dom)
 
         /* ACPI module 0 is the RSDP */
         start_info->rsdp_paddr = dom->acpi_modules[0].guest_addr_out ? : 0;
+
+        /*
+         * Check a couple of XEN_HVM_MEMMAP_TYPEs to verify consistency with
+         * their corresponding e820 numerical values.
+         */
+        BUILD_BUG_ON(XEN_HVM_MEMMAP_TYPE_RAM != E820_RAM);
+        BUILD_BUG_ON(XEN_HVM_MEMMAP_TYPE_ACPI != E820_ACPI);
+
+        modsize = HVMLOADER_MODULE_MAX_COUNT *
+            (sizeof(*modlist) + HVMLOADER_MODULE_CMDLINE_SIZE);
+        memmap = (void*)modlist + modsize;
+
+        start_info->memmap_paddr = (dom->start_info_seg.pfn << PAGE_SHIFT) +
+                            ((uintptr_t)modlist - (uintptr_t)start_info) + modsize;
+        start_info->memmap_entries = dom->e820_entries;
+        for ( i = 0; i < dom->e820_entries; i++ )
+        {
+            memmap[i].addr = dom->e820[i].addr;
+            memmap[i].size = dom->e820[i].size;
+            memmap[i].type = dom->e820[i].type;
+        }
     }
     else
     {
@@ -1732,6 +1759,7 @@ static int bootlate_hvm(struct xc_dom_image *dom)
     }
 
     start_info->magic = XEN_HVM_START_MAGIC_VALUE;
+    start_info->version = 1;
 
     munmap(start_info, start_info_size);
 
-- 
1.8.3.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v3 1/4] x86/PVHv2: Add memory map pointer to hvm_start_info struct

[Jan Beulich]

>>> On 15.03.18 at 22:33, <maran.wilson@oracle.com> wrote:
> The start info structure that is defined as part of the x86/HVM direct boot
> ABI and used for starting Xen PVH guests would be more versatile if it also
> included a way to pass information about the memory map to the guest. This
> would allow KVM guests to share the same entry point.
> 
> Signed-off-by: Maran Wilson <maran.wilson@oracle.com>

Acked-by: Jan Beulich <jbeulich@suse.com>



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v3 1/4] x86/PVHv2: Add memory map pointer to hvm_start_info struct

[Roger Pau Monné]

On Thu, Mar 15, 2018 at 02:33:09PM -0700, Maran Wilson wrote:
> The start info structure that is defined as part of the x86/HVM direct boot
> ABI and used for starting Xen PVH guests would be more versatile if it also
> included a way to pass information about the memory map to the guest. This
> would allow KVM guests to share the same entry point.
> 
> Signed-off-by: Maran Wilson <maran.wilson@oracle.com>
> ---
>  xen/include/public/arch-x86/hvm/start_info.h | 66 +++++++++++++++++++++++++++-
>  1 file changed, 65 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/include/public/arch-x86/hvm/start_info.h b/xen/include/public/arch-x86/hvm/start_info.h
> index 6484159..f2e8ba6 100644
> --- a/xen/include/public/arch-x86/hvm/start_info.h
> +++ b/xen/include/public/arch-x86/hvm/start_info.h
> @@ -33,8 +33,9 @@
>   *    | magic          | Contains the magic value XEN_HVM_START_MAGIC_VALUE
>   *    |                | ("xEn3" with the 0x80 bit of the "E" set).
>   *  4 +----------------+
> - *    | version        | Version of this structure. Current version is 0. New
> + *    | version        | Version of this structure. Current version is 1. New
>   *    |                | versions are guaranteed to be backwards-compatible.
> + *    |                | For PV guests only 0 allowed, for PVH 0 or 1 allowed.

Why are you adding the above sentence? PV guest never used or will use
the hvm_start_info structure (note the hvm_ prefix).

>   *  8 +----------------+
>   *    | flags          | SIF_xxx flags.
>   * 12 +----------------+
> @@ -48,6 +49,15 @@
>   * 32 +----------------+
>   *    | rsdp_paddr     | Physical address of the RSDP ACPI data structure.
>   * 40 +----------------+
> + *    | memmap_paddr   | Physical address of the (optional) memory map. Only
> + *    |                | present in version 1 and newer of the structure.
> + * 48 +----------------+
> + *    | memmap_entries | Number of entries in the memory map table. Zero
> + *    |                | if there is no memory map being provided. Only

Again (as I've mentioned in previous reviews) the way to signal a
non-present memory map is to set memmap_paddr to 0, not memmap_entries
to 0. This is already covered by the comment at the top of the header,
which states:

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.

> + *    |                | present in version 1 and newer of the structure.
> + * 52 +----------------+
> + *    | reserved       | Version 1 and newer only.
> + * 56 +----------------+
>   *
>   * The layout of each entry in the module structure is the following:
>   *
> @@ -62,14 +72,53 @@
>   *    | reserved       |
>   * 32 +----------------+
>   *
> + * The layout of each entry in the memory map table is as follows:
> + *
> + *  0 +----------------+
> + *    | addr           | Base address
> + *  8 +----------------+
> + *    | size           | Size of mapping in bytes
> + * 16 +----------------+
> + *    | type           | Type of mapping as defined between the hypervisor
> + *    |                | and guest it's starting. See XEN_HVM_MEMMAP_TYPE_*
> + *    |                | values below.
> + * 20 +----------------|
> + *    | reserved       |
> + * 24 +----------------+
> + *
>   * The address and sizes are always a 64bit little endian unsigned integer.
>   *
>   * NB: Xen on x86 will always try to place all the data below the 4GiB
>   * boundary.
> + *
> + * Version numbers of the hvm_start_info structure have evolved like this:
> + *
> + * Version 0:  Initial implementation.
> + *
> + * Version 1:  Added the memmap_paddr/memmap_entries fields (plus 4 bytes of
> + *             padding) to the end of the hvm_start_info struct. These new
> + *             fields can be used to pass a memory map to the guest. The
> + *             memory map is optional and so guests that understand version 1
> + *             of the structure must check that memmap_entries is non-zero
> + *             before trying to read the memory map.

Same again, the guest will have to check memmap_paddr != 0, not
memmap_entries, like it's done for other fields that contain a
physical address.

>   */
>  #define XEN_HVM_START_MAGIC_VALUE 0x336ec578
>  
>  /*
> + * The values used in the type field of the memory map table entries are
> + * defined below and match the Address Range Types as defined in the "System
> + * Address Map Interfaces" section of the ACPI Specification. Please refer to
> + * section 15 in version 6.2 of the ACPI spec: http://uefi.org/specifications
> + */
> +#define XEN_HVM_MEMMAP_TYPE_RAM       1
> +#define XEN_HVM_MEMMAP_TYPE_RESERVED  2
> +#define XEN_HVM_MEMMAP_TYPE_ACPI      3
> +#define XEN_HVM_MEMMAP_TYPE_NVS       4
> +#define XEN_HVM_MEMMAP_TYPE_UNUSABLE  5
> +#define XEN_HVM_MEMMAP_TYPE_DISABLED  6
> +#define XEN_HVM_MEMMAP_TYPE_PMEM      7
> +
> +/*
>   * C representation of the x86/HVM start info layout.
>   *
>   * The canonical definition of this layout is above, this is just a way to
> @@ -86,6 +135,14 @@ struct hvm_start_info {
>      uint64_t cmdline_paddr;     /* Physical address of the command line.     */
>      uint64_t rsdp_paddr;        /* Physical address of the RSDP ACPI data    */
>                                  /* structure.                                */
> +    uint64_t memmap_paddr;	/* Physical address of an array of           */
> +				/* hvm_memmap_table_entry. Only present in   */
> +				/* version 1 and newer of the structure      */
> +    uint32_t memmap_entries;	/* Number of entries in the memmap table.    */
> +				/* Only present in version 1 and newer of    */
> +				/* the structure. Value will be zero if      */
> +				/* there is no memory map being provided.    */
> +    uint32_t reserved;		/* Must be zero for Version 1.		     */

As mentioned in previous reviews: no tabs please.

>  };
>  
>  struct hvm_modlist_entry {
> @@ -95,4 +152,11 @@ struct hvm_modlist_entry {
>      uint64_t reserved;
>  };
>  
> +struct hvm_memmap_table_entry {
> +    uint64_t addr;		/* Base address of the memory region         */
> +    uint64_t size;		/* Size of the memory region in bytes        */
> +    uint32_t type;		/* Mapping type                              */
> +    uint32_t reserved;		/* Must be zero for Version 1.		     */
> +};

No tabs please.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v3 1/4] x86/PVHv2: Add memory map pointer to hvm_start_info struct

[Jan Beulich]

>>> On 16.03.18 at 12:11, <roger.pau@citrix.com> wrote:
> On Thu, Mar 15, 2018 at 02:33:09PM -0700, Maran Wilson wrote:
>> @@ -48,6 +49,15 @@
>>   * 32 +----------------+
>>   *    | rsdp_paddr     | Physical address of the RSDP ACPI data structure.
>>   * 40 +----------------+
>> + *    | memmap_paddr   | Physical address of the (optional) memory map. Only
>> + *    |                | present in version 1 and newer of the structure.
>> + * 48 +----------------+
>> + *    | memmap_entries | Number of entries in the memory map table. Zero
>> + *    |                | if there is no memory map being provided. Only
> 
> Again (as I've mentioned in previous reviews) the way to signal a
> non-present memory map is to set memmap_paddr to 0, not memmap_entries
> to 0. This is already covered by the comment at the top of the header,
> which states:
> 
> 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.

I still think it is legitimate to direct consumers to look at the entry
count here.

>> @@ -86,6 +135,14 @@ struct hvm_start_info {
>>      uint64_t cmdline_paddr;     /* Physical address of the command line.     */
>>      uint64_t rsdp_paddr;        /* Physical address of the RSDP ACPI data    */
>>                                  /* structure.                                */
>> +    uint64_t memmap_paddr;	/* Physical address of an array of           */
>> +				/* hvm_memmap_table_entry. Only present in   */
>> +				/* version 1 and newer of the structure      */
>> +    uint32_t memmap_entries;	/* Number of entries in the memmap table.    */
>> +				/* Only present in version 1 and newer of    */
>> +				/* the structure. Value will be zero if      */
>> +				/* there is no memory map being provided.    */
>> +    uint32_t reserved;		/* Must be zero for Version 1.		     */
> 
> As mentioned in previous reviews: no tabs please.
> 
>>  };
>>  
>>  struct hvm_modlist_entry {
>> @@ -95,4 +152,11 @@ struct hvm_modlist_entry {
>>      uint64_t reserved;
>>  };
>>  
>> +struct hvm_memmap_table_entry {
>> +    uint64_t addr;		/* Base address of the memory region         */
>> +    uint64_t size;		/* Size of the memory region in bytes        */
>> +    uint32_t type;		/* Mapping type                              */
>> +    uint32_t reserved;		/* Must be zero for Version 1.		     */
>> +};
> 
> No tabs please.

Oh, I didn't even notice these - my ack is clearly dependent on them
gone.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v3 1/4] x86/PVHv2: Add memory map pointer to hvm_start_info struct

[Roger Pau Monné]

On Fri, Mar 16, 2018 at 05:29:27AM -0600, Jan Beulich wrote:
> >>> On 16.03.18 at 12:11, <roger.pau@citrix.com> wrote:
> > On Thu, Mar 15, 2018 at 02:33:09PM -0700, Maran Wilson wrote:
> >> @@ -48,6 +49,15 @@
> >>   * 32 +----------------+
> >>   *    | rsdp_paddr     | Physical address of the RSDP ACPI data structure.
> >>   * 40 +----------------+
> >> + *    | memmap_paddr   | Physical address of the (optional) memory map. Only
> >> + *    |                | present in version 1 and newer of the structure.
> >> + * 48 +----------------+
> >> + *    | memmap_entries | Number of entries in the memory map table. Zero
> >> + *    |                | if there is no memory map being provided. Only
> > 
> > Again (as I've mentioned in previous reviews) the way to signal a
> > non-present memory map is to set memmap_paddr to 0, not memmap_entries
> > to 0. This is already covered by the comment at the top of the header,
> > which states:
> > 
> > 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.
> 
> I still think it is legitimate to direct consumers to look at the entry
> count here.

We have another similar field tuple already, modlist_paddr and
nr_modules and in that case (according to the current comments) the
proper way to check if there are modules is to check modlist_paddr !=
0 and then get the count from nr_modules.

Using this access strategy we avoid adding more comments about
checking nr_modules != 0 before trying to access modlist_paddr.

Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v3 1/4] x86/PVHv2: Add memory map pointer to hvm_start_info struct

[Jan Beulich]

>>> On 16.03.18 at 12:37, <roger.pau@citrix.com> wrote:
> On Fri, Mar 16, 2018 at 05:29:27AM -0600, Jan Beulich wrote:
>> >>> On 16.03.18 at 12:11, <roger.pau@citrix.com> wrote:
>> > On Thu, Mar 15, 2018 at 02:33:09PM -0700, Maran Wilson wrote:
>> >> @@ -48,6 +49,15 @@
>> >>   * 32 +----------------+
>> >>   *    | rsdp_paddr     | Physical address of the RSDP ACPI data structure.
>> >>   * 40 +----------------+
>> >> + *    | memmap_paddr   | Physical address of the (optional) memory map. 
> Only
>> >> + *    |                | present in version 1 and newer of the structure.
>> >> + * 48 +----------------+
>> >> + *    | memmap_entries | Number of entries in the memory map table. Zero
>> >> + *    |                | if there is no memory map being provided. Only
>> > 
>> > Again (as I've mentioned in previous reviews) the way to signal a
>> > non-present memory map is to set memmap_paddr to 0, not memmap_entries
>> > to 0. This is already covered by the comment at the top of the header,
>> > which states:
>> > 
>> > 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.
>> 
>> I still think it is legitimate to direct consumers to look at the entry
>> count here.
> 
> We have another similar field tuple already, modlist_paddr and
> nr_modules and in that case (according to the current comments) the
> proper way to check if there are modules is to check modlist_paddr !=
> 0 and then get the count from nr_modules.

Well, that's the way it is now, as it's out in the wild already.

> Using this access strategy we avoid adding more comments about
> checking nr_modules != 0 before trying to access modlist_paddr.

I don't follow this argumentation: There is an obvious implication
that only nr_modules entries are valid to access at modlist_paddr.
If nr_modules is zero, no entry is valid to access. Nothing to be
said explicitly in the comment.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v3 1/4] x86/PVHv2: Add memory map pointer to hvm_start_info struct

[Roger Pau Monné]

On Fri, Mar 16, 2018 at 05:48:09AM -0600, Jan Beulich wrote:
> >>> On 16.03.18 at 12:37, <roger.pau@citrix.com> wrote:
> > On Fri, Mar 16, 2018 at 05:29:27AM -0600, Jan Beulich wrote:
> >> >>> On 16.03.18 at 12:11, <roger.pau@citrix.com> wrote:
> >> > On Thu, Mar 15, 2018 at 02:33:09PM -0700, Maran Wilson wrote:
> >> >> @@ -48,6 +49,15 @@
> >> >>   * 32 +----------------+
> >> >>   *    | rsdp_paddr     | Physical address of the RSDP ACPI data structure.
> >> >>   * 40 +----------------+
> >> >> + *    | memmap_paddr   | Physical address of the (optional) memory map. 
> > Only
> >> >> + *    |                | present in version 1 and newer of the structure.
> >> >> + * 48 +----------------+
> >> >> + *    | memmap_entries | Number of entries in the memory map table. Zero
> >> >> + *    |                | if there is no memory map being provided. Only
> >> > 
> >> > Again (as I've mentioned in previous reviews) the way to signal a
> >> > non-present memory map is to set memmap_paddr to 0, not memmap_entries
> >> > to 0. This is already covered by the comment at the top of the header,
> >> > which states:
> >> > 
> >> > 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.
> >> 
> >> I still think it is legitimate to direct consumers to look at the entry
> >> count here.
> > 
> > We have another similar field tuple already, modlist_paddr and
> > nr_modules and in that case (according to the current comments) the
> > proper way to check if there are modules is to check modlist_paddr !=
> > 0 and then get the count from nr_modules.
> 
> Well, that's the way it is now, as it's out in the wild already.
> 
> > Using this access strategy we avoid adding more comments about
> > checking nr_modules != 0 before trying to access modlist_paddr.
> 
> I don't follow this argumentation: There is an obvious implication
> that only nr_modules entries are valid to access at modlist_paddr.
> If nr_modules is zero, no entry is valid to access. Nothing to be
> said explicitly in the comment.

Right, I don't think there should be any mention about how
memmap_paddr should be accessed, the current comment is enough IMHO,
and adding the "Zero if there is no memory map being provided" and
"... so guests that understand version 1 of the structure must check
that memmap_entries is non-zero before trying to read the memory map."
is just redundant and should be removed.

Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v3 1/4] x86/PVHv2: Add memory map pointer to hvm_start_info struct

[Juergen Gross]

On 16/03/18 12:48, Jan Beulich wrote:
>>>> On 16.03.18 at 12:37, <roger.pau@citrix.com> wrote:
>> On Fri, Mar 16, 2018 at 05:29:27AM -0600, Jan Beulich wrote:
>>>>>> On 16.03.18 at 12:11, <roger.pau@citrix.com> wrote:
>>>> On Thu, Mar 15, 2018 at 02:33:09PM -0700, Maran Wilson wrote:
>>>>> @@ -48,6 +49,15 @@
>>>>>   * 32 +----------------+
>>>>>   *    | rsdp_paddr     | Physical address of the RSDP ACPI data structure.
>>>>>   * 40 +----------------+
>>>>> + *    | memmap_paddr   | Physical address of the (optional) memory map. 
>> Only
>>>>> + *    |                | present in version 1 and newer of the structure.
>>>>> + * 48 +----------------+
>>>>> + *    | memmap_entries | Number of entries in the memory map table. Zero
>>>>> + *    |                | if there is no memory map being provided. Only
>>>>
>>>> Again (as I've mentioned in previous reviews) the way to signal a
>>>> non-present memory map is to set memmap_paddr to 0, not memmap_entries
>>>> to 0. This is already covered by the comment at the top of the header,
>>>> which states:
>>>>
>>>> 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.
>>>
>>> I still think it is legitimate to direct consumers to look at the entry
>>> count here.
>>
>> We have another similar field tuple already, modlist_paddr and
>> nr_modules and in that case (according to the current comments) the
>> proper way to check if there are modules is to check modlist_paddr !=
>> 0 and then get the count from nr_modules.
> 
> Well, that's the way it is now, as it's out in the wild already.
> 
>> Using this access strategy we avoid adding more comments about
>> checking nr_modules != 0 before trying to access modlist_paddr.
> 
> I don't follow this argumentation: There is an obvious implication
> that only nr_modules entries are valid to access at modlist_paddr.
> If nr_modules is zero, no entry is valid to access. Nothing to be
> said explicitly in the comment.

The 0 address is important for cases without a count field (e.g. the
rsdp_paddr field).

In case where a count is supplied I'd say a count value of 0 should be
used to indicate no entry is present. Setting the address to zero in
this case, too, should be allowed, of course.

I'd regard an address of zero and count > 0 as invalid.


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v3 1/4] x86/PVHv2: Add memory map pointer to hvm_start_info struct

[Jan Beulich]

>>> On 16.03.18 at 13:05, <jgross@suse.com> wrote:
> I'd regard an address of zero and count > 0 as invalid.

Indeed.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v3 1/4] x86/PVHv2: Add memory map pointer to hvm_start_info struct

[Maran Wilson]

On 3/16/2018 4:11 AM, Roger Pau Monné wrote:
> On Thu, Mar 15, 2018 at 02:33:09PM -0700, Maran Wilson wrote:
>> The start info structure that is defined as part of the x86/HVM direct boot
>> ABI and used for starting Xen PVH guests would be more versatile if it also
>> included a way to pass information about the memory map to the guest. This
>> would allow KVM guests to share the same entry point.
>>
>> Signed-off-by: Maran Wilson <maran.wilson@oracle.com>
>> ---
>>   xen/include/public/arch-x86/hvm/start_info.h | 66 +++++++++++++++++++++++++++-
>>   1 file changed, 65 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/include/public/arch-x86/hvm/start_info.h b/xen/include/public/arch-x86/hvm/start_info.h
>> index 6484159..f2e8ba6 100644
>> --- a/xen/include/public/arch-x86/hvm/start_info.h
>> +++ b/xen/include/public/arch-x86/hvm/start_info.h
>> @@ -33,8 +33,9 @@
>>    *    | magic          | Contains the magic value XEN_HVM_START_MAGIC_VALUE
>>    *    |                | ("xEn3" with the 0x80 bit of the "E" set).
>>    *  4 +----------------+
>> - *    | version        | Version of this structure. Current version is 0. New
>> + *    | version        | Version of this structure. Current version is 1. New
>>    *    |                | versions are guaranteed to be backwards-compatible.
>> + *    |                | For PV guests only 0 allowed, for PVH 0 or 1 allowed.
> Why are you adding the above sentence? PV guest never used or will use
> the hvm_start_info structure (note the hvm_ prefix).

Thanks for the feed back Roger,

As you noticed, my first version did not contain that comment. Konrad 
suggested adding that particular line (in reply to the Linux tree 
version of this patch) and no one seemed to object at the time so I went 
ahead and added it.

Konrad, do you care to weigh in here?

>>    *  8 +----------------+
>>    *    | flags          | SIF_xxx flags.
>>    * 12 +----------------+
>> @@ -48,6 +49,15 @@
>>    * 32 +----------------+
>>    *    | rsdp_paddr     | Physical address of the RSDP ACPI data structure.
>>    * 40 +----------------+
>> + *    | memmap_paddr   | Physical address of the (optional) memory map. Only
>> + *    |                | present in version 1 and newer of the structure.
>> + * 48 +----------------+
>> + *    | memmap_entries | Number of entries in the memory map table. Zero
>> + *    |                | if there is no memory map being provided. Only
> Again (as I've mentioned in previous reviews) the way to signal a
> non-present memory map is to set memmap_paddr to 0, not memmap_entries
> to 0. This is already covered by the comment at the top of the header,
> which states:
>
> 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.

I'm not really following the argument for why checking for PA != 0 is a 
better approach. The way I see it, we have to check #entries anyway so 
the consumer side code is more efficient if we just check #entries and 
assume the PA is valid when #entries != 0 (seems like a reasonable 
demand for the producer side). If we define it to be "PA of zero means 
no valid entries", then the consumer side code has to make the 
additional check of PA != 0 in addition to making sure there are greater 
than 0 entries. It's not a huge deal, but then again, I'm not seeing a 
huge win by going the other way either.

The fact that a previous comment already exists regarding PA of 0 is 
*slightly* awkward (redundant) depending on how you look at it. But it's 
not blatantly contradictory (in most cases, I'm sure PA and #entries 
will both be zero when no memory map is being provided) and it probably 
serves a purpose for the rsdp_paddr field.

So after carefully reading everyone's input on this thread, my 
preference as the person coding this up is to stick with the #entries 
check and leave the other existing comments in place to cover the 
existing fields and code that is already out there.

But if it's really a deal breaker for someone or if broad consensus is 
that it is better to just make the change so that PA of 0 is the 
definitive way to check for the presence of a memory map, then I will go 
ahead and make the change.

>> + *    |                | present in version 1 and newer of the structure.
>> + * 52 +----------------+
>> + *    | reserved       | Version 1 and newer only.
>> + * 56 +----------------+
>>    *
>>    * The layout of each entry in the module structure is the following:
>>    *
>> @@ -62,14 +72,53 @@
>>    *    | reserved       |
>>    * 32 +----------------+
>>    *
>> + * The layout of each entry in the memory map table is as follows:
>> + *
>> + *  0 +----------------+
>> + *    | addr           | Base address
>> + *  8 +----------------+
>> + *    | size           | Size of mapping in bytes
>> + * 16 +----------------+
>> + *    | type           | Type of mapping as defined between the hypervisor
>> + *    |                | and guest it's starting. See XEN_HVM_MEMMAP_TYPE_*
>> + *    |                | values below.
>> + * 20 +----------------|
>> + *    | reserved       |
>> + * 24 +----------------+
>> + *
>>    * The address and sizes are always a 64bit little endian unsigned integer.
>>    *
>>    * NB: Xen on x86 will always try to place all the data below the 4GiB
>>    * boundary.
>> + *
>> + * Version numbers of the hvm_start_info structure have evolved like this:
>> + *
>> + * Version 0:  Initial implementation.
>> + *
>> + * Version 1:  Added the memmap_paddr/memmap_entries fields (plus 4 bytes of
>> + *             padding) to the end of the hvm_start_info struct. These new
>> + *             fields can be used to pass a memory map to the guest. The
>> + *             memory map is optional and so guests that understand version 1
>> + *             of the structure must check that memmap_entries is non-zero
>> + *             before trying to read the memory map.
> Same again, the guest will have to check memmap_paddr != 0, not
> memmap_entries, like it's done for other fields that contain a
> physical address.
>
>>    */
>>   #define XEN_HVM_START_MAGIC_VALUE 0x336ec578
>>   
>>   /*
>> + * The values used in the type field of the memory map table entries are
>> + * defined below and match the Address Range Types as defined in the "System
>> + * Address Map Interfaces" section of the ACPI Specification. Please refer to
>> + * section 15 in version 6.2 of the ACPI spec: http://uefi.org/specifications
>> + */
>> +#define XEN_HVM_MEMMAP_TYPE_RAM       1
>> +#define XEN_HVM_MEMMAP_TYPE_RESERVED  2
>> +#define XEN_HVM_MEMMAP_TYPE_ACPI      3
>> +#define XEN_HVM_MEMMAP_TYPE_NVS       4
>> +#define XEN_HVM_MEMMAP_TYPE_UNUSABLE  5
>> +#define XEN_HVM_MEMMAP_TYPE_DISABLED  6
>> +#define XEN_HVM_MEMMAP_TYPE_PMEM      7
>> +
>> +/*
>>    * C representation of the x86/HVM start info layout.
>>    *
>>    * The canonical definition of this layout is above, this is just a way to
>> @@ -86,6 +135,14 @@ struct hvm_start_info {
>>       uint64_t cmdline_paddr;     /* Physical address of the command line.     */
>>       uint64_t rsdp_paddr;        /* Physical address of the RSDP ACPI data    */
>>                                   /* structure.                                */
>> +    uint64_t memmap_paddr;	/* Physical address of an array of           */
>> +				/* hvm_memmap_table_entry. Only present in   */
>> +				/* version 1 and newer of the structure      */
>> +    uint32_t memmap_entries;	/* Number of entries in the memmap table.    */
>> +				/* Only present in version 1 and newer of    */
>> +				/* the structure. Value will be zero if      */
>> +				/* there is no memory map being provided.    */
>> +    uint32_t reserved;		/* Must be zero for Version 1.		     */
> As mentioned in previous reviews: no tabs please.

I'm embarrassed. I'll fix that and make sure the rest of the file is 
using soft tabs everywhere I have touched.

My problem was switching between the Xen tree and other source trees 
where tabs are required :-)

Thanks,
-Maran

>>   };
>>   
>>   struct hvm_modlist_entry {
>> @@ -95,4 +152,11 @@ struct hvm_modlist_entry {
>>       uint64_t reserved;
>>   };
>>   
>> +struct hvm_memmap_table_entry {
>> +    uint64_t addr;		/* Base address of the memory region         */
>> +    uint64_t size;		/* Size of the memory region in bytes        */
>> +    uint32_t type;		/* Mapping type                              */
>> +    uint32_t reserved;		/* Must be zero for Version 1.		     */
>> +};
> No tabs please.
>
> Thanks, Roger.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v3 1/4] x86/PVHv2: Add memory map pointer to hvm_start_info struct

[Roger Pau Monné]

On Fri, Mar 16, 2018 at 10:00:54AM -0700, Maran Wilson wrote:
> On 3/16/2018 4:11 AM, Roger Pau Monné wrote:
> > On Thu, Mar 15, 2018 at 02:33:09PM -0700, Maran Wilson wrote:
> > >    *  8 +----------------+
> > >    *    | flags          | SIF_xxx flags.
> > >    * 12 +----------------+
> > > @@ -48,6 +49,15 @@
> > >    * 32 +----------------+
> > >    *    | rsdp_paddr     | Physical address of the RSDP ACPI data structure.
> > >    * 40 +----------------+
> > > + *    | memmap_paddr   | Physical address of the (optional) memory map. Only
> > > + *    |                | present in version 1 and newer of the structure.
> > > + * 48 +----------------+
> > > + *    | memmap_entries | Number of entries in the memory map table. Zero
> > > + *    |                | if there is no memory map being provided. Only
> > Again (as I've mentioned in previous reviews) the way to signal a
> > non-present memory map is to set memmap_paddr to 0, not memmap_entries
> > to 0. This is already covered by the comment at the top of the header,
> > which states:
> > 
> > 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.
> 
> I'm not really following the argument for why checking for PA != 0 is a
> better approach. The way I see it, we have to check #entries anyway so the
> consumer side code is more efficient if we just check #entries and assume
> the PA is valid when #entries != 0 (seems like a reasonable demand for the
> producer side). If we define it to be "PA of zero means no valid entries",
> then the consumer side code has to make the additional check of PA != 0 in
> addition to making sure there are greater than 0 entries. It's not a huge
> deal, but then again, I'm not seeing a huge win by going the other way
> either.
> 
> The fact that a previous comment already exists regarding PA of 0 is
> *slightly* awkward (redundant) depending on how you look at it. But it's not
> blatantly contradictory (in most cases, I'm sure PA and #entries will both
> be zero when no memory map is being provided) and it probably serves a
> purpose for the rsdp_paddr field.
> 
> So after carefully reading everyone's input on this thread, my preference as
> the person coding this up is to stick with the #entries check and leave the
> other existing comments in place to cover the existing fields and code that
> is already out there.
> 
> But if it's really a deal breaker for someone or if broad consensus is that
> it is better to just make the change so that PA of 0 is the definitive way
> to check for the presence of a memory map, then I will go ahead and make the
> change.

OK, I just wanted to have consistency with the nr_modules and
modlist_paddr field tuple. The consensus seem to be that spelling out
the "Zero if there is no memory map..." condition is fine, so I'm not
going to argue over it anymore.

Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v3 2/4] libxl: Move libxl__arch_domain_construct_memmap() earlier

[Roger Pau Monné]

On Thu, Mar 15, 2018 at 02:35:16PM -0700, Maran Wilson wrote:
> From: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> 
> Since hvm_start_info has now been expanded to include PVH guest's
> memory map (i.e. e820) we need to know size of this map by the time we
> create dom->start_info_seg in alloc_magic_pages_hvm().
> 
> To do so we have to call libxl__arch_domain_construct_memmap() earlier,
> before xc_dom_build_image().
> 
> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> ---
>  tools/libxl/libxl_create.c   |  2 +-
>  tools/libxl/libxl_dom.c      | 12 +++++++++---
>  tools/libxl/libxl_internal.h |  1 +
>  tools/libxl/libxl_x86.c      |  3 +++
>  4 files changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> index c498135..5dce3df 100644
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> @@ -488,7 +488,7 @@ int libxl__domain_build(libxl__gc *gc,
>  
>          break;
>      case LIBXL_DOMAIN_TYPE_PV:
> -        ret = libxl__build_pv(gc, domid, info, state);
> +        ret = libxl__build_pv(gc, domid, d_config, info, state);
>          if (ret)
>              goto out;
>  
> diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
> index 2e29b52..917b45e 100644
> --- a/tools/libxl/libxl_dom.c
> +++ b/tools/libxl/libxl_dom.c
> @@ -698,6 +698,7 @@ static int set_vnuma_info(libxl__gc *gc, uint32_t domid,
>  }
>  
>  static int libxl__build_dom(libxl__gc *gc, uint32_t domid,
> +             libxl_domain_config *d_config,
>               libxl_domain_build_info *info, libxl__domain_build_state *state,
>               struct xc_dom_image *dom)
>  {
> @@ -737,6 +738,11 @@ static int libxl__build_dom(libxl__gc *gc, uint32_t domid,
>          LOGE(ERROR, "libxl__arch_domain_finalise_hw_description failed");
>          goto out;
>      }
> +    ret = libxl__arch_domain_construct_memmap(gc, d_config, domid, dom);
> +    if (ret != 0) {
> +        LOG(ERROR, "setting domain memory map failed");
> +        goto out;
> +    }

If you add this call here, shouldn't you remove the same one from
libxl__build_hvm?

Does it make sense to place this in the
libxl__arch_domain_finalise_hw_description hook?

On ARM libxl__arch_domain_construct_memmap it's just an empty wrapper
anyway.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v3 3/4] libxl: Store PVH guest's e820 map in xc_dom_image

[Roger Pau Monné]

On Thu, Mar 15, 2018 at 02:35:17PM -0700, Maran Wilson wrote:
> From: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> 
> We will later copy it to hvm_start_info.
> 
> (Also remove stale comment claming that xc_dom_image.start_info_seg is
> only used for HVMlite guests)
> 
> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> ---
>  tools/libxc/include/xc_dom.h | 8 +++++++-
>  tools/libxl/libxl_x86.c      | 6 ++++++
>  2 files changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/libxc/include/xc_dom.h b/tools/libxc/include/xc_dom.h
> index 491cad8..6ef68f9 100644
> --- a/tools/libxc/include/xc_dom.h
> +++ b/tools/libxc/include/xc_dom.h
> @@ -99,7 +99,7 @@ struct xc_dom_image {
>      struct xc_dom_seg p2m_seg;
>      struct xc_dom_seg pgtables_seg;
>      struct xc_dom_seg devicetree_seg;
> -    struct xc_dom_seg start_info_seg; /* HVMlite only */
> +    struct xc_dom_seg start_info_seg;
>      xen_pfn_t start_info_pfn;
>      xen_pfn_t console_pfn;
>      xen_pfn_t xenstore_pfn;
> @@ -224,6 +224,12 @@ struct xc_dom_image {
>      /* Extra SMBIOS structures passed to HVMLOADER */
>      struct xc_hvm_firmware_module smbios_module;
>  
> +    /* PVH guests */
> +#if defined(__i386__) || defined(__x86_64__)
> +    struct e820entry *e820;
> +    unsigned int e820_entries;
> +#endif

Not sure whether it's worth protecting this with x86 guards,
xc_dom_image it's already a mix of ARM/x86 fields without much
protection.

> +
>      xen_pfn_t vuart_gfn;
>  };
>  
> diff --git a/tools/libxl/libxl_x86.c b/tools/libxl/libxl_x86.c
> index 3331cc5..0de278f 100644
> --- a/tools/libxl/libxl_x86.c
> +++ b/tools/libxl/libxl_x86.c
> @@ -600,6 +600,12 @@ int libxl__arch_domain_construct_memmap(libxl__gc *gc,
>          goto out;
>      }
>  
> +    if (d_config->b_info.type == LIBXL_DOMAIN_TYPE_PVH) {
> +        dom->e820 = libxl__malloc(gc, sizeof(struct e820entry) * e820_entries);
> +        dom->e820_entries = e820_entries;
> +        memcpy(dom->e820,  e820, e820_entries * sizeof(*(dom->e820)));
                            ^ extra space.

Also could you please use sizeof(*(dom->e820)) or sizeof(struct
e820entry) consistently? (I think I prefer the latter).

Also you want this for both PVH and HVM, since hvmloader now also uses
the hvm_start_info structure to pass the BIOS blob as a module.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v3 4/4] libxc: Pass e820 map to PVH guest via hvm_start_info

[Roger Pau Monné]

On Thu, Mar 15, 2018 at 02:35:18PM -0700, Maran Wilson wrote:
> From: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> 
> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Signed-off-by: Maran Wilson <maran.wilson@oracle.com>
> ---
>  tools/libxc/xc_dom_x86.c | 30 +++++++++++++++++++++++++++++-
>  1 file changed, 29 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
> @@ -1718,6 +1724,27 @@ static int bootlate_hvm(struct xc_dom_image *dom)
>  
>          /* ACPI module 0 is the RSDP */
>          start_info->rsdp_paddr = dom->acpi_modules[0].guest_addr_out ? : 0;
> +
> +        /*
> +         * Check a couple of XEN_HVM_MEMMAP_TYPEs to verify consistency with
> +         * their corresponding e820 numerical values.
> +         */
> +        BUILD_BUG_ON(XEN_HVM_MEMMAP_TYPE_RAM != E820_RAM);
> +        BUILD_BUG_ON(XEN_HVM_MEMMAP_TYPE_ACPI != E820_ACPI);

I guess you could test all the types, but I'm not sure if it's worth
it, let's see what others think.

> +
> +        modsize = HVMLOADER_MODULE_MAX_COUNT *
> +            (sizeof(*modlist) + HVMLOADER_MODULE_CMDLINE_SIZE);
> +        memmap = (void*)modlist + modsize;
                         ^ space.
> +
> +        start_info->memmap_paddr = (dom->start_info_seg.pfn << PAGE_SHIFT) +
> +                            ((uintptr_t)modlist - (uintptr_t)start_info) + modsize;
> +        start_info->memmap_entries = dom->e820_entries;
> +        for ( i = 0; i < dom->e820_entries; i++ )
> +        {
> +            memmap[i].addr = dom->e820[i].addr;
> +            memmap[i].size = dom->e820[i].size;
> +            memmap[i].type = dom->e820[i].type;
> +        }

I see no reason to do this for the PVH type only, you should pass the
memory map to both PVH and HVM (and it's going to reduce the
indentation).

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v3 2/4] libxl: Move libxl__arch_domain_construct_memmap() earlier

[Boris Ostrovsky]

On 03/16/2018 02:12 PM, Roger Pau Monné wrote:
> On Thu, Mar 15, 2018 at 02:35:16PM -0700, Maran Wilson wrote:
>> From: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>>
>> Since hvm_start_info has now been expanded to include PVH guest's
>> memory map (i.e. e820) we need to know size of this map by the time we
>> create dom->start_info_seg in alloc_magic_pages_hvm().
>>
>> To do so we have to call libxl__arch_domain_construct_memmap() earlier,
>> before xc_dom_build_image().
>>
>> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>> ---
>>  tools/libxl/libxl_create.c   |  2 +-
>>  tools/libxl/libxl_dom.c      | 12 +++++++++---
>>  tools/libxl/libxl_internal.h |  1 +
>>  tools/libxl/libxl_x86.c      |  3 +++
>>  4 files changed, 14 insertions(+), 4 deletions(-)
>>
>> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
>> index c498135..5dce3df 100644
>> --- a/tools/libxl/libxl_create.c
>> +++ b/tools/libxl/libxl_create.c
>> @@ -488,7 +488,7 @@ int libxl__domain_build(libxl__gc *gc,
>>  
>>          break;
>>      case LIBXL_DOMAIN_TYPE_PV:
>> -        ret = libxl__build_pv(gc, domid, info, state);
>> +        ret = libxl__build_pv(gc, domid, d_config, info, state);
>>          if (ret)
>>              goto out;
>>  
>> diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
>> index 2e29b52..917b45e 100644
>> --- a/tools/libxl/libxl_dom.c
>> +++ b/tools/libxl/libxl_dom.c
>> @@ -698,6 +698,7 @@ static int set_vnuma_info(libxl__gc *gc, uint32_t domid,
>>  }
>>  
>>  static int libxl__build_dom(libxl__gc *gc, uint32_t domid,
>> +             libxl_domain_config *d_config,
>>               libxl_domain_build_info *info, libxl__domain_build_state *state,
>>               struct xc_dom_image *dom)
>>  {
>> @@ -737,6 +738,11 @@ static int libxl__build_dom(libxl__gc *gc, uint32_t domid,
>>          LOGE(ERROR, "libxl__arch_domain_finalise_hw_description failed");
>>          goto out;
>>      }
>> +    ret = libxl__arch_domain_construct_memmap(gc, d_config, domid, dom);
>> +    if (ret != 0) {
>> +        LOG(ERROR, "setting domain memory map failed");
>> +        goto out;
>> +    }
> If you add this call here, shouldn't you remove the same one from
> libxl__build_hvm?

Hmm... I thought I removed it but apparently I didn't.

>
> Does it make sense to place this in the
> libxl__arch_domain_finalise_hw_description hook?
>
> On ARM libxl__arch_domain_construct_memmap it's just an empty wrapper
> anyway.

Yes, I can do that. And then remove ARM's
libxl__arch_domain_construct_memmap altogether.

-boris


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v3 3/4] libxl: Store PVH guest's e820 map in xc_dom_image

[Boris Ostrovsky]

On 03/16/2018 02:20 PM, Roger Pau Monné wrote:
> On Thu, Mar 15, 2018 at 02:35:17PM -0700, Maran Wilson wrote:
>> From: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>>
>> We will later copy it to hvm_start_info.
>>
>> (Also remove stale comment claming that xc_dom_image.start_info_seg is
>> only used for HVMlite guests)
>>
>> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>> ---
>>  tools/libxc/include/xc_dom.h | 8 +++++++-
>>  tools/libxl/libxl_x86.c      | 6 ++++++
>>  2 files changed, 13 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/libxc/include/xc_dom.h b/tools/libxc/include/xc_dom.h
>> index 491cad8..6ef68f9 100644
>> --- a/tools/libxc/include/xc_dom.h
>> +++ b/tools/libxc/include/xc_dom.h
>> @@ -99,7 +99,7 @@ struct xc_dom_image {
>>      struct xc_dom_seg p2m_seg;
>>      struct xc_dom_seg pgtables_seg;
>>      struct xc_dom_seg devicetree_seg;
>> -    struct xc_dom_seg start_info_seg; /* HVMlite only */
>> +    struct xc_dom_seg start_info_seg;
>>      xen_pfn_t start_info_pfn;
>>      xen_pfn_t console_pfn;
>>      xen_pfn_t xenstore_pfn;
>> @@ -224,6 +224,12 @@ struct xc_dom_image {
>>      /* Extra SMBIOS structures passed to HVMLOADER */
>>      struct xc_hvm_firmware_module smbios_module;
>>  
>> +    /* PVH guests */
>> +#if defined(__i386__) || defined(__x86_64__)
>> +    struct e820entry *e820;
>> +    unsigned int e820_entries;
>> +#endif
> Not sure whether it's worth protecting this with x86 guards,
> xc_dom_image it's already a mix of ARM/x86 fields without much
> protection.

struct e820entry is defined (in include/xenctrl.h) under ifdef.


>
>> +
>>      xen_pfn_t vuart_gfn;
>>  };
>>  
>> diff --git a/tools/libxl/libxl_x86.c b/tools/libxl/libxl_x86.c
>> index 3331cc5..0de278f 100644
>> --- a/tools/libxl/libxl_x86.c
>> +++ b/tools/libxl/libxl_x86.c
>> @@ -600,6 +600,12 @@ int libxl__arch_domain_construct_memmap(libxl__gc *gc,
>>          goto out;
>>      }
>>  
>> +    if (d_config->b_info.type == LIBXL_DOMAIN_TYPE_PVH) {
>> +        dom->e820 = libxl__malloc(gc, sizeof(struct e820entry) * e820_entries);
>> +        dom->e820_entries = e820_entries;
>> +        memcpy(dom->e820,  e820, e820_entries * sizeof(*(dom->e820)));
>                             ^ extra space.
>
> Also could you please use sizeof(*(dom->e820)) or sizeof(struct
> e820entry) consistently? (I think I prefer the latter).

I would actually prefer the former, I think we try not to use data types
in sizeof. But yes, I need to do it consistently.

>
> Also you want this for both PVH and HVM, since hvmloader now also uses
> the hvm_start_info structure to pass the BIOS blob as a module.

OK.

-boris

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v3 1/4] x86/PVHv2: Add memory map pointer to hvm_start_info struct

[Konrad Rzeszutek Wilk]

On Fri, Mar 16, 2018 at 10:00:54AM -0700, Maran Wilson wrote:
> On 3/16/2018 4:11 AM, Roger Pau Monné wrote:
> > On Thu, Mar 15, 2018 at 02:33:09PM -0700, Maran Wilson wrote:
> > > The start info structure that is defined as part of the x86/HVM direct boot
> > > ABI and used for starting Xen PVH guests would be more versatile if it also
> > > included a way to pass information about the memory map to the guest. This
> > > would allow KVM guests to share the same entry point.
> > > 
> > > Signed-off-by: Maran Wilson <maran.wilson@oracle.com>
> > > ---
> > >   xen/include/public/arch-x86/hvm/start_info.h | 66 +++++++++++++++++++++++++++-
> > >   1 file changed, 65 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/xen/include/public/arch-x86/hvm/start_info.h b/xen/include/public/arch-x86/hvm/start_info.h
> > > index 6484159..f2e8ba6 100644
> > > --- a/xen/include/public/arch-x86/hvm/start_info.h
> > > +++ b/xen/include/public/arch-x86/hvm/start_info.h
> > > @@ -33,8 +33,9 @@
> > >    *    | magic          | Contains the magic value XEN_HVM_START_MAGIC_VALUE
> > >    *    |                | ("xEn3" with the 0x80 bit of the "E" set).
> > >    *  4 +----------------+
> > > - *    | version        | Version of this structure. Current version is 0. New
> > > + *    | version        | Version of this structure. Current version is 1. New
> > >    *    |                | versions are guaranteed to be backwards-compatible.
> > > + *    |                | For PV guests only 0 allowed, for PVH 0 or 1 allowed.
> > Why are you adding the above sentence? PV guest never used or will use
> > the hvm_start_info structure (note the hvm_ prefix).
> 
> Thanks for the feed back Roger,
> 
> As you noticed, my first version did not contain that comment. Konrad
> suggested adding that particular line (in reply to the Linux tree version of
> this patch) and no one seemed to object at the time so I went ahead and
> added it.
> 
> Konrad, do you care to weigh in here?

Could point about the hvm. My thought was more if somebody starts comparing
the 'start_info' structures they may get confused.

It totally is fine to ditch my idea.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v4 0/4] x86/PVHv2: Add memory map pointer to hvm_start_info struct

[Maran Wilson]

Here is the patch series for updating the canonical definition of the
hvm_start_info struct corresponding to the discussion happening on the
linux-kernel and kvm mailing lists regarding Qemu/KVM use of the PVH
entry point:

   KVM: x86: Allow Qemu/KVM to use PVH entry point
   https://lkml.org/lkml/2018/2/28/1121

Patch 1 contains all the changes to the hvm_start_info struct and
patches 2-4 modify Xen to use the new memory map fields of the structure.

Changes since v3:
 * Cleaned up hard tabs in start_info.h (patch 1)
 * Removed comment about "For PV guests only 0 allowed, for PVH 0 or 1
   allowed" from start_info.h (patch 1)
 * Make the map available to both HVM and PVH guests (patches 2-4)
 * Re-organize libxl changes (patches 2-4)

Changes since v2:
 * Better definition of the memory map types including addition of new
   symbols and tightening up the comments as suggested.
 * Added a couple of BUILD_BUG_ON() statements to the c code in patch #4
   to document and verify the relationship between these memory types
   and e820 types.

Changes since v1:
 * Made updates to code comments as suggested by Jan and Roger, including
   better definition of the memory map type field.   
 * Boris provided additional patches to populate the new fields in the
   hvm_start_info struct as Jan (and later Roger also) had requested.

Boris Ostrovsky (3):
  libxl/x86: Build e820 map earlier for HVM/PVH guests
  libxl: Store e820 map in xc_dom_image
  libxc: Pass e820 map to HVM/PVH guests via hvm_start_info

Maran Wilson (1):
  x86/PVHv2: Add memory map pointer to hvm_start_info struct

 tools/libxc/include/xc_dom.h                 |  7 ++-
 tools/libxc/xc_dom_x86.c                     | 29 ++++++++++++-
 tools/libxl/libxl_arch.h                     |  9 +---
 tools/libxl/libxl_arm.c                      | 10 +----
 tools/libxl/libxl_create.c                   |  2 +-
 tools/libxl/libxl_dom.c                      | 16 +++----
 tools/libxl/libxl_internal.h                 |  1 +
 tools/libxl/libxl_x86.c                      | 53 +++++++++++++++--------
 xen/include/public/arch-x86/hvm/start_info.h | 65 +++++++++++++++++++++++++++-
 9 files changed, 144 insertions(+), 48 deletions(-)

-- 
1.8.3.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v4 1/4] x86/PVHv2: Add memory map pointer to hvm_start_info struct

[Maran Wilson]

The start info structure that is defined as part of the x86/HVM direct boot
ABI and used for starting Xen PVH guests would be more versatile if it also
included a way to pass information about the memory map to the guest. This
would allow KVM guests to share the same entry point.

Signed-off-by: Maran Wilson <maran.wilson@oracle.com>
---
 xen/include/public/arch-x86/hvm/start_info.h | 65 +++++++++++++++++++++++++++-
 1 file changed, 64 insertions(+), 1 deletion(-)

diff --git a/xen/include/public/arch-x86/hvm/start_info.h b/xen/include/public/arch-x86/hvm/start_info.h
index 6484159..d491f2d 100644
--- a/xen/include/public/arch-x86/hvm/start_info.h
+++ b/xen/include/public/arch-x86/hvm/start_info.h
@@ -33,7 +33,7 @@
  *    | magic          | Contains the magic value XEN_HVM_START_MAGIC_VALUE
  *    |                | ("xEn3" with the 0x80 bit of the "E" set).
  *  4 +----------------+
- *    | version        | Version of this structure. Current version is 0. New
+ *    | version        | Version of this structure. Current version is 1. New
  *    |                | versions are guaranteed to be backwards-compatible.
  *  8 +----------------+
  *    | flags          | SIF_xxx flags.
@@ -48,6 +48,15 @@
  * 32 +----------------+
  *    | rsdp_paddr     | Physical address of the RSDP ACPI data structure.
  * 40 +----------------+
+ *    | memmap_paddr   | Physical address of the (optional) memory map. Only
+ *    |                | present in version 1 and newer of the structure.
+ * 48 +----------------+
+ *    | memmap_entries | Number of entries in the memory map table. Zero
+ *    |                | if there is no memory map being provided. Only
+ *    |                | present in version 1 and newer of the structure.
+ * 52 +----------------+
+ *    | reserved       | Version 1 and newer only.
+ * 56 +----------------+
  *
  * The layout of each entry in the module structure is the following:
  *
@@ -62,14 +71,53 @@
  *    | reserved       |
  * 32 +----------------+
  *
+ * The layout of each entry in the memory map table is as follows:
+ *
+ *  0 +----------------+
+ *    | addr           | Base address
+ *  8 +----------------+
+ *    | size           | Size of mapping in bytes
+ * 16 +----------------+
+ *    | type           | Type of mapping as defined between the hypervisor
+ *    |                | and guest it's starting. See XEN_HVM_MEMMAP_TYPE_*
+ *    |                | values below.
+ * 20 +----------------|
+ *    | reserved       |
+ * 24 +----------------+
+ *
  * The address and sizes are always a 64bit little endian unsigned integer.
  *
  * NB: Xen on x86 will always try to place all the data below the 4GiB
  * boundary.
+ *
+ * Version numbers of the hvm_start_info structure have evolved like this:
+ *
+ * Version 0:  Initial implementation.
+ *
+ * Version 1:  Added the memmap_paddr/memmap_entries fields (plus 4 bytes of
+ *             padding) to the end of the hvm_start_info struct. These new
+ *             fields can be used to pass a memory map to the guest. The
+ *             memory map is optional and so guests that understand version 1
+ *             of the structure must check that memmap_entries is non-zero
+ *             before trying to read the memory map.
  */
 #define XEN_HVM_START_MAGIC_VALUE 0x336ec578
 
 /*
+ * The values used in the type field of the memory map table entries are
+ * defined below and match the Address Range Types as defined in the "System
+ * Address Map Interfaces" section of the ACPI Specification. Please refer to
+ * section 15 in version 6.2 of the ACPI spec: http://uefi.org/specifications
+ */
+#define XEN_HVM_MEMMAP_TYPE_RAM       1
+#define XEN_HVM_MEMMAP_TYPE_RESERVED  2
+#define XEN_HVM_MEMMAP_TYPE_ACPI      3
+#define XEN_HVM_MEMMAP_TYPE_NVS       4
+#define XEN_HVM_MEMMAP_TYPE_UNUSABLE  5
+#define XEN_HVM_MEMMAP_TYPE_DISABLED  6
+#define XEN_HVM_MEMMAP_TYPE_PMEM      7
+
+/*
  * C representation of the x86/HVM start info layout.
  *
  * The canonical definition of this layout is above, this is just a way to
@@ -86,6 +134,14 @@ struct hvm_start_info {
     uint64_t cmdline_paddr;     /* Physical address of the command line.     */
     uint64_t rsdp_paddr;        /* Physical address of the RSDP ACPI data    */
                                 /* structure.                                */
+    uint64_t memmap_paddr;      /* Physical address of an array of           */
+                                /* hvm_memmap_table_entry. Only present in   */
+                                /* version 1 and newer of the structure      */
+    uint32_t memmap_entries;    /* Number of entries in the memmap table.    */
+                                /* Only present in version 1 and newer of    */
+                                /* the structure. Value will be zero if      */
+                                /* there is no memory map being provided.    */
+    uint32_t reserved;          /* Must be zero for Version 1.               */
 };
 
 struct hvm_modlist_entry {
@@ -95,4 +151,11 @@ struct hvm_modlist_entry {
     uint64_t reserved;
 };
 
+struct hvm_memmap_table_entry {
+    uint64_t addr;              /* Base address of the memory region         */
+    uint64_t size;              /* Size of the memory region in bytes        */
+    uint32_t type;              /* Mapping type                              */
+    uint32_t reserved;          /* Must be zero for Version 1.               */
+};
+
 #endif /* __XEN_PUBLIC_ARCH_X86_HVM_START_INFO_H__ */
-- 
1.8.3.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v4 2/4] libxl/x86: Build e820 map earlier for HVM/PVH guests

[Maran Wilson]

From: Boris Ostrovsky <boris.ostrovsky@oracle.com>

Since hvm_start_info has now been expanded to include memory map (i.e.
e820) we need to know size of this map by the time we create
dom->start_info_seg in alloc_magic_pages_hvm().

To do so we have to call libxl__arch_domain_construct_memmap() earlier,
before xc_dom_build_image(). And since libxl__arch_domain_construct_memmap()
is only used by for x86 we can make this call from x86's
libxl__arch_domain_finalise_hw_description(), at the same time removing
its NOP definition from ARM code and renaming and making it static in
libxl_x86.c

Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
---
 tools/libxl/libxl_arch.h     |  9 ++------
 tools/libxl/libxl_arm.c      | 10 ++-------
 tools/libxl/libxl_create.c   |  2 +-
 tools/libxl/libxl_dom.c      | 16 ++++++---------
 tools/libxl/libxl_internal.h |  1 +
 tools/libxl/libxl_x86.c      | 49 +++++++++++++++++++++++++++-----------------
 6 files changed, 42 insertions(+), 45 deletions(-)

diff --git a/tools/libxl/libxl_arch.h b/tools/libxl/libxl_arch.h
index 784ec7f..9be06fa 100644
--- a/tools/libxl/libxl_arch.h
+++ b/tools/libxl/libxl_arch.h
@@ -41,6 +41,8 @@ int libxl__arch_domain_init_hw_description(libxl__gc *gc,
 /* finalize arch specific hardware description. */
 _hidden
 int libxl__arch_domain_finalise_hw_description(libxl__gc *gc,
+                                      uint32_t domid,
+                                      libxl_domain_config *d_config,
                                       libxl_domain_build_info *info,
                                       struct xc_dom_image *dom);
 
@@ -62,13 +64,6 @@ int libxl__arch_vnuma_build_vmemrange(libxl__gc *gc,
 _hidden
 int libxl__arch_domain_map_irq(libxl__gc *gc, uint32_t domid, int irq);
 
-/* arch specific to construct memory mapping function */
-_hidden
-int libxl__arch_domain_construct_memmap(libxl__gc *gc,
-                                        libxl_domain_config *d_config,
-                                        uint32_t domid,
-                                        struct xc_dom_image *dom);
-
 _hidden
 void libxl__arch_domain_build_info_acpi_setdefault(
                                         libxl_domain_build_info *b_info);
diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
index 3e46554..13a8347 100644
--- a/tools/libxl/libxl_arm.c
+++ b/tools/libxl/libxl_arm.c
@@ -1046,6 +1046,8 @@ static void finalise_one_node(libxl__gc *gc, void *fdt, const char *uname,
 }
 
 int libxl__arch_domain_finalise_hw_description(libxl__gc *gc,
+                                               uint32_t domid,
+                                               libxl_domain_config *d_config,
                                                libxl_domain_build_info *info,
                                                struct xc_dom_image *dom)
 {
@@ -1140,14 +1142,6 @@ int libxl__arch_domain_map_irq(libxl__gc *gc, uint32_t domid, int irq)
     return xc_domain_bind_pt_spi_irq(CTX->xch, domid, irq, irq);
 }
 
-int libxl__arch_domain_construct_memmap(libxl__gc *gc,
-                                        libxl_domain_config *d_config,
-                                        uint32_t domid,
-                                        struct xc_dom_image *dom)
-{
-    return 0;
-}
-
 void libxl__arch_domain_build_info_acpi_setdefault(
                                         libxl_domain_build_info *b_info)
 {
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index c498135..5dce3df 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -488,7 +488,7 @@ int libxl__domain_build(libxl__gc *gc,
 
         break;
     case LIBXL_DOMAIN_TYPE_PV:
-        ret = libxl__build_pv(gc, domid, info, state);
+        ret = libxl__build_pv(gc, domid, d_config, info, state);
         if (ret)
             goto out;
 
diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index 2e29b52..e83aeb9 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -698,6 +698,7 @@ static int set_vnuma_info(libxl__gc *gc, uint32_t domid,
 }
 
 static int libxl__build_dom(libxl__gc *gc, uint32_t domid,
+             libxl_domain_config *d_config,
              libxl_domain_build_info *info, libxl__domain_build_state *state,
              struct xc_dom_image *dom)
 {
@@ -733,7 +734,8 @@ static int libxl__build_dom(libxl__gc *gc, uint32_t domid,
         LOGE(ERROR, "xc_dom_boot_mem_init failed");
         goto out;
     }
-    if ( (ret = libxl__arch_domain_finalise_hw_description(gc, info, dom)) != 0 ) {
+    if ( (ret = libxl__arch_domain_finalise_hw_description(gc, domid, d_config,
+                                                           info, dom)) != 0 ) {
         LOGE(ERROR, "libxl__arch_domain_finalise_hw_description failed");
         goto out;
     }
@@ -758,7 +760,7 @@ out:
     return ret != 0 ? ERROR_FAIL : 0;
 }
 
-int libxl__build_pv(libxl__gc *gc, uint32_t domid,
+int libxl__build_pv(libxl__gc *gc, uint32_t domid, libxl_domain_config *d_config,
              libxl_domain_build_info *info, libxl__domain_build_state *state)
 {
     libxl_ctx *ctx = libxl__gc_owner(gc);
@@ -847,7 +849,7 @@ int libxl__build_pv(libxl__gc *gc, uint32_t domid,
             dom->vnode_to_pnode[i] = info->vnuma_nodes[i].pnode;
     }
 
-    ret = libxl__build_dom(gc, domid, info, state, dom);
+    ret = libxl__build_dom(gc, domid, d_config, info, state, dom);
     if (ret != 0)
         goto out;
 
@@ -1293,16 +1295,10 @@ int libxl__build_hvm(libxl__gc *gc, uint32_t domid,
             dom->vnode_to_pnode[i] = info->vnuma_nodes[i].pnode;
     }
 
-    rc = libxl__build_dom(gc, domid, info, state, dom);
+    rc = libxl__build_dom(gc, domid, d_config, info, state, dom);
     if (rc != 0)
         goto out;
 
-    rc = libxl__arch_domain_construct_memmap(gc, d_config, domid, dom);
-    if (rc != 0) {
-        LOG(ERROR, "setting domain memory map failed");
-        goto out;
-    }
-
     rc = hvm_build_set_params(ctx->xch, domid, info, state->store_port,
                                &state->store_mfn, state->console_port,
                                &state->console_mfn, state->store_domid,
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 506687f..914df23 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1159,6 +1159,7 @@ _hidden int libxl__build_post(libxl__gc *gc, uint32_t domid,
                char **vms_ents, char **local_ents);
 
 _hidden int libxl__build_pv(libxl__gc *gc, uint32_t domid,
+                            libxl_domain_config *const d_config,
              libxl_domain_build_info *info, libxl__domain_build_state *state);
 _hidden int libxl__build_hvm(libxl__gc *gc, uint32_t domid,
               libxl_domain_config *d_config,
diff --git a/tools/libxl/libxl_x86.c b/tools/libxl/libxl_x86.c
index d82013f..7cbbfd0 100644
--- a/tools/libxl/libxl_x86.c
+++ b/tools/libxl/libxl_x86.c
@@ -377,21 +377,6 @@ int libxl__arch_domain_init_hw_description(libxl__gc *gc,
     return 0;
 }
 
-int libxl__arch_domain_finalise_hw_description(libxl__gc *gc,
-                                               libxl_domain_build_info *info,
-                                               struct xc_dom_image *dom)
-{
-    int rc = 0;
-
-    if (info->type == LIBXL_DOMAIN_TYPE_PVH) {
-        rc = libxl__dom_load_acpi(gc, info, dom);
-        if (rc != 0)
-            LOGE(ERROR, "libxl_dom_load_acpi failed");
-    }
-
-    return rc;
-}
-
 int libxl__arch_build_dom_finish(libxl__gc *gc,
                                  libxl_domain_build_info *info,
                                  struct xc_dom_image *dom,
@@ -510,10 +495,10 @@ int libxl__arch_domain_map_irq(libxl__gc *gc, uint32_t domid, int irq)
  * to adjust them. Please refer to libxl__domain_device_construct_rdm().
  */
 #define GUEST_LOW_MEM_START_DEFAULT 0x100000
-int libxl__arch_domain_construct_memmap(libxl__gc *gc,
-                                        libxl_domain_config *d_config,
-                                        uint32_t domid,
-                                        struct xc_dom_image *dom)
+static int domain_construct_memmap(libxl__gc *gc,
+                                   libxl_domain_config *d_config,
+                                   uint32_t domid,
+                                   struct xc_dom_image *dom)
 {
     int rc = 0;
     unsigned int nr = 0, i;
@@ -601,6 +586,32 @@ out:
     return rc;
 }
 
+int libxl__arch_domain_finalise_hw_description(libxl__gc *gc,
+                                               uint32_t domid,
+                                               libxl_domain_config *d_config,
+                                               libxl_domain_build_info *info,
+                                               struct xc_dom_image *dom)
+{
+    int rc;
+
+    if (info->type == LIBXL_DOMAIN_TYPE_PV)
+        return 0;
+
+    if (info->type == LIBXL_DOMAIN_TYPE_PVH) {
+        rc = libxl__dom_load_acpi(gc, info, dom);
+        if (rc != 0) {
+            LOGE(ERROR, "libxl_dom_load_acpi failed");
+            return rc;
+        }
+    }
+
+    rc = domain_construct_memmap(gc, d_config, domid, dom);
+    if (rc != 0)
+        LOGE(ERROR, "setting domain memory map failed");
+
+    return rc;
+}
+
 void libxl__arch_domain_build_info_acpi_setdefault(
                                         libxl_domain_build_info *b_info)
 {
-- 
1.8.3.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v4 3/4] libxl: Store e820 map in xc_dom_image

[Maran Wilson]

From: Boris Ostrovsky <boris.ostrovsky@oracle.com>

We will later copy it to hvm_start_info.

(Also remove stale comment claming that xc_dom_image.start_info_seg is
only used for HVMlite guests)

Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
---
 tools/libxc/include/xc_dom.h | 7 ++++++-
 tools/libxl/libxl_x86.c      | 4 ++++
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/tools/libxc/include/xc_dom.h b/tools/libxc/include/xc_dom.h
index 491cad8..8a66889 100644
--- a/tools/libxc/include/xc_dom.h
+++ b/tools/libxc/include/xc_dom.h
@@ -99,7 +99,7 @@ struct xc_dom_image {
     struct xc_dom_seg p2m_seg;
     struct xc_dom_seg pgtables_seg;
     struct xc_dom_seg devicetree_seg;
-    struct xc_dom_seg start_info_seg; /* HVMlite only */
+    struct xc_dom_seg start_info_seg;
     xen_pfn_t start_info_pfn;
     xen_pfn_t console_pfn;
     xen_pfn_t xenstore_pfn;
@@ -224,6 +224,11 @@ struct xc_dom_image {
     /* Extra SMBIOS structures passed to HVMLOADER */
     struct xc_hvm_firmware_module smbios_module;
 
+#if defined(__i386__) || defined(__x86_64__)
+    struct e820entry *e820;
+    unsigned int e820_entries;
+#endif
+
     xen_pfn_t vuart_gfn;
 };
 
diff --git a/tools/libxl/libxl_x86.c b/tools/libxl/libxl_x86.c
index 7cbbfd0..651b7d5 100644
--- a/tools/libxl/libxl_x86.c
+++ b/tools/libxl/libxl_x86.c
@@ -582,6 +582,10 @@ static int domain_construct_memmap(libxl__gc *gc,
         goto out;
     }
 
+    dom->e820 = libxl__malloc(gc, e820_entries * sizeof(*(dom->e820)));
+    dom->e820_entries = e820_entries;
+    memcpy(dom->e820, e820, e820_entries * sizeof(*(dom->e820)));
+
 out:
     return rc;
 }
-- 
1.8.3.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v4 4/4] libxc: Pass e820 map to HVM/PVH guests via hvm_start_info

[Maran Wilson]

From: Boris Ostrovsky <boris.ostrovsky@oracle.com>

Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Signed-off-by: Maran Wilson <maran.wilson@oracle.com>
---
 tools/libxc/xc_dom_x86.c | 29 ++++++++++++++++++++++++++++-
 1 file changed, 28 insertions(+), 1 deletion(-)

diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
index 0b65dab..b2d8403 100644
--- a/tools/libxc/xc_dom_x86.c
+++ b/tools/libxc/xc_dom_x86.c
@@ -35,6 +35,8 @@
 #include <xen/arch-x86/hvm/start_info.h>
 #include <xen/io/protocols.h>
 
+#include <xen-tools/libs.h>
+
 #include "xg_private.h"
 #include "xc_dom.h"
 #include "xenctrl.h"
@@ -640,6 +642,8 @@ static int alloc_magic_pages_hvm(struct xc_dom_image *dom)
             dom->cmdline_size = ROUNDUP(strlen(dom->cmdline) + 1, 8);
             start_info_size += dom->cmdline_size;
         }
+
+        start_info_size += dom->e820_entries * sizeof(*(dom->e820));
     }
     else
     {
@@ -1666,8 +1670,9 @@ 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;
+    size_t start_info_size, modsize;
     struct hvm_modlist_entry *modlist;
+    struct hvm_memmap_table_entry *memmap;
     unsigned int i;
 
     start_info_size = sizeof(*start_info) + dom->cmdline_size;
@@ -1731,7 +1736,29 @@ static int bootlate_hvm(struct xc_dom_image *dom)
                             ((uintptr_t)modlist - (uintptr_t)start_info);
     }
 
+    /*
+     * Check a couple of XEN_HVM_MEMMAP_TYPEs to verify consistency with
+     * their corresponding e820 numerical values.
+     */
+    BUILD_BUG_ON(XEN_HVM_MEMMAP_TYPE_RAM != E820_RAM);
+    BUILD_BUG_ON(XEN_HVM_MEMMAP_TYPE_ACPI != E820_ACPI);
+
+    modsize = HVMLOADER_MODULE_MAX_COUNT *
+        (sizeof(*modlist) + HVMLOADER_MODULE_CMDLINE_SIZE);
+    memmap = (void*)modlist + modsize;
+
+    start_info->memmap_paddr = (dom->start_info_seg.pfn << PAGE_SHIFT) +
+        ((uintptr_t)modlist - (uintptr_t)start_info) + modsize;
+    start_info->memmap_entries = dom->e820_entries;
+    for ( i = 0; i < dom->e820_entries; i++ )
+    {
+        memmap[i].addr = dom->e820[i].addr;
+        memmap[i].size = dom->e820[i].size;
+        memmap[i].type = dom->e820[i].type;
+    }
+
     start_info->magic = XEN_HVM_START_MAGIC_VALUE;
+    start_info->version = 1;
 
     munmap(start_info, start_info_size);
 
-- 
1.8.3.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v4 1/4] x86/PVHv2: Add memory map pointer to hvm_start_info struct

[Jan Beulich]

>>> On 20.03.18 at 17:48, <maran.wilson@oracle.com> wrote:
> The start info structure that is defined as part of the x86/HVM direct boot
> ABI and used for starting Xen PVH guests would be more versatile if it also
> included a way to pass information about the memory map to the guest. This
> would allow KVM guests to share the same entry point.
> 
> Signed-off-by: Maran Wilson <maran.wilson@oracle.com>

Once again
Acked-by: Jan Beulich <jbeulich@suse.com>

But I'd like Roger to confirm his concerns have all been dealt with.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v4 1/4] x86/PVHv2: Add memory map pointer to hvm_start_info struct

[Roger Pau Monné]

On Tue, Mar 20, 2018 at 09:48:56AM -0700, Maran Wilson wrote:
> The start info structure that is defined as part of the x86/HVM direct boot
> ABI and used for starting Xen PVH guests would be more versatile if it also
> included a way to pass information about the memory map to the guest. This
> would allow KVM guests to share the same entry point.
> 
> Signed-off-by: Maran Wilson <maran.wilson@oracle.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Just a couple of nit suggestions...

> ---
>  xen/include/public/arch-x86/hvm/start_info.h | 65 +++++++++++++++++++++++++++-
>  1 file changed, 64 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/include/public/arch-x86/hvm/start_info.h b/xen/include/public/arch-x86/hvm/start_info.h
> index 6484159..d491f2d 100644
> --- a/xen/include/public/arch-x86/hvm/start_info.h
> +++ b/xen/include/public/arch-x86/hvm/start_info.h
> @@ -33,7 +33,7 @@
>   *    | magic          | Contains the magic value XEN_HVM_START_MAGIC_VALUE
>   *    |                | ("xEn3" with the 0x80 bit of the "E" set).
>   *  4 +----------------+
> - *    | version        | Version of this structure. Current version is 0. New
> + *    | version        | Version of this structure. Current version is 1. New
>   *    |                | versions are guaranteed to be backwards-compatible.
>   *  8 +----------------+
>   *    | flags          | SIF_xxx flags.
> @@ -48,6 +48,15 @@
>   * 32 +----------------+
>   *    | rsdp_paddr     | Physical address of the RSDP ACPI data structure.
>   * 40 +----------------+
> + *    | memmap_paddr   | Physical address of the (optional) memory map. Only
> + *    |                | present in version 1 and newer of the structure.
> + * 48 +----------------+
> + *    | memmap_entries | Number of entries in the memory map table. Zero
> + *    |                | if there is no memory map being provided. Only
> + *    |                | present in version 1 and newer of the structure.
> + * 52 +----------------+
> + *    | reserved       | Version 1 and newer only.
> + * 56 +----------------+
>   *
>   * The layout of each entry in the module structure is the following:
>   *
> @@ -62,14 +71,53 @@
>   *    | reserved       |
>   * 32 +----------------+
>   *
> + * The layout of each entry in the memory map table is as follows:
> + *
> + *  0 +----------------+
> + *    | addr           | Base address
> + *  8 +----------------+
> + *    | size           | Size of mapping in bytes
> + * 16 +----------------+
> + *    | type           | Type of mapping as defined between the hypervisor
> + *    |                | and guest it's starting. See XEN_HVM_MEMMAP_TYPE_*

I would remove "it's starting" here.

> + *    |                | values below.
> + * 20 +----------------|
> + *    | reserved       |
> + * 24 +----------------+
> + *
>   * The address and sizes are always a 64bit little endian unsigned integer.
>   *
>   * NB: Xen on x86 will always try to place all the data below the 4GiB
>   * boundary.
> + *
> + * Version numbers of the hvm_start_info structure have evolved like this:
> + *
> + * Version 0:  Initial implementation.
> + *
> + * Version 1:  Added the memmap_paddr/memmap_entries fields (plus 4 bytes of
> + *             padding) to the end of the hvm_start_info struct. These new
> + *             fields can be used to pass a memory map to the guest. The
> + *             memory map is optional and so guests that understand version 1
> + *             of the structure must check that memmap_entries is non-zero
> + *             before trying to read the memory map.
>   */
>  #define XEN_HVM_START_MAGIC_VALUE 0x336ec578
>  
>  /*
> + * The values used in the type field of the memory map table entries are
> + * defined below and match the Address Range Types as defined in the "System
> + * Address Map Interfaces" section of the ACPI Specification. Please refer to
> + * section 15 in version 6.2 of the ACPI spec: http://uefi.org/specifications
> + */
> +#define XEN_HVM_MEMMAP_TYPE_RAM       1
> +#define XEN_HVM_MEMMAP_TYPE_RESERVED  2
> +#define XEN_HVM_MEMMAP_TYPE_ACPI      3
> +#define XEN_HVM_MEMMAP_TYPE_NVS       4
> +#define XEN_HVM_MEMMAP_TYPE_UNUSABLE  5
> +#define XEN_HVM_MEMMAP_TYPE_DISABLED  6
> +#define XEN_HVM_MEMMAP_TYPE_PMEM      7
> +
> +/*
>   * C representation of the x86/HVM start info layout.
>   *
>   * The canonical definition of this layout is above, this is just a way to
> @@ -86,6 +134,14 @@ struct hvm_start_info {
>      uint64_t cmdline_paddr;     /* Physical address of the command line.     */
>      uint64_t rsdp_paddr;        /* Physical address of the RSDP ACPI data    */
>                                  /* structure.                                */
> +    uint64_t memmap_paddr;      /* Physical address of an array of           */
> +                                /* hvm_memmap_table_entry. Only present in   */
> +                                /* version 1 and newer of the structure      */
> +    uint32_t memmap_entries;    /* Number of entries in the memmap table.    */
> +                                /* Only present in version 1 and newer of    */
> +                                /* the structure. Value will be zero if      */
> +                                /* there is no memory map being provided.    */
> +    uint32_t reserved;          /* Must be zero for Version 1.               */

I would write "Must be zero." only. If at some point we introduce
version 2 we would likely have to fixup this comment to mention
version 1 and version 2.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v4 2/4] libxl/x86: Build e820 map earlier for HVM/PVH guests

[Roger Pau Monné]

On Tue, Mar 20, 2018 at 09:50:50AM -0700, Maran Wilson wrote:
> From: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> 
> Since hvm_start_info has now been expanded to include memory map (i.e.
> e820) we need to know size of this map by the time we create
> dom->start_info_seg in alloc_magic_pages_hvm().
> 
> To do so we have to call libxl__arch_domain_construct_memmap() earlier,
> before xc_dom_build_image(). And since libxl__arch_domain_construct_memmap()
> is only used by for x86 we can make this call from x86's
> libxl__arch_domain_finalise_hw_description(), at the same time removing
> its NOP definition from ARM code and renaming and making it static in
> libxl_x86.c
> 
> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Just one nit.

> diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
> index 2e29b52..e83aeb9 100644
> --- a/tools/libxl/libxl_dom.c
> +++ b/tools/libxl/libxl_dom.c
> @@ -698,6 +698,7 @@ static int set_vnuma_info(libxl__gc *gc, uint32_t domid,
>  }
>  
>  static int libxl__build_dom(libxl__gc *gc, uint32_t domid,
> +             libxl_domain_config *d_config,
>               libxl_domain_build_info *info, libxl__domain_build_state *state,
>               struct xc_dom_image *dom)
>  {
> @@ -733,7 +734,8 @@ static int libxl__build_dom(libxl__gc *gc, uint32_t domid,
>          LOGE(ERROR, "xc_dom_boot_mem_init failed");
>          goto out;
>      }
> -    if ( (ret = libxl__arch_domain_finalise_hw_description(gc, info, dom)) != 0 ) {
> +    if ( (ret = libxl__arch_domain_finalise_hw_description(gc, domid, d_config,
> +                                                           info, dom)) != 0 ) {
>          LOGE(ERROR, "libxl__arch_domain_finalise_hw_description failed");
>          goto out;
>      }
> @@ -758,7 +760,7 @@ out:
>      return ret != 0 ? ERROR_FAIL : 0;
>  }
>  
> -int libxl__build_pv(libxl__gc *gc, uint32_t domid,
> +int libxl__build_pv(libxl__gc *gc, uint32_t domid, libxl_domain_config *d_config,
>               libxl_domain_build_info *info, libxl__domain_build_state *state)

If you pass libxl_domain_config as a parameter you no longer need to
pass libxl_domain_build_info, and you can do:

libxl_domain_build_info *const info = &d_config->b_info;

That would make the parameters of libxl__build_hvm and libxl__build_pv
the same :).

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v4 1/4] x86/PVHv2: Add memory map pointer to hvm_start_info struct

[Juergen Gross]

On 21/03/18 10:28, Roger Pau Monné wrote:
> On Tue, Mar 20, 2018 at 09:48:56AM -0700, Maran Wilson wrote:
>> The start info structure that is defined as part of the x86/HVM direct boot
>> ABI and used for starting Xen PVH guests would be more versatile if it also
>> included a way to pass information about the memory map to the guest. This
>> would allow KVM guests to share the same entry point.
>>
>> Signed-off-by: Maran Wilson <maran.wilson@oracle.com>
> 
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> Just a couple of nit suggestions...
> 
>> ---
>>  xen/include/public/arch-x86/hvm/start_info.h | 65 +++++++++++++++++++++++++++-
>>  1 file changed, 64 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/include/public/arch-x86/hvm/start_info.h b/xen/include/public/arch-x86/hvm/start_info.h
>> index 6484159..d491f2d 100644
>> --- a/xen/include/public/arch-x86/hvm/start_info.h
>> +++ b/xen/include/public/arch-x86/hvm/start_info.h
>> @@ -33,7 +33,7 @@
>>   *    | magic          | Contains the magic value XEN_HVM_START_MAGIC_VALUE
>>   *    |                | ("xEn3" with the 0x80 bit of the "E" set).
>>   *  4 +----------------+
>> - *    | version        | Version of this structure. Current version is 0. New
>> + *    | version        | Version of this structure. Current version is 1. New
>>   *    |                | versions are guaranteed to be backwards-compatible.
>>   *  8 +----------------+
>>   *    | flags          | SIF_xxx flags.
>> @@ -48,6 +48,15 @@
>>   * 32 +----------------+
>>   *    | rsdp_paddr     | Physical address of the RSDP ACPI data structure.
>>   * 40 +----------------+
>> + *    | memmap_paddr   | Physical address of the (optional) memory map. Only
>> + *    |                | present in version 1 and newer of the structure.
>> + * 48 +----------------+
>> + *    | memmap_entries | Number of entries in the memory map table. Zero
>> + *    |                | if there is no memory map being provided. Only
>> + *    |                | present in version 1 and newer of the structure.
>> + * 52 +----------------+
>> + *    | reserved       | Version 1 and newer only.
>> + * 56 +----------------+
>>   *
>>   * The layout of each entry in the module structure is the following:
>>   *
>> @@ -62,14 +71,53 @@
>>   *    | reserved       |
>>   * 32 +----------------+
>>   *
>> + * The layout of each entry in the memory map table is as follows:
>> + *
>> + *  0 +----------------+
>> + *    | addr           | Base address
>> + *  8 +----------------+
>> + *    | size           | Size of mapping in bytes
>> + * 16 +----------------+
>> + *    | type           | Type of mapping as defined between the hypervisor
>> + *    |                | and guest it's starting. See XEN_HVM_MEMMAP_TYPE_*
> 
> I would remove "it's starting" here.
> 
>> + *    |                | values below.
>> + * 20 +----------------|
>> + *    | reserved       |
>> + * 24 +----------------+
>> + *
>>   * The address and sizes are always a 64bit little endian unsigned integer.
>>   *
>>   * NB: Xen on x86 will always try to place all the data below the 4GiB
>>   * boundary.
>> + *
>> + * Version numbers of the hvm_start_info structure have evolved like this:
>> + *
>> + * Version 0:  Initial implementation.
>> + *
>> + * Version 1:  Added the memmap_paddr/memmap_entries fields (plus 4 bytes of
>> + *             padding) to the end of the hvm_start_info struct. These new
>> + *             fields can be used to pass a memory map to the guest. The
>> + *             memory map is optional and so guests that understand version 1
>> + *             of the structure must check that memmap_entries is non-zero
>> + *             before trying to read the memory map.
>>   */
>>  #define XEN_HVM_START_MAGIC_VALUE 0x336ec578
>>  
>>  /*
>> + * The values used in the type field of the memory map table entries are
>> + * defined below and match the Address Range Types as defined in the "System
>> + * Address Map Interfaces" section of the ACPI Specification. Please refer to
>> + * section 15 in version 6.2 of the ACPI spec: http://uefi.org/specifications
>> + */
>> +#define XEN_HVM_MEMMAP_TYPE_RAM       1
>> +#define XEN_HVM_MEMMAP_TYPE_RESERVED  2
>> +#define XEN_HVM_MEMMAP_TYPE_ACPI      3
>> +#define XEN_HVM_MEMMAP_TYPE_NVS       4
>> +#define XEN_HVM_MEMMAP_TYPE_UNUSABLE  5
>> +#define XEN_HVM_MEMMAP_TYPE_DISABLED  6
>> +#define XEN_HVM_MEMMAP_TYPE_PMEM      7
>> +
>> +/*
>>   * C representation of the x86/HVM start info layout.
>>   *
>>   * The canonical definition of this layout is above, this is just a way to
>> @@ -86,6 +134,14 @@ struct hvm_start_info {
>>      uint64_t cmdline_paddr;     /* Physical address of the command line.     */
>>      uint64_t rsdp_paddr;        /* Physical address of the RSDP ACPI data    */
>>                                  /* structure.                                */
>> +    uint64_t memmap_paddr;      /* Physical address of an array of           */
>> +                                /* hvm_memmap_table_entry. Only present in   */
>> +                                /* version 1 and newer of the structure      */
>> +    uint32_t memmap_entries;    /* Number of entries in the memmap table.    */
>> +                                /* Only present in version 1 and newer of    */
>> +                                /* the structure. Value will be zero if      */
>> +                                /* there is no memory map being provided.    */
>> +    uint32_t reserved;          /* Must be zero for Version 1.               */
> 
> I would write "Must be zero." only. If at some point we introduce
> version 2 we would likely have to fixup this comment to mention
> version 1 and version 2.

In case you are going this route I'd suggest to drop the version remarks
for the individual fields and just add a comment like:

/* All following fields only present in version 1 and newer. */

above memmap_paddr.


Juergen


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v4 3/4] libxl: Store e820 map in xc_dom_image

[Roger Pau Monné]

On Tue, Mar 20, 2018 at 09:50:51AM -0700, Maran Wilson wrote:
> From: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> diff --git a/tools/libxl/libxl_x86.c b/tools/libxl/libxl_x86.c
> index 7cbbfd0..651b7d5 100644
> --- a/tools/libxl/libxl_x86.c
> +++ b/tools/libxl/libxl_x86.c
> @@ -582,6 +582,10 @@ static int domain_construct_memmap(libxl__gc *gc,
>          goto out;
>      }
>  
> +    dom->e820 = libxl__malloc(gc, e820_entries * sizeof(*(dom->e820)));
> +    dom->e820_entries = e820_entries;
> +    memcpy(dom->e820, e820, e820_entries * sizeof(*(dom->e820)));

e820 is already allocated with libxl_malloc, why not simply use:

dom->e820 = e820;
dom->e820_entries = e820_entries;

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v4 4/4] libxc: Pass e820 map to HVM/PVH guests via hvm_start_info

[Roger Pau Monné]

On Tue, Mar 20, 2018 at 09:50:52AM -0700, Maran Wilson wrote:
> From: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> 
> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Signed-off-by: Maran Wilson <maran.wilson@oracle.com>
> ---
>  tools/libxc/xc_dom_x86.c | 29 ++++++++++++++++++++++++++++-
>  1 file changed, 28 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
> index 0b65dab..b2d8403 100644
> --- a/tools/libxc/xc_dom_x86.c
> +++ b/tools/libxc/xc_dom_x86.c
> @@ -35,6 +35,8 @@
>  #include <xen/arch-x86/hvm/start_info.h>
>  #include <xen/io/protocols.h>
>  
> +#include <xen-tools/libs.h>
> +
>  #include "xg_private.h"
>  #include "xc_dom.h"
>  #include "xenctrl.h"
> @@ -640,6 +642,8 @@ static int alloc_magic_pages_hvm(struct xc_dom_image *dom)
>              dom->cmdline_size = ROUNDUP(strlen(dom->cmdline) + 1, 8);
>              start_info_size += dom->cmdline_size;
>          }
> +
> +        start_info_size += dom->e820_entries * sizeof(*(dom->e820));

This is not correct because sizeof(struct e820entry) != sizeof(struct
hvm_modlist_entry) AFAICT. This should instead be sizeof(struct
hvm_modlist_entry).

>      }
>      else
>      {
> @@ -1666,8 +1670,9 @@ 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;
> +    size_t start_info_size, modsize;
>      struct hvm_modlist_entry *modlist;
> +    struct hvm_memmap_table_entry *memmap;
>      unsigned int i;
>  
>      start_info_size = sizeof(*start_info) + dom->cmdline_size;
> @@ -1731,7 +1736,29 @@ static int bootlate_hvm(struct xc_dom_image *dom)
>                              ((uintptr_t)modlist - (uintptr_t)start_info);
>      }
>  
> +    /*
> +     * Check a couple of XEN_HVM_MEMMAP_TYPEs to verify consistency with
> +     * their corresponding e820 numerical values.
> +     */
> +    BUILD_BUG_ON(XEN_HVM_MEMMAP_TYPE_RAM != E820_RAM);
> +    BUILD_BUG_ON(XEN_HVM_MEMMAP_TYPE_ACPI != E820_ACPI);
> +
> +    modsize = HVMLOADER_MODULE_MAX_COUNT *
> +        (sizeof(*modlist) + HVMLOADER_MODULE_CMDLINE_SIZE);

Hm, I'm not sure this is fully correct, but I think there are previous
issues in this area.

The mapped area (start_info) is of size sizeof(*start_info) +
dom->cmdline_size + sizeof(struct hvm_modlist_entry) *
dom->num_modules. Yet here you seem to assume num_modules ==
HVMLOADER_MODULE_MAX_COUNT?

Also the initial space calculation doesn't seem to take
HVMLOADER_MODULE_CMDLINE_SIZE into account at all.

And cmdline_paddr seems to be set to point to garbage if cmdline is not
set.

Or am I missing something?

Adding Jonathan Ludlam and Anthony PERARD who are the ones that added
this code.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v4 3/4] libxl: Store e820 map in xc_dom_image

[Boris Ostrovsky]

On 03/21/2018 05:42 AM, Roger Pau Monné wrote:
> On Tue, Mar 20, 2018 at 09:50:51AM -0700, Maran Wilson wrote:
>> From: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>> diff --git a/tools/libxl/libxl_x86.c b/tools/libxl/libxl_x86.c
>> index 7cbbfd0..651b7d5 100644
>> --- a/tools/libxl/libxl_x86.c
>> +++ b/tools/libxl/libxl_x86.c
>> @@ -582,6 +582,10 @@ static int domain_construct_memmap(libxl__gc *gc,
>>          goto out;
>>      }
>>  
>> +    dom->e820 = libxl__malloc(gc, e820_entries * sizeof(*(dom->e820)));
>> +    dom->e820_entries = e820_entries;
>> +    memcpy(dom->e820, e820, e820_entries * sizeof(*(dom->e820)));
> e820 is already allocated with libxl_malloc, why not simply use:
>
> dom->e820 = e820;
> dom->e820_entries = e820_entries;

Sure, I can do this.

-boris


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v4 4/4] libxc: Pass e820 map to HVM/PVH guests via hvm_start_info

[Boris Ostrovsky]

On 03/21/2018 06:07 AM, Roger Pau Monné wrote:
> On Tue, Mar 20, 2018 at 09:50:52AM -0700, Maran Wilson wrote:
>> From: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>>
>> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>> Signed-off-by: Maran Wilson <maran.wilson@oracle.com>
>> ---
>>  tools/libxc/xc_dom_x86.c | 29 ++++++++++++++++++++++++++++-
>>  1 file changed, 28 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
>> index 0b65dab..b2d8403 100644
>> --- a/tools/libxc/xc_dom_x86.c
>> +++ b/tools/libxc/xc_dom_x86.c
>> @@ -35,6 +35,8 @@
>>  #include <xen/arch-x86/hvm/start_info.h>
>>  #include <xen/io/protocols.h>
>>  
>> +#include <xen-tools/libs.h>
>> +
>>  #include "xg_private.h"
>>  #include "xc_dom.h"
>>  #include "xenctrl.h"
>> @@ -640,6 +642,8 @@ static int alloc_magic_pages_hvm(struct xc_dom_image *dom)
>>              dom->cmdline_size = ROUNDUP(strlen(dom->cmdline) + 1, 8);
>>              start_info_size += dom->cmdline_size;
>>          }
>> +
>> +        start_info_size += dom->e820_entries * sizeof(*(dom->e820));
> This is not correct because sizeof(struct e820entry) != sizeof(struct
> hvm_modlist_entry) AFAICT. This should instead be sizeof(struct
> hvm_modlist_entry).


The area for modlist is calculated above:

start_info_size +=
        sizeof(struct hvm_modlist_entry) * HVMLOADER_MODULE_MAX_COUNT;

(What I should do though is move this from under 'if (
!dom->device_model )', now that we are providing this data to both HVM
and PVH guests.

>
>>      }
>>      else
>>      {
>> @@ -1666,8 +1670,9 @@ 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;
>> +    size_t start_info_size, modsize;
>>      struct hvm_modlist_entry *modlist;
>> +    struct hvm_memmap_table_entry *memmap;
>>      unsigned int i;
>>  
>>      start_info_size = sizeof(*start_info) + dom->cmdline_size;
>> @@ -1731,7 +1736,29 @@ static int bootlate_hvm(struct xc_dom_image *dom)
>>                              ((uintptr_t)modlist - (uintptr_t)start_info);
>>      }
>>  
>> +    /*
>> +     * Check a couple of XEN_HVM_MEMMAP_TYPEs to verify consistency with
>> +     * their corresponding e820 numerical values.
>> +     */
>> +    BUILD_BUG_ON(XEN_HVM_MEMMAP_TYPE_RAM != E820_RAM);
>> +    BUILD_BUG_ON(XEN_HVM_MEMMAP_TYPE_ACPI != E820_ACPI);
>> +
>> +    modsize = HVMLOADER_MODULE_MAX_COUNT *
>> +        (sizeof(*modlist) + HVMLOADER_MODULE_CMDLINE_SIZE);
> Hm, I'm not sure this is fully correct, but I think there are previous
> issues in this area.
>
> The mapped area (start_info) is of size sizeof(*start_info) +
> dom->cmdline_size + sizeof(struct hvm_modlist_entry) *
> dom->num_modules. Yet here you seem to assume num_modules ==
> HVMLOADER_MODULE_MAX_COUNT?

Yes, see my response above. We've already allocated the segment to
accommodate HVMLOADER_MODULE_MAX_COUNT entries. Which may indeed be an
overkill.


>
> Also the initial space calculation doesn't seem to take
> HVMLOADER_MODULE_CMDLINE_SIZE into account at all.


modlist's offset is calculated with commandline's size in mind:

    modlist = (void*)(start_info + 1) + dom->cmdline_size;



>
> And cmdline_paddr seems to be set to point to garbage if cmdline is not
> set.

Isn't the hvm_start_info set to zero when allocated?

-boris

>
> Or am I missing something?
>
> Adding Jonathan Ludlam and Anthony PERARD who are the ones that added
> this code.
>
> Thanks, Roger.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v4 4/4] libxc: Pass e820 map to HVM/PVH guests via hvm_start_info

[Roger Pau Monné]

On Wed, Mar 21, 2018 at 09:37:09AM -0400, Boris Ostrovsky wrote:
> On 03/21/2018 06:07 AM, Roger Pau Monné wrote:
> > On Tue, Mar 20, 2018 at 09:50:52AM -0700, Maran Wilson wrote:
> >> From: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> >>
> >> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> >> Signed-off-by: Maran Wilson <maran.wilson@oracle.com>
> >> ---
> >>  tools/libxc/xc_dom_x86.c | 29 ++++++++++++++++++++++++++++-
> >>  1 file changed, 28 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
> >> index 0b65dab..b2d8403 100644
> >> --- a/tools/libxc/xc_dom_x86.c
> >> +++ b/tools/libxc/xc_dom_x86.c
> >> @@ -35,6 +35,8 @@
> >>  #include <xen/arch-x86/hvm/start_info.h>
> >>  #include <xen/io/protocols.h>
> >>  
> >> +#include <xen-tools/libs.h>
> >> +
> >>  #include "xg_private.h"
> >>  #include "xc_dom.h"
> >>  #include "xenctrl.h"
> >> @@ -640,6 +642,8 @@ static int alloc_magic_pages_hvm(struct xc_dom_image *dom)
> >>              dom->cmdline_size = ROUNDUP(strlen(dom->cmdline) + 1, 8);
> >>              start_info_size += dom->cmdline_size;
> >>          }
> >> +
> >> +        start_info_size += dom->e820_entries * sizeof(*(dom->e820));
> > This is not correct because sizeof(struct e820entry) != sizeof(struct
> > hvm_modlist_entry) AFAICT. This should instead be sizeof(struct
> > hvm_modlist_entry).
> 
> 
> The area for modlist is calculated above:
> 
> start_info_size +=
>         sizeof(struct hvm_modlist_entry) * HVMLOADER_MODULE_MAX_COUNT;
> 
> (What I should do though is move this from under 'if (
> !dom->device_model )', now that we are providing this data to both HVM
> and PVH guests.

Sorry, I meant sizeof(struct hvm_memmap_table_entry) != sizeof(e820entry), so
the above should be:

start_info_size += dom->e820_entries * sizeof(struct hvm_memmap_table_entry);

> >
> >>      }
> >>      else
> >>      {
> >> @@ -1666,8 +1670,9 @@ 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;
> >> +    size_t start_info_size, modsize;
> >>      struct hvm_modlist_entry *modlist;
> >> +    struct hvm_memmap_table_entry *memmap;
> >>      unsigned int i;
> >>  
> >>      start_info_size = sizeof(*start_info) + dom->cmdline_size;
> >> @@ -1731,7 +1736,29 @@ static int bootlate_hvm(struct xc_dom_image *dom)
> >>                              ((uintptr_t)modlist - (uintptr_t)start_info);
> >>      }
> >>  
> >> +    /*
> >> +     * Check a couple of XEN_HVM_MEMMAP_TYPEs to verify consistency with
> >> +     * their corresponding e820 numerical values.
> >> +     */
> >> +    BUILD_BUG_ON(XEN_HVM_MEMMAP_TYPE_RAM != E820_RAM);
> >> +    BUILD_BUG_ON(XEN_HVM_MEMMAP_TYPE_ACPI != E820_ACPI);
> >> +
> >> +    modsize = HVMLOADER_MODULE_MAX_COUNT *
> >> +        (sizeof(*modlist) + HVMLOADER_MODULE_CMDLINE_SIZE);
> > Hm, I'm not sure this is fully correct, but I think there are previous
> > issues in this area.
> >
> > The mapped area (start_info) is of size sizeof(*start_info) +
> > dom->cmdline_size + sizeof(struct hvm_modlist_entry) *
> > dom->num_modules. Yet here you seem to assume num_modules ==
> > HVMLOADER_MODULE_MAX_COUNT?
> 
> Yes, see my response above. We've already allocated the segment to
> accommodate HVMLOADER_MODULE_MAX_COUNT entries. Which may indeed be an
> overkill.

I'm sorry, but I don't think I follow. There's only a single
xc_map_foreign_range call that maps start_info_size space:

start_info_size = sizeof(*start_info) + dom->cmdline_size;
start_info_size += sizeof(struct hvm_modlist_entry) * dom->num_modules;

So for start_info_size bootlate_hvm takes into account the exact
number of modules used.

Yet modsize seems to assume dom->num_modules ==
HVMLOADER_MODULE_MAX_COUNT?

I think those are all previous issues in this code, TBH.

> >
> > Also the initial space calculation doesn't seem to take
> > HVMLOADER_MODULE_CMDLINE_SIZE into account at all.
> 
> 
> modlist's offset is calculated with commandline's size in mind:
> 
>     modlist = (void*)(start_info + 1) + dom->cmdline_size;

Yes, that's fine AFAICT.

> >
> > And cmdline_paddr seems to be set to point to garbage if cmdline is not
> > set.
> 
> Isn't the hvm_start_info set to zero when allocated?

Yes, but the following code in add_module_to_list seems wrong to me:

if ( cmdline )
{
    assert(strnlen(cmdline, HVMLOADER_MODULE_CMDLINE_SIZE)
           < HVMLOADER_MODULE_CMDLINE_SIZE);
    strncpy(modules_cmdline_start + HVMLOADER_MODULE_CMDLINE_SIZE * index,
            cmdline, HVMLOADER_MODULE_CMDLINE_SIZE);
}

modlist[index].cmdline_paddr =
    modules_cmdline_paddr + HVMLOADER_MODULE_CMDLINE_SIZE * index;

AFAICT it will set cmdline_paddr to point to garbage if cmdline is not
set.

Anyway, this is not introduced by your patch. I will send a couple of
patches to try to fix the already existing issues in this area.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v3 4/4] libxc: Pass e820 map to PVH guest via hvm_start_info

[Wei Liu]

On Fri, Mar 16, 2018 at 06:29:24PM +0000, Roger Pau Monné wrote:
> On Thu, Mar 15, 2018 at 02:35:18PM -0700, Maran Wilson wrote:
> > From: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> > 
> > Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> > Signed-off-by: Maran Wilson <maran.wilson@oracle.com>
> > ---
> >  tools/libxc/xc_dom_x86.c | 30 +++++++++++++++++++++++++++++-
> >  1 file changed, 29 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
> > @@ -1718,6 +1724,27 @@ static int bootlate_hvm(struct xc_dom_image *dom)
> >  
> >          /* ACPI module 0 is the RSDP */
> >          start_info->rsdp_paddr = dom->acpi_modules[0].guest_addr_out ? : 0;
> > +
> > +        /*
> > +         * Check a couple of XEN_HVM_MEMMAP_TYPEs to verify consistency with
> > +         * their corresponding e820 numerical values.
> > +         */
> > +        BUILD_BUG_ON(XEN_HVM_MEMMAP_TYPE_RAM != E820_RAM);
> > +        BUILD_BUG_ON(XEN_HVM_MEMMAP_TYPE_ACPI != E820_ACPI);
> 
> I guess you could test all the types, but I'm not sure if it's worth
> it, let's see what others think.

TBH I don't really care about this. It is easy enough to spot something
is wrong in the E820 table.

Wei.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v4 1/4] x86/PVHv2: Add memory map pointer to hvm_start_info struct

[Maran Wilson]

On 3/21/2018 2:40 AM, Juergen Gross wrote:
> On 21/03/18 10:28, Roger Pau Monné wrote:
>> On Tue, Mar 20, 2018 at 09:48:56AM -0700, Maran Wilson wrote:
>>> +/*
>>>    * C representation of the x86/HVM start info layout.
>>>    *
>>>    * The canonical definition of this layout is above, this is just a way to
>>> @@ -86,6 +134,14 @@ struct hvm_start_info {
>>>       uint64_t cmdline_paddr;     /* Physical address of the command line.     */
>>>       uint64_t rsdp_paddr;        /* Physical address of the RSDP ACPI data    */
>>>                                   /* structure.                                */
>>> +    uint64_t memmap_paddr;      /* Physical address of an array of           */
>>> +                                /* hvm_memmap_table_entry. Only present in   */
>>> +                                /* version 1 and newer of the structure      */
>>> +    uint32_t memmap_entries;    /* Number of entries in the memmap table.    */
>>> +                                /* Only present in version 1 and newer of    */
>>> +                                /* the structure. Value will be zero if      */
>>> +                                /* there is no memory map being provided.    */
>>> +    uint32_t reserved;          /* Must be zero for Version 1.               */
>> I would write "Must be zero." only. If at some point we introduce
>> version 2 we would likely have to fixup this comment to mention
>> version 1 and version 2.
> In case you are going this route I'd suggest to drop the version remarks
> for the individual fields and just add a comment like:
>
> /* All following fields only present in version 1 and newer. */
>
> above memmap_paddr.

OK, so combining the above suggestions, I'd have the following. Is the 
formatting and alignment of comments what you had in mind and acceptable 
to all?

struct hvm_start_info {
     uint32_t magic;             /* Contains the magic value 
0x336ec578       */
                                 /* ("xEn3" with the 0x80 bit of the "E" 
set).*/
     uint32_t version;           /* Version of this 
structure.                */
     uint32_t flags;             /* SIF_xxx 
flags.                            */
     uint32_t nr_modules;        /* Number of modules passed to the 
kernel.   */
     uint64_t modlist_paddr;     /* Physical address of an array 
of           */
                                 /* 
hvm_modlist_entry.                        */
     uint64_t cmdline_paddr;     /* Physical address of the command 
line.     */
     uint64_t rsdp_paddr;        /* Physical address of the RSDP ACPI 
data    */
                                 /* 
structure.                                */
     /* All following fields only present in version 1 and newer */
     uint64_t memmap_paddr;      /* Physical address of an array 
of           */
                                 /* 
hvm_memmap_table_entry.                   */
     uint32_t memmap_entries;    /* Number of entries in the memmap 
table.    */
                                 /* Value will be zero if there is no 
memory  */
                                 /* map being 
provided.                       */
     uint32_t reserved;          /* Must be 
zero.                             */
};

Thanks,
-Maran


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v4 1/4] x86/PVHv2: Add memory map pointer to hvm_start_info struct

[Juergen Gross]

On 21/03/18 17:46, Maran Wilson wrote:
> On 3/21/2018 2:40 AM, Juergen Gross wrote:
>> On 21/03/18 10:28, Roger Pau Monné wrote:
>>> On Tue, Mar 20, 2018 at 09:48:56AM -0700, Maran Wilson wrote:
>>>> +/*
>>>>    * C representation of the x86/HVM start info layout.
>>>>    *
>>>>    * The canonical definition of this layout is above, this is just
>>>> a way to
>>>> @@ -86,6 +134,14 @@ struct hvm_start_info {
>>>>       uint64_t cmdline_paddr;     /* Physical address of the command
>>>> line.     */
>>>>       uint64_t rsdp_paddr;        /* Physical address of the RSDP
>>>> ACPI data    */
>>>>                                   /*
>>>> structure.                                */
>>>> +    uint64_t memmap_paddr;      /* Physical address of an array
>>>> of           */
>>>> +                                /* hvm_memmap_table_entry. Only
>>>> present in   */
>>>> +                                /* version 1 and newer of the
>>>> structure      */
>>>> +    uint32_t memmap_entries;    /* Number of entries in the memmap
>>>> table.    */
>>>> +                                /* Only present in version 1 and
>>>> newer of    */
>>>> +                                /* the structure. Value will be
>>>> zero if      */
>>>> +                                /* there is no memory map being
>>>> provided.    */
>>>> +    uint32_t reserved;          /* Must be zero for Version
>>>> 1.               */
>>> I would write "Must be zero." only. If at some point we introduce
>>> version 2 we would likely have to fixup this comment to mention
>>> version 1 and version 2.
>> In case you are going this route I'd suggest to drop the version remarks
>> for the individual fields and just add a comment like:
>>
>> /* All following fields only present in version 1 and newer. */
>>
>> above memmap_paddr.
> 
> OK, so combining the above suggestions, I'd have the following. Is the
> formatting and alignment of comments what you had in mind and acceptable
> to all?

Looks good to me.


Juergen

> 
> struct hvm_start_info {
>     uint32_t magic;             /* Contains the magic value
> 0x336ec578       */
>                                 /* ("xEn3" with the 0x80 bit of the "E"
> set).*/
>     uint32_t version;           /* Version of this
> structure.                */
>     uint32_t flags;             /* SIF_xxx
> flags.                            */
>     uint32_t nr_modules;        /* Number of modules passed to the
> kernel.   */
>     uint64_t modlist_paddr;     /* Physical address of an array
> of           */
>                                 /*
> hvm_modlist_entry.                        */
>     uint64_t cmdline_paddr;     /* Physical address of the command
> line.     */
>     uint64_t rsdp_paddr;        /* Physical address of the RSDP ACPI
> data    */
>                                 /*
> structure.                                */
>     /* All following fields only present in version 1 and newer */
>     uint64_t memmap_paddr;      /* Physical address of an array
> of           */
>                                 /*
> hvm_memmap_table_entry.                   */
>     uint32_t memmap_entries;    /* Number of entries in the memmap
> table.    */
>                                 /* Value will be zero if there is no
> memory  */
>                                 /* map being
> provided.                       */
>     uint32_t reserved;          /* Must be
> zero.                             */
> };
> 
> Thanks,
> -Maran
> 
> 


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v4 1/4] x86/PVHv2: Add memory map pointer to hvm_start_info struct

[Roger Pau Monné]

On Wed, Mar 21, 2018 at 09:46:21AM -0700, Maran Wilson wrote:
> On 3/21/2018 2:40 AM, Juergen Gross wrote:
> > On 21/03/18 10:28, Roger Pau Monné wrote:
> > > On Tue, Mar 20, 2018 at 09:48:56AM -0700, Maran Wilson wrote:
> > > > +/*
> > > >    * C representation of the x86/HVM start info layout.
> > > >    *
> > > >    * The canonical definition of this layout is above, this is just a way to
> > > > @@ -86,6 +134,14 @@ struct hvm_start_info {
> > > >       uint64_t cmdline_paddr;     /* Physical address of the command line.     */
> > > >       uint64_t rsdp_paddr;        /* Physical address of the RSDP ACPI data    */
> > > >                                   /* structure.                                */
> > > > +    uint64_t memmap_paddr;      /* Physical address of an array of           */
> > > > +                                /* hvm_memmap_table_entry. Only present in   */
> > > > +                                /* version 1 and newer of the structure      */
> > > > +    uint32_t memmap_entries;    /* Number of entries in the memmap table.    */
> > > > +                                /* Only present in version 1 and newer of    */
> > > > +                                /* the structure. Value will be zero if      */
> > > > +                                /* there is no memory map being provided.    */
> > > > +    uint32_t reserved;          /* Must be zero for Version 1.               */
> > > I would write "Must be zero." only. If at some point we introduce
> > > version 2 we would likely have to fixup this comment to mention
> > > version 1 and version 2.
> > In case you are going this route I'd suggest to drop the version remarks
> > for the individual fields and just add a comment like:
> > 
> > /* All following fields only present in version 1 and newer. */
> > 
> > above memmap_paddr.
> 
> OK, so combining the above suggestions, I'd have the following. Is the
> formatting and alignment of comments what you had in mind and acceptable to
> all?

It seems like your email client has skewed the formatting (or maybe
mine...)

Anyway, LGTM, I think this is better.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v4 2/4] libxl/x86: Build e820 map earlier for HVM/PVH guests

[Wei Liu]

On Tue, Mar 20, 2018 at 09:50:50AM -0700, Maran Wilson wrote:
>      case LIBXL_DOMAIN_TYPE_PV:
> -        ret = libxl__build_pv(gc, domid, info, state);
> +        ret = libxl__build_pv(gc, domid, d_config, info, state);
>          if (ret)
>              goto out;
>  
> diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
> index 2e29b52..e83aeb9 100644
> --- a/tools/libxl/libxl_dom.c
> +++ b/tools/libxl/libxl_dom.c
> @@ -698,6 +698,7 @@ static int set_vnuma_info(libxl__gc *gc, uint32_t domid,
>  }
>  
>  static int libxl__build_dom(libxl__gc *gc, uint32_t domid,
> +             libxl_domain_config *d_config,
>               libxl_domain_build_info *info, libxl__domain_build_state *state,

AFAICT build_info is a member of domain_config. You can only take the
latter in this function. Further adjustment is required to accommodate
that change.

Wei.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v4 4/4] libxc: Pass e820 map to HVM/PVH guests via hvm_start_info

[Boris Ostrovsky]

On 03/21/2018 10:18 AM, Roger Pau Monné wrote:
> On Wed, Mar 21, 2018 at 09:37:09AM -0400, Boris Ostrovsky wrote:
>> On 03/21/2018 06:07 AM, Roger Pau Monné wrote:
>>> On Tue, Mar 20, 2018 at 09:50:52AM -0700, Maran Wilson wrote:
>>>> From: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>>>>
>>>> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>>>> Signed-off-by: Maran Wilson <maran.wilson@oracle.com>
>>>> ---
>>>>  tools/libxc/xc_dom_x86.c | 29 ++++++++++++++++++++++++++++-
>>>>  1 file changed, 28 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
>>>> index 0b65dab..b2d8403 100644
>>>> --- a/tools/libxc/xc_dom_x86.c
>>>> +++ b/tools/libxc/xc_dom_x86.c
>>>> @@ -35,6 +35,8 @@
>>>>  #include <xen/arch-x86/hvm/start_info.h>
>>>>  #include <xen/io/protocols.h>
>>>>  
>>>> +#include <xen-tools/libs.h>
>>>> +
>>>>  #include "xg_private.h"
>>>>  #include "xc_dom.h"
>>>>  #include "xenctrl.h"
>>>> @@ -640,6 +642,8 @@ static int alloc_magic_pages_hvm(struct xc_dom_image *dom)
>>>>              dom->cmdline_size = ROUNDUP(strlen(dom->cmdline) + 1, 8);
>>>>              start_info_size += dom->cmdline_size;
>>>>          }
>>>> +
>>>> +        start_info_size += dom->e820_entries * sizeof(*(dom->e820));
>>> This is not correct because sizeof(struct e820entry) != sizeof(struct
>>> hvm_modlist_entry) AFAICT. This should instead be sizeof(struct
>>> hvm_modlist_entry).
>>
>> The area for modlist is calculated above:
>>
>> start_info_size +=
>>         sizeof(struct hvm_modlist_entry) * HVMLOADER_MODULE_MAX_COUNT;
>>
>> (What I should do though is move this from under 'if (
>> !dom->device_model )', now that we are providing this data to both HVM
>> and PVH guests.
> Sorry, I meant sizeof(struct hvm_memmap_table_entry) != sizeof(e820entry), so
> the above should be:
>
> start_info_size += dom->e820_entries * sizeof(struct hvm_memmap_table_entry);


Oh, duh!

>
>>>>      }
>>>>      else
>>>>      {
>>>> @@ -1666,8 +1670,9 @@ 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;
>>>> +    size_t start_info_size, modsize;
>>>>      struct hvm_modlist_entry *modlist;
>>>> +    struct hvm_memmap_table_entry *memmap;
>>>>      unsigned int i;
>>>>  
>>>>      start_info_size = sizeof(*start_info) + dom->cmdline_size;
>>>> @@ -1731,7 +1736,29 @@ static int bootlate_hvm(struct xc_dom_image *dom)
>>>>                              ((uintptr_t)modlist - (uintptr_t)start_info);
>>>>      }
>>>>  
>>>> +    /*
>>>> +     * Check a couple of XEN_HVM_MEMMAP_TYPEs to verify consistency with
>>>> +     * their corresponding e820 numerical values.
>>>> +     */
>>>> +    BUILD_BUG_ON(XEN_HVM_MEMMAP_TYPE_RAM != E820_RAM);
>>>> +    BUILD_BUG_ON(XEN_HVM_MEMMAP_TYPE_ACPI != E820_ACPI);
>>>> +
>>>> +    modsize = HVMLOADER_MODULE_MAX_COUNT *
>>>> +        (sizeof(*modlist) + HVMLOADER_MODULE_CMDLINE_SIZE);
>>> Hm, I'm not sure this is fully correct, but I think there are previous
>>> issues in this area.
>>>
>>> The mapped area (start_info) is of size sizeof(*start_info) +
>>> dom->cmdline_size + sizeof(struct hvm_modlist_entry) *
>>> dom->num_modules. Yet here you seem to assume num_modules ==
>>> HVMLOADER_MODULE_MAX_COUNT?
>> Yes, see my response above. We've already allocated the segment to
>> accommodate HVMLOADER_MODULE_MAX_COUNT entries. Which may indeed be an
>> overkill.
> I'm sorry, but I don't think I follow. There's only a single
> xc_map_foreign_range call that maps start_info_size space:
>
> start_info_size = sizeof(*start_info) + dom->cmdline_size;
> start_info_size += sizeof(struct hvm_modlist_entry) * dom->num_modules;
>
> So for start_info_size bootlate_hvm takes into account the exact
> number of modules used.
>
> Yet modsize seems to assume dom->num_modules ==
> HVMLOADER_MODULE_MAX_COUNT?


If you look at add_module_to_list() above you'll notice that it stores
modules' commandlines after HVMLOADER_MODULE_MAX_COUNT modules:

    void *modules_cmdline_start = modlist + HVMLOADER_MODULE_MAX_COUNT;


One thing I could do is

    modsize = HVMLOADER_MODULE_MAX_COUNT *(sizeof(*modlist)) + 
dom->num_modules * HVMLOADER_MODULE_CMDLINE_SIZE;

but I think the resulting difference between expected/reserved number of
modules vs number of commandlines makes this not worthwhile.

(As a side note, dom->num_modules is meaningless for HVM guests here ---
we only add one module, the FW blob.)


>
> I think those are all previous issues in this code, TBH.
>
>>> Also the initial space calculation doesn't seem to take
>>> HVMLOADER_MODULE_CMDLINE_SIZE into account at all.
>>
>> modlist's offset is calculated with commandline's size in mind:
>>
>>     modlist = (void*)(start_info + 1) + dom->cmdline_size;
> Yes, that's fine AFAICT.
>
>>> And cmdline_paddr seems to be set to point to garbage if cmdline is not
>>> set.
>> Isn't the hvm_start_info set to zero when allocated?
> Yes, but the following code in add_module_to_list seems wrong to me:
>
> if ( cmdline )
> {
>     assert(strnlen(cmdline, HVMLOADER_MODULE_CMDLINE_SIZE)
>            < HVMLOADER_MODULE_CMDLINE_SIZE);
>     strncpy(modules_cmdline_start + HVMLOADER_MODULE_CMDLINE_SIZE * index,
>             cmdline, HVMLOADER_MODULE_CMDLINE_SIZE);
> }
>
> modlist[index].cmdline_paddr =
>     modules_cmdline_paddr + HVMLOADER_MODULE_CMDLINE_SIZE * index;
>
> AFAICT it will set cmdline_paddr to point to garbage if cmdline is not
> set.


Oh, I thought you were referring to start_info->cmdline_paddr. Yes, you
are right for the modules' pointer.

-boris

>
> Anyway, this is not introduced by your patch. I will send a couple of
> patches to try to fix the already existing issues in this area.
>
> Thanks, Roger.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v4 2/4] libxl/x86: Build e820 map earlier for HVM/PVH guests

[Boris Ostrovsky]

On 03/21/2018 01:49 PM, Wei Liu wrote:
> On Tue, Mar 20, 2018 at 09:50:50AM -0700, Maran Wilson wrote:
>>      case LIBXL_DOMAIN_TYPE_PV:
>> -        ret = libxl__build_pv(gc, domid, info, state);
>> +        ret = libxl__build_pv(gc, domid, d_config, info, state);
>>          if (ret)
>>              goto out;
>>  
>> diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
>> index 2e29b52..e83aeb9 100644
>> --- a/tools/libxl/libxl_dom.c
>> +++ b/tools/libxl/libxl_dom.c
>> @@ -698,6 +698,7 @@ static int set_vnuma_info(libxl__gc *gc, uint32_t domid,
>>  }
>>  
>>  static int libxl__build_dom(libxl__gc *gc, uint32_t domid,
>> +             libxl_domain_config *d_config,
>>               libxl_domain_build_info *info, libxl__domain_build_state *state,
> AFAICT build_info is a member of domain_config. You can only take the
> latter in this function. Further adjustment is required to accommodate
> that change.


Yes, Roger also pointed this out to me.

-boris

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v4 4/4] libxc: Pass e820 map to HVM/PVH guests via hvm_start_info

[Roger Pau Monné]

On Wed, Mar 21, 2018 at 01:53:37PM -0400, Boris Ostrovsky wrote:
> On 03/21/2018 10:18 AM, Roger Pau Monné wrote:
> > On Wed, Mar 21, 2018 at 09:37:09AM -0400, Boris Ostrovsky wrote:
> >> On 03/21/2018 06:07 AM, Roger Pau Monné wrote:
> >>> On Tue, Mar 20, 2018 at 09:50:52AM -0700, Maran Wilson wrote:
> >>>> From: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> >>>>      }
> >>>>      else
> >>>>      {
> >>>> @@ -1666,8 +1670,9 @@ 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;
> >>>> +    size_t start_info_size, modsize;
> >>>>      struct hvm_modlist_entry *modlist;
> >>>> +    struct hvm_memmap_table_entry *memmap;
> >>>>      unsigned int i;
> >>>>  
> >>>>      start_info_size = sizeof(*start_info) + dom->cmdline_size;
> >>>> @@ -1731,7 +1736,29 @@ static int bootlate_hvm(struct xc_dom_image *dom)
> >>>>                              ((uintptr_t)modlist - (uintptr_t)start_info);
> >>>>      }
> >>>>  
> >>>> +    /*
> >>>> +     * Check a couple of XEN_HVM_MEMMAP_TYPEs to verify consistency with
> >>>> +     * their corresponding e820 numerical values.
> >>>> +     */
> >>>> +    BUILD_BUG_ON(XEN_HVM_MEMMAP_TYPE_RAM != E820_RAM);
> >>>> +    BUILD_BUG_ON(XEN_HVM_MEMMAP_TYPE_ACPI != E820_ACPI);
> >>>> +
> >>>> +    modsize = HVMLOADER_MODULE_MAX_COUNT *
> >>>> +        (sizeof(*modlist) + HVMLOADER_MODULE_CMDLINE_SIZE);
> >>> Hm, I'm not sure this is fully correct, but I think there are previous
> >>> issues in this area.
> >>>
> >>> The mapped area (start_info) is of size sizeof(*start_info) +
> >>> dom->cmdline_size + sizeof(struct hvm_modlist_entry) *
> >>> dom->num_modules. Yet here you seem to assume num_modules ==
> >>> HVMLOADER_MODULE_MAX_COUNT?
> >> Yes, see my response above. We've already allocated the segment to
> >> accommodate HVMLOADER_MODULE_MAX_COUNT entries. Which may indeed be an
> >> overkill.
> > I'm sorry, but I don't think I follow. There's only a single
> > xc_map_foreign_range call that maps start_info_size space:
> >
> > start_info_size = sizeof(*start_info) + dom->cmdline_size;
> > start_info_size += sizeof(struct hvm_modlist_entry) * dom->num_modules;
> >
> > So for start_info_size bootlate_hvm takes into account the exact
> > number of modules used.
> >
> > Yet modsize seems to assume dom->num_modules ==
> > HVMLOADER_MODULE_MAX_COUNT?
> 
> 
> If you look at add_module_to_list() above you'll notice that it stores
> modules' commandlines after HVMLOADER_MODULE_MAX_COUNT modules:
> 
>     void *modules_cmdline_start = modlist + HVMLOADER_MODULE_MAX_COUNT;
> 
> 
> One thing I could do is
> 
>     modsize = HVMLOADER_MODULE_MAX_COUNT *(sizeof(*modlist)) + 
> dom->num_modules * HVMLOADER_MODULE_CMDLINE_SIZE;
> 
> but I think the resulting difference between expected/reserved number of
> modules vs number of commandlines makes this not worthwhile.
> 
> (As a side note, dom->num_modules is meaningless for HVM guests here ---
> we only add one module, the FW blob.)

Right. I think that after the fixes I've sent for the start_info_size
your calculation is correct:

https://lists.xenproject.org/archives/html/xen-devel/2018-03/msg02493.html

Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v4 4/4] libxc: Pass e820 map to HVM/PVH guests via hvm_start_info

[Wei Liu]

On Wed, Mar 21, 2018 at 01:53:37PM -0400, Boris Ostrovsky wrote:
> (As a side note, dom->num_modules is meaningless for HVM guests here ---
> we only add one module, the FW blob.)
> 

FWIW It will soon be relevant as we split ipxe from rombios.

Wei.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel