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

[PATCH v2 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

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.

 tools/libxc/include/xc_dom.h                 |  8 ++-
 tools/libxc/xc_dom_x86.c                     | 21 +++++-
 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 | 63 +++++++++++++++++-
 7 files changed, 109 insertions(+), 7 deletions(-)

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


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

[PATCH v2 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 | 63 +++++++++++++++++++++++++++-
 1 file changed, 62 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..f8d6a1a 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,10 +72,46 @@
  *    | 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.
+ * 20 +----------------|
+ *    | reserved       |
+ * 24 +----------------+
+ *
  * The address and sizes are always a 64bit little endian unsigned integer.
  *
+ * For x86 implementations at least, the values used in the type field will
+ * match the Address Range Types as defined in section 15 (System Address
+ * Map Interfaces) of the ACPI Specification (http://uefi.org/specifications)
+ * where:
+ *     AddressRangeMemory = 1 (E820_RAM)
+ *     AddressRangeReserved = 2 (E820_RESERVED)
+ *     AddressRangeACPI = 3 (E820_ACPI)
+ *     AddressRangeNVS = 4 (E820_NVS)
+ *     AddressRangeUnusable = 5 (E820_UNUSABLE)
+ *     AddressRangeDisabled = 6 (E820_DISABLED)
+ *     AddressRangePersistentMemory = 7 (E820_PMEM)
+ *
  * 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
 
@@ -86,6 +132,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 +149,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 v2 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 v2 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 v2 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>
---
 tools/libxc/xc_dom_x86.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
index 0b65dab..4f4b26e 100644
--- a/tools/libxc/xc_dom_x86.c
+++ b/tools/libxc/xc_dom_x86.c
@@ -640,6 +640,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 +1668,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 +1695,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 +1722,20 @@ 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;
+
+        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 +1750,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 v2 1/4] x86/PVHv2: Add memory map pointer to hvm_start_info struct

[Jan Beulich]

>>> On 14.03.18 at 00:31, <maran.wilson@oracle.com> wrote:
> + * For x86 implementations at least, the values used in the type field will
> + * match the Address Range Types as defined in section 15 (System Address
> + * Map Interfaces) of the ACPI Specification (http://uefi.org/specifications)
> + * where:
> + *     AddressRangeMemory = 1 (E820_RAM)
> + *     AddressRangeReserved = 2 (E820_RESERVED)
> + *     AddressRangeACPI = 3 (E820_ACPI)
> + *     AddressRangeNVS = 4 (E820_NVS)
> + *     AddressRangeUnusable = 5 (E820_UNUSABLE)
> + *     AddressRangeDisabled = 6 (E820_DISABLED)
> + *     AddressRangePersistentMemory = 7 (E820_PMEM)

Would you mind waiting for a discussion to settle before sending
out new patch versions? As indicated in an earlier reply to v1, I
consider this still insufficient. And no, I'm not asking for you to
add redundant and potentially conflicting definitions of E820_*,
but instead you want to use Xen specific ones (prefixed e.g.
by XEN_HVM_MEMMAP_TYPE_).

Jan


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

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

[Boris Ostrovsky]

On 03/14/2018 03:55 AM, Jan Beulich wrote:
>>>> On 14.03.18 at 00:31, <maran.wilson@oracle.com> wrote:
>> + * For x86 implementations at least, the values used in the type field will
>> + * match the Address Range Types as defined in section 15 (System Address
>> + * Map Interfaces) of the ACPI Specification (http://uefi.org/specifications)
>> + * where:
>> + *     AddressRangeMemory = 1 (E820_RAM)
>> + *     AddressRangeReserved = 2 (E820_RESERVED)
>> + *     AddressRangeACPI = 3 (E820_ACPI)
>> + *     AddressRangeNVS = 4 (E820_NVS)
>> + *     AddressRangeUnusable = 5 (E820_UNUSABLE)
>> + *     AddressRangeDisabled = 6 (E820_DISABLED)
>> + *     AddressRangePersistentMemory = 7 (E820_PMEM)
> Would you mind waiting for a discussion to settle before sending
> out new patch versions? As indicated in an earlier reply to v1, I
> consider this still insufficient. And no, I'm not asking for you to
> add redundant and potentially conflicting definitions of E820_*,
> but instead you want to use Xen specific ones (prefixed e.g.
> by XEN_HVM_MEMMAP_TYPE_).

Since we will now have a non-Xen user of this interface perhaps
PVH_MEMMAP_TYPE_ ?

-boris

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

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

[Maran Wilson]

On 3/14/2018 10:28 AM, Boris Ostrovsky wrote:
> On 03/14/2018 03:55 AM, Jan Beulich wrote:
>>>>> On 14.03.18 at 00:31, <maran.wilson@oracle.com> wrote:
>>> + * For x86 implementations at least, the values used in the type field will
>>> + * match the Address Range Types as defined in section 15 (System Address
>>> + * Map Interfaces) of the ACPI Specification (http://uefi.org/specifications)
>>> + * where:
>>> + *     AddressRangeMemory = 1 (E820_RAM)
>>> + *     AddressRangeReserved = 2 (E820_RESERVED)
>>> + *     AddressRangeACPI = 3 (E820_ACPI)
>>> + *     AddressRangeNVS = 4 (E820_NVS)
>>> + *     AddressRangeUnusable = 5 (E820_UNUSABLE)
>>> + *     AddressRangeDisabled = 6 (E820_DISABLED)
>>> + *     AddressRangePersistentMemory = 7 (E820_PMEM)
>> Would you mind waiting for a discussion to settle before sending
>> out new patch versions? As indicated in an earlier reply to v1, I
>> consider this still insufficient. And no, I'm not asking for you to
>> add redundant and potentially conflicting definitions of E820_*,
>> but instead you want to use Xen specific ones (prefixed e.g.
>> by XEN_HVM_MEMMAP_TYPE_).
> Since we will now have a non-Xen user of this interface perhaps
> PVH_MEMMAP_TYPE_ ?

OK, I think I'm following the specifics now. But just to make sure we 
all on the same page before sending out the next version...

I'll be adding something like the following to the header file:

...
/*
  * For x86 implementations at least, 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 section 15 (System Address Map Interfaces) of the ACPI
  * Specification (http://uefi.org/specifications)
  */
#define PVH_MEMMAP_TYPE_RAM       1
#define PVH_MEMMAP_TYPE_RESERVED  2
#define PVH_MEMMAP_TYPE_ACPI      3
#define PVH_MEMMAP_TYPE_NVS       4
#define PVH_MEMMAP_TYPE_UNUSABLE  5
#define PVH_MEMMAP_TYPE_PMEM      7
...

And then we will find an appropriate place in the c code to add a couple 
of BUILD_BUG_ON() macros to make sure the above remain consistent with 
E820_xxx.

Does that sound about right?

Thanks,
-Maran

>
> -boris


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

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

[Jan Beulich]

>>> On 14.03.18 at 18:28, <boris.ostrovsky@oracle.com> wrote:
> On 03/14/2018 03:55 AM, Jan Beulich wrote:
>>>>> On 14.03.18 at 00:31, <maran.wilson@oracle.com> wrote:
>>> + * For x86 implementations at least, the values used in the type field will
>>> + * match the Address Range Types as defined in section 15 (System Address
>>> + * Map Interfaces) of the ACPI Specification (http://uefi.org/specifications)
>>> + * where:
>>> + *     AddressRangeMemory = 1 (E820_RAM)
>>> + *     AddressRangeReserved = 2 (E820_RESERVED)
>>> + *     AddressRangeACPI = 3 (E820_ACPI)
>>> + *     AddressRangeNVS = 4 (E820_NVS)
>>> + *     AddressRangeUnusable = 5 (E820_UNUSABLE)
>>> + *     AddressRangeDisabled = 6 (E820_DISABLED)
>>> + *     AddressRangePersistentMemory = 7 (E820_PMEM)
>> Would you mind waiting for a discussion to settle before sending
>> out new patch versions? As indicated in an earlier reply to v1, I
>> consider this still insufficient. And no, I'm not asking for you to
>> add redundant and potentially conflicting definitions of E820_*,
>> but instead you want to use Xen specific ones (prefixed e.g.
>> by XEN_HVM_MEMMAP_TYPE_).
> 
> Since we will now have a non-Xen user of this interface perhaps
> PVH_MEMMAP_TYPE_ ?

Not really, no. For one I don't think PVH is meaningful to KVM. And
then, this is the canonical Xen header. It's up to the Linux side
customization to use different name prefixes. Just look at the
pvclock interface definitions and how they differ between the
canonical Xen headers and their Linux derivatives.

Jan


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

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

[Jan Beulich]

>>> On 14.03.18 at 21:07, <maran.wilson@oracle.com> wrote:
> OK, I think I'm following the specifics now. But just to make sure we 
> all on the same page before sending out the next version...
> 
> I'll be adding something like the following to the header file:
> 
> ...
> /*
>   * For x86 implementations at least, 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 section 15 (System Address Map Interfaces) of the ACPI
>   * Specification (http://uefi.org/specifications)
>   */
> #define PVH_MEMMAP_TYPE_RAM       1
> #define PVH_MEMMAP_TYPE_RESERVED  2
> #define PVH_MEMMAP_TYPE_ACPI      3
> #define PVH_MEMMAP_TYPE_NVS       4
> #define PVH_MEMMAP_TYPE_UNUSABLE  5
> #define PVH_MEMMAP_TYPE_PMEM      7
> ...
> 
> And then we will find an appropriate place in the c code to add a couple 
> of BUILD_BUG_ON() macros to make sure the above remain consistent with 
> E820_xxx.
> 
> Does that sound about right?

About, yes. Subject to the name prefix adjustment as per the other
reply (to Boris) just sent. Plus I'm not convinced of the x86
restriction here: There's nothing x86-specific about these in the
ACPI spec.

Additionally, please either avoid referring to doc sections by number,
or also give the ACPI version so that the number changing in a future
revision won't cause the comment to become stale.

Finally please include all values defined by ACPI 6.2.

Jan


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

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

[Roger Pau Monné]

On Thu, Mar 15, 2018 at 01:22:24AM -0600, Jan Beulich wrote:
> >>> On 14.03.18 at 21:07, <maran.wilson@oracle.com> wrote:
> > OK, I think I'm following the specifics now. But just to make sure we 
> > all on the same page before sending out the next version...
> > 
> > I'll be adding something like the following to the header file:
> > 
> > ...
> > /*
> >   * For x86 implementations at least, 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 section 15 (System Address Map Interfaces) of the ACPI
> >   * Specification (http://uefi.org/specifications)
> >   */
> > #define PVH_MEMMAP_TYPE_RAM       1
> > #define PVH_MEMMAP_TYPE_RESERVED  2
> > #define PVH_MEMMAP_TYPE_ACPI      3
> > #define PVH_MEMMAP_TYPE_NVS       4
> > #define PVH_MEMMAP_TYPE_UNUSABLE  5
> > #define PVH_MEMMAP_TYPE_PMEM      7
> > ...
> > 
> > And then we will find an appropriate place in the c code to add a couple 
> > of BUILD_BUG_ON() macros to make sure the above remain consistent with 
> > E820_xxx.
> > 
> > Does that sound about right?
> 
> About, yes. Subject to the name prefix adjustment as per the other
> reply (to Boris) just sent. Plus I'm not convinced of the x86
> restriction here: There's nothing x86-specific about these in the
> ACPI spec.

This header and interface is already x86 specific ATM, so I would
simply avoid the starting "For x86...". Iff this is ever used by
other arches we can then decide what to do with the memory types.

The rest LGTM (modulo Jan's requested changes).

Thanks, Roger.

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