From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Chen, Tiejun" Subject: Re: [RFC][v3][PATCH 3/6] tools:firmware:hvmloader: reserve RMRR mappings in e820 Date: Mon, 18 Aug 2014 15:51:24 +0800 Message-ID: <53F1B07C.8060909@intel.com> References: <1408091238-18364-1-git-send-email-tiejun.chen@intel.com> <1408091238-18364-4-git-send-email-tiejun.chen@intel.com> <53EDD9CF.601@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <53EDD9CF.601@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Andrew Cooper , JBeulich@suse.com, ian.jackson@eu.citrix.com, stefano.stabellini@eu.citrix.com, ian.campbell@citrix.com, yang.z.zhang@intel.com, kevin.tian@intel.com Cc: xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On 2014/8/15 17:58, Andrew Cooper wrote: > On 15/08/14 09:27, Tiejun Chen wrote: >> We need to reserve all RMRR mappings in e820 to avoid any >> potential guest memory conflict. >> >> Signed-off-by: Tiejun Chen >> --- >> tools/firmware/hvmloader/e820.c | 94 +++++++++++++++++++++++++++++++++++++++++ >> tools/firmware/hvmloader/e820.h | 6 +++ >> tools/firmware/hvmloader/util.c | 16 +++++++ >> tools/firmware/hvmloader/util.h | 2 + >> 4 files changed, 118 insertions(+) >> >> diff --git a/tools/firmware/hvmloader/e820.c b/tools/firmware/hvmloader/e820.c >> index 2e05e93..7f54eab 100644 >> --- a/tools/firmware/hvmloader/e820.c >> +++ b/tools/firmware/hvmloader/e820.c >> @@ -68,12 +68,89 @@ void dump_e820_table(struct e820entry *e820, unsigned int nr) >> } >> } > > Please apply Xen coding style to all of this file - there are a lot of > errors. I asked previously what Xen coding style is, and I always try to follow some existing code but seems its not enough. Here I still can't find out what you're pointing, so please tell me exactly what I should change exactly. And why don't you guy generate a checkpatch.pl file? > >> >> +static unsigned int construct_rmrr_e820_maps(unsigned int nr, >> + struct rmrr_map *e820_rmrr_map, >> + struct e820entry *e820) >> +{ >> + unsigned int i = 0, j = 0, m = 0, sum_nr = 0; >> + uint64_t start, end, rmrr_start, rmrr_end; >> + unsigned int insert = 0, do_insert = 0; >> + >> +do_real_construct: >> + sum_nr = nr + e820_rmrr_map->nr_map; >> + for ( i = 0; i < e820_rmrr_map->nr_map; i++ ) >> + { >> + rmrr_start = e820_rmrr_map->map[i].addr; >> + rmrr_end = e820_rmrr_map->map[i].addr + >> + e820_rmrr_map->map[i].size + 1; >> + >> + for ( j = 0; j < nr; j++ ) >> + { >> + end = e820[j].addr + e820[j].size; >> + start = e820[j+1].addr; >> + >> + /* Between those existing e820 entries. */ >> + if ( (rmrr_start > end) && (rmrr_end < start) ) >> + { >> + if (do_insert) >> + { >> + /* Move to free this entry. */ >> + for ( m = sum_nr - 1; m > j; m-- ) >> + { >> + e820[m].addr = e820[m-1].addr; >> + e820[m].size = e820[m-1].size; >> + e820[m].type = e820[m-1].type; >> + } >> + >> + /* Then fill RMRR into that entry. */ >> + e820[j+1].addr = rmrr_start; >> + e820[j+1].size = rmrr_end - rmrr_start; >> + e820[j+1].type = E820_RESERVED; >> + nr++; >> + } >> + insert++; >> + } >> + /* Already at the end. */ >> + else if ( (rmrr_start > end) && !start ) >> + { >> + if (do_insert) >> + { >> + e820[nr].addr = rmrr_start; >> + e820[nr].size = rmrr_end - rmrr_start; >> + e820[nr].type = E820_RESERVED; >> + nr++; >> + } >> + insert++; >> + } >> + } >> + } >> + >> + /* Just return if done. */ >> + if (do_insert) >> + return nr; >> + >> + /* Fine to construct RMRR mappings into e820. */ >> + if ( insert == e820_rmrr_map->nr_map) >> + { >> + do_insert = 1; >> + goto do_real_construct; >> + } >> + /* Overlap. */ >> + else >> + { >> + printf("RMRR overlap with thoese existing e820 entries!\n"); >> + printf("So we don't construct RMRR mapping in e820!\n"); >> + } >> + >> + return nr; >> +} >> /* Create an E820 table based on memory parameters provided in hvm_info. */ >> int build_e820_table(struct e820entry *e820, >> unsigned int lowmem_reserved_base, >> unsigned int bios_image_base) >> { >> unsigned int nr = 0; >> + struct rmrr_map *rmrr_maps; >> >> if ( !lowmem_reserved_base ) >> lowmem_reserved_base = 0xA0000; >> @@ -169,6 +246,23 @@ int build_e820_table(struct e820entry *e820, >> nr++; >> } >> >> + /* We'd better reserve RMRR mapping for each VM to avoid potential >> + * memory conflict. >> + */ >> + rmrr_maps = get_rmrr_map_info(); >> + if ( rmrr_maps->nr_map ) >> + { >> + if ( (nr + rmrr_maps->nr_map) > E820MAX ) >> + { >> + printf(" No free space to insert all RMRR mapping entry!!\n"); >> + return nr; >> + } >> + else >> + { >> + nr = construct_rmrr_e820_maps(nr, rmrr_maps, e820); >> + } >> + } >> + >> return nr; >> } >> >> diff --git a/tools/firmware/hvmloader/e820.h b/tools/firmware/hvmloader/e820.h >> index b2ead7f..a61b80b 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 rmrr_map { >> + unsigned 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..f63434a 100644 >> --- a/tools/firmware/hvmloader/util.c >> +++ b/tools/firmware/hvmloader/util.c >> @@ -766,6 +766,22 @@ struct shared_info *get_shared_info(void) >> return shared_info; >> } >> >> +struct rmrr_map *get_rmrr_map_info(void) >> +{ >> + static int no_rmrr = 1; >> + >> + if ( no_rmrr == 0 ) >> + return &rmrr_e820map; >> + >> + if ( hypercall_memory_op(XENMEM_reserved_device_memory_map, >> + &rmrr_e820map) != 0 ) > > This *only* works because you are not passing a xen_rmrr_memory_map_t as > per the hypercall definition, and Xen is clobbering your heap in a way > which matches the hvmloader layout. I will check this. Thanks Tiejun > >> + BUG(); >> + >> + no_rmrr = 0; >> + >> + return &rmrr_e820map; >> +} >> + >> 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..5f48a86 100644 >> --- a/tools/firmware/hvmloader/util.h >> +++ b/tools/firmware/hvmloader/util.h >> @@ -236,6 +236,8 @@ unsigned long create_pir_tables(void); >> void smp_initialise(void); >> >> #include "e820.h" >> +struct rmrr_map rmrr_e820map; > > Unless I am mistaken, this will put an rmrr_map object named > rmrr_e820map in each translation unit which includes util.h I suspect > it is not what you actually want to do. > > ~Andrew > >> +struct rmrr_map *get_rmrr_map_info(void); >> int build_e820_table(struct e820entry *e820, >> unsigned int lowmem_reserved_base, >> unsigned int bios_image_base); > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel >