[v4][PATCH 0/9] xen: reserve RMRR to avoid conflicting MMIO/RAM

[v4][PATCH 0/9] xen: reserve RMRR to avoid conflicting MMIO/RAM

[Tiejun Chen]

v4:

* Drop the original patch #1. Instead, we use acpi_rmrr_units to get
  rmrr info directly.
* Refine the hypercall definition to make sure we can use it safely.
* Introduce introduce nr_reserved_device_memory_map in hvm_info_table,
  then we can avoid issue unnecessary hypercall, even we can know
  current RMRR entries to issue hypercall one time.
* Cleanup and rebase

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 (9):
      xen:vtd:rmrr: export acpi_rmrr_units
      xen:x86: define a new hypercall to get RMRR mappings
      tools:libxc: introduce hypercall for xc_reserved_device_memory_map
      tools:libxc: check if mmio BAR is out of RMRR mappings
      hvm_info_table: introduce nr_reserved_device_memory_map
      xen:x86:: support xc_reserved_device_memory_map in compat case
      tools:firmware:hvmloader: introduce hypercall for xc_reserved_device_memory_map
      tools:firmware:hvmloader: check to reserve RMRR mappings in e820
      xen:vtd: make USB RMRR mapping safe

 tools/firmware/hvmloader/e820.c         | 101 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 tools/firmware/hvmloader/util.c         |  22 ++++++++++++++++++++++
 tools/firmware/hvmloader/util.h         |  11 +++++++++++
 tools/libxc/xc_domain.c                 |  29 +++++++++++++++++++++++++++++
 tools/libxc/xc_hvm_build_x86.c          |  81 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 tools/libxc/xenctrl.h                   |   4 ++++
 xen/arch/x86/mm.c                       |  71 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 xen/arch/x86/x86_64/compat/mm.c         |  62 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 xen/drivers/passthrough/vtd/dmar.h      |  16 +---------------
 xen/drivers/passthrough/vtd/iommu.c     |   8 --------
 xen/drivers/passthrough/vtd/iommu.h     |   1 -
 xen/include/asm-x86/acpi.h              |  17 +++++++++++++++++
 xen/include/public/hvm/hvm_info_table.h |   3 +++
 xen/include/public/memory.h             |  37 ++++++++++++++++++++++++++++++++++++-
 14 files changed, 436 insertions(+), 27 deletions(-)

Thanks
Tiejun

[v4][PATCH 1/9] xen:vtd:rmrr: export acpi_rmrr_units

[Tiejun Chen]

We need to expose acpi_rmrr_units to make sure we can use
that somewhere else.

Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
---
 xen/drivers/passthrough/vtd/dmar.h  | 16 +---------------
 xen/drivers/passthrough/vtd/iommu.h |  1 -
 xen/include/asm-x86/acpi.h          | 17 +++++++++++++++++
 3 files changed, 18 insertions(+), 16 deletions(-)

diff --git a/xen/drivers/passthrough/vtd/dmar.h b/xen/drivers/passthrough/vtd/dmar.h
index af1feef..1839ca2 100644
--- a/xen/drivers/passthrough/vtd/dmar.h
+++ b/xen/drivers/passthrough/vtd/dmar.h
@@ -24,6 +24,7 @@
 #include <xen/list.h>
 #include <xen/iommu.h>
 #include <xen/kexec.h>
+#include <asm/acpi.h>
 
 /* This one is for interrupt remapping */
 struct acpi_ioapic_unit {
@@ -52,12 +53,6 @@ struct acpi_hpet_unit {
     };
 };
 
-struct dmar_scope {
-    DECLARE_BITMAP(buses, 256);         /* buses owned by this unit */
-    u16    *devices;                    /* devices owned by this unit */
-    int    devices_cnt;
-};
-
 struct acpi_drhd_unit {
     struct dmar_scope scope;
     struct list_head list;
@@ -69,15 +64,6 @@ struct acpi_drhd_unit {
     struct list_head hpet_list;
 };
 
-struct acpi_rmrr_unit {
-    struct dmar_scope scope;
-    struct list_head list;
-    u64    base_address;
-    u64    end_address;
-    u16    segment;
-    u8     allow_all:1;
-};
-
 struct acpi_atsr_unit {
     struct dmar_scope scope;
     struct list_head list;
diff --git a/xen/drivers/passthrough/vtd/iommu.h b/xen/drivers/passthrough/vtd/iommu.h
index 6b2cf1a..741e62b 100644
--- a/xen/drivers/passthrough/vtd/iommu.h
+++ b/xen/drivers/passthrough/vtd/iommu.h
@@ -482,7 +482,6 @@ struct qinval_entry {
 #define MAX_IOMMU_REGS 0xc0
 
 extern struct list_head acpi_drhd_units;
-extern struct list_head acpi_rmrr_units;
 extern struct list_head acpi_ioapic_units;
 
 struct qi_ctrl {
diff --git a/xen/include/asm-x86/acpi.h b/xen/include/asm-x86/acpi.h
index 5e85b38..b3c9e90 100644
--- a/xen/include/asm-x86/acpi.h
+++ b/xen/include/asm-x86/acpi.h
@@ -28,6 +28,7 @@
 #include <acpi/pdc_intel.h>
 #include <acpi/acconfig.h>
 #include <acpi/actbl.h>
+#include <xen/list.h>
 
 #define COMPILER_DEPENDENT_INT64   long long
 #define COMPILER_DEPENDENT_UINT64  unsigned long long
@@ -141,6 +142,21 @@ struct acpi_sleep_info {
     bool_t sleep_extended;
 };
 
+struct dmar_scope {
+    DECLARE_BITMAP(buses, 256);         /* buses owned by this unit */
+    u16    *devices;                    /* devices owned by this unit */
+    int    devices_cnt;
+};
+
+struct acpi_rmrr_unit {
+    struct dmar_scope scope;
+    struct list_head list;
+    u64    base_address;
+    u64    end_address;
+    u16    segment;
+    u8     allow_all:1;
+};
+
 #endif /* CONFIG_ACPI_SLEEP */
 
 #define MAX_MADT_ENTRIES	MAX(256, 2 * NR_CPUS)
@@ -164,4 +180,5 @@ void hvm_acpi_sleep_button(struct domain *d);
 void save_rest_processor_state(void);
 void restore_rest_processor_state(void);
 
+extern struct list_head acpi_rmrr_units;
 #endif /*__X86_ASM_ACPI_H*/
-- 
1.9.1

[v4][PATCH 2/9] xen:x86: define 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/mm.c           | 71 +++++++++++++++++++++++++++++++++++++++++++++
 xen/include/public/memory.h | 37 ++++++++++++++++++++++-
 2 files changed, 107 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index d23cb3f..e0d6650 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -123,6 +123,7 @@
 #include <asm/setup.h>
 #include <asm/fixmap.h>
 #include <asm/pci.h>
+#include <asm/acpi.h>
 
 /* Mapping of the fixmap space needed early. */
 l1_pgentry_t __attribute__ ((__section__ (".bss.page_aligned")))
@@ -4842,6 +4843,76 @@ long arch_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
         return rc;
     }
 
+    case XENMEM_reserved_device_memory_map:
+    {
+        struct xen_reserved_device_memory_map map;
+        XEN_GUEST_HANDLE(xen_reserved_device_memory_t) buffer;
+        XEN_GUEST_HANDLE_PARAM(xen_reserved_device_memory_t) buffer_param;
+        unsigned int i = 0;
+        static unsigned int nr_entries = 0;
+        static struct xen_reserved_device_memory *rmrr_map;
+        struct acpi_rmrr_unit *rmrr;
+
+        if ( copy_from_guest(&map, arg, 1) )
+            return -EFAULT;
+
+        if ( !nr_entries )
+            /* Currently we just need to cover RMRR. */
+            list_for_each_entry( rmrr, &acpi_rmrr_units, list )
+                nr_entries++;
+
+        if ( !nr_entries )
+                return -ENOENT;
+        else
+        {
+            if ( rmrr_map == NULL )
+            {
+                rmrr_map = xmalloc_array(xen_reserved_device_memory_t,
+                                         nr_entries);
+                if ( rmrr_map == NULL )
+                {
+                    return -ENOMEM;
+                }
+
+                list_for_each_entry( rmrr, &acpi_rmrr_units, list )
+                {
+                    rmrr_map[i].pfn = rmrr->base_address >> PAGE_SHIFT;
+                    rmrr_map[i].count = PAGE_ALIGN(rmrr->end_address -
+                                                   rmrr->base_address) /
+                                                   PAGE_SIZE;
+                    i++;
+                }
+            }
+        }
+
+        if ( map.nr_entries < nr_entries )
+        {
+            map.nr_entries =  nr_entries;
+            if ( copy_to_guest(arg, &map, 1) )
+                return -EFAULT;
+            return -ENOBUFS;
+        }
+
+        map.nr_entries =  nr_entries;
+        buffer_param = guest_handle_cast(map.buffer,
+                                         xen_reserved_device_memory_t);
+        buffer = guest_handle_from_param(buffer_param,
+                                         xen_reserved_device_memory_t);
+        if ( !guest_handle_okay(buffer, map.nr_entries) )
+            return -EFAULT;
+
+        for ( i = 0; i < map.nr_entries; ++i )
+        {
+            if ( copy_to_guest_offset(buffer, i, rmrr_map + i, 1) )
+                return -EFAULT;
+        }
+
+        if ( copy_to_guest(arg, &map, 1) )
+                return -EFAULT;
+
+        return 0;
+    }
+
     default:
         return subarch_memory_op(cmd, arg);
     }
diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h
index 2c57aa0..8481843 100644
--- a/xen/include/public/memory.h
+++ b/xen/include/public/memory.h
@@ -523,7 +523,42 @@ 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
+struct xen_reserved_device_memory {
+    /* PFN of the current mapping of the page. */
+    xen_pfn_t pfn;
+    /* Number of the current mapping pages. */
+    xen_ulong_t count;
+};
+typedef struct xen_reserved_device_memory xen_reserved_device_memory_t;
+DEFINE_XEN_GUEST_HANDLE(xen_reserved_device_memory_t);
+
+struct xen_reserved_device_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
+     * xen_reserved_device_memory.
+     */
+    XEN_GUEST_HANDLE(void) buffer;
+};
+typedef struct xen_reserved_device_memory_map xen_reserved_device_memory_map_t;
+DEFINE_XEN_GUEST_HANDLE(xen_reserved_device_memory_map_t);
+
+/* Next available subop number is 27 */
 
 #endif /* __XEN_PUBLIC_MEMORY_H__ */
 
-- 
1.9.1

[v4][PATCH 3/9] tools:libxc: introduce hypercall for xc_reserved_device_memory_map

[Tiejun Chen]

We will introduce that hypercall xc_reserved_device_memory_map
to libxc.

Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
---
 tools/libxc/xc_domain.c | 29 +++++++++++++++++++++++++++++
 tools/libxc/xenctrl.h   |  4 ++++
 2 files changed, 33 insertions(+)

diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c
index c67ac9a..08dc16f 100644
--- a/tools/libxc/xc_domain.c
+++ b/tools/libxc/xc_domain.c
@@ -649,6 +649,35 @@ int xc_domain_set_memory_map(xc_interface *xch,
 
     return rc;
 }
+
+int xc_reserved_device_memory_map(xc_interface *xch,
+                                  struct xen_reserved_device_memory entries[],
+                                  uint32_t max_entries)
+{
+    int rc;
+    struct xen_reserved_device_memory_map memmap = {
+        .nr_entries = max_entries
+    };
+    DECLARE_HYPERCALL_BOUNCE(entries,
+                             sizeof(struct xen_reserved_device_memory) *
+                             max_entries, XC_HYPERCALL_BUFFER_BOUNCE_OUT);
+
+    if ( xc_hypercall_bounce_pre(xch, entries) )
+        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);
+
+    if ( errno == ENOBUFS )
+        return memmap.nr_entries;
+
+    return rc ? -errno : memmap.nr_entries;
+}
+
 int xc_get_machine_memory_map(xc_interface *xch,
                               struct e820entry entries[],
                               uint32_t max_entries)
