* [v4][PATCH 1/9] xen:vtd:rmrr: export acpi_rmrr_units
2014-08-22 10:09 [v4][PATCH 0/9] xen: reserve RMRR to avoid conflicting MMIO/RAM Tiejun Chen
@ 2014-08-22 10:09 ` Tiejun Chen
2014-08-22 10:09 ` [v4][PATCH 2/9] xen:x86: define a new hypercall to get RMRR mappings Tiejun Chen
` (7 subsequent siblings)
8 siblings, 0 replies; 30+ messages in thread
From: Tiejun Chen @ 2014-08-22 10:09 UTC (permalink / raw)
To: JBeulich, ian.jackson, stefano.stabellini, ian.campbell,
yang.z.zhang, kevin.tian
Cc: xen-devel
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
^ permalink raw reply related [flat|nested] 30+ messages in thread* [v4][PATCH 2/9] xen:x86: define a new hypercall to get RMRR mappings
2014-08-22 10:09 [v4][PATCH 0/9] xen: reserve RMRR to avoid conflicting MMIO/RAM Tiejun Chen
2014-08-22 10:09 ` [v4][PATCH 1/9] xen:vtd:rmrr: export acpi_rmrr_units Tiejun Chen
@ 2014-08-22 10:09 ` Tiejun Chen
2014-08-22 10:53 ` Andrew Cooper
2014-08-22 10:09 ` [v4][PATCH 3/9] tools:libxc: introduce hypercall for xc_reserved_device_memory_map Tiejun Chen
` (6 subsequent siblings)
8 siblings, 1 reply; 30+ messages in thread
From: Tiejun Chen @ 2014-08-22 10:09 UTC (permalink / raw)
To: JBeulich, ian.jackson, stefano.stabellini, ian.campbell,
yang.z.zhang, kevin.tian
Cc: xen-devel
We need this new hypercall to get RMRR mapping for VM.
Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
---
xen/arch/x86/mm.c | 71 +++++++++++++++++++++++++++++++++++++++++++++
xen/include/public/memory.h | 37 ++++++++++++++++++++++-
2 files changed, 107 insertions(+), 1 deletion(-)
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index d23cb3f..e0d6650 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -123,6 +123,7 @@
#include <asm/setup.h>
#include <asm/fixmap.h>
#include <asm/pci.h>
+#include <asm/acpi.h>
/* Mapping of the fixmap space needed early. */
l1_pgentry_t __attribute__ ((__section__ (".bss.page_aligned")))
@@ -4842,6 +4843,76 @@ long arch_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
return rc;
}
+ case XENMEM_reserved_device_memory_map:
+ {
+ struct xen_reserved_device_memory_map map;
+ XEN_GUEST_HANDLE(xen_reserved_device_memory_t) buffer;
+ XEN_GUEST_HANDLE_PARAM(xen_reserved_device_memory_t) buffer_param;
+ unsigned int i = 0;
+ static unsigned int nr_entries = 0;
+ static struct xen_reserved_device_memory *rmrr_map;
+ struct acpi_rmrr_unit *rmrr;
+
+ if ( copy_from_guest(&map, arg, 1) )
+ return -EFAULT;
+
+ if ( !nr_entries )
+ /* Currently we just need to cover RMRR. */
+ list_for_each_entry( rmrr, &acpi_rmrr_units, list )
+ nr_entries++;
+
+ if ( !nr_entries )
+ return -ENOENT;
+ else
+ {
+ if ( rmrr_map == NULL )
+ {
+ rmrr_map = xmalloc_array(xen_reserved_device_memory_t,
+ nr_entries);
+ if ( rmrr_map == NULL )
+ {
+ return -ENOMEM;
+ }
+
+ list_for_each_entry( rmrr, &acpi_rmrr_units, list )
+ {
+ rmrr_map[i].pfn = rmrr->base_address >> PAGE_SHIFT;
+ rmrr_map[i].count = PAGE_ALIGN(rmrr->end_address -
+ rmrr->base_address) /
+ PAGE_SIZE;
+ i++;
+ }
+ }
+ }
+
+ if ( map.nr_entries < nr_entries )
+ {
+ map.nr_entries = nr_entries;
+ if ( copy_to_guest(arg, &map, 1) )
+ return -EFAULT;
+ return -ENOBUFS;
+ }
+
+ map.nr_entries = nr_entries;
+ buffer_param = guest_handle_cast(map.buffer,
+ xen_reserved_device_memory_t);
+ buffer = guest_handle_from_param(buffer_param,
+ xen_reserved_device_memory_t);
+ if ( !guest_handle_okay(buffer, map.nr_entries) )
+ return -EFAULT;
+
+ for ( i = 0; i < map.nr_entries; ++i )
+ {
+ if ( copy_to_guest_offset(buffer, i, rmrr_map + i, 1) )
+ return -EFAULT;
+ }
+
+ if ( copy_to_guest(arg, &map, 1) )
+ return -EFAULT;
+
+ return 0;
+ }
+
default:
return subarch_memory_op(cmd, arg);
}
diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h
index 2c57aa0..8481843 100644
--- a/xen/include/public/memory.h
+++ b/xen/include/public/memory.h
@@ -523,7 +523,42 @@ DEFINE_XEN_GUEST_HANDLE(xen_mem_sharing_op_t);
#endif /* defined(__XEN__) || defined(__XEN_TOOLS__) */
-/* Next available subop number is 26 */
+/*
+ * Some devices may reserve some range.
+ *
+ * Currently we just have RMRR
+ * - Reserved memory Region Reporting Structure,
+ * So returns the RMRR memory map as it was when the domain
+ * was started.
+ */
+#define XENMEM_reserved_device_memory_map 26
+struct xen_reserved_device_memory {
+ /* PFN of the current mapping of the page. */
+ xen_pfn_t pfn;
+ /* Number of the current mapping pages. */
+ xen_ulong_t count;
+};
+typedef struct xen_reserved_device_memory xen_reserved_device_memory_t;
+DEFINE_XEN_GUEST_HANDLE(xen_reserved_device_memory_t);
+
+struct xen_reserved_device_memory_map {
+ /*
+ * On call the number of entries which can be stored in buffer. On
+ * return the number of entries which have been stored in
+ * buffer.
+ */
+ unsigned int nr_entries;
+
+ /*
+ * Entries in the buffer are in the same format as
+ * xen_reserved_device_memory.
+ */
+ XEN_GUEST_HANDLE(void) buffer;
+};
+typedef struct xen_reserved_device_memory_map xen_reserved_device_memory_map_t;
+DEFINE_XEN_GUEST_HANDLE(xen_reserved_device_memory_map_t);
+
+/* Next available subop number is 27 */
#endif /* __XEN_PUBLIC_MEMORY_H__ */
--
1.9.1
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [v4][PATCH 2/9] xen:x86: define a new hypercall to get RMRR mappings
2014-08-22 10:09 ` [v4][PATCH 2/9] xen:x86: define a new hypercall to get RMRR mappings Tiejun Chen
@ 2014-08-22 10:53 ` Andrew Cooper
2014-08-22 11:36 ` Jan Beulich
2014-08-25 11:21 ` Chen, Tiejun
0 siblings, 2 replies; 30+ messages in thread
From: Andrew Cooper @ 2014-08-22 10:53 UTC (permalink / raw)
To: Tiejun Chen, JBeulich, ian.jackson, stefano.stabellini,
ian.campbell, yang.z.zhang, kevin.tian
Cc: xen-devel
On 22/08/14 11:09, Tiejun Chen wrote:
> We need this new hypercall to get RMRR mapping for VM.
>
> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
> ---
> xen/arch/x86/mm.c | 71 +++++++++++++++++++++++++++++++++++++++++++++
> xen/include/public/memory.h | 37 ++++++++++++++++++++++-
> 2 files changed, 107 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> index d23cb3f..e0d6650 100644
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -123,6 +123,7 @@
> #include <asm/setup.h>
> #include <asm/fixmap.h>
> #include <asm/pci.h>
> +#include <asm/acpi.h>
>
> /* Mapping of the fixmap space needed early. */
> l1_pgentry_t __attribute__ ((__section__ (".bss.page_aligned")))
> @@ -4842,6 +4843,76 @@ long arch_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
> return rc;
> }
>
> + case XENMEM_reserved_device_memory_map:
> + {
> + struct xen_reserved_device_memory_map map;
> + XEN_GUEST_HANDLE(xen_reserved_device_memory_t) buffer;
> + XEN_GUEST_HANDLE_PARAM(xen_reserved_device_memory_t) buffer_param;
> + unsigned int i = 0;
> + static unsigned int nr_entries = 0;
> + static struct xen_reserved_device_memory *rmrr_map;
Absolutely not. This hypercall can easy be run concurrently.
> + struct acpi_rmrr_unit *rmrr;
> +
> + if ( copy_from_guest(&map, arg, 1) )
> + return -EFAULT;
> +
> + if ( !nr_entries )
> + /* Currently we just need to cover RMRR. */
> + list_for_each_entry( rmrr, &acpi_rmrr_units, list )
> + nr_entries++;
Maintain a global count as entries are added/removed from this list. It
is a waste of time recounting this list for each hypercall.
> +
> + if ( !nr_entries )
> + return -ENOENT;
> + else
> + {
> + if ( rmrr_map == NULL )
> + {
> + rmrr_map = xmalloc_array(xen_reserved_device_memory_t,
> + nr_entries);
You can do all of this without any memory allocation.
> + if ( rmrr_map == NULL )
> + {
> + return -ENOMEM;
> + }
> +
> + list_for_each_entry( rmrr, &acpi_rmrr_units, list )
> + {
> + rmrr_map[i].pfn = rmrr->base_address >> PAGE_SHIFT;
> + rmrr_map[i].count = PAGE_ALIGN(rmrr->end_address -
> + rmrr->base_address) /
> + PAGE_SIZE;
> + i++;
> + }
> + }
> + }
> +
> + if ( map.nr_entries < nr_entries )
> + {
> + map.nr_entries = nr_entries;
> + if ( copy_to_guest(arg, &map, 1) )
> + return -EFAULT;
> + return -ENOBUFS;
> + }
> +
> + map.nr_entries = nr_entries;
> + buffer_param = guest_handle_cast(map.buffer,
> + xen_reserved_device_memory_t);
> + buffer = guest_handle_from_param(buffer_param,
> + xen_reserved_device_memory_t);
> + if ( !guest_handle_okay(buffer, map.nr_entries) )
> + return -EFAULT;
> +
> + for ( i = 0; i < map.nr_entries; ++i )
> + {
> + if ( copy_to_guest_offset(buffer, i, rmrr_map + i, 1) )
> + return -EFAULT;
> + }
> +
> + if ( copy_to_guest(arg, &map, 1) )
> + return -EFAULT;
> +
> + return 0;
> + }
> +
> default:
> return subarch_memory_op(cmd, arg);
> }
> diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h
> index 2c57aa0..8481843 100644
> --- a/xen/include/public/memory.h
> +++ b/xen/include/public/memory.h
> @@ -523,7 +523,42 @@ DEFINE_XEN_GUEST_HANDLE(xen_mem_sharing_op_t);
>
> #endif /* defined(__XEN__) || defined(__XEN_TOOLS__) */
>
> -/* Next available subop number is 26 */
> +/*
> + * Some devices may reserve some range.
"range" is not a useful unit of measure.
> + *
> + * Currently we just have RMRR
> + * - Reserved memory Region Reporting Structure,
> + * So returns the RMRR memory map as it was when the domain
> + * was started.
> + */
> +#define XENMEM_reserved_device_memory_map 26
> +struct xen_reserved_device_memory {
xen_mem_ to match the prevailing style
> + /* PFN of the current mapping of the page. */
> + xen_pfn_t pfn;
> + /* Number of the current mapping pages. */
> + xen_ulong_t count;
> +};
This struct marks a range, but the fields don't make it clear. I would
suggest "start" and "nr_frames" as names.
~Andrew
> +typedef struct xen_reserved_device_memory xen_reserved_device_memory_t;
> +DEFINE_XEN_GUEST_HANDLE(xen_reserved_device_memory_t);
> +
> +struct xen_reserved_device_memory_map {
> + /*
> + * On call the number of entries which can be stored in buffer. On
> + * return the number of entries which have been stored in
> + * buffer.
> + */
> + unsigned int nr_entries;
> +
> + /*
> + * Entries in the buffer are in the same format as
> + * xen_reserved_device_memory.
> + */
> + XEN_GUEST_HANDLE(void) buffer;
> +};
> +typedef struct xen_reserved_device_memory_map xen_reserved_device_memory_map_t;
> +DEFINE_XEN_GUEST_HANDLE(xen_reserved_device_memory_map_t);
> +
> +/* Next available subop number is 27 */
>
> #endif /* __XEN_PUBLIC_MEMORY_H__ */
>
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [v4][PATCH 2/9] xen:x86: define a new hypercall to get RMRR mappings
2014-08-22 10:53 ` Andrew Cooper
@ 2014-08-22 11:36 ` Jan Beulich
2014-08-25 11:03 ` Chen, Tiejun
2014-08-25 11:21 ` Chen, Tiejun
1 sibling, 1 reply; 30+ messages in thread
From: Jan Beulich @ 2014-08-22 11:36 UTC (permalink / raw)
To: Andrew Cooper, ian.campbell, ian.jackson, stefano.stabellini,
kevin.tian, Tiejun Chen, yang.z.zhang
Cc: xen-devel
>>> On 22.08.14 at 12:53, <andrew.cooper3@citrix.com> wrote:
> On 22/08/14 11:09, Tiejun Chen wrote:
>> + /* PFN of the current mapping of the page. */
>> + xen_pfn_t pfn;
>> + /* Number of the current mapping pages. */
>> + xen_ulong_t count;
>> +};
>
> This struct marks a range, but the fields don't make it clear. I would
> suggest "start" and "nr_frames" as names.
Perhaps "start_pfn" to be even more explicit (and not just
depend on the field's type), and then maybe also "nr_pfns"
or "nr_pages".
Jan
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [v4][PATCH 2/9] xen:x86: define a new hypercall to get RMRR mappings
2014-08-22 11:36 ` Jan Beulich
@ 2014-08-25 11:03 ` Chen, Tiejun
0 siblings, 0 replies; 30+ messages in thread
From: Chen, Tiejun @ 2014-08-25 11:03 UTC (permalink / raw)
To: Jan Beulich, Andrew Cooper, ian.campbell, ian.jackson,
stefano.stabellini, kevin.tian, yang.z.zhang
Cc: xen-devel
On 2014/8/22 19:36, Jan Beulich wrote:
>>>> On 22.08.14 at 12:53, <andrew.cooper3@citrix.com> wrote:
>> On 22/08/14 11:09, Tiejun Chen wrote:
>>> + /* PFN of the current mapping of the page. */
>>> + xen_pfn_t pfn;
>>> + /* Number of the current mapping pages. */
>>> + xen_ulong_t count;
>>> +};
>>
>> This struct marks a range, but the fields don't make it clear. I would
>> suggest "start" and "nr_frames" as names.
>
> Perhaps "start_pfn" to be even more explicit (and not just
> depend on the field's type), and then maybe also "nr_pfns"
> or "nr_pages".
>
Okay, I will go this combo, 'start_pfn' and 'nr_pages'.
Thanks
Tiejun
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [v4][PATCH 2/9] xen:x86: define a new hypercall to get RMRR mappings
2014-08-22 10:53 ` Andrew Cooper
2014-08-22 11:36 ` Jan Beulich
@ 2014-08-25 11:21 ` Chen, Tiejun
2014-08-25 12:07 ` Andrew Cooper
1 sibling, 1 reply; 30+ messages in thread
From: Chen, Tiejun @ 2014-08-25 11:21 UTC (permalink / raw)
To: Andrew Cooper, JBeulich, ian.jackson, stefano.stabellini,
ian.campbell, yang.z.zhang, kevin.tian
Cc: xen-devel
On 2014/8/22 18:53, Andrew Cooper wrote:
> On 22/08/14 11:09, Tiejun Chen wrote:
>> We need this new hypercall to get RMRR mapping for VM.
>>
>> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
>> ---
>> xen/arch/x86/mm.c | 71 +++++++++++++++++++++++++++++++++++++++++++++
>> xen/include/public/memory.h | 37 ++++++++++++++++++++++-
>> 2 files changed, 107 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
>> index d23cb3f..e0d6650 100644
>> --- a/xen/arch/x86/mm.c
>> +++ b/xen/arch/x86/mm.c
>> @@ -123,6 +123,7 @@
>> #include <asm/setup.h>
>> #include <asm/fixmap.h>
>> #include <asm/pci.h>
>> +#include <asm/acpi.h>
>>
>> /* Mapping of the fixmap space needed early. */
>> l1_pgentry_t __attribute__ ((__section__ (".bss.page_aligned")))
>> @@ -4842,6 +4843,76 @@ long arch_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>> return rc;
>> }
>>
>> + case XENMEM_reserved_device_memory_map:
>> + {
>> + struct xen_reserved_device_memory_map map;
>> + XEN_GUEST_HANDLE(xen_reserved_device_memory_t) buffer;
>> + XEN_GUEST_HANDLE_PARAM(xen_reserved_device_memory_t) buffer_param;
>> + unsigned int i = 0;
>> + static unsigned int nr_entries = 0;
>> + static struct xen_reserved_device_memory *rmrr_map;
>
> Absolutely not. This hypercall can easy be run concurrently.
>
>> + struct acpi_rmrr_unit *rmrr;
>> +
>> + if ( copy_from_guest(&map, arg, 1) )
>> + return -EFAULT;
>> +
>> + if ( !nr_entries )
>> + /* Currently we just need to cover RMRR. */
>> + list_for_each_entry( rmrr, &acpi_rmrr_units, list )
>> + nr_entries++;
>
> Maintain a global count as entries are added/removed from this list. It
> is a waste of time recounting this list for each hypercall.
Are you saying push this 'nr_entries' as a global count somewhere? I
guess I can set this when we first construct acpi_rmrr_units in ACPI stuff.
>
>> +
>> + if ( !nr_entries )
>> + return -ENOENT;
>> + else
>> + {
>> + if ( rmrr_map == NULL )
>> + {
>> + rmrr_map = xmalloc_array(xen_reserved_device_memory_t,
>> + nr_entries);
>
> You can do all of this without any memory allocation.
I will check this.
>
>> + if ( rmrr_map == NULL )
>> + {
>> + return -ENOMEM;
>> + }
>> +
>> + list_for_each_entry( rmrr, &acpi_rmrr_units, list )
>> + {
>> + rmrr_map[i].pfn = rmrr->base_address >> PAGE_SHIFT;
>> + rmrr_map[i].count = PAGE_ALIGN(rmrr->end_address -
>> + rmrr->base_address) /
>> + PAGE_SIZE;
>> + i++;
>> + }
>> + }
>> + }
>> +
>> + if ( map.nr_entries < nr_entries )
>> + {
>> + map.nr_entries = nr_entries;
>> + if ( copy_to_guest(arg, &map, 1) )
>> + return -EFAULT;
>> + return -ENOBUFS;
>> + }
>> +
>> + map.nr_entries = nr_entries;
>> + buffer_param = guest_handle_cast(map.buffer,
>> + xen_reserved_device_memory_t);
>> + buffer = guest_handle_from_param(buffer_param,
>> + xen_reserved_device_memory_t);
>> + if ( !guest_handle_okay(buffer, map.nr_entries) )
>> + return -EFAULT;
>> +
>> + for ( i = 0; i < map.nr_entries; ++i )
>> + {
>> + if ( copy_to_guest_offset(buffer, i, rmrr_map + i, 1) )
>> + return -EFAULT;
>> + }
>> +
>> + if ( copy_to_guest(arg, &map, 1) )
>> + return -EFAULT;
>> +
>> + return 0;
>> + }
>> +
>> default:
>> return subarch_memory_op(cmd, arg);
>> }
>> diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h
>> index 2c57aa0..8481843 100644
>> --- a/xen/include/public/memory.h
>> +++ b/xen/include/public/memory.h
>> @@ -523,7 +523,42 @@ DEFINE_XEN_GUEST_HANDLE(xen_mem_sharing_op_t);
>>
>> #endif /* defined(__XEN__) || defined(__XEN_TOOLS__) */
>>
>> -/* Next available subop number is 26 */
>> +/*
>> + * Some devices may reserve some range.
>
> "range" is not a useful unit of measure.
So what about this?
The regions of memory used for some devices should be reserved.
>
>> + *
>> + * Currently we just have RMRR
>> + * - Reserved memory Region Reporting Structure,
>> + * So returns the RMRR memory map as it was when the domain
>> + * was started.
>> + */
>> +#define XENMEM_reserved_device_memory_map 26
>> +struct xen_reserved_device_memory {
>
> xen_mem_ to match the prevailing style
Okay.
>
>> + /* PFN of the current mapping of the page. */
>> + xen_pfn_t pfn;
>> + /* Number of the current mapping pages. */
>> + xen_ulong_t count;
>> +};
>
> This struct marks a range, but the fields don't make it clear. I would
> suggest "start" and "nr_frames" as names.
>
I will prefer Jan's suggestion.
Thanks
Tiejun
> ~Andrew
>
>> +typedef struct xen_reserved_device_memory xen_reserved_device_memory_t;
>> +DEFINE_XEN_GUEST_HANDLE(xen_reserved_device_memory_t);
>> +
>> +struct xen_reserved_device_memory_map {
>> + /*
>> + * On call the number of entries which can be stored in buffer. On
>> + * return the number of entries which have been stored in
>> + * buffer.
>> + */
>> + unsigned int nr_entries;
>> +
>> + /*
>> + * Entries in the buffer are in the same format as
>> + * xen_reserved_device_memory.
>> + */
>> + XEN_GUEST_HANDLE(void) buffer;
>> +};
>> +typedef struct xen_reserved_device_memory_map xen_reserved_device_memory_map_t;
>> +DEFINE_XEN_GUEST_HANDLE(xen_reserved_device_memory_map_t);
>> +
>> +/* Next available subop number is 27 */
>>
>> #endif /* __XEN_PUBLIC_MEMORY_H__ */
>>
>
>
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [v4][PATCH 2/9] xen:x86: define a new hypercall to get RMRR mappings
2014-08-25 11:21 ` Chen, Tiejun
@ 2014-08-25 12:07 ` Andrew Cooper
2014-08-26 3:12 ` Chen, Tiejun
0 siblings, 1 reply; 30+ messages in thread
From: Andrew Cooper @ 2014-08-25 12:07 UTC (permalink / raw)
To: Chen, Tiejun, JBeulich, ian.jackson, stefano.stabellini,
ian.campbell, yang.z.zhang, kevin.tian
Cc: xen-devel
On 25/08/14 12:21, Chen, Tiejun wrote:
> On 2014/8/22 18:53, Andrew Cooper wrote:
>> On 22/08/14 11:09, Tiejun Chen wrote:
>>> We need this new hypercall to get RMRR mapping for VM.
>>>
>>> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
>>> ---
>>> xen/arch/x86/mm.c | 71
>>> +++++++++++++++++++++++++++++++++++++++++++++
>>> xen/include/public/memory.h | 37 ++++++++++++++++++++++-
>>> 2 files changed, 107 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
>>> index d23cb3f..e0d6650 100644
>>> --- a/xen/arch/x86/mm.c
>>> +++ b/xen/arch/x86/mm.c
>>> @@ -123,6 +123,7 @@
>>> #include <asm/setup.h>
>>> #include <asm/fixmap.h>
>>> #include <asm/pci.h>
>>> +#include <asm/acpi.h>
>>>
>>> /* Mapping of the fixmap space needed early. */
>>> l1_pgentry_t __attribute__ ((__section__ (".bss.page_aligned")))
>>> @@ -4842,6 +4843,76 @@ long arch_memory_op(unsigned long cmd,
>>> XEN_GUEST_HANDLE_PARAM(void) arg)
>>> return rc;
>>> }
>>>
>>> + case XENMEM_reserved_device_memory_map:
>>> + {
>>> + struct xen_reserved_device_memory_map map;
>>> + XEN_GUEST_HANDLE(xen_reserved_device_memory_t) buffer;
>>> + XEN_GUEST_HANDLE_PARAM(xen_reserved_device_memory_t)
>>> buffer_param;
>>> + unsigned int i = 0;
>>> + static unsigned int nr_entries = 0;
>>> + static struct xen_reserved_device_memory *rmrr_map;
>>
>> Absolutely not. This hypercall can easy be run concurrently.
>>
>>> + struct acpi_rmrr_unit *rmrr;
>>> +
>>> + if ( copy_from_guest(&map, arg, 1) )
>>> + return -EFAULT;
>>> +
>>> + if ( !nr_entries )
>>> + /* Currently we just need to cover RMRR. */
>>> + list_for_each_entry( rmrr, &acpi_rmrr_units, list )
>>> + nr_entries++;
>>
>> Maintain a global count as entries are added/removed from this list. It
>> is a waste of time recounting this list for each hypercall.
>
> Are you saying push this 'nr_entries' as a global count somewhere? I
> guess I can set this when we first construct acpi_rmrr_units in ACPI
> stuff.
Not named "nr_entries", but yes. It is constant after boot.
>
>>
>>> +
>>> + if ( !nr_entries )
>>> + return -ENOENT;
>>> + else
>>> + {
>>> + if ( rmrr_map == NULL )
>>> + {
>>> + rmrr_map = xmalloc_array(xen_reserved_device_memory_t,
>>> + nr_entries);
>>
>> You can do all of this without any memory allocation.
>
> I will check this.
It is easy...
>
>>
>>> + if ( rmrr_map == NULL )
>>> + {
>>> + return -ENOMEM;
>>> + }
>>> +
>>> + list_for_each_entry( rmrr, &acpi_rmrr_units, list )
>>> + {
>>> + rmrr_map[i].pfn = rmrr->base_address >>
>>> PAGE_SHIFT;
>>> + rmrr_map[i].count = PAGE_ALIGN(rmrr->end_address -
>>> +
>>> rmrr->base_address) /
>>> + PAGE_SIZE;
>>> + i++;
In this loop, construct one on the stack and copy_to_guest, breaking if
there is a fault or you exceed the guests array.
>>> + }
>>> + }
>>> + }
>>> +
>>> + if ( map.nr_entries < nr_entries )
>>> + {
>>> + map.nr_entries = nr_entries;
>>> + if ( copy_to_guest(arg, &map, 1) )
>>> + return -EFAULT;
>>> + return -ENOBUFS;
>>> + }
>>> +
>>> + map.nr_entries = nr_entries;
>>> + buffer_param = guest_handle_cast(map.buffer,
>>> +
>>> xen_reserved_device_memory_t);
>>> + buffer = guest_handle_from_param(buffer_param,
>>> +
>>> xen_reserved_device_memory_t);
>>> + if ( !guest_handle_okay(buffer, map.nr_entries) )
>>> + return -EFAULT;
>>> +
>>> + for ( i = 0; i < map.nr_entries; ++i )
>>> + {
>>> + if ( copy_to_guest_offset(buffer, i, rmrr_map + i, 1) )
>>> + return -EFAULT;
>>> + }
>>> +
>>> + if ( copy_to_guest(arg, &map, 1) )
>>> + return -EFAULT;
>>> +
>>> + return 0;
>>> + }
>>> +
>>> default:
>>> return subarch_memory_op(cmd, arg);
>>> }
>>> diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h
>>> index 2c57aa0..8481843 100644
>>> --- a/xen/include/public/memory.h
>>> +++ b/xen/include/public/memory.h
>>> @@ -523,7 +523,42 @@ DEFINE_XEN_GUEST_HANDLE(xen_mem_sharing_op_t);
>>>
>>> #endif /* defined(__XEN__) || defined(__XEN_TOOLS__) */
>>>
>>> -/* Next available subop number is 26 */
>>> +/*
>>> + * Some devices may reserve some range.
>>
>> "range" is not a useful unit of measure.
>
> So what about this?
>
> The regions of memory used for some devices should be reserved.
No - it is not a case of "should", it is a case of "must".
"For legacy reasons, some devices must be configured with special memory
regions to function correctly. The guest must avoid using any of these
regions."
>
>>
>>> + *
>>> + * Currently we just have RMRR
>>> + * - Reserved memory Region Reporting Structure,
>>> + * So returns the RMRR memory map as it was when the domain
>>> + * was started.
>>> + */
>>> +#define XENMEM_reserved_device_memory_map 26
>>> +struct xen_reserved_device_memory {
>>
>> xen_mem_ to match the prevailing style
>
> Okay.
>
>>
>>> + /* PFN of the current mapping of the page. */
>>> + xen_pfn_t pfn;
>>> + /* Number of the current mapping pages. */
>>> + xen_ulong_t count;
>>> +};
>>
>> This struct marks a range, but the fields don't make it clear. I would
>> suggest "start" and "nr_frames" as names.
>>
>
> I will prefer Jan's suggestion.
As do I,
~Andrew
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [v4][PATCH 2/9] xen:x86: define a new hypercall to get RMRR mappings
2014-08-25 12:07 ` Andrew Cooper
@ 2014-08-26 3:12 ` Chen, Tiejun
2014-08-26 9:25 ` Andrew Cooper
0 siblings, 1 reply; 30+ messages in thread
From: Chen, Tiejun @ 2014-08-26 3:12 UTC (permalink / raw)
To: Andrew Cooper, JBeulich, ian.jackson, stefano.stabellini,
ian.campbell, yang.z.zhang, kevin.tian
Cc: xen-devel
On 2014/8/25 20:07, Andrew Cooper wrote:
> On 25/08/14 12:21, Chen, Tiejun wrote:
>> On 2014/8/22 18:53, Andrew Cooper wrote:
>>> On 22/08/14 11:09, Tiejun Chen wrote:
>>>> We need this new hypercall to get RMRR mapping for VM.
>>>>
>>>> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
>>>> ---
>>>> xen/arch/x86/mm.c | 71
>>>> +++++++++++++++++++++++++++++++++++++++++++++
>>>> xen/include/public/memory.h | 37 ++++++++++++++++++++++-
>>>> 2 files changed, 107 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
>>>> index d23cb3f..e0d6650 100644
>>>> --- a/xen/arch/x86/mm.c
>>>> +++ b/xen/arch/x86/mm.c
>>>> @@ -123,6 +123,7 @@
>>>> #include <asm/setup.h>
>>>> #include <asm/fixmap.h>
>>>> #include <asm/pci.h>
>>>> +#include <asm/acpi.h>
>>>>
>>>> /* Mapping of the fixmap space needed early. */
>>>> l1_pgentry_t __attribute__ ((__section__ (".bss.page_aligned")))
>>>> @@ -4842,6 +4843,76 @@ long arch_memory_op(unsigned long cmd,
>>>> XEN_GUEST_HANDLE_PARAM(void) arg)
>>>> return rc;
>>>> }
>>>>
>>>> + case XENMEM_reserved_device_memory_map:
>>>> + {
>>>> + struct xen_reserved_device_memory_map map;
>>>> + XEN_GUEST_HANDLE(xen_reserved_device_memory_t) buffer;
>>>> + XEN_GUEST_HANDLE_PARAM(xen_reserved_device_memory_t)
>>>> buffer_param;
>>>> + unsigned int i = 0;
>>>> + static unsigned int nr_entries = 0;
>>>> + static struct xen_reserved_device_memory *rmrr_map;
>>>
>>> Absolutely not. This hypercall can easy be run concurrently.
>>>
>>>> + struct acpi_rmrr_unit *rmrr;
>>>> +
>>>> + if ( copy_from_guest(&map, arg, 1) )
>>>> + return -EFAULT;
>>>> +
>>>> + if ( !nr_entries )
>>>> + /* Currently we just need to cover RMRR. */
>>>> + list_for_each_entry( rmrr, &acpi_rmrr_units, list )
>>>> + nr_entries++;
>>>
>>> Maintain a global count as entries are added/removed from this list. It
>>> is a waste of time recounting this list for each hypercall.
>>
>> Are you saying push this 'nr_entries' as a global count somewhere? I
>> guess I can set this when we first construct acpi_rmrr_units in ACPI
>> stuff.
>
> Not named "nr_entries", but yes. It is constant after boot.
>
>>
>>>
>>>> +
>>>> + if ( !nr_entries )
>>>> + return -ENOENT;
>>>> + else
>>>> + {
>>>> + if ( rmrr_map == NULL )
>>>> + {
>>>> + rmrr_map = xmalloc_array(xen_reserved_device_memory_t,
>>>> + nr_entries);
>>>
>>> You can do all of this without any memory allocation.
>>
What is your way without any memory allocation? Do you mean I should
predefine a static array here? But how to predetermine the size?
Or you mean I should do something with one rmrr_map, like this,
struct xen_mem_reserved_device_memory rmrr_map;
list_for_each_entry( rmrr, &acpi_rmrr_units, list )
{
rmrr_map.start_pfn = ...;
rmrr_map.nr_pages = ...;
if ( copy_to_guest_offset(buffer, i, &rmrr_map, 1) ) return -EFAULT;
i++;
}
>> I will check this.
>
> It is easy...
>
>>
>>>
>>>> + if ( rmrr_map == NULL )
>>>> + {
>>>> + return -ENOMEM;
>>>> + }
>>>> +
>>>> + list_for_each_entry( rmrr, &acpi_rmrr_units, list )
>>>> + {
>>>> + rmrr_map[i].pfn = rmrr->base_address >>
>>>> PAGE_SHIFT;
>>>> + rmrr_map[i].count = PAGE_ALIGN(rmrr->end_address -
>>>> +
>>>> rmrr->base_address) /
>>>> + PAGE_SIZE;
>>>> + i++;
>
> In this loop, construct one on the stack and copy_to_guest, breaking if
> there is a fault or you exceed the guests array.
Its not possible to exceed the guests array since the caller always
check if it get such a error, -ENOBUFS, then if yes, it can reallocate
appropriate array.
Thanks
Tiejun
>
>>>> + }
>>>> + }
>>>> + }
>>>> +
>>>> + if ( map.nr_entries < nr_entries )
>>>> + {
>>>> + map.nr_entries = nr_entries;
>>>> + if ( copy_to_guest(arg, &map, 1) )
>>>> + return -EFAULT;
>>>> + return -ENOBUFS;
>>>> + }
>>>> +
>>>> + map.nr_entries = nr_entries;
>>>> + buffer_param = guest_handle_cast(map.buffer,
>>>> +
>>>> xen_reserved_device_memory_t);
>>>> + buffer = guest_handle_from_param(buffer_param,
>>>> +
>>>> xen_reserved_device_memory_t);
>>>> + if ( !guest_handle_okay(buffer, map.nr_entries) )
>>>> + return -EFAULT;
>>>> +
>>>> + for ( i = 0; i < map.nr_entries; ++i )
>>>> + {
>>>> + if ( copy_to_guest_offset(buffer, i, rmrr_map + i, 1) )
>>>> + return -EFAULT;
>>>> + }
>>>> +
>>>> + if ( copy_to_guest(arg, &map, 1) )
>>>> + return -EFAULT;
>>>> +
>>>> + return 0;
>>>> + }
>>>> +
>>>> default:
>>>> return subarch_memory_op(cmd, arg);
>>>> }
>>>> diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h
>>>> index 2c57aa0..8481843 100644
>>>> --- a/xen/include/public/memory.h
>>>> +++ b/xen/include/public/memory.h
>>>> @@ -523,7 +523,42 @@ DEFINE_XEN_GUEST_HANDLE(xen_mem_sharing_op_t);
>>>>
>>>> #endif /* defined(__XEN__) || defined(__XEN_TOOLS__) */
>>>>
>>>> -/* Next available subop number is 26 */
>>>> +/*
>>>> + * Some devices may reserve some range.
>>>
>>> "range" is not a useful unit of measure.
>>
>> So what about this?
>>
>> The regions of memory used for some devices should be reserved.
>
> No - it is not a case of "should", it is a case of "must".
>
> "For legacy reasons, some devices must be configured with special memory
> regions to function correctly. The guest must avoid using any of these
> regions."
>
>>
>>>
>>>> + *
>>>> + * Currently we just have RMRR
>>>> + * - Reserved memory Region Reporting Structure,
>>>> + * So returns the RMRR memory map as it was when the domain
>>>> + * was started.
>>>> + */
>>>> +#define XENMEM_reserved_device_memory_map 26
>>>> +struct xen_reserved_device_memory {
>>>
>>> xen_mem_ to match the prevailing style
>>
>> Okay.
>>
>>>
>>>> + /* PFN of the current mapping of the page. */
>>>> + xen_pfn_t pfn;
>>>> + /* Number of the current mapping pages. */
>>>> + xen_ulong_t count;
>>>> +};
>>>
>>> This struct marks a range, but the fields don't make it clear. I would
>>> suggest "start" and "nr_frames" as names.
>>>
>>
>> I will prefer Jan's suggestion.
>
> As do I,
>
> ~Andrew
>
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [v4][PATCH 2/9] xen:x86: define a new hypercall to get RMRR mappings
2014-08-26 3:12 ` Chen, Tiejun
@ 2014-08-26 9:25 ` Andrew Cooper
0 siblings, 0 replies; 30+ messages in thread
From: Andrew Cooper @ 2014-08-26 9:25 UTC (permalink / raw)
To: Chen, Tiejun, JBeulich, ian.jackson, stefano.stabellini,
ian.campbell, yang.z.zhang, kevin.tian
Cc: xen-devel
On 26/08/14 04:12, Chen, Tiejun wrote:
> On 2014/8/25 20:07, Andrew Cooper wrote:
>> On 25/08/14 12:21, Chen, Tiejun wrote:
>>> On 2014/8/22 18:53, Andrew Cooper wrote:
>>>> On 22/08/14 11:09, Tiejun Chen wrote:
>>>>> We need this new hypercall to get RMRR mapping for VM.
>>>>>
>>>>> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
>>>>> ---
>>>>> xen/arch/x86/mm.c | 71
>>>>> +++++++++++++++++++++++++++++++++++++++++++++
>>>>> xen/include/public/memory.h | 37 ++++++++++++++++++++++-
>>>>> 2 files changed, 107 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
>>>>> index d23cb3f..e0d6650 100644
>>>>> --- a/xen/arch/x86/mm.c
>>>>> +++ b/xen/arch/x86/mm.c
>>>>> @@ -123,6 +123,7 @@
>>>>> #include <asm/setup.h>
>>>>> #include <asm/fixmap.h>
>>>>> #include <asm/pci.h>
>>>>> +#include <asm/acpi.h>
>>>>>
>>>>> /* Mapping of the fixmap space needed early. */
>>>>> l1_pgentry_t __attribute__ ((__section__ (".bss.page_aligned")))
>>>>> @@ -4842,6 +4843,76 @@ long arch_memory_op(unsigned long cmd,
>>>>> XEN_GUEST_HANDLE_PARAM(void) arg)
>>>>> return rc;
>>>>> }
>>>>>
>>>>> + case XENMEM_reserved_device_memory_map:
>>>>> + {
>>>>> + struct xen_reserved_device_memory_map map;
>>>>> + XEN_GUEST_HANDLE(xen_reserved_device_memory_t) buffer;
>>>>> + XEN_GUEST_HANDLE_PARAM(xen_reserved_device_memory_t)
>>>>> buffer_param;
>>>>> + unsigned int i = 0;
>>>>> + static unsigned int nr_entries = 0;
>>>>> + static struct xen_reserved_device_memory *rmrr_map;
>>>>
>>>> Absolutely not. This hypercall can easy be run concurrently.
>>>>
>>>>> + struct acpi_rmrr_unit *rmrr;
>>>>> +
>>>>> + if ( copy_from_guest(&map, arg, 1) )
>>>>> + return -EFAULT;
>>>>> +
>>>>> + if ( !nr_entries )
>>>>> + /* Currently we just need to cover RMRR. */
>>>>> + list_for_each_entry( rmrr, &acpi_rmrr_units, list )
>>>>> + nr_entries++;
>>>>
>>>> Maintain a global count as entries are added/removed from this
>>>> list. It
>>>> is a waste of time recounting this list for each hypercall.
>>>
>>> Are you saying push this 'nr_entries' as a global count somewhere? I
>>> guess I can set this when we first construct acpi_rmrr_units in ACPI
>>> stuff.
>>
>> Not named "nr_entries", but yes. It is constant after boot.
>>
>>>
>>>>
>>>>> +
>>>>> + if ( !nr_entries )
>>>>> + return -ENOENT;
>>>>> + else
>>>>> + {
>>>>> + if ( rmrr_map == NULL )
>>>>> + {
>>>>> + rmrr_map =
>>>>> xmalloc_array(xen_reserved_device_memory_t,
>>>>> + nr_entries);
>>>>
>>>> You can do all of this without any memory allocation.
>>>
>
> What is your way without any memory allocation? Do you mean I should
> predefine a static array here? But how to predetermine the size?
>
> Or you mean I should do something with one rmrr_map, like this,
>
> struct xen_mem_reserved_device_memory rmrr_map;
>
> list_for_each_entry( rmrr, &acpi_rmrr_units, list )
> {
> rmrr_map.start_pfn = ...;
> rmrr_map.nr_pages = ...;
> if ( copy_to_guest_offset(buffer, i, &rmrr_map, 1)
> ) return -EFAULT;
> i++;
> }
Yes - that is along the right lines.
~Andrew
^ permalink raw reply [flat|nested] 30+ messages in thread
* [v4][PATCH 3/9] tools:libxc: introduce hypercall for xc_reserved_device_memory_map
2014-08-22 10:09 [v4][PATCH 0/9] xen: reserve RMRR to avoid conflicting MMIO/RAM Tiejun Chen
2014-08-22 10:09 ` [v4][PATCH 1/9] xen:vtd:rmrr: export acpi_rmrr_units Tiejun Chen
2014-08-22 10:09 ` [v4][PATCH 2/9] xen:x86: define a new hypercall to get RMRR mappings Tiejun Chen
@ 2014-08-22 10:09 ` Tiejun Chen
2014-08-22 10:55 ` Andrew Cooper
2014-08-22 10:09 ` [v4][PATCH 4/9] tools:libxc: check if mmio BAR is out of RMRR mappings Tiejun Chen
` (5 subsequent siblings)
8 siblings, 1 reply; 30+ messages in thread
From: Tiejun Chen @ 2014-08-22 10:09 UTC (permalink / raw)
To: JBeulich, ian.jackson, stefano.stabellini, ian.campbell,
yang.z.zhang, kevin.tian
Cc: xen-devel
We will introduce that hypercall xc_reserved_device_memory_map
to libxc.
Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
---
tools/libxc/xc_domain.c | 29 +++++++++++++++++++++++++++++
tools/libxc/xenctrl.h | 4 ++++
2 files changed, 33 insertions(+)
diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c
index c67ac9a..08dc16f 100644
--- a/tools/libxc/xc_domain.c
+++ b/tools/libxc/xc_domain.c
@@ -649,6 +649,35 @@ int xc_domain_set_memory_map(xc_interface *xch,
return rc;
}
+
+int xc_reserved_device_memory_map(xc_interface *xch,
+ struct xen_reserved_device_memory entries[],
+ uint32_t max_entries)
+{
+ int rc;
+ struct xen_reserved_device_memory_map memmap = {
+ .nr_entries = max_entries
+ };
+ DECLARE_HYPERCALL_BOUNCE(entries,
+ sizeof(struct xen_reserved_device_memory) *
+ max_entries, XC_HYPERCALL_BUFFER_BOUNCE_OUT);
+
+ if ( xc_hypercall_bounce_pre(xch, entries) )
+ return -1;
+
+ set_xen_guest_handle(memmap.buffer, entries);
+
+ rc = do_memory_op(xch, XENMEM_reserved_device_memory_map,
+ &memmap, sizeof(memmap));
+
+ xc_hypercall_bounce_post(xch, entries);
+
+ if ( errno == ENOBUFS )
+ return memmap.nr_entries;
+
+ return rc ? -errno : memmap.nr_entries;
+}
+
int xc_get_machine_memory_map(xc_interface *xch,
struct e820entry entries[],
uint32_t max_entries)
diff --git a/tools/libxc/xenctrl.h b/tools/libxc/xenctrl.h
index 1c5d0db..1cc7852 100644
--- a/tools/libxc/xenctrl.h
+++ b/tools/libxc/xenctrl.h
@@ -1270,6 +1270,10 @@ int xc_domain_set_memory_map(xc_interface *xch,
int xc_get_machine_memory_map(xc_interface *xch,
struct e820entry entries[],
uint32_t max_entries);
+
+int xc_reserved_device_memory_map(xc_interface *xch,
+ struct xen_reserved_device_memory entries[],
+ uint32_t max_entries);
#endif
int xc_domain_set_time_offset(xc_interface *xch,
uint32_t domid,
--
1.9.1
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [v4][PATCH 3/9] tools:libxc: introduce hypercall for xc_reserved_device_memory_map
2014-08-22 10:09 ` [v4][PATCH 3/9] tools:libxc: introduce hypercall for xc_reserved_device_memory_map Tiejun Chen
@ 2014-08-22 10:55 ` Andrew Cooper
2014-08-25 11:11 ` Chen, Tiejun
0 siblings, 1 reply; 30+ messages in thread
From: Andrew Cooper @ 2014-08-22 10:55 UTC (permalink / raw)
To: Tiejun Chen, JBeulich, ian.jackson, stefano.stabellini,
ian.campbell, yang.z.zhang, kevin.tian
Cc: xen-devel
On 22/08/14 11:09, Tiejun Chen wrote:
> We will introduce that hypercall xc_reserved_device_memory_map
> to libxc.
>
> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
> ---
> tools/libxc/xc_domain.c | 29 +++++++++++++++++++++++++++++
> tools/libxc/xenctrl.h | 4 ++++
> 2 files changed, 33 insertions(+)
>
> diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c
> index c67ac9a..08dc16f 100644
> --- a/tools/libxc/xc_domain.c
> +++ b/tools/libxc/xc_domain.c
> @@ -649,6 +649,35 @@ int xc_domain_set_memory_map(xc_interface *xch,
>
> return rc;
> }
> +
> +int xc_reserved_device_memory_map(xc_interface *xch,
> + struct xen_reserved_device_memory entries[],
> + uint32_t max_entries)
> +{
> + int rc;
> + struct xen_reserved_device_memory_map memmap = {
> + .nr_entries = max_entries
> + };
> + DECLARE_HYPERCALL_BOUNCE(entries,
> + sizeof(struct xen_reserved_device_memory) *
> + max_entries, XC_HYPERCALL_BUFFER_BOUNCE_OUT);
> +
> + if ( xc_hypercall_bounce_pre(xch, entries) )
> + return -1;
> +
> + set_xen_guest_handle(memmap.buffer, entries);
> +
> + rc = do_memory_op(xch, XENMEM_reserved_device_memory_map,
> + &memmap, sizeof(memmap));
> +
> + xc_hypercall_bounce_post(xch, entries);
> +
> + if ( errno == ENOBUFS )
> + return memmap.nr_entries;
> +
> + return rc ? -errno : memmap.nr_entries;
So how does the caller distinguish between "xen filled in N entries" and
"xen said you need N entries for all the information" ?
~Andrew
> +}
> +
> int xc_get_machine_memory_map(xc_interface *xch,
> struct e820entry entries[],
> uint32_t max_entries)
> diff --git a/tools/libxc/xenctrl.h b/tools/libxc/xenctrl.h
> index 1c5d0db..1cc7852 100644
> --- a/tools/libxc/xenctrl.h
> +++ b/tools/libxc/xenctrl.h
> @@ -1270,6 +1270,10 @@ int xc_domain_set_memory_map(xc_interface *xch,
> int xc_get_machine_memory_map(xc_interface *xch,
> struct e820entry entries[],
> uint32_t max_entries);
> +
> +int xc_reserved_device_memory_map(xc_interface *xch,
> + struct xen_reserved_device_memory entries[],
> + uint32_t max_entries);
> #endif
> int xc_domain_set_time_offset(xc_interface *xch,
> uint32_t domid,
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [v4][PATCH 3/9] tools:libxc: introduce hypercall for xc_reserved_device_memory_map
2014-08-22 10:55 ` Andrew Cooper
@ 2014-08-25 11:11 ` Chen, Tiejun
2014-08-25 11:58 ` Andrew Cooper
0 siblings, 1 reply; 30+ messages in thread
From: Chen, Tiejun @ 2014-08-25 11:11 UTC (permalink / raw)
To: Andrew Cooper, JBeulich, ian.jackson, stefano.stabellini,
ian.campbell, yang.z.zhang, kevin.tian
Cc: xen-devel
On 2014/8/22 18:55, Andrew Cooper wrote:
> On 22/08/14 11:09, Tiejun Chen wrote:
>> We will introduce that hypercall xc_reserved_device_memory_map
>> to libxc.
>>
>> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
>> ---
>> tools/libxc/xc_domain.c | 29 +++++++++++++++++++++++++++++
>> tools/libxc/xenctrl.h | 4 ++++
>> 2 files changed, 33 insertions(+)
>>
>> diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c
>> index c67ac9a..08dc16f 100644
>> --- a/tools/libxc/xc_domain.c
>> +++ b/tools/libxc/xc_domain.c
>> @@ -649,6 +649,35 @@ int xc_domain_set_memory_map(xc_interface *xch,
>>
>> return rc;
>> }
>> +
>> +int xc_reserved_device_memory_map(xc_interface *xch,
>> + struct xen_reserved_device_memory entries[],
>> + uint32_t max_entries)
>> +{
>> + int rc;
>> + struct xen_reserved_device_memory_map memmap = {
>> + .nr_entries = max_entries
>> + };
>> + DECLARE_HYPERCALL_BOUNCE(entries,
>> + sizeof(struct xen_reserved_device_memory) *
>> + max_entries, XC_HYPERCALL_BUFFER_BOUNCE_OUT);
>> +
>> + if ( xc_hypercall_bounce_pre(xch, entries) )
>> + return -1;
>> +
>> + set_xen_guest_handle(memmap.buffer, entries);
>> +
>> + rc = do_memory_op(xch, XENMEM_reserved_device_memory_map,
>> + &memmap, sizeof(memmap));
>> +
>> + xc_hypercall_bounce_post(xch, entries);
>> +
>> + if ( errno == ENOBUFS )
>> + return memmap.nr_entries;
>> +
>> + return rc ? -errno : memmap.nr_entries;
>
> So how does the caller distinguish between "xen filled in N entries" and
> "xen said you need N entries for all the information" ?
Thanks for your reminder, I will add something to check this point.
I think the caller can compare that number of entries as that input
parameter with the return value. If equal, this should be in case of
"xen filled in N entries". If not, this is in case of "xen said you need
N entries for all the information".
Thanks
Tiejun
>
> ~Andrew
>
>> +}
>> +
>> int xc_get_machine_memory_map(xc_interface *xch,
>> struct e820entry entries[],
>> uint32_t max_entries)
>> diff --git a/tools/libxc/xenctrl.h b/tools/libxc/xenctrl.h
>> index 1c5d0db..1cc7852 100644
>> --- a/tools/libxc/xenctrl.h
>> +++ b/tools/libxc/xenctrl.h
>> @@ -1270,6 +1270,10 @@ int xc_domain_set_memory_map(xc_interface *xch,
>> int xc_get_machine_memory_map(xc_interface *xch,
>> struct e820entry entries[],
>> uint32_t max_entries);
>> +
>> +int xc_reserved_device_memory_map(xc_interface *xch,
>> + struct xen_reserved_device_memory entries[],
>> + uint32_t max_entries);
>> #endif
>> int xc_domain_set_time_offset(xc_interface *xch,
>> uint32_t domid,
>
>
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [v4][PATCH 3/9] tools:libxc: introduce hypercall for xc_reserved_device_memory_map
2014-08-25 11:11 ` Chen, Tiejun
@ 2014-08-25 11:58 ` Andrew Cooper
0 siblings, 0 replies; 30+ messages in thread
From: Andrew Cooper @ 2014-08-25 11:58 UTC (permalink / raw)
To: Chen, Tiejun, JBeulich, ian.jackson, stefano.stabellini,
ian.campbell, yang.z.zhang, kevin.tian
Cc: xen-devel
On 25/08/14 12:11, Chen, Tiejun wrote:
>
>
> On 2014/8/22 18:55, Andrew Cooper wrote:
>> On 22/08/14 11:09, Tiejun Chen wrote:
>>> We will introduce that hypercall xc_reserved_device_memory_map
>>> to libxc.
>>>
>>> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
>>> ---
>>> tools/libxc/xc_domain.c | 29 +++++++++++++++++++++++++++++
>>> tools/libxc/xenctrl.h | 4 ++++
>>> 2 files changed, 33 insertions(+)
>>>
>>> diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c
>>> index c67ac9a..08dc16f 100644
>>> --- a/tools/libxc/xc_domain.c
>>> +++ b/tools/libxc/xc_domain.c
>>> @@ -649,6 +649,35 @@ int xc_domain_set_memory_map(xc_interface *xch,
>>>
>>> return rc;
>>> }
>>> +
>>> +int xc_reserved_device_memory_map(xc_interface *xch,
>>> + struct xen_reserved_device_memory
>>> entries[],
>>> + uint32_t max_entries)
>>> +{
>>> + int rc;
>>> + struct xen_reserved_device_memory_map memmap = {
>>> + .nr_entries = max_entries
>>> + };
>>> + DECLARE_HYPERCALL_BOUNCE(entries,
>>> + sizeof(struct
>>> xen_reserved_device_memory) *
>>> + max_entries,
>>> XC_HYPERCALL_BUFFER_BOUNCE_OUT);
>>> +
>>> + if ( xc_hypercall_bounce_pre(xch, entries) )
>>> + return -1;
>>> +
>>> + set_xen_guest_handle(memmap.buffer, entries);
>>> +
>>> + rc = do_memory_op(xch, XENMEM_reserved_device_memory_map,
>>> + &memmap, sizeof(memmap));
>>> +
>>> + xc_hypercall_bounce_post(xch, entries);
>>> +
>>> + if ( errno == ENOBUFS )
>>> + return memmap.nr_entries;
>>> +
>>> + return rc ? -errno : memmap.nr_entries;
>>
>> So how does the caller distinguish between "xen filled in N entries" and
>> "xen said you need N entries for all the information" ?
>
> Thanks for your reminder, I will add something to check this point.
>
> I think the caller can compare that number of entries as that input
> parameter with the return value. If equal, this should be in case of
> "xen filled in N entries". If not, this is in case of "xen said you
> need N entries for all the information".
>
> Thanks
> Tiejun
You have to pass max_entries by pointer, and leave the ENOBUFS handling
to the caller, as it is the only one capable of acting upon the information.
~Andrew
^ permalink raw reply [flat|nested] 30+ messages in thread
* [v4][PATCH 4/9] tools:libxc: check if mmio BAR is out of RMRR mappings
2014-08-22 10:09 [v4][PATCH 0/9] xen: reserve RMRR to avoid conflicting MMIO/RAM Tiejun Chen
` (2 preceding siblings ...)
2014-08-22 10:09 ` [v4][PATCH 3/9] tools:libxc: introduce hypercall for xc_reserved_device_memory_map Tiejun Chen
@ 2014-08-22 10:09 ` Tiejun Chen
2014-08-26 20:36 ` Ian Campbell
2014-08-22 10:09 ` [v4][PATCH 5/9] hvm_info_table: introduce nr_reserved_device_memory_map Tiejun Chen
` (4 subsequent siblings)
8 siblings, 1 reply; 30+ messages in thread
From: Tiejun Chen @ 2014-08-22 10:09 UTC (permalink / raw)
To: JBeulich, ian.jackson, stefano.stabellini, ian.campbell,
yang.z.zhang, kevin.tian
Cc: xen-devel
We need to avoid allocating MMIO BAR conflicting to RMRR range.
Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
---
tools/libxc/xc_hvm_build_x86.c | 69 ++++++++++++++++++++++++++++++++++++++++++
1 file changed, 69 insertions(+)
diff --git a/tools/libxc/xc_hvm_build_x86.c b/tools/libxc/xc_hvm_build_x86.c
index c81a25b..20e4e0c 100644
--- a/tools/libxc/xc_hvm_build_x86.c
+++ b/tools/libxc/xc_hvm_build_x86.c
@@ -239,6 +239,71 @@ static int check_mmio_hole(uint64_t start, uint64_t memsize,
return 1;
}
+/*
+ * Check whether there exists mmio overplap with the specified RMRR
+ * memory range.
+ */
+static int check_rmrr_overlap(xc_interface *xch, uint64_t mmio_start,
+ uint64_t mmio_size)
+{
+ struct xen_reserved_device_memory *map = NULL;
+ uint64_t rmrr_start = 0, rmrr_end = 0;
+ unsigned int i = 0;
+ int rc = 0;
+
+ /* We should check if mmio range is out of RMRR mapping.
+ *
+ * Assume we have one entry if not enough we'll expand.
+ */
+ if ( (map = malloc(1 * sizeof(xen_reserved_device_memory_t))) == NULL )
+ {
+ PERROR("Could not allocate memory for map.");
+ return -1;
+ }
+ rc = xc_reserved_device_memory_map(xch, map, 1);
+ if ( rc < 0 )
+ {
+ /* RMRR doesn't exist. */
+ if ( rc == -ENOENT )
+ rc = 0;
+ else
+ PERROR("Could not get RMRR info on domain");
+ } else if ( rc > 0 ) {
+ free(map);
+ /* Now we need more space to map all RMRR mappings.
+ */
+ if ( (map = malloc(rc * sizeof(xen_reserved_device_memory_t))) == NULL )
+ {
+ PERROR("Could not allocate memory for map.");
+ return -1;
+ }
+ rc = xc_reserved_device_memory_map(xch, map, rc);
+ if ( rc < 0 )
+ {
+ PERROR("Could not get RMRR info on domain");
+ free(map);
+ return rc;
+ }
+ }
+
+ for ( i = 0; i < rc; i++ )
+ {
+ rmrr_start = map[i].pfn << PAGE_SHIFT;
+ rmrr_end = rmrr_start + map[i].count * PAGE_SIZE;
+ if ( check_mmio_hole(rmrr_start, map[i].count * PAGE_SIZE,
+ mmio_start, mmio_size) )
+ {
+ PERROR("MMIO: [%lx]<->[%lx] overlap RMRR [%lx]<->[%lx]\n",
+ mmio_start, (mmio_start + mmio_size), rmrr_start, rmrr_end);
+ free(map);
+ return -1;
+ }
+ }
+
+ free(map);
+ return rc;
+}
+
static int setup_guest(xc_interface *xch,
uint32_t dom, struct xc_hvm_build_args *args,
char *image, unsigned long image_size)
@@ -300,6 +365,10 @@ static int setup_guest(xc_interface *xch,
goto error_out;
}
+ rc = check_rmrr_overlap(xch, mmio_start, mmio_start);
+ if ( rc < 0 )
+ goto error_out;
+
for ( i = 0; i < nr_pages; i++ )
page_array[i] = i;
for ( i = mmio_start >> PAGE_SHIFT; i < nr_pages; i++ )
--
1.9.1
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [v4][PATCH 4/9] tools:libxc: check if mmio BAR is out of RMRR mappings
2014-08-22 10:09 ` [v4][PATCH 4/9] tools:libxc: check if mmio BAR is out of RMRR mappings Tiejun Chen
@ 2014-08-26 20:36 ` Ian Campbell
2014-08-27 1:46 ` Chen, Tiejun
0 siblings, 1 reply; 30+ messages in thread
From: Ian Campbell @ 2014-08-26 20:36 UTC (permalink / raw)
To: Tiejun Chen
Cc: kevin.tian, stefano.stabellini, ian.jackson, xen-devel, JBeulich,
yang.z.zhang
On Fri, 2014-08-22 at 18:09 +0800, Tiejun Chen wrote:
> + /* We should check if mmio range is out of RMRR mapping.
> + *
> + * Assume we have one entry if not enough we'll expand.
> + */
The usual approach with such hypervisor interfaces (which I suppose
xc_reserved_device_memory_map turns into) is to first call it with NULL
to get the required size and then allocate a suitable buffer and call a
second time.
> + for ( i = 0; i < rc; i++ )
> + {
> + rmrr_start = map[i].pfn << PAGE_SHIFT;
> + rmrr_end = rmrr_start + map[i].count * PAGE_SIZE;
> + if ( check_mmio_hole(rmrr_start, map[i].count * PAGE_SIZE,
Adding rmrr_size = map... & PAGE_SIZE could be used twice here.
Ian.
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [v4][PATCH 4/9] tools:libxc: check if mmio BAR is out of RMRR mappings
2014-08-26 20:36 ` Ian Campbell
@ 2014-08-27 1:46 ` Chen, Tiejun
2014-08-27 2:20 ` Ian Campbell
0 siblings, 1 reply; 30+ messages in thread
From: Chen, Tiejun @ 2014-08-27 1:46 UTC (permalink / raw)
To: Ian Campbell
Cc: kevin.tian, stefano.stabellini, ian.jackson, xen-devel, JBeulich,
yang.z.zhang
On 2014/8/27 4:36, Ian Campbell wrote:
> On Fri, 2014-08-22 at 18:09 +0800, Tiejun Chen wrote:
>
>> + /* We should check if mmio range is out of RMRR mapping.
>> + *
>> + * Assume we have one entry if not enough we'll expand.
>> + */
>
> The usual approach with such hypervisor interfaces (which I suppose
> xc_reserved_device_memory_map turns into) is to first call it with NULL
> to get the required size and then allocate a suitable buffer and call a
> second time.
Ofentimes, RMRR should be rare with one or two entries, even zero. So I
think its reasonable to start posting one entry since this can cover
such a scenario the platform really owns one entry.
>
>
>> + for ( i = 0; i < rc; i++ )
>> + {
>> + rmrr_start = map[i].pfn << PAGE_SHIFT;
>> + rmrr_end = rmrr_start + map[i].count * PAGE_SIZE;
>> + if ( check_mmio_hole(rmrr_start, map[i].count * PAGE_SIZE,
>
> Adding rmrr_size = map... & PAGE_SIZE could be used twice here.
>
Yes, so I think the follows should be expected:
--- a/tools/libxc/xc_hvm_build_x86.c
+++ b/tools/libxc/xc_hvm_build_x86.c
@@ -251,7 +251,7 @@ static int check_rmrr_overlap(xc_interface *xch,
uint64_t mmio_start,
uint64_t mmio_size)
{
struct xen_mem_reserved_device_memory *map = NULL;
- uint64_t rmrr_start = 0, rmrr_end = 0;
+ uint64_t rmrr_start = 0, rmrr_end = 0, rmrr_size = 0;
unsigned int i = 0;
int rc = 0;
/* Assume we have one entry if not enough we'll expand.*/
@@ -295,9 +295,9 @@ static int check_rmrr_overlap(xc_interface *xch,
uint64_t mmio_start,
for ( i = 0; i < rc; i++ )
{
rmrr_start = map[i].start_pfn << PAGE_SHIFT;
- rmrr_end = rmrr_start + map[i].nr_pages * PAGE_SIZE;
- if ( check_mmio_hole(rmrr_start, map[i].nr_pages * PAGE_SIZE,
- mmio_start, mmio_size) )
+ rmrr_size = map[i].nr_pages * PAGE_SIZE;
+ rmrr_end = rmrr_start + rmrr_size;
+ if ( check_mmio_hole(rmrr_start, rmrr_size, mmio_start,
mmio_size) )
{
PERROR("MMIO: [%lx]<->[%lx] overlap RMRR [%lx]<->[%lx]\n",
mmio_start, (mmio_start + mmio_size), rmrr_start,
rmrr_end);
Thanks
Tiejun
> Ian.
>
>
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [v4][PATCH 4/9] tools:libxc: check if mmio BAR is out of RMRR mappings
2014-08-27 1:46 ` Chen, Tiejun
@ 2014-08-27 2:20 ` Ian Campbell
2014-08-27 2:40 ` Chen, Tiejun
0 siblings, 1 reply; 30+ messages in thread
From: Ian Campbell @ 2014-08-27 2:20 UTC (permalink / raw)
To: Chen, Tiejun
Cc: kevin.tian, stefano.stabellini, ian.jackson, xen-devel, JBeulich,
yang.z.zhang
On Wed, 2014-08-27 at 09:46 +0800, Chen, Tiejun wrote:
> On 2014/8/27 4:36, Ian Campbell wrote:
> > On Fri, 2014-08-22 at 18:09 +0800, Tiejun Chen wrote:
> >
> >> + /* We should check if mmio range is out of RMRR mapping.
> >> + *
> >> + * Assume we have one entry if not enough we'll expand.
> >> + */
> >
> > The usual approach with such hypervisor interfaces (which I suppose
> > xc_reserved_device_memory_map turns into) is to first call it with NULL
> > to get the required size and then allocate a suitable buffer and call a
> > second time.
>
> Ofentimes, RMRR should be rare with one or two entries, even zero.
It's not clear to me what number you are saying is the norm here.
Even if some N is common today what guarantee is there that N won't grow
or shrink with the next generation of systems?
> So I
> think its reasonable to start posting one entry since this can cover
> such a scenario the platform really owns one entry.
Making the call twice is not terribly expensive (nor is this a hot path)
and it allows you to avoid the reallocation and recall and the twisty
error handling structure which that implies.
Ian.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [v4][PATCH 4/9] tools:libxc: check if mmio BAR is out of RMRR mappings
2014-08-27 2:20 ` Ian Campbell
@ 2014-08-27 2:40 ` Chen, Tiejun
2014-08-27 2:47 ` Chen, Tiejun
0 siblings, 1 reply; 30+ messages in thread
From: Chen, Tiejun @ 2014-08-27 2:40 UTC (permalink / raw)
To: Ian Campbell
Cc: kevin.tian, stefano.stabellini, ian.jackson, xen-devel, JBeulich,
yang.z.zhang
On 2014/8/27 10:20, Ian Campbell wrote:
> On Wed, 2014-08-27 at 09:46 +0800, Chen, Tiejun wrote:
>> On 2014/8/27 4:36, Ian Campbell wrote:
>>> On Fri, 2014-08-22 at 18:09 +0800, Tiejun Chen wrote:
>>>
>>>> + /* We should check if mmio range is out of RMRR mapping.
>>>> + *
>>>> + * Assume we have one entry if not enough we'll expand.
>>>> + */
>>>
>>> The usual approach with such hypervisor interfaces (which I suppose
>>> xc_reserved_device_memory_map turns into) is to first call it with NULL
>>> to get the required size and then allocate a suitable buffer and call a
>>> second time.
>>
>> Ofentimes, RMRR should be rare with one or two entries, even zero.
>
> It's not clear to me what number you are saying is the norm here.
In my broadwell platform we just have two entries.
>
> Even if some N is common today what guarantee is there that N won't grow
> or shrink with the next generation of systems?
As I understand RMRR may be legacy, and this should go away.
What is RMRR?
-------------
There are some devices the BIOS controls, for e.g USB devices to perform
PS2 emulation. The regions of memory used for these devices are marked
reserved in the e820 map. When we turn on DMA translation, DMA to those
regions will fail. Hence BIOS uses RMRR to specify these regions along
with devices that need to access these regions. OS is expected to setup
unity mappings for these regions for these devices to access these regions.
>
>> So I
>> think its reasonable to start posting one entry since this can cover
>> such a scenario the platform really owns one entry.
>
> Making the call twice is not terribly expensive (nor is this a hot path)
> and it allows you to avoid the reallocation and recall and the twisty
> error handling structure which that implies.
>
As I understand when we call one given hypercall, we may know that
possible numbers to issue that. Then we can get the appropriate number
via the returned value if that is not enough. I think its better than we
always issue twice hypercall unconditionally :)
But if you persist in this fixed twice-call mechanism, I can try to
rework out this implementation :)
Thanks
Tiejun
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [v4][PATCH 4/9] tools:libxc: check if mmio BAR is out of RMRR mappings
2014-08-27 2:40 ` Chen, Tiejun
@ 2014-08-27 2:47 ` Chen, Tiejun
0 siblings, 0 replies; 30+ messages in thread
From: Chen, Tiejun @ 2014-08-27 2:47 UTC (permalink / raw)
To: Ian Campbell
Cc: kevin.tian, stefano.stabellini, ian.jackson, xen-devel, JBeulich,
yang.z.zhang
On 2014/8/27 10:40, Chen, Tiejun wrote:
> On 2014/8/27 10:20, Ian Campbell wrote:
>> On Wed, 2014-08-27 at 09:46 +0800, Chen, Tiejun wrote:
>>> On 2014/8/27 4:36, Ian Campbell wrote:
>>>> On Fri, 2014-08-22 at 18:09 +0800, Tiejun Chen wrote:
>>>>
>>>>> + /* We should check if mmio range is out of RMRR mapping.
>>>>> + *
>>>>> + * Assume we have one entry if not enough we'll expand.
>>>>> + */
>>>>
>>>> The usual approach with such hypervisor interfaces (which I suppose
>>>> xc_reserved_device_memory_map turns into) is to first call it with NULL
>>>> to get the required size and then allocate a suitable buffer and call a
>>>> second time.
>>>
>>> Ofentimes, RMRR should be rare with one or two entries, even zero.
>>
>> It's not clear to me what number you are saying is the norm here.
>
> In my broadwell platform we just have two entries.
>
>>
>> Even if some N is common today what guarantee is there that N won't grow
>> or shrink with the next generation of systems?
>
> As I understand RMRR may be legacy, and this should go away.
>
> What is RMRR?
> -------------
>
> There are some devices the BIOS controls, for e.g USB devices to perform
> PS2 emulation. The regions of memory used for these devices are marked
> reserved in the e820 map. When we turn on DMA translation, DMA to those
> regions will fail. Hence BIOS uses RMRR to specify these regions along
> with devices that need to access these regions. OS is expected to setup
> unity mappings for these regions for these devices to access these regions.
>
>>
>>> So I
>>> think its reasonable to start posting one entry since this can cover
>>> such a scenario the platform really owns one entry.
>>
>> Making the call twice is not terribly expensive (nor is this a hot path)
>> and it allows you to avoid the reallocation and recall and the twisty
>> error handling structure which that implies.
>>
>
> As I understand when we call one given hypercall, we may know that
> possible numbers to issue that. Then we can get the appropriate number
> via the returned value if that is not enough. I think its better than we
> always issue twice hypercall unconditionally :)
>
> But if you persist in this fixed twice-call mechanism, I can try to
> rework out this implementation :)
>
And actually, I think current implementation is already as you expect.
Please check patch #3,
@@ -4842,6 +4843,55 @@ long arch_memory_op(unsigned long cmd,
XEN_GUEST_HANDLE_PARAM(void) arg)
return rc;
}
+ case XENMEM_reserved_device_memory_map:
+ {
+ struct xen_mem_reserved_device_memory_map map;
+ XEN_GUEST_HANDLE(xen_mem_reserved_device_memory_t) buffer;
+ XEN_GUEST_HANDLE_PARAM(xen_mem_reserved_device_memory_t)
buffer_param;
+ unsigned int i = 0;
+ struct xen_mem_reserved_device_memory rmrr_map;
+ struct acpi_rmrr_unit *rmrr;
+
+ if ( !acpi_rmrr_unit_entries )
+ return -ENOENT;
+
+ if ( copy_from_guest(&map, arg, 1) )
+ return -EFAULT;
+
+ if ( map.nr_entries < acpi_rmrr_unit_entries )
+ {
+ map.nr_entries = acpi_rmrr_unit_entries;
+ if ( copy_to_guest(arg, &map, 1) )
+ return -EFAULT;
+ return -ENOBUFS;
+ }
And turn back, here I just set map.nr_entries = 1, not '0' as you image.
+ /* Assume we have one entry if not enough we'll expand.*/
+ uint32_t nr_entries = 1;
+
+ /* We should check if mmio range is out of RMRR mapping. */
+ if ( (map = malloc(nr_entries *
+ sizeof(xen_mem_reserved_device_memory_t))) == NULL )
So you really hope I should set map.nr_entries = 0 firstly?
Thanks
Tiejun
^ permalink raw reply [flat|nested] 30+ messages in thread
* [v4][PATCH 5/9] hvm_info_table: introduce nr_reserved_device_memory_map
2014-08-22 10:09 [v4][PATCH 0/9] xen: reserve RMRR to avoid conflicting MMIO/RAM Tiejun Chen
` (3 preceding siblings ...)
2014-08-22 10:09 ` [v4][PATCH 4/9] tools:libxc: check if mmio BAR is out of RMRR mappings Tiejun Chen
@ 2014-08-22 10:09 ` Tiejun Chen
2014-08-26 20:38 ` Ian Campbell
2014-08-22 10:09 ` [v4][PATCH 6/9] xen:x86:: support xc_reserved_device_memory_map in compat case Tiejun Chen
` (3 subsequent siblings)
8 siblings, 1 reply; 30+ messages in thread
From: Tiejun Chen @ 2014-08-22 10:09 UTC (permalink / raw)
To: JBeulich, ian.jackson, stefano.stabellini, ian.campbell,
yang.z.zhang, kevin.tian
Cc: xen-devel
libxc can expose how many reserved device memory entries
hvmloader should get. And '0' means that doesn't exist so
we can skip this check.
Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
---
tools/libxc/xc_hvm_build_x86.c | 12 ++++++++++--
xen/include/public/hvm/hvm_info_table.h | 3 +++
2 files changed, 13 insertions(+), 2 deletions(-)
diff --git a/tools/libxc/xc_hvm_build_x86.c b/tools/libxc/xc_hvm_build_x86.c
index 20e4e0c..093b089 100644
--- a/tools/libxc/xc_hvm_build_x86.c
+++ b/tools/libxc/xc_hvm_build_x86.c
@@ -89,7 +89,8 @@ static int modules_init(struct xc_hvm_build_args *args,
}
static void build_hvm_info(void *hvm_info_page, uint64_t mem_size,
- uint64_t mmio_start, uint64_t mmio_size)
+ uint64_t mmio_start, uint64_t mmio_size,
+ unsigned int num)
{
struct hvm_info_table *hvm_info = (struct hvm_info_table *)
(((unsigned char *)hvm_info_page) + HVM_INFO_OFFSET);
@@ -119,6 +120,9 @@ static void build_hvm_info(void *hvm_info_page, uint64_t mem_size,
hvm_info->high_mem_pgend = highmem_end >> PAGE_SHIFT;
hvm_info->reserved_mem_pgstart = ioreq_server_pfn(0);
+ /* Reserved device memory number. */
+ hvm_info->nr_reserved_device_memory_map = num;
+
/* Finish with the checksum. */
for ( i = 0, sum = 0; i < hvm_info->length; i++ )
sum += ((uint8_t *)hvm_info)[i];
@@ -327,6 +331,7 @@ static int setup_guest(xc_interface *xch,
int claim_enabled = args->claim_enabled;
xen_pfn_t special_array[NR_SPECIAL_PAGES];
xen_pfn_t ioreq_server_array[NR_IOREQ_SERVER_PAGES];
+ unsigned int num_reserved = 0;
if ( nr_pages > target_pages )
pod_mode = XENMEMF_populate_on_demand;
@@ -368,6 +373,9 @@ static int setup_guest(xc_interface *xch,
rc = check_rmrr_overlap(xch, mmio_start, mmio_start);
if ( rc < 0 )
goto error_out;
+ /* Always return entries number. */
+ else
+ num_reserved = rc;
for ( i = 0; i < nr_pages; i++ )
page_array[i] = i;
@@ -538,7 +546,7 @@ static int setup_guest(xc_interface *xch,
xch, dom, PAGE_SIZE, PROT_READ | PROT_WRITE,
HVM_INFO_PFN)) == NULL )
goto error_out;
- build_hvm_info(hvm_info_page, v_end, mmio_start, mmio_size);
+ build_hvm_info(hvm_info_page, v_end, mmio_start, mmio_size, num_reserved);
munmap(hvm_info_page, PAGE_SIZE);
/* Allocate and clear special pages. */
diff --git a/xen/include/public/hvm/hvm_info_table.h b/xen/include/public/hvm/hvm_info_table.h
index 36085fa..52c2b80 100644
--- a/xen/include/public/hvm/hvm_info_table.h
+++ b/xen/include/public/hvm/hvm_info_table.h
@@ -67,6 +67,9 @@ struct hvm_info_table {
/* Bitmap of which CPUs are online at boot time. */
uint8_t vcpu_online[(HVM_MAX_VCPUS + 7)/8];
+
+ /* How many reserved device memory does this domain have? */
+ uint32_t nr_reserved_device_memory_map;
};
#endif /* __XEN_PUBLIC_HVM_HVM_INFO_TABLE_H__ */
--
1.9.1
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [v4][PATCH 5/9] hvm_info_table: introduce nr_reserved_device_memory_map
2014-08-22 10:09 ` [v4][PATCH 5/9] hvm_info_table: introduce nr_reserved_device_memory_map Tiejun Chen
@ 2014-08-26 20:38 ` Ian Campbell
2014-08-27 1:54 ` Chen, Tiejun
0 siblings, 1 reply; 30+ messages in thread
From: Ian Campbell @ 2014-08-26 20:38 UTC (permalink / raw)
To: Tiejun Chen
Cc: kevin.tian, stefano.stabellini, ian.jackson, xen-devel, JBeulich,
yang.z.zhang
On Fri, 2014-08-22 at 18:09 +0800, Tiejun Chen wrote:
> libxc can expose how many reserved device memory entries
> hvmloader should get.
"get" in what sense?
> And '0' means that doesn't exist so
> we can skip this check.
Which check?
The code is trivial enough that I guess it is correct, and I don't
expect a full explanation here (since I assume that comes in some future
patch) but an accurate/meaningful quick indication would be useful here.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [v4][PATCH 5/9] hvm_info_table: introduce nr_reserved_device_memory_map
2014-08-26 20:38 ` Ian Campbell
@ 2014-08-27 1:54 ` Chen, Tiejun
2014-08-27 1:57 ` Chen, Tiejun
2014-08-27 2:21 ` Ian Campbell
0 siblings, 2 replies; 30+ messages in thread
From: Chen, Tiejun @ 2014-08-27 1:54 UTC (permalink / raw)
To: Ian Campbell
Cc: kevin.tian, stefano.stabellini, ian.jackson, xen-devel, JBeulich,
yang.z.zhang
On 2014/8/27 4:38, Ian Campbell wrote:
> On Fri, 2014-08-22 at 18:09 +0800, Tiejun Chen wrote:
>> libxc can expose how many reserved device memory entries
>> hvmloader should get.
>
> "get" in what sense?
>
>> And '0' means that doesn't exist so
>> we can skip this check.
>
> Which check?
>
> The code is trivial enough that I guess it is correct, and I don't
> expect a full explanation here (since I assume that comes in some future
> patch) but an accurate/meaningful quick indication would be useful here.
>
Okay, I think I should describe this case explicitly as follows:
hvm_info_table: introduce nr_reserved_device_memory_map
libxc can expose how many reserved device memory entries to
notify hvmloader. Then hvmloader would check if those reserved
memory overlap current memory range in e820.
Note if nr_reserved_device_memory_map is '0', this means we have
any reserved device memory so we can skip that check.
Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
Thanks
Tiejun
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [v4][PATCH 5/9] hvm_info_table: introduce nr_reserved_device_memory_map
2014-08-27 1:54 ` Chen, Tiejun
@ 2014-08-27 1:57 ` Chen, Tiejun
2014-08-27 2:21 ` Ian Campbell
1 sibling, 0 replies; 30+ messages in thread
From: Chen, Tiejun @ 2014-08-27 1:57 UTC (permalink / raw)
To: Ian Campbell
Cc: kevin.tian, stefano.stabellini, ian.jackson, xen-devel, JBeulich,
yang.z.zhang
On 2014/8/27 9:54, Chen, Tiejun wrote:
> On 2014/8/27 4:38, Ian Campbell wrote:
>> On Fri, 2014-08-22 at 18:09 +0800, Tiejun Chen wrote:
>>> libxc can expose how many reserved device memory entries
>>> hvmloader should get.
>>
>> "get" in what sense?
>>
>>> And '0' means that doesn't exist so
>>> we can skip this check.
>>
>> Which check?
>>
>> The code is trivial enough that I guess it is correct, and I don't
>> expect a full explanation here (since I assume that comes in some future
>> patch) but an accurate/meaningful quick indication would be useful here.
>>
>
> Okay, I think I should describe this case explicitly as follows:
>
> hvm_info_table: introduce nr_reserved_device_memory_map
>
> libxc can expose how many reserved device memory entries to
> notify hvmloader. Then hvmloader would check if those reserved
> memory overlap current memory range in e820.
>
> Note if nr_reserved_device_memory_map is '0', this means we have
> any reserved device memory so we can skip that check.
s/any/no any
Thanks
Tiejun
>
> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
>
> Thanks
> Tiejun
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [v4][PATCH 5/9] hvm_info_table: introduce nr_reserved_device_memory_map
2014-08-27 1:54 ` Chen, Tiejun
2014-08-27 1:57 ` Chen, Tiejun
@ 2014-08-27 2:21 ` Ian Campbell
2014-08-27 2:28 ` Chen, Tiejun
1 sibling, 1 reply; 30+ messages in thread
From: Ian Campbell @ 2014-08-27 2:21 UTC (permalink / raw)
To: Chen, Tiejun
Cc: kevin.tian, stefano.stabellini, ian.jackson, xen-devel, JBeulich,
yang.z.zhang
On Wed, 2014-08-27 at 09:54 +0800, Chen, Tiejun wrote:
> On 2014/8/27 4:38, Ian Campbell wrote:
> > On Fri, 2014-08-22 at 18:09 +0800, Tiejun Chen wrote:
> >> libxc can expose how many reserved device memory entries
> >> hvmloader should get.
> >
> > "get" in what sense?
> >
> >> And '0' means that doesn't exist so
> >> we can skip this check.
> >
> > Which check?
> >
> > The code is trivial enough that I guess it is correct, and I don't
> > expect a full explanation here (since I assume that comes in some future
> > patch) but an accurate/meaningful quick indication would be useful here.
> >
>
> Okay, I think I should describe this case explicitly as follows:
>
> hvm_info_table: introduce nr_reserved_device_memory_map
>
> libxc can expose how many reserved device memory entries to
> notify hvmloader.
"libxc exposes to hvmloader the number of reserved device memory entries
which are present" is what I think you are trying to say.
> Then hvmloader would check if those reserved
s/would/will/
> memory overlap current memory range in e820.
>
> Note if nr_reserved_device_memory_map is '0', this means we have
> any reserved device memory so we can skip that check.
s/any/no/ ?
Ian.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [v4][PATCH 5/9] hvm_info_table: introduce nr_reserved_device_memory_map
2014-08-27 2:21 ` Ian Campbell
@ 2014-08-27 2:28 ` Chen, Tiejun
0 siblings, 0 replies; 30+ messages in thread
From: Chen, Tiejun @ 2014-08-27 2:28 UTC (permalink / raw)
To: Ian Campbell
Cc: kevin.tian, stefano.stabellini, ian.jackson, xen-devel, JBeulich,
yang.z.zhang
On 2014/8/27 10:21, Ian Campbell wrote:
> On Wed, 2014-08-27 at 09:54 +0800, Chen, Tiejun wrote:
>> On 2014/8/27 4:38, Ian Campbell wrote:
>>> On Fri, 2014-08-22 at 18:09 +0800, Tiejun Chen wrote:
>>>> libxc can expose how many reserved device memory entries
>>>> hvmloader should get.
>>>
>>> "get" in what sense?
>>>
>>>> And '0' means that doesn't exist so
>>>> we can skip this check.
>>>
>>> Which check?
>>>
>>> The code is trivial enough that I guess it is correct, and I don't
>>> expect a full explanation here (since I assume that comes in some future
>>> patch) but an accurate/meaningful quick indication would be useful here.
>>>
>>
>> Okay, I think I should describe this case explicitly as follows:
>>
>> hvm_info_table: introduce nr_reserved_device_memory_map
>>
>> libxc can expose how many reserved device memory entries to
>> notify hvmloader.
>
> "libxc exposes to hvmloader the number of reserved device memory entries
> which are present" is what I think you are trying to say.
>
>> Then hvmloader would check if those reserved
>
> s/would/will/
>
>> memory overlap current memory range in e820.
>>
>> Note if nr_reserved_device_memory_map is '0', this means we have
>> any reserved device memory so we can skip that check.
>
> s/any/no/ ?
>
Thanks for your revision kindly, so now,
hvm_info_table: introduce nr_reserved_device_memory_map
libxc exposes to hvmloader how many reserved device memory entries
which are present. Then hvmloader will check if those reserved
memory overlap current memory range in e820.
Note if nr_reserved_device_memory_map is '0', this means we have
no reserved device memory so we can skip that check.
Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
Thanks
Tiejun
^ permalink raw reply [flat|nested] 30+ messages in thread
* [v4][PATCH 6/9] xen:x86:: support xc_reserved_device_memory_map in compat case
2014-08-22 10:09 [v4][PATCH 0/9] xen: reserve RMRR to avoid conflicting MMIO/RAM Tiejun Chen
` (4 preceding siblings ...)
2014-08-22 10:09 ` [v4][PATCH 5/9] hvm_info_table: introduce nr_reserved_device_memory_map Tiejun Chen
@ 2014-08-22 10:09 ` Tiejun Chen
2014-08-22 10:09 ` [v4][PATCH 7/9] tools:firmware:hvmloader: introduce hypercall for xc_reserved_device_memory_map Tiejun Chen
` (2 subsequent siblings)
8 siblings, 0 replies; 30+ messages in thread
From: Tiejun Chen @ 2014-08-22 10:09 UTC (permalink / raw)
To: JBeulich, ian.jackson, stefano.stabellini, ian.campbell,
yang.z.zhang, kevin.tian
Cc: xen-devel
We also need to make sure xc_reserved_device_memory_map can
work in compat case.
Mostly refer to that complete xc_reserved_device_memory_map
hypercall definition, just work with struct
compat_reserved_device_memory_map and struct
compat_reserved_device_memory.
Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
---
xen/arch/x86/x86_64/compat/mm.c | 62 +++++++++++++++++++++++++++++++++++++++++
1 file changed, 62 insertions(+)
diff --git a/xen/arch/x86/x86_64/compat/mm.c b/xen/arch/x86/x86_64/compat/mm.c
index 69c6195..887a778 100644
--- a/xen/arch/x86/x86_64/compat/mm.c
+++ b/xen/arch/x86/x86_64/compat/mm.c
@@ -215,6 +215,68 @@ int compat_arch_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
break;
}
+ case XENMEM_reserved_device_memory_map:
+ {
+ struct compat_reserved_device_memory_map map;
+ unsigned int i = 0;
+ static unsigned int nr_entries = 0;
+ static struct compat_reserved_device_memory *rmrr_map;
+ struct acpi_rmrr_unit *rmrr;
+
+ if ( copy_from_guest(&map, arg, 1) )
+ return -EFAULT;
+
+ if ( !nr_entries )
+ /* Currently we just need to cover RMRR. */
+ list_for_each_entry( rmrr, &acpi_rmrr_units, list )
+ nr_entries++;
+
+ if ( !nr_entries )
+ return -ENOENT;
+ else
+ {
+ if ( rmrr_map == NULL )
+ {
+ rmrr_map = xmalloc_array(compat_reserved_device_memory_t,
+ nr_entries);
+ if ( rmrr_map == NULL )
+ {
+ return -ENOMEM;
+ }
+
+ list_for_each_entry( rmrr, &acpi_rmrr_units, list )
+ {
+ rmrr_map[i].pfn = rmrr->base_address >> PAGE_SHIFT;
+ rmrr_map[i].count = PAGE_ALIGN(rmrr->end_address -
+ rmrr->base_address) /
+ PAGE_SIZE;
+ i++;
+ }
+ }
+ }
+
+ if ( map.nr_entries < nr_entries )
+ {
+ map.nr_entries = nr_entries;
+ if ( copy_to_guest(arg, &map, 1) )
+ return -EFAULT;
+ return -ENOBUFS;
+ }
+
+ map.nr_entries = nr_entries;
+
+ for ( i = 0; i < map.nr_entries; ++i )
+ {
+ if ( copy_to_compat_offset(map.buffer, i, rmrr_map + i, 1) )
+ return -EFAULT;
+ }
+
+ if ( copy_to_guest(arg, &map, 1) )
+ return -EFAULT;
+
+ break;
+ }
+
default:
rc = -ENOSYS;
break;
--
1.9.1
^ permalink raw reply related [flat|nested] 30+ messages in thread* [v4][PATCH 7/9] tools:firmware:hvmloader: introduce hypercall for xc_reserved_device_memory_map
2014-08-22 10:09 [v4][PATCH 0/9] xen: reserve RMRR to avoid conflicting MMIO/RAM Tiejun Chen
` (5 preceding siblings ...)
2014-08-22 10:09 ` [v4][PATCH 6/9] xen:x86:: support xc_reserved_device_memory_map in compat case Tiejun Chen
@ 2014-08-22 10:09 ` Tiejun Chen
2014-08-22 10:09 ` [v4][PATCH 8/9] tools:firmware:hvmloader: check to reserve RMRR mappings in e820 Tiejun Chen
2014-08-22 10:09 ` [v4][PATCH 9/9] xen:vtd: make USB RMRR mapping safe Tiejun Chen
8 siblings, 0 replies; 30+ messages in thread
From: Tiejun Chen @ 2014-08-22 10:09 UTC (permalink / raw)
To: JBeulich, ian.jackson, stefano.stabellini, ian.campbell,
yang.z.zhang, kevin.tian
Cc: xen-devel
We will introduce that hypercall xc_reserved_device_memory_map
to hvmloader.
Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
---
tools/firmware/hvmloader/util.c | 22 ++++++++++++++++++++++
tools/firmware/hvmloader/util.h | 11 +++++++++++
2 files changed, 33 insertions(+)
diff --git a/tools/firmware/hvmloader/util.c b/tools/firmware/hvmloader/util.c
index 80d822f..5d656f1 100644
--- a/tools/firmware/hvmloader/util.c
+++ b/tools/firmware/hvmloader/util.c
@@ -828,6 +828,28 @@ int hpet_exists(unsigned long hpet_base)
return ((hpet_id >> 16) == 0x8086);
}
+int get_reserved_device_memory_map(struct reserved_device_memory *entries,
+ uint32_t max_entries)
+{
+ static int map_done = 0;
+ struct xen_reserved_device_memory_map memmap = {
+ .nr_entries = max_entries
+ };
+
+ if ( map_done )
+ return 0;
+
+ set_xen_guest_handle(memmap.buffer, entries);
+
+ if ( hypercall_memory_op(XENMEM_reserved_device_memory_map,
+ &memmap) != 0 )
+ BUG();
+
+ map_done = 1;
+
+ return 0;
+}
+
/*
* Local variables:
* mode: C
diff --git a/tools/firmware/hvmloader/util.h b/tools/firmware/hvmloader/util.h
index a70e4aa..b6ca739 100644
--- a/tools/firmware/hvmloader/util.h
+++ b/tools/firmware/hvmloader/util.h
@@ -241,6 +241,17 @@ int build_e820_table(struct e820entry *e820,
unsigned int bios_image_base);
void dump_e820_table(struct e820entry *e820, unsigned int nr);
+
+struct reserved_device_memory {
+ /* PFN of the current mapping of the page. */
+ unsigned int pfn;
+ /* Number of the current mapping pages. */
+ unsigned int count;
+};
+typedef struct reserved_device_memory reserved_device_memory_t;
+
+int get_reserved_device_memory_map(struct reserved_device_memory *entries,
+ uint32_t max_entries);
#ifndef NDEBUG
void perform_tests(void);
#else
--
1.9.1
^ permalink raw reply related [flat|nested] 30+ messages in thread* [v4][PATCH 8/9] tools:firmware:hvmloader: check to reserve RMRR mappings in e820
2014-08-22 10:09 [v4][PATCH 0/9] xen: reserve RMRR to avoid conflicting MMIO/RAM Tiejun Chen
` (6 preceding siblings ...)
2014-08-22 10:09 ` [v4][PATCH 7/9] tools:firmware:hvmloader: introduce hypercall for xc_reserved_device_memory_map Tiejun Chen
@ 2014-08-22 10:09 ` Tiejun Chen
2014-08-22 10:09 ` [v4][PATCH 9/9] xen:vtd: make USB RMRR mapping safe Tiejun Chen
8 siblings, 0 replies; 30+ messages in thread
From: Tiejun Chen @ 2014-08-22 10:09 UTC (permalink / raw)
To: JBeulich, ian.jackson, stefano.stabellini, ian.campbell,
yang.z.zhang, kevin.tian
Cc: xen-devel
We need to check to reserve all RMRR mappings in e820 to avoid any
potential guest memory conflict.
Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
---
tools/firmware/hvmloader/e820.c | 101 ++++++++++++++++++++++++++++++++++++++++
1 file changed, 101 insertions(+)
diff --git a/tools/firmware/hvmloader/e820.c b/tools/firmware/hvmloader/e820.c
index 2e05e93..ae51fa6 100644
--- a/tools/firmware/hvmloader/e820.c
+++ b/tools/firmware/hvmloader/e820.c
@@ -68,12 +68,90 @@ void dump_e820_table(struct e820entry *e820, unsigned int nr)
}
}
+static unsigned int construct_rmrr_e820_maps(unsigned int nr,
+ uint32_t nr_map,
+ struct reserved_device_memory *map,
+ struct e820entry *e820)
+{
+ unsigned int i = 0, j = 0, m = 0, sum_nr = 0;
+ uint64_t start, end, rmrr_start, rmrr_end;
+ unsigned int insert = 0, do_insert = 0;
+
+do_real_construct:
+ sum_nr = nr + nr_map;
+ for ( i = 0; i < nr_map; i++ )
+ {
+ rmrr_start = map[i].pfn << PAGE_SHIFT;
+ rmrr_end = rmrr_start + map[i].count * PAGE_SIZE;
+
+ for ( j = 0; j < nr; j++ )
+ {
+ end = e820[j].addr + e820[j].size;
+ start = e820[j+1].addr;
+
+ /* Between those existing e820 entries. */
+ if ( (rmrr_start > end) && (rmrr_end < start) )
+ {
+ if (do_insert)
+ {
+ /* Move to free this entry. */
+ for ( m = sum_nr - 1; m > j; m-- )
+ {
+ e820[m].addr = e820[m-1].addr;
+ e820[m].size = e820[m-1].size;
+ e820[m].type = e820[m-1].type;
+ }
+
+ /* Then fill RMRR into that entry. */
+ e820[j+1].addr = rmrr_start;
+ e820[j+1].size = rmrr_end - rmrr_start;
+ e820[j+1].type = E820_RESERVED;
+ nr++;
+ }
+ insert++;
+ }
+ /* Already at the end. */
+ else if ( (rmrr_start > end) && !start )
+ {
+ if ( do_insert )
+ {
+ e820[nr].addr = rmrr_start;
+ e820[nr].size = rmrr_end - rmrr_start;
+ e820[nr].type = E820_RESERVED;
+ nr++;
+ }
+ insert++;
+ }
+ }
+ }
+
+ /* Just return if done. */
+ if ( do_insert )
+ return nr;
+
+ /* Fine to construct RMRR mappings into e820. */
+ if ( insert == nr_map)
+ {
+ do_insert = 1;
+ goto do_real_construct;
+ }
+ /* Overlap. */
+ else
+ {
+ printf("RMRR overlap with those existing e820 entries!\n");
+ printf("So we don't construct RMRR mapping in e820!\n");
+ }
+
+ return nr;
+}
/* Create an E820 table based on memory parameters provided in hvm_info. */
int build_e820_table(struct e820entry *e820,
unsigned int lowmem_reserved_base,
unsigned int bios_image_base)
{
unsigned int nr = 0;
+ struct reserved_device_memory *map = NULL;
+ int rc;
if ( !lowmem_reserved_base )
lowmem_reserved_base = 0xA0000;
@@ -169,6 +247,29 @@ int build_e820_table(struct e820entry *e820,
nr++;
}
+ /* We'd better reserve RMRR mapping for each VM to avoid potential
+ * memory conflict.
+ */
+ if ( hvm_info->nr_reserved_device_memory_map )
+ {
+ if ( !map )
+ map = mem_alloc(hvm_info->nr_reserved_device_memory_map * sizeof(struct reserved_device_memory), 0);
+ if ( map )
+ {
+ rc = get_reserved_device_memory_map(map,
+ hvm_info->nr_reserved_device_memory_map);
+ if ( !rc )
+ {
+ nr = construct_rmrr_e820_maps(nr,
+ hvm_info->nr_reserved_device_memory_map,
+ map,
+ e820);
+ }
+ }
+ else
+ printf("No space to get reserved_device_memory_map!\n");
+ }
+
return nr;
}
--
1.9.1
^ permalink raw reply related [flat|nested] 30+ messages in thread* [v4][PATCH 9/9] xen:vtd: make USB RMRR mapping safe
2014-08-22 10:09 [v4][PATCH 0/9] xen: reserve RMRR to avoid conflicting MMIO/RAM Tiejun Chen
` (7 preceding siblings ...)
2014-08-22 10:09 ` [v4][PATCH 8/9] tools:firmware:hvmloader: check to reserve RMRR mappings in e820 Tiejun Chen
@ 2014-08-22 10:09 ` Tiejun Chen
8 siblings, 0 replies; 30+ messages in thread
From: Tiejun Chen @ 2014-08-22 10:09 UTC (permalink / raw)
To: JBeulich, ian.jackson, stefano.stabellini, ian.campbell,
yang.z.zhang, kevin.tian
Cc: xen-devel
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
^ permalink raw reply related [flat|nested] 30+ messages in thread