From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Chen, Tiejun" Subject: Re: [v3][PATCH 08/16] hvmloader/e820: construct guest e820 table Date: Fri, 12 Jun 2015 16:19:26 +0800 Message-ID: <557A960E.4030309@intel.com> References: <1433985325-16676-1-git-send-email-tiejun.chen@intel.com> <1433985325-16676-9-git-send-email-tiejun.chen@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: "Tian, Kevin" , "jbeulich@suse.com" , "tim@xen.org" , "andrew.cooper3@citrix.com" , "Zhang, Yang Z" , "wei.liu2@citrix.com" , "ian.campbell@citrix.com" , "Ian.Jackson@eu.citrix.com" , "stefano.stabellini@citrix.com" Cc: "xen-devel@lists.xen.org" List-Id: xen-devel@lists.xenproject.org On 2015/6/11 17:59, Tian, Kevin wrote: >> From: Chen, Tiejun >> Sent: Thursday, June 11, 2015 9:15 AM >> >> Now we can use that memory map to build our final >> e820 table but it may need to reorder all e820 >> entries. >> >> Signed-off-by: Tiejun Chen >> --- >> tools/firmware/hvmloader/e820.c | 62 >> +++++++++++++++++++++++++++++++---------- >> 1 file changed, 48 insertions(+), 14 deletions(-) >> >> diff --git a/tools/firmware/hvmloader/e820.c b/tools/firmware/hvmloader/e820.c >> index 2e05e93..c39b0aa 100644 >> --- 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; >> + uint64_t low_mem_pgend = hvm_info->low_mem_pgend << PAGE_SHIFT; > > You may call it low_mem_end to differentiate from original > low_mem_pgend since one means actual address while the > other means pfn. Okay. > >> >> if ( !lowmem_reserved_base ) >> lowmem_reserved_base = 0xA0000; >> @@ -117,13 +118,6 @@ int build_e820_table(struct e820entry *e820, >> e820[nr].type = E820_RESERVED; >> nr++; >> >> - /* 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++; >> - >> /* >> * Explicitly reserve space for special pages. >> * This space starts at RESERVED_MEMBASE an extends to cover various >> @@ -159,16 +153,56 @@ int build_e820_table(struct e820entry *e820, >> nr++; >> } >> >> - >> - if ( hvm_info->high_mem_pgend ) >> + /* >> + * Construct the remaining according memory_map. > > "Construct E820 table according to recorded memory map" Fixed. > >> + * >> + * Note memory_map includes, > > The memory map created by toolstack may include: > Fixed. >> + * >> + * #1. Low memory region >> + * >> + * Low RAM starts at least from 1M to make sure all standard regions >> + * of the PC memory map, like BIOS, VGA memory-mapped I/O and vgabios, >> + * have enough space. >> + * >> + * #2. RDM region if it exists > > "Reserved regions if they exist" Fixed. > >> + * >> + * #3. High memory region if it exists >> + */ >> + 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] = memory_map.map[i]; >> nr++; >> } >> >> + /* Low RAM goes here. Reserve space for special pages. */ >> + BUG_ON(low_mem_pgend < (2u << 20)); >> + /* >> + * We may need to adjust real lowmem end since we may >> + * populate RAM to get enough MMIO previously. >> + */ >> + for ( i = 0; i < memory_map.nr_map; i++ ) > > since you already translate memory map into e820 earlier, here > you should use 'nr' instead of memory_map.nr_map. > As we're saying in the code comment above, we're just handling the lowmem entry, so I think memory_map.nr_map is enough. >> + { >> + uint64_t end = e820[i].addr + e820[i].size; >> + if ( e820[i].type == E820_RAM && >> + low_mem_pgend > e820[i].addr && low_mem_pgend < end ) >> + e820[i].size = low_mem_pgend - e820[i].addr; >> + } > > Sorry I may miss the code but could you elaborate where the > low_mem_pgend is changed after memory map is created? If > it happens within hvmloader, suppose the amount of reduced > memory from original E820_RAM entry should be added to > another E820_RAM entry for highmem, right? You're right so I really should compensate this in highmem entry, add_high_mem = end - low_mem_end; /* * And then we also need to adjust highmem. */ if ( add_high_mem ) { for ( i = 0; i < memory_map.nr_map; i++ ) { if ( e820[i].type == E820_RAM && e820[i].addr > (1ull << 32)) e820[i].size += add_high_mem; } } Thanks Tiejun > >> + >> + /* Finally we 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 ) >> + { >> + struct e820entry tmp; >> + tmp = e820[j]; >> + e820[j] = e820[i]; >> + e820[i] = tmp; >> + } >> + } >> + } >> + >> return nr; >> } >> >> -- >> 1.9.1 > > >