diff --git a/tools/libxc/xenctrl.h b/tools/libxc/xenctrl.h
index 1c5d0db..1cc7852 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_reserved_device_memory_map(xc_interface *xch,
+                                  struct xen_reserved_device_memory entries[],
+                                  uint32_t max_entries);
 #endif
 int xc_domain_set_time_offset(xc_interface *xch,
                               uint32_t domid,
-- 
1.9.1

[v4][PATCH 4/9] tools:libxc: check if mmio BAR is out of RMRR mappings

[Tiejun Chen]

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

Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
---
 tools/libxc/xc_hvm_build_x86.c | 69 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 69 insertions(+)

diff --git a/tools/libxc/xc_hvm_build_x86.c b/tools/libxc/xc_hvm_build_x86.c
index c81a25b..20e4e0c 100644
--- a/tools/libxc/xc_hvm_build_x86.c
+++ b/tools/libxc/xc_hvm_build_x86.c
@@ -239,6 +239,71 @@ static int check_mmio_hole(uint64_t start, uint64_t memsize,
         return 1;
 }
 
+/*
+ * Check whether there exists mmio overplap with the specified RMRR
+ * memory range.
+ */
+static int check_rmrr_overlap(xc_interface *xch, uint64_t mmio_start,
+                              uint64_t mmio_size)
+{
+    struct xen_reserved_device_memory *map = NULL;
+    uint64_t rmrr_start = 0, rmrr_end = 0;
+    unsigned int i = 0;
+    int rc = 0;
+
+    /* We should check if mmio range is out of RMRR mapping.
+     *
+     * Assume we have one entry if not enough we'll expand.
+     */
+    if ( (map = malloc(1 * sizeof(xen_reserved_device_memory_t))) == NULL )
+    {
+        PERROR("Could not allocate memory for map.");
+        return -1;
+    }
+    rc = xc_reserved_device_memory_map(xch, map, 1);
+    if ( rc < 0 )
+    {
+        /* RMRR doesn't exist. */
+        if ( rc == -ENOENT )
+            rc = 0;
+        else
+            PERROR("Could not get RMRR info on domain");
+    } else if ( rc > 0 ) {
+        free(map);
+        /* Now we need more space to map all RMRR mappings.
+         */
+        if ( (map = malloc(rc * sizeof(xen_reserved_device_memory_t))) == NULL )
+        {
+            PERROR("Could not allocate memory for map.");
+            return -1;
+        }
+        rc = xc_reserved_device_memory_map(xch, map, rc);
+        if ( rc < 0 )
+        {
+            PERROR("Could not get RMRR info on domain");
+            free(map);
+            return rc;
+        }
+    }
+
+    for ( i = 0; i < rc; i++ )
+    {
+        rmrr_start = map[i].pfn << PAGE_SHIFT;
+        rmrr_end = rmrr_start + map[i].count * PAGE_SIZE;
+        if ( check_mmio_hole(rmrr_start, map[i].count * PAGE_SIZE,
+                             mmio_start, mmio_size) )
+        {
+            PERROR("MMIO: [%lx]<->[%lx] overlap RMRR [%lx]<->[%lx]\n",
+                   mmio_start, (mmio_start + mmio_size), rmrr_start, rmrr_end);
+            free(map);
+            return -1;
+        }
+    }
+
+    free(map);
+    return rc;
+}
+
 static int setup_guest(xc_interface *xch,
                        uint32_t dom, struct xc_hvm_build_args *args,
                        char *image, unsigned long image_size)
@@ -300,6 +365,10 @@ static int setup_guest(xc_interface *xch,
         goto error_out;
     }
 
+    rc = check_rmrr_overlap(xch, mmio_start, mmio_start);
+    if ( rc < 0 )
+        goto error_out;
+
     for ( i = 0; i < nr_pages; i++ )
         page_array[i] = i;
     for ( i = mmio_start >> PAGE_SHIFT; i < nr_pages; i++ )
-- 
1.9.1

[v4][PATCH 5/9] hvm_info_table: introduce nr_reserved_device_memory_map

[Tiejun Chen]

libxc can expose how many reserved device memory entries
hvmloader should get. And '0' means that doesn't exist so
we can skip this check.

Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
---
 tools/libxc/xc_hvm_build_x86.c          | 12 ++++++++++--
 xen/include/public/hvm/hvm_info_table.h |  3 +++
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/tools/libxc/xc_hvm_build_x86.c b/tools/libxc/xc_hvm_build_x86.c
index 20e4e0c..093b089 100644
--- a/tools/libxc/xc_hvm_build_x86.c
+++ b/tools/libxc/xc_hvm_build_x86.c
@@ -89,7 +89,8 @@ static int modules_init(struct xc_hvm_build_args *args,
 }
 
 static void build_hvm_info(void *hvm_info_page, uint64_t mem_size,
-                           uint64_t mmio_start, uint64_t mmio_size)
+                           uint64_t mmio_start, uint64_t mmio_size,
+                           unsigned int num)
 {
     struct hvm_info_table *hvm_info = (struct hvm_info_table *)
         (((unsigned char *)hvm_info_page) + HVM_INFO_OFFSET);
@@ -119,6 +120,9 @@ static void build_hvm_info(void *hvm_info_page, uint64_t mem_size,
     hvm_info->high_mem_pgend = highmem_end >> PAGE_SHIFT;
     hvm_info->reserved_mem_pgstart = ioreq_server_pfn(0);
 
+    /* Reserved device memory number. */
+    hvm_info->nr_reserved_device_memory_map = num;
+
     /* Finish with the checksum. */
     for ( i = 0, sum = 0; i < hvm_info->length; i++ )
         sum += ((uint8_t *)hvm_info)[i];
@@ -327,6 +331,7 @@ 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];
+    unsigned int num_reserved = 0;
 
     if ( nr_pages > target_pages )
         pod_mode = XENMEMF_populate_on_demand;
@@ -368,6 +373,9 @@ static int setup_guest(xc_interface *xch,
     rc = check_rmrr_overlap(xch, mmio_start, mmio_start);
     if ( rc < 0 )
         goto error_out;
+    /* Always return entries number. */
+    else
+        num_reserved = rc;
 
     for ( i = 0; i < nr_pages; i++ )
         page_array[i] = i;
@@ -538,7 +546,7 @@ static int setup_guest(xc_interface *xch,
               xch, dom, PAGE_SIZE, PROT_READ | PROT_WRITE,
               HVM_INFO_PFN)) == NULL )
         goto error_out;
-    build_hvm_info(hvm_info_page, v_end, mmio_start, mmio_size);
+    build_hvm_info(hvm_info_page, v_end, mmio_start, mmio_size, num_reserved);
     munmap(hvm_info_page, PAGE_SIZE);
 
     /* Allocate and clear special pages. */
diff --git a/xen/include/public/hvm/hvm_info_table.h b/xen/include/public/hvm/hvm_info_table.h
index 36085fa..52c2b80 100644
--- a/xen/include/public/hvm/hvm_info_table.h
+++ b/xen/include/public/hvm/hvm_info_table.h
@@ -67,6 +67,9 @@ struct hvm_info_table {
 
     /* Bitmap of which CPUs are online at boot time. */
     uint8_t     vcpu_online[(HVM_MAX_VCPUS + 7)/8];
+
+    /* How many reserved device memory does this domain have? */
+    uint32_t    nr_reserved_device_memory_map;
 };
 
 #endif /* __XEN_PUBLIC_HVM_HVM_INFO_TABLE_H__ */
-- 
1.9.1

[v4][PATCH 6/9] xen:x86:: support xc_reserved_device_memory_map in compat case

[Tiejun Chen]

We also need to make sure xc_reserved_device_memory_map can
work in compat case.

Mostly refer to that complete xc_reserved_device_memory_map
hypercall definition, just work with struct
compat_reserved_device_memory_map and struct
compat_reserved_device_memory.

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

diff --git a/xen/arch/x86/x86_64/compat/mm.c b/xen/arch/x86/x86_64/compat/mm.c
index 69c6195..887a778 100644
--- a/xen/arch/x86/x86_64/compat/mm.c
+++ b/xen/arch/x86/x86_64/compat/mm.c
@@ -215,6 +215,68 @@ int compat_arch_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
         break;
     }
 
