From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Jan Beulich" Subject: Re: [RFC][PATCH 13/13] hvmloader/e820: construct guest e820 table Date: Mon, 20 Apr 2015 15:29:57 +0100 Message-ID: <553529850200007800073DB7@mail.emea.novell.com> References: <1428657724-3498-1-git-send-email-tiejun.chen@intel.com> <1428657724-3498-14-git-send-email-tiejun.chen@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1428657724-3498-14-git-send-email-tiejun.chen@intel.com> Content-Disposition: inline List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Tiejun Chen 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 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. > @@ -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[]? > @@ -159,16 +156,37 @@ int build_e820_table(struct e820entry *e820, > nr++; > } > > - > - if ( hvm_info->high_mem_pgend ) > + /* Construct the remaings 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. > 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. Jan