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

["Chen, Tiejun" <tiejun.chen@intel.com>]

On 2014/8/18 17:51, Andrew Cooper wrote:
> 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,

As you know all RMRR can be listed in a e820 table so its not possible 
to be over 128.

> or 128.  If this function is called more than 128 times, you will start
> clobbering Xen data.

And this is just a static function here, we also don't call that with >128.

Anyway, I can add something to deliver warning message like this,

+
+    if ( rmrr_map.nr_map >= E820MAX )
+        printk(XENLOG_WARNING
+               "Overflow RMRR mappings! Failed to record RMRR 
mappings.\n");

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

I'm a little bit disagreed here. Please see my reply to Jan.

Thanks
Tiejun
>
> ~Andrew
>
>