+    case XENMEM_reserved_device_memory_map:
+    {
+        struct compat_reserved_device_memory_map map;
+        unsigned int i = 0;
+        static unsigned int nr_entries = 0;
+        static struct compat_reserved_device_memory *rmrr_map;
+        struct acpi_rmrr_unit *rmrr;
+
+        if ( copy_from_guest(&map, arg, 1) )
+            return -EFAULT;
+
+        if ( !nr_entries )
+            /* Currently we just need to cover RMRR. */
+            list_for_each_entry( rmrr, &acpi_rmrr_units, list )
+                nr_entries++;
+
+        if ( !nr_entries )
+            return -ENOENT;
+        else
+        {
+            if ( rmrr_map == NULL )
+            {
+                rmrr_map = xmalloc_array(compat_reserved_device_memory_t,
+                                         nr_entries);
+                if ( rmrr_map == NULL )
+                {
+                    return -ENOMEM;
+                }
+
+                list_for_each_entry( rmrr, &acpi_rmrr_units, list )
+                {
+                    rmrr_map[i].pfn = rmrr->base_address >> PAGE_SHIFT;
+                    rmrr_map[i].count = PAGE_ALIGN(rmrr->end_address -
+                                                   rmrr->base_address) /
+                                                   PAGE_SIZE;
+                    i++;
+                }
+            }
+        }
+
+        if ( map.nr_entries < nr_entries )
+        {
+            map.nr_entries =  nr_entries;
+            if ( copy_to_guest(arg, &map, 1) )
+                return -EFAULT;
+            return -ENOBUFS;
+        }
+
+        map.nr_entries = nr_entries;
+
+        for ( i = 0; i < map.nr_entries; ++i )
+        {
+            if ( copy_to_compat_offset(map.buffer, i, rmrr_map + i, 1) )
+                return -EFAULT;
+        }
+
+        if ( copy_to_guest(arg, &map, 1) )
+            return -EFAULT;
+
+        break;
+    }
+
     default:
         rc = -ENOSYS;
         break;
-- 
1.9.1

[v4][PATCH 7/9] tools:firmware:hvmloader: introduce hypercall for xc_reserved_device_memory_map

[Tiejun Chen]

We will introduce that hypercall xc_reserved_device_memory_map
to hvmloader.

Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
---
 tools/firmware/hvmloader/util.c | 22 ++++++++++++++++++++++
 tools/firmware/hvmloader/util.h | 11 +++++++++++
 2 files changed, 33 insertions(+)

diff --git a/tools/firmware/hvmloader/util.c b/tools/firmware/hvmloader/util.c
index 80d822f..5d656f1 100644
--- a/tools/firmware/hvmloader/util.c
+++ b/tools/firmware/hvmloader/util.c
@@ -828,6 +828,28 @@ int hpet_exists(unsigned long hpet_base)
     return ((hpet_id >> 16) == 0x8086);
 }
 
+int get_reserved_device_memory_map(struct reserved_device_memory *entries,
+                                   uint32_t max_entries)
+{
+    static int map_done = 0;
+    struct xen_reserved_device_memory_map memmap = {
+        .nr_entries = max_entries
+    };
+
+    if ( map_done )
+        return 0;
+
+    set_xen_guest_handle(memmap.buffer, entries);
+
+    if ( hypercall_memory_op(XENMEM_reserved_device_memory_map,
+                             &memmap) != 0 )
+        BUG();
+
+    map_done = 1;
+
+    return 0;
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/tools/firmware/hvmloader/util.h b/tools/firmware/hvmloader/util.h
index a70e4aa..b6ca739 100644
--- a/tools/firmware/hvmloader/util.h
+++ b/tools/firmware/hvmloader/util.h
@@ -241,6 +241,17 @@ int build_e820_table(struct e820entry *e820,
                      unsigned int bios_image_base);
 void dump_e820_table(struct e820entry *e820, unsigned int nr);
 
+
+struct reserved_device_memory {
+    /* PFN of the current mapping of the page. */
+    unsigned int pfn;
+    /* Number of the current mapping pages. */
+    unsigned int count;
+};
+typedef struct reserved_device_memory reserved_device_memory_t;
+
+int get_reserved_device_memory_map(struct reserved_device_memory *entries,
+                                   uint32_t max_entries);
 #ifndef NDEBUG
 void perform_tests(void);
 #else
-- 
1.9.1

[v4][PATCH 8/9] tools:firmware:hvmloader: check to reserve RMRR mappings in e820

[Tiejun Chen]

We need to check 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 | 101 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 101 insertions(+)

diff --git a/tools/firmware/hvmloader/e820.c b/tools/firmware/hvmloader/e820.c
index 2e05e93..ae51fa6 100644
--- a/tools/firmware/hvmloader/e820.c
+++ b/tools/firmware/hvmloader/e820.c
@@ -68,12 +68,90 @@ void dump_e820_table(struct e820entry *e820, unsigned int nr)
     }
 }
 
+static unsigned int construct_rmrr_e820_maps(unsigned int nr,
+                                             uint32_t nr_map,
+                                             struct reserved_device_memory *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 + nr_map;
+    for ( i = 0; i < nr_map; i++ )
+    {
+        rmrr_start = map[i].pfn << PAGE_SHIFT;
+        rmrr_end = rmrr_start + map[i].count * PAGE_SIZE;
+
+        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 == nr_map)
+    {
+        do_insert = 1;
+        goto do_real_construct;
+    }
+    /* Overlap. */
+    else
+    {
+        printf("RMRR overlap with those 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 reserved_device_memory *map = NULL;
+    int rc;
 
     if ( !lowmem_reserved_base )
             lowmem_reserved_base = 0xA0000;
@@ -169,6 +247,29 @@ int build_e820_table(struct e820entry *e820,
         nr++;
     }
 
+    /* We'd better reserve RMRR mapping for each VM to avoid potential
+     * memory conflict.
+     */
+    if ( hvm_info->nr_reserved_device_memory_map )
+    {
+        if ( !map )
+            map = mem_alloc(hvm_info->nr_reserved_device_memory_map * sizeof(struct reserved_device_memory), 0);
+        if ( map )
+        {
+            rc = get_reserved_device_memory_map(map,
+                                                hvm_info->nr_reserved_device_memory_map);
+            if ( !rc )
+            {
+                nr = construct_rmrr_e820_maps(nr,
+                                              hvm_info->nr_reserved_device_memory_map,
+                                              map,
+                                              e820);
+            }
+        }
+        else
+            printf("No space to get reserved_device_memory_map!\n");
+    }
+
     return nr;
 }
 
-- 
1.9.1

[v4][PATCH 9/9] 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: [v4][PATCH 2/9] xen:x86: define a new hypercall to get RMRR mappings

[Andrew Cooper]

On 22/08/14 11:09, 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/mm.c           | 71 +++++++++++++++++++++++++++++++++++++++++++++
>  xen/include/public/memory.h | 37 ++++++++++++++++++++++-
>  2 files changed, 107 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> index d23cb3f..e0d6650 100644
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -123,6 +123,7 @@
>  #include <asm/setup.h>
>  #include <asm/fixmap.h>
>  #include <asm/pci.h>
> +#include <asm/acpi.h>
>  
>  /* Mapping of the fixmap space needed early. */
>  l1_pgentry_t __attribute__ ((__section__ (".bss.page_aligned")))
> @@ -4842,6 +4843,76 @@ long arch_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>          return rc;
>      }
>  
> +    case XENMEM_reserved_device_memory_map:
> +    {
> +        struct xen_reserved_device_memory_map map;
> +        XEN_GUEST_HANDLE(xen_reserved_device_memory_t) buffer;
> +        XEN_GUEST_HANDLE_PARAM(xen_reserved_device_memory_t) buffer_param;
> +        unsigned int i = 0;
> +        static unsigned int nr_entries = 0;
> +        static struct xen_reserved_device_memory *rmrr_map;

Absolutely not.  This hypercall can easy be run concurrently.

> +        struct acpi_rmrr_unit *rmrr;
> +
> +        if ( copy_from_guest(&map, arg, 1) )
> +            return -EFAULT;
> +
> +        if ( !nr_entries )
> +            /* Currently we just need to cover RMRR. */
> +            list_for_each_entry( rmrr, &acpi_rmrr_units, list )
> +                nr_entries++;

Maintain a global count as entries are added/removed from this list.  It
is a waste of time recounting this list for each hypercall.

> +
> +        if ( !nr_entries )
> +                return -ENOENT;
> +        else
> +        {
> +            if ( rmrr_map == NULL )
> +            {
> +                rmrr_map = xmalloc_array(xen_reserved_device_memory_t,
> +                                         nr_entries);

You can do all of this without any memory allocation.

> +                if ( rmrr_map == NULL )
> +                {
> +                    return -ENOMEM;
> +                }
> +
> +                list_for_each_entry( rmrr, &acpi_rmrr_units, list )
> +                {
> +                    rmrr_map[i].pfn = rmrr->base_address >> PAGE_SHIFT;
> +                    rmrr_map[i].count = PAGE_ALIGN(rmrr->end_address -
> +                                                   rmrr->base_address) /
> +                                                   PAGE_SIZE;
> +                    i++;
> +                }
> +            }
> +        }
> +
> +        if ( map.nr_entries < nr_entries )
> +        {
> +            map.nr_entries =  nr_entries;
> +            if ( copy_to_guest(arg, &map, 1) )
> +                return -EFAULT;
> +            return -ENOBUFS;
> +        }
> +
> +        map.nr_entries =  nr_entries;
> +        buffer_param = guest_handle_cast(map.buffer,
> +                                         xen_reserved_device_memory_t);
> +        buffer = guest_handle_from_param(buffer_param,
> +                                         xen_reserved_device_memory_t);
> +        if ( !guest_handle_okay(buffer, map.nr_entries) )
> +            return -EFAULT;
> +
> +        for ( i = 0; i < map.nr_entries; ++i )
> +        {
> +            if ( copy_to_guest_offset(buffer, i, rmrr_map + i, 1) )
> +                return -EFAULT;
> +        }
> +
> +        if ( copy_to_guest(arg, &map, 1) )
> +                return -EFAULT;
> +
> +        return 0;
> +    }
> +
>      default:
>          return subarch_memory_op(cmd, arg);
>      }
> diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h
> index 2c57aa0..8481843 100644
> --- a/xen/include/public/memory.h
> +++ b/xen/include/public/memory.h
> @@ -523,7 +523,42 @@ 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.

