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

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

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
>
>