Re: [v3][PATCH 06/16] hvmloader: get guest memory map into memory_map[]

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

On 2015/6/11 17:38, Tian, Kevin wrote:
>> From: Chen, Tiejun
>> Sent: Thursday, June 11, 2015 9:15 AM
>>
>> Now we get this map layout by call XENMEM_memory_map then
>> save them into one global variable memory_map[]. It should
>> include lowmem range, rdm range and highmem range. Note
>> rdm range and highmem range may not exist in some cases.
>>
>> And here we need to check if any reserved memory conflicts with
>> [RESERVED_MEMORY_DYNAMIC_START - 1, RESERVED_MEMORY_DYNAMIC_END].
>> This range is used to allocate memory in hvmloder level, and
>> we would lead hvmloader failed in case of conflict since its
>> another rare possibility in real world.
>>
>> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
>> ---
>>   tools/firmware/hvmloader/e820.h      |  7 +++++++
>>   tools/firmware/hvmloader/hvmloader.c | 37
>> ++++++++++++++++++++++++++++++++++++
>>   tools/firmware/hvmloader/util.c      | 26 +++++++++++++++++++++++++
>>   tools/firmware/hvmloader/util.h      | 11 +++++++++++
>>   4 files changed, 81 insertions(+)
>>
>> diff --git a/tools/firmware/hvmloader/e820.h b/tools/firmware/hvmloader/e820.h
>> index b2ead7f..8b5a9e0 100644
>> --- a/tools/firmware/hvmloader/e820.h
>> +++ b/tools/firmware/hvmloader/e820.h
>> @@ -15,6 +15,13 @@ struct e820entry {
>>       uint32_t type;
>>   } __attribute__((packed));
>>
>> +#define E820MAX	128
>> +
>> +struct e820map {
>> +    unsigned int nr_map;
>> +    struct e820entry map[E820MAX];
>> +};
>> +
>>   #endif /* __HVMLOADER_E820_H__ */
>>
>>   /*
>> diff --git a/tools/firmware/hvmloader/hvmloader.c
>> b/tools/firmware/hvmloader/hvmloader.c
>> index 25b7f08..c9f170e 100644
>> --- a/tools/firmware/hvmloader/hvmloader.c
>> +++ b/tools/firmware/hvmloader/hvmloader.c
>> @@ -107,6 +107,8 @@ asm (
>>       "    .text                       \n"
>>       );
>>
>> +struct e820map memory_map;
>> +
>>   unsigned long scratch_start = SCRATCH_PHYSICAL_ADDRESS;
>>
>>   static void init_hypercalls(void)
>> @@ -199,6 +201,39 @@ static void apic_setup(void)
>>       ioapic_write(0x11, SET_APIC_ID(LAPIC_ID(0)));
>>   }
>>
>> +void memory_map_setup(void)
>> +{
>> +    unsigned int nr_entries = E820MAX, i;
>> +    int rc;
>> +    uint64_t alloc_addr = RESERVED_MEMORY_DYNAMIC_START - 1;
>> +    uint64_t alloc_size = RESERVED_MEMORY_DYNAMIC_END - alloc_addr;
>> +
>> +    rc = get_mem_mapping_layout(memory_map.map, &nr_entries);
>> +
>> +    if ( rc )
>> +    {
>> +        printf("Failed to get guest memory map.\n");
>> +        BUG();
>> +    }
>> +
>> +    BUG_ON(!nr_entries);
>> +    memory_map.nr_map = nr_entries;
>> +
>> +    for ( i = 0; i < nr_entries; i++ )
>> +    {
>> +        if ( memory_map.map[i].type == E820_RESERVED )
>> +        {
>> +            if ( check_overlap(alloc_addr, alloc_size,
>> +                               memory_map.map[i].addr,
>> +                               memory_map.map[i].size) )
>> +            {
>> +                printf("RDM conflicts Memory allocation.\n");
>
> hvmloader has no concept of RDM here. It's just E820_RESERVED
> type. Please make the error message clear, e.g. "Fail to setup
> memory map due to conflict on dynamic reserved memory range."
>

Okay,

+            {
+                printf("Fail to setup memory map due to conflict");
+                printf(" on dynamic reserved memory range.\n");
+                BUG();
+            }


> Otherwise:
>
> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
>

Thanks
Tiejun