"range" is not a useful unit of measure.

> + *
> + * 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
> +struct xen_reserved_device_memory {

xen_mem_ to match the prevailing style

> +    /* PFN of the current mapping of the page. */
> +    xen_pfn_t pfn;
> +    /* Number of the current mapping pages. */
> +    xen_ulong_t count;
> +};

This struct marks a range, but the fields don't make it clear.  I would
suggest "start" and "nr_frames" as names.

~Andrew

> +typedef struct xen_reserved_device_memory xen_reserved_device_memory_t;
> +DEFINE_XEN_GUEST_HANDLE(xen_reserved_device_memory_t);
> +
> +struct xen_reserved_device_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
> +     * xen_reserved_device_memory.
> +     */
> +    XEN_GUEST_HANDLE(void) buffer;
> +};
> +typedef struct xen_reserved_device_memory_map xen_reserved_device_memory_map_t;
> +DEFINE_XEN_GUEST_HANDLE(xen_reserved_device_memory_map_t);
> +
> +/* Next available subop number is 27 */
>  
>  #endif /* __XEN_PUBLIC_MEMORY_H__ */
>  

Re: [v4][PATCH 3/9] tools:libxc: introduce hypercall for xc_reserved_device_memory_map

[Andrew Cooper]

On 22/08/14 11:09, Tiejun Chen wrote:
> We will introduce that hypercall xc_reserved_device_memory_map
> to libxc.
>
> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
> ---
>  tools/libxc/xc_domain.c | 29 +++++++++++++++++++++++++++++
>  tools/libxc/xenctrl.h   |  4 ++++
>  2 files changed, 33 insertions(+)
>
> diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c
> index c67ac9a..08dc16f 100644
> --- a/tools/libxc/xc_domain.c
> +++ b/tools/libxc/xc_domain.c
> @@ -649,6 +649,35 @@ int xc_domain_set_memory_map(xc_interface *xch,
>  
>      return rc;
>  }
> +
> +int xc_reserved_device_memory_map(xc_interface *xch,
> +                                  struct xen_reserved_device_memory entries[],
> +                                  uint32_t max_entries)
> +{
> +    int rc;
> +    struct xen_reserved_device_memory_map memmap = {
> +        .nr_entries = max_entries
> +    };
> +    DECLARE_HYPERCALL_BOUNCE(entries,
> +                             sizeof(struct xen_reserved_device_memory) *
> +                             max_entries, XC_HYPERCALL_BUFFER_BOUNCE_OUT);
> +
> +    if ( xc_hypercall_bounce_pre(xch, entries) )
> +        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);
> +
> +    if ( errno == ENOBUFS )
> +        return memmap.nr_entries;
> +
> +    return rc ? -errno : memmap.nr_entries;

So how does the caller distinguish between "xen filled in N entries" and
"xen said you need N entries for all the information" ?

~Andrew

> +}
> +
>  int xc_get_machine_memory_map(xc_interface *xch,
>                                struct e820entry entries[],
>                                uint32_t max_entries)
> diff --git a/tools/libxc/xenctrl.h b/tools/libxc/xenctrl.h
> index 1c5d0db..1cc7852 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_reserved_device_memory_map(xc_interface *xch,
> +                                  struct xen_reserved_device_memory entries[],
> +                                  uint32_t max_entries);
>  #endif
>  int xc_domain_set_time_offset(xc_interface *xch,
>                                uint32_t domid,

Re: [v4][PATCH 2/9] xen:x86: define a new hypercall to get RMRR mappings

[Jan Beulich]

>>> On 22.08.14 at 12:53, <andrew.cooper3@citrix.com> wrote:
> On 22/08/14 11:09, Tiejun Chen wrote:
>> +    /* PFN of the current mapping of the page. */
>> +    xen_pfn_t pfn;
>> +    /* Number of the current mapping pages. */
>> +    xen_ulong_t count;
>> +};
> 
> This struct marks a range, but the fields don't make it clear.  I would
> suggest "start" and "nr_frames" as names.

Perhaps "start_pfn" to be even more explicit (and not just
depend on the field's type), and then maybe also "nr_pfns"
or "nr_pages".

Jan

Re: [v4][PATCH 2/9] xen:x86: define a new hypercall to get RMRR mappings

[Chen, Tiejun]

On 2014/8/22 19:36, Jan Beulich wrote:
>>>> On 22.08.14 at 12:53, <andrew.cooper3@citrix.com> wrote:
>> On 22/08/14 11:09, Tiejun Chen wrote:
>>> +    /* PFN of the current mapping of the page. */
>>> +    xen_pfn_t pfn;
>>> +    /* Number of the current mapping pages. */
>>> +    xen_ulong_t count;
>>> +};
>>
>> This struct marks a range, but the fields don't make it clear.  I would
>> suggest "start" and "nr_frames" as names.
>
> Perhaps "start_pfn" to be even more explicit (and not just
> depend on the field's type), and then maybe also "nr_pfns"
> or "nr_pages".
>

Okay, I will go this combo, 'start_pfn' and 'nr_pages'.

Thanks
Tiejun

Re: [v4][PATCH 3/9] tools:libxc: introduce hypercall for xc_reserved_device_memory_map

[Chen, Tiejun]

On 2014/8/22 18:55, Andrew Cooper wrote:
> On 22/08/14 11:09, Tiejun Chen wrote:
>> We will introduce that hypercall xc_reserved_device_memory_map
>> to libxc.
>>
>> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
>> ---
>>   tools/libxc/xc_domain.c | 29 +++++++++++++++++++++++++++++
>>   tools/libxc/xenctrl.h   |  4 ++++
>>   2 files changed, 33 insertions(+)
>>
>> diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c
>> index c67ac9a..08dc16f 100644
>> --- a/tools/libxc/xc_domain.c
>> +++ b/tools/libxc/xc_domain.c
>> @@ -649,6 +649,35 @@ int xc_domain_set_memory_map(xc_interface *xch,
>>
>>       return rc;
>>   }
>> +
>> +int xc_reserved_device_memory_map(xc_interface *xch,
>> +                                  struct xen_reserved_device_memory entries[],
>> +                                  uint32_t max_entries)
>> +{
>> +    int rc;
>> +    struct xen_reserved_device_memory_map memmap = {
>> +        .nr_entries = max_entries
>> +    };
>> +    DECLARE_HYPERCALL_BOUNCE(entries,
>> +                             sizeof(struct xen_reserved_device_memory) *
>> +                             max_entries, XC_HYPERCALL_BUFFER_BOUNCE_OUT);
>> +
>> +    if ( xc_hypercall_bounce_pre(xch, entries) )
>> +        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);
>> +
>> +    if ( errno == ENOBUFS )
>> +        return memmap.nr_entries;
>> +
>> +    return rc ? -errno : memmap.nr_entries;
>
> So how does the caller distinguish between "xen filled in N entries" and
> "xen said you need N entries for all the information" ?

Thanks for your reminder, I will add something to check this point.

I think the caller can compare that number of entries as that input 
parameter with the return value. If equal, this should be in case of 
"xen filled in N entries". If not, this is in case of "xen said you need 
N entries for all the information".

Thanks
Tiejun

>
> ~Andrew
>
>> +}
>> +
>>   int xc_get_machine_memory_map(xc_interface *xch,
>>                                 struct e820entry entries[],
>>                                 uint32_t max_entries)
>> diff --git a/tools/libxc/xenctrl.h b/tools/libxc/xenctrl.h
>> index 1c5d0db..1cc7852 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_reserved_device_memory_map(xc_interface *xch,
>> +                                  struct xen_reserved_device_memory entries[],
>> +                                  uint32_t max_entries);
>>   #endif
>>   int xc_domain_set_time_offset(xc_interface *xch,
>>                                 uint32_t domid,
>
>

Re: [v4][PATCH 2/9] xen:x86: define a new hypercall to get RMRR mappings

[Chen, Tiejun]

On 2014/8/22 18:53, Andrew Cooper wrote:
> On 22/08/14 11:09, 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/mm.c           | 71 +++++++++++++++++++++++++++++++++++++++++++++
>>   xen/include/public/memory.h | 37 ++++++++++++++++++++++-
>>   2 files changed, 107 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
>> index d23cb3f..e0d6650 100644
>> --- a/xen/arch/x86/mm.c
>> +++ b/xen/arch/x86/mm.c
>> @@ -123,6 +123,7 @@
>>   #include <asm/setup.h>
>>   #include <asm/fixmap.h>
>>   #include <asm/pci.h>
>> +#include <asm/acpi.h>
>>
>>   /* Mapping of the fixmap space needed early. */
>>   l1_pgentry_t __attribute__ ((__section__ (".bss.page_aligned")))
>> @@ -4842,6 +4843,76 @@ long arch_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>>           return rc;
>>       }
>>
>> +    case XENMEM_reserved_device_memory_map:
>> +    {
>> +        struct xen_reserved_device_memory_map map;
>> +        XEN_GUEST_HANDLE(xen_reserved_device_memory_t) buffer;
>> +        XEN_GUEST_HANDLE_PARAM(xen_reserved_device_memory_t) buffer_param;
>> +        unsigned int i = 0;
>> +        static unsigned int nr_entries = 0;
>> +        static struct xen_reserved_device_memory *rmrr_map;
>
> Absolutely not.  This hypercall can easy be run concurrently.
>
>> +        struct acpi_rmrr_unit *rmrr;
>> +
>> +        if ( copy_from_guest(&map, arg, 1) )
>> +            return -EFAULT;
>> +
>> +        if ( !nr_entries )
>> +            /* Currently we just need to cover RMRR. */
>> +            list_for_each_entry( rmrr, &acpi_rmrr_units, list )
>> +                nr_entries++;
>
> Maintain a global count as entries are added/removed from this list.  It
> is a waste of time recounting this list for each hypercall.

Are you saying push this 'nr_entries' as a global count somewhere? I 
guess I can set this when we first construct acpi_rmrr_units in ACPI stuff.

>
>> +
>> +        if ( !nr_entries )
>> +                return -ENOENT;
>> +        else
>> +        {
>> +            if ( rmrr_map == NULL )
>> +            {
>> +                rmrr_map = xmalloc_array(xen_reserved_device_memory_t,
>> +                                         nr_entries);
>
> You can do all of this without any memory allocation.

I will check this.

>
>> +                if ( rmrr_map == NULL )
>> +                {
>> +                    return -ENOMEM;
>> +                }
>> +
>> +                list_for_each_entry( rmrr, &acpi_rmrr_units, list )
>> +                {
>> +                    rmrr_map[i].pfn = rmrr->base_address >> PAGE_SHIFT;
>> +                    rmrr_map[i].count = PAGE_ALIGN(rmrr->end_address -
>> +                                                   rmrr->base_address) /
>> +                                                   PAGE_SIZE;
>> +                    i++;
>> +                }
>> +            }
>> +        }
>> +
>> +        if ( map.nr_entries < nr_entries )
>> +        {
>> +            map.nr_entries =  nr_entries;
>> +            if ( copy_to_guest(arg, &map, 1) )
>> +                return -EFAULT;
>> +            return -ENOBUFS;
>> +        }
>> +
>> +        map.nr_entries =  nr_entries;
>> +        buffer_param = guest_handle_cast(map.buffer,
>> +                                         xen_reserved_device_memory_t);
>> +        buffer = guest_handle_from_param(buffer_param,
>> +                                         xen_reserved_device_memory_t);
>> +        if ( !guest_handle_okay(buffer, map.nr_entries) )
>> +            return -EFAULT;
>> +
>> +        for ( i = 0; i < map.nr_entries; ++i )
>> +        {
>> +            if ( copy_to_guest_offset(buffer, i, rmrr_map + i, 1) )
>> +                return -EFAULT;
>> +        }
>> +
>> +        if ( copy_to_guest(arg, &map, 1) )
>> +                return -EFAULT;
>> +
>> +        return 0;
>> +    }
>> +
>>       default:
>>           return subarch_memory_op(cmd, arg);
>>       }
>> diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h
>> index 2c57aa0..8481843 100644
>> --- a/xen/include/public/memory.h
>> +++ b/xen/include/public/memory.h
>> @@ -523,7 +523,42 @@ 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.
>
> "range" is not a useful unit of measure.

