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 12:32:53 +0800 Message-ID: <5407EB75.7010101@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> <5407D6D9.40007@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <5407D6D9.40007@intel.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/4 11:04, Chen, Tiejun wrote: > 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 Based on that first patch I think I should rename some terms in other patches: s/device reserved memory/reserved device memory s/DRM/RDM/ s/drm/rdm Thanks Tiejun > >>> + 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 >> >> >> >> > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel > >