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

[Andrew Cooper <andrew.cooper3@citrix.com>]

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

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

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

~Andrew