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

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

[Tiejun Chen]

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 (5):
      xen:x86: record RMRR mappings
      xen:x86: introduce a new hypercall to get RMRR mappings
      tools:libxc: remove mmio BAR out of RMRR mappings
      tools:firmware:hvmloader: reserve RMRR mappings in e820
      xen:vtd: make USB RMRR mapping safe

 tools/firmware/hvmloader/e820.c     | 14 ++++++++++++++
 tools/firmware/hvmloader/e820.h     |  6 ++++++
 tools/firmware/hvmloader/util.c     | 13 +++++++++++++
 tools/firmware/hvmloader/util.h     |  1 +
 tools/libxc/xc_domain.c             | 25 +++++++++++++++++++++++++
 tools/libxc/xc_hvm_build_x86.c      | 26 ++++++++++++++++++++++++++
 tools/libxc/xenctrl.h               |  4 ++++
 xen/arch/x86/e820.c                 |  2 ++
 xen/arch/x86/mm.c                   | 35 +++++++++++++++++++++++++++++++++++
 xen/arch/x86/x86_64/compat/mm.c     |  8 ++++++++
 xen/drivers/passthrough/vtd/dmar.c  | 15 +++++++++++++++
 xen/drivers/passthrough/vtd/iommu.c |  8 --------
 xen/include/asm-x86/e820.h          |  1 +
 xen/include/public/memory.h         | 10 +++++++++-
 14 files changed, 159 insertions(+), 9 deletions(-)

Thanks
Tiejun

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

[Tiejun Chen]

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

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

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

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

[Jan Beulich]

>>> On 07.08.14 at 13:02, <tiejun.chen@intel.com> wrote:
> +/* Record RMRR mapping to ready expose VM. */
> +static int __init rmrr_e820_register(struct acpi_rmrr_unit *rmrr)
> +{
> +    static int i = 0;
> +
> +    rmrr_e820.map[i].addr = rmrr->base_address;
> +    rmrr_e820.map[i].size = rmrr->end_address - rmrr->base_address;
> +    rmrr_e820.map[i].type = E820_RESERVED;
> +    rmrr_e820.nr_map = i;
> +    i++;
> +    return 0;
> +}

As already said elsewhere, the piggybacking on the E820 structure
isn't suitable here due to that ones limited size.

Also, just as general remarks,
- "i" should be unsigned and placed in __initdata
- "i" anyway is redundant with rmrr_e820.nr_map
- there should be a blank line between the last "ordinary"
  and the return statements

Jan

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

[Chen, Tiejun]

On 2014/8/8 23:36, Jan Beulich wrote:
>>>> On 07.08.14 at 13:02, <tiejun.chen@intel.com> wrote:
>> +/* Record RMRR mapping to ready expose VM. */
>> +static int __init rmrr_e820_register(struct acpi_rmrr_unit *rmrr)
>> +{
>> +    static int i = 0;
>> +
>> +    rmrr_e820.map[i].addr = rmrr->base_address;
>> +    rmrr_e820.map[i].size = rmrr->end_address - rmrr->base_address;
>> +    rmrr_e820.map[i].type = E820_RESERVED;
>> +    rmrr_e820.nr_map = i;
>> +    i++;
>> +    return 0;
>> +}
>
> As already said elsewhere, the piggybacking on the E820 structure
> isn't suitable here due to that ones limited size.

Are you saying the limited number of e820entry? If yes, I don't think 
this would be limited here.

Because struct e820map always define this as follows,

#define E820MAX 128

struct e820map {
     unsigned int nr_map;
     struct e820entry map[E820MAX];
};

>
> Also, just as general remarks,
> - "i" should be unsigned and placed in __initdata
> - "i" anyway is redundant with rmrr_e820.nr_map
> - there should be a blank line between the last "ordinary"
>    and the return statements
>

I will try to refine these stuffs next revision.

Thanks
Tiejun

> Jan
>
>

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

[Jan Beulich]

>>> On 11.08.14 at 05:04, <tiejun.chen@intel.com> wrote:
> On 2014/8/8 23:36, Jan Beulich wrote:
>>>>> On 07.08.14 at 13:02, <tiejun.chen@intel.com> wrote:
>>> +/* Record RMRR mapping to ready expose VM. */
>>> +static int __init rmrr_e820_register(struct acpi_rmrr_unit *rmrr)
>>> +{
>>> +    static int i = 0;
>>> +
>>> +    rmrr_e820.map[i].addr = rmrr->base_address;
>>> +    rmrr_e820.map[i].size = rmrr->end_address - rmrr->base_address;
>>> +    rmrr_e820.map[i].type = E820_RESERVED;
>>> +    rmrr_e820.nr_map = i;
>>> +    i++;
>>> +    return 0;
>>> +}
>>
>> As already said elsewhere, the piggybacking on the E820 structure
>> isn't suitable here due to that ones limited size.
> 
> Are you saying the limited number of e820entry? If yes, I don't think 
> this would be limited here.
> 
> Because struct e820map always define this as follows,
> 
> #define E820MAX 128
> 
> struct e820map {
>      unsigned int nr_map;
>      struct e820entry map[E820MAX];
> };

So are you saying that to you 128 is not a limit?

Btw., along with re-doing all this, I'd also strongly suggest not making
the new hypercall a mem-op - this is clearly more like a sysctl or
platform-op (but the latter might be too restrictive in terms of who
may call it).

Jan

Jan

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

[Chen, Tiejun]

On 2014/8/11 14:51, Jan Beulich wrote:
>>>> On 11.08.14 at 05:04, <tiejun.chen@intel.com> wrote:
>> On 2014/8/8 23:36, Jan Beulich wrote:
>>>>>> On 07.08.14 at 13:02, <tiejun.chen@intel.com> wrote:
>>>> +/* Record RMRR mapping to ready expose VM. */
>>>> +static int __init rmrr_e820_register(struct acpi_rmrr_unit *rmrr)
>>>> +{
>>>> +    static int i = 0;
>>>> +
>>>> +    rmrr_e820.map[i].addr = rmrr->base_address;
>>>> +    rmrr_e820.map[i].size = rmrr->end_address - rmrr->base_address;
>>>> +    rmrr_e820.map[i].type = E820_RESERVED;
>>>> +    rmrr_e820.nr_map = i;
>>>> +    i++;
>>>> +    return 0;
>>>> +}
>>>
>>> As already said elsewhere, the piggybacking on the E820 structure
>>> isn't suitable here due to that ones limited size.
>>
>> Are you saying the limited number of e820entry? If yes, I don't think
>> this would be limited here.
>>
>> Because struct e820map always define this as follows,
>>
>> #define E820MAX 128
>>
>> struct e820map {
>>       unsigned int nr_map;
>>       struct e820entry map[E820MAX];
>> };
>
> So are you saying that to you 128 is not a limit?

When we post that hypercall initially, we don't set up the limitation 
but Xen should reset the nr_map to indicate the real number.

>
> Btw., along with re-doing all this, I'd also strongly suggest not making
> the new hypercall a mem-op - this is clearly more like a sysctl or

Without hypercall how to build guest e820 table by the hvmloader? Or you 
mean we can set these rmrr info into build_info_table or elsewhere 
hvmloader can get them directly?

Thanks
Tiejun

> platform-op (but the latter might be too restrictive in terms of who
> may call it).
>
> Jan
>
> Jan
>
>

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

[Jan Beulich]

>>> On 11.08.14 at 09:00, <tiejun.chen@intel.com> wrote:
> On 2014/8/11 14:51, Jan Beulich wrote:
>>>>> On 11.08.14 at 05:04, <tiejun.chen@intel.com> wrote:
>>> On 2014/8/8 23:36, Jan Beulich wrote:
>>>>>>> On 07.08.14 at 13:02, <tiejun.chen@intel.com> wrote:
>>>>> +/* Record RMRR mapping to ready expose VM. */
>>>>> +static int __init rmrr_e820_register(struct acpi_rmrr_unit *rmrr)
>>>>> +{
>>>>> +    static int i = 0;
>>>>> +
>>>>> +    rmrr_e820.map[i].addr = rmrr->base_address;
>>>>> +    rmrr_e820.map[i].size = rmrr->end_address - rmrr->base_address;
>>>>> +    rmrr_e820.map[i].type = E820_RESERVED;
>>>>> +    rmrr_e820.nr_map = i;
>>>>> +    i++;
>>>>> +    return 0;
>>>>> +}
>>>>
>>>> As already said elsewhere, the piggybacking on the E820 structure
>>>> isn't suitable here due to that ones limited size.
>>>
>>> Are you saying the limited number of e820entry? If yes, I don't think
>>> this would be limited here.
>>>
>>> Because struct e820map always define this as follows,
>>>
>>> #define E820MAX 128
>>>
>>> struct e820map {
>>>       unsigned int nr_map;
>>>       struct e820entry map[E820MAX];
>>> };
>>
>> So are you saying that to you 128 is not a limit?
> 
> When we post that hypercall initially, we don't set up the limitation 
> but Xen should reset the nr_map to indicate the real number.

???

>> Btw., along with re-doing all this, I'd also strongly suggest not making
>> the new hypercall a mem-op - this is clearly more like a sysctl or
> 
> Without hypercall how to build guest e820 table by the hvmloader? Or you 
> mean we can set these rmrr info into build_info_table or elsewhere 
> hvmloader can get them directly?

I similarly (as above) don't really understand what you're trying
to ask/tell me, but at least I now realize that you need to do this
call _inside_ the guest, so it can't be a sysctl.

Jan

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

[Tiejun Chen]

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

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

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index d23cb3f..8176fd4 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -4769,6 +4769,41 @@ long arch_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
         return 0;
     }
 