So what about this?

The regions of memory used for some devices should be reserved.

>
>> + *
>> + * 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
>> +struct xen_reserved_device_memory {
>
> xen_mem_ to match the prevailing style

Okay.

>
>> +    /* PFN of the current mapping of the page. */
>> +    xen_pfn_t pfn;
>> +    /* Number of the current mapping pages. */
>> +    xen_ulong_t count;
>> +};
>
> This struct marks a range, but the fields don't make it clear.  I would
> suggest "start" and "nr_frames" as names.
>

I will prefer Jan's suggestion.

Thanks
Tiejun

> ~Andrew
>
>> +typedef struct xen_reserved_device_memory xen_reserved_device_memory_t;
>> +DEFINE_XEN_GUEST_HANDLE(xen_reserved_device_memory_t);
>> +
>> +struct xen_reserved_device_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
>> +     * xen_reserved_device_memory.
>> +     */
>> +    XEN_GUEST_HANDLE(void) buffer;
>> +};
>> +typedef struct xen_reserved_device_memory_map xen_reserved_device_memory_map_t;
>> +DEFINE_XEN_GUEST_HANDLE(xen_reserved_device_memory_map_t);
>> +
>> +/* Next available subop number is 27 */
>>
>>   #endif /* __XEN_PUBLIC_MEMORY_H__ */
>>
>
>

Re: [v4][PATCH 3/9] tools:libxc: introduce hypercall for xc_reserved_device_memory_map

[Andrew Cooper]

On 25/08/14 12:11, Chen, Tiejun wrote:
>
>
> On 2014/8/22 18:55, Andrew Cooper wrote:
>> On 22/08/14 11:09, Tiejun Chen wrote:
>>> We will introduce that hypercall xc_reserved_device_memory_map
>>> to libxc.
>>>
>>> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
>>> ---
>>>   tools/libxc/xc_domain.c | 29 +++++++++++++++++++++++++++++
>>>   tools/libxc/xenctrl.h   |  4 ++++
>>>   2 files changed, 33 insertions(+)
>>>
>>> diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c
>>> index c67ac9a..08dc16f 100644
>>> --- a/tools/libxc/xc_domain.c
>>> +++ b/tools/libxc/xc_domain.c
>>> @@ -649,6 +649,35 @@ int xc_domain_set_memory_map(xc_interface *xch,
>>>
>>>       return rc;
>>>   }
>>> +
>>> +int xc_reserved_device_memory_map(xc_interface *xch,
>>> +                                  struct xen_reserved_device_memory
>>> entries[],
>>> +                                  uint32_t max_entries)
>>> +{
>>> +    int rc;
>>> +    struct xen_reserved_device_memory_map memmap = {
>>> +        .nr_entries = max_entries
>>> +    };
>>> +    DECLARE_HYPERCALL_BOUNCE(entries,
>>> +                             sizeof(struct
>>> xen_reserved_device_memory) *
>>> +                             max_entries,
>>> XC_HYPERCALL_BUFFER_BOUNCE_OUT);
>>> +
>>> +    if ( xc_hypercall_bounce_pre(xch, entries) )
>>> +        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);
>>> +
>>> +    if ( errno == ENOBUFS )
>>> +        return memmap.nr_entries;
>>> +
>>> +    return rc ? -errno : memmap.nr_entries;
>>
>> So how does the caller distinguish between "xen filled in N entries" and
>> "xen said you need N entries for all the information" ?
>
> Thanks for your reminder, I will add something to check this point.
>
> I think the caller can compare that number of entries as that input
> parameter with the return value. If equal, this should be in case of
> "xen filled in N entries". If not, this is in case of "xen said you
> need N entries for all the information".
>
> Thanks
> Tiejun

You have to pass max_entries by pointer, and leave the ENOBUFS handling
to the caller, as it is the only one capable of acting upon the information.

~Andrew

Re: [v4][PATCH 2/9] xen:x86: define a new hypercall to get RMRR mappings

[Andrew Cooper]

On 25/08/14 12:21, Chen, Tiejun wrote:
> On 2014/8/22 18:53, Andrew Cooper wrote:
>> On 22/08/14 11:09, 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/mm.c           | 71
>>> +++++++++++++++++++++++++++++++++++++++++++++
>>>   xen/include/public/memory.h | 37 ++++++++++++++++++++++-
>>>   2 files changed, 107 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
>>> index d23cb3f..e0d6650 100644
>>> --- a/xen/arch/x86/mm.c
>>> +++ b/xen/arch/x86/mm.c
>>> @@ -123,6 +123,7 @@
>>>   #include <asm/setup.h>
>>>   #include <asm/fixmap.h>
>>>   #include <asm/pci.h>
>>> +#include <asm/acpi.h>
>>>
>>>   /* Mapping of the fixmap space needed early. */
>>>   l1_pgentry_t __attribute__ ((__section__ (".bss.page_aligned")))
>>> @@ -4842,6 +4843,76 @@ long arch_memory_op(unsigned long cmd,
>>> XEN_GUEST_HANDLE_PARAM(void) arg)
>>>           return rc;
>>>       }
>>>
>>> +    case XENMEM_reserved_device_memory_map:
>>> +    {
>>> +        struct xen_reserved_device_memory_map map;
>>> +        XEN_GUEST_HANDLE(xen_reserved_device_memory_t) buffer;
>>> +        XEN_GUEST_HANDLE_PARAM(xen_reserved_device_memory_t)
>>> buffer_param;
>>> +        unsigned int i = 0;
>>> +        static unsigned int nr_entries = 0;
>>> +        static struct xen_reserved_device_memory *rmrr_map;
>>
>> Absolutely not.  This hypercall can easy be run concurrently.
>>
>>> +        struct acpi_rmrr_unit *rmrr;
>>> +
>>> +        if ( copy_from_guest(&map, arg, 1) )
>>> +            return -EFAULT;
>>> +
>>> +        if ( !nr_entries )
>>> +            /* Currently we just need to cover RMRR. */
>>> +            list_for_each_entry( rmrr, &acpi_rmrr_units, list )
>>> +                nr_entries++;
>>
>> Maintain a global count as entries are added/removed from this list.  It
>> is a waste of time recounting this list for each hypercall.
>
> Are you saying push this 'nr_entries' as a global count somewhere? I
> guess I can set this when we first construct acpi_rmrr_units in ACPI
> stuff.

Not named "nr_entries", but yes.  It is constant after boot.

>
>>
>>> +
>>> +        if ( !nr_entries )
>>> +                return -ENOENT;
>>> +        else
>>> +        {
>>> +            if ( rmrr_map == NULL )
>>> +            {
>>> +                rmrr_map = xmalloc_array(xen_reserved_device_memory_t,
>>> +                                         nr_entries);
>>
>> You can do all of this without any memory allocation.
>
> I will check this.

It is easy...

>
>>
>>> +                if ( rmrr_map == NULL )
>>> +                {
>>> +                    return -ENOMEM;
>>> +                }
>>> +
>>> +                list_for_each_entry( rmrr, &acpi_rmrr_units, list )
>>> +                {
>>> +                    rmrr_map[i].pfn = rmrr->base_address >>
>>> PAGE_SHIFT;
>>> +                    rmrr_map[i].count = PAGE_ALIGN(rmrr->end_address -
>>> +                                                  
>>> rmrr->base_address) /
>>> +                                                   PAGE_SIZE;
>>> +                    i++;

In this loop, construct one on the stack and copy_to_guest, breaking if
there is a fault or you exceed the guests array.

>>> +                }
>>> +            }
>>> +        }
>>> +
>>> +        if ( map.nr_entries < nr_entries )
>>> +        {
>>> +            map.nr_entries =  nr_entries;
>>> +            if ( copy_to_guest(arg, &map, 1) )
>>> +                return -EFAULT;
>>> +            return -ENOBUFS;
>>> +        }
>>> +
>>> +        map.nr_entries =  nr_entries;
>>> +        buffer_param = guest_handle_cast(map.buffer,
>>> +                                        
>>> xen_reserved_device_memory_t);
>>> +        buffer = guest_handle_from_param(buffer_param,
>>> +                                        
>>> xen_reserved_device_memory_t);
>>> +        if ( !guest_handle_okay(buffer, map.nr_entries) )
>>> +            return -EFAULT;
>>> +
>>> +        for ( i = 0; i < map.nr_entries; ++i )
>>> +        {
>>> +            if ( copy_to_guest_offset(buffer, i, rmrr_map + i, 1) )
>>> +                return -EFAULT;
>>> +        }
>>> +
>>> +        if ( copy_to_guest(arg, &map, 1) )
>>> +                return -EFAULT;
>>> +
>>> +        return 0;
>>> +    }
>>> +
>>>       default:
>>>           return subarch_memory_op(cmd, arg);
>>>       }
>>> diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h
>>> index 2c57aa0..8481843 100644
>>> --- a/xen/include/public/memory.h
>>> +++ b/xen/include/public/memory.h
>>> @@ -523,7 +523,42 @@ 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.
>>
>> "range" is not a useful unit of measure.
>
> So what about this?
>
> The regions of memory used for some devices should be reserved.

