From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Chen, Tiejun" Subject: Re: [v7][PATCH 05/16] hvmloader: get guest memory map into memory_map[] Date: Mon, 13 Jul 2015 15:03:16 +0800 Message-ID: <55A362B4.4020408@intel.com> References: <1436420047-25356-1-git-send-email-tiejun.chen@intel.com> <1436420047-25356-6-git-send-email-tiejun.chen@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: George Dunlap Cc: Keir Fraser , Ian Campbell , Stefano Stabellini , Andrew Cooper , Ian Jackson , "xen-devel@lists.xen.org" , Jan Beulich , Wei Liu List-Id: xen-devel@lists.xenproject.org On 2015/7/10 21:49, George Dunlap wrote: > On Thu, Jul 9, 2015 at 6:33 AM, Tiejun Chen 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 >> CC: Jan Beulich >> CC: Andrew Cooper >> CC: Ian Jackson >> CC: Stefano Stabellini >> CC: Ian Campbell >> CC: Wei Liu >> Signed-off-by: Tiejun Chen >> Reviewed-by: Kevin Tian >> --- >> 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 >> #include >> >> +/* >> + * 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 >> #include >> #include >> +#include >> #include >> #include >> +#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 >