Re: [v7][PATCH 05/16] hvmloader: get guest memory map into memory_map[]

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

On 2015/7/10 21:49, George Dunlap wrote:
> On Thu, Jul 9, 2015 at 6:33 AM, Tiejun Chen <tiejun.chen@intel.com> wrote:
>> 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.
>>
>> CC: Keir Fraser <keir@xen.org>
>> CC: Jan Beulich <jbeulich@suse.com>
>> CC: Andrew Cooper <andrew.cooper3@citrix.com>
>> CC: Ian Jackson <ian.jackson@eu.citrix.com>
>> CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>> CC: Ian Campbell <ian.campbell@citrix.com>
>> CC: Wei Liu <wei.liu2@citrix.com>
>> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
>> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
>> ---
>> v5 ~ v7:
>>
>> * Nothing is changed.
>>
>> v4:
>>
>> * Move some codes related to e820 to that specific file, e820.c.
>>
>> * Consolidate "printf()+BUG()" and "BUG_ON()"
>>
>> * Avoid another fixed width type for the parameter of get_mem_mapping_layout()
>>
>>   tools/firmware/hvmloader/e820.c      | 35 +++++++++++++++++++++++++++++++++++
>>   tools/firmware/hvmloader/e820.h      |  7 +++++++
>>   tools/firmware/hvmloader/hvmloader.c |  2 ++
>>   tools/firmware/hvmloader/util.c      | 26 ++++++++++++++++++++++++++
>>   tools/firmware/hvmloader/util.h      | 12 ++++++++++++
>>   5 files changed, 82 insertions(+)
>>
>> diff --git a/tools/firmware/hvmloader/e820.c b/tools/firmware/hvmloader/e820.c
>> index 2e05e93..3e53c47 100644
>> --- a/tools/firmware/hvmloader/e820.c
>> +++ b/tools/firmware/hvmloader/e820.c
>> @@ -23,6 +23,41 @@
>>   #include "config.h"
>>   #include "util.h"
>>
>> +struct e820map memory_map;
>> +
>> +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;
>
> Why START-1 rather than just START?

I also think this is wrong after I double check this point. This two 
lines seems be copied simply from another place where we're allocating 
space based on RESERVED_MEMORY_DYNAMIC_{START, END}. But here I think 
you're right.

So let me correct this and update the patch description.

Thanks
Tiejun

>
> It looks like RESERVED_MEMORY_DYNAMIC_START is set to 0xFC001000.  In
> the code the way it is, if there is an RMRR from 0xFC000000 of size
> 0x1000, it looks like check_overlap() below will fail and hvmloader
> will BUG().
>
> Is that really what we want?  Why can we not have an RMRR range that
> goes right up to the edge of the reserved range?
>
> Other than that this patch looks good.
>
>   -George
>
>> +
>> +    rc = get_mem_mapping_layout(memory_map.map, &nr_entries);
>> +
>> +    if ( rc || !nr_entries )
>> +    {
>> +        printf("Get guest memory maps[%d] failed. (%d)\n", nr_entries, rc);
>> +        BUG();
>> +    }
>> +
>> +    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("Fail to setup memory map due to conflict");
>> +                printf(" on dynamic reserved memory range.\n");
>> +                BUG();
>> +            }
>> +        }
>> +    }
>> +}
>> +
>>   void dump_e820_table(struct e820entry *e820, unsigned int nr)
>>   {
>>       uint64_t last_end = 0, start, end;
>> 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..84c588c 100644
>> --- a/tools/firmware/hvmloader/hvmloader.c
>> +++ b/tools/firmware/hvmloader/hvmloader.c
>> @@ -262,6 +262,8 @@ int main(void)
>>
>>       init_hypercalls();
>>
>> +    memory_map_setup();
>> +
>>       xenbus_setup();
>>
>>       bios = detect_bios();
>> diff --git a/tools/firmware/hvmloader/util.c b/tools/firmware/hvmloader/util.c
>> index 80d822f..122e3fa 100644
>> --- a/tools/firmware/hvmloader/util.c
>> +++ b/tools/firmware/hvmloader/util.c
>> @@ -27,6 +27,17 @@
>>   #include <xen/memory.h>
>>   #include <xen/sched.h>
>>
>> +/*
>> + * Check whether there exists overlap in the specified memory range.
>> + * Returns true if exists, else returns false.
>> + */
>> +bool check_overlap(uint64_t start, uint64_t size,
>> +                   uint64_t reserved_start, uint64_t reserved_size)
>> +{
>> +    return (start + size > reserved_start) &&
>> +            (start < reserved_start + reserved_size);
>> +}
>> +
>>   void wrmsr(uint32_t idx, uint64_t v)
>>   {
>>       asm volatile (
>> @@ -368,6 +379,21 @@ uuid_to_string(char *dest, uint8_t *uuid)
>>       *p = '\0';
>>   }
>>
>> +int get_mem_mapping_layout(struct e820entry entries[], uint32_t *max_entries)
>> +{
>> +    int rc;
>> +    struct xen_memory_map memmap = {
>> +        .nr_entries = *max_entries
>> +    };
>> +
>> +    set_xen_guest_handle(memmap.buffer, entries);
>> +
>> +    rc = hypercall_memory_op(XENMEM_memory_map, &memmap);
>> +    *max_entries = memmap.nr_entries;
>> +
>> +    return rc;
>> +}
>> +
>>   void mem_hole_populate_ram(xen_pfn_t mfn, uint32_t nr_mfns)
>>   {
>>       static int over_allocated;
>> diff --git a/tools/firmware/hvmloader/util.h b/tools/firmware/hvmloader/util.h
>> index f99c0f19..1100a3b 100644
>> --- a/tools/firmware/hvmloader/util.h
>> +++ b/tools/firmware/hvmloader/util.h
>> @@ -4,8 +4,10 @@
>>   #include <stdarg.h>
>>   #include <stdint.h>
>>   #include <stddef.h>
>> +#include <stdbool.h>
>>   #include <xen/xen.h>
>>   #include <xen/hvm/hvm_info_table.h>
>> +#include "e820.h"
>>
>>   #define __STR(...) #__VA_ARGS__
>>   #define STR(...) __STR(__VA_ARGS__)
>> @@ -222,6 +224,9 @@ int hvm_param_set(uint32_t index, uint64_t value);
>>   /* Setup PCI bus */
>>   void pci_setup(void);
>>
>> +/* Setup memory map  */
>> +void memory_map_setup(void);
>> +
>>   /* Prepare the 32bit BIOS */
>>   uint32_t rombios_highbios_setup(void);
>>
>> @@ -249,6 +254,13 @@ void perform_tests(void);
>>
>>   extern char _start[], _end[];
>>
>> +int get_mem_mapping_layout(struct e820entry entries[],
>> +                           unsigned int *max_entries);
>> +
>> +extern struct e820map memory_map;
>> +bool check_overlap(uint64_t start, uint64_t size,
>> +                   uint64_t reserved_start, uint64_t reserved_size);
>> +
>>   #endif /* __HVMLOADER_UTIL_H__ */
>>
>>   /*
>> --
>> 1.9.1
>>
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org
>> http://lists.xen.org/xen-devel
>