No - it is not a case of "should", it is a case of "must".

"For legacy reasons, some devices must be configured with special memory
regions to function correctly.  The guest must avoid using any of these
regions."

>
>>
>>> + *
>>> + * 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
>>> +struct xen_reserved_device_memory {
>>
>> xen_mem_ to match the prevailing style
>
> Okay.
>
>>
>>> +    /* PFN of the current mapping of the page. */
>>> +    xen_pfn_t pfn;
>>> +    /* Number of the current mapping pages. */
>>> +    xen_ulong_t count;
>>> +};
>>
>> This struct marks a range, but the fields don't make it clear.  I would
>> suggest "start" and "nr_frames" as names.
>>
>
> I will prefer Jan's suggestion.

As do I,

~Andrew

Re: [v4][PATCH 2/9] xen:x86: define a new hypercall to get RMRR mappings

[Chen, Tiejun]

On 2014/8/25 20:07, Andrew Cooper wrote:
> On 25/08/14 12:21, Chen, Tiejun wrote:
>> On 2014/8/22 18:53, Andrew Cooper wrote:
>>> On 22/08/14 11:09, 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/mm.c           | 71
>>>> +++++++++++++++++++++++++++++++++++++++++++++
>>>>    xen/include/public/memory.h | 37 ++++++++++++++++++++++-
>>>>    2 files changed, 107 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
>>>> index d23cb3f..e0d6650 100644
>>>> --- a/xen/arch/x86/mm.c
>>>> +++ b/xen/arch/x86/mm.c
>>>> @@ -123,6 +123,7 @@
>>>>    #include <asm/setup.h>
>>>>    #include <asm/fixmap.h>
>>>>    #include <asm/pci.h>
>>>> +#include <asm/acpi.h>
>>>>
>>>>    /* Mapping of the fixmap space needed early. */
>>>>    l1_pgentry_t __attribute__ ((__section__ (".bss.page_aligned")))
>>>> @@ -4842,6 +4843,76 @@ long arch_memory_op(unsigned long cmd,
>>>> XEN_GUEST_HANDLE_PARAM(void) arg)
>>>>            return rc;
>>>>        }
>>>>
>>>> +    case XENMEM_reserved_device_memory_map:
>>>> +    {
>>>> +        struct xen_reserved_device_memory_map map;
>>>> +        XEN_GUEST_HANDLE(xen_reserved_device_memory_t) buffer;
>>>> +        XEN_GUEST_HANDLE_PARAM(xen_reserved_device_memory_t)
>>>> buffer_param;
>>>> +        unsigned int i = 0;
>>>> +        static unsigned int nr_entries = 0;
>>>> +        static struct xen_reserved_device_memory *rmrr_map;
>>>
>>> Absolutely not.  This hypercall can easy be run concurrently.
>>>
>>>> +        struct acpi_rmrr_unit *rmrr;
>>>> +
>>>> +        if ( copy_from_guest(&map, arg, 1) )
>>>> +            return -EFAULT;
>>>> +
>>>> +        if ( !nr_entries )
>>>> +            /* Currently we just need to cover RMRR. */
>>>> +            list_for_each_entry( rmrr, &acpi_rmrr_units, list )
>>>> +                nr_entries++;
>>>
>>> Maintain a global count as entries are added/removed from this list.  It
>>> is a waste of time recounting this list for each hypercall.
>>
>> Are you saying push this 'nr_entries' as a global count somewhere? I
>> guess I can set this when we first construct acpi_rmrr_units in ACPI
>> stuff.
>
> Not named "nr_entries", but yes.  It is constant after boot.
>
>>
>>>
>>>> +
>>>> +        if ( !nr_entries )
>>>> +                return -ENOENT;
>>>> +        else
>>>> +        {
>>>> +            if ( rmrr_map == NULL )
>>>> +            {
>>>> +                rmrr_map = xmalloc_array(xen_reserved_device_memory_t,
>>>> +                                         nr_entries);
>>>
>>> You can do all of this without any memory allocation.
>>

What is your way without any memory allocation? Do you mean I should 
predefine a static array here? But how to predetermine the size?

Or you mean I should do something with one rmrr_map, like this,

	struct xen_mem_reserved_device_memory rmrr_map;

	list_for_each_entry( rmrr, &acpi_rmrr_units, list )
	{
		rmrr_map.start_pfn = ...;
		rmrr_map.nr_pages = ...;
		if ( copy_to_guest_offset(buffer, i, &rmrr_map, 1) )			return -EFAULT;
		i++;
	}


>> I will check this.
>
> It is easy...
>
>>
>>>
>>>> +                if ( rmrr_map == NULL )
>>>> +                {
>>>> +                    return -ENOMEM;
>>>> +                }
>>>> +
>>>> +                list_for_each_entry( rmrr, &acpi_rmrr_units, list )
>>>> +                {
>>>> +                    rmrr_map[i].pfn = rmrr->base_address >>
>>>> PAGE_SHIFT;
>>>> +                    rmrr_map[i].count = PAGE_ALIGN(rmrr->end_address -
>>>> +
>>>> rmrr->base_address) /
>>>> +                                                   PAGE_SIZE;
>>>> +                    i++;
>
> In this loop, construct one on the stack and copy_to_guest, breaking if
> there is a fault or you exceed the guests array.

Its not possible to exceed the guests array since the caller always 
check if it get such a error, -ENOBUFS, then if yes, it can reallocate 
appropriate array.

Thanks
Tiejun

>
>>>> +                }
>>>> +            }
>>>> +        }
>>>> +
>>>> +        if ( map.nr_entries < nr_entries )
>>>> +        {
>>>> +            map.nr_entries =  nr_entries;
>>>> +            if ( copy_to_guest(arg, &map, 1) )
>>>> +                return -EFAULT;
>>>> +            return -ENOBUFS;
>>>> +        }
>>>> +
>>>> +        map.nr_entries =  nr_entries;
>>>> +        buffer_param = guest_handle_cast(map.buffer,
>>>> +
>>>> xen_reserved_device_memory_t);
>>>> +        buffer = guest_handle_from_param(buffer_param,
>>>> +
>>>> xen_reserved_device_memory_t);
>>>> +        if ( !guest_handle_okay(buffer, map.nr_entries) )
>>>> +            return -EFAULT;
>>>> +
>>>> +        for ( i = 0; i < map.nr_entries; ++i )
>>>> +        {
>>>> +            if ( copy_to_guest_offset(buffer, i, rmrr_map + i, 1) )
>>>> +                return -EFAULT;
>>>> +        }
>>>> +
>>>> +        if ( copy_to_guest(arg, &map, 1) )
>>>> +                return -EFAULT;
>>>> +
>>>> +        return 0;
>>>> +    }
>>>> +
>>>>        default:
>>>>            return subarch_memory_op(cmd, arg);
>>>>        }
>>>> diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h
>>>> index 2c57aa0..8481843 100644
>>>> --- a/xen/include/public/memory.h
>>>> +++ b/xen/include/public/memory.h
>>>> @@ -523,7 +523,42 @@ 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.
>>>
>>> "range" is not a useful unit of measure.
>>
>> So what about this?
>>
>> The regions of memory used for some devices should be reserved.
>
> No - it is not a case of "should", it is a case of "must".
>
> "For legacy reasons, some devices must be configured with special memory
> regions to function correctly.  The guest must avoid using any of these
> regions."
>
>>
>>>
>>>> + *
>>>> + * 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
>>>> +struct xen_reserved_device_memory {
>>>
>>> xen_mem_ to match the prevailing style
>>
>> Okay.
>>
>>>
>>>> +    /* PFN of the current mapping of the page. */
>>>> +    xen_pfn_t pfn;
>>>> +    /* Number of the current mapping pages. */
>>>> +    xen_ulong_t count;
>>>> +};
>>>
>>> This struct marks a range, but the fields don't make it clear.  I would
>>> suggest "start" and "nr_frames" as names.
>>>
>>
>> I will prefer Jan's suggestion.
>
> As do I,
>
> ~Andrew
>

Re: [v4][PATCH 2/9] xen:x86: define a new hypercall to get RMRR mappings

[Andrew Cooper]

