From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Chen, Tiejun" Subject: Re: [v5][PATCH 09/10] tools:firmware:hvmloader: check to reserve RMRR mappings in e820 Date: Thu, 04 Sep 2014 11:04:57 +0800 Message-ID: <5407D6D9.40007@intel.com> References: <1409050980-21933-1-git-send-email-tiejun.chen@intel.com> <1409050980-21933-10-git-send-email-tiejun.chen@intel.com> <5405A029020000780002FBE2@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <5405A029020000780002FBE2@mail.emea.novell.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: Jan Beulich Cc: kevin.tian@intel.com, ian.campbell@citrix.com, stefano.stabellini@eu.citrix.com, andrew.cooper3@citrix.com, ian.jackson@eu.citrix.com, xen-devel@lists.xen.org, yang.z.zhang@intel.com List-Id: xen-devel@lists.xenproject.org On 2014/9/2 16:47, Jan Beulich wrote: >>>> On 26.08.14 at 13:02, wrote: >> +static unsigned int construct_rmrr_e820_maps(unsigned int nr, s/construct_rmrr_e820_maps/construct_drm_e820_maps >> + uint32_t nr_map, >> + struct reserved_device_memory *map, >> + struct e820entry *e820) >> +{ >> + unsigned int i = 0, j = 0, m = 0, sum_nr = 0; >> + uint64_t start, end, rmrr_start, rmrr_end; I rename this pair of variables in this whole function, s/rmrr_start/drm_start s/rmrr_end/drm_end >> + unsigned int insert = 0, do_insert = 0; >> + >> +do_real_construct: > > Please indent labels by at least one space (and there are further > coding style issues to address elsewhere). Is this necessary? static struct tmem_object_root * obj_find(struct tmem_pool *pool, struct oid *oidp) { struct rb_node *node; struct tmem_object_root *obj; restart_find: read_lock(&pool->pool_rwlock); ... read_unlock(&pool->pool_rwlock); goto restart_find; > >> + sum_nr = nr + nr_map; > > Afaict neither nr nor nr_map change before you get here the second > (and last) time. Hence the calculation can be moved up (ideally into > the initializer of the variable). + unsigned int i = 0, j = 0, m = 0, sum_nr = nr + nr_map; > >> + for ( i = 0; i < nr_map; i++ ) >> + { >> + rmrr_start = map[i].start_pfn << PAGE_SHIFT; >> + rmrr_end = rmrr_start + map[i].nr_pages * PAGE_SIZE; >> + >> + for ( j = 0; j < nr; j++ ) >> + { >> + end = e820[j].addr + e820[j].size; >> + start = e820[j+1].addr; > > This is not valid when j == nr - 1 (last iteration). > - for ( j = 0; j < nr; j++ ) + for ( j = 0; j < nr - 1; j++ ) >> + >> + /* Between those existing e820 entries. */ >> + if ( (rmrr_start > end) && (rmrr_end < start) ) >> + { >> + if (do_insert) - if (do_insert) + 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 == nr_map) - if ( insert == nr_map) + if ( insert == nr_map ) >> + { >> + do_insert = 1; >> + goto do_real_construct; >> + } >> + /* Overlap. */ >> + else >> + { >> + printf("RMRR overlap with those existing e820 entries!\n"); >> + printf("So we don't construct RMRR mapping in e820!\n"); s/RMRR/DRM Thanks Tiejun >> + } >> + >> + 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 reserved_device_memory *map = NULL; >> + int rc; >> >> if ( !lowmem_reserved_base ) >> lowmem_reserved_base = 0xA0000; >> @@ -169,6 +247,29 @@ int build_e820_table(struct e820entry *e820, >> nr++; >> } >> >> + /* We'd better reserve RMRR mapping for each VM to avoid potential >> + * memory conflict. >> + */ >> + if ( hvm_info->nr_reserved_device_memory_map ) >> + { >> + if ( !map ) >> + map = mem_alloc(hvm_info->nr_reserved_device_memory_map * >> sizeof(struct reserved_device_memory), 0); >> + if ( map ) >> + { >> + rc = get_reserved_device_memory_map(map, >> + >> hvm_info->nr_reserved_device_memory_map); >> + if ( !rc ) >> + { >> + nr = construct_rmrr_e820_maps(nr, >> + >> hvm_info->nr_reserved_device_memory_map, >> + map, >> + e820); >> + } >> + } >> + else >> + printf("No space to get reserved_device_memory_map!\n"); >> + } >> + >> return nr; >> } >> >> -- >> 1.9.1 > > > >