[RFC][v3][PATCH 0/6] xen: reserve RMRR to avoid conflicting MMIO/RAM

[RFC][v3][PATCH 0/6] xen: reserve RMRR to avoid conflicting MMIO/RAM

[Tiejun Chen]

v3:

* Use XENMEM_reserved_device_memory_map to replace XENMEM_RMRR_memory_map
* Then rebase all patches

v2:

* Don't use e820map to define RMRR maps directly to avoid any confusion.
* In patch #3 we introduce construct_rmrr_e820_maps() to check if we can
  insert RMRR maps and then we will sort all e820 entries.
* Clean patch #4
* In patch #5 we reuse check_mmio_hole() to check if current mmio range is
  fine to RMRR maps. If not, we just issue error to notify the user since
  mostly mmio should be configured again.

While we work for supporting RMRR mapping for Windows GFX driver in case
shared table,

http://osdir.com/ml/general/2014-07/msg55347.html
http://osdir.com/ml/general/2014-07/msg55348.html

we realize we should reserve RMRR range to avoid any potential MMIO/RAM
overlap with our discussion so here these preliminary patches are intended
to cover this.

----------------------------------------------------------------
Tiejun Chen (6):
      xen:x86: record RMRR mappings
      xen:x86: introduce a new hypercall to get RMRR mappings
      tools:firmware:hvmloader: reserve RMRR mappings in e820
      xen:x86: add XENMEM_reserved_device_memory_map to expose RMRR
      tools:libxc: check if mmio BAR is out of RMRR mappings
      xen:vtd: make USB RMRR mapping safe

 tools/firmware/hvmloader/e820.c     | 94 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 tools/firmware/hvmloader/e820.h     |  6 ++++++
 tools/firmware/hvmloader/util.c     | 16 ++++++++++++++++
 tools/firmware/hvmloader/util.h     |  2 ++
 tools/libxc/xc_domain.c             | 26 ++++++++++++++++++++++++++
 tools/libxc/xc_hvm_build_x86.c      | 23 +++++++++++++++++++++++
 tools/libxc/xenctrl.h               |  4 ++++
 xen/arch/x86/e820.c                 |  2 ++
 xen/arch/x86/mm.c                   | 32 ++++++++++++++++++++++++++++++++
 xen/arch/x86/x86_64/compat/mm.c     |  9 +++++++++
 xen/drivers/passthrough/vtd/dmar.c  | 14 ++++++++++++++
 xen/drivers/passthrough/vtd/iommu.c |  8 --------
 xen/include/asm-x86/e820.h          |  3 +++
 xen/include/public/memory.h         | 14 +++++++++++++-
 14 files changed, 244 insertions(+), 9 deletions(-)

Thanks
Tiejun

[RFC][v3][PATCH 1/6] xen:x86: record RMRR mappings

[Tiejun Chen]

We will expose RMRR mappings to VM so need to record all
RMRR mappings firstly.

Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
---
 xen/arch/x86/e820.c                |  2 ++
 xen/drivers/passthrough/vtd/dmar.c | 14 ++++++++++++++
 xen/include/asm-x86/e820.h         |  3 +++
 3 files changed, 19 insertions(+)

diff --git a/xen/arch/x86/e820.c b/xen/arch/x86/e820.c
index 55fe0d6..db44afd 100644
--- a/xen/arch/x86/e820.c
+++ b/xen/arch/x86/e820.c
@@ -34,6 +34,8 @@ static bool_t __initdata e820_verbose;
 boolean_param("e820-verbose", e820_verbose);
 
 struct e820map e820;
+/* Used to record RMRR mapping. */
+rmrr_maps_t rmrr_maps;
 
 /*
  * This function checks if the entire range <start,end> is mapped with type.
diff --git a/xen/drivers/passthrough/vtd/dmar.c b/xen/drivers/passthrough/vtd/dmar.c
index 1152c3a..24321e3 100644
--- a/xen/drivers/passthrough/vtd/dmar.c
+++ b/xen/drivers/passthrough/vtd/dmar.c
@@ -29,6 +29,7 @@
 #include <xen/pci.h>
 #include <xen/pci_regs.h>
 #include <asm/string.h>
+#include <asm/e820.h>
 #include "dmar.h"
 #include "iommu.h"
 #include "extern.h"
@@ -80,6 +81,18 @@ static int __init acpi_register_rmrr_unit(struct acpi_rmrr_unit *rmrr)
     return 0;
 }
 
+/* Record RMRR mapping to ready expose VM. */
+static int __init rmrr_maps_register(struct acpi_rmrr_unit *rmrr)
+{
+    rmrr_maps.map[rmrr_maps.nr_map].addr = rmrr->base_address;
+    rmrr_maps.map[rmrr_maps.nr_map].size = rmrr->end_address -
+                                           rmrr->base_address;
+    rmrr_maps.map[rmrr_maps.nr_map].type = E820_RESERVED;
+    rmrr_maps.nr_map++;
+
+    return 0;
+}
+
 static void __init disable_all_dmar_units(void)
 {
     struct acpi_drhd_unit *drhd, *_drhd;
@@ -675,6 +688,7 @@ acpi_parse_one_rmrr(struct acpi_dmar_header *header)
                         " end_address %"PRIx64"\n",
                         rmrru->base_address, rmrru->end_address);
             acpi_register_rmrr_unit(rmrru);
+            rmrr_maps_register(rmrru);
         }
     }
 
diff --git a/xen/include/asm-x86/e820.h b/xen/include/asm-x86/e820.h
index 71a804c..74262c9 100644
--- a/xen/include/asm-x86/e820.h
+++ b/xen/include/asm-x86/e820.h
@@ -23,6 +23,8 @@ struct e820map {
     struct e820entry map[E820MAX];
 };
 
+typedef struct e820map rmrr_maps_t;
+
 extern int e820_all_mapped(u64 start, u64 end, unsigned type);
 extern int reserve_e820_ram(struct e820map *e820, uint64_t s, uint64_t e);
 extern int e820_change_range_type(
@@ -32,6 +34,7 @@ extern int e820_add_range(
     struct e820map *, uint64_t s, uint64_t e, uint32_t type);
 extern unsigned long init_e820(const char *, struct e820entry *, int *);
 extern struct e820map e820;
+extern rmrr_maps_t rmrr_maps;
 
 /* These symbols live in the boot trampoline. */
 extern struct e820entry e820map[];
-- 
1.9.1

[RFC][v3][PATCH 2/6] xen:x86: introduce a new hypercall to get RMRR mappings

[Tiejun Chen]

We need this new hypercall to get RMRR mapping for VM.

Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
---
 xen/arch/x86/x86_64/compat/mm.c |  9 +++++++++
 xen/include/public/memory.h     | 14 +++++++++++++-
 2 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/x86_64/compat/mm.c b/xen/arch/x86/x86_64/compat/mm.c
index 69c6195..ff16f17 100644
--- a/xen/arch/x86/x86_64/compat/mm.c
+++ b/xen/arch/x86/x86_64/compat/mm.c
@@ -132,6 +132,15 @@ int compat_arch_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
         break;
     }
 
+    case XENMEM_reserved_device_memory_map:
+    {
+        /* Currently we just need to cover RMRR. */
+        if ( copy_to_guest(arg, &rmrr_maps, 1) )
+            return -EFAULT;
+
+        return 0;
+    }
+
     case XENMEM_machphys_mapping:
     {
         struct domain *d = current->domain;
diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h
index 2c57aa0..13e539f 100644
--- a/xen/include/public/memory.h
+++ b/xen/include/public/memory.h
@@ -523,7 +523,19 @@ DEFINE_XEN_GUEST_HANDLE(xen_mem_sharing_op_t);
 
 #endif /* defined(__XEN__) || defined(__XEN_TOOLS__) */
 
-/* Next available subop number is 26 */
+/*
+ * Some devices may reserve some range.
+ *
+ * Currently we just have RMRR
+ * - Reserved memory Region Reporting Structure,
+ * So returns the RMRR memory map as it was when the domain
+ * was started.
+ */
+#define XENMEM_reserved_device_memory_map   26
+typedef struct xen_memory_map xen_rmrr_memory_map_t;
+DEFINE_XEN_GUEST_HANDLE(xen_rmrr_memory_map_t);
+
+/* Next available subop number is 27 */
 
 #endif /* __XEN_PUBLIC_MEMORY_H__ */
 
-- 
1.9.1

[RFC][v3][PATCH 3/6] tools:firmware:hvmloader: reserve RMRR mappings in e820

[Tiejun Chen]

We need to reserve all RMRR mappings in e820 to avoid any
potential guest memory conflict.

Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
---
 tools/firmware/hvmloader/e820.c | 94 +++++++++++++++++++++++++++++++++++++++++
 tools/firmware/hvmloader/e820.h |  6 +++
 tools/firmware/hvmloader/util.c | 16 +++++++
 tools/firmware/hvmloader/util.h |  2 +
 4 files changed, 118 insertions(+)

diff --git a/tools/firmware/hvmloader/e820.c b/tools/firmware/hvmloader/e820.c
index 2e05e93..7f54eab 100644
--- a/tools/firmware/hvmloader/e820.c
+++ b/tools/firmware/hvmloader/e820.c
@@ -68,12 +68,89 @@ void dump_e820_table(struct e820entry *e820, unsigned int nr)
     }
 }
 
+static unsigned int construct_rmrr_e820_maps(unsigned int nr,
+                                     struct rmrr_map *e820_rmrr_map,
+                                     struct e820entry *e820)
+{
+    unsigned int i = 0, j = 0, m = 0, sum_nr = 0;
+    uint64_t start, end, rmrr_start, rmrr_end;
+    unsigned int insert = 0, do_insert = 0;
+
+do_real_construct:
+    sum_nr = nr + e820_rmrr_map->nr_map;
+    for ( i = 0; i < e820_rmrr_map->nr_map; i++ )
+    {
+        rmrr_start = e820_rmrr_map->map[i].addr;
+        rmrr_end = e820_rmrr_map->map[i].addr +
+                   e820_rmrr_map->map[i].size + 1;
+
+        for ( j = 0; j < nr; j++ )
+        {
+            end = e820[j].addr + e820[j].size;
+            start = e820[j+1].addr;
+
+            /* Between those existing e820 entries. */
+            if ( (rmrr_start > end) && (rmrr_end < start) )
+            {
+                if (do_insert)
+                {
+                    /* Move to free this entry. */
+                    for ( m = sum_nr - 1; m > j; m-- )
+                    {
+                        e820[m].addr = e820[m-1].addr;
+                        e820[m].size = e820[m-1].size;
+                        e820[m].type = e820[m-1].type;
+                    }
+
+                    /* Then fill RMRR into that entry. */
+                    e820[j+1].addr = rmrr_start;
+                    e820[j+1].size = rmrr_end - rmrr_start;
+                    e820[j+1].type = E820_RESERVED;
+                    nr++;
+                }
+                insert++;
+            }
+            /* Already at the end. */
+            else if ( (rmrr_start > end) && !start )
+            {
+                if (do_insert)
+                {
+                    e820[nr].addr = rmrr_start;
+                    e820[nr].size = rmrr_end - rmrr_start;
+                    e820[nr].type = E820_RESERVED;
+                    nr++;
+                }
+                insert++;
+            }
+        }
+    }
+
+    /* Just return if done. */
+    if (do_insert)
+        return nr;
+
+    /* Fine to construct RMRR mappings into e820. */
+    if ( insert == e820_rmrr_map->nr_map)
+    {
+        do_insert = 1;
+        goto do_real_construct;
+    }
+    /* Overlap. */
+    else
+    {
+        printf("RMRR overlap with thoese existing e820 entries!\n");
+        printf("So we don't construct RMRR mapping in e820!\n");
+    }
+
+    return nr;
+}
 /* Create an E820 table based on memory parameters provided in hvm_info. */
 int build_e820_table(struct e820entry *e820,
                      unsigned int lowmem_reserved_base,
                      unsigned int bios_image_base)
 {
     unsigned int nr = 0;
+    struct rmrr_map *rmrr_maps;
 
     if ( !lowmem_reserved_base )
             lowmem_reserved_base = 0xA0000;
@@ -169,6 +246,23 @@ int build_e820_table(struct e820entry *e820,
         nr++;
     }
 
+    /* We'd better reserve RMRR mapping for each VM to avoid potential
+     * memory conflict.
+     */
+    rmrr_maps = get_rmrr_map_info();
+    if ( rmrr_maps->nr_map )
+    {
+        if ( (nr + rmrr_maps->nr_map) > E820MAX )
+        {
+            printf(" No free space to insert all RMRR mapping entry!!\n");
+            return nr;
+        }
+        else
+        {
+            nr = construct_rmrr_e820_maps(nr, rmrr_maps, e820);
+        }
+    }
+
     return nr;
 }
 