On 26/08/14 04:12, Chen, Tiejun wrote:
> On 2014/8/25 20:07, Andrew Cooper wrote:
>> On 25/08/14 12:21, Chen, Tiejun wrote:
>>> On 2014/8/22 18:53, Andrew Cooper wrote:
>>>> On 22/08/14 11:09, 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/mm.c           | 71
>>>>> +++++++++++++++++++++++++++++++++++++++++++++
>>>>>    xen/include/public/memory.h | 37 ++++++++++++++++++++++-
>>>>>    2 files changed, 107 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
>>>>> index d23cb3f..e0d6650 100644
>>>>> --- a/xen/arch/x86/mm.c
>>>>> +++ b/xen/arch/x86/mm.c
>>>>> @@ -123,6 +123,7 @@
>>>>>    #include <asm/setup.h>
>>>>>    #include <asm/fixmap.h>
>>>>>    #include <asm/pci.h>
>>>>> +#include <asm/acpi.h>
>>>>>
>>>>>    /* Mapping of the fixmap space needed early. */
>>>>>    l1_pgentry_t __attribute__ ((__section__ (".bss.page_aligned")))
>>>>> @@ -4842,6 +4843,76 @@ long arch_memory_op(unsigned long cmd,
>>>>> XEN_GUEST_HANDLE_PARAM(void) arg)
>>>>>            return rc;
>>>>>        }
>>>>>
>>>>> +    case XENMEM_reserved_device_memory_map:
>>>>> +    {
>>>>> +        struct xen_reserved_device_memory_map map;
>>>>> +        XEN_GUEST_HANDLE(xen_reserved_device_memory_t) buffer;
>>>>> +        XEN_GUEST_HANDLE_PARAM(xen_reserved_device_memory_t)
>>>>> buffer_param;
>>>>> +        unsigned int i = 0;
>>>>> +        static unsigned int nr_entries = 0;
>>>>> +        static struct xen_reserved_device_memory *rmrr_map;
>>>>
>>>> Absolutely not.  This hypercall can easy be run concurrently.
>>>>
>>>>> +        struct acpi_rmrr_unit *rmrr;
>>>>> +
>>>>> +        if ( copy_from_guest(&map, arg, 1) )
>>>>> +            return -EFAULT;
>>>>> +
>>>>> +        if ( !nr_entries )
>>>>> +            /* Currently we just need to cover RMRR. */
>>>>> +            list_for_each_entry( rmrr, &acpi_rmrr_units, list )
>>>>> +                nr_entries++;
>>>>
>>>> Maintain a global count as entries are added/removed from this
>>>> list.  It
>>>> is a waste of time recounting this list for each hypercall.
>>>
>>> Are you saying push this 'nr_entries' as a global count somewhere? I
>>> guess I can set this when we first construct acpi_rmrr_units in ACPI
>>> stuff.
>>
>> Not named "nr_entries", but yes.  It is constant after boot.
>>
>>>
>>>>
>>>>> +
>>>>> +        if ( !nr_entries )
>>>>> +                return -ENOENT;
>>>>> +        else
>>>>> +        {
>>>>> +            if ( rmrr_map == NULL )
>>>>> +            {
>>>>> +                rmrr_map =
>>>>> xmalloc_array(xen_reserved_device_memory_t,
>>>>> +                                         nr_entries);
>>>>
>>>> You can do all of this without any memory allocation.
>>>
>
> What is your way without any memory allocation? Do you mean I should
> predefine a static array here? But how to predetermine the size?
>
> Or you mean I should do something with one rmrr_map, like this,
>
>     struct xen_mem_reserved_device_memory rmrr_map;
>
>     list_for_each_entry( rmrr, &acpi_rmrr_units, list )
>     {
>         rmrr_map.start_pfn = ...;
>         rmrr_map.nr_pages = ...;
>         if ( copy_to_guest_offset(buffer, i, &rmrr_map, 1)
> )            return -EFAULT;
>         i++;
>     }

Yes - that is along the right lines.

~Andrew

Re: [v4][PATCH 4/9] tools:libxc: check if mmio BAR is out of RMRR mappings

[Ian Campbell]

On Fri, 2014-08-22 at 18:09 +0800, Tiejun Chen wrote:

> +    /* We should check if mmio range is out of RMRR mapping.
> +     *
> +     * Assume we have one entry if not enough we'll expand.
> +     */

The usual approach with such hypervisor interfaces (which I suppose
xc_reserved_device_memory_map turns into) is to first call it with NULL
to get the required size and then allocate a suitable buffer and call a
second time.


> +    for ( i = 0; i < rc; i++ )
> +    {
> +        rmrr_start = map[i].pfn << PAGE_SHIFT;
> +        rmrr_end = rmrr_start + map[i].count * PAGE_SIZE;
> +        if ( check_mmio_hole(rmrr_start, map[i].count * PAGE_SIZE,

Adding rmrr_size = map... & PAGE_SIZE could be used twice here.

Ian.

Re: [v4][PATCH 5/9] hvm_info_table: introduce nr_reserved_device_memory_map

[Ian Campbell]

On Fri, 2014-08-22 at 18:09 +0800, Tiejun Chen wrote:
> libxc can expose how many reserved device memory entries
> hvmloader should get.

"get" in what sense?

>  And '0' means that doesn't exist so
> we can skip this check.

Which check?

The code is trivial enough that I guess it is correct, and I don't
expect a full explanation here (since I assume that comes in some future
patch) but an accurate/meaningful quick indication would be useful here.

Re: [v4][PATCH 4/9] tools:libxc: check if mmio BAR is out of RMRR mappings

[Chen, Tiejun]

On 2014/8/27 4:36, Ian Campbell wrote:
> On Fri, 2014-08-22 at 18:09 +0800, Tiejun Chen wrote:
>
>> +    /* We should check if mmio range is out of RMRR mapping.
>> +     *
>> +     * Assume we have one entry if not enough we'll expand.
>> +     */
>
> The usual approach with such hypervisor interfaces (which I suppose
> xc_reserved_device_memory_map turns into) is to first call it with NULL
> to get the required size and then allocate a suitable buffer and call a
> second time.

Ofentimes, RMRR should be rare with one or two entries, even zero. So I 
think its reasonable to start posting one entry since this can cover 
such a scenario the platform really owns one entry.

>
>
>> +    for ( i = 0; i < rc; i++ )
>> +    {
>> +        rmrr_start = map[i].pfn << PAGE_SHIFT;
>> +        rmrr_end = rmrr_start + map[i].count * PAGE_SIZE;
>> +        if ( check_mmio_hole(rmrr_start, map[i].count * PAGE_SIZE,
>
> Adding rmrr_size = map... & PAGE_SIZE could be used twice here.
>

Yes, so I think the follows should be expected:

--- a/tools/libxc/xc_hvm_build_x86.c
+++ b/tools/libxc/xc_hvm_build_x86.c
@@ -251,7 +251,7 @@ static int check_rmrr_overlap(xc_interface *xch, 
uint64_t mmio_start,
                                uint64_t mmio_size)
  {
      struct xen_mem_reserved_device_memory *map = NULL;
-    uint64_t rmrr_start = 0, rmrr_end = 0;
+    uint64_t rmrr_start = 0, rmrr_end = 0, rmrr_size = 0;
      unsigned int i = 0;
      int rc = 0;
      /* Assume we have one entry if not enough we'll expand.*/
@@ -295,9 +295,9 @@ static int check_rmrr_overlap(xc_interface *xch, 
uint64_t mmio_start,
      for ( i = 0; i < rc; i++ )
      {
          rmrr_start = map[i].start_pfn << PAGE_SHIFT;
-        rmrr_end = rmrr_start + map[i].nr_pages * PAGE_SIZE;
-        if ( check_mmio_hole(rmrr_start, map[i].nr_pages * PAGE_SIZE,
-                             mmio_start, mmio_size) )
+        rmrr_size = map[i].nr_pages * PAGE_SIZE;
+        rmrr_end = rmrr_start + rmrr_size;
+        if ( check_mmio_hole(rmrr_start, rmrr_size, mmio_start, 
mmio_size) )
          {
              PERROR("MMIO: [%lx]<->[%lx] overlap RMRR [%lx]<->[%lx]\n",
                     mmio_start, (mmio_start + mmio_size), rmrr_start, 
rmrr_end);

Thanks
Tiejun

> Ian.
>
>

Re: [v4][PATCH 5/9] hvm_info_table: introduce nr_reserved_device_memory_map

[Chen, Tiejun]

On 2014/8/27 4:38, Ian Campbell wrote:
> On Fri, 2014-08-22 at 18:09 +0800, Tiejun Chen wrote:
>> libxc can expose how many reserved device memory entries
>> hvmloader should get.
>
> "get" in what sense?
>
>>   And '0' means that doesn't exist so
>> we can skip this check.
>
> Which check?
>
> The code is trivial enough that I guess it is correct, and I don't
> expect a full explanation here (since I assume that comes in some future
> patch) but an accurate/meaningful quick indication would be useful here.
>

Okay, I think I should describe this case explicitly as follows:

     hvm_info_table: introduce nr_reserved_device_memory_map

     libxc can expose how many reserved device memory entries to
     notify hvmloader. Then hvmloader would check if those reserved
     memory overlap current memory range in e820.

     Note if nr_reserved_device_memory_map is '0', this means we have
     any reserved device memory so we can skip that check.

     Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>

Thanks
Tiejun

Re: [v4][PATCH 5/9] hvm_info_table: introduce nr_reserved_device_memory_map

[Chen, Tiejun]

On 2014/8/27 9:54, Chen, Tiejun wrote:
> On 2014/8/27 4:38, Ian Campbell wrote:
>> On Fri, 2014-08-22 at 18:09 +0800, Tiejun Chen wrote:
>>> libxc can expose how many reserved device memory entries
>>> hvmloader should get.
>>
>> "get" in what sense?
>>
>>>   And '0' means that doesn't exist so
>>> we can skip this check.
>>
>> Which check?
>>
>> The code is trivial enough that I guess it is correct, and I don't
>> expect a full explanation here (since I assume that comes in some future
>> patch) but an accurate/meaningful quick indication would be useful here.
>>
>
> Okay, I think I should describe this case explicitly as follows:
>
>      hvm_info_table: introduce nr_reserved_device_memory_map
>
>      libxc can expose how many reserved device memory entries to
>      notify hvmloader. Then hvmloader would check if those reserved
>      memory overlap current memory range in e820.
>
>      Note if nr_reserved_device_memory_map is '0', this means we have
>      any reserved device memory so we can skip that check.

	s/any/no any

Thanks
Tiejun

>
>      Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
>
> Thanks
> Tiejun
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>
>

Re: [v4][PATCH 4/9] tools:libxc: check if mmio BAR is out of RMRR mappings

[Ian Campbell]

On Wed, 2014-08-27 at 09:46 +0800, Chen, Tiejun wrote:
> On 2014/8/27 4:36, Ian Campbell wrote:
> > On Fri, 2014-08-22 at 18:09 +0800, Tiejun Chen wrote:
> >
> >> +    /* We should check if mmio range is out of RMRR mapping.
> >> +     *
> >> +     * Assume we have one entry if not enough we'll expand.
> >> +     */
> >
> > The usual approach with such hypervisor interfaces (which I suppose
> > xc_reserved_device_memory_map turns into) is to first call it with NULL
> > to get the required size and then allocate a suitable buffer and call a
> > second time.
> 
> Ofentimes, RMRR should be rare with one or two entries, even zero.

It's not clear to me what number you are saying is the norm here.

Even if some N is common today what guarantee is there that N won't grow
or shrink with the next generation of systems?

>  So I 
> think its reasonable to start posting one entry since this can cover 
> such a scenario the platform really owns one entry.

Making the call twice is not terribly expensive (nor is this a hot path)
and it allows you to avoid the reallocation and recall and the twisty
error handling structure which that implies.

Ian.

Re: [v4][PATCH 5/9] hvm_info_table: introduce nr_reserved_device_memory_map

[Ian Campbell]

On Wed, 2014-08-27 at 09:54 +0800, Chen, Tiejun wrote:
> On 2014/8/27 4:38, Ian Campbell wrote:
> > On Fri, 2014-08-22 at 18:09 +0800, Tiejun Chen wrote:
> >> libxc can expose how many reserved device memory entries
> >> hvmloader should get.
> >
> > "get" in what sense?
> >
> >>   And '0' means that doesn't exist so
> >> we can skip this check.
> >
> > Which check?
> >
> > The code is trivial enough that I guess it is correct, and I don't
> > expect a full explanation here (since I assume that comes in some future
> > patch) but an accurate/meaningful quick indication would be useful here.
> >
> 
> Okay, I think I should describe this case explicitly as follows:
> 
>      hvm_info_table: introduce nr_reserved_device_memory_map
> 
>      libxc can expose how many reserved device memory entries to
>      notify hvmloader. 

"libxc exposes to hvmloader the number of reserved device memory entries
which are present" is what I think you are trying to say.

> Then hvmloader would check if those reserved

s/would/will/

>      memory overlap current memory range in e820.
> 
>      Note if nr_reserved_device_memory_map is '0', this means we have
>      any reserved device memory so we can skip that check.

s/any/no/ ?

Ian.

Re: [v4][PATCH 5/9] hvm_info_table: introduce nr_reserved_device_memory_map

[Chen, Tiejun]

On 2014/8/27 10:21, Ian Campbell wrote:
> On Wed, 2014-08-27 at 09:54 +0800, Chen, Tiejun wrote:
>> On 2014/8/27 4:38, Ian Campbell wrote:
>>> On Fri, 2014-08-22 at 18:09 +0800, Tiejun Chen wrote:
>>>> libxc can expose how many reserved device memory entries
>>>> hvmloader should get.
>>>
>>> "get" in what sense?
>>>
>>>>    And '0' means that doesn't exist so
>>>> we can skip this check.
>>>
>>> Which check?
>>>
>>> The code is trivial enough that I guess it is correct, and I don't
>>> expect a full explanation here (since I assume that comes in some future
>>> patch) but an accurate/meaningful quick indication would be useful here.
>>>
>>
>> Okay, I think I should describe this case explicitly as follows:
>>
>>       hvm_info_table: introduce nr_reserved_device_memory_map
>>
>>       libxc can expose how many reserved device memory entries to
>>       notify hvmloader.
>
> "libxc exposes to hvmloader the number of reserved device memory entries
> which are present" is what I think you are trying to say.
>
>> Then hvmloader would check if those reserved
>
> s/would/will/
>
>>       memory overlap current memory range in e820.
>>
>>       Note if nr_reserved_device_memory_map is '0', this means we have
>>       any reserved device memory so we can skip that check.
>
> s/any/no/ ?
>

Thanks for your revision kindly, so now,

     hvm_info_table: introduce nr_reserved_device_memory_map

     libxc exposes to hvmloader how many reserved device memory entries
     which are present. Then hvmloader will check if those reserved
     memory overlap current memory range in e820.

     Note if nr_reserved_device_memory_map is '0', this means we have
     no reserved device memory so we can skip that check.

     Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>

Thanks
Tiejun

Re: [v4][PATCH 4/9] tools:libxc: check if mmio BAR is out of RMRR mappings

[Chen, Tiejun]

On 2014/8/27 10:20, Ian Campbell wrote:
> On Wed, 2014-08-27 at 09:46 +0800, Chen, Tiejun wrote:
>> On 2014/8/27 4:36, Ian Campbell wrote:
>>> On Fri, 2014-08-22 at 18:09 +0800, Tiejun Chen wrote:
>>>
>>>> +    /* We should check if mmio range is out of RMRR mapping.
>>>> +     *
>>>> +     * Assume we have one entry if not enough we'll expand.
>>>> +     */
>>>
>>> The usual approach with such hypervisor interfaces (which I suppose
>>> xc_reserved_device_memory_map turns into) is to first call it with NULL
>>> to get the required size and then allocate a suitable buffer and call a
>>> second time.
>>
>> Ofentimes, RMRR should be rare with one or two entries, even zero.
>
> It's not clear to me what number you are saying is the norm here.

In my broadwell platform we just have two entries.

>
> Even if some N is common today what guarantee is there that N won't grow
> or shrink with the next generation of systems?

As I understand RMRR may be legacy, and this should go away.

What is RMRR?
-------------

There are some devices the BIOS controls, for e.g USB devices to perform
PS2 emulation. The regions of memory used for these devices are marked
reserved in the e820 map. When we turn on DMA translation, DMA to those
regions will fail. Hence BIOS uses RMRR to specify these regions along 
with devices that need to access these regions. OS is expected to setup
unity mappings for these regions for these devices to access these regions.

>
>>   So I
>> think its reasonable to start posting one entry since this can cover
>> such a scenario the platform really owns one entry.
>
> Making the call twice is not terribly expensive (nor is this a hot path)
> and it allows you to avoid the reallocation and recall and the twisty
> error handling structure which that implies.
>

As I understand when we call one given hypercall, we may know that 
possible numbers to issue that. Then we can get the appropriate number 
via the returned value if that is not enough. I think its better than we 
always issue twice hypercall unconditionally :)

But if you persist in this fixed twice-call mechanism, I can try to 
rework out this implementation :)

Thanks
Tiejun

Re: [v4][PATCH 4/9] tools:libxc: check if mmio BAR is out of RMRR mappings

[Chen, Tiejun]

On 2014/8/27 10:40, Chen, Tiejun wrote:
> On 2014/8/27 10:20, Ian Campbell wrote:
>> On Wed, 2014-08-27 at 09:46 +0800, Chen, Tiejun wrote:
>>> On 2014/8/27 4:36, Ian Campbell wrote:
>>>> On Fri, 2014-08-22 at 18:09 +0800, Tiejun Chen wrote:
>>>>
>>>>> +    /* We should check if mmio range is out of RMRR mapping.
>>>>> +     *
>>>>> +     * Assume we have one entry if not enough we'll expand.
>>>>> +     */
>>>>
>>>> The usual approach with such hypervisor interfaces (which I suppose
>>>> xc_reserved_device_memory_map turns into) is to first call it with NULL
>>>> to get the required size and then allocate a suitable buffer and call a
>>>> second time.
>>>
>>> Ofentimes, RMRR should be rare with one or two entries, even zero.
>>
>> It's not clear to me what number you are saying is the norm here.
>
> In my broadwell platform we just have two entries.
>
>>
>> Even if some N is common today what guarantee is there that N won't grow
>> or shrink with the next generation of systems?
>
> As I understand RMRR may be legacy, and this should go away.
>
> What is RMRR?
> -------------
>
> There are some devices the BIOS controls, for e.g USB devices to perform
> PS2 emulation. The regions of memory used for these devices are marked
> reserved in the e820 map. When we turn on DMA translation, DMA to those
> regions will fail. Hence BIOS uses RMRR to specify these regions along
> with devices that need to access these regions. OS is expected to setup
> unity mappings for these regions for these devices to access these regions.
>
>>
>>>   So I
>>> think its reasonable to start posting one entry since this can cover
>>> such a scenario the platform really owns one entry.
>>
>> Making the call twice is not terribly expensive (nor is this a hot path)
>> and it allows you to avoid the reallocation and recall and the twisty
>> error handling structure which that implies.
>>
>
> As I understand when we call one given hypercall, we may know that
> possible numbers to issue that. Then we can get the appropriate number
> via the returned value if that is not enough. I think its better than we
> always issue twice hypercall unconditionally :)
>
> But if you persist in this fixed twice-call mechanism, I can try to
> rework out this implementation :)
>

And actually, I think current implementation is already as you expect. 
Please check patch #3,

@@ -4842,6 +4843,55 @@ long arch_memory_op(unsigned long cmd, 
XEN_GUEST_HANDLE_PARAM(void) arg)
          return rc;
      }

+    case XENMEM_reserved_device_memory_map:
+    {
+        struct xen_mem_reserved_device_memory_map map;
+        XEN_GUEST_HANDLE(xen_mem_reserved_device_memory_t) buffer;
+        XEN_GUEST_HANDLE_PARAM(xen_mem_reserved_device_memory_t) 
buffer_param;
+        unsigned int i = 0;
+        struct xen_mem_reserved_device_memory rmrr_map;
+        struct acpi_rmrr_unit *rmrr;
+
+        if ( !acpi_rmrr_unit_entries )
+                return -ENOENT;
+
+        if ( copy_from_guest(&map, arg, 1) )
+            return -EFAULT;
+
+        if ( map.nr_entries < acpi_rmrr_unit_entries )
+        {
+            map.nr_entries = acpi_rmrr_unit_entries;
+            if ( copy_to_guest(arg, &map, 1) )
+                return -EFAULT;
+            return -ENOBUFS;
+        }

And turn back, here I just set map.nr_entries = 1, not '0' as you image.

+    /* Assume we have one entry if not enough we'll expand.*/
+    uint32_t nr_entries = 1;
+
+    /* We should check if mmio range is out of RMRR mapping. */
+    if ( (map = malloc(nr_entries *
+                       sizeof(xen_mem_reserved_device_memory_t))) == NULL )

So you really hope I should set map.nr_entries = 0 firstly?

Thanks
Tiejun