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

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

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.

> than 128 times, or you need to do dynamic allocation of the storage.
>
>> +    rmrr_maps.map[rmrr_maps.nr_map].addr = rmrr->base_address;
>> +    rmrr_maps.map[rmrr_maps.nr_map].size = rmrr->end_address -
>> +                                           rmrr->base_address;
>> +    rmrr_maps.map[rmrr_maps.nr_map].type = E820_RESERVED;
>> +    rmrr_maps.nr_map++;
>> +
>> +    return 0;
>> +}
>> +
>>   static void __init disable_all_dmar_units(void)
>>   {
>>       struct acpi_drhd_unit *drhd, *_drhd;
>> @@ -675,6 +688,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_maps_register(rmrru);
>>           }
>>       }
>>
>> diff --git a/xen/include/asm-x86/e820.h b/xen/include/asm-x86/e820.h
>> index 71a804c..74262c9 100644
>> --- a/xen/include/asm-x86/e820.h
>> +++ b/xen/include/asm-x86/e820.h
>> @@ -23,6 +23,8 @@ struct e820map {
>>       struct e820entry map[E820MAX];
>>   };
>>
>> +typedef struct e820map rmrr_maps_t;
>
> This type is a single map of RMRR regions, not multiple maps.
> rmrr_map_t please.

Okay.

Thanks
Tiejun

>
> ~Andrew
>
>> +
>>   extern int e820_all_mapped(u64 start, u64 end, unsigned type);
>>   extern int reserve_e820_ram(struct e820map *e820, uint64_t s, uint64_t e);
>>   extern int e820_change_range_type(
>> @@ -32,6 +34,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 rmrr_maps_t rmrr_maps;
>>
>>   /* These symbols live in the boot trampoline. */
>>   extern struct e820entry e820map[];
>
>