[v5][PATCH 0/10] xen: reserve RMRR to avoid conflicting MMIO/RAM

[v5][PATCH 0/10] xen: reserve RMRR to avoid conflicting MMIO/RAM

[Tiejun Chen]

v5:

* Add patch #2 to introduce a global count, acpi_rmrr_unit_entries. 
* Refine hypercall return value to make sure the caller can distinguish
  clearly between "xen filled in N entries" and "xen said you need N
  entries for all information".
* Refine some structures
* Then Rebase 

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 (10):
      xen:vtd:rmrr: export acpi_rmrr_units
      xen:vtd:rmrr: introduce acpi_rmrr_unit_entries
      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          |  83 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 tools/libxc/xenctrl.h                   |   4 ++++
 xen/arch/x86/mm.c                       |  49 +++++++++++++++++++++++++++++++++++++++++++++++++
 xen/arch/x86/x86_64/compat/mm.c         |  40 ++++++++++++++++++++++++++++++++++++++++
 xen/drivers/passthrough/vtd/dmar.c      |   2 ++
 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              |  18 ++++++++++++++++++
 xen/include/public/hvm/hvm_info_table.h |   3 +++
 xen/include/public/memory.h             |  39 ++++++++++++++++++++++++++++++++++++++-
 15 files changed, 399 insertions(+), 27 deletions(-)

Thanks
Tiejun

[v5][PATCH 01/10] 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

[v5][PATCH 02/10] xen:vtd:rmrr: introduce acpi_rmrr_unit_entries

[Tiejun Chen]

We need a global count, acpi_rmrr_unit_entries, to record the number
of rmrr entries, then we can use that somewhere else conveniently.

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

diff --git a/xen/drivers/passthrough/vtd/dmar.c b/xen/drivers/passthrough/vtd/dmar.c
index 1152c3a..c1b62a8 100644
--- a/xen/drivers/passthrough/vtd/dmar.c
+++ b/xen/drivers/passthrough/vtd/dmar.c
@@ -74,8 +74,10 @@ static int __init acpi_register_drhd_unit(struct acpi_drhd_unit *drhd)
     return 0;
 }
 
+uint32_t acpi_rmrr_unit_entries = 0;
 static int __init acpi_register_rmrr_unit(struct acpi_rmrr_unit *rmrr)
 {
+    acpi_rmrr_unit_entries++;
     list_add(&rmrr->list, &acpi_rmrr_units);
     return 0;
 }
diff --git a/xen/include/asm-x86/acpi.h b/xen/include/asm-x86/acpi.h
index b3c9e90..6b9d3ab 100644
--- a/xen/include/asm-x86/acpi.h
+++ b/xen/include/asm-x86/acpi.h
@@ -181,4 +181,5 @@ void save_rest_processor_state(void);
 void restore_rest_processor_state(void);
 
 extern struct list_head acpi_rmrr_units;
+extern uint32_t acpi_rmrr_unit_entries;
 #endif /*__X86_ASM_ACPI_H*/
-- 
1.9.1

[v5][PATCH 03/10] 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           | 49 +++++++++++++++++++++++++++++++++++++++++++++
 xen/include/public/memory.h | 39 +++++++++++++++++++++++++++++++++++-
 2 files changed, 87 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index d23cb3f..f54294c 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,54 @@ 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;
+        static struct xen_mem_reserved_device_memory rmrr_map;
+        struct acpi_rmrr_unit *rmrr;
+
+        if ( copy_from_guest(&map, arg, 1) )
+            return -EFAULT;
+
+        if ( !acpi_rmrr_unit_entries )
+                return -ENOENT;
+
+        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;
+        }
+
+        map.nr_entries = acpi_rmrr_unit_entries;
+        buffer_param = guest_handle_cast(map.buffer,
+                                         xen_mem_reserved_device_memory_t);
+        buffer = guest_handle_from_param(buffer_param,
+                                         xen_mem_reserved_device_memory_t);
+        if ( !guest_handle_okay(buffer, map.nr_entries) )
+            return -EFAULT;
+
+        list_for_each_entry( rmrr, &acpi_rmrr_units, list )
+        {
+            rmrr_map.start_pfn = rmrr->base_address >> PAGE_SHIFT;
+            rmrr_map.nr_pages = PAGE_ALIGN(rmrr->end_address -
+                                           rmrr->base_address) /
+                                           PAGE_SIZE;
+            if ( copy_to_guest_offset(buffer, i, &rmrr_map, 1) )
+                return -EFAULT;
+            i++;
+        }
+
+        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..47ce8b2 100644
--- a/xen/include/public/memory.h
+++ b/xen/include/public/memory.h
@@ -523,7 +523,44 @@ DEFINE_XEN_GUEST_HANDLE(xen_mem_sharing_op_t);
 
 #endif /* defined(__XEN__) || defined(__XEN_TOOLS__) */
 
-/* Next available subop number is 26 */
+/*
+ * 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_mem_reserved_device_memory {
+    /* Start PFN of the current mapping of the page. */
+    xen_pfn_t start_pfn;
+    /* Number of the current mapping pages. */
+    xen_ulong_t nr_pages;
+};
+typedef struct xen_mem_reserved_device_memory xen_mem_reserved_device_memory_t;
+DEFINE_XEN_GUEST_HANDLE(xen_mem_reserved_device_memory_t);
+
+struct xen_mem_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_mem_reserved_device_memory.
+     */
+    XEN_GUEST_HANDLE(void) buffer;
+};
+typedef struct xen_mem_reserved_device_memory_map xen_mem_reserved_device_memory_map_t;
+DEFINE_XEN_GUEST_HANDLE(xen_mem_reserved_device_memory_map_t);
+
+/* Next available subop number is 27 */
 
 #endif /* __XEN_PUBLIC_MEMORY_H__ */
 
-- 
1.9.1

[v5][PATCH 04/10] 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..b9374d2 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_mem_reserved_device_memory entries[],
+                                  uint32_t *max_entries)
+{
+    int rc;
+    struct xen_mem_reserved_device_memory_map memmap = {
+        .nr_entries = *max_entries
+    };
+    DECLARE_HYPERCALL_BOUNCE(entries,
+                             sizeof(struct xen_mem_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 )
+        *max_entries = 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..aa32c81 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_mem_reserved_device_memory entries[],
+                                  uint32_t *max_entries);
 #endif
 int xc_domain_set_time_offset(xc_interface *xch,
                               uint32_t domid,
-- 
1.9.1

[v5][PATCH 05/10] 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 | 71 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 71 insertions(+)

diff --git a/tools/libxc/xc_hvm_build_x86.c b/tools/libxc/xc_hvm_build_x86.c
index c81a25b..612cafa 100644
--- a/tools/libxc/xc_hvm_build_x86.c
+++ b/tools/libxc/xc_hvm_build_x86.c
@@ -239,6 +239,73 @@ 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_mem_reserved_device_memory *map = NULL;
+    uint64_t rmrr_start = 0, rmrr_end = 0;
+    unsigned int i = 0;
+    int rc = 0;
+    /* 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 )
+    {
+        PERROR("Could not allocate memory for map.");
+        return -1;
+    }
+    rc = xc_reserved_device_memory_map(xch, map, &nr_entries);
+    if ( rc < 0 )
+    {
+        /* RMRR doesn't exist. */
+        if ( rc == -ENOENT )
+            rc = 0;
+        else if ( rc == -ENOBUFS)
+        {
+            free(map);
+            /* Now we need more space to map all RMRR mappings. */
+            if ( (map = malloc(nr_entries *
+                               sizeof(xen_mem_reserved_device_memory_t))) == NULL )
+            {
+                PERROR("Could not allocate memory for map.");
+                return -1;
+            }
+            rc = xc_reserved_device_memory_map(xch, map, &nr_entries);
+            if ( rc < 0 )
+            {
+                PERROR("Could not get RMRR info on domain");
+                free(map);
+                return rc;
+            }
+        }
+        else
+            PERROR("Could not get RMRR info on domain");
+    }
+
+    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) )
+        {
+            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 +367,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

[v5][PATCH 06/10] 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 612cafa..8214780 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];
@@ -329,6 +333,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;
@@ -370,6 +375,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;
@@ -540,7 +548,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

[v5][PATCH 07/10] 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_mem_reserved_device_memory_map and struct
compat_mem_reserved_device_memory.

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

diff --git a/xen/arch/x86/x86_64/compat/mm.c b/xen/arch/x86/x86_64/compat/mm.c
index 69c6195..5758c30 100644
--- a/xen/arch/x86/x86_64/compat/mm.c
+++ b/xen/arch/x86/x86_64/compat/mm.c
@@ -215,6 +215,46 @@ int compat_arch_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
         break;
     }
 
+    case XENMEM_reserved_device_memory_map:
+    {
+        struct compat_mem_reserved_device_memory_map map;
+        unsigned int i = 0;
+        static struct compat_mem_reserved_device_memory rmrr_map;
+        struct acpi_rmrr_unit *rmrr;
+
+        if ( copy_from_guest(&map, arg, 1) )
+            return -EFAULT;
+
+        if ( !acpi_rmrr_unit_entries )
+            return -ENOENT;
+
+        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;
+        }
+
+        map.nr_entries = acpi_rmrr_unit_entries;
+
+        list_for_each_entry( rmrr, &acpi_rmrr_units, list )
+        {
+            rmrr_map.start_pfn = rmrr->base_address >> PAGE_SHIFT;
+            rmrr_map.nr_pages = PAGE_ALIGN(rmrr->end_address -
+                                           rmrr->base_address) /
+                                           PAGE_SIZE;
+            if ( copy_to_compat_offset(map.buffer, i, &rmrr_map, 1) )
+                return -EFAULT;
+            i++;
+        }
+
+        if ( copy_to_guest(arg, &map, 1) )
+            return -EFAULT;
+
+        break;
+    }
+
     default:
         rc = -ENOSYS;
         break;
-- 
1.9.1

[v5][PATCH 08/10] 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..2d2fb7a 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_mem_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..644b557 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 {
+    /* Start PFN of the current mapping of the page. */
+    unsigned int start_pfn;
+    /* Number of the current mapping pages. */
+    unsigned int nr_pages;
+};
+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

[v5][PATCH 09/10] 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..758d46d 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].start_pfn << PAGE_SHIFT;
+        rmrr_end = rmrr_start + map[i].nr_pages * 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

[v5][PATCH 10/10] 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: [v5][PATCH 03/10] xen:x86: define a new hypercall to get RMRR mappings

[Andrew Cooper]

On 26/08/14 12:02, 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           | 49 +++++++++++++++++++++++++++++++++++++++++++++
>  xen/include/public/memory.h | 39 +++++++++++++++++++++++++++++++++++-
>  2 files changed, 87 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> index d23cb3f..f54294c 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,54 @@ 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;
> +        static struct xen_mem_reserved_device_memory rmrr_map;

This absolutely must not be static, as I have indicated twice before. 
Concurrent hypercalls will corrupt it.  Furthermore, its scope can be
reduced to inside the list_for_each() loop

> +        struct acpi_rmrr_unit *rmrr;
> +
> +        if ( copy_from_guest(&map, arg, 1) )
> +            return -EFAULT;
> +
> +        if ( !acpi_rmrr_unit_entries )
> +                return -ENOENT;

The indentation here is wrong.  It would make sense to move this above
the copy_from_guest(), to avoid the copy in the case that there are no
rmrr ranges.

> +
> +        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;
> +        }
> +
> +        map.nr_entries = acpi_rmrr_unit_entries;

Move this assignment below, and use i which is the actual number of
entries written.

~Andrew

> +        buffer_param = guest_handle_cast(map.buffer,
> +                                         xen_mem_reserved_device_memory_t);
> +        buffer = guest_handle_from_param(buffer_param,
> +                                         xen_mem_reserved_device_memory_t);
> +        if ( !guest_handle_okay(buffer, map.nr_entries) )
> +            return -EFAULT;
> +
> +        list_for_each_entry( rmrr, &acpi_rmrr_units, list )
> +        {
> +            rmrr_map.start_pfn = rmrr->base_address >> PAGE_SHIFT;
> +            rmrr_map.nr_pages = PAGE_ALIGN(rmrr->end_address -
> +                                           rmrr->base_address) /
> +                                           PAGE_SIZE;
> +            if ( copy_to_guest_offset(buffer, i, &rmrr_map, 1) )
> +                return -EFAULT;
> +            i++;
> +        }
> +
> +        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..47ce8b2 100644
> --- a/xen/include/public/memory.h
> +++ b/xen/include/public/memory.h
> @@ -523,7 +523,44 @@ DEFINE_XEN_GUEST_HANDLE(xen_mem_sharing_op_t);
>  
>  #endif /* defined(__XEN__) || defined(__XEN_TOOLS__) */
>  
> -/* Next available subop number is 26 */
> +/*
> + * 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_mem_reserved_device_memory {
> +    /* Start PFN of the current mapping of the page. */
> +    xen_pfn_t start_pfn;
> +    /* Number of the current mapping pages. */
> +    xen_ulong_t nr_pages;
> +};
> +typedef struct xen_mem_reserved_device_memory xen_mem_reserved_device_memory_t;
> +DEFINE_XEN_GUEST_HANDLE(xen_mem_reserved_device_memory_t);
> +
> +struct xen_mem_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_mem_reserved_device_memory.
> +     */
> +    XEN_GUEST_HANDLE(void) buffer;
> +};
> +typedef struct xen_mem_reserved_device_memory_map xen_mem_reserved_device_memory_map_t;
> +DEFINE_XEN_GUEST_HANDLE(xen_mem_reserved_device_memory_map_t);
> +
> +/* Next available subop number is 27 */
>  
>  #endif /* __XEN_PUBLIC_MEMORY_H__ */
>  

Re: [v5][PATCH 03/10] xen:x86: define a new hypercall to get RMRR mappings

[Jan Beulich]

>>> On 26.08.14 at 14:02, <andrew.cooper3@citrix.com> wrote:
> On 26/08/14 12:02, Tiejun Chen wrote:
>> +    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;
>> +        static struct xen_mem_reserved_device_memory rmrr_map;
> 
> This absolutely must not be static, as I have indicated twice before. 

Indeed, you should be reading what you're being told, and not
submit new patch versions without having addressed (by altering
the patch or addressing the comment verbally) previous comments.
For example, I had also asked you to adjust your patch titles, yet
they still come in the same bogus form (namely with colons rather
than slashes as prefix separators - this ones should e.g. start with
"xen/x86:", albeit I personally dislike the xen/ prefix and tend to
strip it).

It is for that reason that I didn't comment on the last two or so
iterations of your patch series - the effort the repeated pointing
out of issues causes just makes it sit at the very end of the list of
things needing looking at).

Jan

Re: [v5][PATCH 03/10] xen:x86: define a new hypercall to get RMRR mappings

[Chen, Tiejun]

On 2014/8/26 20:02, Andrew Cooper wrote:
> On 26/08/14 12:02, 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           | 49 +++++++++++++++++++++++++++++++++++++++++++++
>>   xen/include/public/memory.h | 39 +++++++++++++++++++++++++++++++++++-
>>   2 files changed, 87 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
>> index d23cb3f..f54294c 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,54 @@ 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;
>> +        static struct xen_mem_reserved_device_memory rmrr_map;
>
> This absolutely must not be static, as I have indicated twice before.

Sorry this is a typo. Looks I really forget to remove 'static' here, 
just remember to work without ant memory allocation.

> Concurrent hypercalls will corrupt it.  Furthermore, its scope can be
> reduced to inside the list_for_each() loop
>
>> +        struct acpi_rmrr_unit *rmrr;
>> +
>> +        if ( copy_from_guest(&map, arg, 1) )
>> +            return -EFAULT;
>> +
>> +        if ( !acpi_rmrr_unit_entries )
>> +                return -ENOENT;
>
> The indentation here is wrong.  It would make sense to move this above
> the copy_from_guest(), to avoid the copy in the case that there are no
> rmrr ranges.
>

Okay.

>> +
>> +        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;
>> +        }
>> +
>> +        map.nr_entries = acpi_rmrr_unit_entries;
>
> Move this assignment below, and use i which is the actual number of
> entries written.

Okay, so what about this?

@@ -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;
+        }
+
+        buffer_param = guest_handle_cast(map.buffer,
+                                         xen_mem_reserved_device_memory_t);
+        buffer = guest_handle_from_param(buffer_param,
+                                         xen_mem_reserved_device_memory_t);
+        if ( !guest_handle_okay(buffer, map.nr_entries) )
+            return -EFAULT;
+
+        list_for_each_entry( rmrr, &acpi_rmrr_units, list )
+        {
+            rmrr_map.start_pfn = rmrr->base_address >> PAGE_SHIFT;
+            rmrr_map.nr_pages = PAGE_ALIGN(rmrr->end_address -
+                                           rmrr->base_address) /
+                                           PAGE_SIZE;
+            if ( copy_to_guest_offset(buffer, i, &rmrr_map, 1) )
+                return -EFAULT;
+            i++;
+        }
+
+        map.nr_entries = i;
+
+        if ( copy_to_guest(arg, &map, 1) )
+                return -EFAULT;
+
+        return 0;
+    }
+
      default:
          return subarch_memory_op(cmd, arg);
      }

Thanks
Tiejun

>
> ~Andrew
>
>> +        buffer_param = guest_handle_cast(map.buffer,
>> +                                         xen_mem_reserved_device_memory_t);
>> +        buffer = guest_handle_from_param(buffer_param,
>> +                                         xen_mem_reserved_device_memory_t);
>> +        if ( !guest_handle_okay(buffer, map.nr_entries) )
>> +            return -EFAULT;
>> +
>> +        list_for_each_entry( rmrr, &acpi_rmrr_units, list )
>> +        {
>> +            rmrr_map.start_pfn = rmrr->base_address >> PAGE_SHIFT;
>> +            rmrr_map.nr_pages = PAGE_ALIGN(rmrr->end_address -
>> +                                           rmrr->base_address) /
>> +                                           PAGE_SIZE;
>> +            if ( copy_to_guest_offset(buffer, i, &rmrr_map, 1) )
>> +                return -EFAULT;
>> +            i++;
>> +        }
>> +
>> +        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..47ce8b2 100644
>> --- a/xen/include/public/memory.h
>> +++ b/xen/include/public/memory.h
>> @@ -523,7 +523,44 @@ DEFINE_XEN_GUEST_HANDLE(xen_mem_sharing_op_t);
>>
>>   #endif /* defined(__XEN__) || defined(__XEN_TOOLS__) */
>>
>> -/* Next available subop number is 26 */
>> +/*
>> + * 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_mem_reserved_device_memory {
>> +    /* Start PFN of the current mapping of the page. */
>> +    xen_pfn_t start_pfn;
>> +    /* Number of the current mapping pages. */
>> +    xen_ulong_t nr_pages;
>> +};
>> +typedef struct xen_mem_reserved_device_memory xen_mem_reserved_device_memory_t;
>> +DEFINE_XEN_GUEST_HANDLE(xen_mem_reserved_device_memory_t);
>> +
>> +struct xen_mem_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_mem_reserved_device_memory.
>> +     */
>> +    XEN_GUEST_HANDLE(void) buffer;
>> +};
>> +typedef struct xen_mem_reserved_device_memory_map xen_mem_reserved_device_memory_map_t;
>> +DEFINE_XEN_GUEST_HANDLE(xen_mem_reserved_device_memory_map_t);
>> +
>> +/* Next available subop number is 27 */
>>
>>   #endif /* __XEN_PUBLIC_MEMORY_H__ */
>>
>
>