diff --git a/tools/firmware/hvmloader/e820.h b/tools/firmware/hvmloader/e820.h
index b2ead7f..a61b80b 100644
--- a/tools/firmware/hvmloader/e820.h
+++ b/tools/firmware/hvmloader/e820.h
@@ -15,6 +15,12 @@ struct e820entry {
     uint32_t type;
 } __attribute__((packed));
 
+#define E820MAX 128
+
+struct rmrr_map {
+    unsigned int nr_map;
+    struct e820entry map[E820MAX];
+};
 #endif /* __HVMLOADER_E820_H__ */
 
 /*
diff --git a/tools/firmware/hvmloader/util.c b/tools/firmware/hvmloader/util.c
index 80d822f..f63434a 100644
--- a/tools/firmware/hvmloader/util.c
+++ b/tools/firmware/hvmloader/util.c
@@ -766,6 +766,22 @@ struct shared_info *get_shared_info(void)
     return shared_info;
 }
 
+struct rmrr_map *get_rmrr_map_info(void)
+{
+    static int no_rmrr = 1;
+
+    if ( no_rmrr == 0 )
+        return &rmrr_e820map;
+
+    if ( hypercall_memory_op(XENMEM_reserved_device_memory_map,
+                             &rmrr_e820map) != 0 )
+        BUG();
+
+    no_rmrr = 0;
+
+    return &rmrr_e820map;
+}
+
 uint16_t get_cpu_mhz(void)
 {
     struct shared_info *shared_info = get_shared_info();
diff --git a/tools/firmware/hvmloader/util.h b/tools/firmware/hvmloader/util.h
index a70e4aa..5f48a86 100644
--- a/tools/firmware/hvmloader/util.h
+++ b/tools/firmware/hvmloader/util.h
@@ -236,6 +236,8 @@ unsigned long create_pir_tables(void);
 void smp_initialise(void);
 
 #include "e820.h"
+struct rmrr_map rmrr_e820map;
+struct rmrr_map *get_rmrr_map_info(void);
 int build_e820_table(struct e820entry *e820,
                      unsigned int lowmem_reserved_base,
                      unsigned int bios_image_base);
-- 
1.9.1

[RFC][v3][PATCH 4/6] xen:x86: add XENMEM_reserved_device_memory_map to expose RMRR

[Tiejun Chen]

We should expose RMRR mapping to libxc, then setup_guest() can
check if current MMIO is not covered by any RMRR mapping.

Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
---
 xen/arch/x86/mm.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index d23cb3f..fb6e92f 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -4769,6 +4769,38 @@ long arch_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
         return 0;
     }
 
+    case XENMEM_reserved_device_memory_map:
+    {
+        struct xen_memory_map map;
+        XEN_GUEST_HANDLE(e820entry_t) buffer;
+        XEN_GUEST_HANDLE_PARAM(e820entry_t) buffer_param;
+        unsigned int i;
+
+        if ( copy_from_guest(&map, arg, 1) )
+            return -EFAULT;
+        if ( map.nr_entries < rmrr_maps.nr_map + 1 )
+            return -EINVAL;
+
+        buffer_param = guest_handle_cast(map.buffer, e820entry_t);
+        buffer = guest_handle_from_param(buffer_param, e820entry_t);
+        if ( !guest_handle_okay(buffer, map.nr_entries) )
+            return -EFAULT;
+
+        /* Currently we just need to cover RMRR. */
+        for ( i = 0; i < rmrr_maps.nr_map; ++i )
+        {
+            if ( __copy_to_guest_offset(buffer, i, rmrr_maps.map + i, 1) )
+                return -EFAULT;
+        }
+
+        map.nr_entries = rmrr_maps.nr_map;
+
+        if ( __copy_to_guest(arg, &map, 1) )
+            return -EFAULT;
+
+        return 0;
+    }
+
     case XENMEM_machphys_mapping:
     {
         struct xen_machphys_mapping mapping = {
-- 
1.9.1

[RFC][v3][PATCH 5/6] tools:libxc: check if mmio BAR is out of RMRR mappings

[Tiejun Chen]

We need to avoid allocating MMIO BAR conficting to RMRR range.

Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
---
 tools/libxc/xc_domain.c        | 26 ++++++++++++++++++++++++++
 tools/libxc/xc_hvm_build_x86.c | 23 +++++++++++++++++++++++
 tools/libxc/xenctrl.h          |  4 ++++
 3 files changed, 53 insertions(+)

diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c
index c67ac9a..8d011ef 100644
--- a/tools/libxc/xc_domain.c
+++ b/tools/libxc/xc_domain.c
@@ -649,6 +649,32 @@ int xc_domain_set_memory_map(xc_interface *xch,
 
     return rc;
 }
+
+int xc_get_rmrr_map(xc_interface *xch,
+                    struct e820entry entries[],
+                    uint32_t max_entries)
+{
+    int rc;
+    struct xen_memory_map memmap = {
+        .nr_entries = max_entries
+    };
+    DECLARE_HYPERCALL_BOUNCE(entries, sizeof(struct e820entry) * max_entries,
+                             XC_HYPERCALL_BUFFER_BOUNCE_OUT);
+
+    if ( !entries || xc_hypercall_bounce_pre(xch, entries) || max_entries <= 1)
+        return -1;
+
+
+    set_xen_guest_handle(memmap.buffer, entries);
+
+    rc = do_memory_op(xch, XENMEM_reserved_device_memory_map,
+                      &memmap, sizeof(memmap));
+
+    xc_hypercall_bounce_post(xch, entries);
+
+    return rc ? rc : memmap.nr_entries;
+}
+
 int xc_get_machine_memory_map(xc_interface *xch,
                               struct e820entry entries[],
                               uint32_t max_entries)
diff --git a/tools/libxc/xc_hvm_build_x86.c b/tools/libxc/xc_hvm_build_x86.c
index c81a25b..2196cdb 100644
--- a/tools/libxc/xc_hvm_build_x86.c
+++ b/tools/libxc/xc_hvm_build_x86.c
@@ -262,6 +262,8 @@ static int setup_guest(xc_interface *xch,
     int claim_enabled = args->claim_enabled;
     xen_pfn_t special_array[NR_SPECIAL_PAGES];
     xen_pfn_t ioreq_server_array[NR_IOREQ_SERVER_PAGES];
+    struct e820entry map[E820MAX];
+    uint64_t rmrr_start = 0, rmrr_end = 0;
 
     if ( nr_pages > target_pages )
         pod_mode = XENMEMF_populate_on_demand;
@@ -300,6 +302,27 @@ static int setup_guest(xc_interface *xch,
         goto error_out;
     }
 
+    /* We should check if mmio range is out of RMRR mapping. */
+    rc = xc_get_rmrr_map(xch, map, E820MAX);
+    if (rc < 0)
+    {
+        PERROR("Could not get RMRR info on domain");
+    }
+    else if ( rc )
+    {
+        for ( i = 0; i < rc; i++ )
+        {
+            rmrr_start = map[i].addr;
+            rmrr_end = map[i].addr + map[i].size + 1;
+            if ( check_mmio_hole(rmrr_start, map[i].size + 1, mmio_start, mmio_size) )
+            {
+                PERROR("MMIO: [%lx]<->[%lx] overlap RMRR [%lx]<->[%lx]\n",
+                       mmio_start, (mmio_start + mmio_size), rmrr_start, rmrr_end);
+                goto error_out;
+            }
+        }
+    }
+
     for ( i = 0; i < nr_pages; i++ )
         page_array[i] = i;
     for ( i = mmio_start >> PAGE_SHIFT; i < nr_pages; i++ )
diff --git a/tools/libxc/xenctrl.h b/tools/libxc/xenctrl.h
index 1c5d0db..6d3b135 100644
--- a/tools/libxc/xenctrl.h
+++ b/tools/libxc/xenctrl.h
@@ -1270,6 +1270,10 @@ int xc_domain_set_memory_map(xc_interface *xch,
 int xc_get_machine_memory_map(xc_interface *xch,
                               struct e820entry entries[],
                               uint32_t max_entries);
+
+int xc_get_rmrr_map(xc_interface *xch,
+                    struct e820entry entries[],
+                    uint32_t max_entries);
 #endif
 int xc_domain_set_time_offset(xc_interface *xch,
                               uint32_t domid,
-- 
1.9.1

[RFC][v3][PATCH 6/6] xen:vtd: make USB RMRR mapping safe

[Tiejun Chen]

We already reserve all RMRR mappings so USB should be safe with
RMRR.

Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
---
 xen/drivers/passthrough/vtd/iommu.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
index 042b882..319ae05 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -2273,16 +2273,8 @@ static int intel_iommu_assign_device(
     if ( ret )
         goto done;
 
-    /* FIXME: Because USB RMRR conflicts with guest bios region,
-     * ignore USB RMRR temporarily.
-     */
     seg = pdev->seg;
     bus = pdev->bus;
-    if ( is_usb_device(seg, bus, pdev->devfn) )
-    {
-        ret = 0;
-        goto done;
-    }
 
     /* Setup rmrr identity mapping */
     for_each_rmrr_device( rmrr, bdf, i )
-- 
1.9.1

Re: [RFC][v3][PATCH 1/6] xen:x86: record RMRR mappings

[Andrew Cooper]

On 15/08/14 09:27, Tiejun Chen wrote:
> We will expose RMRR mappings to VM so need to record all
> RMRR mappings firstly.
>
> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
> ---
>  xen/arch/x86/e820.c                |  2 ++
>  xen/drivers/passthrough/vtd/dmar.c | 14 ++++++++++++++
>  xen/include/asm-x86/e820.h         |  3 +++
>  3 files changed, 19 insertions(+)
>
> diff --git a/xen/arch/x86/e820.c b/xen/arch/x86/e820.c
> index 55fe0d6..db44afd 100644
> --- a/xen/arch/x86/e820.c
> +++ b/xen/arch/x86/e820.c
> @@ -34,6 +34,8 @@ static bool_t __initdata e820_verbose;
>  boolean_param("e820-verbose", e820_verbose);
>  
>  struct e820map e820;
> +/* Used to record RMRR mapping. */
> +rmrr_maps_t rmrr_maps;
>  
>  /*
>   * This function checks if the entire range <start,end> is mapped with type.
> diff --git a/xen/drivers/passthrough/vtd/dmar.c b/xen/drivers/passthrough/vtd/dmar.c
> index 1152c3a..24321e3 100644
> --- a/xen/drivers/passthrough/vtd/dmar.c
> +++ b/xen/drivers/passthrough/vtd/dmar.c
> @@ -29,6 +29,7 @@
>  #include <xen/pci.h>
>  #include <xen/pci_regs.h>
>  #include <asm/string.h>
> +#include <asm/e820.h>
>  #include "dmar.h"
>  #include "iommu.h"
>  #include "extern.h"
> @@ -80,6 +81,18 @@ static int __init acpi_register_rmrr_unit(struct acpi_rmrr_unit *rmrr)
>      return 0;
>  }
>  
> +/* Record RMRR mapping to ready expose VM. */
> +static int __init rmrr_maps_register(struct acpi_rmrr_unit *rmrr)
> +{

You absolutely need some protection against calling this function more
than 128 times, or you need to do dynamic allocation of the storage.

> +    rmrr_maps.map[rmrr_maps.nr_map].addr = rmrr->base_address;
> +    rmrr_maps.map[rmrr_maps.nr_map].size = rmrr->end_address -
> +                                           rmrr->base_address;
> +    rmrr_maps.map[rmrr_maps.nr_map].type = E820_RESERVED;
> +    rmrr_maps.nr_map++;
> +
> +    return 0;
> +}
> +
>  static void __init disable_all_dmar_units(void)
>  {
>      struct acpi_drhd_unit *drhd, *_drhd;
> @@ -675,6 +688,7 @@ acpi_parse_one_rmrr(struct acpi_dmar_header *header)
>                          " end_address %"PRIx64"\n",
>                          rmrru->base_address, rmrru->end_address);
>              acpi_register_rmrr_unit(rmrru);
> +            rmrr_maps_register(rmrru);
>          }
>      }
>  
> diff --git a/xen/include/asm-x86/e820.h b/xen/include/asm-x86/e820.h
> index 71a804c..74262c9 100644
> --- a/xen/include/asm-x86/e820.h
> +++ b/xen/include/asm-x86/e820.h
> @@ -23,6 +23,8 @@ struct e820map {
>      struct e820entry map[E820MAX];
>  };
>  
> +typedef struct e820map rmrr_maps_t;

This type is a single map of RMRR regions, not multiple maps. 
rmrr_map_t please.

~Andrew

> +
>  extern int e820_all_mapped(u64 start, u64 end, unsigned type);
>  extern int reserve_e820_ram(struct e820map *e820, uint64_t s, uint64_t e);
>  extern int e820_change_range_type(
> @@ -32,6 +34,7 @@ extern int e820_add_range(
>      struct e820map *, uint64_t s, uint64_t e, uint32_t type);
>  extern unsigned long init_e820(const char *, struct e820entry *, int *);
>  extern struct e820map e820;
> +extern rmrr_maps_t rmrr_maps;
>  
>  /* These symbols live in the boot trampoline. */
>  extern struct e820entry e820map[];

Re: [RFC][v3][PATCH 2/6] xen:x86: introduce a new hypercall to get RMRR mappings

[Andrew Cooper]

On 15/08/14 09:27, Tiejun Chen wrote:
> We need this new hypercall to get RMRR mapping for VM.
>
> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
> ---
>  xen/arch/x86/x86_64/compat/mm.c |  9 +++++++++
>  xen/include/public/memory.h     | 14 +++++++++++++-
>  2 files changed, 22 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/x86/x86_64/compat/mm.c b/xen/arch/x86/x86_64/compat/mm.c
> index 69c6195..ff16f17 100644
> --- a/xen/arch/x86/x86_64/compat/mm.c
> +++ b/xen/arch/x86/x86_64/compat/mm.c
> @@ -132,6 +132,15 @@ int compat_arch_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>          break;
>      }
>  
> +    case XENMEM_reserved_device_memory_map:
> +    {
> +        /* Currently we just need to cover RMRR. */
> +        if ( copy_to_guest(arg, &rmrr_maps, 1) )
> +            return -EFAULT;

This will trivially clobber the hypercaller's stack/heap.

You are not even using the correct indirection of
xen_rmrr_memory_map_t.buffer

You *must* start by copying xen_rmrr_memory_map_t from the guest.

~Andrew

> +
> +        return 0;
> +    }
> +
>      case XENMEM_machphys_mapping:
>      {
>          struct domain *d = current->domain;
> diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h
> index 2c57aa0..13e539f 100644
> --- a/xen/include/public/memory.h
> +++ b/xen/include/public/memory.h
> @@ -523,7 +523,19 @@ DEFINE_XEN_GUEST_HANDLE(xen_mem_sharing_op_t);
>  
>  #endif /* defined(__XEN__) || defined(__XEN_TOOLS__) */
>  
> -/* Next available subop number is 26 */
> +/*
> + * Some devices may reserve some range.
> + *
> + * Currently we just have RMRR
> + * - Reserved memory Region Reporting Structure,
> + * So returns the RMRR memory map as it was when the domain
> + * was started.
> + */
> +#define XENMEM_reserved_device_memory_map   26
> +typedef struct xen_memory_map xen_rmrr_memory_map_t;
> +DEFINE_XEN_GUEST_HANDLE(xen_rmrr_memory_map_t);
> +
> +/* Next available subop number is 27 */
>  
>  #endif /* __XEN_PUBLIC_MEMORY_H__ */
>  

Re: [RFC][v3][PATCH 3/6] tools:firmware:hvmloader: reserve RMRR mappings in e820

[Andrew Cooper]

On 15/08/14 09:27, Tiejun Chen wrote:
> We need to reserve all RMRR mappings in e820 to avoid any
> potential guest memory conflict.
>
> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
> ---
>  tools/firmware/hvmloader/e820.c | 94 +++++++++++++++++++++++++++++++++++++++++
>  tools/firmware/hvmloader/e820.h |  6 +++
>  tools/firmware/hvmloader/util.c | 16 +++++++
>  tools/firmware/hvmloader/util.h |  2 +
>  4 files changed, 118 insertions(+)
>
> diff --git a/tools/firmware/hvmloader/e820.c b/tools/firmware/hvmloader/e820.c
> index 2e05e93..7f54eab 100644
> --- a/tools/firmware/hvmloader/e820.c
> +++ b/tools/firmware/hvmloader/e820.c
> @@ -68,12 +68,89 @@ void dump_e820_table(struct e820entry *e820, unsigned int nr)
>      }
>  }

Please apply Xen coding style to all of this file - there are a lot of
errors.

>  
> +static unsigned int construct_rmrr_e820_maps(unsigned int nr,
> +                                     struct rmrr_map *e820_rmrr_map,
> +                                     struct e820entry *e820)
> +{
> +    unsigned int i = 0, j = 0, m = 0, sum_nr = 0;
> +    uint64_t start, end, rmrr_start, rmrr_end;
> +    unsigned int insert = 0, do_insert = 0;
> +
> +do_real_construct:
> +    sum_nr = nr + e820_rmrr_map->nr_map;
> +    for ( i = 0; i < e820_rmrr_map->nr_map; i++ )
> +    {
> +        rmrr_start = e820_rmrr_map->map[i].addr;
> +        rmrr_end = e820_rmrr_map->map[i].addr +
> +                   e820_rmrr_map->map[i].size + 1;
> +
> +        for ( j = 0; j < nr; j++ )
> +        {
> +            end = e820[j].addr + e820[j].size;
> +            start = e820[j+1].addr;
> +
> +            /* Between those existing e820 entries. */
> +            if ( (rmrr_start > end) && (rmrr_end < start) )
> +            {
> +                if (do_insert)
> +                {
> +                    /* Move to free this entry. */
> +                    for ( m = sum_nr - 1; m > j; m-- )
> +                    {
> +                        e820[m].addr = e820[m-1].addr;
> +                        e820[m].size = e820[m-1].size;
> +                        e820[m].type = e820[m-1].type;
> +                    }
> +
> +                    /* Then fill RMRR into that entry. */
> +                    e820[j+1].addr = rmrr_start;
> +                    e820[j+1].size = rmrr_end - rmrr_start;
> +                    e820[j+1].type = E820_RESERVED;
> +                    nr++;
> +                }
> +                insert++;
> +            }
> +            /* Already at the end. */
> +            else if ( (rmrr_start > end) && !start )
> +            {
> +                if (do_insert)
> +                {
> +                    e820[nr].addr = rmrr_start;
> +                    e820[nr].size = rmrr_end - rmrr_start;
> +                    e820[nr].type = E820_RESERVED;
> +                    nr++;
> +                }
> +                insert++;
> +            }
> +        }
> +    }
> +
> +    /* Just return if done. */
> +    if (do_insert)
> +        return nr;
> +
> +    /* Fine to construct RMRR mappings into e820. */
> +    if ( insert == e820_rmrr_map->nr_map)
> +    {
> +        do_insert = 1;
> +        goto do_real_construct;
> +    }
> +    /* Overlap. */
> +    else
> +    {
> +        printf("RMRR overlap with thoese existing e820 entries!\n");
> +        printf("So we don't construct RMRR mapping in e820!\n");
> +    }
> +
> +    return nr;
> +}
>  /* Create an E820 table based on memory parameters provided in hvm_info. */
>  int build_e820_table(struct e820entry *e820,
>                       unsigned int lowmem_reserved_base,
>                       unsigned int bios_image_base)
>  {
>      unsigned int nr = 0;
> +    struct rmrr_map *rmrr_maps;
>  
>      if ( !lowmem_reserved_base )
>              lowmem_reserved_base = 0xA0000;
> @@ -169,6 +246,23 @@ int build_e820_table(struct e820entry *e820,
>          nr++;
>      }
>  
> +    /* We'd better reserve RMRR mapping for each VM to avoid potential
> +     * memory conflict.
> +     */
> +    rmrr_maps = get_rmrr_map_info();
> +    if ( rmrr_maps->nr_map )
> +    {
> +        if ( (nr + rmrr_maps->nr_map) > E820MAX )
> +        {
> +            printf(" No free space to insert all RMRR mapping entry!!\n");
> +            return nr;
> +        }
> +        else
> +        {
> +            nr = construct_rmrr_e820_maps(nr, rmrr_maps, e820);
> +        }
> +    }
> +
>      return nr;
>  }
>  
> diff --git a/tools/firmware/hvmloader/e820.h b/tools/firmware/hvmloader/e820.h
> index b2ead7f..a61b80b 100644
> --- a/tools/firmware/hvmloader/e820.h
> +++ b/tools/firmware/hvmloader/e820.h
> @@ -15,6 +15,12 @@ struct e820entry {
>      uint32_t type;
>  } __attribute__((packed));
>  
> +#define E820MAX 128
> +
> +struct rmrr_map {
> +    unsigned int nr_map;
> +    struct e820entry map[E820MAX];
> +};
>  #endif /* __HVMLOADER_E820_H__ */
>  
>  /*
> diff --git a/tools/firmware/hvmloader/util.c b/tools/firmware/hvmloader/util.c
> index 80d822f..f63434a 100644
> --- a/tools/firmware/hvmloader/util.c
> +++ b/tools/firmware/hvmloader/util.c
> @@ -766,6 +766,22 @@ struct shared_info *get_shared_info(void)
>      return shared_info;
>  }
>  
> +struct rmrr_map *get_rmrr_map_info(void)
> +{
> +    static int no_rmrr = 1;
> +
> +    if ( no_rmrr == 0 )
> +        return &rmrr_e820map;
> +
> +    if ( hypercall_memory_op(XENMEM_reserved_device_memory_map,
> +                             &rmrr_e820map) != 0 )

This *only* works because you are not passing a xen_rmrr_memory_map_t as
per the hypercall definition, and Xen is clobbering your heap in a way
which matches the hvmloader layout.

> +        BUG();
> +
> +    no_rmrr = 0;
> +
> +    return &rmrr_e820map;
> +}
> +
>  uint16_t get_cpu_mhz(void)
>  {
>      struct shared_info *shared_info = get_shared_info();
> diff --git a/tools/firmware/hvmloader/util.h b/tools/firmware/hvmloader/util.h
> index a70e4aa..5f48a86 100644
> --- a/tools/firmware/hvmloader/util.h
> +++ b/tools/firmware/hvmloader/util.h
> @@ -236,6 +236,8 @@ unsigned long create_pir_tables(void);
>  void smp_initialise(void);
>  
>  #include "e820.h"
> +struct rmrr_map rmrr_e820map;

Unless I am mistaken, this will put an rmrr_map object named
rmrr_e820map in each translation unit which includes util.h  I suspect
it is not what you actually want to do.

~Andrew

> +struct rmrr_map *get_rmrr_map_info(void);
>  int build_e820_table(struct e820entry *e820,
>                       unsigned int lowmem_reserved_base,
>                       unsigned int bios_image_base);

Re: [RFC][v3][PATCH 4/6] xen:x86: add XENMEM_reserved_device_memory_map to expose RMRR

[Andrew Cooper]

On 15/08/14 09:27, Tiejun Chen wrote:
> We should expose RMRR mapping to libxc, then setup_guest() can
> check if current MMIO is not covered by any RMRR mapping.
>
> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
> ---
>  xen/arch/x86/mm.c | 32 ++++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
>
> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> index d23cb3f..fb6e92f 100644
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -4769,6 +4769,38 @@ long arch_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>          return 0;
>      }
>  
> +    case XENMEM_reserved_device_memory_map:
> +    {
> +        struct xen_memory_map map;
> +        XEN_GUEST_HANDLE(e820entry_t) buffer;
> +        XEN_GUEST_HANDLE_PARAM(e820entry_t) buffer_param;
> +        unsigned int i;
> +
> +        if ( copy_from_guest(&map, arg, 1) )
> +            return -EFAULT;

This hypercall implementation is looking somewhat more plausible, but
still has some issues.

> +        if ( map.nr_entries < rmrr_maps.nr_map + 1 )
> +            return -EINVAL;

This causes a fencepost error, does it not?

Furthermore, the useful error would be to return -ENOBUFS and fill
arg.nr_entries with the rmrr_maps.nr_map so the caller can allocate an
appropriately sized buffer.


It is also very common with hypercalls like this to have allow a null
guest handle as an explicit request for size.

See XEN_DOMCTL_get_vcpu_msrs as an (admittedly more complicated) example.

> +
> +        buffer_param = guest_handle_cast(map.buffer, e820entry_t);
> +        buffer = guest_handle_from_param(buffer_param, e820entry_t);
> +        if ( !guest_handle_okay(buffer, map.nr_entries) )
> +            return -EFAULT;
> +
> +        /* Currently we just need to cover RMRR. */
> +        for ( i = 0; i < rmrr_maps.nr_map; ++i )
> +        {
> +            if ( __copy_to_guest_offset(buffer, i, rmrr_maps.map + i, 1) )
> +                return -EFAULT;

What is wrong with a single copy_to_guest_offset() ?

> +        }
> +
> +        map.nr_entries = rmrr_maps.nr_map;
> +
> +        if ( __copy_to_guest(arg, &map, 1) )
> +            return -EFAULT;
> +
> +        return 0;
> +    }
> +
>      case XENMEM_machphys_mapping:
>      {
>          struct xen_machphys_mapping mapping = {

Re: [RFC][v3][PATCH 5/6] tools:libxc: check if mmio BAR is out of RMRR mappings

[Andrew Cooper]

On 15/08/14 09:27, Tiejun Chen wrote:
> We need to avoid allocating MMIO BAR conficting to RMRR range.
>
> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
> ---
>  tools/libxc/xc_domain.c        | 26 ++++++++++++++++++++++++++
>  tools/libxc/xc_hvm_build_x86.c | 23 +++++++++++++++++++++++
>  tools/libxc/xenctrl.h          |  4 ++++
>  3 files changed, 53 insertions(+)
>
> diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c
> index c67ac9a..8d011ef 100644
> --- a/tools/libxc/xc_domain.c
> +++ b/tools/libxc/xc_domain.c
> @@ -649,6 +649,32 @@ int xc_domain_set_memory_map(xc_interface *xch,
>  
>      return rc;
>  }
> +
> +int xc_get_rmrr_map(xc_interface *xch,

This function name should match the hypercall.  Perhaps
xc_reserved_device_memory_map() ?

> +                    struct e820entry entries[],
> +                    uint32_t max_entries)

This libxc function would be far more use if it took a single
xen_memory_map parameter, rather than splitting the return information
from Xen between one of the parameters and the return value.

~Andrew

> +{
> +    int rc;
> +    struct xen_memory_map memmap = {
> +        .nr_entries = max_entries
> +    };
> +    DECLARE_HYPERCALL_BOUNCE(entries, sizeof(struct e820entry) * max_entries,
> +                             XC_HYPERCALL_BUFFER_BOUNCE_OUT);
> +
> +    if ( !entries || xc_hypercall_bounce_pre(xch, entries) || max_entries <= 1)
> +        return -1;
> +
> +
> +    set_xen_guest_handle(memmap.buffer, entries);
> +
> +    rc = do_memory_op(xch, XENMEM_reserved_device_memory_map,
> +                      &memmap, sizeof(memmap));
> +
> +    xc_hypercall_bounce_post(xch, entries);
> +
> +    return rc ? rc : memmap.nr_entries;
> +}
> +
>  int xc_get_machine_memory_map(xc_interface *xch,
>                                struct e820entry entries[],
>                                uint32_t max_entries)
> diff --git a/tools/libxc/xc_hvm_build_x86.c b/tools/libxc/xc_hvm_build_x86.c
> index c81a25b..2196cdb 100644
> --- a/tools/libxc/xc_hvm_build_x86.c
> +++ b/tools/libxc/xc_hvm_build_x86.c
> @@ -262,6 +262,8 @@ static int setup_guest(xc_interface *xch,
>      int claim_enabled = args->claim_enabled;
>      xen_pfn_t special_array[NR_SPECIAL_PAGES];
>      xen_pfn_t ioreq_server_array[NR_IOREQ_SERVER_PAGES];
> +    struct e820entry map[E820MAX];
> +    uint64_t rmrr_start = 0, rmrr_end = 0;
>  
>      if ( nr_pages > target_pages )
>          pod_mode = XENMEMF_populate_on_demand;
> @@ -300,6 +302,27 @@ static int setup_guest(xc_interface *xch,
>          goto error_out;
>      }
>  
> +    /* We should check if mmio range is out of RMRR mapping. */
> +    rc = xc_get_rmrr_map(xch, map, E820MAX);
> +    if (rc < 0)
> +    {
> +        PERROR("Could not get RMRR info on domain");
> +    }
> +    else if ( rc )
> +    {
> +        for ( i = 0; i < rc; i++ )
> +        {
> +            rmrr_start = map[i].addr;
> +            rmrr_end = map[i].addr + map[i].size + 1;
> +            if ( check_mmio_hole(rmrr_start, map[i].size + 1, mmio_start, mmio_size) )
> +            {
> +                PERROR("MMIO: [%lx]<->[%lx] overlap RMRR [%lx]<->[%lx]\n",
> +                       mmio_start, (mmio_start + mmio_size), rmrr_start, rmrr_end);
> +                goto error_out;
> +            }
> +        }
> +    }
> +
>      for ( i = 0; i < nr_pages; i++ )
>          page_array[i] = i;
>      for ( i = mmio_start >> PAGE_SHIFT; i < nr_pages; i++ )
> diff --git a/tools/libxc/xenctrl.h b/tools/libxc/xenctrl.h
> index 1c5d0db..6d3b135 100644
> --- a/tools/libxc/xenctrl.h
> +++ b/tools/libxc/xenctrl.h
> @@ -1270,6 +1270,10 @@ int xc_domain_set_memory_map(xc_interface *xch,
>  int xc_get_machine_memory_map(xc_interface *xch,
>                                struct e820entry entries[],
>                                uint32_t max_entries);
> +
> +int xc_get_rmrr_map(xc_interface *xch,
> +                    struct e820entry entries[],
> +                    uint32_t max_entries);
>  #endif
>  int xc_domain_set_time_offset(xc_interface *xch,
>                                uint32_t domid,

Re: [RFC][v3][PATCH 1/6] xen:x86: record RMRR mappings

[Jan Beulich]

>>> On 15.08.14 at 11:39, <andrew.cooper3@citrix.com> wrote:
>> @@ -80,6 +81,18 @@ static int __init acpi_register_rmrr_unit(struct acpi_rmrr_unit *rmrr)
>>      return 0;
>>  }
>>  
>> +/* Record RMRR mapping to ready expose VM. */
>> +static int __init rmrr_maps_register(struct acpi_rmrr_unit *rmrr)
>> +{
> 
> You absolutely need some protection against calling this function more
> than 128 times, or you need to do dynamic allocation of the storage.

This together with ...

>> --- a/xen/include/asm-x86/e820.h
>> +++ b/xen/include/asm-x86/e820.h
>> @@ -23,6 +23,8 @@ struct e820map {
>>      struct e820entry map[E820MAX];
>>  };
>>  
>> +typedef struct e820map rmrr_maps_t;
> 
> This type is a single map of RMRR regions, not multiple maps. 
> rmrr_map_t please.

... this once again stresses what I stated previously: Piggybacking
on the E820 handling here is just the wrong approach. There's
really no correlation with E820 other than us wanting to use the
gathered information for (among other things) adjusting the guest
E820 table. But that doesn't in any way require any re-use of
non-suitable data structures.

In fact I don't see the need for this first patch anyway, as RMRRs
are already being put on a linked list as they get found. I.e. the
patch here just clones existing information.

Jan

Re: [RFC][v3][PATCH 1/6] xen:x86: record RMRR mappings

[Chen, Tiejun]

On 2014/8/16 0:29, Jan Beulich wrote:
>>>> On 15.08.14 at 11:39, <andrew.cooper3@citrix.com> wrote:
>>> @@ -80,6 +81,18 @@ static int __init acpi_register_rmrr_unit(struct acpi_rmrr_unit *rmrr)
>>>       return 0;
>>>   }
>>>
>>> +/* Record RMRR mapping to ready expose VM. */
>>> +static int __init rmrr_maps_register(struct acpi_rmrr_unit *rmrr)
>>> +{
>>
>> You absolutely need some protection against calling this function more
>> than 128 times, or you need to do dynamic allocation of the storage.
>
> This together with ...
>
>>> --- a/xen/include/asm-x86/e820.h
>>> +++ b/xen/include/asm-x86/e820.h
>>> @@ -23,6 +23,8 @@ struct e820map {
>>>       struct e820entry map[E820MAX];
>>>   };
>>>
>>> +typedef struct e820map rmrr_maps_t;
>>
>> This type is a single map of RMRR regions, not multiple maps.
>> rmrr_map_t please.
>
> ... this once again stresses what I stated previously: Piggybacking
> on the E820 handling here is just the wrong approach. There's
> really no correlation with E820 other than us wanting to use the
> gathered information for (among other things) adjusting the guest
> E820 table. But that doesn't in any way require any re-use of
> non-suitable data structures.

Why are you saying this is not suitable?

We need a structure to represent a RMRR entry including three fields, 
start, size and type, and especially, essentially RMRR entry belongs to 
e820 table as one entry.

>
> In fact I don't see the need for this first patch anyway, as RMRRs
> are already being put on a linked list as they get found. I.e. the

Yes, that list, acpi_rmrr_unit, can be exposed here. But before you copy 
to guest, don't you need to grab those fields from that list then 
convert them as a suitable structure (mostly this is still same as 
e820entry) to be copied into a buffer?

Thanks
Tiejun

> patch here just clones existing information.
>
> Jan
>
>

Re: [RFC][v3][PATCH 1/6] xen:x86: record RMRR mappings

[Chen, Tiejun]

On 2014/8/15 17:39, Andrew Cooper wrote:
> On 15/08/14 09:27, Tiejun Chen wrote:
>> We will expose RMRR mappings to VM so need to record all
>> RMRR mappings firstly.
>>
>> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
>> ---
>>   xen/arch/x86/e820.c                |  2 ++
>>   xen/drivers/passthrough/vtd/dmar.c | 14 ++++++++++++++
>>   xen/include/asm-x86/e820.h         |  3 +++
>>   3 files changed, 19 insertions(+)
>>
>> diff --git a/xen/arch/x86/e820.c b/xen/arch/x86/e820.c
>> index 55fe0d6..db44afd 100644
>> --- a/xen/arch/x86/e820.c
>> +++ b/xen/arch/x86/e820.c
>> @@ -34,6 +34,8 @@ static bool_t __initdata e820_verbose;
>>   boolean_param("e820-verbose", e820_verbose);
>>
>>   struct e820map e820;
>> +/* Used to record RMRR mapping. */
>> +rmrr_maps_t rmrr_maps;
>>
>>   /*
>>    * This function checks if the entire range <start,end> is mapped with type.
>> diff --git a/xen/drivers/passthrough/vtd/dmar.c b/xen/drivers/passthrough/vtd/dmar.c
>> index 1152c3a..24321e3 100644
>> --- a/xen/drivers/passthrough/vtd/dmar.c
>> +++ b/xen/drivers/passthrough/vtd/dmar.c
>> @@ -29,6 +29,7 @@
>>   #include <xen/pci.h>
>>   #include <xen/pci_regs.h>
>>   #include <asm/string.h>
>> +#include <asm/e820.h>
>>   #include "dmar.h"
>>   #include "iommu.h"
>>   #include "extern.h"
>> @@ -80,6 +81,18 @@ static int __init acpi_register_rmrr_unit(struct acpi_rmrr_unit *rmrr)
>>       return 0;
>>   }
>>
>> +/* Record RMRR mapping to ready expose VM. */
>> +static int __init rmrr_maps_register(struct acpi_rmrr_unit *rmrr)
>> +{
>
> You absolutely need some protection against calling this function more

Could you show this assumed scenario?

IMO, this should never be happened since we always use a e820 table to 
cover RMRR range/entries.

> than 128 times, or you need to do dynamic allocation of the storage.
>
>> +    rmrr_maps.map[rmrr_maps.nr_map].addr = rmrr->base_address;
>> +    rmrr_maps.map[rmrr_maps.nr_map].size = rmrr->end_address -
>> +                                           rmrr->base_address;
>> +    rmrr_maps.map[rmrr_maps.nr_map].type = E820_RESERVED;
>> +    rmrr_maps.nr_map++;
>> +
>> +    return 0;
>> +}
>> +
>>   static void __init disable_all_dmar_units(void)
>>   {
>>       struct acpi_drhd_unit *drhd, *_drhd;
>> @@ -675,6 +688,7 @@ acpi_parse_one_rmrr(struct acpi_dmar_header *header)
>>                           " end_address %"PRIx64"\n",
>>                           rmrru->base_address, rmrru->end_address);
>>               acpi_register_rmrr_unit(rmrru);
>> +            rmrr_maps_register(rmrru);
>>           }
>>       }
>>
>> diff --git a/xen/include/asm-x86/e820.h b/xen/include/asm-x86/e820.h
>> index 71a804c..74262c9 100644
>> --- a/xen/include/asm-x86/e820.h
>> +++ b/xen/include/asm-x86/e820.h
>> @@ -23,6 +23,8 @@ struct e820map {
>>       struct e820entry map[E820MAX];
>>   };
>>
>> +typedef struct e820map rmrr_maps_t;
>
> This type is a single map of RMRR regions, not multiple maps.
> rmrr_map_t please.

Okay.

Thanks
Tiejun

>
> ~Andrew
>
>> +
>>   extern int e820_all_mapped(u64 start, u64 end, unsigned type);
>>   extern int reserve_e820_ram(struct e820map *e820, uint64_t s, uint64_t e);
>>   extern int e820_change_range_type(
>> @@ -32,6 +34,7 @@ extern int e820_add_range(
>>       struct e820map *, uint64_t s, uint64_t e, uint32_t type);
>>   extern unsigned long init_e820(const char *, struct e820entry *, int *);
>>   extern struct e820map e820;
>> +extern rmrr_maps_t rmrr_maps;
>>
>>   /* These symbols live in the boot trampoline. */
>>   extern struct e820entry e820map[];
>
>

Re: [RFC][v3][PATCH 2/6] xen:x86: introduce a new hypercall to get RMRR mappings

[Chen, Tiejun]

On 2014/8/15 17:46, Andrew Cooper wrote:
> On 15/08/14 09:27, Tiejun Chen wrote:
>> We need this new hypercall to get RMRR mapping for VM.
>>
>> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
>> ---
>>   xen/arch/x86/x86_64/compat/mm.c |  9 +++++++++
>>   xen/include/public/memory.h     | 14 +++++++++++++-
>>   2 files changed, 22 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/x86/x86_64/compat/mm.c b/xen/arch/x86/x86_64/compat/mm.c
>> index 69c6195..ff16f17 100644
>> --- a/xen/arch/x86/x86_64/compat/mm.c
>> +++ b/xen/arch/x86/x86_64/compat/mm.c
>> @@ -132,6 +132,15 @@ int compat_arch_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>>           break;
>>       }
>>
>> +    case XENMEM_reserved_device_memory_map:
>> +    {
>> +        /* Currently we just need to cover RMRR. */
>> +        if ( copy_to_guest(arg, &rmrr_maps, 1) )
>> +            return -EFAULT;
>
> This will trivially clobber the hypercaller's stack/heap.
>
> You are not even using the correct indirection of
> xen_rmrr_memory_map_t.buffer
>
> You *must* start by copying xen_rmrr_memory_map_t from the guest.

Okay, I will try to do this.

Thanks
Tiejun

>
> ~Andrew
>
>> +
>> +        return 0;
>> +    }
>> +
>>       case XENMEM_machphys_mapping:
>>       {
>>           struct domain *d = current->domain;
>> diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h
>> index 2c57aa0..13e539f 100644
>> --- a/xen/include/public/memory.h
>> +++ b/xen/include/public/memory.h
>> @@ -523,7 +523,19 @@ DEFINE_XEN_GUEST_HANDLE(xen_mem_sharing_op_t);
>>
>>   #endif /* defined(__XEN__) || defined(__XEN_TOOLS__) */
>>
>> -/* Next available subop number is 26 */
>> +/*
>> + * Some devices may reserve some range.
>> + *
>> + * Currently we just have RMRR
>> + * - Reserved memory Region Reporting Structure,
>> + * So returns the RMRR memory map as it was when the domain
>> + * was started.
>> + */
>> +#define XENMEM_reserved_device_memory_map   26
>> +typedef struct xen_memory_map xen_rmrr_memory_map_t;
>> +DEFINE_XEN_GUEST_HANDLE(xen_rmrr_memory_map_t);
>> +
>> +/* Next available subop number is 27 */
>>
>>   #endif /* __XEN_PUBLIC_MEMORY_H__ */
>>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>

Re: [RFC][v3][PATCH 3/6] tools:firmware:hvmloader: reserve RMRR mappings in e820

[Chen, Tiejun]

On 2014/8/15 17:58, Andrew Cooper wrote:
> On 15/08/14 09:27, Tiejun Chen wrote:
>> We need to reserve all RMRR mappings in e820 to avoid any
>> potential guest memory conflict.
>>
>> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
>> ---
>>   tools/firmware/hvmloader/e820.c | 94 +++++++++++++++++++++++++++++++++++++++++
>>   tools/firmware/hvmloader/e820.h |  6 +++
>>   tools/firmware/hvmloader/util.c | 16 +++++++
>>   tools/firmware/hvmloader/util.h |  2 +
>>   4 files changed, 118 insertions(+)
>>
>> diff --git a/tools/firmware/hvmloader/e820.c b/tools/firmware/hvmloader/e820.c
>> index 2e05e93..7f54eab 100644
>> --- a/tools/firmware/hvmloader/e820.c
>> +++ b/tools/firmware/hvmloader/e820.c
>> @@ -68,12 +68,89 @@ void dump_e820_table(struct e820entry *e820, unsigned int nr)
>>       }
>>   }
>
> Please apply Xen coding style to all of this file - there are a lot of
> errors.

I asked previously what Xen coding style is, and I always try to follow 
some existing code but seems its not enough. Here I still can't find out 
what you're pointing, so please tell me exactly what I should change 
exactly.

And why don't you guy generate a checkpatch.pl file?

>
>>
>> +static unsigned int construct_rmrr_e820_maps(unsigned int nr,
>> +                                     struct rmrr_map *e820_rmrr_map,
>> +                                     struct e820entry *e820)
>> +{
>> +    unsigned int i = 0, j = 0, m = 0, sum_nr = 0;
>> +    uint64_t start, end, rmrr_start, rmrr_end;
>> +    unsigned int insert = 0, do_insert = 0;
>> +
>> +do_real_construct:
>> +    sum_nr = nr + e820_rmrr_map->nr_map;
>> +    for ( i = 0; i < e820_rmrr_map->nr_map; i++ )
>> +    {
>> +        rmrr_start = e820_rmrr_map->map[i].addr;
>> +        rmrr_end = e820_rmrr_map->map[i].addr +
>> +                   e820_rmrr_map->map[i].size + 1;
>> +
>> +        for ( j = 0; j < nr; j++ )
>> +        {
>> +            end = e820[j].addr + e820[j].size;
>> +            start = e820[j+1].addr;
>> +
>> +            /* Between those existing e820 entries. */
>> +            if ( (rmrr_start > end) && (rmrr_end < start) )
>> +            {
>> +                if (do_insert)
>> +                {
>> +                    /* Move to free this entry. */
>> +                    for ( m = sum_nr - 1; m > j; m-- )
>> +                    {
>> +                        e820[m].addr = e820[m-1].addr;
>> +                        e820[m].size = e820[m-1].size;
>> +                        e820[m].type = e820[m-1].type;
>> +                    }
>> +
>> +                    /* Then fill RMRR into that entry. */
>> +                    e820[j+1].addr = rmrr_start;
>> +                    e820[j+1].size = rmrr_end - rmrr_start;
>> +                    e820[j+1].type = E820_RESERVED;
>> +                    nr++;
>> +                }
>> +                insert++;
>> +            }
>> +            /* Already at the end. */
>> +            else if ( (rmrr_start > end) && !start )
>> +            {
>> +                if (do_insert)
>> +                {
>> +                    e820[nr].addr = rmrr_start;
>> +                    e820[nr].size = rmrr_end - rmrr_start;
>> +                    e820[nr].type = E820_RESERVED;
>> +                    nr++;
>> +                }
>> +                insert++;
>> +            }
>> +        }
>> +    }
>> +
>> +    /* Just return if done. */
>> +    if (do_insert)
>> +        return nr;
>> +
>> +    /* Fine to construct RMRR mappings into e820. */
>> +    if ( insert == e820_rmrr_map->nr_map)
>> +    {
>> +        do_insert = 1;
>> +        goto do_real_construct;
>> +    }
>> +    /* Overlap. */
>> +    else
>> +    {
>> +        printf("RMRR overlap with thoese existing e820 entries!\n");
>> +        printf("So we don't construct RMRR mapping in e820!\n");
>> +    }
>> +
>> +    return nr;
>> +}
>>   /* Create an E820 table based on memory parameters provided in hvm_info. */
>>   int build_e820_table(struct e820entry *e820,
>>                        unsigned int lowmem_reserved_base,
>>                        unsigned int bios_image_base)
>>   {
>>       unsigned int nr = 0;
>> +    struct rmrr_map *rmrr_maps;
>>
>>       if ( !lowmem_reserved_base )
>>               lowmem_reserved_base = 0xA0000;
>> @@ -169,6 +246,23 @@ int build_e820_table(struct e820entry *e820,
>>           nr++;
>>       }
>>
>> +    /* We'd better reserve RMRR mapping for each VM to avoid potential
>> +     * memory conflict.
>> +     */
>> +    rmrr_maps = get_rmrr_map_info();
>> +    if ( rmrr_maps->nr_map )
>> +    {
>> +        if ( (nr + rmrr_maps->nr_map) > E820MAX )
>> +        {
>> +            printf(" No free space to insert all RMRR mapping entry!!\n");
>> +            return nr;
>> +        }
>> +        else
>> +        {
>> +            nr = construct_rmrr_e820_maps(nr, rmrr_maps, e820);
>> +        }
>> +    }
>> +
>>       return nr;
>>   }
>>
>> diff --git a/tools/firmware/hvmloader/e820.h b/tools/firmware/hvmloader/e820.h
>> index b2ead7f..a61b80b 100644
>> --- a/tools/firmware/hvmloader/e820.h
>> +++ b/tools/firmware/hvmloader/e820.h
>> @@ -15,6 +15,12 @@ struct e820entry {
>>       uint32_t type;
>>   } __attribute__((packed));
>>
>> +#define E820MAX 128
>> +
>> +struct rmrr_map {
>> +    unsigned int nr_map;
>> +    struct e820entry map[E820MAX];
>> +};
>>   #endif /* __HVMLOADER_E820_H__ */
>>
>>   /*
>> diff --git a/tools/firmware/hvmloader/util.c b/tools/firmware/hvmloader/util.c
>> index 80d822f..f63434a 100644
>> --- a/tools/firmware/hvmloader/util.c
>> +++ b/tools/firmware/hvmloader/util.c
>> @@ -766,6 +766,22 @@ struct shared_info *get_shared_info(void)
>>       return shared_info;
>>   }
>>
>> +struct rmrr_map *get_rmrr_map_info(void)
>> +{
>> +    static int no_rmrr = 1;
>> +
>> +    if ( no_rmrr == 0 )
>> +        return &rmrr_e820map;
>> +
>> +    if ( hypercall_memory_op(XENMEM_reserved_device_memory_map,
>> +                             &rmrr_e820map) != 0 )
>
> This *only* works because you are not passing a xen_rmrr_memory_map_t as
> per the hypercall definition, and Xen is clobbering your heap in a way
> which matches the hvmloader layout.

I will check this.

Thanks
Tiejun

>
>> +        BUG();
>> +
>> +    no_rmrr = 0;
>> +
>> +    return &rmrr_e820map;
>> +}
>> +
>>   uint16_t get_cpu_mhz(void)
>>   {
>>       struct shared_info *shared_info = get_shared_info();
>> diff --git a/tools/firmware/hvmloader/util.h b/tools/firmware/hvmloader/util.h
>> index a70e4aa..5f48a86 100644
>> --- a/tools/firmware/hvmloader/util.h
>> +++ b/tools/firmware/hvmloader/util.h
>> @@ -236,6 +236,8 @@ unsigned long create_pir_tables(void);
>>   void smp_initialise(void);
>>
>>   #include "e820.h"
>> +struct rmrr_map rmrr_e820map;
>
> Unless I am mistaken, this will put an rmrr_map object named
> rmrr_e820map in each translation unit which includes util.h  I suspect
> it is not what you actually want to do.
>
> ~Andrew
>
>> +struct rmrr_map *get_rmrr_map_info(void);
>>   int build_e820_table(struct e820entry *e820,
>>                        unsigned int lowmem_reserved_base,
>>                        unsigned int bios_image_base);
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>

Re: [RFC][v3][PATCH 4/6] xen:x86: add XENMEM_reserved_device_memory_map to expose RMRR

[Chen, Tiejun]

On 2014/8/15 20:15, Andrew Cooper wrote:
> On 15/08/14 09:27, Tiejun Chen wrote:
>> We should expose RMRR mapping to libxc, then setup_guest() can
>> check if current MMIO is not covered by any RMRR mapping.
>>
>> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
>> ---
>>   xen/arch/x86/mm.c | 32 ++++++++++++++++++++++++++++++++
>>   1 file changed, 32 insertions(+)
>>
>> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
>> index d23cb3f..fb6e92f 100644
>> --- a/xen/arch/x86/mm.c
>> +++ b/xen/arch/x86/mm.c
>> @@ -4769,6 +4769,38 @@ long arch_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>>           return 0;
>>       }
>>
>> +    case XENMEM_reserved_device_memory_map:
>> +    {
>> +        struct xen_memory_map map;
>> +        XEN_GUEST_HANDLE(e820entry_t) buffer;
>> +        XEN_GUEST_HANDLE_PARAM(e820entry_t) buffer_param;
>> +        unsigned int i;
>> +
>> +        if ( copy_from_guest(&map, arg, 1) )
>> +            return -EFAULT;
>
> This hypercall implementation is looking somewhat more plausible, but
> still has some issues.
>
>> +        if ( map.nr_entries < rmrr_maps.nr_map + 1 )
>> +            return -EINVAL;
>
> This causes a fencepost error, does it not?

map.nr_entries = E820MAX, and obviously rmrr_maps.nr_map should be 
smaller than far E820MAX. So what is your problem?

Here I have a reference to XENMEM_machine_memory_map.

>
> Furthermore, the useful error would be to return -ENOBUFS and fill
> arg.nr_entries with the rmrr_maps.nr_map so the caller can allocate an
> appropriately sized buffer.
>
>
> It is also very common with hypercalls like this to have allow a null
> guest handle as an explicit request for size.

Looks you like to issue twice time with a hypercall to finish, but 
what's wrong with my way?

Again, here I have a reference to XENMEM_machine_memory_map.

>
> See XEN_DOMCTL_get_vcpu_msrs as an (admittedly more complicated) example.
>
>> +
>> +        buffer_param = guest_handle_cast(map.buffer, e820entry_t);
>> +        buffer = guest_handle_from_param(buffer_param, e820entry_t);
>> +        if ( !guest_handle_okay(buffer, map.nr_entries) )
>> +            return -EFAULT;
>> +
>> +        /* Currently we just need to cover RMRR. */
>> +        for ( i = 0; i < rmrr_maps.nr_map; ++i )
>> +        {
>> +            if ( __copy_to_guest_offset(buffer, i, rmrr_maps.map + i, 1) )
>> +                return -EFAULT;
>
> What is wrong with a single copy_to_guest_offset() ?

I didn't try this but I'd like to try this to check if this still works 
well.

Thanks
Tiejun

>
>> +        }
>> +
>> +        map.nr_entries = rmrr_maps.nr_map;
>> +
>> +        if ( __copy_to_guest(arg, &map, 1) )
>> +            return -EFAULT;
>> +
>> +        return 0;
>> +    }
>> +
>>       case XENMEM_machphys_mapping:
>>       {
>>           struct xen_machphys_mapping mapping = {
>
>

Re: [RFC][v3][PATCH 5/6] tools:libxc: check if mmio BAR is out of RMRR mappings

[Chen, Tiejun]

On 2014/8/15 20:21, Andrew Cooper wrote:
> On 15/08/14 09:27, Tiejun Chen wrote:
>> We need to avoid allocating MMIO BAR conflicting to RMRR range.
>>
>> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
>> ---
>>   tools/libxc/xc_domain.c        | 26 ++++++++++++++++++++++++++
>>   tools/libxc/xc_hvm_build_x86.c | 23 +++++++++++++++++++++++
>>   tools/libxc/xenctrl.h          |  4 ++++
>>   3 files changed, 53 insertions(+)
>>
>> diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c
>> index c67ac9a..8d011ef 100644
>> --- a/tools/libxc/xc_domain.c
>> +++ b/tools/libxc/xc_domain.c
>> @@ -649,6 +649,32 @@ int xc_domain_set_memory_map(xc_interface *xch,
>>
>>       return rc;
>>   }
>> +
>> +int xc_get_rmrr_map(xc_interface *xch,
>
> This function name should match the hypercall.  Perhaps
> xc_reserved_device_memory_map() ?
>

Okay.

>> +                    struct e820entry entries[],
>> +                    uint32_t max_entries)
>
> This libxc function would be far more use if it took a single
> xen_memory_map parameter, rather than splitting the return information
> from Xen between one of the parameters and the return value.

I can't understand why we do something specific here. Please take a look 
at xc_get_machine_memory_map(). So why did you think we should do this?

Thanks
Tiejun

>
> ~Andrew
>
>> +{
>> +    int rc;
>> +    struct xen_memory_map memmap = {
>> +        .nr_entries = max_entries
>> +    };
>> +    DECLARE_HYPERCALL_BOUNCE(entries, sizeof(struct e820entry) * max_entries,
>> +                             XC_HYPERCALL_BUFFER_BOUNCE_OUT);
>> +
>> +    if ( !entries || xc_hypercall_bounce_pre(xch, entries) || max_entries <= 1)
>> +        return -1;
>> +
>> +
>> +    set_xen_guest_handle(memmap.buffer, entries);
>> +
>> +    rc = do_memory_op(xch, XENMEM_reserved_device_memory_map,
>> +                      &memmap, sizeof(memmap));
>> +
>> +    xc_hypercall_bounce_post(xch, entries);
>> +
>> +    return rc ? rc : memmap.nr_entries;
>> +}
>> +
>>   int xc_get_machine_memory_map(xc_interface *xch,
>>                                 struct e820entry entries[],
>>                                 uint32_t max_entries)
>> diff --git a/tools/libxc/xc_hvm_build_x86.c b/tools/libxc/xc_hvm_build_x86.c
>> index c81a25b..2196cdb 100644
>> --- a/tools/libxc/xc_hvm_build_x86.c
>> +++ b/tools/libxc/xc_hvm_build_x86.c
>> @@ -262,6 +262,8 @@ static int setup_guest(xc_interface *xch,
>>       int claim_enabled = args->claim_enabled;
>>       xen_pfn_t special_array[NR_SPECIAL_PAGES];
>>       xen_pfn_t ioreq_server_array[NR_IOREQ_SERVER_PAGES];
>> +    struct e820entry map[E820MAX];
>> +    uint64_t rmrr_start = 0, rmrr_end = 0;
>>
>>       if ( nr_pages > target_pages )
>>           pod_mode = XENMEMF_populate_on_demand;
>> @@ -300,6 +302,27 @@ static int setup_guest(xc_interface *xch,
>>           goto error_out;
>>       }
>>
>> +    /* We should check if mmio range is out of RMRR mapping. */
>> +    rc = xc_get_rmrr_map(xch, map, E820MAX);
>> +    if (rc < 0)
>> +    {
>> +        PERROR("Could not get RMRR info on domain");
>> +    }
>> +    else if ( rc )
>> +    {
>> +        for ( i = 0; i < rc; i++ )
>> +        {
>> +            rmrr_start = map[i].addr;
>> +            rmrr_end = map[i].addr + map[i].size + 1;
>> +            if ( check_mmio_hole(rmrr_start, map[i].size + 1, mmio_start, mmio_size) )
>> +            {
>> +                PERROR("MMIO: [%lx]<->[%lx] overlap RMRR [%lx]<->[%lx]\n",
>> +                       mmio_start, (mmio_start + mmio_size), rmrr_start, rmrr_end);
>> +                goto error_out;
>> +            }
>> +        }
>> +    }
>> +
>>       for ( i = 0; i < nr_pages; i++ )
>>           page_array[i] = i;
>>       for ( i = mmio_start >> PAGE_SHIFT; i < nr_pages; i++ )
>> diff --git a/tools/libxc/xenctrl.h b/tools/libxc/xenctrl.h
>> index 1c5d0db..6d3b135 100644
>> --- a/tools/libxc/xenctrl.h
>> +++ b/tools/libxc/xenctrl.h
>> @@ -1270,6 +1270,10 @@ int xc_domain_set_memory_map(xc_interface *xch,
>>   int xc_get_machine_memory_map(xc_interface *xch,
>>                                 struct e820entry entries[],
>>                                 uint32_t max_entries);
>> +
>> +int xc_get_rmrr_map(xc_interface *xch,
>> +                    struct e820entry entries[],
>> +                    uint32_t max_entries);
>>   #endif
>>   int xc_domain_set_time_offset(xc_interface *xch,
>>                                 uint32_t domid,
>
>

Re: [RFC][v3][PATCH 1/6] xen:x86: record RMRR mappings

[Andrew Cooper]

On 18/08/14 08:45, Chen, Tiejun wrote:
> On 2014/8/15 17:39, Andrew Cooper wrote:
>> On 15/08/14 09:27, Tiejun Chen wrote:
>>> We will expose RMRR mappings to VM so need to record all
>>> RMRR mappings firstly.
>>>
>>> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
>>> ---
>>>   xen/arch/x86/e820.c                |  2 ++
>>>   xen/drivers/passthrough/vtd/dmar.c | 14 ++++++++++++++
>>>   xen/include/asm-x86/e820.h         |  3 +++
>>>   3 files changed, 19 insertions(+)
>>>
>>> diff --git a/xen/arch/x86/e820.c b/xen/arch/x86/e820.c
>>> index 55fe0d6..db44afd 100644
>>> --- a/xen/arch/x86/e820.c
>>> +++ b/xen/arch/x86/e820.c
>>> @@ -34,6 +34,8 @@ static bool_t __initdata e820_verbose;
>>>   boolean_param("e820-verbose", e820_verbose);
>>>
>>>   struct e820map e820;
>>> +/* Used to record RMRR mapping. */
>>> +rmrr_maps_t rmrr_maps;
>>>
>>>   /*
>>>    * This function checks if the entire range <start,end> is mapped
>>> with type.
>>> diff --git a/xen/drivers/passthrough/vtd/dmar.c
>>> b/xen/drivers/passthrough/vtd/dmar.c
>>> index 1152c3a..24321e3 100644
>>> --- a/xen/drivers/passthrough/vtd/dmar.c
>>> +++ b/xen/drivers/passthrough/vtd/dmar.c
>>> @@ -29,6 +29,7 @@
>>>   #include <xen/pci.h>
>>>   #include <xen/pci_regs.h>
>>>   #include <asm/string.h>
>>> +#include <asm/e820.h>
>>>   #include "dmar.h"
>>>   #include "iommu.h"
>>>   #include "extern.h"
>>> @@ -80,6 +81,18 @@ static int __init acpi_register_rmrr_unit(struct
>>> acpi_rmrr_unit *rmrr)
>>>       return 0;
>>>   }
>>>
>>> +/* Record RMRR mapping to ready expose VM. */
>>> +static int __init rmrr_maps_register(struct acpi_rmrr_unit *rmrr)
>>> +{
>>
>> You absolutely need some protection against calling this function more
>
> Could you show this assumed scenario?
>
> IMO, this should never be happened since we always use a e820 table to
> cover RMRR range/entries.

There is, as far as I am aware, no upper bound to the number of RMRRs
reported by the firmware.  rmrr_maps.map is an array of length E820_MAX,
or 128.  If this function is called more than 128 times, you will start
clobbering Xen data.

But as Jan pointed out in the other fork of this thread, this entire
function is not needed, and can be removed.

~Andrew

Re: [RFC][v3][PATCH 1/6] xen:x86: record RMRR mappings

[Andrew Cooper]

On 18/08/14 08:42, Chen, Tiejun wrote:
> On 2014/8/16 0:29, Jan Beulich wrote:
>>>>> On 15.08.14 at 11:39, <andrew.cooper3@citrix.com> wrote:
>>>> @@ -80,6 +81,18 @@ static int __init acpi_register_rmrr_unit(struct
>>>> acpi_rmrr_unit *rmrr)
>>>>       return 0;
>>>>   }
>>>>
>>>> +/* Record RMRR mapping to ready expose VM. */
>>>> +static int __init rmrr_maps_register(struct acpi_rmrr_unit *rmrr)
>>>> +{
>>>
>>> You absolutely need some protection against calling this function more
>>> than 128 times, or you need to do dynamic allocation of the storage.
>>
>> This together with ...
>>
>>>> --- a/xen/include/asm-x86/e820.h
>>>> +++ b/xen/include/asm-x86/e820.h
>>>> @@ -23,6 +23,8 @@ struct e820map {
>>>>       struct e820entry map[E820MAX];
>>>>   };
>>>>
>>>> +typedef struct e820map rmrr_maps_t;
>>>
>>> This type is a single map of RMRR regions, not multiple maps.
>>> rmrr_map_t please.
>>
>> ... this once again stresses what I stated previously: Piggybacking
>> on the E820 handling here is just the wrong approach. There's
>> really no correlation with E820 other than us wanting to use the
>> gathered information for (among other things) adjusting the guest
>> E820 table. But that doesn't in any way require any re-use of
>> non-suitable data structures.
>
> Why are you saying this is not suitable?
>
> We need a structure to represent a RMRR entry including three fields,
> start, size and type, and especially, essentially RMRR entry belongs
> to e820 table as one entry.

Not in Xen.  Only as reported to guests, in which case an e820-like
structure is most appropriate.

>
>>
>> In fact I don't see the need for this first patch anyway, as RMRRs
>> are already being put on a linked list as they get found. I.e. the
>
> Yes, that list, acpi_rmrr_unit, can be exposed here. But before you
> copy to guest, don't you need to grab those fields from that list then
> convert them as a suitable structure (mostly this is still same as
> e820entry) to be copied into a buffer?

Yes, but the hypercall handler can do this which avoids all need to
store an intermediate representation in Xen.

list_for_each_entry(rmrr, &acpi_rmrr_units, list)
{
    e820entry e;

    e.start = ...

    copy_to_guest_offset(...
}

But with appropriate error checking.

~Andrew

Re: [RFC][v3][PATCH 3/6] tools:firmware:hvmloader: reserve RMRR mappings in e820

[Andrew Cooper]

On 18/08/14 08:51, Chen, Tiejun wrote:
> On 2014/8/15 17:58, Andrew Cooper wrote:
>> On 15/08/14 09:27, Tiejun Chen wrote:
>>> We need to reserve all RMRR mappings in e820 to avoid any
>>> potential guest memory conflict.
>>>
>>> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
>>> ---
>>>   tools/firmware/hvmloader/e820.c | 94
>>> +++++++++++++++++++++++++++++++++++++++++
>>>   tools/firmware/hvmloader/e820.h |  6 +++
>>>   tools/firmware/hvmloader/util.c | 16 +++++++
>>>   tools/firmware/hvmloader/util.h |  2 +
>>>   4 files changed, 118 insertions(+)
>>>
>>> diff --git a/tools/firmware/hvmloader/e820.c
>>> b/tools/firmware/hvmloader/e820.c
>>> index 2e05e93..7f54eab 100644
>>> --- a/tools/firmware/hvmloader/e820.c
>>> +++ b/tools/firmware/hvmloader/e820.c
>>> @@ -68,12 +68,89 @@ void dump_e820_table(struct e820entry *e820,
>>> unsigned int nr)
>>>       }
>>>   }
>>
>> Please apply Xen coding style to all of this file - there are a lot of
>> errors.
>
> I asked previously what Xen coding style is, and I always try to
> follow some existing code but seems its not enough. Here I still can't
> find out what you're pointing, so please tell me exactly what I should
> change exactly.
>
> And why don't you guy generate a checkpatch.pl file?

Xen coding style can be found in the document CODING_STYLE in the source
root.

There have been various attempts at a checkpatch.pl, but we suffer from
inheriting different coding styles from different sources.  Even in Xen,
we mix between Xen and Linux.

~Andrew

Re: [RFC][v3][PATCH 1/6] xen:x86: record RMRR mappings

[Chen, Tiejun]

On 2014/8/18 17:51, Andrew Cooper wrote:
> On 18/08/14 08:45, Chen, Tiejun wrote:
>> On 2014/8/15 17:39, Andrew Cooper wrote:
>>> On 15/08/14 09:27, Tiejun Chen wrote:
>>>> We will expose RMRR mappings to VM so need to record all
>>>> RMRR mappings firstly.
>>>>
>>>> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
>>>> ---
>>>>    xen/arch/x86/e820.c                |  2 ++
>>>>    xen/drivers/passthrough/vtd/dmar.c | 14 ++++++++++++++
>>>>    xen/include/asm-x86/e820.h         |  3 +++
>>>>    3 files changed, 19 insertions(+)
>>>>
>>>> diff --git a/xen/arch/x86/e820.c b/xen/arch/x86/e820.c
>>>> index 55fe0d6..db44afd 100644
>>>> --- a/xen/arch/x86/e820.c
>>>> +++ b/xen/arch/x86/e820.c
>>>> @@ -34,6 +34,8 @@ static bool_t __initdata e820_verbose;
>>>>    boolean_param("e820-verbose", e820_verbose);
>>>>
>>>>    struct e820map e820;
>>>> +/* Used to record RMRR mapping. */
>>>> +rmrr_maps_t rmrr_maps;
>>>>
>>>>    /*
>>>>     * This function checks if the entire range <start,end> is mapped
>>>> with type.
>>>> diff --git a/xen/drivers/passthrough/vtd/dmar.c
>>>> b/xen/drivers/passthrough/vtd/dmar.c
>>>> index 1152c3a..24321e3 100644
>>>> --- a/xen/drivers/passthrough/vtd/dmar.c
>>>> +++ b/xen/drivers/passthrough/vtd/dmar.c
>>>> @@ -29,6 +29,7 @@
>>>>    #include <xen/pci.h>
>>>>    #include <xen/pci_regs.h>
>>>>    #include <asm/string.h>
>>>> +#include <asm/e820.h>
>>>>    #include "dmar.h"
>>>>    #include "iommu.h"
>>>>    #include "extern.h"
>>>> @@ -80,6 +81,18 @@ static int __init acpi_register_rmrr_unit(struct
>>>> acpi_rmrr_unit *rmrr)
>>>>        return 0;
>>>>    }
>>>>
>>>> +/* Record RMRR mapping to ready expose VM. */
>>>> +static int __init rmrr_maps_register(struct acpi_rmrr_unit *rmrr)
>>>> +{
>>>
>>> You absolutely need some protection against calling this function more
>>
>> Could you show this assumed scenario?
>>
>> IMO, this should never be happened since we always use a e820 table to
>> cover RMRR range/entries.
>
> There is, as far as I am aware, no upper bound to the number of RMRRs
> reported by the firmware.  rmrr_maps.map is an array of length E820_MAX,

As you know all RMRR can be listed in a e820 table so its not possible 
to be over 128.

> or 128.  If this function is called more than 128 times, you will start
> clobbering Xen data.

And this is just a static function here, we also don't call that with >128.

Anyway, I can add something to deliver warning message like this,

+
+    if ( rmrr_map.nr_map >= E820MAX )
+        printk(XENLOG_WARNING
+               "Overflow RMRR mappings! Failed to record RMRR 
mappings.\n");

>
> But as Jan pointed out in the other fork of this thread, this entire
> function is not needed, and can be removed.

I'm a little bit disagreed here. Please see my reply to Jan.

Thanks
Tiejun
>
> ~Andrew
>
>

Re: [RFC][v3][PATCH 1/6] xen:x86: record RMRR mappings

[Chen, Tiejun]

On 2014/8/18 17:57, Andrew Cooper wrote:
> On 18/08/14 08:42, Chen, Tiejun wrote:
>> On 2014/8/16 0:29, Jan Beulich wrote:
>>>>>> On 15.08.14 at 11:39, <andrew.cooper3@citrix.com> wrote:
>>>>> @@ -80,6 +81,18 @@ static int __init acpi_register_rmrr_unit(struct
>>>>> acpi_rmrr_unit *rmrr)
>>>>>        return 0;
>>>>>    }
>>>>>
>>>>> +/* Record RMRR mapping to ready expose VM. */
>>>>> +static int __init rmrr_maps_register(struct acpi_rmrr_unit *rmrr)
>>>>> +{
>>>>
>>>> You absolutely need some protection against calling this function more
>>>> than 128 times, or you need to do dynamic allocation of the storage.
>>>
>>> This together with ...
>>>
>>>>> --- a/xen/include/asm-x86/e820.h
>>>>> +++ b/xen/include/asm-x86/e820.h
>>>>> @@ -23,6 +23,8 @@ struct e820map {
>>>>>        struct e820entry map[E820MAX];
>>>>>    };
>>>>>
>>>>> +typedef struct e820map rmrr_maps_t;
>>>>
>>>> This type is a single map of RMRR regions, not multiple maps.
>>>> rmrr_map_t please.
>>>
>>> ... this once again stresses what I stated previously: Piggybacking
>>> on the E820 handling here is just the wrong approach. There's
>>> really no correlation with E820 other than us wanting to use the
>>> gathered information for (among other things) adjusting the guest
>>> E820 table. But that doesn't in any way require any re-use of
>>> non-suitable data structures.
>>
>> Why are you saying this is not suitable?
>>
>> We need a structure to represent a RMRR entry including three fields,
>> start, size and type, and especially, essentially RMRR entry belongs
>> to e820 table as one entry.
>
> Not in Xen.  Only as reported to guests, in which case an e820-like
> structure is most appropriate.
>
>>
>>>
>>> In fact I don't see the need for this first patch anyway, as RMRRs
>>> are already being put on a linked list as they get found. I.e. the
>>
>> Yes, that list, acpi_rmrr_unit, can be exposed here. But before you
>> copy to guest, don't you need to grab those fields from that list then
>> convert them as a suitable structure (mostly this is still same as
>> e820entry) to be copied into a buffer?
>
> Yes, but the hypercall handler can do this which avoids all need to
> store an intermediate representation in Xen.
>
> list_for_each_entry(rmrr, &acpi_rmrr_units, list)
> {
>      e820entry e;
>
>      e.start = ...
>
>      copy_to_guest_offset(...
> }
>

This is just what I mean. You always need to grab all info from 
acpi_rmrr_units to covert as a e820entry to copy.

So I guess you guys mean we should avoid duplicating RMRR many times in 
global. acpi_rmrr_units is fine enough, so I'd like to do as you expect.

Thanks
Tiejun

> But with appropriate error checking.
>
> ~Andrew
>
>

Re: [RFC][v3][PATCH 4/6] xen:x86: add XENMEM_reserved_device_memory_map to expose RMRR

[Andrew Cooper]

On 18/08/14 09:00, Chen, Tiejun wrote:
> On 2014/8/15 20:15, Andrew Cooper wrote:
>> On 15/08/14 09:27, Tiejun Chen wrote:
>>> We should expose RMRR mapping to libxc, then setup_guest() can
>>> check if current MMIO is not covered by any RMRR mapping.
>>>
>>> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
>>> ---
>>>   xen/arch/x86/mm.c | 32 ++++++++++++++++++++++++++++++++
>>>   1 file changed, 32 insertions(+)
>>>
>>> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
>>> index d23cb3f..fb6e92f 100644
>>> --- a/xen/arch/x86/mm.c
>>> +++ b/xen/arch/x86/mm.c
>>> @@ -4769,6 +4769,38 @@ long arch_memory_op(unsigned long cmd,
>>> XEN_GUEST_HANDLE_PARAM(void) arg)
>>>           return 0;
>>>       }
>>>
>>> +    case XENMEM_reserved_device_memory_map:
>>> +    {
>>> +        struct xen_memory_map map;
>>> +        XEN_GUEST_HANDLE(e820entry_t) buffer;
>>> +        XEN_GUEST_HANDLE_PARAM(e820entry_t) buffer_param;
>>> +        unsigned int i;
>>> +
>>> +        if ( copy_from_guest(&map, arg, 1) )
>>> +            return -EFAULT;
>>
>> This hypercall implementation is looking somewhat more plausible, but
>> still has some issues.
>>
>>> +        if ( map.nr_entries < rmrr_maps.nr_map + 1 )
>>> +            return -EINVAL;
>>
>> This causes a fencepost error, does it not?
>
> map.nr_entries = E820MAX, and obviously rmrr_maps.nr_map should be
> smaller than far E820MAX. So what is your problem?
>
> Here I have a reference to XENMEM_machine_memory_map.

Looks like XENMEM_machine_memory_map is also wrong.

Consider the case where the caller provides a buffer of exactly the
correct number of entries.  In that case, the hypercall would fail with
-EINVAL despite being able to complete successfully.

>
>>
>> Furthermore, the useful error would be to return -ENOBUFS and fill
>> arg.nr_entries with the rmrr_maps.nr_map so the caller can allocate an
>> appropriately sized buffer.
>>
>>
>> It is also very common with hypercalls like this to have allow a null
>> guest handle as an explicit request for size.
>
> Looks you like to issue twice time with a hypercall to finish, but
> what's wrong with my way?
>
> Again, here I have a reference to XENMEM_machine_memory_map.

Some lessons have been learnt since some of the older hypercall handlers
were written.  Specifically, there is no way to gauge the required
buffer size if a buffer too small is provided.

~Andrew

Re: [RFC][v3][PATCH 1/6] xen:x86: record RMRR mappings

[Jan Beulich]

>>>> Andrew Cooper <andrew.cooper3@citrix.com> 08/18/14 11:57 AM >>>
>On 18/08/14 08:42, Chen, Tiejun wrote:
>> On 2014/8/16 0:29, Jan Beulich wrote:
>>>>>> On 15.08.14 at 11:39, <andrew.cooper3@citrix.com> wrote:
>>>>> --- a/xen/include/asm-x86/e820.h
>>>>> +++ b/xen/include/asm-x86/e820.h
>>>>> @@ -23,6 +23,8 @@ struct e820map {
>>>>>       struct e820entry map[E820MAX];
>>>>>   };
>>>>>
>>>>> +typedef struct e820map rmrr_maps_t;
>>>>
>>>> This type is a single map of RMRR regions, not multiple maps.
>>>> rmrr_map_t please.
>>>
>>> ... this once again stresses what I stated previously: Piggybacking
>>> on the E820 handling here is just the wrong approach. There's
>>> really no correlation with E820 other than us wanting to use the
>>> gathered information for (among other things) adjusting the guest
>>> E820 table. But that doesn't in any way require any re-use of
>>> non-suitable data structures.
>>
>> Why are you saying this is not suitable?
>>
>> We need a structure to represent a RMRR entry including three fields,
>> start, size and type, and especially, essentially RMRR entry belongs
>> to e820 table as one entry.
>
>Not in Xen.  Only as reported to guests, in which case an e820-like
>structure is most appropriate.

E280-like yes, but ...

>>> In fact I don't see the need for this first patch anyway, as RMRRs
>>> are already being put on a linked list as they get found. I.e. the
>>
>> Yes, that list, acpi_rmrr_unit, can be exposed here. But before you
>> copy to guest, don't you need to grab those fields from that list then
>> convert them as a suitable structure (mostly this is still same as
>> e820entry) to be copied into a buffer?
>
>Yes, but the hypercall handler can do this which avoids all need to
>store an intermediate representation in Xen.
>
>list_for_each_entry(rmrr, &acpi_rmrr_units, list)
>{
    >e820entry e;
>
    >e.start = ...
>
    >copy_to_guest_offset(...
>}

... as said before, I don't think using the E820 structure as-is is the right
approach: Neither do we need byte-granular fields, nor do we need a type
here.

Jan

Re: [RFC][v3][PATCH 1/6] xen:x86: record RMRR mappings

[Jan Beulich]

>>> "Chen, Tiejun" <tiejun.chen@intel.com> 08/18/14 12:01 PM >>>
>On 2014/8/18 17:51, Andrew Cooper wrote:
>> There is, as far as I am aware, no upper bound to the number of RMRRs
>> reported by the firmware.  rmrr_maps.map is an array of length E820_MAX,
>
>As you know all RMRR can be listed in a e820 table so its not possible 
>to be over 128.

No matter that this thread ought to be dead by now anyway - please stop
making wrong assumptions in code you add to the hypervisor. I.e. here -
since when is this? Linux has specifically got code added to support >128
entry E820 maps, and I assume sooner or later Xen will need to get this
too. With UEFI, it is already an uncertain business to consolidate the UEFI
memory map into an E820 one (collapsing compatible successive UEFI
memory blocks into one usually reduces the entry count significantly, but
there's really no guarantee for this to be the case now and forever).

Jan

Re: [RFC][v3][PATCH 1/6] xen:x86: record RMRR mappings

[Chen, Tiejun]

On 2014/8/18 20:31, Jan Beulich wrote:
>>>>> Andrew Cooper <andrew.cooper3@citrix.com> 08/18/14 11:57 AM >>>
>> On 18/08/14 08:42, Chen, Tiejun wrote:
>>> On 2014/8/16 0:29, Jan Beulich wrote:
>>>>>>> On 15.08.14 at 11:39, <andrew.cooper3@citrix.com> wrote:
>>>>>> --- a/xen/include/asm-x86/e820.h
>>>>>> +++ b/xen/include/asm-x86/e820.h
>>>>>> @@ -23,6 +23,8 @@ struct e820map {
>>>>>>        struct e820entry map[E820MAX];
>>>>>>    };
>>>>>>
>>>>>> +typedef struct e820map rmrr_maps_t;
>>>>>
>>>>> This type is a single map of RMRR regions, not multiple maps.
>>>>> rmrr_map_t please.
>>>>
>>>> ... this once again stresses what I stated previously: Piggybacking
>>>> on the E820 handling here is just the wrong approach. There's
>>>> really no correlation with E820 other than us wanting to use the
>>>> gathered information for (among other things) adjusting the guest
>>>> E820 table. But that doesn't in any way require any re-use of
>>>> non-suitable data structures.
>>>
>>> Why are you saying this is not suitable?
>>>
>>> We need a structure to represent a RMRR entry including three fields,
>>> start, size and type, and especially, essentially RMRR entry belongs
>>> to e820 table as one entry.
>>
>> Not in Xen.  Only as reported to guests, in which case an e820-like
>> structure is most appropriate.
>
> E280-like yes, but ...
>
>>>> In fact I don't see the need for this first patch anyway, as RMRRs
>>>> are already being put on a linked list as they get found. I.e. the
>>>
>>> Yes, that list, acpi_rmrr_unit, can be exposed here. But before you
>>> copy to guest, don't you need to grab those fields from that list then
>>> convert them as a suitable structure (mostly this is still same as
>>> 	) to be copied into a buffer?
>>
>> Yes, but the hypercall handler can do this which avoids all need to
>> store an intermediate representation in Xen.
>>
>> list_for_each_entry(rmrr, &acpi_rmrr_units, list)
>> {
>      >e820entry e;
>>
>      >e.start = ...
>>
>      >copy_to_guest_offset(...
>> }
>
> ... as said before, I don't think using the E820 structure as-is is the right
> approach: Neither do we need byte-granular fields, nor do we need a type
> here.
>

Please don't say simply that e820entry is not suitable, what's your 
preferred structure here?

Looks you are saying something like,

struct __packed rmrr_entry {
     uint64_t addr;
     uint64_t size;
};

but compare that to the existing e820entry,

struct __packed e820entry {
     uint64_t addr;
     uint64_t size;
     uint32_t type;
};

Anyway, please show me your ideal structure then I'd like to follow-up 
that since it's no big deal.

Thanks
Tiejun

Re: [RFC][v3][PATCH 1/6] xen:x86: record RMRR mappings

[Chen, Tiejun]

On 2014/8/19 10:14, Chen, Tiejun wrote:
> On 2014/8/18 20:31, Jan Beulich wrote:
>>>>>> Andrew Cooper <andrew.cooper3@citrix.com> 08/18/14 11:57 AM >>>
>>> On 18/08/14 08:42, Chen, Tiejun wrote:
>>>> On 2014/8/16 0:29, Jan Beulich wrote:
>>>>>>>> On 15.08.14 at 11:39, <andrew.cooper3@citrix.com> wrote:
>>>>>>> --- a/xen/include/asm-x86/e820.h
>>>>>>> +++ b/xen/include/asm-x86/e820.h
>>>>>>> @@ -23,6 +23,8 @@ struct e820map {
>>>>>>>        struct e820entry map[E820MAX];
>>>>>>>    };
>>>>>>>
>>>>>>> +typedef struct e820map rmrr_maps_t;
>>>>>>
>>>>>> This type is a single map of RMRR regions, not multiple maps.
>>>>>> rmrr_map_t please.
>>>>>
>>>>> ... this once again stresses what I stated previously: Piggybacking
>>>>> on the E820 handling here is just the wrong approach. There's
>>>>> really no correlation with E820 other than us wanting to use the
>>>>> gathered information for (among other things) adjusting the guest
>>>>> E820 table. But that doesn't in any way require any re-use of
>>>>> non-suitable data structures.
>>>>
>>>> Why are you saying this is not suitable?
>>>>
>>>> We need a structure to represent a RMRR entry including three fields,
>>>> start, size and type, and especially, essentially RMRR entry belongs
>>>> to e820 table as one entry.
>>>
>>> Not in Xen.  Only as reported to guests, in which case an e820-like
>>> structure is most appropriate.
>>
>> E280-like yes, but ...
>>
>>>>> In fact I don't see the need for this first patch anyway, as RMRRs
>>>>> are already being put on a linked list as they get found. I.e. the
>>>>
>>>> Yes, that list, acpi_rmrr_unit, can be exposed here. But before you
>>>> copy to guest, don't you need to grab those fields from that list then
>>>> convert them as a suitable structure (mostly this is still same as
>>>>     ) to be copied into a buffer?
>>>
>>> Yes, but the hypercall handler can do this which avoids all need to
>>> store an intermediate representation in Xen.
>>>
>>> list_for_each_entry(rmrr, &acpi_rmrr_units, list)
>>> {
>>      >e820entry e;
>>>
>>      >e.start = ...
>>>
>>      >copy_to_guest_offset(...
>>> }
>>
>> ... as said before, I don't think using the E820 structure as-is is
>> the right
>> approach: Neither do we need byte-granular fields, nor do we need a type
>> here.
>>
>
> Please don't say simply that e820entry is not suitable, what's your
> preferred structure here?
>
> Looks you are saying something like,
>
> struct __packed rmrr_entry {
>      uint64_t addr;
>      uint64_t size;
> };
>
> but compare that to the existing e820entry,
>
> struct __packed e820entry {
>      uint64_t addr;
>      uint64_t size;
>      uint32_t type;
> };
>

Another concern is that we always use xen_memory_map for the hypercall,

struct xen_memory_map {
     /*
      * On call the number of entries which can be stored in buffer. On
      * return the number of entries which have been stored in
      * buffer.
      */
     unsigned int nr_entries;

     /*
      * Entries in the buffer are in the same format as returned by the
      * BIOS INT 0x15 EAX=0xE820 call.
      */
     XEN_GUEST_HANDLE(void) buffer;
};

As it comments above, theoretical e820 is expected in buffer.

Thanks
Tiejun

> Anyway, please show me your ideal structure then I'd like to follow-up
> that since it's no big deal.
>
> Thanks
> Tiejun
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>
>

Re: [RFC][v3][PATCH 1/6] xen:x86: record RMRR mappings

[Jan Beulich]

>>> "Chen, Tiejun" <tiejun.chen@intel.com> 08/19/14 4:28 AM >>>
>On 2014/8/19 10:14, Chen, Tiejun wrote:
> Please don't say simply that e820entry is not suitable, what's your
> preferred structure here?
>
> Looks you are saying something like,
>
> struct __packed rmrr_entry {
>      uint64_t addr;
>      uint64_t size;
> };
>
> but compare that to the existing e820entry,
>
> struct __packed e820entry {
>      uint64_t addr;
>      uint64_t size;
>      uint32_t type;
> };

struct xen_reserved_device_memory {
    xen_pfn_t pfn;
    xen_ulong_t count;
};

>Another concern is that we always use xen_memory_map for the hypercall,
>
>struct xen_memory_map {
     >/*
      >* On call the number of entries which can be stored in buffer. On
      >* return the number of entries which have been stored in
      >* buffer.
      >*/
     >unsigned int nr_entries;
>
     >/*
      >* Entries in the buffer are in the same format as returned by the
      >* BIOS INT 0x15 EAX=0xE820 call.
      >*/
     >XEN_GUEST_HANDLE(void) buffer;
>};
>
>As it comments above, theoretical e820 is expected in buffer.

That's what your patch currently does - nothing keeps you from either altering
the comment or defining a new structure (and then right away with a properly
typed handle).

Jan