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:39:38 +0800 Message-ID: <555594AA.4040905@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> <55558E20.6020305@intel.com> <5555AD8B020000780007A5AE@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: <5555AD8B020000780007A5AE@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/5/15 14:25, Jan Beulich wrote: >>>> On 15.05.15 at 08:11, wrote: >> On 2015/4/20 22:29, Jan Beulich wrote: >>>>>> On 10.04.15 at 11:22, wrote: >>>> @@ -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? > > I think so, but as said it's becoming less used and hence less > relevant here. Understood. > >> And >> additionally, there's no any obvious flag to indicate which >> memory_map.map[x] is that last low memory map. > > I didn't imply it would be immediately obvious _how_ to do this. > I'm merely wanting to avoid leaving meaningless BUG_ON()s in > the code, while meaningful ones are amiss. Maybe we should lookup all .map[] to get the lowest memory map and then BUG_ON? > >> Even we may separate the >> low memory to construct memory_map.map[]... > > ??? Sorry I just mean that the low memory is not represented with only one memory_map.map[] in some cases. Is it impossible? Even in the future? Or actually we always consider the lowest memory 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)); > > No, structure assignment (which, other than memcpy(), is type safe): > > e820[nr] = memory_map.map[i]; > Understood. Thanks Tiejun