+    case XENMEM_RMRR_memory_map:
+    {
+        struct memory_map_context ctxt;
+        XEN_GUEST_HANDLE(e820entry_t) buffer;
+        XEN_GUEST_HANDLE_PARAM(e820entry_t) buffer_param;
+        unsigned int i;
+
+        rc = xsm_machine_memory_map(XSM_PRIV);
+        if ( rc )
+            return rc;
+
+        if ( copy_from_guest(&ctxt.map, arg, 1) )
+            return -EFAULT;
+        if ( ctxt.map.nr_entries < rmrr_e820.nr_map + 1 )
+            return -EINVAL;
+
+        buffer_param = guest_handle_cast(ctxt.map.buffer, e820entry_t);
+        buffer = guest_handle_from_param(buffer_param, e820entry_t);
+        if ( !guest_handle_okay(buffer, ctxt.map.nr_entries) )
+            return -EFAULT;
+
+        for ( i = 0, ctxt.n = 0, ctxt.s = 0; i < rmrr_e820.nr_map; ++i, ++ctxt.n )
+        {
+            if ( __copy_to_guest_offset(buffer, ctxt.n, rmrr_e820.map + i, 1) )
+                return -EFAULT;
+        }
+
+        ctxt.map.nr_entries = ctxt.n;
+
+        if ( __copy_to_guest(arg, &ctxt.map, 1) )
+            return -EFAULT;
+
+        return 0;
+    }
+
     case XENMEM_machphys_mapping:
     {
         struct xen_machphys_mapping mapping = {
diff --git a/xen/arch/x86/x86_64/compat/mm.c b/xen/arch/x86/x86_64/compat/mm.c
index 69c6195..c35d05d 100644
--- a/xen/arch/x86/x86_64/compat/mm.c
+++ b/xen/arch/x86/x86_64/compat/mm.c
@@ -132,6 +132,14 @@ int compat_arch_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
         break;
     }
 
+    case XENMEM_RMRR_memory_map:
+    {
+        if ( copy_to_guest(arg, &rmrr_e820, 1) )
+            return -EFAULT;
+
+        return 0;
+    }
+
     case XENMEM_machphys_mapping:
     {
         struct domain *d = current->domain;
diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h
index 2c57aa0..e04c0a3 100644
--- a/xen/include/public/memory.h
+++ b/xen/include/public/memory.h
@@ -523,7 +523,15 @@ DEFINE_XEN_GUEST_HANDLE(xen_mem_sharing_op_t);
 
 #endif /* defined(__XEN__) || defined(__XEN_TOOLS__) */
 
-/* Next available subop number is 26 */
+/*
+ * Returns the RMRR memory map as it was when the domain
+ * was started.
+ */
+#define XENMEM_RMRR_memory_map           26
+typedef struct e820map rmrr_e820_t;
+DEFINE_XEN_GUEST_HANDLE(rmrr_e820_t);
+
+/* Next available subop number is 27 */
 
 #endif /* __XEN_PUBLIC_MEMORY_H__ */
 
-- 
1.9.1

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

[Jan Beulich]

>>> On 07.08.14 at 13:02, <tiejun.chen@intel.com> wrote:
> +    case XENMEM_RMRR_memory_map:
> +    {
> +        struct memory_map_context ctxt;

???

> +        XEN_GUEST_HANDLE(e820entry_t) buffer;
> +        XEN_GUEST_HANDLE_PARAM(e820entry_t) buffer_param;
> +        unsigned int i;
> +
> +        rc = xsm_machine_memory_map(XSM_PRIV);

Are you sure? Can (and should) this really not be exposed to semi-
privileged domains?

> +        if ( rc )
> +            return rc;
> +
> +        if ( copy_from_guest(&ctxt.map, arg, 1) )
> +            return -EFAULT;
> +        if ( ctxt.map.nr_entries < rmrr_e820.nr_map + 1 )
> +            return -EINVAL;

So how would the caller know how many entries are needed?

> +
> +        buffer_param = guest_handle_cast(ctxt.map.buffer, e820entry_t);
> +        buffer = guest_handle_from_param(buffer_param, e820entry_t);
> +        if ( !guest_handle_okay(buffer, ctxt.map.nr_entries) )
> +            return -EFAULT;
> +
> +        for ( i = 0, ctxt.n = 0, ctxt.s = 0; i < rmrr_e820.nr_map; ++i, ++ctxt.n )

i and ctxt.n are redundant.

> +        {
> +            if ( __copy_to_guest_offset(buffer, ctxt.n, rmrr_e820.map + i, 1) )
> +                return -EFAULT;
> +        }
> +
> +        ctxt.map.nr_entries = ctxt.n;
> +
> +        if ( __copy_to_guest(arg, &ctxt.map, 1) )

__copy_field_to_guest() if all you need to copy back is a single field.

> --- a/xen/arch/x86/x86_64/compat/mm.c
> +++ b/xen/arch/x86/x86_64/compat/mm.c
> @@ -132,6 +132,14 @@ int compat_arch_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>          break;
>      }
>  
> +    case XENMEM_RMRR_memory_map:
> +    {
> +        if ( copy_to_guest(arg, &rmrr_e820, 1) )
> +            return -EFAULT;
> +
> +        return 0;
> +    }

Pointless braces. And how come this is so much simpler than the
native version?

> --- a/xen/include/public/memory.h
> +++ b/xen/include/public/memory.h
> @@ -523,7 +523,15 @@ DEFINE_XEN_GUEST_HANDLE(xen_mem_sharing_op_t);
>  
>  #endif /* defined(__XEN__) || defined(__XEN_TOOLS__) */
>  
> -/* Next available subop number is 26 */
> +/*
> + * Returns the RMRR memory map as it was when the domain
> + * was started.
> + */
> +#define XENMEM_RMRR_memory_map           26
> +typedef struct e820map rmrr_e820_t;
> +DEFINE_XEN_GUEST_HANDLE(rmrr_e820_t);

Again just as a general remark: What in the world does the "e820"
in here mean?

Jan

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

[Chen, Tiejun]

On 2014/8/8 23:45, Jan Beulich wrote:
>>>> On 07.08.14 at 13:02, <tiejun.chen@intel.com> wrote:
>> +    case XENMEM_RMRR_memory_map:
>> +    {
>> +        struct memory_map_context ctxt;
>
> ???
>
>> +        XEN_GUEST_HANDLE(e820entry_t) buffer;
>> +        XEN_GUEST_HANDLE_PARAM(e820entry_t) buffer_param;
>> +        unsigned int i;
>> +
>> +        rc = xsm_machine_memory_map(XSM_PRIV);
>
> Are you sure? Can (and should) this really not be exposed to semi-
> privileged domains?

Will fixed.

>
>> +        if ( rc )
>> +            return rc;
>> +
>> +        if ( copy_from_guest(&ctxt.map, arg, 1) )
>> +            return -EFAULT;
>> +        if ( ctxt.map.nr_entries < rmrr_e820.nr_map + 1 )
>> +            return -EINVAL;
>
> So how would the caller know how many entries are needed?
>
>> +
>> +        buffer_param = guest_handle_cast(ctxt.map.buffer, e820entry_t);
>> +        buffer = guest_handle_from_param(buffer_param, e820entry_t);
>> +        if ( !guest_handle_okay(buffer, ctxt.map.nr_entries) )
>> +            return -EFAULT;
>> +
>> +        for ( i = 0, ctxt.n = 0, ctxt.s = 0; i < rmrr_e820.nr_map; ++i, ++ctxt.n )
>
> i and ctxt.n are redundant.
>
>> +        {
>> +            if ( __copy_to_guest_offset(buffer, ctxt.n, rmrr_e820.map + i, 1) )
>> +                return -EFAULT;
>> +        }
>> +
>> +        ctxt.map.nr_entries = ctxt.n;
>> +
>> +        if ( __copy_to_guest(arg, &ctxt.map, 1) )
>
> __copy_field_to_guest() if all you need to copy back is a single field.

I will try to address all comments in next revision.

>
>> --- a/xen/arch/x86/x86_64/compat/mm.c
>> +++ b/xen/arch/x86/x86_64/compat/mm.c
>> @@ -132,6 +132,14 @@ int compat_arch_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>>           break;
>>       }
>>
>> +    case XENMEM_RMRR_memory_map:
>> +    {
>> +        if ( copy_to_guest(arg, &rmrr_e820, 1) )
>> +            return -EFAULT;
>> +
>> +        return 0;
>> +    }
>
> Pointless braces. And how come this is so much simpler than the
> native version?

Just hvmloader would walk this with a hypercall, and with a test I don't 
see any issue here.

If you think this is not correct, please comment this in next revision.

>
>> --- a/xen/include/public/memory.h
>> +++ b/xen/include/public/memory.h
>> @@ -523,7 +523,15 @@ DEFINE_XEN_GUEST_HANDLE(xen_mem_sharing_op_t);
>>
>>   #endif /* defined(__XEN__) || defined(__XEN_TOOLS__) */
>>
>> -/* Next available subop number is 26 */
>> +/*
>> + * Returns the RMRR memory map as it was when the domain
>> + * was started.
>> + */
>> +#define XENMEM_RMRR_memory_map           26
>> +typedef struct e820map rmrr_e820_t;
>> +DEFINE_XEN_GUEST_HANDLE(rmrr_e820_t);
>
> Again just as a general remark: What in the world does the "e820"
> in here mean?

I will redefine a struct to represent this to avoid any confusion.

Thanks
Tiejun

>
> Jan
>
>

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

[Jan Beulich]

>>> On 12.08.14 at 12:55, <tiejun.chen@intel.com> wrote:
> On 2014/8/8 23:45, Jan Beulich wrote:
>>>>> On 07.08.14 at 13:02, <tiejun.chen@intel.com> wrote:
>>> +/*
>>> + * Returns the RMRR memory map as it was when the domain
>>> + * was started.
>>> + */
>>> +#define XENMEM_RMRR_memory_map           26
>>> +typedef struct e820map rmrr_e820_t;
>>> +DEFINE_XEN_GUEST_HANDLE(rmrr_e820_t);
>>
>> Again just as a general remark: What in the world does the "e820"
>> in here mean?
> 
> I will redefine a struct to represent this to avoid any confusion.

And just to avoid another needless round: The term RMRR shouldn't
be in the hypercall public interface definitions either. This needs to
be properly abstracted.

Jan

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

[Chen, Tiejun]

On 2014/8/12 20:19, Jan Beulich wrote:
>>>> On 12.08.14 at 12:55, <tiejun.chen@intel.com> wrote:
>> On 2014/8/8 23:45, Jan Beulich wrote:
>>>>>> On 07.08.14 at 13:02, <tiejun.chen@intel.com> wrote:
>>>> +/*
>>>> + * Returns the RMRR memory map as it was when the domain
>>>> + * was started.
>>>> + */
>>>> +#define XENMEM_RMRR_memory_map           26
>>>> +typedef struct e820map rmrr_e820_t;
>>>> +DEFINE_XEN_GUEST_HANDLE(rmrr_e820_t);
>>>
>>> Again just as a general remark: What in the world does the "e820"
>>> in here mean?
>>
>> I will redefine a struct to represent this to avoid any confusion.
>
> And just to avoid another needless round: The term RMRR shouldn't
> be in the hypercall public interface definitions either. This needs to
> be properly abstracted.
>

Without such a term RMRR I can't figure out what definition should be 
better as you expect, I guess you already have a better case so please 
share it to avoid further discussion.

Thanks
Tiejun

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

[Tian, Kevin]

> From: Chen, Tiejun
> Sent: Tuesday, August 12, 2014 5:40 PM
> 
> On 2014/8/12 20:19, Jan Beulich wrote:
> >>>> On 12.08.14 at 12:55, <tiejun.chen@intel.com> wrote:
> >> On 2014/8/8 23:45, Jan Beulich wrote:
> >>>>>> On 07.08.14 at 13:02, <tiejun.chen@intel.com> wrote:
> >>>> +/*
> >>>> + * Returns the RMRR memory map as it was when the domain
> >>>> + * was started.
> >>>> + */
> >>>> +#define XENMEM_RMRR_memory_map           26
> >>>> +typedef struct e820map rmrr_e820_t;
> >>>> +DEFINE_XEN_GUEST_HANDLE(rmrr_e820_t);
> >>>
> >>> Again just as a general remark: What in the world does the "e820"
> >>> in here mean?
> >>
> >> I will redefine a struct to represent this to avoid any confusion.
> >
> > And just to avoid another needless round: The term RMRR shouldn't
> > be in the hypercall public interface definitions either. This needs to
> > be properly abstracted.
> >
> 
> Without such a term RMRR I can't figure out what definition should be
> better as you expect, I guess you already have a better case so please
> share it to avoid further discussion.
> 

XENMEM_reserved_memory_map

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

[Chen, Tiejun]

On 2014/8/14 2:21, Tian, Kevin wrote:
>> From: Chen, Tiejun
>> Sent: Tuesday, August 12, 2014 5:40 PM
>>
>> On 2014/8/12 20:19, Jan Beulich wrote:
>>>>>> On 12.08.14 at 12:55, <tiejun.chen@intel.com> wrote:
>>>> On 2014/8/8 23:45, Jan Beulich wrote:
>>>>>>>> On 07.08.14 at 13:02, <tiejun.chen@intel.com> wrote:
>>>>>> +/*
>>>>>> + * Returns the RMRR memory map as it was when the domain
>>>>>> + * was started.
>>>>>> + */
>>>>>> +#define XENMEM_RMRR_memory_map           26
>>>>>> +typedef struct e820map rmrr_e820_t;
>>>>>> +DEFINE_XEN_GUEST_HANDLE(rmrr_e820_t);
>>>>>
>>>>> Again just as a general remark: What in the world does the "e820"
>>>>> in here mean?
>>>>
>>>> I will redefine a struct to represent this to avoid any confusion.
>>>
>>> And just to avoid another needless round: The term RMRR shouldn't
>>> be in the hypercall public interface definitions either. This needs to
>>> be properly abstracted.
>>>
>>
>> Without such a term RMRR I can't figure out what definition should be
>> better as you expect, I guess you already have a better case so please
>> share it to avoid further discussion.
>>
>
> XENMEM_reserved_memory_map
>

Okay, but I prefer to XENMEM_DEVICE_reserved_memory_map or 
XENMEM_PLATFORM_reserved_memory_map since RMRR seems to be dedicated to 
device or platform, right?

Anyway, I'm fine as well once Jan have no any objection to this.

Thanks
Tiejun

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

[Jan Beulich]

>>> On 14.08.14 at 03:07, <tiejun.chen@intel.com> wrote:
> On 2014/8/14 2:21, Tian, Kevin wrote:
>>> From: Chen, Tiejun
>>> Sent: Tuesday, August 12, 2014 5:40 PM
>>>
>>> On 2014/8/12 20:19, Jan Beulich wrote:
>>>>>>> On 12.08.14 at 12:55, <tiejun.chen@intel.com> wrote:
>>>>> On 2014/8/8 23:45, Jan Beulich wrote:
>>>>>>>>> On 07.08.14 at 13:02, <tiejun.chen@intel.com> wrote:
>>>>>>> +/*
>>>>>>> + * Returns the RMRR memory map as it was when the domain
>>>>>>> + * was started.
>>>>>>> + */
>>>>>>> +#define XENMEM_RMRR_memory_map           26
>>>>>>> +typedef struct e820map rmrr_e820_t;
>>>>>>> +DEFINE_XEN_GUEST_HANDLE(rmrr_e820_t);
>>>>>>
>>>>>> Again just as a general remark: What in the world does the "e820"
>>>>>> in here mean?
>>>>>
>>>>> I will redefine a struct to represent this to avoid any confusion.
>>>>
>>>> And just to avoid another needless round: The term RMRR shouldn't
>>>> be in the hypercall public interface definitions either. This needs to
>>>> be properly abstracted.
>>>>
>>>
>>> Without such a term RMRR I can't figure out what definition should be
>>> better as you expect, I guess you already have a better case so please
>>> share it to avoid further discussion.
>>>
>>
>> XENMEM_reserved_memory_map
>>
> 
> Okay, but I prefer to XENMEM_DEVICE_reserved_memory_map or 
> XENMEM_PLATFORM_reserved_memory_map since RMRR seems to be dedicated to 
> device or platform, right?
> 
> Anyway, I'm fine as well once Jan have no any objection to this.

I'd be fine with Kevin's suggestion (as I can't see any other
reserved memory maps to appear), but I'd also be fine with
XENMEM_reserved_device_memory_map or some such to
suit your concerns.

Jan

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

[Chen, Tiejun]

On 2014/8/15 0:51, Jan Beulich wrote:
>>>> On 14.08.14 at 03:07, <tiejun.chen@intel.com> wrote:
>> On 2014/8/14 2:21, Tian, Kevin wrote:
>>>> From: Chen, Tiejun
>>>> Sent: Tuesday, August 12, 2014 5:40 PM
>>>>
>>>> On 2014/8/12 20:19, Jan Beulich wrote:
>>>>>>>> On 12.08.14 at 12:55, <tiejun.chen@intel.com> wrote:
>>>>>> On 2014/8/8 23:45, Jan Beulich wrote:
>>>>>>>>>> On 07.08.14 at 13:02, <tiejun.chen@intel.com> wrote:
>>>>>>>> +/*
>>>>>>>> + * Returns the RMRR memory map as it was when the domain
>>>>>>>> + * was started.
>>>>>>>> + */
>>>>>>>> +#define XENMEM_RMRR_memory_map           26
>>>>>>>> +typedef struct e820map rmrr_e820_t;
>>>>>>>> +DEFINE_XEN_GUEST_HANDLE(rmrr_e820_t);
>>>>>>>
>>>>>>> Again just as a general remark: What in the world does the "e820"
>>>>>>> in here mean?
>>>>>>
>>>>>> I will redefine a struct to represent this to avoid any confusion.
>>>>>
>>>>> And just to avoid another needless round: The term RMRR shouldn't
>>>>> be in the hypercall public interface definitions either. This needs to
>>>>> be properly abstracted.
>>>>>
>>>>
>>>> Without such a term RMRR I can't figure out what definition should be
>>>> better as you expect, I guess you already have a better case so please
>>>> share it to avoid further discussion.
>>>>
>>>
>>> XENMEM_reserved_memory_map
>>>
>>
>> Okay, but I prefer to XENMEM_DEVICE_reserved_memory_map or
>> XENMEM_PLATFORM_reserved_memory_map since RMRR seems to be dedicated to
>> device or platform, right?
>>
>> Anyway, I'm fine as well once Jan have no any objection to this.
>
> I'd be fine with Kevin's suggestion (as I can't see any other
> reserved memory maps to appear), but I'd also be fine with

I double check this again and looks we already have the following 
structure definition that may be easy to confuse with 
XENMEM_reserved_memory_map,

typedef struct xen_memory_reservation xen_memory_reservation_t;
DEFINE_XEN_GUEST_HANDLE(xen_memory_reservation_t);

> XENMEM_reserved_device_memory_map or some such to

So I'd like to adopt this.

Thanks
Tiejun

> suit your concerns.
>
> Jan
>
>

[RFC][PATCH 3/5] tools:libxc: remove mmio BAR out of RMRR mappings

[Tiejun Chen]

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

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

diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c
index 0230c6c..c1dddb1 100644
--- a/tools/libxc/xc_domain.c
+++ b/tools/libxc/xc_domain.c
@@ -648,6 +648,31 @@ int xc_domain_set_memory_map(xc_interface *xch,
 
     return rc;
 }
+
+int xc_get_rmrr_map(xc_interface *xch,
+                    struct e820entry entries[],
+                    uint32_t max_entries)
+{
+    int rc;
+    struct xen_memory_map memmap = {
+        .nr_entries = max_entries
+    };
+    DECLARE_HYPERCALL_BOUNCE(entries, sizeof(struct e820entry) * max_entries,
+                             XC_HYPERCALL_BUFFER_BOUNCE_OUT);
+
+    if ( !entries || xc_hypercall_bounce_pre(xch, entries) || max_entries <= 1)
+        return -1;
+
+
+    set_xen_guest_handle(memmap.buffer, entries);
+
+    rc = do_memory_op(xch, XENMEM_RMRR_memory_map, &memmap, sizeof(memmap));
+
+    xc_hypercall_bounce_post(xch, entries);
+
+    return rc ? rc : memmap.nr_entries;
+}
+
 int xc_get_machine_memory_map(xc_interface *xch,
                               struct e820entry entries[],
                               uint32_t max_entries)
diff --git a/tools/libxc/xc_hvm_build_x86.c b/tools/libxc/xc_hvm_build_x86.c
index c81a25b..27d8182 100644
--- a/tools/libxc/xc_hvm_build_x86.c
+++ b/tools/libxc/xc_hvm_build_x86.c
@@ -262,6 +262,8 @@ static int setup_guest(xc_interface *xch,
     int claim_enabled = args->claim_enabled;
     xen_pfn_t special_array[NR_SPECIAL_PAGES];
     xen_pfn_t ioreq_server_array[NR_IOREQ_SERVER_PAGES];
+    struct e820entry map[E820MAX];
+    uint64_t rmrr_end = 0;
 
     if ( nr_pages > target_pages )
         pod_mode = XENMEMF_populate_on_demand;
@@ -300,6 +302,30 @@ static int setup_guest(xc_interface *xch,
         goto error_out;
     }
 
+    /* We need to move mmio range out of RMRR mapping. */
+    rc = xc_get_rmrr_map(xch, map, E820MAX);
+    if (rc < 0)
+    {
+        PERROR("Could not get RMRR info on domain");
+    }
+    else if ( rc )
+    {
+        for ( i = 0; i < rc; i++ )
+        {
+            rmrr_end = map[i].addr + map[i].size + 1;
+            if ( rmrr_end > mmio_start )
+            {
+                mmio_start = rmrr_end;
+            }
+        }
+        mmio_size = (1ull << 32) - mmio_start;
+        if ( mmio_size <= 0 )
+        {
+            PERROR("Could not allocate MMIO memory.");
+            goto error_out;
+        }
+    }
+
     for ( i = 0; i < nr_pages; i++ )
         page_array[i] = i;
     for ( i = mmio_start >> PAGE_SHIFT; i < nr_pages; i++ )
diff --git a/tools/libxc/xenctrl.h b/tools/libxc/xenctrl.h
index 5beb846..14ef42f 100644
--- a/tools/libxc/xenctrl.h
+++ b/tools/libxc/xenctrl.h
@@ -1270,6 +1270,10 @@ int xc_domain_set_memory_map(xc_interface *xch,
 int xc_get_machine_memory_map(xc_interface *xch,
                               struct e820entry entries[],
                               uint32_t max_entries);
+
+int xc_get_rmrr_map(xc_interface *xch,
+                    struct e820entry entries[],
+                    uint32_t max_entries);
 #endif
 int xc_domain_set_time_offset(xc_interface *xch,
                               uint32_t domid,
-- 
1.9.1

Re: [RFC][PATCH 3/5] tools:libxc: remove mmio BAR out of RMRR mappings

[Jan Beulich]

>>> On 07.08.14 at 13:02, <tiejun.chen@intel.com> wrote:
> @@ -300,6 +302,30 @@ static int setup_guest(xc_interface *xch,
>          goto error_out;
>      }
>  
> +    /* We need to move mmio range out of RMRR mapping. */
> +    rc = xc_get_rmrr_map(xch, map, E820MAX);
> +    if (rc < 0)
> +    {
> +        PERROR("Could not get RMRR info on domain");
> +    }
> +    else if ( rc )
> +    {
> +        for ( i = 0; i < rc; i++ )
> +        {
> +            rmrr_end = map[i].addr + map[i].size + 1;
> +            if ( rmrr_end > mmio_start )
> +            {
> +                mmio_start = rmrr_end;
> +            }
> +        }

This seems way too simplistic - what if the RMRRs are referring to
memory regions extremely far apart?

> +        mmio_size = (1ull << 32) - mmio_start;

Limiting things to 4Gb?

> +        if ( mmio_size <= 0 )

mmio_size is an unsigned quantity, so this won't do what you intend.

Jan

Re: [RFC][PATCH 3/5] tools:libxc: remove mmio BAR out of RMRR mappings

[Tian, Kevin]

> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Friday, August 08, 2014 8:49 AM
> 
> >>> On 07.08.14 at 13:02, <tiejun.chen@intel.com> wrote:
> > @@ -300,6 +302,30 @@ static int setup_guest(xc_interface *xch,
> >          goto error_out;
> >      }
> >
> > +    /* We need to move mmio range out of RMRR mapping. */
> > +    rc = xc_get_rmrr_map(xch, map, E820MAX);
> > +    if (rc < 0)
> > +    {
> > +        PERROR("Could not get RMRR info on domain");
> > +    }
> > +    else if ( rc )
> > +    {
> > +        for ( i = 0; i < rc; i++ )
> > +        {
> > +            rmrr_end = map[i].addr + map[i].size + 1;
> > +            if ( rmrr_end > mmio_start )
> > +            {
> > +                mmio_start = rmrr_end;
> > +            }
> > +        }
> 
> This seems way too simplistic - what if the RMRRs are referring to
> memory regions extremely far apart?
> 
> > +        mmio_size = (1ull << 32) - mmio_start;
> 
> Limiting things to 4Gb?
> 
> > +        if ( mmio_size <= 0 )
> 
> mmio_size is an unsigned quantity, so this won't do what you intend.
> 

and it's not only about the start of mmio, but also the confliction with normal
guest memory pages, i.e. all the populate_physmap calls should check with
RMRR ranges.

Thanks
Kevin

Re: [RFC][PATCH 3/5] tools:libxc: remove mmio BAR out of RMRR mappings

[Chen, Tiejun]

On 2014/8/9 5:33, Tian, Kevin wrote:
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: Friday, August 08, 2014 8:49 AM
>>
>>>>> On 07.08.14 at 13:02, <tiejun.chen@intel.com> wrote:
>>> @@ -300,6 +302,30 @@ static int setup_guest(xc_interface *xch,
>>>           goto error_out;
>>>       }
>>>
>>> +    /* We need to move mmio range out of RMRR mapping. */
>>> +    rc = xc_get_rmrr_map(xch, map, E820MAX);
>>> +    if (rc < 0)
>>> +    {
>>> +        PERROR("Could not get RMRR info on domain");
>>> +    }
>>> +    else if ( rc )
>>> +    {
>>> +        for ( i = 0; i < rc; i++ )
>>> +        {
>>> +            rmrr_end = map[i].addr + map[i].size + 1;
>>> +            if ( rmrr_end > mmio_start )
>>> +            {
>>> +                mmio_start = rmrr_end;
>>> +            }
>>> +        }
>>
>> This seems way too simplistic - what if the RMRRs are referring to
>> memory regions extremely far apart?
>>
>>> +        mmio_size = (1ull << 32) - mmio_start;
>>
>> Limiting things to 4Gb?
>>
>>> +        if ( mmio_size <= 0 )
>>
>> mmio_size is an unsigned quantity, so this won't do what you intend.
>>
>
> and it's not only about the start of mmio, but also the confliction with normal
> guest memory pages, i.e. all the populate_physmap calls should check with

RMRR maps should be as reserved by Xen, right? So why will we still 
check this? I mean Xen should guarantee we don't allocate those reserved 
pages to guest.

Thanks
Tiejun

> RMRR ranges.
>
> Thanks
> Kevin
>
>

Re: [RFC][PATCH 3/5] tools:libxc: remove mmio BAR out of RMRR mappings

[Jan Beulich]

>>> On 12.08.14 at 12:56, <tiejun.chen@intel.com> wrote:
> On 2014/8/9 5:33, Tian, Kevin wrote:
>> and it's not only about the start of mmio, but also the confliction with normal
>> guest memory pages, i.e. all the populate_physmap calls should check with
> 
> RMRR maps should be as reserved by Xen, right? So why will we still 
> check this? I mean Xen should guarantee we don't allocate those reserved 
> pages to guest.

The question is not the memory pages and their use, but the physical
address space use in the guests.

Jan

Re: [RFC][PATCH 3/5] tools:libxc: remove mmio BAR out of RMRR mappings

[Chen, Tiejun]

On 2014/8/8 23:49, Jan Beulich wrote:
>>>> On 07.08.14 at 13:02, <tiejun.chen@intel.com> wrote:
>> @@ -300,6 +302,30 @@ static int setup_guest(xc_interface *xch,
>>           goto error_out;
>>       }
>>
>> +    /* We need to move mmio range out of RMRR mapping. */
>> +    rc = xc_get_rmrr_map(xch, map, E820MAX);
>> +    if (rc < 0)
>> +    {
>> +        PERROR("Could not get RMRR info on domain");
>> +    }
>> +    else if ( rc )
>> +    {
>> +        for ( i = 0; i < rc; i++ )
>> +        {
>> +            rmrr_end = map[i].addr + map[i].size + 1;
>> +            if ( rmrr_end > mmio_start )
>> +            {
>> +                mmio_start = rmrr_end;
>> +            }
>> +        }
>
> This seems way too simplistic - what if the RMRRs are referring to
> memory regions extremely far apart?
>
>> +        mmio_size = (1ull << 32) - mmio_start;
>
> Limiting things to 4Gb?
>
>> +        if ( mmio_size <= 0 )
>
> mmio_size is an unsigned quantity, so this won't do what you intend.

Yes, you're right.

With a further consideration, mmio size should be configured so I think 
it may be enough just to check if current mmio is overlapping, then let 
user determine what next should do.

Please comments this in next revision directly.

Thanks
Tiejun
>
> Jan
>
>

[RFC][PATCH 4/5] tools:firmware:hvmloader: reserve RMRR mappings in e820

[Tiejun Chen]

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

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

diff --git a/tools/firmware/hvmloader/e820.c b/tools/firmware/hvmloader/e820.c
index 2e05e93..8cf8c75 100644
--- a/tools/firmware/hvmloader/e820.c
+++ b/tools/firmware/hvmloader/e820.c
@@ -74,6 +74,8 @@ int build_e820_table(struct e820entry *e820,
                      unsigned int bios_image_base)
 {
     unsigned int nr = 0;
+    struct e820map *e820_rmrr_map;
+    unsigned int i = 0;
 
     if ( !lowmem_reserved_base )
             lowmem_reserved_base = 0xA0000;
@@ -124,6 +126,18 @@ int build_e820_table(struct e820entry *e820,
     e820[nr].type = E820_RAM;
     nr++;
 
+    /* We'd better reserve RMRR mapping for each VM to avoid potential
+     * memory conflict.
+     */
+    e820_rmrr_map = get_rmrr_map_info();
+    for ( i = 0; i <= e820_rmrr_map->nr_map; i++ )
+    {
+        e820[nr].addr = e820_rmrr_map->map[i].addr;
+        e820[nr].size = e820_rmrr_map->map[i].size + 1;
+        e820[nr].type = E820_RESERVED;
+        nr++;
+    }
+
     /*
      * Explicitly reserve space for special pages.
      * This space starts at RESERVED_MEMBASE an extends to cover various
diff --git a/tools/firmware/hvmloader/e820.h b/tools/firmware/hvmloader/e820.h
index b2ead7f..ae4cc55 100644
--- a/tools/firmware/hvmloader/e820.h
+++ b/tools/firmware/hvmloader/e820.h
@@ -15,6 +15,12 @@ struct e820entry {
     uint32_t type;
 } __attribute__((packed));
 
+#define E820MAX 128
+
+struct e820map {
+    int nr_map;
+    struct e820entry map[E820MAX];
+};
 #endif /* __HVMLOADER_E820_H__ */
 
 /*
diff --git a/tools/firmware/hvmloader/util.c b/tools/firmware/hvmloader/util.c
index 80d822f..270700f 100644
--- a/tools/firmware/hvmloader/util.c
+++ b/tools/firmware/hvmloader/util.c
@@ -766,6 +766,19 @@ struct shared_info *get_shared_info(void)
     return shared_info;
 }
 
+struct e820map *get_rmrr_map_info(void)
+{
+    static struct e820map *e820_map = NULL;
+
+    if ( e820_map != NULL )
+        return e820_map;
+
+    if ( hypercall_memory_op(XENMEM_RMRR_memory_map, e820_map) != 0 )
+        BUG();
+
+    return e820_map;
+}
+
 uint16_t get_cpu_mhz(void)
 {
     struct shared_info *shared_info = get_shared_info();
diff --git a/tools/firmware/hvmloader/util.h b/tools/firmware/hvmloader/util.h
index a70e4aa..26bfb8c 100644
--- a/tools/firmware/hvmloader/util.h
+++ b/tools/firmware/hvmloader/util.h
@@ -236,6 +236,7 @@ unsigned long create_pir_tables(void);
 void smp_initialise(void);
 
 #include "e820.h"
+struct e820map *get_rmrr_map_info(void);
 int build_e820_table(struct e820entry *e820,
                      unsigned int lowmem_reserved_base,
                      unsigned int bios_image_base);
-- 
1.9.1

Re: [RFC][PATCH 4/5] tools:firmware:hvmloader: reserve RMRR mappings in e820

[Andrew Cooper]

On 07/08/14 12:02, Tiejun Chen wrote:
> We need to reserve all RMRR mappings in e820 to avoid any
> potential guest memory conflict.
>
> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
> ---
>  tools/firmware/hvmloader/e820.c | 14 ++++++++++++++
>  tools/firmware/hvmloader/e820.h |  6 ++++++
>  tools/firmware/hvmloader/util.c | 13 +++++++++++++
>  tools/firmware/hvmloader/util.h |  1 +
>  4 files changed, 34 insertions(+)
>
> diff --git a/tools/firmware/hvmloader/e820.c b/tools/firmware/hvmloader/e820.c
> index 2e05e93..8cf8c75 100644
> --- a/tools/firmware/hvmloader/e820.c
> +++ b/tools/firmware/hvmloader/e820.c
> @@ -74,6 +74,8 @@ int build_e820_table(struct e820entry *e820,
>                       unsigned int bios_image_base)
>  {
>      unsigned int nr = 0;
> +    struct e820map *e820_rmrr_map;
> +    unsigned int i = 0;
>  
>      if ( !lowmem_reserved_base )
>              lowmem_reserved_base = 0xA0000;
> @@ -124,6 +126,18 @@ int build_e820_table(struct e820entry *e820,
>      e820[nr].type = E820_RAM;
>      nr++;
>  
> +    /* We'd better reserve RMRR mapping for each VM to avoid potential
> +     * memory conflict.
> +     */
> +    e820_rmrr_map = get_rmrr_map_info();
> +    for ( i = 0; i <= e820_rmrr_map->nr_map; i++ )
> +    {
> +        e820[nr].addr = e820_rmrr_map->map[i].addr;
> +        e820[nr].size = e820_rmrr_map->map[i].size + 1;
> +        e820[nr].type = E820_RESERVED;
> +        nr++;
> +    }
> +
>      /*
>       * Explicitly reserve space for special pages.
>       * This space starts at RESERVED_MEMBASE an extends to cover various
> diff --git a/tools/firmware/hvmloader/e820.h b/tools/firmware/hvmloader/e820.h
> index b2ead7f..ae4cc55 100644
> --- a/tools/firmware/hvmloader/e820.h
> +++ b/tools/firmware/hvmloader/e820.h
> @@ -15,6 +15,12 @@ struct e820entry {
>      uint32_t type;
>  } __attribute__((packed));
>  
> +#define E820MAX 128
> +
> +struct e820map {
> +    int nr_map;

This value should be unsigned.

> +    struct e820entry map[E820MAX];
> +};
>  #endif /* __HVMLOADER_E820_H__ */
>  
>  /*
> diff --git a/tools/firmware/hvmloader/util.c b/tools/firmware/hvmloader/util.c
> index 80d822f..270700f 100644
> --- a/tools/firmware/hvmloader/util.c
> +++ b/tools/firmware/hvmloader/util.c
> @@ -766,6 +766,19 @@ struct shared_info *get_shared_info(void)
>      return shared_info;
>  }
>  
> +struct e820map *get_rmrr_map_info(void)
> +{
> +    static struct e820map *e820_map = NULL;
> +
> +    if ( e820_map != NULL )
> +        return e820_map;
> +
> +    if ( hypercall_memory_op(XENMEM_RMRR_memory_map, e820_map) != 0 )
> +        BUG();

This instructs Xen to clobber the memory starting at 0, and works
because HVMLoader is in protected, non-paging mode at this point.  I
don't think this is what you meant to do, and will repeatedly make the
hypercall rather than caching the result.

> +
> +    return e820_map;
> +}
> +
>  uint16_t get_cpu_mhz(void)
>  {
>      struct shared_info *shared_info = get_shared_info();
> diff --git a/tools/firmware/hvmloader/util.h b/tools/firmware/hvmloader/util.h
> index a70e4aa..26bfb8c 100644
> --- a/tools/firmware/hvmloader/util.h
> +++ b/tools/firmware/hvmloader/util.h
> @@ -236,6 +236,7 @@ unsigned long create_pir_tables(void);
>  void smp_initialise(void);
>  
>  #include "e820.h"
> +struct e820map *get_rmrr_map_info(void);
>  int build_e820_table(struct e820entry *e820,
>                       unsigned int lowmem_reserved_base,
>                       unsigned int bios_image_base);

Re: [RFC][PATCH 4/5] tools:firmware:hvmloader: reserve RMRR mappings in e820

[Chen, Tiejun]

On 2014/8/7 20:03, Andrew Cooper wrote:
> On 07/08/14 12:02, Tiejun Chen wrote:
>> We need to reserve all RMRR mappings in e820 to avoid any
>> potential guest memory conflict.
>>
>> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
>> ---
>>   tools/firmware/hvmloader/e820.c | 14 ++++++++++++++
>>   tools/firmware/hvmloader/e820.h |  6 ++++++
>>   tools/firmware/hvmloader/util.c | 13 +++++++++++++
>>   tools/firmware/hvmloader/util.h |  1 +
>>   4 files changed, 34 insertions(+)
>>
>> diff --git a/tools/firmware/hvmloader/e820.c b/tools/firmware/hvmloader/e820.c
>> index 2e05e93..8cf8c75 100644
>> --- a/tools/firmware/hvmloader/e820.c
>> +++ b/tools/firmware/hvmloader/e820.c
>> @@ -74,6 +74,8 @@ int build_e820_table(struct e820entry *e820,
>>                        unsigned int bios_image_base)
>>   {
>>       unsigned int nr = 0;
>> +    struct e820map *e820_rmrr_map;
>> +    unsigned int i = 0;
>>
>>       if ( !lowmem_reserved_base )
>>               lowmem_reserved_base = 0xA0000;
>> @@ -124,6 +126,18 @@ int build_e820_table(struct e820entry *e820,
>>       e820[nr].type = E820_RAM;
>>       nr++;
>>
>> +    /* We'd better reserve RMRR mapping for each VM to avoid potential
>> +     * memory conflict.
>> +     */
>> +    e820_rmrr_map = get_rmrr_map_info();
>> +    for ( i = 0; i <= e820_rmrr_map->nr_map; i++ )
>> +    {
>> +        e820[nr].addr = e820_rmrr_map->map[i].addr;
>> +        e820[nr].size = e820_rmrr_map->map[i].size + 1;
>> +        e820[nr].type = E820_RESERVED;
>> +        nr++;
>> +    }
>> +
>>       /*
>>        * Explicitly reserve space for special pages.
>>        * This space starts at RESERVED_MEMBASE an extends to cover various
>> diff --git a/tools/firmware/hvmloader/e820.h b/tools/firmware/hvmloader/e820.h
>> index b2ead7f..ae4cc55 100644
>> --- a/tools/firmware/hvmloader/e820.h
>> +++ b/tools/firmware/hvmloader/e820.h
>> @@ -15,6 +15,12 @@ struct e820entry {
>>       uint32_t type;
>>   } __attribute__((packed));
>>
>> +#define E820MAX 128
>> +
>> +struct e820map {
>> +    int nr_map;
>
> This value should be unsigned.

I'm not sure if we need to change this since here I just copy this from 
the xen/include/asm-x86/e820.h file,

struct e820map {
     int nr_map;
     struct e820entry map[E820MAX];
};

>
>> +    struct e820entry map[E820MAX];
>> +};
>>   #endif /* __HVMLOADER_E820_H__ */
>>
>>   /*
>> diff --git a/tools/firmware/hvmloader/util.c b/tools/firmware/hvmloader/util.c
>> index 80d822f..270700f 100644
>> --- a/tools/firmware/hvmloader/util.c
>> +++ b/tools/firmware/hvmloader/util.c
>> @@ -766,6 +766,19 @@ struct shared_info *get_shared_info(void)
>>       return shared_info;
>>   }
>>
>> +struct e820map *get_rmrr_map_info(void)
>> +{
>> +    static struct e820map *e820_map = NULL;
>> +
>> +    if ( e820_map != NULL )
>> +        return e820_map;
>> +
>> +    if ( hypercall_memory_op(XENMEM_RMRR_memory_map, e820_map) != 0 )
>> +        BUG();
>
> This instructs Xen to clobber the memory starting at 0, and works
> because HVMLoader is in protected, non-paging mode at this point.  I
> don't think this is what you meant to do, and will repeatedly make the

Sorry I can't understand this explicitly. Here I just want a way to get 
RMRR mapping info under hvmloader circumstance.

Could you elaborate what you mean? Or show me a proper way I should do 
as you expect.

Thanks
Tiejun

> hypercall rather than caching the result.
>
>> +
>> +    return e820_map;
>> +}
>> +
>>   uint16_t get_cpu_mhz(void)
>>   {
>>       struct shared_info *shared_info = get_shared_info();
>> diff --git a/tools/firmware/hvmloader/util.h b/tools/firmware/hvmloader/util.h
>> index a70e4aa..26bfb8c 100644
>> --- a/tools/firmware/hvmloader/util.h
>> +++ b/tools/firmware/hvmloader/util.h
>> @@ -236,6 +236,7 @@ unsigned long create_pir_tables(void);
>>   void smp_initialise(void);
>>
>>   #include "e820.h"
>> +struct e820map *get_rmrr_map_info(void);
>>   int build_e820_table(struct e820entry *e820,
>>                        unsigned int lowmem_reserved_base,
>>                        unsigned int bios_image_base);
>
>

Re: [RFC][PATCH 4/5] tools:firmware:hvmloader: reserve RMRR mappings in e820

[Jan Beulich]

>>> On 08.08.14 at 04:11, <tiejun.chen@intel.com> wrote:
> On 2014/8/7 20:03, Andrew Cooper wrote:
>> On 07/08/14 12:02, Tiejun Chen wrote:
>>> --- a/tools/firmware/hvmloader/e820.h
>>> +++ b/tools/firmware/hvmloader/e820.h
>>> @@ -15,6 +15,12 @@ struct e820entry {
>>>       uint32_t type;
>>>   } __attribute__((packed));
>>>
>>> +#define E820MAX 128
>>> +
>>> +struct e820map {
>>> +    int nr_map;
>>
>> This value should be unsigned.
> 
> I'm not sure if we need to change this since here I just copy this from 
> the xen/include/asm-x86/e820.h file,
> 
> struct e820map {
>      int nr_map;
>      struct e820entry map[E820MAX];
> };

While it be welcome for you to (in a separate patch) also change this
one, it is not considered okay to copy existing mistakes: We've been
slowly switching over to put more attention on correct signed-ness
(and const-ness) - variables/fields that can't ever be negative
shouldn't have signed types.

>>> --- a/tools/firmware/hvmloader/util.c
>>> +++ b/tools/firmware/hvmloader/util.c
>>> @@ -766,6 +766,19 @@ struct shared_info *get_shared_info(void)
>>>       return shared_info;
>>>   }
>>>
>>> +struct e820map *get_rmrr_map_info(void)
>>> +{
>>> +    static struct e820map *e820_map = NULL;
>>> +
>>> +    if ( e820_map != NULL )
>>> +        return e820_map;
>>> +
>>> +    if ( hypercall_memory_op(XENMEM_RMRR_memory_map, e820_map) != 0 )
>>> +        BUG();
>>
>> This instructs Xen to clobber the memory starting at 0, and works
>> because HVMLoader is in protected, non-paging mode at this point.  I
>> don't think this is what you meant to do, and will repeatedly make the
> 
> Sorry I can't understand this explicitly. Here I just want a way to get 
> RMRR mapping info under hvmloader circumstance.
> 
> Could you elaborate what you mean? Or show me a proper way I should do 
> as you expect.

You never allocate memory for the map, i.e. you invoke the
hypercall with a NULL second argument. This just happens to work,
but is very unlikely what you intended to do.

Jan

Re: [RFC][PATCH 4/5] tools:firmware:hvmloader: reserve RMRR mappings in e820

[Chen, Tiejun]

On 2014/8/8 14:42, Jan Beulich wrote:
>>>> On 08.08.14 at 04:11, <tiejun.chen@intel.com> wrote:
>> On 2014/8/7 20:03, Andrew Cooper wrote:
>>> On 07/08/14 12:02, Tiejun Chen wrote:
>>>> --- a/tools/firmware/hvmloader/e820.h
>>>> +++ b/tools/firmware/hvmloader/e820.h
>>>> @@ -15,6 +15,12 @@ struct e820entry {
>>>>        uint32_t type;
>>>>    } __attribute__((packed));
>>>>
>>>> +#define E820MAX 128
>>>> +
>>>> +struct e820map {
>>>> +    int nr_map;
>>>
>>> This value should be unsigned.
>>
>> I'm not sure if we need to change this since here I just copy this from
>> the xen/include/asm-x86/e820.h file,
>>
>> struct e820map {
>>       int nr_map;
>>       struct e820entry map[E820MAX];
>> };
>
> While it be welcome for you to (in a separate patch) also change this
> one, it is not considered okay to copy existing mistakes: We've been
> slowly switching over to put more attention on correct signed-ness
> (and const-ness) - variables/fields that can't ever be negative
> shouldn't have signed types.

I'm always afraid I'm missing something magic behind this signed type 
but with your clarification I already send one small patch to address 
this, and I also will update this point in this thread.

>
>>>> --- a/tools/firmware/hvmloader/util.c
>>>> +++ b/tools/firmware/hvmloader/util.c
>>>> @@ -766,6 +766,19 @@ struct shared_info *get_shared_info(void)
>>>>        return shared_info;
>>>>    }
>>>>
>>>> +struct e820map *get_rmrr_map_info(void)
>>>> +{
>>>> +    static struct e820map *e820_map = NULL;
>>>> +
>>>> +    if ( e820_map != NULL )
>>>> +        return e820_map;
>>>> +
>>>> +    if ( hypercall_memory_op(XENMEM_RMRR_memory_map, e820_map) != 0 )
>>>> +        BUG();
>>>
>>> This instructs Xen to clobber the memory starting at 0, and works
>>> because HVMLoader is in protected, non-paging mode at this point.  I
>>> don't think this is what you meant to do, and will repeatedly make the
>>
>> Sorry I can't understand this explicitly. Here I just want a way to get
>> RMRR mapping info under hvmloader circumstance.
>>
>> Could you elaborate what you mean? Or show me a proper way I should do
>> as you expect.
>
> You never allocate memory for the map, i.e. you invoke the
> hypercall with a NULL second argument. This just happens to work,
> but is very unlikely what you intended to do.
>

Looks scratch_alloc() should be used to allocate in hvmloader, so what 
about this?

diff --git a/tools/firmware/hvmloader/util.c 
b/tools/firmware/hvmloader/util.c
index 80d822f..90011fa 100644
--- a/tools/firmware/hvmloader/util.c
+++ b/tools/firmware/hvmloader/util.c
@@ -766,6 +766,16 @@ struct shared_info *get_shared_info(void)
      return shared_info;
  }

+struct e820map *get_rmrr_map_info(void)
+{
+    struct e820map *e820_map = scratch_alloc(sizeof(struct e820map), 0);
+
+    if ( hypercall_memory_op(XENMEM_RMRR_memory_map, e820_map) != 0 )
+        BUG();
+
+    return e820_map;
+}
+
  uint16_t get_cpu_mhz(void)
  {
      struct shared_info *shared_info = get_shared_info();

Thanks
Tiejun

Re: [RFC][PATCH 4/5] tools:firmware:hvmloader: reserve RMRR mappings in e820

[Jan Beulich]

>>> On 08.08.14 at 09:30, <tiejun.chen@intel.com> wrote:
> On 2014/8/8 14:42, Jan Beulich wrote:
>> You never allocate memory for the map, i.e. you invoke the
>> hypercall with a NULL second argument. This just happens to work,
>> but is very unlikely what you intended to do.
>>
> 
> Looks scratch_alloc() should be used to allocate in hvmloader, so what 
> about this?
> 
> --- a/tools/firmware/hvmloader/util.c
> +++ b/tools/firmware/hvmloader/util.c
> @@ -766,6 +766,16 @@ struct shared_info *get_shared_info(void)
>       return shared_info;
>   }
> 
> +struct e820map *get_rmrr_map_info(void)
> +{
> +    struct e820map *e820_map = scratch_alloc(sizeof(struct e820map), 0);
> +
> +    if ( hypercall_memory_op(XENMEM_RMRR_memory_map, e820_map) != 0 )
> +        BUG();
> +
> +    return e820_map;
> +}

More reasonable, albeit now you lost the fetch-just-once behavior.
Furthermore - do you really need this to be a dynamic allocation?
The structure you use right now is fixed size. Which gets me to
another point though: Is it said in any part of the VT-d spec that
there is an upper limit to the number of RMRRs? The re-use of the
E820 interface structure looks pretty odd here (I simply didn't get
to looking at your patches in full yet).

Jan

Re: [RFC][PATCH 4/5] tools:firmware:hvmloader: reserve RMRR mappings in e820

[Chen, Tiejun]

On 2014/8/8 15:43, Jan Beulich wrote:
>>>> On 08.08.14 at 09:30, <tiejun.chen@intel.com> wrote:
>> On 2014/8/8 14:42, Jan Beulich wrote:
>>> You never allocate memory for the map, i.e. you invoke the
>>> hypercall with a NULL second argument. This just happens to work,
>>> but is very unlikely what you intended to do.
>>>
>>
>> Looks scratch_alloc() should be used to allocate in hvmloader, so what
>> about this?
>>
>> --- a/tools/firmware/hvmloader/util.c
>> +++ b/tools/firmware/hvmloader/util.c
>> @@ -766,6 +766,16 @@ struct shared_info *get_shared_info(void)
>>        return shared_info;
>>    }
>>
>> +struct e820map *get_rmrr_map_info(void)
>> +{
>> +    struct e820map *e820_map = scratch_alloc(sizeof(struct e820map), 0);
>> +
>> +    if ( hypercall_memory_op(XENMEM_RMRR_memory_map, e820_map) != 0 )
>> +        BUG();
>> +
>> +    return e820_map;
>> +}
>
> More reasonable, albeit now you lost the fetch-just-once behavior.
> Furthermore - do you really need this to be a dynamic allocation?

So,

diff --git a/tools/firmware/hvmloader/util.c 
b/tools/firmware/hvmloader/util.c
index 80d822f..d55773e 100644
--- a/tools/firmware/hvmloader/util.c
+++ b/tools/firmware/hvmloader/util.c
@@ -766,6 +766,17 @@ struct shared_info *get_shared_info(void)
      return shared_info;
  }

+struct e820map *get_rmrr_map_info(void)
+{
+    if ( rmrr_e820map.nr_map )
+        return &rmrr_e820map;
+
+    if ( hypercall_memory_op(XENMEM_RMRR_memory_map, &rmrr_e820map) != 0 )
+        BUG();
+
+    return &rmrr_e820map;
+}
+
  uint16_t get_cpu_mhz(void)
  {
      struct shared_info *shared_info = get_shared_info();
diff --git a/tools/firmware/hvmloader/util.h 
b/tools/firmware/hvmloader/util.h
index a70e4aa..433be99 100644
--- a/tools/firmware/hvmloader/util.h
+++ b/tools/firmware/hvmloader/util.h
@@ -236,6 +236,8 @@ unsigned long create_pir_tables(void);
  void smp_initialise(void);

  #include "e820.h"
+struct e820map rmrr_e820map;
+struct e820map *get_rmrr_map_info(void);
  int build_e820_table(struct e820entry *e820,
                       unsigned int lowmem_reserved_base,
                       unsigned int bios_image_base);


> The structure you use right now is fixed size. Which gets me to
> another point though: Is it said in any part of the VT-d spec that
> there is an upper limit to the number of RMRRs? The re-use of the

After look up some relevant info in VT-D specs, I think we have no any 
direct field to limit the number of RMRRs. Just in 8.1 DMA Remapping 
Reporting Structure, there are some fields to be referred here:

#1 Length:

in bytes, of the description table including the length of the 
associated remapping structures.

#2 Remapping Structures[]:

A list of structures. The list will contain one or more DMA Remapping 
Hardware Unit Definition (DRHD) structures, and zero or more Reserved 
Memory Region Reporting (RMRR) and Root Port ATS Capability Reporting 
(ATSR) structures.

Seems no any explicit restriction.

Thanks
Tiejun

> E820 interface structure looks pretty odd here (I simply didn't get
> to looking at your patches in full yet).
>
> Jan
>
>
>

Re: [RFC][PATCH 4/5] tools:firmware:hvmloader: reserve RMRR mappings in e820

[Jan Beulich]

>>> On 08.08.14 at 10:39, <tiejun.chen@intel.com> wrote:
> On 2014/8/8 15:43, Jan Beulich wrote:
>> More reasonable, albeit now you lost the fetch-just-once behavior.
>> Furthermore - do you really need this to be a dynamic allocation?
> 
> So,
> 
> diff --git a/tools/firmware/hvmloader/util.c 
> b/tools/firmware/hvmloader/util.c
> index 80d822f..d55773e 100644
> --- a/tools/firmware/hvmloader/util.c
> +++ b/tools/firmware/hvmloader/util.c
> @@ -766,6 +766,17 @@ struct shared_info *get_shared_info(void)
>       return shared_info;
>   }
> 
> +struct e820map *get_rmrr_map_info(void)
> +{
> +    if ( rmrr_e820map.nr_map )
> +        return &rmrr_e820map;
> +
> +    if ( hypercall_memory_op(XENMEM_RMRR_memory_map, &rmrr_e820map) != 0 )
> +        BUG();
> +
> +    return &rmrr_e820map;
> +}

Still not quite, since now you still re-invoke the hypercall if the first
one didn't return any entries (e.g. on a VT-d-less system).

> --- a/tools/firmware/hvmloader/util.h
> +++ b/tools/firmware/hvmloader/util.h
> @@ -236,6 +236,8 @@ unsigned long create_pir_tables(void);
>   void smp_initialise(void);
> 
>   #include "e820.h"
> +struct e820map rmrr_e820map;
> +struct e820map *get_rmrr_map_info(void);
>   int build_e820_table(struct e820entry *e820,
>                        unsigned int lowmem_reserved_base,
>                        unsigned int bios_image_base);
> 
> 
>> The structure you use right now is fixed size. Which gets me to
>> another point though: Is it said in any part of the VT-d spec that
>> there is an upper limit to the number of RMRRs? The re-use of the
> 
> After look up some relevant info in VT-D specs, I think we have no any 
> direct field to limit the number of RMRRs. Just in 8.1 DMA Remapping 
> Reporting Structure, there are some fields to be referred here:
> 
> #1 Length:
> 
> in bytes, of the description table including the length of the 
> associated remapping structures.
> 
> #2 Remapping Structures[]:
> 
> A list of structures. The list will contain one or more DMA Remapping 
> Hardware Unit Definition (DRHD) structures, and zero or more Reserved 
> Memory Region Reporting (RMRR) and Root Port ATS Capability Reporting 
> (ATSR) structures.
> 
> Seems no any explicit restriction.

IOW a fixed-length interface structure isn't a proper fit here.

Jan

Re: [RFC][PATCH 4/5] tools:firmware:hvmloader: reserve RMRR mappings in e820

[Chen, Tiejun]

On 2014/8/8 17:01, Jan Beulich wrote:
>>>> On 08.08.14 at 10:39, <tiejun.chen@intel.com> wrote:
>> On 2014/8/8 15:43, Jan Beulich wrote:
>>> More reasonable, albeit now you lost the fetch-just-once behavior.
>>> Furthermore - do you really need this to be a dynamic allocation?
>>
>> So,
>>
>> diff --git a/tools/firmware/hvmloader/util.c
>> b/tools/firmware/hvmloader/util.c
>> index 80d822f..d55773e 100644
>> --- a/tools/firmware/hvmloader/util.c
>> +++ b/tools/firmware/hvmloader/util.c
>> @@ -766,6 +766,17 @@ struct shared_info *get_shared_info(void)
>>        return shared_info;
>>    }
>>
>> +struct e820map *get_rmrr_map_info(void)
>> +{
>> +    if ( rmrr_e820map.nr_map )
>> +        return &rmrr_e820map;
>> +
>> +    if ( hypercall_memory_op(XENMEM_RMRR_memory_map, &rmrr_e820map) != 0 )
>> +        BUG();
>> +
>> +    return &rmrr_e820map;
>> +}
>
> Still not quite, since now you still re-invoke the hypercall if the first
> one didn't return any entries (e.g. on a VT-d-less system).

I think we just should focus on if we already call that hypercall.

struct e820map *get_rmrr_map_info(void)
{
     static int no_rmrr = 1;

     if ( no_rmrr == 0 )
         return &rmrr_e820map;

     if ( hypercall_memory_op(XENMEM_RMRR_memory_map, &rmrr_e820map) != 0 )
         BUG();

     no_rmrr = 0;

     return &rmrr_e820map;
}

If rmrr_e820map is void, the subsequent codes always do nothing since 
rmrr_e820map.nr_map is zero.

>
>> --- a/tools/firmware/hvmloader/util.h
>> +++ b/tools/firmware/hvmloader/util.h
>> @@ -236,6 +236,8 @@ unsigned long create_pir_tables(void);
>>    void smp_initialise(void);
>>
>>    #include "e820.h"
>> +struct e820map rmrr_e820map;
>> +struct e820map *get_rmrr_map_info(void);
>>    int build_e820_table(struct e820entry *e820,
>>                         unsigned int lowmem_reserved_base,
>>                         unsigned int bios_image_base);
>>
>>
>>> The structure you use right now is fixed size. Which gets me to
>>> another point though: Is it said in any part of the VT-d spec that
>>> there is an upper limit to the number of RMRRs? The re-use of the
>>
>> After look up some relevant info in VT-D specs, I think we have no any
>> direct field to limit the number of RMRRs. Just in 8.1 DMA Remapping
>> Reporting Structure, there are some fields to be referred here:
>>
>> #1 Length:
>>
>> in bytes, of the description table including the length of the
>> associated remapping structures.
>>
>> #2 Remapping Structures[]:
>>
>> A list of structures. The list will contain one or more DMA Remapping
>> Hardware Unit Definition (DRHD) structures, and zero or more Reserved
>> Memory Region Reporting (RMRR) and Root Port ATS Capability Reporting
>> (ATSR) structures.
>>
>> Seems no any explicit restriction.
>
> IOW a fixed-length interface structure isn't a proper fit here.
>

Yes.

Thanks
Tiejun

[RFC][PATCH 4/5] tools:firmware:hvmloader: reserve RMRR mappings in e820

[Jan Beulich]

>>> On 07.08.14 at 13:02, <tiejun.chen@intel.com> wrote:
> We need to reserve all RMRR mappings in e820 to avoid any
> potential guest memory conflict.
> 
> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
> ---
>  tools/firmware/hvmloader/e820.c | 14 ++++++++++++++
>  tools/firmware/hvmloader/e820.h |  6 ++++++
>  tools/firmware/hvmloader/util.c | 13 +++++++++++++
>  tools/firmware/hvmloader/util.h |  1 +
>  4 files changed, 34 insertions(+)

> @@ -124,6 +126,18 @@ int build_e820_table(struct e820entry *e820,
>      e820[nr].type = E820_RAM;
>      nr++;
>  
> +    /* We'd better reserve RMRR mapping for each VM to avoid potential
> +     * memory conflict.
> +     */
> +    e820_rmrr_map = get_rmrr_map_info();
> +    for ( i = 0; i <= e820_rmrr_map->nr_map; i++ )
> +    {
> +        e820[nr].addr = e820_rmrr_map->map[i].addr;
> +        e820[nr].size = e820_rmrr_map->map[i].size + 1;
> +        e820[nr].type = E820_RESERVED;
> +        nr++;
> +    }

You can't just put this in an arbitrary place, without caring for overlaps
(and ordering perhaps - I don't recall offhand whether E820 is
required to be sorted, or whether that's just common practice). In fact
I'm not certain the code block following your insertion (interestingly
another Intel special) doesn't violate this too.

Jan

Re: [RFC][PATCH 4/5] tools:firmware:hvmloader: reserve RMRR mappings in e820

[Andrew Cooper]

On 08/08/14 16:53, Jan Beulich wrote:
>>>> On 07.08.14 at 13:02, <tiejun.chen@intel.com> wrote:
>> We need to reserve all RMRR mappings in e820 to avoid any
>> potential guest memory conflict.
>>
>> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
>> ---
>>  tools/firmware/hvmloader/e820.c | 14 ++++++++++++++
>>  tools/firmware/hvmloader/e820.h |  6 ++++++
>>  tools/firmware/hvmloader/util.c | 13 +++++++++++++
>>  tools/firmware/hvmloader/util.h |  1 +
>>  4 files changed, 34 insertions(+)
>> @@ -124,6 +126,18 @@ int build_e820_table(struct e820entry *e820,
>>      e820[nr].type = E820_RAM;
>>      nr++;
>>  
>> +    /* We'd better reserve RMRR mapping for each VM to avoid potential
>> +     * memory conflict.
>> +     */
>> +    e820_rmrr_map = get_rmrr_map_info();
>> +    for ( i = 0; i <= e820_rmrr_map->nr_map; i++ )
>> +    {
>> +        e820[nr].addr = e820_rmrr_map->map[i].addr;
>> +        e820[nr].size = e820_rmrr_map->map[i].size + 1;
>> +        e820[nr].type = E820_RESERVED;
>> +        nr++;
>> +    }
> You can't just put this in an arbitrary place, without caring for overlaps
> (and ordering perhaps - I don't recall offhand whether E820 is
> required to be sorted, or whether that's just common practice). In fact
> I'm not certain the code block following your insertion (interestingly
> another Intel special) doesn't violate this too.
>
> Jan

Whether it is required to be sorted or not, it is certainly the case
that there exist many BIOSes which present all manor of garbage in the
E820, including entries not aligned on page boundaries.

Any decent OS should be able to cope, but it is very bad form for
HVMLoader to present an E820 table like this.  It should be sorted and
lacking any overlapping regions.

~Andrew

Re: [RFC][PATCH 4/5] tools:firmware:hvmloader: reserve RMRR mappings in e820

[Jan Beulich]

>>> On 08.08.14 at 17:58, <andrew.cooper3@citrix.com> wrote:
> Whether it is required to be sorted or not, it is certainly the case
> that there exist many BIOSes which present all manor of garbage in the
> E820, including entries not aligned on page boundaries.

Non-page-aligned blocks are certainly fine for a BIOS to supply.

Jan

Re: [RFC][PATCH 4/5] tools:firmware:hvmloader: reserve RMRR mappings in e820

[Chen, Tiejun]

On 2014/8/8 23:53, Jan Beulich wrote:
>>>> On 07.08.14 at 13:02, <tiejun.chen@intel.com> wrote:
>> We need to reserve all RMRR mappings in e820 to avoid any
>> potential guest memory conflict.
>>
>> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
>> ---
>>   tools/firmware/hvmloader/e820.c | 14 ++++++++++++++
>>   tools/firmware/hvmloader/e820.h |  6 ++++++
>>   tools/firmware/hvmloader/util.c | 13 +++++++++++++
>>   tools/firmware/hvmloader/util.h |  1 +
>>   4 files changed, 34 insertions(+)
>
>> @@ -124,6 +126,18 @@ int build_e820_table(struct e820entry *e820,
>>       e820[nr].type = E820_RAM;
>>       nr++;
>>
>> +    /* We'd better reserve RMRR mapping for each VM to avoid potential
>> +     * memory conflict.
>> +     */
>> +    e820_rmrr_map = get_rmrr_map_info();
>> +    for ( i = 0; i <= e820_rmrr_map->nr_map; i++ )
>> +    {
>> +        e820[nr].addr = e820_rmrr_map->map[i].addr;
>> +        e820[nr].size = e820_rmrr_map->map[i].size + 1;
>> +        e820[nr].type = E820_RESERVED;
>> +        nr++;
>> +    }
>
> You can't just put this in an arbitrary place, without caring for overlaps
> (and ordering perhaps - I don't recall offhand whether E820 is
> required to be sorted, or whether that's just common practice). In fact
> I'm not certain the code block following your insertion (interestingly
> another Intel special) doesn't violate this too.

I will add some codes to sort nice in next revision.

Thanks
Tiejun

>
> Jan
>
>

Re: [RFC][PATCH 4/5] tools:firmware:hvmloader: reserve RMRR mappings in e820

[Tian, Kevin]

> From: Chen, Tiejun
> Sent: Thursday, August 07, 2014 4:03 AM
> 
> We need to reserve all RMRR mappings in e820 to avoid any
> potential guest memory conflict.

strictly speaking besides reserving in e820, you should also poke later
MMIO BAR allocations to avoid confliction too. Currently it's relative
to low_mem_pgend, which is likely to be different from host layout
so it's still possible to see a virtual MMIO bar base conflicting to the
RMRR ranges which are supposed to be sparse.

Thanks
Kevin

> 
> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
> ---
>  tools/firmware/hvmloader/e820.c | 14 ++++++++++++++
>  tools/firmware/hvmloader/e820.h |  6 ++++++
>  tools/firmware/hvmloader/util.c | 13 +++++++++++++
>  tools/firmware/hvmloader/util.h |  1 +
>  4 files changed, 34 insertions(+)
> 
> diff --git a/tools/firmware/hvmloader/e820.c
> b/tools/firmware/hvmloader/e820.c
> index 2e05e93..8cf8c75 100644
> --- a/tools/firmware/hvmloader/e820.c
> +++ b/tools/firmware/hvmloader/e820.c
> @@ -74,6 +74,8 @@ int build_e820_table(struct e820entry *e820,
>                       unsigned int bios_image_base)
>  {
>      unsigned int nr = 0;
> +    struct e820map *e820_rmrr_map;
> +    unsigned int i = 0;
> 
>      if ( !lowmem_reserved_base )
>              lowmem_reserved_base = 0xA0000;
> @@ -124,6 +126,18 @@ int build_e820_table(struct e820entry *e820,
>      e820[nr].type = E820_RAM;
>      nr++;
> 
> +    /* We'd better reserve RMRR mapping for each VM to avoid potential
> +     * memory conflict.
> +     */
> +    e820_rmrr_map = get_rmrr_map_info();
> +    for ( i = 0; i <= e820_rmrr_map->nr_map; i++ )
> +    {
> +        e820[nr].addr = e820_rmrr_map->map[i].addr;
> +        e820[nr].size = e820_rmrr_map->map[i].size + 1;
> +        e820[nr].type = E820_RESERVED;
> +        nr++;
> +    }
> +
>      /*
>       * Explicitly reserve space for special pages.
>       * This space starts at RESERVED_MEMBASE an extends to cover
> various
> diff --git a/tools/firmware/hvmloader/e820.h
> b/tools/firmware/hvmloader/e820.h
> index b2ead7f..ae4cc55 100644
> --- a/tools/firmware/hvmloader/e820.h
> +++ b/tools/firmware/hvmloader/e820.h
> @@ -15,6 +15,12 @@ struct e820entry {
>      uint32_t type;
>  } __attribute__((packed));
> 
> +#define E820MAX 128
> +
> +struct e820map {
> +    int nr_map;
> +    struct e820entry map[E820MAX];
> +};
>  #endif /* __HVMLOADER_E820_H__ */
> 
>  /*
> diff --git a/tools/firmware/hvmloader/util.c b/tools/firmware/hvmloader/util.c
> index 80d822f..270700f 100644
> --- a/tools/firmware/hvmloader/util.c
> +++ b/tools/firmware/hvmloader/util.c
> @@ -766,6 +766,19 @@ struct shared_info *get_shared_info(void)
>      return shared_info;
>  }
> 
> +struct e820map *get_rmrr_map_info(void)
> +{
> +    static struct e820map *e820_map = NULL;
> +
> +    if ( e820_map != NULL )
> +        return e820_map;
> +
> +    if ( hypercall_memory_op(XENMEM_RMRR_memory_map,
> e820_map) != 0 )
> +        BUG();
> +
> +    return e820_map;
> +}
> +
>  uint16_t get_cpu_mhz(void)
>  {
>      struct shared_info *shared_info = get_shared_info();
> diff --git a/tools/firmware/hvmloader/util.h
> b/tools/firmware/hvmloader/util.h
> index a70e4aa..26bfb8c 100644
> --- a/tools/firmware/hvmloader/util.h
> +++ b/tools/firmware/hvmloader/util.h
> @@ -236,6 +236,7 @@ unsigned long create_pir_tables(void);
>  void smp_initialise(void);
> 
>  #include "e820.h"
> +struct e820map *get_rmrr_map_info(void);
>  int build_e820_table(struct e820entry *e820,
>                       unsigned int lowmem_reserved_base,
>                       unsigned int bios_image_base);
> --
> 1.9.1

Re: [RFC][PATCH 4/5] tools:firmware:hvmloader: reserve RMRR mappings in e820

[Jan Beulich]

>>> On 08.08.14 at 23:47, <kevin.tian@intel.com> wrote:
>>  From: Chen, Tiejun
>> Sent: Thursday, August 07, 2014 4:03 AM
>> 
>> We need to reserve all RMRR mappings in e820 to avoid any
>> potential guest memory conflict.
> 
> strictly speaking besides reserving in e820, you should also poke later
> MMIO BAR allocations to avoid confliction too. Currently it's relative
> to low_mem_pgend, which is likely to be different from host layout
> so it's still possible to see a virtual MMIO bar base conflicting to the
> RMRR ranges which are supposed to be sparse.

Correct. And what's worse: Possible collisions between RMRRs and
the BIOS we place into the VM need to be taken care of, which may
turn out rather tricky.

Jan

Re: [RFC][PATCH 4/5] tools:firmware:hvmloader: reserve RMRR mappings in e820

[Tian, Kevin]

> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Sunday, August 10, 2014 11:53 PM
> 
> >>> On 08.08.14 at 23:47, <kevin.tian@intel.com> wrote:
> >>  From: Chen, Tiejun
> >> Sent: Thursday, August 07, 2014 4:03 AM
> >>
> >> We need to reserve all RMRR mappings in e820 to avoid any
> >> potential guest memory conflict.
> >
> > strictly speaking besides reserving in e820, you should also poke later
> > MMIO BAR allocations to avoid confliction too. Currently it's relative
> > to low_mem_pgend, which is likely to be different from host layout
> > so it's still possible to see a virtual MMIO bar base conflicting to the
> > RMRR ranges which are supposed to be sparse.
> 
> Correct. And what's worse: Possible collisions between RMRRs and
> the BIOS we place into the VM need to be taken care of, which may
> turn out rather tricky.
> 

right that becomes tricky. We can provide another hypercall to allow a 
VM tell Xen which RMRR can't be assigned due to confliction with gust
BIOS or other hvmloader allocation (if confliction can't be resolved).

If Xen detects a device owning RMRR is already assigned to the VM,
then fail the hypercall and hvmloader just panic with information to
indicate confliction.

Otherwise Xen records the information and future dynamic device
assignment like hotplug will be failed if associated RMRR will be in
the confliction list.

Thanks
Kevin

Re: [RFC][PATCH 4/5] tools:firmware:hvmloader: reserve RMRR mappings in e820

[Chen, Tiejun]

On 2014/8/12 0:00, Tian, Kevin wrote:
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: Sunday, August 10, 2014 11:53 PM
>>
>>>>> On 08.08.14 at 23:47, <kevin.tian@intel.com> wrote:
>>>>   From: Chen, Tiejun
>>>> Sent: Thursday, August 07, 2014 4:03 AM
>>>>
>>>> We need to reserve all RMRR mappings in e820 to avoid any
>>>> potential guest memory conflict.
>>>
>>> strictly speaking besides reserving in e820, you should also poke later
>>> MMIO BAR allocations to avoid confliction too. Currently it's relative
>>> to low_mem_pgend, which is likely to be different from host layout
>>> so it's still possible to see a virtual MMIO bar base conflicting to the
>>> RMRR ranges which are supposed to be sparse.
>>
>> Correct. And what's worse: Possible collisions between RMRRs and
>> the BIOS we place into the VM need to be taken care of, which may
>> turn out rather tricky.
>>
>
> right that becomes tricky. We can provide another hypercall to allow a
> VM tell Xen which RMRR can't be assigned due to confliction with gust
> BIOS or other hvmloader allocation (if confliction can't be resolved).
>
> If Xen detects a device owning RMRR is already assigned to the VM,
> then fail the hypercall and hvmloader just panic with information to
> indicate confliction.
>
> Otherwise Xen records the information and future dynamic device
> assignment like hotplug will be failed if associated RMRR will be in
> the confliction list.

 From my point of view its becoming over complicated.

In HVM case, theoretically any devices involving RMRR may be assigned to 
any given VM. So it may not be necessary to introduce such complex 
mechanism. Therefore, I think we can reserve all RMRR maps simply in 
e820, and check if MMIO is overlapping with RMRR for every VM. It should 
be acceptable.

Or please further comments in next revision, I can address your opinion 
again.

Thanks
Tiejun

>
> Thanks
> Kevin
>

Re: [RFC][PATCH 4/5] tools:firmware:hvmloader: reserve RMRR mappings in e820

[Jan Beulich]

>>> On 12.08.14 at 12:59, <tiejun.chen@intel.com> wrote:
> On 2014/8/12 0:00, Tian, Kevin wrote:
>>> From: Jan Beulich [mailto:JBeulich@suse.com]
>>> Sent: Sunday, August 10, 2014 11:53 PM
>>>>>> On 08.08.14 at 23:47, <kevin.tian@intel.com> wrote:
>>>> strictly speaking besides reserving in e820, you should also poke later
>>>> MMIO BAR allocations to avoid confliction too. Currently it's relative
>>>> to low_mem_pgend, which is likely to be different from host layout
>>>> so it's still possible to see a virtual MMIO bar base conflicting to the
>>>> RMRR ranges which are supposed to be sparse.
>>>
>>> Correct. And what's worse: Possible collisions between RMRRs and
>>> the BIOS we place into the VM need to be taken care of, which may
>>> turn out rather tricky.
>>>
>>
>> right that becomes tricky. We can provide another hypercall to allow a
>> VM tell Xen which RMRR can't be assigned due to confliction with gust
>> BIOS or other hvmloader allocation (if confliction can't be resolved).
>>
>> If Xen detects a device owning RMRR is already assigned to the VM,
>> then fail the hypercall and hvmloader just panic with information to
>> indicate confliction.
>>
>> Otherwise Xen records the information and future dynamic device
>> assignment like hotplug will be failed if associated RMRR will be in
>> the confliction list.
> 
>  From my point of view its becoming over complicated.
> 
> In HVM case, theoretically any devices involving RMRR may be assigned to 
> any given VM. So it may not be necessary to introduce such complex 
> mechanism. Therefore, I think we can reserve all RMRR maps simply in 
> e820, and check if MMIO is overlapping with RMRR for every VM. It should 
> be acceptable.

Then you didn't understand what Kevin and I said above. Just
keep in mind that the RMRRs can conflict not just with MMIO
ranges inside the guest, but also RAM ranges (which include, as
mentioned above, the range where the BIOS for the guest gets
put).

Jan

Re: [RFC][PATCH 4/5] tools:firmware:hvmloader: reserve RMRR mappings in e820

[Chen, Tiejun]

On 2014/8/12 20:25, Jan Beulich wrote:
>>>> On 12.08.14 at 12:59, <tiejun.chen@intel.com> wrote:
>> On 2014/8/12 0:00, Tian, Kevin wrote:
>>>> From: Jan Beulich [mailto:JBeulich@suse.com]
>>>> Sent: Sunday, August 10, 2014 11:53 PM
>>>>>>> On 08.08.14 at 23:47, <kevin.tian@intel.com> wrote:
>>>>> strictly speaking besides reserving in e820, you should also poke later
>>>>> MMIO BAR allocations to avoid confliction too. Currently it's relative
>>>>> to low_mem_pgend, which is likely to be different from host layout
>>>>> so it's still possible to see a virtual MMIO bar base conflicting to the
>>>>> RMRR ranges which are supposed to be sparse.
>>>>
>>>> Correct. And what's worse: Possible collisions between RMRRs and
>>>> the BIOS we place into the VM need to be taken care of, which may
>>>> turn out rather tricky.
>>>>
>>>
>>> right that becomes tricky. We can provide another hypercall to allow a
>>> VM tell Xen which RMRR can't be assigned due to confliction with gust
>>> BIOS or other hvmloader allocation (if confliction can't be resolved).
>>>
>>> If Xen detects a device owning RMRR is already assigned to the VM,
>>> then fail the hypercall and hvmloader just panic with information to
>>> indicate confliction.
>>>
>>> Otherwise Xen records the information and future dynamic device
>>> assignment like hotplug will be failed if associated RMRR will be in
>>> the confliction list.
>>
>>   From my point of view its becoming over complicated.
>>
>> In HVM case, theoretically any devices involving RMRR may be assigned to
>> any given VM. So it may not be necessary to introduce such complex
>> mechanism. Therefore, I think we can reserve all RMRR maps simply in
>> e820, and check if MMIO is overlapping with RMRR for every VM. It should
>> be acceptable.
>
> Then you didn't understand what Kevin and I said above. Just

I have to admit I'm poor in this coverage.

> keep in mind that the RMRRs can conflict not just with MMIO
> ranges inside the guest, but also RAM ranges (which include, as
> mentioned above, the range where the BIOS for the guest gets
> put).
>
> Jan
>

So just to clarify, as a summary there are four ranges we should be 
addressed:

#1 MMIO in guest

In my patch [RFC][v2][PATCH 5/6] tools:libxc: check if mmio BAR is out 
of RMRR mappings,

I will check if this is overlapping.

#2 RAM in guest

tools/firmware/hvmloader/e820.c:
     e820[nr].addr = 0x100000;
     e820[nr].size = (hvm_info->low_mem_pgend << PAGE_SHIFT) - 
e820[nr].addr;

#3 Guest BIOS itself

tools/firmware/hvmloader/e820.c:
     e820[nr].addr = bios_image_base;

For #2 and #3 in my patch [RFC][v2][PATCH 3/6] tools:firmware:hvmloader: 
reserve RMRR mappings in e820, we will check if RMRR is overlapping 
these ranges.

#4 Machine RAM range for a given guest

In this point I think RMRR already is as reserved in host e820, so its 
not possible to allocate any RMRR as physical RAM to a VM.

If I'm still misunderstanding please correct me.

Thanks
Tiejun

Re: [RFC][PATCH 4/5] tools:firmware:hvmloader: reserve RMRR mappings in e820

[Tian, Kevin]

> From: Chen, Tiejun
> Sent: Tuesday, August 12, 2014 5:57 PM
> 
> On 2014/8/12 20:25, Jan Beulich wrote:
> >>>> On 12.08.14 at 12:59, <tiejun.chen@intel.com> wrote:
> >> On 2014/8/12 0:00, Tian, Kevin wrote:
> >>>> From: Jan Beulich [mailto:JBeulich@suse.com]
> >>>> Sent: Sunday, August 10, 2014 11:53 PM
> >>>>>>> On 08.08.14 at 23:47, <kevin.tian@intel.com> wrote:
> >>>>> strictly speaking besides reserving in e820, you should also poke later
> >>>>> MMIO BAR allocations to avoid confliction too. Currently it's relative
> >>>>> to low_mem_pgend, which is likely to be different from host layout
> >>>>> so it's still possible to see a virtual MMIO bar base conflicting to the
> >>>>> RMRR ranges which are supposed to be sparse.
> >>>>
> >>>> Correct. And what's worse: Possible collisions between RMRRs and
> >>>> the BIOS we place into the VM need to be taken care of, which may
> >>>> turn out rather tricky.
> >>>>
> >>>
> >>> right that becomes tricky. We can provide another hypercall to allow a
> >>> VM tell Xen which RMRR can't be assigned due to confliction with gust
> >>> BIOS or other hvmloader allocation (if confliction can't be resolved).
> >>>
> >>> If Xen detects a device owning RMRR is already assigned to the VM,
> >>> then fail the hypercall and hvmloader just panic with information to
> >>> indicate confliction.
> >>>
> >>> Otherwise Xen records the information and future dynamic device
> >>> assignment like hotplug will be failed if associated RMRR will be in
> >>> the confliction list.
> >>
> >>   From my point of view its becoming over complicated.
> >>
> >> In HVM case, theoretically any devices involving RMRR may be assigned to
> >> any given VM. So it may not be necessary to introduce such complex
> >> mechanism. Therefore, I think we can reserve all RMRR maps simply in
> >> e820, and check if MMIO is overlapping with RMRR for every VM. It should
> >> be acceptable.
> >
> > Then you didn't understand what Kevin and I said above. Just
> 
> I have to admit I'm poor in this coverage.
> 
> > keep in mind that the RMRRs can conflict not just with MMIO
> > ranges inside the guest, but also RAM ranges (which include, as
> > mentioned above, the range where the BIOS for the guest gets
> > put).
> >
> > Jan
> >
> 
> So just to clarify, as a summary there are four ranges we should be
> addressed:
> 
> #1 MMIO in guest
> 
> In my patch [RFC][v2][PATCH 5/6] tools:libxc: check if mmio BAR is out
> of RMRR mappings,
> 
> I will check if this is overlapping.

hvmloader controls actual mmio BAR allocation, so it's important to have
check there. And your patch treats the whole mmio as one big region
to check overlapping with RMRR which is too coarse-grained. Better to check
overlapping every time when an allocation, either of memory ranges, or
MMIO ranges, actually happen.

> 
> #2 RAM in guest
> 
> tools/firmware/hvmloader/e820.c:
>      e820[nr].addr = 0x100000;
>      e820[nr].size = (hvm_info->low_mem_pgend << PAGE_SHIFT) -
> e820[nr].addr;

Note memory allocation actually happens in libxc, where when you see a
populate_physmap occurs it actually means a real allocation in guest 
physical address space. That's what you want to detect and avoid overlapping
in the 1st place. hvmloader builds e820 assuming the same policy as libxc
where it's the 2nd place to check (it's not a good design between hvmloader
and libxc, ideally the memory e820 ranges should be passed from libxc)

> 
> #3 Guest BIOS itself
> 
> tools/firmware/hvmloader/e820.c:
>      e820[nr].addr = bios_image_base;
> 
> For #2 and #3 in my patch [RFC][v2][PATCH 3/6] tools:firmware:hvmloader:
> reserve RMRR mappings in e820, we will check if RMRR is overlapping
> these ranges.
> 
> #4 Machine RAM range for a given guest
> 
> In this point I think RMRR already is as reserved in host e820, so its
> not possible to allocate any RMRR as physical RAM to a VM.

yes this is not the concern in this topic.

> 
> If I'm still misunderstanding please correct me.
> 
> Thanks
> Tiejun

Re: [RFC][PATCH 4/5] tools:firmware:hvmloader: reserve RMRR mappings in e820

[Chen, Tiejun]

On 2014/8/14 3:10, Tian, Kevin wrote:
>> From: Chen, Tiejun
>> Sent: Tuesday, August 12, 2014 5:57 PM
>>
>> On 2014/8/12 20:25, Jan Beulich wrote:
>>>>>> On 12.08.14 at 12:59, <tiejun.chen@intel.com> wrote:
>>>> On 2014/8/12 0:00, Tian, Kevin wrote:
>>>>>> From: Jan Beulich [mailto:JBeulich@suse.com]
>>>>>> Sent: Sunday, August 10, 2014 11:53 PM
>>>>>>>>> On 08.08.14 at 23:47, <kevin.tian@intel.com> wrote:
>>>>>>> strictly speaking besides reserving in e820, you should also poke later
>>>>>>> MMIO BAR allocations to avoid confliction too. Currently it's relative
>>>>>>> to low_mem_pgend, which is likely to be different from host layout
>>>>>>> so it's still possible to see a virtual MMIO bar base conflicting to the
>>>>>>> RMRR ranges which are supposed to be sparse.
>>>>>>
>>>>>> Correct. And what's worse: Possible collisions between RMRRs and
>>>>>> the BIOS we place into the VM need to be taken care of, which may
>>>>>> turn out rather tricky.
>>>>>>
>>>>>
>>>>> right that becomes tricky. We can provide another hypercall to allow a
>>>>> VM tell Xen which RMRR can't be assigned due to confliction with gust
>>>>> BIOS or other hvmloader allocation (if confliction can't be resolved).
>>>>>
>>>>> If Xen detects a device owning RMRR is already assigned to the VM,
>>>>> then fail the hypercall and hvmloader just panic with information to
>>>>> indicate confliction.
>>>>>
>>>>> Otherwise Xen records the information and future dynamic device
>>>>> assignment like hotplug will be failed if associated RMRR will be in
>>>>> the confliction list.
>>>>
>>>>    From my point of view its becoming over complicated.
>>>>
>>>> In HVM case, theoretically any devices involving RMRR may be assigned to
>>>> any given VM. So it may not be necessary to introduce such complex
>>>> mechanism. Therefore, I think we can reserve all RMRR maps simply in
>>>> e820, and check if MMIO is overlapping with RMRR for every VM. It should
>>>> be acceptable.
>>>
>>> Then you didn't understand what Kevin and I said above. Just
>>
>> I have to admit I'm poor in this coverage.
>>
>>> keep in mind that the RMRRs can conflict not just with MMIO
>>> ranges inside the guest, but also RAM ranges (which include, as
>>> mentioned above, the range where the BIOS for the guest gets
>>> put).
>>>
>>> Jan
>>>
>>
>> So just to clarify, as a summary there are four ranges we should be
>> addressed:
>>
>> #1 MMIO in guest
>>
>> In my patch [RFC][v2][PATCH 5/6] tools:libxc: check if mmio BAR is out
>> of RMRR mappings,
>>
>> I will check if this is overlapping.
>
> hvmloader controls actual mmio BAR allocation, so it's important to have

I guess you're saying pci_setup().

After setup_guest(), in pci_setup() we will reallocate mmio and ram if 
necessary and possible. Then all final info is reflected to fill into GS 
e820.

> check there. And your patch treats the whole mmio as one big region
> to check overlapping with RMRR which is too coarse-grained. Better to check

But its easy to feasible.

> overlapping every time when an allocation, either of memory ranges, or
> MMIO ranges, actually happen.

What is your policy to handle a conflict?

I mean those RMRR mapping entries are undermined and often they are not 
continuous. For example, IGD needs two entries in my current BDW,

#1 ab805000 ~ ab819000
#2 ad000000 ~ af800000

So if just one of them conflicts something, how to handle such a case? 
Push mmio out of RMRR? Or allow many mmio hole?  As you know IGD can't 
work as long as one of two entries is overlapping.

So I think it may not be necessary to handle this as complicated mechanism.

 From my point of view its enough to double check RMRR in GS e820 since 
just do check rather than check-to-fix. If any overlap occurs we will 
post WARNING/ERROR to notify the user, then let user decide what we 
should do next. If they know don't need any PCI passthrough its fine. 
And especially, actually RMRR should be rare.

>
>>
>> #2 RAM in guest
>>
>> tools/firmware/hvmloader/e820.c:
>>       e820[nr].addr = 0x100000;
>>       e820[nr].size = (hvm_info->low_mem_pgend << PAGE_SHIFT) -
>> e820[nr].addr;
>
> Note memory allocation actually happens in libxc, where when you see a
> populate_physmap occurs it actually means a real allocation in guest
> physical address space. That's what you want to detect and avoid overlapping
> in the 1st place. hvmloader builds e820 assuming the same policy as libxc
> where it's the 2nd place to check (it's not a good design between hvmloader
> and libxc, ideally the memory e820 ranges should be passed from libxc)
>

Same concern like the above.

Thanks
Tiejun

>>
>> #3 Guest BIOS itself
>>
>> tools/firmware/hvmloader/e820.c:
>>       e820[nr].addr = bios_image_base;
>>
>> For #2 and #3 in my patch [RFC][v2][PATCH 3/6] tools:firmware:hvmloader:
>> reserve RMRR mappings in e820, we will check if RMRR is overlapping
>> these ranges.
>>
>> #4 Machine RAM range for a given guest
>>
>> In this point I think RMRR already is as reserved in host e820, so its
>> not possible to allocate any RMRR as physical RAM to a VM.
>
> yes this is not the concern in this topic.
>
>>
>> If I'm still misunderstanding please correct me.
>>
>> Thanks
>> Tiejun
>

Re: [RFC][PATCH 4/5] tools:firmware:hvmloader: reserve RMRR mappings in e820

[Tian, Kevin]

> From: Chen, Tiejun
> Sent: Wednesday, August 13, 2014 8:03 PM
> 
> On 2014/8/14 3:10, Tian, Kevin wrote:
> >> From: Chen, Tiejun
> >> Sent: Tuesday, August 12, 2014 5:57 PM
> >>
> >> On 2014/8/12 20:25, Jan Beulich wrote:
> >>>>>> On 12.08.14 at 12:59, <tiejun.chen@intel.com> wrote:
> >>>> On 2014/8/12 0:00, Tian, Kevin wrote:
> >>>>>> From: Jan Beulich [mailto:JBeulich@suse.com]
> >>>>>> Sent: Sunday, August 10, 2014 11:53 PM
> >>>>>>>>> On 08.08.14 at 23:47, <kevin.tian@intel.com> wrote:
> >>>>>>> strictly speaking besides reserving in e820, you should also poke later
> >>>>>>> MMIO BAR allocations to avoid confliction too. Currently it's relative
> >>>>>>> to low_mem_pgend, which is likely to be different from host layout
> >>>>>>> so it's still possible to see a virtual MMIO bar base conflicting to the
> >>>>>>> RMRR ranges which are supposed to be sparse.
> >>>>>>
> >>>>>> Correct. And what's worse: Possible collisions between RMRRs and
> >>>>>> the BIOS we place into the VM need to be taken care of, which may
> >>>>>> turn out rather tricky.
> >>>>>>
> >>>>>
> >>>>> right that becomes tricky. We can provide another hypercall to allow a
> >>>>> VM tell Xen which RMRR can't be assigned due to confliction with gust
> >>>>> BIOS or other hvmloader allocation (if confliction can't be resolved).
> >>>>>
> >>>>> If Xen detects a device owning RMRR is already assigned to the VM,
> >>>>> then fail the hypercall and hvmloader just panic with information to
> >>>>> indicate confliction.
> >>>>>
> >>>>> Otherwise Xen records the information and future dynamic device
> >>>>> assignment like hotplug will be failed if associated RMRR will be in
> >>>>> the confliction list.
> >>>>
> >>>>    From my point of view its becoming over complicated.
> >>>>
> >>>> In HVM case, theoretically any devices involving RMRR may be assigned
> to
> >>>> any given VM. So it may not be necessary to introduce such complex
> >>>> mechanism. Therefore, I think we can reserve all RMRR maps simply in
> >>>> e820, and check if MMIO is overlapping with RMRR for every VM. It
> should
> >>>> be acceptable.
> >>>
> >>> Then you didn't understand what Kevin and I said above. Just
> >>
> >> I have to admit I'm poor in this coverage.
> >>
> >>> keep in mind that the RMRRs can conflict not just with MMIO
> >>> ranges inside the guest, but also RAM ranges (which include, as
> >>> mentioned above, the range where the BIOS for the guest gets
> >>> put).
> >>>
> >>> Jan
> >>>
> >>
> >> So just to clarify, as a summary there are four ranges we should be
> >> addressed:
> >>
> >> #1 MMIO in guest
> >>
> >> In my patch [RFC][v2][PATCH 5/6] tools:libxc: check if mmio BAR is out
> >> of RMRR mappings,
> >>
> >> I will check if this is overlapping.
> >
> > hvmloader controls actual mmio BAR allocation, so it's important to have
> 
> I guess you're saying pci_setup().
> 
> After setup_guest(), in pci_setup() we will reallocate mmio and ram if
> necessary and possible. Then all final info is reflected to fill into GS
> e820.
> 
> > check there. And your patch treats the whole mmio as one big region
> > to check overlapping with RMRR which is too coarse-grained. Better to
> check
> 
> But its easy to feasible.
> 
> > overlapping every time when an allocation, either of memory ranges, or
> > MMIO ranges, actually happen.
> 
> What is your policy to handle a conflict?
> 
> I mean those RMRR mapping entries are undermined and often they are not
> continuous. For example, IGD needs two entries in my current BDW,
> 
> #1 ab805000 ~ ab819000
> #2 ad000000 ~ af800000
> 
> So if just one of them conflicts something, how to handle such a case?
> Push mmio out of RMRR? Or allow many mmio hole?  As you know IGD can't
> work as long as one of two entries is overlapping.
> 
> So I think it may not be necessary to handle this as complicated mechanism.
> 
>  From my point of view its enough to double check RMRR in GS e820 since
> just do check rather than check-to-fix. If any overlap occurs we will
> post WARNING/ERROR to notify the user, then let user decide what we
> should do next. If they know don't need any PCI passthrough its fine.
> And especially, actually RMRR should be rare.
> 

I'm OK to do check-only w/o check-and-fix, at least it's a step forward
to fail-safe.

Thanks
Kevin

Re: [RFC][PATCH 4/5] tools:firmware:hvmloader: reserve RMRR mappings in e820

[Chen, Tiejun]

On 2014/8/15 7:11, Tian, Kevin wrote:
>> From: Chen, Tiejun
>> Sent: Wednesday, August 13, 2014 8:03 PM
>>
>> On 2014/8/14 3:10, Tian, Kevin wrote:
>>>> From: Chen, Tiejun
>>>> Sent: Tuesday, August 12, 2014 5:57 PM
>>>>
>>>> On 2014/8/12 20:25, Jan Beulich wrote:
>>>>>>>> On 12.08.14 at 12:59, <tiejun.chen@intel.com> wrote:
>>>>>> On 2014/8/12 0:00, Tian, Kevin wrote:
>>>>>>>> From: Jan Beulich [mailto:JBeulich@suse.com]
>>>>>>>> Sent: Sunday, August 10, 2014 11:53 PM
>>>>>>>>>>> On 08.08.14 at 23:47, <kevin.tian@intel.com> wrote:
>>>>>>>>> strictly speaking besides reserving in e820, you should also poke later
>>>>>>>>> MMIO BAR allocations to avoid confliction too. Currently it's relative
>>>>>>>>> to low_mem_pgend, which is likely to be different from host layout
>>>>>>>>> so it's still possible to see a virtual MMIO bar base conflicting to the
>>>>>>>>> RMRR ranges which are supposed to be sparse.
>>>>>>>>
>>>>>>>> Correct. And what's worse: Possible collisions between RMRRs and
>>>>>>>> the BIOS we place into the VM need to be taken care of, which may
>>>>>>>> turn out rather tricky.
>>>>>>>>
>>>>>>>
>>>>>>> right that becomes tricky. We can provide another hypercall to allow a
>>>>>>> VM tell Xen which RMRR can't be assigned due to confliction with gust
>>>>>>> BIOS or other hvmloader allocation (if confliction can't be resolved).
>>>>>>>
>>>>>>> If Xen detects a device owning RMRR is already assigned to the VM,
>>>>>>> then fail the hypercall and hvmloader just panic with information to
>>>>>>> indicate confliction.
>>>>>>>
>>>>>>> Otherwise Xen records the information and future dynamic device
>>>>>>> assignment like hotplug will be failed if associated RMRR will be in
>>>>>>> the confliction list.
>>>>>>
>>>>>>     From my point of view its becoming over complicated.
>>>>>>
>>>>>> In HVM case, theoretically any devices involving RMRR may be assigned
>> to
>>>>>> any given VM. So it may not be necessary to introduce such complex
>>>>>> mechanism. Therefore, I think we can reserve all RMRR maps simply in
>>>>>> e820, and check if MMIO is overlapping with RMRR for every VM. It
>> should
>>>>>> be acceptable.
>>>>>
>>>>> Then you didn't understand what Kevin and I said above. Just
>>>>
>>>> I have to admit I'm poor in this coverage.
>>>>
>>>>> keep in mind that the RMRRs can conflict not just with MMIO
>>>>> ranges inside the guest, but also RAM ranges (which include, as
>>>>> mentioned above, the range where the BIOS for the guest gets
>>>>> put).
>>>>>
>>>>> Jan
>>>>>
>>>>
>>>> So just to clarify, as a summary there are four ranges we should be
>>>> addressed:
>>>>
>>>> #1 MMIO in guest
>>>>
>>>> In my patch [RFC][v2][PATCH 5/6] tools:libxc: check if mmio BAR is out
>>>> of RMRR mappings,
>>>>
>>>> I will check if this is overlapping.
>>>
>>> hvmloader controls actual mmio BAR allocation, so it's important to have
>>
>> I guess you're saying pci_setup().
>>
>> After setup_guest(), in pci_setup() we will reallocate mmio and ram if
>> necessary and possible. Then all final info is reflected to fill into GS
>> e820.
>>
>>> check there. And your patch treats the whole mmio as one big region
>>> to check overlapping with RMRR which is too coarse-grained. Better to
>> check
>>
>> But its easy to feasible.
>>
>>> overlapping every time when an allocation, either of memory ranges, or
>>> MMIO ranges, actually happen.
>>
>> What is your policy to handle a conflict?
>>
>> I mean those RMRR mapping entries are undermined and often they are not
>> continuous. For example, IGD needs two entries in my current BDW,
>>
>> #1 ab805000 ~ ab819000
>> #2 ad000000 ~ af800000
>>
>> So if just one of them conflicts something, how to handle such a case?
>> Push mmio out of RMRR? Or allow many mmio hole?  As you know IGD can't
>> work as long as one of two entries is overlapping.
>>
>> So I think it may not be necessary to handle this as complicated mechanism.
>>
>>   From my point of view its enough to double check RMRR in GS e820 since
>> just do check rather than check-to-fix. If any overlap occurs we will
>> post WARNING/ERROR to notify the user, then let user decide what we
>> should do next. If they know don't need any PCI passthrough its fine.
>> And especially, actually RMRR should be rare.
>>
>
> I'm OK to do check-only w/o check-and-fix, at least it's a step forward
> to fail-safe.

Thanks a lot.

BTW, looks Xen have many known places, even bug, we need to clean or 
improve, right? So why don't we have an explicit plan to push this step 
by step? I mean at least we can document these somewhere like this,

#1 Notice some known problems
#2 Any useful discussion
#2 Workaround if possible
#3 Next step or plan

Its convenient to track them. Once one guy meet this again, he can find 
enough information to know how to deal with this.

Thanks
Tiejun

>
> Thanks
> Kevin
>

Re: [RFC][PATCH 4/5] tools:firmware:hvmloader: reserve RMRR mappings in e820

[Chen, Tiejun]

On 2014/8/11 14:53, Jan Beulich wrote:
>>>> On 08.08.14 at 23:47, <kevin.tian@intel.com> wrote:
>>>   From: Chen, Tiejun
>>> Sent: Thursday, August 07, 2014 4:03 AM
>>>
>>> We need to reserve all RMRR mappings in e820 to avoid any
>>> potential guest memory conflict.
>>
>> strictly speaking besides reserving in e820, you should also poke later
>> MMIO BAR allocations to avoid confliction too. Currently it's relative
>> to low_mem_pgend, which is likely to be different from host layout
>> so it's still possible to see a virtual MMIO bar base conflicting to the
>> RMRR ranges which are supposed to be sparse.
>
> Correct. And what's worse: Possible collisions between RMRRs and
> the BIOS we place into the VM need to be taken care of, which may
> turn out rather tricky.

Looks BIOS itself ranges is covered with a e820 entry, so I think the 
codes to sort all entries can check this thing as well.

Thanks
Tiejun

>
> Jan
>
>
>

Re: [RFC][PATCH 4/5] tools:firmware:hvmloader: reserve RMRR mappings in e820

[Jan Beulich]

>>> On 12.08.14 at 12:56, <tiejun.chen@intel.com> wrote:
> On 2014/8/11 14:53, Jan Beulich wrote:
>>>>> On 08.08.14 at 23:47, <kevin.tian@intel.com> wrote:
>>>>   From: Chen, Tiejun
>>>> Sent: Thursday, August 07, 2014 4:03 AM
>>>>
>>>> We need to reserve all RMRR mappings in e820 to avoid any
>>>> potential guest memory conflict.
>>>
>>> strictly speaking besides reserving in e820, you should also poke later
>>> MMIO BAR allocations to avoid confliction too. Currently it's relative
>>> to low_mem_pgend, which is likely to be different from host layout
>>> so it's still possible to see a virtual MMIO bar base conflicting to the
>>> RMRR ranges which are supposed to be sparse.
>>
>> Correct. And what's worse: Possible collisions between RMRRs and
>> the BIOS we place into the VM need to be taken care of, which may
>> turn out rather tricky.
> 
> Looks BIOS itself ranges is covered with a e820 entry, so I think the 
> codes to sort all entries can check this thing as well.

Hmm, just like said in the reply sent to patch 3 a minute ago -
please don't mix up the host's and guest's E820 maps.

Jan

Re: [RFC][PATCH 4/5] tools:firmware:hvmloader: reserve RMRR mappings in e820

[Chen, Tiejun]

On 2014/8/9 5:47, Tian, Kevin wrote:
>> From: Chen, Tiejun
>> Sent: Thursday, August 07, 2014 4:03 AM
>>
>> We need to reserve all RMRR mappings in e820 to avoid any
>> potential guest memory conflict.
>
> strictly speaking besides reserving in e820, you should also poke later
> MMIO BAR allocations to avoid confliction too. Currently it's relative
> to low_mem_pgend, which is likely to be different from host layout

low_mem_pgend is already in e820 so this is checked as well when we try 
to insert RMRR maps into GS e820.

Thanks
Tiejun

> so it's still possible to see a virtual MMIO bar base conflicting to the
> RMRR ranges which are supposed to be sparse.
>
> Thanks
> Kevin
>
>>
>> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
>> ---
>>   tools/firmware/hvmloader/e820.c | 14 ++++++++++++++
>>   tools/firmware/hvmloader/e820.h |  6 ++++++
>>   tools/firmware/hvmloader/util.c | 13 +++++++++++++
>>   tools/firmware/hvmloader/util.h |  1 +
>>   4 files changed, 34 insertions(+)
>>
>> diff --git a/tools/firmware/hvmloader/e820.c
>> b/tools/firmware/hvmloader/e820.c
>> index 2e05e93..8cf8c75 100644
>> --- a/tools/firmware/hvmloader/e820.c
>> +++ b/tools/firmware/hvmloader/e820.c
>> @@ -74,6 +74,8 @@ int build_e820_table(struct e820entry *e820,
>>                        unsigned int bios_image_base)
>>   {
>>       unsigned int nr = 0;
>> +    struct e820map *e820_rmrr_map;
>> +    unsigned int i = 0;
>>
>>       if ( !lowmem_reserved_base )
>>               lowmem_reserved_base = 0xA0000;
>> @@ -124,6 +126,18 @@ int build_e820_table(struct e820entry *e820,
>>       e820[nr].type = E820_RAM;
>>       nr++;
>>
>> +    /* We'd better reserve RMRR mapping for each VM to avoid potential
>> +     * memory conflict.
>> +     */
>> +    e820_rmrr_map = get_rmrr_map_info();
>> +    for ( i = 0; i <= e820_rmrr_map->nr_map; i++ )
>> +    {
>> +        e820[nr].addr = e820_rmrr_map->map[i].addr;
>> +        e820[nr].size = e820_rmrr_map->map[i].size + 1;
>> +        e820[nr].type = E820_RESERVED;
>> +        nr++;
>> +    }
>> +
>>       /*
>>        * Explicitly reserve space for special pages.
>>        * This space starts at RESERVED_MEMBASE an extends to cover
>> various
>> diff --git a/tools/firmware/hvmloader/e820.h
>> b/tools/firmware/hvmloader/e820.h
>> index b2ead7f..ae4cc55 100644
>> --- a/tools/firmware/hvmloader/e820.h
>> +++ b/tools/firmware/hvmloader/e820.h
>> @@ -15,6 +15,12 @@ struct e820entry {
>>       uint32_t type;
>>   } __attribute__((packed));
>>
>> +#define E820MAX 128
>> +
>> +struct e820map {
>> +    int nr_map;
>> +    struct e820entry map[E820MAX];
>> +};
>>   #endif /* __HVMLOADER_E820_H__ */
>>
>>   /*
>> diff --git a/tools/firmware/hvmloader/util.c b/tools/firmware/hvmloader/util.c
>> index 80d822f..270700f 100644
>> --- a/tools/firmware/hvmloader/util.c
>> +++ b/tools/firmware/hvmloader/util.c
>> @@ -766,6 +766,19 @@ struct shared_info *get_shared_info(void)
>>       return shared_info;
>>   }
>>
>> +struct e820map *get_rmrr_map_info(void)
>> +{
>> +    static struct e820map *e820_map = NULL;
>> +
>> +    if ( e820_map != NULL )
>> +        return e820_map;
>> +
>> +    if ( hypercall_memory_op(XENMEM_RMRR_memory_map,
>> e820_map) != 0 )
>> +        BUG();
>> +
>> +    return e820_map;
>> +}
>> +
>>   uint16_t get_cpu_mhz(void)
>>   {
>>       struct shared_info *shared_info = get_shared_info();
>> diff --git a/tools/firmware/hvmloader/util.h
>> b/tools/firmware/hvmloader/util.h
>> index a70e4aa..26bfb8c 100644
>> --- a/tools/firmware/hvmloader/util.h
>> +++ b/tools/firmware/hvmloader/util.h
>> @@ -236,6 +236,7 @@ unsigned long create_pir_tables(void);
>>   void smp_initialise(void);
>>
>>   #include "e820.h"
>> +struct e820map *get_rmrr_map_info(void);
>>   int build_e820_table(struct e820entry *e820,
>>                        unsigned int lowmem_reserved_base,
>>                        unsigned int bios_image_base);
>> --
>> 1.9.1
>
>

[RFC][PATCH 5/5] 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