From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Chen, Tiejun" Subject: Re: [RFC][PATCH 13/13] hvmloader/e820: construct guest e820 table Date: Fri, 15 May 2015 14:11:44 +0800 Message-ID: <55558E20.6020305@intel.com> References: <1428657724-3498-1-git-send-email-tiejun.chen@intel.com> <1428657724-3498-14-git-send-email-tiejun.chen@intel.com> <553529850200007800073DB7@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: <553529850200007800073DB7@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: tim@xen.org, kevin.tian@intel.com, wei.liu2@citrix.com, ian.campbell@citrix.com, andrew.cooper3@citrix.com, Ian.Jackson@eu.citrix.com, xen-devel@lists.xen.org, stefano.stabellini@citrix.com, yang.z.zhang@intel.com List-Id: xen-devel@lists.xenproject.org On 2015/4/20 22:29, Jan Beulich wrote: >>>> On 10.04.15 at 11:22, wrote: >> --- a/tools/firmware/hvmloader/e820.c >> +++ b/tools/firmware/hvmloader/e820.c >> @@ -73,7 +73,8 @@ int build_e820_table(struct e820entry *e820, >> unsigned int lowmem_reserved_base, >> unsigned int bios_image_base) >> { >> - unsigned int nr = 0; >> + unsigned int nr = 0, i, j; >> + struct e820entry tmp; > > The declaration of "tmp" belongs in the most narrow scope you need > it in. Right. > >> @@ -119,10 +120,6 @@ int build_e820_table(struct e820entry *e820, >> >> /* Low RAM goes here. Reserve space for special pages. */ >> BUG_ON((hvm_info->low_mem_pgend << PAGE_SHIFT) < (2u << 20)); >> - e820[nr].addr = 0x100000; >> - e820[nr].size = (hvm_info->low_mem_pgend << PAGE_SHIFT) - e820[nr].addr; >> - e820[nr].type = E820_RAM; >> - nr++; > > I think the above comment needs adjustment with all this code > removed. I also wonder how meaningful the BUG_ON() is with > ->low_mem_pgend no longer used for E820 table construction. > Perhaps this needs another BUG_ON() validating that the field > matches some value from memory_map.map[]? But I think hvm_info->low_mem_pgend is still correct, right? And additionally, there's no any obvious flag to indicate which memory_map.map[x] is that last low memory map. Even we may separate the low memory to construct memory_map.map[]... > >> @@ -159,16 +156,37 @@ int build_e820_table(struct e820entry *e820, >> nr++; >> } >> >> - >> - if ( hvm_info->high_mem_pgend ) >> + /* Construct the remaining according memory_map[]. */ >> + for ( i = 0; i < memory_map.nr_map; i++ ) >> { >> - e820[nr].addr = ((uint64_t)1 << 32); >> - e820[nr].size = >> - ((uint64_t)hvm_info->high_mem_pgend << PAGE_SHIFT) - e820[nr].addr; >> - e820[nr].type = E820_RAM; >> + e820[nr].addr = memory_map.map[i].addr; >> + e820[nr].size = memory_map.map[i].size; >> + e820[nr].type = memory_map.map[i].type; > > Afaict you could use structure assignment here to make this > more readable. Sorry, are you saying this? memcpy(&e820[nr], &memory_map.map[i], sizeof(struct e820entry)); > >> nr++; >> } >> >> + /* May need to reorder all e820 entries. */ >> + for ( j = 0; j < nr-1; j++ ) >> + { >> + for ( i = j+1; i < nr; i++ ) >> + { >> + if ( e820[j].addr > e820[i].addr ) >> + { >> + tmp.addr = e820[j].addr; >> + tmp.size = e820[j].size; >> + tmp.type = e820[j].type; >> + >> + e820[j].addr = e820[i].addr; >> + e820[j].size = e820[i].size; >> + e820[j].type = e820[i].type; >> + >> + e820[i].addr = tmp.addr; >> + e820[i].size = tmp.size; >> + e820[i].type = tmp.type; > > Please again use structure assignments to make this more readable. > And here, for ( j = 0; j < nr-1; j++ ) { for ( i = j+1; i < nr; i++ ) { if ( e820[j].addr > e820[i].addr ) { struct e820entry tmp; memcpy(&tmp, &e820[j], sizeof(struct e820entry)); memcpy(&e820[j], &e820[i], sizeof(struct e820entry)); memcpy(&e820[i], &tmp, sizeof(struct e820entry)); } } } If I'm wrong please correct me. Thanks Tiejun