Re: [v5][PATCH 03/10] xen:x86: define a new hypercall to get RMRR mappings

[Chen, Tiejun]

On 2014/8/26 20:37, Jan Beulich wrote:
>>>> On 26.08.14 at 14:02, <andrew.cooper3@citrix.com> wrote:
>> On 26/08/14 12:02, Tiejun Chen wrote:
>>> +    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;
>>> +        static struct xen_mem_reserved_device_memory rmrr_map;
>>
>> This absolutely must not be static, as I have indicated twice before.
>
> Indeed, you should be reading what you're being told, and not
> submit new patch versions without having addressed (by altering
> the patch or addressing the comment verbally) previous comments.

This is my typo. I already see Andrew's comment to work that without any 
memory allocation, but really forget to remove 'static'.

> For example, I had also asked you to adjust your patch titles, yet

Are you sure? I recheck all e-mails you replied to me but I don't find 
this comment.

> they still come in the same bogus form (namely with colons rather
> than slashes as prefix separators - this ones should e.g. start with
> "xen/x86:", albeit I personally dislike the xen/ prefix and tend to
> strip it).

Anyway, I think you'd like to change all titles as follows:

1> xen/vtd/rmrr: export acpi_rmrr_units
2> xen/vtd/rmrr: introduce acpi_rmrr_unit_entries
3> xen/x86: define a new hypercall to get RMRR mappings
4> tools/libxc: introduce hypercall for xc_reserved_device_memory_map
5> tools/libxc: check if mmio BAR is out of RMRR mappings
6> hvm_info_table: introduce nr_reserved_device_memory_map
7> xen/x86: support xc_reserved_device_memory_map in compat case
8> tools/firmware/hvmloader: introduce hypercall for
  xc_reserved_device_memory_map
9> tools/firmware/hvmloader: check to reserve RMRR
  mappings in e820
10> xen/vtd: make USB RMRR mapping safe

>
> It is for that reason that I didn't comment on the last two or so
> iterations of your patch series - the effort the repeated pointing
> out of issues causes just makes it sit at the very end of the list of
> things needing looking at).
>

I always read to address all comments but looks I may be missing 
something, I think I should check carefully.

Thanks
Tiejun

> Jan
>
>

Re: [v5][PATCH 03/10] xen:x86: define a new hypercall to get RMRR mappings

[Jan Beulich]

>>> On 27.08.14 at 03:37, <tiejun.chen@intel.com> wrote:
> On 2014/8/26 20:37, Jan Beulich wrote:
>> For example, I had also asked you to adjust your patch titles, yet
> 
> Are you sure? I recheck all e-mails you replied to me but I don't find 
> this comment.

http://lists.xenproject.org/archives/html/xen-devel/2014-08/msg00925.html

>> they still come in the same bogus form (namely with colons rather
>> than slashes as prefix separators - this ones should e.g. start with
>> "xen/x86:", albeit I personally dislike the xen/ prefix and tend to
>> strip it).
> 
> Anyway, I think you'd like to change all titles as follows:

Along those lines, albeit some of the prefixes continue to be
overly long:

> 1> xen/vtd/rmrr: export acpi_rmrr_units
> 2> xen/vtd/rmrr: introduce acpi_rmrr_unit_entries

In these two, the trailing rmrr is meaningless: rmrr is not really
a sub-component, and the rest of the title already establishes
enough context.

> 3> xen/x86: define a new hypercall to get RMRR mappings

To avoid needless extra rounds, iirc the hypercall was requested
to no longer be RMRR-centric. Hence the title shouldn't be either.

> 4> tools/libxc: introduce hypercall for xc_reserved_device_memory_map
> 5> tools/libxc: check if mmio BAR is out of RMRR mappings

Similarly, this wouldn't be RMRR specific the either.

> 6> hvm_info_table: introduce nr_reserved_device_memory_map

Here the prefix is rather odd: Knowing most of the sub-component
placement throughout the code base quite well, I can't really identify
which sub-component this is about. Please remember that the
primary purpose of these prefixes is to make it easy to identify
roughly which area of the code base a change affects. Hence this
should neither be too fine grained (like you seemed to be picking
e.g. individual header file names as prefixes) nor too coarse grained.

Just take on for yourself the viewing angle a maintainer or potential
reviewer would take: Is this an area I should be looking at or I am
interested in?

> 7> xen/x86: support xc_reserved_device_memory_map in compat case
> 8> tools/firmware/hvmloader: introduce hypercall for
>   xc_reserved_device_memory_map
> 9> tools/firmware/hvmloader: check to reserve RMRR
>   mappings in e820

For these two just "hvmloader:" would seem to provide enough
context.

Jan

> 10> xen/vtd: make USB RMRR mapping safe

Re: [v5][PATCH 03/10] xen:x86: define a new hypercall to get RMRR mappings

[Chen, Tiejun]

On 2014/8/27 14:51, Jan Beulich wrote:
>>>> On 27.08.14 at 03:37, <tiejun.chen@intel.com> wrote:
>> On 2014/8/26 20:37, Jan Beulich wrote:
>>> For example, I had also asked you to adjust your patch titles, yet
>>
>> Are you sure? I recheck all e-mails you replied to me but I don't find
>> this comment.
>
> http://lists.xenproject.org/archives/html/xen-devel/2014-08/msg00925.html
>

Seems in another e-mail thread, but I guess '(not just here)' means that 
should be valid here as well.

>>> they still come in the same bogus form (namely with colons rather
>>> than slashes as prefix separators - this ones should e.g. start with
>>> "xen/x86:", albeit I personally dislike the xen/ prefix and tend to
>>> strip it).
>>
>> Anyway, I think you'd like to change all titles as follows:
>
> Along those lines, albeit some of the prefixes continue to be
> overly long:
>
>> 1> xen/vtd/rmrr: export acpi_rmrr_units
>> 2> xen/vtd/rmrr: introduce acpi_rmrr_unit_entries
>
> In these two, the trailing rmrr is meaningless: rmrr is not really
> a sub-component, and the rest of the title already establishes
> enough context.

Right.

1> xen/vtd: export acpi_rmrr_units
2> xen/vtd: introduce acpi_rmrr_unit_entries

>
>> 3> xen/x86: define a new hypercall to get RMRR mappings
>
> To avoid needless extra rounds, iirc the hypercall was requested
> to no longer be RMRR-centric. Hence the title shouldn't be either.
>
>> 4> tools/libxc: introduce hypercall for xc_reserved_device_memory_map
>> 5> tools/libxc: check if mmio BAR is out of RMRR mappings
>
> Similarly, this wouldn't be RMRR specific the either.

3> xen/x86: define a new hypercall to get reserved device memory map

>
>> 6> hvm_info_table: introduce nr_reserved_device_memory_map
>
> Here the prefix is rather odd: Knowing most of the sub-component
> placement throughout the code base quite well, I can't really identify
> which sub-component this is about. Please remember that the
> primary purpose of these prefixes is to make it easy to identify
> roughly which area of the code base a change affects. Hence this
> should neither be too fine grained (like you seemed to be picking
> e.g. individual header file names as prefixes) nor too coarse grained.

Agreed, so thanks for your guide.

>
> Just take on for yourself the viewing angle a maintainer or potential
> reviewer would take: Is this an area I should be looking at or I am
> interested in?

Hope this is fine,

6> tools/libxc: introduce nr_reserved_device_memory_map into hvm_info_table

>
>> 7> xen/x86: support xc_reserved_device_memory_map in compat case
>> 8> tools/firmware/hvmloader: introduce hypercall for
>>    xc_reserved_device_memory_map
>> 9> tools/firmware/hvmloader: check to reserve RMRR
>>    mappings in e820
>
> For these two just "hvmloader:" would seem to provide enough
> context.

8> hvmloader: check to reserve all device reserved memory in e820
9> hvmloader: introduce hypercall for xc_reserved_device_memory_map

Thanks
Tiejun

Re: [v5][PATCH 03/10] xen:x86: define a new hypercall to get RMRR mappings

[Chen, Tiejun]

On 2014/8/27 15:21, Chen, Tiejun wrote:
> On 2014/8/27 14:51, Jan Beulich wrote:
>>>>> On 27.08.14 at 03:37, <tiejun.chen@intel.com> wrote:
>>> On 2014/8/26 20:37, Jan Beulich wrote:
>>>> For example, I had also asked you to adjust your patch titles, yet
>>>
>>> Are you sure? I recheck all e-mails you replied to me but I don't find
>>> this comment.
>>
>> http://lists.xenproject.org/archives/html/xen-devel/2014-08/msg00925.html
>>
>
> Seems in another e-mail thread, but I guess '(not just here)' means that
> should be valid here as well.
>
>>>> they still come in the same bogus form (namely with colons rather
>>>> than slashes as prefix separators - this ones should e.g. start with
>>>> "xen/x86:", albeit I personally dislike the xen/ prefix and tend to
>>>> strip it).
>>>
>>> Anyway, I think you'd like to change all titles as follows:
>>
>> Along those lines, albeit some of the prefixes continue to be
>> overly long:
>>
>>> 1> xen/vtd/rmrr: export acpi_rmrr_units
>>> 2> xen/vtd/rmrr: introduce acpi_rmrr_unit_entries
>>
>> In these two, the trailing rmrr is meaningless: rmrr is not really
>> a sub-component, and the rest of the title already establishes
>> enough context.
>
> Right.
>
> 1> xen/vtd: export acpi_rmrr_units
> 2> xen/vtd: introduce acpi_rmrr_unit_entries
>
>>
>>> 3> xen/x86: define a new hypercall to get RMRR mappings
>>
>> To avoid needless extra rounds, iirc the hypercall was requested
>> to no longer be RMRR-centric. Hence the title shouldn't be either.
>>
>>> 4> tools/libxc: introduce hypercall for xc_reserved_device_memory_map
>>> 5> tools/libxc: check if mmio BAR is out of RMRR mappings
>>
>> Similarly, this wouldn't be RMRR specific the either.
>
> 3> xen/x86: define a new hypercall to get reserved device memory map
>
>>
>>> 6> hvm_info_table: introduce nr_reserved_device_memory_map
>>
>> Here the prefix is rather odd: Knowing most of the sub-component
>> placement throughout the code base quite well, I can't really identify
>> which sub-component this is about. Please remember that the
>> primary purpose of these prefixes is to make it easy to identify
>> roughly which area of the code base a change affects. Hence this
>> should neither be too fine grained (like you seemed to be picking
>> e.g. individual header file names as prefixes) nor too coarse grained.
>
> Agreed, so thanks for your guide.
>
>>
>> Just take on for yourself the viewing angle a maintainer or potential
>> reviewer would take: Is this an area I should be looking at or I am
>> interested in?
>
> Hope this is fine,
>
> 6> tools/libxc: introduce nr_reserved_device_memory_map into hvm_info_table
>
>>
>>> 7> xen/x86: support xc_reserved_device_memory_map in compat case
>>> 8> tools/firmware/hvmloader: introduce hypercall for
>>>    xc_reserved_device_memory_map
>>> 9> tools/firmware/hvmloader: check to reserve RMRR
>>>    mappings in e820
>>
>> For these two just "hvmloader:" would seem to provide enough
>> context.
>
> 8> hvmloader: check to reserve all device reserved memory in e820
> 9> hvmloader: introduce hypercall for xc_reserved_device_memory_map
>
> Thanks
> Tiejun

Jan, Andrew and Ian,

I hope I already cover all comments based v5,

1> Refine patch titles from Jan
2> Improve one patch description from Ian
3> Remove 'static' from Andrew
4> Use i which is the actual number of entries written from Andrew
5> Move checking acpi_rmrr_unit_entries before the copy_from_guest() 
from Andrew.

And I also posted those fixed code fragments inline as well.

If you guys have no more comments, could I send a new series to review?

Thanks
Tiejun

Re: [v5][PATCH 03/10] xen:x86: define a new hypercall to get RMRR mappings

[Jan Beulich]

>>> On 28.08.14 at 04:24, <tiejun.chen@intel.com> wrote:
> If you guys have no more comments, could I send a new series to review?

You certainly can do so at any time, but as said before I didn't get
to looking at the current version yet; briefly having looked at the
first two patches I'm already pretty convinced that the structuring
still isn't right (you shouldn't be exposing VT-d internals into
arbitrary parts of the hypervisor, but rather introduce a new
vendor IOMMU hook/actor to get at the reserved page information
- this all still goes along the lines of you apparently not
understanding that a reasonable level of abstraction helps in the
long term maintenance of such code).

Jan

Re: [v5][PATCH 03/10] xen:x86: define a new hypercall to get RMRR mappings

[Chen, Tiejun]

On 2014/8/28 14:50, Jan Beulich wrote:
>>>> On 28.08.14 at 04:24, <tiejun.chen@intel.com> wrote:
>> If you guys have no more comments, could I send a new series to review?
>
> You certainly can do so at any time, but as said before I didn't get
> to looking at the current version yet; briefly having looked at the

I knew this point so this is just why here I's like to ask if I can send 
new revision. I hope I can do better as you expect.

> first two patches I'm already pretty convinced that the structuring
> still isn't right (you shouldn't be exposing VT-d internals into

If you have any comment, I think you can point inline, then I can take a 
look at that to improve or fix anything as you expect.

As you know, I'm not familiar with Xen codes so sometimes I can't 
understand what you mean properly, even what you were saying. So I have 
to ask you to explain explicitly again.

> arbitrary parts of the hypervisor, but rather introduce a new
> vendor IOMMU hook/actor to get at the reserved page information
> - this all still goes along the lines of you apparently not
> understanding that a reasonable level of abstraction helps in the
> long term maintenance of such code).

If you always say I'm wrong to understand rather than show me something 
explicitly, its still difficult to fine such codes for a fresh man.

Thanks
Tiejun

Re: [v5][PATCH 03/10] xen:x86: define a new hypercall to get RMRR mappings

[Chen, Tiejun]

On 2014/8/28 15:09, Chen, Tiejun wrote:
> On 2014/8/28 14:50, Jan Beulich wrote:
>>>>> On 28.08.14 at 04:24, <tiejun.chen@intel.com> wrote:
>>> If you guys have no more comments, could I send a new series to review?
>>
>> You certainly can do so at any time, but as said before I didn't get
>> to looking at the current version yet; briefly having looked at the
>
> I knew this point so this is just why here I's like to ask if I can send
> new revision. I hope I can do better as you expect.
>
>> first two patches I'm already pretty convinced that the structuring
>> still isn't right (you shouldn't be exposing VT-d internals into
>
> If you have any comment, I think you can point inline, then I can take a
> look at that to improve or fix anything as you expect.
>
> As you know, I'm not familiar with Xen codes so sometimes I can't
> understand what you mean properly, even what you were saying. So I have
> to ask you to explain explicitly again.
>
>> arbitrary parts of the hypervisor, but rather introduce a new

As I remember you or Andrew told me not to use acpi_rmrr_units directly, 
instead I previously introduced a new array to store such RMRR info.

Current first two patches are just introduced from you and Andrew's 
comments.

Thanks
Tiejun

>> vendor IOMMU hook/actor to get at the reserved page information
>> - this all still goes along the lines of you apparently not
>> understanding that a reasonable level of abstraction helps in the
>> long term maintenance of such code).
>
> If you always say I'm wrong to understand rather than show me something
> explicitly, its still difficult to fine such codes for a fresh man.
>
> Thanks
> Tiejun
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>
>

Re: [v5][PATCH 03/10] xen:x86: define a new hypercall to get RMRR mappings

[Chen, Tiejun]

On 2014/8/28 15:19, Chen, Tiejun wrote:
> On 2014/8/28 15:09, Chen, Tiejun wrote:
>> On 2014/8/28 14:50, Jan Beulich wrote:
>>>>>> On 28.08.14 at 04:24, <tiejun.chen@intel.com> wrote:
>>>> If you guys have no more comments, could I send a new series to review?
>>>
>>> You certainly can do so at any time, but as said before I didn't get
>>> to looking at the current version yet; briefly having looked at the
>>
>> I knew this point so this is just why here I's like to ask if I can send
>> new revision. I hope I can do better as you expect.
>>
>>> first two patches I'm already pretty convinced that the structuring
>>> still isn't right (you shouldn't be exposing VT-d internals into
>>
>> If you have any comment, I think you can point inline, then I can take a
>> look at that to improve or fix anything as you expect.
>>
>> As you know, I'm not familiar with Xen codes so sometimes I can't
>> understand what you mean properly, even what you were saying. So I have
>> to ask you to explain explicitly again.
>>
>>> arbitrary parts of the hypervisor, but rather introduce a new
>
> As I remember you or Andrew told me not to use acpi_rmrr_units directly,
> instead I previously introduced a new array to store such RMRR info.
>

"
... 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
"

"
 >> 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.

"

Thanks
Tiejun

> Current first two patches are just introduced from you and Andrew's
> comments.
>
> Thanks
> Tiejun
>
>>> vendor IOMMU hook/actor to get at the reserved page information
>>> - this all still goes along the lines of you apparently not
>>> understanding that a reasonable level of abstraction helps in the
>>> long term maintenance of such code).
>>
>> If you always say I'm wrong to understand rather than show me something
>> explicitly, its still difficult to fine such codes for a fresh man.
>>
>> Thanks
>> Tiejun
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org
>> http://lists.xen.org/xen-devel
>>
>>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>
>

Re: [v5][PATCH 03/10] xen:x86: define a new hypercall to get RMRR mappings

[Jan Beulich]

>>> On 28.08.14 at 09:19, <tiejun.chen@intel.com> wrote:
> On 2014/8/28 15:09, Chen, Tiejun wrote:
>> On 2014/8/28 14:50, Jan Beulich wrote:
>>>>>> On 28.08.14 at 04:24, <tiejun.chen@intel.com> wrote:
>>>> If you guys have no more comments, could I send a new series to review?
>>>
>>> You certainly can do so at any time, but as said before I didn't get
>>> to looking at the current version yet; briefly having looked at the
>>
>> I knew this point so this is just why here I's like to ask if I can send
>> new revision. I hope I can do better as you expect.
>>
>>> first two patches I'm already pretty convinced that the structuring
>>> still isn't right (you shouldn't be exposing VT-d internals into
>>
>> If you have any comment, I think you can point inline, then I can take a
>> look at that to improve or fix anything as you expect.
>>
>> As you know, I'm not familiar with Xen codes so sometimes I can't
>> understand what you mean properly, even what you were saying. So I have
>> to ask you to explain explicitly again.
>>
>>> arbitrary parts of the hypervisor, but rather introduce a new
> 
> As I remember you or Andrew told me not to use acpi_rmrr_units directly, 
> instead I previously introduced a new array to store such RMRR info.

Duplicating information for no reason. Did you check whether adding
a new method to struct iommu_ops couldn't do what you want, at
once retaining proper isolation _and_ not duplicating anything?

Jan

Re: [v5][PATCH 03/10] xen:x86: define a new hypercall to get RMRR mappings

[Chen, Tiejun]

On 2014/8/28 15:44, Jan Beulich wrote:
>>>> On 28.08.14 at 09:19, <tiejun.chen@intel.com> wrote:
>> On 2014/8/28 15:09, Chen, Tiejun wrote:
>>> On 2014/8/28 14:50, Jan Beulich wrote:
>>>>>>> On 28.08.14 at 04:24, <tiejun.chen@intel.com> wrote:
>>>>> If you guys have no more comments, could I send a new series to review?
>>>>
>>>> You certainly can do so at any time, but as said before I didn't get
>>>> to looking at the current version yet; briefly having looked at the
>>>
>>> I knew this point so this is just why here I's like to ask if I can send
>>> new revision. I hope I can do better as you expect.
>>>
>>>> first two patches I'm already pretty convinced that the structuring
>>>> still isn't right (you shouldn't be exposing VT-d internals into
>>>
>>> If you have any comment, I think you can point inline, then I can take a
>>> look at that to improve or fix anything as you expect.
>>>
>>> As you know, I'm not familiar with Xen codes so sometimes I can't
>>> understand what you mean properly, even what you were saying. So I have
>>> to ask you to explain explicitly again.
>>>
>>>> arbitrary parts of the hypervisor, but rather introduce a new
>>
>> As I remember you or Andrew told me not to use acpi_rmrr_units directly,
>> instead I previously introduced a new array to store such RMRR info.
>
> Duplicating information for no reason. Did you check whether adding
> a new method to struct iommu_ops couldn't do what you want, at
> once retaining proper isolation _and_ not duplicating anything?
>

I tried to figure out solution as you suggestion but I'd like show my 
draft design before post anything to review since please give some 
suggestions here:

1. In the xen/include/xen/iommu.h file,

struct iommu_ops {
	...
	int (*get_device_reserved_memory)(struct list_head *dev_reserved_memory);

2. In the xen/drivers/passthrough/vtd/iommu.c file,

extern int get_device_acpi_reserved_memory(struct list_head 
*dev_reserved_memory);

const struct iommu_ops intel_iommu_ops = {
	...
	.get_device_reserved_memory = get_device_acpi_reserved_memory,

3. In the xen/drivers/passthrough/vtd/dmar.c file,

struct list_head devices_reserved_memory = LIST_HEAD_INIT ( 
devices_reserved_memory );
int get_device_acpi_reserved_memory(struct list_head *dev_reserved_memory)
{
     static unsigned int device_reserved_memory_entries = 0;
     static unsigned int check_done = 0;
     struct acpi_rmrr_unit *rmrru;
     struct device_acpi_reserved_memory *darm = NULL;

     dev_reserved_memory = &devices_reserved_memory;

     if ( check_done )
         return device_reserved_memory_entries;
     else
     {
         list_for_each_entry(rmrru, &acpi_rmrr_units, list)
         {
             darm = xzalloc(struct device_acpi_reserved_memory);
             if ( !darm )
                 return -ENOMEM;

             darm->base_address = rmrru->base_address;
             darm->end_address = rmrru->end_address;
             list_add(&darm->list, &devices_reserved_memory);
             device_reserved_memory_entries++;
         }
     }

     check_done = 1;

     return device_reserved_memory_entries;
}

4. In the xen/include/asm-x86/acpi.h file,

+struct device_acpi_reserved_memory {
+    struct list_head list;
+    u64    base_address;
+    u64    end_address;
+};


Here a couple of questions:

1. Here I introduce this struct device_acpi_reserved_memory to avoid 
exposing that existing structure and list acpi_rmrr_units

struct acpi_rmrr_unit {
     struct dmar_scope scope;
     struct list_head list;
     u64    base_address;
     u64    end_address;
     u16    segment;
     u8     allow_all:1;
};

Because:

1> Actually we just need two fields, base_address and end_address.
2> If reuse that structure, we still have to change some head files to 
make sure we can use this in other files like I did in original patch #1 
you don't like.

So what is your idea?

2. Based on your isolation policy, I don't expose acpi_rmrr_units 
directly. Instead, I will copy this to another list, 
devices_reserved_memory as I show above.

Is this reasonable and expected?

3. If #1 and #2 are fine to you, the go the follows"

struct device_acpi_reserved_memory *darm;
int nr_entries = 0;
unsigned int i = 0;
struct list_head *dev_reserved_memory = NULL;
const struct iommu_ops *ops = iommu_get_ops();

if ( ops->get_device_reserved_memory )
{
	nr_entries = ops->get_device_reserved_memory(dev_reserved_memory);
	if ( !nr_entries )
		return -ENOENT;
	else if ( nr_entries < 0 )
                 return -EFAULT;
         }
         else
             return -ENOENT;
}

Any comments are appreciated.

Thanks
Tiejun

Re: [v5][PATCH 03/10] xen:x86: define a new hypercall to get RMRR mappings

[Jan Beulich]

>>> On 29.08.14 at 05:02, <tiejun.chen@intel.com> wrote:
> I tried to figure out solution as you suggestion but I'd like show my 
> draft design before post anything to review since please give some 
> suggestions here:
> 
> 1. In the xen/include/xen/iommu.h file,
> 
> struct iommu_ops {
> 	...
> 	int (*get_device_reserved_memory)(struct list_head *dev_reserved_memory);
> 
> 2. In the xen/drivers/passthrough/vtd/iommu.c file,
> 
> extern int get_device_acpi_reserved_memory(struct list_head 
> *dev_reserved_memory);
> 
> const struct iommu_ops intel_iommu_ops = {
> 	...
> 	.get_device_reserved_memory = get_device_acpi_reserved_memory,
> 
> 3. In the xen/drivers/passthrough/vtd/dmar.c file,
> 
> struct list_head devices_reserved_memory = LIST_HEAD_INIT ( 
> devices_reserved_memory );
> int get_device_acpi_reserved_memory(struct list_head *dev_reserved_memory)
> {
>      static unsigned int device_reserved_memory_entries = 0;
>      static unsigned int check_done = 0;
>      struct acpi_rmrr_unit *rmrru;
>      struct device_acpi_reserved_memory *darm = NULL;
> 
>      dev_reserved_memory = &devices_reserved_memory;
> 
>      if ( check_done )
>          return device_reserved_memory_entries;
>      else
>      {
>          list_for_each_entry(rmrru, &acpi_rmrr_units, list)
>          {
>              darm = xzalloc(struct device_acpi_reserved_memory);
>              if ( !darm )
>                  return -ENOMEM;
> 
>              darm->base_address = rmrru->base_address;
>              darm->end_address = rmrru->end_address;
>              list_add(&darm->list, &devices_reserved_memory);
>              device_reserved_memory_entries++;
>          }
>      }
> 
>      check_done = 1;
> 
>      return device_reserved_memory_entries;
> }
> 
> 4. In the xen/include/asm-x86/acpi.h file,
> 
> +struct device_acpi_reserved_memory {
> +    struct list_head list;
> +    u64    base_address;
> +    u64    end_address;
> +};
> 
> 
> Here a couple of questions:
> 
> 1. Here I introduce this struct device_acpi_reserved_memory to avoid 
> exposing that existing structure and list acpi_rmrr_units
> 
> struct acpi_rmrr_unit {
>      struct dmar_scope scope;
>      struct list_head list;
>      u64    base_address;
>      u64    end_address;
>      u16    segment;
>      u8     allow_all:1;
> };
> 
> Because:
> 
> 1> Actually we just need two fields, base_address and end_address.
> 2> If reuse that structure, we still have to change some head files to 
> make sure we can use this in other files like I did in original patch #1 
> you don't like.
> 
> So what is your idea?
> 
> 2. Based on your isolation policy, I don't expose acpi_rmrr_units 
> directly. Instead, I will copy this to another list, 
> devices_reserved_memory as I show above.
> 
> Is this reasonable and expected?

This still allocates another instance of structures to create a second
linked list. Did you consider get_device_reserved_memory() to take
a callback function instead?

Jan

Re: [v5][PATCH 03/10] xen:x86: define a new hypercall to get RMRR mappings

[Chen, Tiejun]

On 2014/8/29 17:18, Jan Beulich wrote:
>>>> On 29.08.14 at 05:02, <tiejun.chen@intel.com> wrote:
>> I tried to figure out solution as you suggestion but I'd like show my
>> draft design before post anything to review since please give some
>> suggestions here:
>>
>> 1. In the xen/include/xen/iommu.h file,
>>
>> struct iommu_ops {
>> 	...
>> 	int (*get_device_reserved_memory)(struct list_head *dev_reserved_memory);
>>
>> 2. In the xen/drivers/passthrough/vtd/iommu.c file,
>>
>> extern int get_device_acpi_reserved_memory(struct list_head
>> *dev_reserved_memory);
>>
>> const struct iommu_ops intel_iommu_ops = {
>> 	...
>> 	.get_device_reserved_memory = get_device_acpi_reserved_memory,
>>
>> 3. In the xen/drivers/passthrough/vtd/dmar.c file,
>>
>> struct list_head devices_reserved_memory = LIST_HEAD_INIT (
>> devices_reserved_memory );
>> int get_device_acpi_reserved_memory(struct list_head *dev_reserved_memory)
>> {
>>       static unsigned int device_reserved_memory_entries = 0;
>>       static unsigned int check_done = 0;
>>       struct acpi_rmrr_unit *rmrru;
>>       struct device_acpi_reserved_memory *darm = NULL;
>>
>>       dev_reserved_memory = &devices_reserved_memory;
>>
>>       if ( check_done )
>>           return device_reserved_memory_entries;
>>       else
>>       {
>>           list_for_each_entry(rmrru, &acpi_rmrr_units, list)
>>           {
>>               darm = xzalloc(struct device_acpi_reserved_memory);
>>               if ( !darm )
>>                   return -ENOMEM;
>>
>>               darm->base_address = rmrru->base_address;
>>               darm->end_address = rmrru->end_address;
>>               list_add(&darm->list, &devices_reserved_memory);
>>               device_reserved_memory_entries++;
>>           }
>>       }
>>
>>       check_done = 1;
>>
>>       return device_reserved_memory_entries;
>> }
>>
>> 4. In the xen/include/asm-x86/acpi.h file,
>>
>> +struct device_acpi_reserved_memory {
>> +    struct list_head list;
>> +    u64    base_address;
>> +    u64    end_address;
>> +};
>>
>>
>> Here a couple of questions:
>>
>> 1. Here I introduce this struct device_acpi_reserved_memory to avoid
>> exposing that existing structure and list acpi_rmrr_units
>>
>> struct acpi_rmrr_unit {
>>       struct dmar_scope scope;
>>       struct list_head list;
>>       u64    base_address;
>>       u64    end_address;
>>       u16    segment;
>>       u8     allow_all:1;
>> };
>>
>> Because:
>>
>> 1> Actually we just need two fields, base_address and end_address.
>> 2> If reuse that structure, we still have to change some head files to
>> make sure we can use this in other files like I did in original patch #1
>> you don't like.
>>
>> So what is your idea?
>>
>> 2. Based on your isolation policy, I don't expose acpi_rmrr_units
>> directly. Instead, I will copy this to another list,
>> devices_reserved_memory as I show above.
>>
>> Is this reasonable and expected?
>
> This still allocates another instance of structures to create a second
> linked list. Did you consider get_device_reserved_memory() to take

Do you mean we still use this existing type combo, acpi_rmrr_units and 
acpi_rmrr_units?

> a callback function instead?
>

But we should do something like this,

1. .get_device_reserved_memory = get_drm_all,
2.  static int get_drm_all(struct list_head *dev_reserved_memory)
     {
         return (get_drm_callback(dev_reserved_memory));
     }

3. get_drm_callback = get_device_acpi_reserved_memory;
4.  static int get_device_acpi_reserved_memory(struct list_head 
*dev_reserved_memory)
     {
	...
	dev_reserved_memory = &acpi_rmrr_units;
	...
     }

Then while calling the hypercall,

struct list_head *dev_reserved_memory = NULL;
nr_entries = ops->get_device_reserved_memory(dev_reserved_memory);
if (!nr_entries)
	list_for_each_entry( darm, dev_reserved_memory, list )
	{
		xxx.start_pfn = ...;
		xxx.nr_pages = ...;
		if ( copy_to_guest_offset(buffer, i, &xxx, 1) )
		...
	}

Thanks
Tiejun

Re: [v5][PATCH 03/10] xen:x86: define a new hypercall to get RMRR mappings

[Jan Beulich]

>>> On 01.09.14 at 11:44, <tiejun.chen@intel.com> wrote:
> On 2014/8/29 17:18, Jan Beulich wrote:
>>
>> This still allocates another instance of structures to create a second
>> linked list. Did you consider get_device_reserved_memory() to take
> 
> Do you mean we still use this existing type combo, acpi_rmrr_units and 
> acpi_rmrr_units?
> 
>> a callback function instead?
>>
> 
> But we should do something like this,
> 
> 1. .get_device_reserved_memory = get_drm_all,
> 2.  static int get_drm_all(struct list_head *dev_reserved_memory)
>      {
>          return (get_drm_callback(dev_reserved_memory));
>      }
> 
> 3. get_drm_callback = get_device_acpi_reserved_memory;
> 4.  static int get_device_acpi_reserved_memory(struct list_head 
> *dev_reserved_memory)
>      {
> 	...
> 	dev_reserved_memory = &acpi_rmrr_units;
> 	...
>      }
> 
> Then while calling the hypercall,
> 
> struct list_head *dev_reserved_memory = NULL;
> nr_entries = ops->get_device_reserved_memory(dev_reserved_memory);
> if (!nr_entries)
> 	list_for_each_entry( darm, dev_reserved_memory, list )
> 	{
> 		xxx.start_pfn = ...;
> 		xxx.nr_pages = ...;
> 		if ( copy_to_guest_offset(buffer, i, &xxx, 1) )
> 		...
> 	}

Clearly not: The callback ought to be used _while_ processing the
hypercall. And of course the callback shouldn't be used to retrieve
&acpi_rmrr_units, but to report back to the calling entity the
individual regions.

Jan

Re: [v5][PATCH 03/10] xen:x86: define a new hypercall to get RMRR mappings

[Jan Beulich]

>>> On 26.08.14 at 13:02, <tiejun.chen@intel.com> wrote:
> @@ -4842,6 +4843,54 @@ 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;
> +        static struct xen_mem_reserved_device_memory rmrr_map;
> +        struct acpi_rmrr_unit *rmrr;
> +
> +        if ( copy_from_guest(&map, arg, 1) )
> +            return -EFAULT;
> +
> +        if ( !acpi_rmrr_unit_entries )
> +                return -ENOENT;
> +
> +        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;
> +        }
> +
> +        map.nr_entries = acpi_rmrr_unit_entries;
> +        buffer_param = guest_handle_cast(map.buffer,
> +                                         xen_mem_reserved_device_memory_t);
> +        buffer = guest_handle_from_param(buffer_param,
> +                                         xen_mem_reserved_device_memory_t);
> +        if ( !guest_handle_okay(buffer, map.nr_entries) )
> +            return -EFAULT;
> +
> +        list_for_each_entry( rmrr, &acpi_rmrr_units, list )
> +        {
> +            rmrr_map.start_pfn = rmrr->base_address >> PAGE_SHIFT;
> +            rmrr_map.nr_pages = PAGE_ALIGN(rmrr->end_address -
> +                                           rmrr->base_address) /
> +                                           PAGE_SIZE;
> +            if ( copy_to_guest_offset(buffer, i, &rmrr_map, 1) )
> +                return -EFAULT;
> +            i++;
> +        }

So as said before, a callback based mechanism will serve you quite
fine here. However, you're not obeying to the buffer size the
caller passed in (i.e. you may corrupt guest memory). And having
used guest_handle_okay() in the loop (which will get moved into
the callback function) you can then use __copy_to_guest_offset().

> +
> +        if ( copy_to_guest(arg, &map, 1) )
> +                return -EFAULT;

As much as, due to the earlier copy_from_guest() on the same
handle, you can use __copy_to_guest() here. For mechanical
things like these please consult other code - there are plenty of
useful examples throughout the tree.

> +struct xen_mem_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_mem_reserved_device_memory.
> +     */
> +    XEN_GUEST_HANDLE(void) buffer;

I think I had asked before that you use a properly typed handle here,
saving you from needing convoluted conversion code as you have
above.

Jan

Re: [v5][PATCH 06/10] hvm_info_table: introduce nr_reserved_device_memory_map

[Jan Beulich]

>>> On 26.08.14 at 13:02, <tiejun.chen@intel.com> wrote:
> 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.

I assume you introduce this without consumer to limit patch size. In
such a case title _and_ description should be more meaningful as
to what this really does and what it's intended use is.

> @@ -370,6 +375,9 @@ static int setup_guest(xc_interface *xch,
>      rc = check_rmrr_overlap(xch, mmio_start, mmio_start);

I skipped several tools side patches, but here I see that somewhere
you still left the term "RMRR" in the code, when you were asked
before to use more abstract naming (and this of course not only
extended to the public interface).

>      if ( rc < 0 )
>          goto error_out;
> +    /* Always return entries number. */
> +    else

The "else" here is bogus considering the "goto" above.

> --- 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;
>  };

This being defacto a private interface between tools and hvmloader
I wonder if it wouldn't be better to put this before the (in the future)
eventually growing vcpu_online[].

Jan

Re: [v5][PATCH 07/10] xen:x86:: support xc_reserved_device_memory_map in compat case

[Jan Beulich]

>>> On 26.08.14 at 13:02, <tiejun.chen@intel.com> wrote:
> We also need to make sure xc_reserved_device_memory_map can
> work in compat case.

This ought to be part of the patch introducing the new operation.

Jan

[v5][PATCH 08/10] tools:firmware:hvmloader: introduce hypercall for xc_reserved_device_memory_map

[Jan Beulich]

>>> On 26.08.14 at 13:02, <tiejun.chen@intel.com> wrote:
> --- 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_mem_reserved_device_memory_map memmap = {
> +        .nr_entries = max_entries
> +    };
> +
> +    if ( map_done )
> +        return 0;
> +
> +    set_xen_guest_handle(memmap.buffer, entries);

struct reserved_device_memory (the type which entries is a pointer
to) is a privately defined type. You shouldn't pass this to a hypercall.

Jan

[v5][PATCH 09/10] tools:firmware:hvmloader: check to reserve RMRR mappings in e820

[Jan Beulich]

>>> On 26.08.14 at 13:02, <tiejun.chen@intel.com> wrote:
> +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:

Please indent labels by at least one space (and there are further
coding style issues to address elsewhere).

> +    sum_nr = nr + nr_map;

Afaict neither nr nor nr_map change before you get here the second
(and last) time. Hence the calculation can be moved up (ideally into
the initializer of the variable).

> +    for ( i = 0; i < nr_map; i++ )
> +    {
> +        rmrr_start = map[i].start_pfn << PAGE_SHIFT;
> +        rmrr_end = rmrr_start + map[i].nr_pages * PAGE_SIZE;
> +
> +        for ( j = 0; j < nr; j++ )
> +        {
> +            end = e820[j].addr + e820[j].size;
> +            start = e820[j+1].addr;

This is not valid when j == nr - 1 (last iteration).

> +
> +            /* 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

Re: [v5][PATCH 03/10] xen:x86: define a new hypercall to get RMRR mappings

[Chen, Tiejun]

On 2014/9/1 18:29, Jan Beulich wrote:
>>>> On 01.09.14 at 11:44, <tiejun.chen@intel.com> wrote:
>> On 2014/8/29 17:18, Jan Beulich wrote:
>>>
>>> This still allocates another instance of structures to create a second
>>> linked list. Did you consider get_device_reserved_memory() to take
>>
>> Do you mean we still use this existing type combo, acpi_rmrr_units and
>> acpi_rmrr_units?
>>
>>> a callback function instead?
>>>
>>
>> But we should do something like this,
>>
>> 1. .get_device_reserved_memory = get_drm_all,
>> 2.  static int get_drm_all(struct list_head *dev_reserved_memory)
>>       {
>>           return (get_drm_callback(dev_reserved_memory));
>>       }
>>
>> 3. get_drm_callback = get_device_acpi_reserved_memory;
>> 4.  static int get_device_acpi_reserved_memory(struct list_head
>> *dev_reserved_memory)
>>       {
>> 	...
>> 	dev_reserved_memory = &acpi_rmrr_units;
>> 	...
>>       }
>>
>> Then while calling the hypercall,
>>
>> struct list_head *dev_reserved_memory = NULL;
>> nr_entries = ops->get_device_reserved_memory(dev_reserved_memory);
>> if (!nr_entries)
>> 	list_for_each_entry( darm, dev_reserved_memory, list )
>> 	{
>> 		xxx.start_pfn = ...;
>> 		xxx.nr_pages = ...;
>> 		if ( copy_to_guest_offset(buffer, i, &xxx, 1) )
>> 		...
>> 	}
>
> Clearly not: The callback ought to be used _while_ processing the
> hypercall. And of course the callback shouldn't be used to retrieve
> &acpi_rmrr_units, but to report back to the calling entity the
> individual regions.
>

Jan,

I see you're reviewing other patches in v5 so really appreciate your 
comments.

But I will address those comments until here I can implement this 
callback mechanism as you expect. Because some comments from other 
patches may need to rebase on this better way. So I hope I can finish 
your callback mechanism firstly to avoid bring you potential duplicated 
faults :)

So could you take a look at the follows?

xen/vtd: add one iommu ops to expose device reserved
  memory

We need this interface to expose device reserved memory
safely in common place.

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

diff --git a/xen/drivers/passthrough/vtd/dmar.c 
b/xen/drivers/passthrough/vtd/dmar.c
index 1152c3a..f46aee2 100644
--- a/xen/drivers/passthrough/vtd/dmar.c
+++ b/xen/drivers/passthrough/vtd/dmar.c
@@ -567,6 +567,44 @@ out:
      return ret;
  }

+extern get_device_reserved_memory_t get_drm_callback;
+struct xen_mem_reserved_device_memory 
*get_device_acpi_reserved_memory(unsigned int *nr_entries)
+{
+    struct acpi_rmrr_unit *rmrru;
+    static struct xen_mem_reserved_device_memory *rmrrm = NULL;
+    static unsigned int drm_entries = 0;
+    static unsigned int check_done = 0;
+    unsigned int i = 0;
+
+    *nr_entries = drm_entries;
+    if ( check_done )
+        return rmrrm;
+
+    list_for_each_entry(rmrru, &acpi_rmrr_units, list)
+        drm_entries++;
+
+    if ( drm_entries )
+    {
+        rmrrm = xzalloc_array(struct xen_mem_reserved_device_memory,
+                              drm_entries);
+        if ( !rmrrm )
+            return NULL;
+
+        list_for_each_entry(rmrru, &acpi_rmrr_units, list)
+        {
+            rmrrm[i].start_pfn = rmrru->base_address >> PAGE_SHIFT;
+            rmrrm[i].nr_pages = PAGE_ALIGN(rmrru->end_address -
+                                           rmrru->base_address) /
+                                           PAGE_SIZE;
+            i++;
+        }
+    }
+
+    check_done = 1;
+
+    return rmrrm;
+}
+
  static int __init
  acpi_parse_one_rmrr(struct acpi_dmar_header *header)
  {
@@ -678,6 +716,8 @@ acpi_parse_one_rmrr(struct acpi_dmar_header *header)
          }
      }

+    get_drm_callback = get_device_acpi_reserved_memory;
+
      return ret;
  }

diff --git a/xen/drivers/passthrough/vtd/iommu.c 
b/xen/drivers/passthrough/vtd/iommu.c
index 042b882..43ff443 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -2461,6 +2461,19 @@ static void vtd_dump_p2m_table(struct domain *d)
      vtd_dump_p2m_table_level(hd->arch.pgd_maddr, 
agaw_to_level(hd->arch.agaw), 0, 0);
  }

+struct xen_mem_reserved_device_memory *dummy_get_drm_callback(unsigned 
int *nr_entries)
+{
+    *nr_entries = 0;
+    return NULL;
+}
+
+get_device_reserved_memory_t get_drm_callback = dummy_get_drm_callback;
+
+struct xen_mem_reserved_device_memory *get_drm_all(unsigned int 
*nr_entries)
+{
+    return (get_drm_callback(nr_entries));
+}
+
  const struct iommu_ops intel_iommu_ops = {
      .init = intel_iommu_domain_init,
      .hwdom_init = intel_iommu_hwdom_init,
@@ -2486,6 +2499,7 @@ const struct iommu_ops intel_iommu_ops = {
      .iotlb_flush = intel_iommu_iotlb_flush,
      .iotlb_flush_all = intel_iommu_iotlb_flush_all,
      .dump_p2m_table = vtd_dump_p2m_table,
+    .get_device_reserved_memory = get_drm_all,
  };

  /*
diff --git a/xen/include/asm-x86/iommu.h b/xen/include/asm-x86/iommu.h
index e7a65da..aead1d7 100644
--- a/xen/include/asm-x86/iommu.h
+++ b/xen/include/asm-x86/iommu.h
@@ -15,6 +15,8 @@
  #ifndef __ARCH_X86_IOMMU_H__
  #define __ARCH_X86_IOMMU_H__

+#include <public/memory.h>
+
  #define MAX_IOMMUS 32

  /* Does this domain have a P2M table we can use as its IOMMU pagetable? */
@@ -32,6 +34,7 @@ int iommu_supports_eim(void);
  int iommu_enable_x2apic_IR(void);
  void iommu_disable_x2apic_IR(void);

+typedef struct xen_mem_reserved_device_memory* 
(*get_device_reserved_memory_t)(unsigned int *nr_entries);
  #endif /* !__ARCH_X86_IOMMU_H__ */
  /*
   * Local variables:
diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
index 8eb764a..8806ef6 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -149,6 +149,7 @@ struct iommu_ops {
      void (*update_ire_from_apic)(unsigned int apic, unsigned int reg, 
unsigned int value);
      unsigned int (*read_apic_from_ire)(unsigned int apic, unsigned int 
reg);
      int (*setup_hpet_msi)(struct msi_desc *);
+    struct xen_mem_reserved_device_memory* 
(*get_device_reserved_memory)(unsigned int *nr_entries);
  #endif /* CONFIG_X86 */
      void (*suspend)(void);
      void (*resume)(void);
-- 


Then when call the hypercall,

+    case XENMEM_reserved_device_memory_map:
+    {
+        struct xen_mem_reserved_device_memory *xmrdm = NULL;
+        struct xen_mem_reserved_device_memory_map xmrdmm;
+        XEN_GUEST_HANDLE(xen_mem_reserved_device_memory_t) buffer;
+        XEN_GUEST_HANDLE_PARAM(xen_mem_reserved_device_memory_t) 
buffer_param;
+        const struct iommu_ops *ops = iommu_get_ops();
+        unsigned int nr_entries = 0;
+        unsigned int i = 0;
+
+        xmrdm = ops->get_device_reserved_memory(&nr_entries);
+        if ( !nr_entries )
+            return -ENOENT;
+        if ( nr_entries < 0 )
+            return -EFAULT;
+
+        if ( copy_from_guest(&xmrdmm, arg, 1) )
+            return -EFAULT;
+
+        if ( xmrdmm.nr_entries < nr_entries )
+        {
+            xmrdmm.nr_entries = nr_entries;
+            if ( copy_to_guest(arg, &xmrdmm, 1) )
+                return -EFAULT;
+            return -ENOBUFS;
+        }
+
+        buffer_param = guest_handle_cast(xmrdmm.buffer,
+                                         xen_mem_reserved_device_memory_t);
+        buffer = guest_handle_from_param(buffer_param,
+                                         xen_mem_reserved_device_memory_t);
+        if ( !guest_handle_okay(buffer, xmrdmm.nr_entries) )
+            return -EFAULT;
+
+        for ( i = 0; i < nr_entries; i++ )
+        {
+            if ( copy_to_guest_offset(buffer, i, xmrdm + i, 1) )
+                return -EFAULT;
+        }
+
+        xmrdmm.nr_entries = i;
+
+        if ( copy_to_guest(arg, &xmrdmm, 1) )
+                return -EFAULT;
+
+        return 0;
+    }
+



Thanks
Tiejun

Re: [v5][PATCH 03/10] xen:x86: define a new hypercall to get RMRR mappings

[Jan Beulich]

>>> On 02.09.14 at 11:59, <tiejun.chen@intel.com> wrote:
> So could you take a look at the follows?

I'm sorry, but this is a nightmare. Do you know what the term
"callback" means?

> Then when call the hypercall,
> 
> +    case XENMEM_reserved_device_memory_map:
> +    {
> +        struct xen_mem_reserved_device_memory *xmrdm = NULL;
> +        struct xen_mem_reserved_device_memory_map xmrdmm;
> +        XEN_GUEST_HANDLE(xen_mem_reserved_device_memory_t) buffer;
> +        XEN_GUEST_HANDLE_PARAM(xen_mem_reserved_device_memory_t) buffer_param;
> +        const struct iommu_ops *ops = iommu_get_ops();
> +        unsigned int nr_entries = 0;
> +        unsigned int i = 0;
> +
> +        xmrdm = ops->get_device_reserved_memory(&nr_entries);
> +        if ( !nr_entries )
> +            return -ENOENT;
> +        if ( nr_entries < 0 )
> +            return -EFAULT;
> +
> +        if ( copy_from_guest(&xmrdmm, arg, 1) )
> +            return -EFAULT;
> +
> +        if ( xmrdmm.nr_entries < nr_entries )
> +        {
> +            xmrdmm.nr_entries = nr_entries;
> +            if ( copy_to_guest(arg, &xmrdmm, 1) )
> +                return -EFAULT;
> +            return -ENOBUFS;
> +        }
> +
> +        buffer_param = guest_handle_cast(xmrdmm.buffer,
> +                                         xen_mem_reserved_device_memory_t);
> +        buffer = guest_handle_from_param(buffer_param,
> +                                         xen_mem_reserved_device_memory_t);
> +        if ( !guest_handle_okay(buffer, xmrdmm.nr_entries) )
> +            return -EFAULT;
> +
> +        for ( i = 0; i < nr_entries; i++ )
> +        {
> +            if ( copy_to_guest_offset(buffer, i, xmrdm + i, 1) )
> +                return -EFAULT;
> +        }

The equivalent of this loop would move into the new (abstracted at
the IOMMU level) VT-d function. Out of that loop you'd call the
callback passed from here once per iteration; the called function
would take care of storing the needed information and advancing a
private counter. No memory allocation, and no VT-d specific
knowledge required in generic code.

Jan

Re: [v5][PATCH 03/10] xen:x86: define a new hypercall to get RMRR mappings

[Chen, Tiejun]

On 2014/9/2 18:15, Jan Beulich wrote:
>>>> On 02.09.14 at 11:59, <tiejun.chen@intel.com> wrote:
>> So could you take a look at the follows?
>
> I'm sorry, but this is a nightmare. Do you know what the term
> "callback" means?

I don't know how to handle this in the hypercall case.

>
>> Then when call the hypercall,
>>
>> +    case XENMEM_reserved_device_memory_map:
>> +    {
>> +        struct xen_mem_reserved_device_memory *xmrdm = NULL;
>> +        struct xen_mem_reserved_device_memory_map xmrdmm;
>> +        XEN_GUEST_HANDLE(xen_mem_reserved_device_memory_t) buffer;
>> +        XEN_GUEST_HANDLE_PARAM(xen_mem_reserved_device_memory_t) buffer_param;
>> +        const struct iommu_ops *ops = iommu_get_ops();
>> +        unsigned int nr_entries = 0;
>> +        unsigned int i = 0;
>> +
>> +        xmrdm = ops->get_device_reserved_memory(&nr_entries);

Do we still need this iommu_ops somewhere else?

>> +        if ( !nr_entries )

Do we still need this 'nr_entries' here?

>> +            return -ENOENT;
>> +        if ( nr_entries < 0 )
>> +            return -EFAULT;
>> +
>> +        if ( copy_from_guest(&xmrdmm, arg, 1) )
>> +            return -EFAULT;
>> +
>> +        if ( xmrdmm.nr_entries < nr_entries )
>> +        {
>> +            xmrdmm.nr_entries = nr_entries;
>> +            if ( copy_to_guest(arg, &xmrdmm, 1) )
>> +                return -EFAULT;
>> +            return -ENOBUFS;
>> +        }
>> +
>> +        buffer_param = guest_handle_cast(xmrdmm.buffer,
>> +                                         xen_mem_reserved_device_memory_t);
>> +        buffer = guest_handle_from_param(buffer_param,
>> +                                         xen_mem_reserved_device_memory_t);
>> +        if ( !guest_handle_okay(buffer, xmrdmm.nr_entries) )
>> +            return -EFAULT;
>> +
>> +        for ( i = 0; i < nr_entries; i++ )
>> +        {
>> +            if ( copy_to_guest_offset(buffer, i, xmrdm + i, 1) )
>> +                return -EFAULT;
>> +        }
>
> The equivalent of this loop would move into the new (abstracted at
> the IOMMU level) VT-d function. Out of that loop you'd call the
> callback passed from here once per iteration; the called function
> would take care of storing the needed information and advancing a
> private counter. No memory allocation, and no VT-d specific
> knowledge required in generic code.
>
> Jan
>
>

Re: [v5][PATCH 03/10] xen:x86: define a new hypercall to get RMRR mappings

[Jan Beulich]

>>> On 02.09.14 at 13:10, <tiejun.chen@intel.com> wrote:
> On 2014/9/2 18:15, Jan Beulich wrote:
>>>>> On 02.09.14 at 11:59, <tiejun.chen@intel.com> wrote:
>>> +    case XENMEM_reserved_device_memory_map:
>>> +    {
>>> +        struct xen_mem_reserved_device_memory *xmrdm = NULL;
>>> +        struct xen_mem_reserved_device_memory_map xmrdmm;
>>> +        XEN_GUEST_HANDLE(xen_mem_reserved_device_memory_t) buffer;
>>> +        XEN_GUEST_HANDLE_PARAM(xen_mem_reserved_device_memory_t) buffer_param;
>>> +        const struct iommu_ops *ops = iommu_get_ops();
>>> +        unsigned int nr_entries = 0;
>>> +        unsigned int i = 0;
>>> +
>>> +        xmrdm = ops->get_device_reserved_memory(&nr_entries);
> 
> Do we still need this iommu_ops somewhere else?

Not this one, but another one (as I had described further down).

>>> +        if ( !nr_entries )
> 
> Do we still need this 'nr_entries' here?

Doesn't look like so. But it's you coding this up - you ought to clearly
see what is needed and what not.

Jan

Re: [v5][PATCH 03/10] xen:x86: define a new hypercall to get RMRR mappings

[Chen, Tiejun]

On 2014/9/2 21:15, Jan Beulich wrote:
>>>> On 02.09.14 at 13:10, <tiejun.chen@intel.com> wrote:
>> On 2014/9/2 18:15, Jan Beulich wrote:
>>>>>> On 02.09.14 at 11:59, <tiejun.chen@intel.com> wrote:
>>>> +    case XENMEM_reserved_device_memory_map:
>>>> +    {
>>>> +        struct xen_mem_reserved_device_memory *xmrdm = NULL;
>>>> +        struct xen_mem_reserved_device_memory_map xmrdmm;
>>>> +        XEN_GUEST_HANDLE(xen_mem_reserved_device_memory_t) buffer;
>>>> +        XEN_GUEST_HANDLE_PARAM(xen_mem_reserved_device_memory_t) buffer_param;
>>>> +        const struct iommu_ops *ops = iommu_get_ops();
>>>> +        unsigned int nr_entries = 0;
>>>> +        unsigned int i = 0;
>>>> +
>>>> +        xmrdm = ops->get_device_reserved_memory(&nr_entries);
>>
>> Do we still need this iommu_ops somewhere else?
>
> Not this one, but another one (as I had described further down).
>
>>>> +        if ( !nr_entries )
>>
>> Do we still need this 'nr_entries' here?
>
> Doesn't look like so. But it's you coding this up - you ought to clearly
> see what is needed and what not.
>

I mean we need to get 'nr_entries' before any real operations since if 
that is zero, any following operations are pointless. And even that is 
nonzero, but actually the user don't know how many buffer should be 
passed, so often we may need to notify user again at the first time then 
the user call this again with a appropriate buffer. Right?

So how do we get this 'nr_entries'? Seems we have to to go VT-D stuff to 
walk that list or other similar approaches. If so, why we still get 
those real mapping entries later by accessing VT-D stuff again?

Shouldn't we figure out a approach we just call one time?

Thanks
Tiejun

Re: [v5][PATCH 03/10] xen:x86: define a new hypercall to get RMRR mappings

[Chen, Tiejun]

On 2014/9/3 9:45, Chen, Tiejun wrote:
> On 2014/9/2 21:15, Jan Beulich wrote:
>>>>> On 02.09.14 at 13:10, <tiejun.chen@intel.com> wrote:
>>> On 2014/9/2 18:15, Jan Beulich wrote:
>>>>>>> On 02.09.14 at 11:59, <tiejun.chen@intel.com> wrote:
>>>>> +    case XENMEM_reserved_device_memory_map:
>>>>> +    {
>>>>> +        struct xen_mem_reserved_device_memory *xmrdm = NULL;
>>>>> +        struct xen_mem_reserved_device_memory_map xmrdmm;
>>>>> +        XEN_GUEST_HANDLE(xen_mem_reserved_device_memory_t) buffer;
>>>>> +        XEN_GUEST_HANDLE_PARAM(xen_mem_reserved_device_memory_t)
>>>>> buffer_param;
>>>>> +        const struct iommu_ops *ops = iommu_get_ops();
>>>>> +        unsigned int nr_entries = 0;
>>>>> +        unsigned int i = 0;
>>>>> +
>>>>> +        xmrdm = ops->get_device_reserved_memory(&nr_entries);
>>>
>>> Do we still need this iommu_ops somewhere else?
>>
>> Not this one, but another one (as I had described further down).
>>
>>>>> +        if ( !nr_entries )
>>>
>>> Do we still need this 'nr_entries' here?
>>
>> Doesn't look like so. But it's you coding this up - you ought to clearly
>> see what is needed and what not.
>>
>
> I mean we need to get 'nr_entries' before any real operations since if
> that is zero, any following operations are pointless. And even that is
> nonzero, but actually the user don't know how many buffer should be
> passed, so often we may need to notify user again at the first time then
> the user call this again with a appropriate buffer. Right?
>
> So how do we get this 'nr_entries'? Seems we have to to go VT-D stuff to
> walk that list or other similar approaches. If so, why we still get
> those real mapping entries later by accessing VT-D stuff again?
>
> Shouldn't we figure out a approach we just call one time?
>

I update this patch based on this point:

xen/x86: define a hypercall to expose device reserved memory maps

We need such a new hypercall to get all device reserved memory
maps, then we can check if these maps are overlapping MMIO/RAM.
Note currently just address RMRR issue.

Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
---
  xen/arch/x86/mm.c                  | 23 +++++++++++++++++++++
  xen/drivers/passthrough/vtd/dmar.c | 41 
++++++++++++++++++++++++++++++++++++++
  xen/include/public/memory.h        | 37 +++++++++++++++++++++++++++++++++-
  xen/include/xen/mm.h               |  3 +++
  4 files changed, 103 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index d23cb3f..db3345b 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -180,6 +180,14 @@ static uint32_t base_disallow_mask;
        is_pv_domain(d)) ?                                        \
       L1_DISALLOW_MASK : (L1_DISALLOW_MASK & ~PAGE_CACHE_ATTRS))

+extern int
+do_copy_device_reserved_memory(struct 
xen_mem_reserved_device_memory_map *xmrdmmap);
+static int
+copy_drmmap_to_guest(drmmap_callback_t f, struct 
xen_mem_reserved_device_memory_map *xmrdmmap)
+{
+    return f(xmrdmmap);
+}
+
  static void __init init_frametable_chunk(void *start, void *end)
  {
      unsigned long s = (unsigned long)start;
@@ -4842,6 +4850,21 @@ 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 xmrdmmap;
+
+        if ( copy_from_guest(&xmrdmmap, arg, 1) )
+            return -EFAULT;
+
+        rc = copy_drmmap_to_guest(do_copy_device_reserved_memory, 
&xmrdmmap);
+
+        if ( __copy_to_guest(arg, &xmrdmmap, 1) )
+                rc = -EFAULT;
+
+        return rc;
+    }
+
      default:
          return subarch_memory_op(cmd, arg);
      }
diff --git a/xen/drivers/passthrough/vtd/dmar.c 
b/xen/drivers/passthrough/vtd/dmar.c
index 1152c3a..6bc5def 100644
--- a/xen/drivers/passthrough/vtd/dmar.c
+++ b/xen/drivers/passthrough/vtd/dmar.c
@@ -28,6 +28,7 @@
  #include <xen/xmalloc.h>
  #include <xen/pci.h>
  #include <xen/pci_regs.h>
+#include <xen/guest_access.h>
  #include <asm/string.h>
  #include "dmar.h"
  #include "iommu.h"
@@ -681,6 +682,46 @@ acpi_parse_one_rmrr(struct acpi_dmar_header *header)
      return ret;
  }

+int
+do_copy_device_reserved_memory(struct 
xen_mem_reserved_device_memory_map *xmrdmmap)
+{
+    struct acpi_rmrr_unit *rmrru;
+    struct xen_mem_reserved_device_memory xmrdm;
+    int i = 0, rc = 0;
+    XEN_GUEST_HANDLE_PARAM(xen_mem_reserved_device_memory_t) buffer_param;
+    XEN_GUEST_HANDLE(xen_mem_reserved_device_memory_t) buffer;
+
+    if ( list_empty(&acpi_rmrr_units) )
+        return -ENOENT;
+
+    buffer_param = guest_handle_cast(xmrdmmap->buffer,
+                                     xen_mem_reserved_device_memory_t);
+    buffer = guest_handle_from_param(buffer_param,
+                                     xen_mem_reserved_device_memory_t);
+    if ( !guest_handle_okay(buffer, xmrdmmap->nr_entries) )
+        return -EFAULT;
+
+    list_for_each_entry(rmrru, &acpi_rmrr_units, list)
+    {
+        if ( i < xmrdmmap->nr_entries )
+        {
+            xmrdm.start_pfn = rmrru->base_address >> PAGE_SHIFT;
+            xmrdm.nr_pages = PAGE_ALIGN(rmrru->end_address -
+                                        rmrru->base_address) /
+                                        PAGE_SIZE;
+            if ( __copy_to_guest_offset(buffer, i, &xmrdm, 1) )
+                return -EFAULT;
+        }
+        else
+            rc = -ENOBUFS;
+        i++;
+    }
+
+    xmrdmmap->nr_entries = i;
+
+    return rc;
+}
+
  static int __init
  acpi_parse_one_atsr(struct acpi_dmar_header *header)
  {
diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h
index 2c57aa0..628c99c 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 */
+/*
+ * For legacy reasons, some devices must be configured with special memory
+ * regions to function correctly. The guest must avoid using any of these
+ * regions.
+ *
+ * - Reserved memory Region Reporting Structure,
+ * So returns the device reserved memory map as it was when the domain
+ * was started.
+ */
+#define XENMEM_reserved_device_memory_map   26
+struct xen_mem_reserved_device_memory {
+    /* Start PFN of the current mapping of the page. */
+    xen_pfn_t start_pfn;
+    /* Number of the current mapping pages. */
+    xen_ulong_t nr_pages;
+};
+typedef struct xen_mem_reserved_device_memory 
xen_mem_reserved_device_memory_t;
+DEFINE_XEN_GUEST_HANDLE(xen_mem_reserved_device_memory_t);
+
+struct xen_mem_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;
+
+    /*
+     * xen_mem_reserved_device_memory entries in the buffer.
+     */
+    XEN_GUEST_HANDLE(xen_mem_reserved_device_memory_t) buffer;
+};
+typedef struct xen_mem_reserved_device_memory_map 
xen_mem_reserved_device_memory_map_t;
+DEFINE_XEN_GUEST_HANDLE(xen_mem_reserved_device_memory_map_t);
+
+/* Next available subop number is 27 */

  #endif /* __XEN_PUBLIC_MEMORY_H__ */

diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
index b183189..4fceafd 100644
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -31,6 +31,7 @@
  #include <xen/types.h>
  #include <xen/list.h>
  #include <xen/spinlock.h>
+#include <public/memory.h>

  struct domain;
  struct page_info;
@@ -371,4 +372,6 @@ int guest_remove_page(struct domain *d, unsigned 
long gmfn);
  /* TRUE if the whole page at @mfn is of the requested RAM type(s) 
above. */
  int page_is_ram_type(unsigned long mfn, unsigned long mem_type);

+typedef int (*drmmap_callback_t)(struct 
xen_mem_reserved_device_memory_map *xmrdmmap);
+
  #endif /* __XEN_MM_H__ */
-- 
1.9.1

Thanks
Tiejun

Re: [v5][PATCH 03/10] xen:x86: define a new hypercall to get RMRR mappings

[Jan Beulich]

>>> On 03.09.14 at 03:45, <tiejun.chen@intel.com> wrote:
> On 2014/9/2 21:15, Jan Beulich wrote:
>>>>> On 02.09.14 at 13:10, <tiejun.chen@intel.com> wrote:
>>> On 2014/9/2 18:15, Jan Beulich wrote:
>>>>>>> On 02.09.14 at 11:59, <tiejun.chen@intel.com> wrote:
>>>>> +    case XENMEM_reserved_device_memory_map:
>>>>> +    {
>>>>> +        struct xen_mem_reserved_device_memory *xmrdm = NULL;
>>>>> +        struct xen_mem_reserved_device_memory_map xmrdmm;
>>>>> +        XEN_GUEST_HANDLE(xen_mem_reserved_device_memory_t) buffer;
>>>>> +        XEN_GUEST_HANDLE_PARAM(xen_mem_reserved_device_memory_t) 
> buffer_param;
>>>>> +        const struct iommu_ops *ops = iommu_get_ops();
>>>>> +        unsigned int nr_entries = 0;
>>>>> +        unsigned int i = 0;
>>>>> +
>>>>> +        xmrdm = ops->get_device_reserved_memory(&nr_entries);
>>>
>>> Do we still need this iommu_ops somewhere else?
>>
>> Not this one, but another one (as I had described further down).
>>
>>>>> +        if ( !nr_entries )
>>>
>>> Do we still need this 'nr_entries' here?
>>
>> Doesn't look like so. But it's you coding this up - you ought to clearly
>> see what is needed and what not.
>>
> 
> I mean we need to get 'nr_entries' before any real operations since if 
> that is zero, any following operations are pointless. And even that is 
> nonzero, but actually the user don't know how many buffer should be 
> passed, so often we may need to notify user again at the first time then 
> the user call this again with a appropriate buffer. Right?
> 
> So how do we get this 'nr_entries'? Seems we have to to go VT-D stuff to 
> walk that list or other similar approaches. If so, why we still get 
> those real mapping entries later by accessing VT-D stuff again?
> 
> Shouldn't we figure out a approach we just call one time?

Just go look for other examples of similar operations: There's
absolutely no need to know the number of entries up front. Just
have a way for the callback function to signal its caller that it
can abort the loop and return right away if you really need this.
But I doubt you do, since even when the caller has provided too
little buffer space you still want to tell it how much is needed. And
the necessary counting would still be done in the callback function.

Jan

Re: [v5][PATCH 03/10] xen:x86: define a new hypercall to get RMRR mappings

[Jan Beulich]

>>> On 03.09.14 at 10:31, <tiejun.chen@intel.com> wrote:
> On 2014/9/3 9:45, Chen, Tiejun wrote:
>> On 2014/9/2 21:15, Jan Beulich wrote:
>>>>>> On 02.09.14 at 13:10, <tiejun.chen@intel.com> wrote:
>>>> On 2014/9/2 18:15, Jan Beulich wrote:
>>>>>>>> On 02.09.14 at 11:59, <tiejun.chen@intel.com> wrote:
>>>>>> +    case XENMEM_reserved_device_memory_map:
>>>>>> +    {
>>>>>> +        struct xen_mem_reserved_device_memory *xmrdm = NULL;
>>>>>> +        struct xen_mem_reserved_device_memory_map xmrdmm;
>>>>>> +        XEN_GUEST_HANDLE(xen_mem_reserved_device_memory_t) buffer;
>>>>>> +        XEN_GUEST_HANDLE_PARAM(xen_mem_reserved_device_memory_t)
>>>>>> buffer_param;
>>>>>> +        const struct iommu_ops *ops = iommu_get_ops();
>>>>>> +        unsigned int nr_entries = 0;
>>>>>> +        unsigned int i = 0;
>>>>>> +
>>>>>> +        xmrdm = ops->get_device_reserved_memory(&nr_entries);
>>>>
>>>> Do we still need this iommu_ops somewhere else?
>>>
>>> Not this one, but another one (as I had described further down).
>>>
>>>>>> +        if ( !nr_entries )
>>>>
>>>> Do we still need this 'nr_entries' here?
>>>
>>> Doesn't look like so. But it's you coding this up - you ought to clearly
>>> see what is needed and what not.
>>>
>>
>> I mean we need to get 'nr_entries' before any real operations since if
>> that is zero, any following operations are pointless. And even that is
>> nonzero, but actually the user don't know how many buffer should be
>> passed, so often we may need to notify user again at the first time then
>> the user call this again with a appropriate buffer. Right?
>>
>> So how do we get this 'nr_entries'? Seems we have to to go VT-D stuff to
>> walk that list or other similar approaches. If so, why we still get
>> those real mapping entries later by accessing VT-D stuff again?
>>
>> Shouldn't we figure out a approach we just call one time?
>>
> 
> I update this patch based on this point:

I'm afraid I have to give up and instead go and implement this for
you (which already by now would clearly have been the much less
time consuming thing at least on my end).

Jan

Re: [v5][PATCH 03/10] xen:x86: define a new hypercall to get RMRR mappings

[Chen, Tiejun]

On 2014/9/3 16:41, Jan Beulich wrote:
>>>> On 03.09.14 at 10:31, <tiejun.chen@intel.com> wrote:
>> On 2014/9/3 9:45, Chen, Tiejun wrote:
>>> On 2014/9/2 21:15, Jan Beulich wrote:
>>>>>>> On 02.09.14 at 13:10, <tiejun.chen@intel.com> wrote:
>>>>> On 2014/9/2 18:15, Jan Beulich wrote:
>>>>>>>>> On 02.09.14 at 11:59, <tiejun.chen@intel.com> wrote:
>>>>>>> +    case XENMEM_reserved_device_memory_map:
>>>>>>> +    {
>>>>>>> +        struct xen_mem_reserved_device_memory *xmrdm = NULL;
>>>>>>> +        struct xen_mem_reserved_device_memory_map xmrdmm;
>>>>>>> +        XEN_GUEST_HANDLE(xen_mem_reserved_device_memory_t) buffer;
>>>>>>> +        XEN_GUEST_HANDLE_PARAM(xen_mem_reserved_device_memory_t)
>>>>>>> buffer_param;
>>>>>>> +        const struct iommu_ops *ops = iommu_get_ops();
>>>>>>> +        unsigned int nr_entries = 0;
>>>>>>> +        unsigned int i = 0;
>>>>>>> +
>>>>>>> +        xmrdm = ops->get_device_reserved_memory(&nr_entries);
>>>>>
>>>>> Do we still need this iommu_ops somewhere else?
>>>>
>>>> Not this one, but another one (as I had described further down).
>>>>
>>>>>>> +        if ( !nr_entries )
>>>>>
>>>>> Do we still need this 'nr_entries' here?
>>>>
>>>> Doesn't look like so. But it's you coding this up - you ought to clearly
>>>> see what is needed and what not.
>>>>
>>>
>>> I mean we need to get 'nr_entries' before any real operations since if
>>> that is zero, any following operations are pointless. And even that is
>>> nonzero, but actually the user don't know how many buffer should be
>>> passed, so often we may need to notify user again at the first time then
>>> the user call this again with a appropriate buffer. Right?
>>>
>>> So how do we get this 'nr_entries'? Seems we have to to go VT-D stuff to
>>> walk that list or other similar approaches. If so, why we still get
>>> those real mapping entries later by accessing VT-D stuff again?
>>>
>>> Shouldn't we figure out a approach we just call one time?
>>>
>>
>> I update this patch based on this point:
>
> I'm afraid I have to give up and instead go and implement this for
> you (which already by now would clearly have been the much less
> time consuming thing at least on my end).
>

I think I should cover your latest comment:

"> Just go look for other examples of similar operations: There's
 > absolutely no need to know the number of entries up front. Just
 > have a way for the callback function to signal its caller that it
 > can abort the loop and return right away if you really need this.
 > But I doubt you do, since even when the caller has provided too
 > little buffer space you still want to tell it how much is needed. And
 > the necessary counting would still be done in the callback function.
 >
 > Jan
"
#1 Don't need to know entries in advance
#2 Just copy the entries as the caller but return a -ENOBUFS

+    case XENMEM_reserved_device_memory_map:
+    {
+        struct xen_mem_reserved_device_memory_map xmrdmmap;
+
+        if ( copy_from_guest(&xmrdmmap, arg, 1) )
+            return -EFAULT;
+
+        rc = copy_drmmap_to_guest(do_copy_device_reserved_memory, 
&xmrdmmap);
+
+        if ( __copy_to_guest(arg, &xmrdmmap, 1) )
+                rc = -EFAULT;
+
+        return rc;
+    }
+

And

+int
+do_copy_device_reserved_memory(struct 
xen_mem_reserved_device_memory_map *xmrdmmap)
+{
+    struct acpi_rmrr_unit *rmrru;
+    struct xen_mem_reserved_device_memory xmrdm;
+    int i = 0, rc = 0;
+    XEN_GUEST_HANDLE_PARAM(xen_mem_reserved_device_memory_t) buffer_param;
+    XEN_GUEST_HANDLE(xen_mem_reserved_device_memory_t) buffer;
+
+    if ( list_empty(&acpi_rmrr_units) )
+        return -ENOENT;
+
+    buffer_param = guest_handle_cast(xmrdmmap->buffer,
+                                     xen_mem_reserved_device_memory_t);
+    buffer = guest_handle_from_param(buffer_param,
+                                     xen_mem_reserved_device_memory_t);
+    if ( !guest_handle_okay(buffer, xmrdmmap->nr_entries) )
+        return -EFAULT;
+
+    list_for_each_entry(rmrru, &acpi_rmrr_units, list)
+    {
+        if ( i < xmrdmmap->nr_entries )
+        {
+            xmrdm.start_pfn = rmrru->base_address >> PAGE_SHIFT;
+            xmrdm.nr_pages = PAGE_ALIGN(rmrru->end_address -
+                                        rmrru->base_address) /
+                                        PAGE_SIZE;
+            if ( __copy_to_guest_offset(buffer, i, &xmrdm, 1) )
+                return -EFAULT;
+        }
+        else
+            rc = -ENOBUFS;
+        i++;
+    }
+
+    xmrdmmap->nr_entries = i;
+
+    return rc;
+}
+

Thanks
Tiejun

Re: [v5][PATCH 03/10] xen:x86: define a new hypercall to get RMRR mappings

[Chen, Tiejun]

On 2014/9/3 16:59, Chen, Tiejun wrote:
> On 2014/9/3 16:41, Jan Beulich wrote:
>>>>> On 03.09.14 at 10:31, <tiejun.chen@intel.com> wrote:
>>> On 2014/9/3 9:45, Chen, Tiejun wrote:
>>>> On 2014/9/2 21:15, Jan Beulich wrote:
>>>>>>>> On 02.09.14 at 13:10, <tiejun.chen@intel.com> wrote:
>>>>>> On 2014/9/2 18:15, Jan Beulich wrote:
>>>>>>>>>> On 02.09.14 at 11:59, <tiejun.chen@intel.com> wrote:
>>>>>>>> +    case XENMEM_reserved_device_memory_map:
>>>>>>>> +    {
>>>>>>>> +        struct xen_mem_reserved_device_memory *xmrdm = NULL;
>>>>>>>> +        struct xen_mem_reserved_device_memory_map xmrdmm;
>>>>>>>> +        XEN_GUEST_HANDLE(xen_mem_reserved_device_memory_t) buffer;
>>>>>>>> +        XEN_GUEST_HANDLE_PARAM(xen_mem_reserved_device_memory_t)
>>>>>>>> buffer_param;
>>>>>>>> +        const struct iommu_ops *ops = iommu_get_ops();
>>>>>>>> +        unsigned int nr_entries = 0;
>>>>>>>> +        unsigned int i = 0;
>>>>>>>> +
>>>>>>>> +        xmrdm = ops->get_device_reserved_memory(&nr_entries);
>>>>>>
>>>>>> Do we still need this iommu_ops somewhere else?
>>>>>
>>>>> Not this one, but another one (as I had described further down).
>>>>>
>>>>>>>> +        if ( !nr_entries )
>>>>>>
>>>>>> Do we still need this 'nr_entries' here?
>>>>>
>>>>> Doesn't look like so. But it's you coding this up - you ought to
>>>>> clearly
>>>>> see what is needed and what not.
>>>>>
>>>>
>>>> I mean we need to get 'nr_entries' before any real operations since if
>>>> that is zero, any following operations are pointless. And even that is
>>>> nonzero, but actually the user don't know how many buffer should be
>>>> passed, so often we may need to notify user again at the first time
>>>> then
>>>> the user call this again with a appropriate buffer. Right?
>>>>
>>>> So how do we get this 'nr_entries'? Seems we have to to go VT-D
>>>> stuff to
>>>> walk that list or other similar approaches. If so, why we still get
>>>> those real mapping entries later by accessing VT-D stuff again?
>>>>
>>>> Shouldn't we figure out a approach we just call one time?
>>>>
>>>
>>> I update this patch based on this point:
>>
>> I'm afraid I have to give up and instead go and implement this for
>> you (which already by now would clearly have been the much less
>> time consuming thing at least on my end).
>>
>
> I think I should cover your latest comment:

I mean last revision I pasted already covered your latest comment.

Thanks
Tiejun

>
> "> Just go look for other examples of similar operations: There's
>  > absolutely no need to know the number of entries up front. Just
>  > have a way for the callback function to signal its caller that it
>  > can abort the loop and return right away if you really need this.
>  > But I doubt you do, since even when the caller has provided too
>  > little buffer space you still want to tell it how much is needed. And
>  > the necessary counting would still be done in the callback function.
>  >
>  > Jan
> "
> #1 Don't need to know entries in advance
> #2 Just copy the entries as the caller but return a -ENOBUFS
>
> +    case XENMEM_reserved_device_memory_map:
> +    {
> +        struct xen_mem_reserved_device_memory_map xmrdmmap;
> +
> +        if ( copy_from_guest(&xmrdmmap, arg, 1) )
> +            return -EFAULT;
> +
> +        rc = copy_drmmap_to_guest(do_copy_device_reserved_memory,
> &xmrdmmap);
> +
> +        if ( __copy_to_guest(arg, &xmrdmmap, 1) )
> +                rc = -EFAULT;
> +
> +        return rc;
> +    }
> +
>
> And
>
> +int
> +do_copy_device_reserved_memory(struct
> xen_mem_reserved_device_memory_map *xmrdmmap)
> +{
> +    struct acpi_rmrr_unit *rmrru;
> +    struct xen_mem_reserved_device_memory xmrdm;
> +    int i = 0, rc = 0;
> +    XEN_GUEST_HANDLE_PARAM(xen_mem_reserved_device_memory_t) buffer_param;
> +    XEN_GUEST_HANDLE(xen_mem_reserved_device_memory_t) buffer;
> +
> +    if ( list_empty(&acpi_rmrr_units) )
> +        return -ENOENT;
> +
> +    buffer_param = guest_handle_cast(xmrdmmap->buffer,
> +                                     xen_mem_reserved_device_memory_t);
> +    buffer = guest_handle_from_param(buffer_param,
> +                                     xen_mem_reserved_device_memory_t);
> +    if ( !guest_handle_okay(buffer, xmrdmmap->nr_entries) )
> +        return -EFAULT;
> +
> +    list_for_each_entry(rmrru, &acpi_rmrr_units, list)
> +    {
> +        if ( i < xmrdmmap->nr_entries )
> +        {
> +            xmrdm.start_pfn = rmrru->base_address >> PAGE_SHIFT;
> +            xmrdm.nr_pages = PAGE_ALIGN(rmrru->end_address -
> +                                        rmrru->base_address) /
> +                                        PAGE_SIZE;
> +            if ( __copy_to_guest_offset(buffer, i, &xmrdm, 1) )
> +                return -EFAULT;
> +        }
> +        else
> +            rc = -ENOBUFS;
> +        i++;
> +    }
> +
> +    xmrdmmap->nr_entries = i;
> +
> +    return rc;
> +}
> +
>
> Thanks
> Tiejun

Re: [v5][PATCH 03/10] xen:x86: define a new hypercall to get RMRR mappings

[Chen, Tiejun]

On 2014/9/3 17:01, Chen, Tiejun wrote:
> On 2014/9/3 16:59, Chen, Tiejun wrote:
>> On 2014/9/3 16:41, Jan Beulich wrote:
>>>>>> On 03.09.14 at 10:31, <tiejun.chen@intel.com> wrote:
>>>> On 2014/9/3 9:45, Chen, Tiejun wrote:
>>>>> On 2014/9/2 21:15, Jan Beulich wrote:
>>>>>>>>> On 02.09.14 at 13:10, <tiejun.chen@intel.com> wrote:
>>>>>>> On 2014/9/2 18:15, Jan Beulich wrote:
>>>>>>>>>>> On 02.09.14 at 11:59, <tiejun.chen@intel.com> wrote:
>>>>>>>>> +    case XENMEM_reserved_device_memory_map:
>>>>>>>>> +    {
>>>>>>>>> +        struct xen_mem_reserved_device_memory *xmrdm = NULL;
>>>>>>>>> +        struct xen_mem_reserved_device_memory_map xmrdmm;
>>>>>>>>> +        XEN_GUEST_HANDLE(xen_mem_reserved_device_memory_t)
>>>>>>>>> buffer;
>>>>>>>>> +        XEN_GUEST_HANDLE_PARAM(xen_mem_reserved_device_memory_t)
>>>>>>>>> buffer_param;
>>>>>>>>> +        const struct iommu_ops *ops = iommu_get_ops();
>>>>>>>>> +        unsigned int nr_entries = 0;
>>>>>>>>> +        unsigned int i = 0;
>>>>>>>>> +
>>>>>>>>> +        xmrdm = ops->get_device_reserved_memory(&nr_entries);
>>>>>>>
>>>>>>> Do we still need this iommu_ops somewhere else?
>>>>>>
>>>>>> Not this one, but another one (as I had described further down).
>>>>>>
>>>>>>>>> +        if ( !nr_entries )
>>>>>>>
>>>>>>> Do we still need this 'nr_entries' here?
>>>>>>
>>>>>> Doesn't look like so. But it's you coding this up - you ought to
>>>>>> clearly
>>>>>> see what is needed and what not.
>>>>>>
>>>>>
>>>>> I mean we need to get 'nr_entries' before any real operations since if
>>>>> that is zero, any following operations are pointless. And even that is
>>>>> nonzero, but actually the user don't know how many buffer should be
>>>>> passed, so often we may need to notify user again at the first time
>>>>> then
>>>>> the user call this again with a appropriate buffer. Right?
>>>>>
>>>>> So how do we get this 'nr_entries'? Seems we have to to go VT-D
>>>>> stuff to
>>>>> walk that list or other similar approaches. If so, why we still get
>>>>> those real mapping entries later by accessing VT-D stuff again?
>>>>>
>>>>> Shouldn't we figure out a approach we just call one time?
>>>>>
>>>>
>>>> I update this patch based on this point:
>>>
>>> I'm afraid I have to give up and instead go and implement this for
>>> you (which already by now would clearly have been the much less
>>> time consuming thing at least on my end).
>>>
>>
>> I think I should cover your latest comment:
>
> I mean last revision I pasted already covered your latest comment.

I guess finally you will "go and implement this" as you said. So thanks 
for your time and guidance on this series of RFC patches.

Once you finish it, please tell me then I can test them.

Thanks
Tiejun

>
> Thanks
> Tiejun
>
>>
>> "> Just go look for other examples of similar operations: There's
>>  > absolutely no need to know the number of entries up front. Just
>>  > have a way for the callback function to signal its caller that it
>>  > can abort the loop and return right away if you really need this.
>>  > But I doubt you do, since even when the caller has provided too
>>  > little buffer space you still want to tell it how much is needed. And
>>  > the necessary counting would still be done in the callback function.
>>  >
>>  > Jan
>> "
>> #1 Don't need to know entries in advance
>> #2 Just copy the entries as the caller but return a -ENOBUFS
>>
>> +    case XENMEM_reserved_device_memory_map:
>> +    {
>> +        struct xen_mem_reserved_device_memory_map xmrdmmap;
>> +
>> +        if ( copy_from_guest(&xmrdmmap, arg, 1) )
>> +            return -EFAULT;
>> +
>> +        rc = copy_drmmap_to_guest(do_copy_device_reserved_memory,
>> &xmrdmmap);
>> +
>> +        if ( __copy_to_guest(arg, &xmrdmmap, 1) )
>> +                rc = -EFAULT;
>> +
>> +        return rc;
>> +    }
>> +
>>
>> And
>>
>> +int
>> +do_copy_device_reserved_memory(struct
>> xen_mem_reserved_device_memory_map *xmrdmmap)
>> +{
>> +    struct acpi_rmrr_unit *rmrru;
>> +    struct xen_mem_reserved_device_memory xmrdm;
>> +    int i = 0, rc = 0;
>> +    XEN_GUEST_HANDLE_PARAM(xen_mem_reserved_device_memory_t)
>> buffer_param;
>> +    XEN_GUEST_HANDLE(xen_mem_reserved_device_memory_t) buffer;
>> +
>> +    if ( list_empty(&acpi_rmrr_units) )
>> +        return -ENOENT;
>> +
>> +    buffer_param = guest_handle_cast(xmrdmmap->buffer,
>> +                                     xen_mem_reserved_device_memory_t);
>> +    buffer = guest_handle_from_param(buffer_param,
>> +                                     xen_mem_reserved_device_memory_t);
>> +    if ( !guest_handle_okay(buffer, xmrdmmap->nr_entries) )
>> +        return -EFAULT;
>> +
>> +    list_for_each_entry(rmrru, &acpi_rmrr_units, list)
>> +    {
>> +        if ( i < xmrdmmap->nr_entries )
>> +        {
>> +            xmrdm.start_pfn = rmrru->base_address >> PAGE_SHIFT;
>> +            xmrdm.nr_pages = PAGE_ALIGN(rmrru->end_address -
>> +                                        rmrru->base_address) /
>> +                                        PAGE_SIZE;
>> +            if ( __copy_to_guest_offset(buffer, i, &xmrdm, 1) )
>> +                return -EFAULT;
>> +        }
>> +        else
>> +            rc = -ENOBUFS;
>> +        i++;
>> +    }
>> +
>> +    xmrdmmap->nr_entries = i;
>> +
>> +    return rc;
>> +}
>> +
>>
>> Thanks
>> Tiejun

Re: [v5][PATCH 03/10] xen:x86: define a new hypercall to get RMRR mappings

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 9108 bytes --]

>>> On 03.09.14 at 10:41, <JBeulich@suse.com> wrote:
> I'm afraid I have to give up and instead go and implement this for
> you (which already by now would clearly have been the much less
> time consuming thing at least on my end).

So here's something to try (only compile tested).

Jan

introduce XENMEM_reserved_device_memory_map

This is a prerequisite for punching holes into HVM and PVH guests' P2M
to allow passing through devices that are associated with (on VT-d)
RMRRs.

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

--- a/xen/common/compat/memory.c
+++ b/xen/common/compat/memory.c
@@ -15,6 +15,35 @@ CHECK_TYPE(domid);
 
 CHECK_mem_access_op;
 
+struct get_reserved_device_memory {
+    struct compat_mem_reserved_device_memory_map map;
+    unsigned int used_entries;
+};
+
+static int get_reserved_device_memory(xen_pfn_t start,
+                                      xen_ulong_t nr, void *ctxt)
+{
+    struct get_reserved_device_memory *grdm = ctxt;
+
+    if ( grdm->used_entries < grdm->map.nr_entries )
+    {
+        struct compat_mem_reserved_device_memory rdm = {
+            .start_pfn = start, .nr_pages = nr
+        };
+
+        if ( rdm.start_pfn != start || rdm.nr_pages != nr )
+            return -ERANGE;
+
+        if ( __copy_to_compat_offset(grdm->map.buffer, grdm->used_entries,
+                                     &rdm, 1) )
+            return -EFAULT;
+    }
+
+    ++grdm->used_entries;
+
+    return 0;
+}
+
 int compat_memory_op(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) compat)
 {
     int split, op = cmd & MEMOP_CMD_MASK;
@@ -272,6 +301,29 @@ int compat_memory_op(unsigned int cmd, X
             break;
         }
 
+#ifdef HAS_PASSTHROUGH
+        case XENMEM_reserved_device_memory_map:
+        {
+            struct get_reserved_device_memory grdm;
+
+            if ( copy_from_guest(&grdm.map, compat, 1) ||
+                 !compat_handle_okay(grdm.map.buffer, grdm.map.nr_entries) )
+                return -EFAULT;
+
+            grdm.used_entries = 0;
+            rc = iommu_get_reserved_device_memory(get_reserved_device_memory,
+                                                  &grdm);
+
+            if ( !rc && grdm.map.nr_entries < grdm.used_entries )
+                rc = -ENOBUFS;
+            grdm.map.nr_entries = grdm.used_entries;
+            if ( __copy_to_guest(compat, &grdm.map, 1) )
+                rc = -EFAULT;
+
+            return rc;
+        }
+#endif
+
         default:
             return compat_arch_memory_op(cmd, compat);
         }
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -695,6 +695,32 @@ out:
     return rc;
 }
 
+struct get_reserved_device_memory {
+    struct xen_mem_reserved_device_memory_map map;
+    unsigned int used_entries;
+};
+
+static int get_reserved_device_memory(xen_pfn_t start,
+                                      xen_ulong_t nr, void *ctxt)
+{
+    struct get_reserved_device_memory *grdm = ctxt;
+
+    if ( grdm->used_entries < grdm->map.nr_entries )
+    {
+        struct xen_mem_reserved_device_memory rdm = {
+            .start_pfn = start, .nr_pages = nr
+        };
+
+        if ( __copy_to_guest_offset(grdm->map.buffer, grdm->used_entries,
+                                    &rdm, 1) )
+            return -EFAULT;
+    }
+
+    ++grdm->used_entries;
+
+    return 0;
+}
+
 long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
 {
     struct domain *d;
@@ -969,6 +995,29 @@ long do_memory_op(unsigned long cmd, XEN
 
         break;
 
+#ifdef HAS_PASSTHROUGH
+    case XENMEM_reserved_device_memory_map:
+    {
+        struct get_reserved_device_memory grdm;
+
+        if ( copy_from_guest(&grdm.map, arg, 1) ||
+             !guest_handle_okay(grdm.map.buffer, grdm.map.nr_entries) )
+            return -EFAULT;
+
+        grdm.used_entries = 0;
+        rc = iommu_get_reserved_device_memory(get_reserved_device_memory,
+                                              &grdm);
+
+        if ( !rc && grdm.map.nr_entries < grdm.used_entries )
+            rc = -ENOBUFS;
+        grdm.map.nr_entries = grdm.used_entries;
+        if ( __copy_to_guest(arg, &grdm.map, 1) )
+            rc = -EFAULT;
+
+        break;
+    }
+#endif
+
     default:
         rc = arch_memory_op(cmd, arg);
         break;
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -344,6 +344,16 @@ void iommu_crash_shutdown(void)
     iommu_enabled = iommu_intremap = 0;
 }
 
+int iommu_get_reserved_device_memory(iommu_grdm_t *func, void *ctxt)
+{
+    const struct iommu_ops *ops = iommu_get_ops();
+
+    if ( !iommu_enabled || !ops->get_reserved_device_memory )
+        return 0;
+
+    return ops->get_reserved_device_memory(func, ctxt);
+}
+
 bool_t iommu_has_feature(struct domain *d, enum iommu_feature feature)
 {
     const struct hvm_iommu *hd = domain_hvm_iommu(d);
--- a/xen/drivers/passthrough/vtd/dmar.c
+++ b/xen/drivers/passthrough/vtd/dmar.c
@@ -893,3 +893,20 @@ int platform_supports_x2apic(void)
     unsigned int mask = ACPI_DMAR_INTR_REMAP | ACPI_DMAR_X2APIC_OPT_OUT;
     return cpu_has_x2apic && ((dmar_flags & mask) == ACPI_DMAR_INTR_REMAP);
 }
+
+int intel_iommu_get_reserved_device_memory(iommu_grdm_t *func, void *ctxt)
+{
+    struct acpi_rmrr_unit *rmrr;
+    int rc = 0;
+
+    list_for_each_entry(rmrr, &acpi_rmrr_units, list)
+    {
+        rc = func(PFN_DOWN(rmrr->base_address),
+                  PFN_UP(rmrr->end_address) - PFN_DOWN(rmrr->base_address),
+                  ctxt);
+        if ( rc )
+            break;
+    }
+
+    return rc;
+}
--- a/xen/drivers/passthrough/vtd/extern.h
+++ b/xen/drivers/passthrough/vtd/extern.h
@@ -75,6 +75,7 @@ int domain_context_mapping_one(struct do
                                u8 bus, u8 devfn, const struct pci_dev *);
 int domain_context_unmap_one(struct domain *domain, struct iommu *iommu,
                              u8 bus, u8 devfn);
+int intel_iommu_get_reserved_device_memory(iommu_grdm_t *func, void *ctxt);
 
 unsigned int io_apic_read_remap_rte(unsigned int apic, unsigned int reg);
 void io_apic_write_remap_rte(unsigned int apic,
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -2485,6 +2485,7 @@ const struct iommu_ops intel_iommu_ops =
     .crash_shutdown = vtd_crash_shutdown,
     .iotlb_flush = intel_iommu_iotlb_flush,
     .iotlb_flush_all = intel_iommu_iotlb_flush_all,
+    .get_reserved_device_memory = intel_iommu_get_reserved_device_memory,
     .dump_p2m_table = vtd_dump_p2m_table,
 };
 
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -120,6 +120,8 @@ void iommu_dt_domain_destroy(struct doma
 
 struct page_info;
 
+typedef int iommu_grdm_t(xen_pfn_t start, xen_ulong_t nr, void *ctxt);
+
 struct iommu_ops {
     int (*init)(struct domain *d);
     void (*hwdom_init)(struct domain *d);
@@ -156,12 +158,14 @@ struct iommu_ops {
     void (*crash_shutdown)(void);
     void (*iotlb_flush)(struct domain *d, unsigned long gfn, unsigned int page_count);
     void (*iotlb_flush_all)(struct domain *d);
+    int (*get_reserved_device_memory)(iommu_grdm_t *, void *);
     void (*dump_p2m_table)(struct domain *d);
 };
 
 void iommu_suspend(void);
 void iommu_resume(void);
 void iommu_crash_shutdown(void);
+int iommu_get_reserved_device_memory(iommu_grdm_t *, void *);
 
 void iommu_share_p2m_table(struct domain *d);
 
--- a/xen/include/public/memory.h
+++ b/xen/include/public/memory.h
@@ -523,7 +523,29 @@ DEFINE_XEN_GUEST_HANDLE(xen_mem_sharing_
 
 #endif /* defined(__XEN__) || defined(__XEN_TOOLS__) */
 
-/* Next available subop number is 26 */
+/*
+ * For legacy reasons, some devices must be configured with special memory
+ * regions to function correctly.  The guest must avoid using any of these
+ * regions.
+ */
+#define XENMEM_reserved_device_memory_map   26
+struct xen_mem_reserved_device_memory {
+    xen_pfn_t start_pfn;
+    xen_ulong_t nr_pages;
+};
+typedef struct xen_mem_reserved_device_memory xen_mem_reserved_device_memory_t;
+DEFINE_XEN_GUEST_HANDLE(xen_mem_reserved_device_memory_t);
+
+struct xen_mem_reserved_device_memory_map {
+    /* IN/OUT */
+    unsigned int nr_entries;
+    /* OUT */
+    XEN_GUEST_HANDLE(xen_mem_reserved_device_memory_t) buffer;
+};
+typedef struct xen_mem_reserved_device_memory_map xen_mem_reserved_device_memory_map_t;
+DEFINE_XEN_GUEST_HANDLE(xen_mem_reserved_device_memory_map_t);
+
+/* Next available subop number is 27 */
 
 #endif /* __XEN_PUBLIC_MEMORY_H__ */
 
--- a/xen/include/xlat.lst
+++ b/xen/include/xlat.lst
@@ -60,7 +60,8 @@
 !	memory_exchange			memory.h
 !	memory_map			memory.h
 !	memory_reservation		memory.h
-?	mem_access_op		memory.h
+?	mem_access_op			memory.h
+!	mem_reserved_device_memory_map	memory.h
 !	pod_target			memory.h
 !	remove_from_physmap		memory.h
 ?	physdev_eoi			physdev.h



[-- Attachment #2: get-reserved-device-memory.patch --]
[-- Type: text/plain, Size: 8806 bytes --]

introduce XENMEM_reserved_device_memory_map

This is a prerequisite for punching holes into HVM and PVH guests' P2M
to allow passing through devices that are associated with (on VT-d)
RMRRs.

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

--- a/xen/common/compat/memory.c
+++ b/xen/common/compat/memory.c
@@ -15,6 +15,35 @@ CHECK_TYPE(domid);
 
 CHECK_mem_access_op;
 
+struct get_reserved_device_memory {
+    struct compat_mem_reserved_device_memory_map map;
+    unsigned int used_entries;
+};
+
+static int get_reserved_device_memory(xen_pfn_t start,
+                                      xen_ulong_t nr, void *ctxt)
+{
+    struct get_reserved_device_memory *grdm = ctxt;
+
+    if ( grdm->used_entries < grdm->map.nr_entries )
+    {
+        struct compat_mem_reserved_device_memory rdm = {
+            .start_pfn = start, .nr_pages = nr
+        };
+
+        if ( rdm.start_pfn != start || rdm.nr_pages != nr )
+            return -ERANGE;
+
+        if ( __copy_to_compat_offset(grdm->map.buffer, grdm->used_entries,
+                                     &rdm, 1) )
+            return -EFAULT;
+    }
+
+    ++grdm->used_entries;
+
+    return 0;
+}
+
 int compat_memory_op(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) compat)
 {
     int split, op = cmd & MEMOP_CMD_MASK;
@@ -272,6 +301,29 @@ int compat_memory_op(unsigned int cmd, X
             break;
         }
 
+#ifdef HAS_PASSTHROUGH
+        case XENMEM_reserved_device_memory_map:
+        {
+            struct get_reserved_device_memory grdm;
+
+            if ( copy_from_guest(&grdm.map, compat, 1) ||
+                 !compat_handle_okay(grdm.map.buffer, grdm.map.nr_entries) )
+                return -EFAULT;
+
+            grdm.used_entries = 0;
+            rc = iommu_get_reserved_device_memory(get_reserved_device_memory,
+                                                  &grdm);
+
+            if ( !rc && grdm.map.nr_entries < grdm.used_entries )
+                rc = -ENOBUFS;
+            grdm.map.nr_entries = grdm.used_entries;
+            if ( __copy_to_guest(compat, &grdm.map, 1) )
+                rc = -EFAULT;
+
+            return rc;
+        }
+#endif
+
         default:
             return compat_arch_memory_op(cmd, compat);
         }
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -695,6 +695,32 @@ out:
     return rc;
 }
 
+struct get_reserved_device_memory {
+    struct xen_mem_reserved_device_memory_map map;
+    unsigned int used_entries;
+};
+
+static int get_reserved_device_memory(xen_pfn_t start,
+                                      xen_ulong_t nr, void *ctxt)
+{
+    struct get_reserved_device_memory *grdm = ctxt;
+
+    if ( grdm->used_entries < grdm->map.nr_entries )
+    {
+        struct xen_mem_reserved_device_memory rdm = {
+            .start_pfn = start, .nr_pages = nr
+        };
+
+        if ( __copy_to_guest_offset(grdm->map.buffer, grdm->used_entries,
+                                    &rdm, 1) )
+            return -EFAULT;
+    }
+
+    ++grdm->used_entries;
+
+    return 0;
+}
+
 long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
 {
     struct domain *d;
@@ -969,6 +995,29 @@ long do_memory_op(unsigned long cmd, XEN
 
         break;
 
+#ifdef HAS_PASSTHROUGH
+    case XENMEM_reserved_device_memory_map:
+    {
+        struct get_reserved_device_memory grdm;
+
+        if ( copy_from_guest(&grdm.map, arg, 1) ||
+             !guest_handle_okay(grdm.map.buffer, grdm.map.nr_entries) )
+            return -EFAULT;
+
+        grdm.used_entries = 0;
+        rc = iommu_get_reserved_device_memory(get_reserved_device_memory,
+                                              &grdm);
+
+        if ( !rc && grdm.map.nr_entries < grdm.used_entries )
+            rc = -ENOBUFS;
+        grdm.map.nr_entries = grdm.used_entries;
+        if ( __copy_to_guest(arg, &grdm.map, 1) )
+            rc = -EFAULT;
+
+        break;
+    }
+#endif
+
     default:
         rc = arch_memory_op(cmd, arg);
         break;
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -344,6 +344,16 @@ void iommu_crash_shutdown(void)
     iommu_enabled = iommu_intremap = 0;
 }
 
+int iommu_get_reserved_device_memory(iommu_grdm_t *func, void *ctxt)
+{
+    const struct iommu_ops *ops = iommu_get_ops();
+
+    if ( !iommu_enabled || !ops->get_reserved_device_memory )
+        return 0;
+
+    return ops->get_reserved_device_memory(func, ctxt);
+}
+
 bool_t iommu_has_feature(struct domain *d, enum iommu_feature feature)
 {
     const struct hvm_iommu *hd = domain_hvm_iommu(d);
--- a/xen/drivers/passthrough/vtd/dmar.c
+++ b/xen/drivers/passthrough/vtd/dmar.c
@@ -893,3 +893,20 @@ int platform_supports_x2apic(void)
     unsigned int mask = ACPI_DMAR_INTR_REMAP | ACPI_DMAR_X2APIC_OPT_OUT;
     return cpu_has_x2apic && ((dmar_flags & mask) == ACPI_DMAR_INTR_REMAP);
 }
+
+int intel_iommu_get_reserved_device_memory(iommu_grdm_t *func, void *ctxt)
+{
+    struct acpi_rmrr_unit *rmrr;
+    int rc = 0;
+
+    list_for_each_entry(rmrr, &acpi_rmrr_units, list)
+    {
+        rc = func(PFN_DOWN(rmrr->base_address),
+                  PFN_UP(rmrr->end_address) - PFN_DOWN(rmrr->base_address),
+                  ctxt);
+        if ( rc )
+            break;
+    }
+
+    return rc;
+}
--- a/xen/drivers/passthrough/vtd/extern.h
+++ b/xen/drivers/passthrough/vtd/extern.h
@@ -75,6 +75,7 @@ int domain_context_mapping_one(struct do
                                u8 bus, u8 devfn, const struct pci_dev *);
 int domain_context_unmap_one(struct domain *domain, struct iommu *iommu,
                              u8 bus, u8 devfn);
+int intel_iommu_get_reserved_device_memory(iommu_grdm_t *func, void *ctxt);
 
 unsigned int io_apic_read_remap_rte(unsigned int apic, unsigned int reg);
 void io_apic_write_remap_rte(unsigned int apic,
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -2485,6 +2485,7 @@ const struct iommu_ops intel_iommu_ops =
     .crash_shutdown = vtd_crash_shutdown,
     .iotlb_flush = intel_iommu_iotlb_flush,
     .iotlb_flush_all = intel_iommu_iotlb_flush_all,
+    .get_reserved_device_memory = intel_iommu_get_reserved_device_memory,
     .dump_p2m_table = vtd_dump_p2m_table,
 };
 
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -120,6 +120,8 @@ void iommu_dt_domain_destroy(struct doma
 
 struct page_info;
 
+typedef int iommu_grdm_t(xen_pfn_t start, xen_ulong_t nr, void *ctxt);
+
 struct iommu_ops {
     int (*init)(struct domain *d);
     void (*hwdom_init)(struct domain *d);
@@ -156,12 +158,14 @@ struct iommu_ops {
     void (*crash_shutdown)(void);
     void (*iotlb_flush)(struct domain *d, unsigned long gfn, unsigned int page_count);
     void (*iotlb_flush_all)(struct domain *d);
+    int (*get_reserved_device_memory)(iommu_grdm_t *, void *);
     void (*dump_p2m_table)(struct domain *d);
 };
 
 void iommu_suspend(void);
 void iommu_resume(void);
 void iommu_crash_shutdown(void);
+int iommu_get_reserved_device_memory(iommu_grdm_t *, void *);
 
 void iommu_share_p2m_table(struct domain *d);
 
--- a/xen/include/public/memory.h
+++ b/xen/include/public/memory.h
@@ -523,7 +523,29 @@ DEFINE_XEN_GUEST_HANDLE(xen_mem_sharing_
 
 #endif /* defined(__XEN__) || defined(__XEN_TOOLS__) */
 
-/* Next available subop number is 26 */
+/*
+ * For legacy reasons, some devices must be configured with special memory
+ * regions to function correctly.  The guest must avoid using any of these
+ * regions.
+ */
+#define XENMEM_reserved_device_memory_map   26
+struct xen_mem_reserved_device_memory {
+    xen_pfn_t start_pfn;
+    xen_ulong_t nr_pages;
+};
+typedef struct xen_mem_reserved_device_memory xen_mem_reserved_device_memory_t;
+DEFINE_XEN_GUEST_HANDLE(xen_mem_reserved_device_memory_t);
+
+struct xen_mem_reserved_device_memory_map {
+    /* IN/OUT */
+    unsigned int nr_entries;
+    /* OUT */
+    XEN_GUEST_HANDLE(xen_mem_reserved_device_memory_t) buffer;
+};
+typedef struct xen_mem_reserved_device_memory_map xen_mem_reserved_device_memory_map_t;
+DEFINE_XEN_GUEST_HANDLE(xen_mem_reserved_device_memory_map_t);
+
+/* Next available subop number is 27 */
 
 #endif /* __XEN_PUBLIC_MEMORY_H__ */
 
--- a/xen/include/xlat.lst
+++ b/xen/include/xlat.lst
@@ -60,7 +60,8 @@
 !	memory_exchange			memory.h
 !	memory_map			memory.h
 !	memory_reservation		memory.h
-?	mem_access_op		memory.h
+?	mem_access_op			memory.h
+!	mem_reserved_device_memory_map	memory.h
 !	pod_target			memory.h
 !	remove_from_physmap		memory.h
 ?	physdev_eoi			physdev.h

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [v5][PATCH 03/10] xen:x86: define a new hypercall to get RMRR mappings

[Chen, Tiejun]

On 2014/9/3 20:54, Jan Beulich wrote:
>>>> On 03.09.14 at 10:41, <JBeulich@suse.com> wrote:
>> I'm afraid I have to give up and instead go and implement this for
>> you (which already by now would clearly have been the much less
>> time consuming thing at least on my end).
>
> So here's something to try (only compile tested).

This can work so I will rebase other patches on this.

Thanks
Tiejun

>
> Jan
>
> introduce XENMEM_reserved_device_memory_map
>
> This is a prerequisite for punching holes into HVM and PVH guests' P2M
> to allow passing through devices that are associated with (on VT-d)
> RMRRs.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> --- a/xen/common/compat/memory.c
> +++ b/xen/common/compat/memory.c
> @@ -15,6 +15,35 @@ CHECK_TYPE(domid);
>
>   CHECK_mem_access_op;
>
> +struct get_reserved_device_memory {
> +    struct compat_mem_reserved_device_memory_map map;
> +    unsigned int used_entries;
> +};
> +
> +static int get_reserved_device_memory(xen_pfn_t start,
> +                                      xen_ulong_t nr, void *ctxt)
> +{
> +    struct get_reserved_device_memory *grdm = ctxt;
> +
> +    if ( grdm->used_entries < grdm->map.nr_entries )
> +    {
> +        struct compat_mem_reserved_device_memory rdm = {
> +            .start_pfn = start, .nr_pages = nr
> +        };
> +
> +        if ( rdm.start_pfn != start || rdm.nr_pages != nr )
> +            return -ERANGE;
> +
> +        if ( __copy_to_compat_offset(grdm->map.buffer, grdm->used_entries,
> +                                     &rdm, 1) )
> +            return -EFAULT;
> +    }
> +
> +    ++grdm->used_entries;
> +
> +    return 0;
> +}
> +
>   int compat_memory_op(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) compat)
>   {
>       int split, op = cmd & MEMOP_CMD_MASK;
> @@ -272,6 +301,29 @@ int compat_memory_op(unsigned int cmd, X
>               break;
>           }
>
> +#ifdef HAS_PASSTHROUGH
> +        case XENMEM_reserved_device_memory_map:
> +        {
> +            struct get_reserved_device_memory grdm;
> +
> +            if ( copy_from_guest(&grdm.map, compat, 1) ||
> +                 !compat_handle_okay(grdm.map.buffer, grdm.map.nr_entries) )
> +                return -EFAULT;
> +
> +            grdm.used_entries = 0;
> +            rc = iommu_get_reserved_device_memory(get_reserved_device_memory,
> +                                                  &grdm);
> +
> +            if ( !rc && grdm.map.nr_entries < grdm.used_entries )
> +                rc = -ENOBUFS;
> +            grdm.map.nr_entries = grdm.used_entries;
> +            if ( __copy_to_guest(compat, &grdm.map, 1) )
> +                rc = -EFAULT;
> +
> +            return rc;
> +        }
> +#endif
> +
>           default:
>               return compat_arch_memory_op(cmd, compat);
>           }
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -695,6 +695,32 @@ out:
>       return rc;
>   }
>
> +struct get_reserved_device_memory {
> +    struct xen_mem_reserved_device_memory_map map;
> +    unsigned int used_entries;
> +};
> +
> +static int get_reserved_device_memory(xen_pfn_t start,
> +                                      xen_ulong_t nr, void *ctxt)
> +{
> +    struct get_reserved_device_memory *grdm = ctxt;
> +
> +    if ( grdm->used_entries < grdm->map.nr_entries )
> +    {
> +        struct xen_mem_reserved_device_memory rdm = {
> +            .start_pfn = start, .nr_pages = nr
> +        };
> +
> +        if ( __copy_to_guest_offset(grdm->map.buffer, grdm->used_entries,
> +                                    &rdm, 1) )
> +            return -EFAULT;
> +    }
> +
> +    ++grdm->used_entries;
> +
> +    return 0;
> +}
> +
>   long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>   {
>       struct domain *d;
> @@ -969,6 +995,29 @@ long do_memory_op(unsigned long cmd, XEN
>
>           break;
>
> +#ifdef HAS_PASSTHROUGH
> +    case XENMEM_reserved_device_memory_map:
> +    {
> +        struct get_reserved_device_memory grdm;
> +
> +        if ( copy_from_guest(&grdm.map, arg, 1) ||
> +             !guest_handle_okay(grdm.map.buffer, grdm.map.nr_entries) )
> +            return -EFAULT;
> +
> +        grdm.used_entries = 0;
> +        rc = iommu_get_reserved_device_memory(get_reserved_device_memory,
> +                                              &grdm);
> +
> +        if ( !rc && grdm.map.nr_entries < grdm.used_entries )
> +            rc = -ENOBUFS;
> +        grdm.map.nr_entries = grdm.used_entries;
> +        if ( __copy_to_guest(arg, &grdm.map, 1) )
> +            rc = -EFAULT;
> +
> +        break;
> +    }
> +#endif
> +
>       default:
>           rc = arch_memory_op(cmd, arg);
>           break;
> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -344,6 +344,16 @@ void iommu_crash_shutdown(void)
>       iommu_enabled = iommu_intremap = 0;
>   }
>
> +int iommu_get_reserved_device_memory(iommu_grdm_t *func, void *ctxt)
> +{
> +    const struct iommu_ops *ops = iommu_get_ops();
> +
> +    if ( !iommu_enabled || !ops->get_reserved_device_memory )
> +        return 0;
> +
> +    return ops->get_reserved_device_memory(func, ctxt);
> +}
> +
>   bool_t iommu_has_feature(struct domain *d, enum iommu_feature feature)
>   {
>       const struct hvm_iommu *hd = domain_hvm_iommu(d);
> --- a/xen/drivers/passthrough/vtd/dmar.c
> +++ b/xen/drivers/passthrough/vtd/dmar.c
> @@ -893,3 +893,20 @@ int platform_supports_x2apic(void)
>       unsigned int mask = ACPI_DMAR_INTR_REMAP | ACPI_DMAR_X2APIC_OPT_OUT;
>       return cpu_has_x2apic && ((dmar_flags & mask) == ACPI_DMAR_INTR_REMAP);
>   }
> +
> +int intel_iommu_get_reserved_device_memory(iommu_grdm_t *func, void *ctxt)
> +{
> +    struct acpi_rmrr_unit *rmrr;
> +    int rc = 0;
> +
> +    list_for_each_entry(rmrr, &acpi_rmrr_units, list)
> +    {
> +        rc = func(PFN_DOWN(rmrr->base_address),
> +                  PFN_UP(rmrr->end_address) - PFN_DOWN(rmrr->base_address),
> +                  ctxt);
> +        if ( rc )
> +            break;
> +    }
> +
> +    return rc;
> +}
> --- a/xen/drivers/passthrough/vtd/extern.h
> +++ b/xen/drivers/passthrough/vtd/extern.h
> @@ -75,6 +75,7 @@ int domain_context_mapping_one(struct do
>                                  u8 bus, u8 devfn, const struct pci_dev *);
>   int domain_context_unmap_one(struct domain *domain, struct iommu *iommu,
>                                u8 bus, u8 devfn);
> +int intel_iommu_get_reserved_device_memory(iommu_grdm_t *func, void *ctxt);
>
>   unsigned int io_apic_read_remap_rte(unsigned int apic, unsigned int reg);
>   void io_apic_write_remap_rte(unsigned int apic,
> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -2485,6 +2485,7 @@ const struct iommu_ops intel_iommu_ops =
>       .crash_shutdown = vtd_crash_shutdown,
>       .iotlb_flush = intel_iommu_iotlb_flush,
>       .iotlb_flush_all = intel_iommu_iotlb_flush_all,
> +    .get_reserved_device_memory = intel_iommu_get_reserved_device_memory,
>       .dump_p2m_table = vtd_dump_p2m_table,
>   };
>
> --- a/xen/include/xen/iommu.h
> +++ b/xen/include/xen/iommu.h
> @@ -120,6 +120,8 @@ void iommu_dt_domain_destroy(struct doma
>
>   struct page_info;
>
> +typedef int iommu_grdm_t(xen_pfn_t start, xen_ulong_t nr, void *ctxt);
> +
>   struct iommu_ops {
>       int (*init)(struct domain *d);
>       void (*hwdom_init)(struct domain *d);
> @@ -156,12 +158,14 @@ struct iommu_ops {
>       void (*crash_shutdown)(void);
>       void (*iotlb_flush)(struct domain *d, unsigned long gfn, unsigned int page_count);
>       void (*iotlb_flush_all)(struct domain *d);
> +    int (*get_reserved_device_memory)(iommu_grdm_t *, void *);
>       void (*dump_p2m_table)(struct domain *d);
>   };
>
>   void iommu_suspend(void);
>   void iommu_resume(void);
>   void iommu_crash_shutdown(void);
> +int iommu_get_reserved_device_memory(iommu_grdm_t *, void *);
>
>   void iommu_share_p2m_table(struct domain *d);
>
> --- a/xen/include/public/memory.h
> +++ b/xen/include/public/memory.h
> @@ -523,7 +523,29 @@ DEFINE_XEN_GUEST_HANDLE(xen_mem_sharing_
>
>   #endif /* defined(__XEN__) || defined(__XEN_TOOLS__) */
>
> -/* Next available subop number is 26 */
> +/*
> + * For legacy reasons, some devices must be configured with special memory
> + * regions to function correctly.  The guest must avoid using any of these
> + * regions.
> + */
> +#define XENMEM_reserved_device_memory_map   26
> +struct xen_mem_reserved_device_memory {
> +    xen_pfn_t start_pfn;
> +    xen_ulong_t nr_pages;
> +};
> +typedef struct xen_mem_reserved_device_memory xen_mem_reserved_device_memory_t;
> +DEFINE_XEN_GUEST_HANDLE(xen_mem_reserved_device_memory_t);
> +
> +struct xen_mem_reserved_device_memory_map {
> +    /* IN/OUT */
> +    unsigned int nr_entries;
> +    /* OUT */
> +    XEN_GUEST_HANDLE(xen_mem_reserved_device_memory_t) buffer;
> +};
> +typedef struct xen_mem_reserved_device_memory_map xen_mem_reserved_device_memory_map_t;
> +DEFINE_XEN_GUEST_HANDLE(xen_mem_reserved_device_memory_map_t);
> +
> +/* Next available subop number is 27 */
>
>   #endif /* __XEN_PUBLIC_MEMORY_H__ */
>
> --- a/xen/include/xlat.lst
> +++ b/xen/include/xlat.lst
> @@ -60,7 +60,8 @@
>   !	memory_exchange			memory.h
>   !	memory_map			memory.h
>   !	memory_reservation		memory.h
> -?	mem_access_op		memory.h
> +?	mem_access_op			memory.h
> +!	mem_reserved_device_memory_map	memory.h
>   !	pod_target			memory.h
>   !	remove_from_physmap		memory.h
>   ?	physdev_eoi			physdev.h
>
>

Re: [v5][PATCH 06/10] hvm_info_table: introduce nr_reserved_device_memory_map

[Chen, Tiejun]

On 2014/9/2 16:34, Jan Beulich wrote:
>>>> On 26.08.14 at 13:02, <tiejun.chen@intel.com> wrote:
>> 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.
>
> I assume you introduce this without consumer to limit patch size. In
> such a case title _and_ description should be more meaningful as
> to what this really does and what it's intended use is.

This patch involves two files, one is xen file, 
xen/include/public/hvm/hvm_info_table.h, and a tools file, 
tools/libxc/xc_hvm_build_x86.c.

So I'm considering to split with two small patches like this:

#1 Introduce this new field in xen/include/public/hvm/hvm_info_table.h

xen/hvm_info_table: introduce a new field nr_device_reserved_memory_map

In hvm_info_table this field represents the number of all device memory 
maps. It will be convenient to expose such a information to VM.

#2 Construct this field in tools/libxc/xc_hvm_build_x86.c

tools/libxc: construct nr_device_reserved_memory_map

While building hvm info, libxc is responsible for constructing
this number after check_drm_overlap().

Is it reasonable to use different patches covering xen internal and 
tools, respectively?

Or just one is already fine?

>
>> @@ -370,6 +375,9 @@ static int setup_guest(xc_interface *xch,
>>       rc = check_rmrr_overlap(xch, mmio_start, mmio_start);
>
> I skipped several tools side patches, but here I see that somewhere
> you still left the term "RMRR" in the code, when you were asked
> before to use more abstract naming (and this of course not only
> extended to the public interface).

I will replace those info with 'device reserved memory maps' and s/RMRR/DRM.

>
>>       if ( rc < 0 )
>>           goto error_out;
>> +    /* Always return entries number. */
>> +    else
>
> The "else" here is bogus considering the "goto" above.

Remove this 'else'.

>
>> --- 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;
>>   };
>
> This being defacto a private interface between tools and hvmloader
> I wonder if it wouldn't be better to put this before the (in the future)
> eventually growing vcpu_online[].
>

Any latest consideration? Could we push line above 'uint8_t 
vcpu_online[(HVM_MAX_VCPUS + 7)/8];'?

And I just think it may be reasonable and convenient to expose this info 
in hvm_info_table since this is a fixed value that we should record for 
all VMs.


Thanks
Tiejun

Re: [v5][PATCH 07/10] xen:x86:: support xc_reserved_device_memory_map in compat case

[Chen, Tiejun]

On 2014/9/2 16:35, Jan Beulich wrote:
>>>> On 26.08.14 at 13:02, <tiejun.chen@intel.com> wrote:
>> We also need to make sure xc_reserved_device_memory_map can
>> work in compat case.
>
> This ought to be part of the patch introducing the new operation.
>

Looks your patch already cover this but I have to test this later after 
a rebase.

Thanks
Tiejun

Re: [v5][PATCH 09/10] tools:firmware:hvmloader: check to reserve RMRR mappings in e820

[Chen, Tiejun]

On 2014/9/2 16:47, Jan Beulich wrote:
>>>> On 26.08.14 at 13:02, <tiejun.chen@intel.com> wrote:
>> +static unsigned int construct_rmrr_e820_maps(unsigned int nr,

s/construct_rmrr_e820_maps/construct_drm_e820_maps

>> +                                             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;

I rename this pair of variables in this whole function,

s/rmrr_start/drm_start
s/rmrr_end/drm_end

>> +    unsigned int insert = 0, do_insert = 0;
>> +
>> +do_real_construct:
>
> Please indent labels by at least one space (and there are further
> coding style issues to address elsewhere).

Is this necessary?

static struct tmem_object_root * obj_find(struct tmem_pool *pool, struct 
oid *oidp)
{
     struct rb_node *node;
     struct tmem_object_root *obj;

restart_find:
     read_lock(&pool->pool_rwlock);
     ...
                     read_unlock(&pool->pool_rwlock);
                     goto restart_find;

>
>> +    sum_nr = nr + nr_map;
>
> Afaict neither nr nor nr_map change before you get here the second
> (and last) time. Hence the calculation can be moved up (ideally into
> the initializer of the variable).

+    unsigned int i = 0, j = 0, m = 0, sum_nr = nr + nr_map;

>
>> +    for ( i = 0; i < nr_map; i++ )
>> +    {
>> +        rmrr_start = map[i].start_pfn << PAGE_SHIFT;
>> +        rmrr_end = rmrr_start + map[i].nr_pages * PAGE_SIZE;
>> +
>> +        for ( j = 0; j < nr; j++ )
>> +        {
>> +            end = e820[j].addr + e820[j].size;
>> +            start = e820[j+1].addr;
>
> This is not valid when j == nr - 1 (last iteration).
>
-        for ( j = 0; j < nr; j++ )
+        for ( j = 0; j < nr - 1; j++ )

>> +
>> +            /* Between those existing e820 entries. */
>> +            if ( (rmrr_start > end) && (rmrr_end < start) )
>> +            {
>> +                if (do_insert)

-                if (do_insert)
+                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)

-    if ( insert == nr_map)
+    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");

s/RMRR/DRM

Thanks
Tiejun

>> +    }
>> +
>> +    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
>
>
>
>

Re: [v5][PATCH 09/10] tools:firmware:hvmloader: check to reserve RMRR mappings in e820

[Chen, Tiejun]

On 2014/9/4 11:04, Chen, Tiejun wrote:
> On 2014/9/2 16:47, Jan Beulich wrote:
>>>>> On 26.08.14 at 13:02, <tiejun.chen@intel.com> wrote:
>>> +static unsigned int construct_rmrr_e820_maps(unsigned int nr,
>
> s/construct_rmrr_e820_maps/construct_drm_e820_maps

Based on that first patch I think I should rename some terms in other 
patches:

s/device reserved memory/reserved device memory
s/DRM/RDM/
s/drm/rdm

Thanks
Tiejun

>
>>> +                                             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;
>
> I rename this pair of variables in this whole function,
>
> s/rmrr_start/drm_start
> s/rmrr_end/drm_end
>
>>> +    unsigned int insert = 0, do_insert = 0;
>>> +
>>> +do_real_construct:
>>
>> Please indent labels by at least one space (and there are further
>> coding style issues to address elsewhere).
>
> Is this necessary?
>
> static struct tmem_object_root * obj_find(struct tmem_pool *pool, struct
> oid *oidp)
> {
>      struct rb_node *node;
>      struct tmem_object_root *obj;
>
> restart_find:
>      read_lock(&pool->pool_rwlock);
>      ...
>                      read_unlock(&pool->pool_rwlock);
>                      goto restart_find;
>
>>
>>> +    sum_nr = nr + nr_map;
>>
>> Afaict neither nr nor nr_map change before you get here the second
>> (and last) time. Hence the calculation can be moved up (ideally into
>> the initializer of the variable).
>
> +    unsigned int i = 0, j = 0, m = 0, sum_nr = nr + nr_map;
>
>>
>>> +    for ( i = 0; i < nr_map; i++ )
>>> +    {
>>> +        rmrr_start = map[i].start_pfn << PAGE_SHIFT;
>>> +        rmrr_end = rmrr_start + map[i].nr_pages * PAGE_SIZE;
>>> +
>>> +        for ( j = 0; j < nr; j++ )
>>> +        {
>>> +            end = e820[j].addr + e820[j].size;
>>> +            start = e820[j+1].addr;
>>
>> This is not valid when j == nr - 1 (last iteration).
>>
> -        for ( j = 0; j < nr; j++ )
> +        for ( j = 0; j < nr - 1; j++ )
>
>>> +
>>> +            /* Between those existing e820 entries. */
>>> +            if ( (rmrr_start > end) && (rmrr_end < start) )
>>> +            {
>>> +                if (do_insert)
>
> -                if (do_insert)
> +                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)
>
> -    if ( insert == nr_map)
> +    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");
>
> s/RMRR/DRM
>
> Thanks
> Tiejun
>
>>> +    }
>>> +
>>> +    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
>>
>>
>>
>>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>
>

Re: [v5][PATCH 06/10] hvm_info_table: introduce nr_reserved_device_memory_map

[Jan Beulich]

>>> On 04.09.14 at 04:07, <tiejun.chen@intel.com> wrote:
> On 2014/9/2 16:34, Jan Beulich wrote:
>>>>> On 26.08.14 at 13:02, <tiejun.chen@intel.com> wrote:
>>> 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.
>>
>> I assume you introduce this without consumer to limit patch size. In
>> such a case title _and_ description should be more meaningful as
>> to what this really does and what it's intended use is.
> 
> This patch involves two files, one is xen file, 
> xen/include/public/hvm/hvm_info_table.h, and a tools file, 
> tools/libxc/xc_hvm_build_x86.c.
> 
> So I'm considering to split with two small patches like this:
> 
> #1 Introduce this new field in xen/include/public/hvm/hvm_info_table.h
> 
> xen/hvm_info_table: introduce a new field nr_device_reserved_memory_map
> 
> In hvm_info_table this field represents the number of all device memory 
> maps. It will be convenient to expose such a information to VM.
> 
> #2 Construct this field in tools/libxc/xc_hvm_build_x86.c
> 
> tools/libxc: construct nr_device_reserved_memory_map
> 
> While building hvm info, libxc is responsible for constructing
> this number after check_drm_overlap().
> 
> Is it reasonable to use different patches covering xen internal and 
> tools, respectively?

I don't see the "xen internal" here. As said this is an interface
between tool stack and hvmloader, both of which are tools
components.

> Or just one is already fine?

I think so.

>>> --- 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;
>>>   };
>>
>> This being defacto a private interface between tools and hvmloader
>> I wonder if it wouldn't be better to put this before the (in the future)
>> eventually growing vcpu_online[].
>>
> 
> Any latest consideration? Could we push line above 'uint8_t 
> vcpu_online[(HVM_MAX_VCPUS + 7)/8];'?
> 
> And I just think it may be reasonable and convenient to expose this info 
> in hvm_info_table since this is a fixed value that we should record for 
> all VMs.

I think information easily obtained via hypercall doesn't belong here,
but I'd also prefer to have input from the tools maintainers here.

Jan

Re: [v5][PATCH 09/10] tools:firmware:hvmloader: check to reserve RMRR mappings in e820

[Jan Beulich]

>>> On 04.09.14 at 05:04, <tiejun.chen@intel.com> wrote:
> On 2014/9/2 16:47, Jan Beulich wrote:
>>>>> On 26.08.14 at 13:02, <tiejun.chen@intel.com> wrote:
>>> +    unsigned int insert = 0, do_insert = 0;
>>> +
>>> +do_real_construct:
>>
>> Please indent labels by at least one space (and there are further
>> coding style issues to address elsewhere).
> 
> Is this necessary?
> 
> static struct tmem_object_root * obj_find(struct tmem_pool *pool, struct 
> oid *oidp)
> {
>      struct rb_node *node;
>      struct tmem_object_root *obj;
> 
> restart_find:
>      read_lock(&pool->pool_rwlock);
>      ...
>                      read_unlock(&pool->pool_rwlock);
>                      goto restart_find;

Question and cited code fragment don't fit together for me, so I
don't think I understand what you're asking. Taking the question
alone - yes, obeying to coding style is necessary.

>>> +    for ( i = 0; i < nr_map; i++ )
>>> +    {
>>> +        rmrr_start = map[i].start_pfn << PAGE_SHIFT;
>>> +        rmrr_end = rmrr_start + map[i].nr_pages * PAGE_SIZE;
>>> +
>>> +        for ( j = 0; j < nr; j++ )
>>> +        {
>>> +            end = e820[j].addr + e820[j].size;
>>> +            start = e820[j+1].addr;
>>
>> This is not valid when j == nr - 1 (last iteration).
>>
> -        for ( j = 0; j < nr; j++ )
> +        for ( j = 0; j < nr - 1; j++ )

And this would skip the last region - I'm not sure that's correct.

Jan

Re: [v5][PATCH 06/10] hvm_info_table: introduce nr_reserved_device_memory_map

[Chen, Tiejun]

On 2014/9/4 14:32, Jan Beulich wrote:
>>>> On 04.09.14 at 04:07, <tiejun.chen@intel.com> wrote:
>> On 2014/9/2 16:34, Jan Beulich wrote:
>>>>>> On 26.08.14 at 13:02, <tiejun.chen@intel.com> wrote:
>>>> 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.
>>>
>>> I assume you introduce this without consumer to limit patch size. In
>>> such a case title _and_ description should be more meaningful as
>>> to what this really does and what it's intended use is.
>>
>> This patch involves two files, one is xen file,
>> xen/include/public/hvm/hvm_info_table.h, and a tools file,
>> tools/libxc/xc_hvm_build_x86.c.
>>
>> So I'm considering to split with two small patches like this:
>>
>> #1 Introduce this new field in xen/include/public/hvm/hvm_info_table.h
>>
>> xen/hvm_info_table: introduce a new field nr_device_reserved_memory_map
>>
>> In hvm_info_table this field represents the number of all device memory
>> maps. It will be convenient to expose such a information to VM.
>>
>> #2 Construct this field in tools/libxc/xc_hvm_build_x86.c
>>
>> tools/libxc: construct nr_device_reserved_memory_map
>>
>> While building hvm info, libxc is responsible for constructing
>> this number after check_drm_overlap().
>>
>> Is it reasonable to use different patches covering xen internal and
>> tools, respectively?
>
> I don't see the "xen internal" here. As said this is an interface
> between tool stack and hvmloader, both of which are tools
> components.

Understood.

>
>> Or just one is already fine?
>
> I think so.
>
>>>> --- 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;
>>>>    };
>>>
>>> This being defacto a private interface between tools and hvmloader
>>> I wonder if it wouldn't be better to put this before the (in the future)
>>> eventually growing vcpu_online[].
>>>
>>
>> Any latest consideration? Could we push line above 'uint8_t
>> vcpu_online[(HVM_MAX_VCPUS + 7)/8];'?
>>
>> And I just think it may be reasonable and convenient to expose this info
>> in hvm_info_table since this is a fixed value that we should record for
>> all VMs.
>
> I think information easily obtained via hypercall doesn't belong here,

Yes, but just don't feel better to call twice hypercall in hvmloader 
since this means we have to reallocate memory to store all entries.

This is a little bit different what we did in libxc since we can't get 
that number of all entries directly in libxc. But here it may be fine.

> but I'd also prefer to have input from the tools maintainers here.
>

I think I already include all guys by get_maintainer.pc here, but maybe 
I need ping them actively, so you think who can give us a ack or nack 
eventually?

Thanks
Tiejun

Re: [v5][PATCH 06/10] hvm_info_table: introduce nr_reserved_device_memory_map

[Chen, Tiejun]

On 2014/9/4 15:17, Jan Beulich wrote:
>>>> On 04.09.14 at 08:55, <tiejun.chen@intel.com> wrote:
>> On 2014/9/4 14:32, Jan Beulich wrote:
>>>>>> On 04.09.14 at 04:07, <tiejun.chen@intel.com> wrote:
>>>> On 2014/9/2 16:34, Jan Beulich wrote:
>>>>>>>> On 26.08.14 at 13:02, <tiejun.chen@intel.com> wrote:
>>>>>> --- 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;
>>>>>>     };
>>>>>
>>>>> This being defacto a private interface between tools and hvmloader
>>>>> I wonder if it wouldn't be better to put this before the (in the future)
>>>>> eventually growing vcpu_online[].
>>>>>
>>>>
>>>> Any latest consideration? Could we push line above 'uint8_t
>>>> vcpu_online[(HVM_MAX_VCPUS + 7)/8];'?
>>>>
>>>> And I just think it may be reasonable and convenient to expose this info
>>>> in hvm_info_table since this is a fixed value that we should record for
>>>> all VMs.
>>>
>>> I think information easily obtained via hypercall doesn't belong here,
>>
>> Yes, but just don't feel better to call twice hypercall in hvmloader
>> since this means we have to reallocate memory to store all entries.
>>
>> This is a little bit different what we did in libxc since we can't get
>> that number of all entries directly in libxc. But here it may be fine.
>
> Hmm, latching the output of a hypercall here seems even less
> reasonable to me. What gets communicated here ought to be
> things that have no other good way of telling the VM - at least
> that's my understanding of this structure.
>
>>> but I'd also prefer to have input from the tools maintainers here.
>>>
>>
>> I think I already include all guys by get_maintainer.pc here, but maybe
>> I need ping them actively, so you think who can give us a ack or nack
>> eventually?
>
> People have been traveling a lot lately, so give them a little more
> time.
>

Jan,

As I understand this point is no a big deal, or even the tools 
maintainer have further comment about this point, but I think that 
should not be difficult to address. I guess it may not bring too much 
more change.

So maybe I can send a new revision rebased on that first patch in 
advance since as you know there are some other good comments from you, 
which actually don't conflict that remaining argument.

Thanks
Tiejun

Thanks
Tiejun