From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?ISO-8859-1?Q?Roger_Pau_Monn=E9?= Subject: Re: [PATCH v2] xen: fix setup of PVH Dom0 memory map Date: Fri, 23 May 2014 17:25:38 +0200 Message-ID: <537F6872.4090701@citrix.com> References: <1400778073-18058-1-git-send-email-roger.pau@citrix.com> <537F11460200007800015345@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta14.messagelabs.com ([193.109.254.103]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1WntB6-0007iv-Vg for xen-devel@lists.xenproject.org; Fri, 23 May 2014 17:22:45 +0000 In-Reply-To: <537F11460200007800015345@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: xen-devel@lists.xenproject.org, Tim Deegan List-Id: xen-devel@lists.xenproject.org On 23/05/14 09:13, Jan Beulich wrote: >>>> On 22.05.14 at 19:01, wrote: >> @@ -369,6 +375,100 @@ static __init void pvh_map_all_iomem(struct domain *d) >> nump = end_pfn - start_pfn; >> pvh_add_mem_mapping(d, start_pfn, start_pfn, nump); >> } >> + >> + /* >> + * Add the memory removed by the holes at the end of the >> + * memory map. >> + */ >> + for ( i = 0, entry = e820.map; i < e820.nr_map && nr_holes > 0; >> + i++, entry++ ) >> + { >> + if ( entry->type != E820_RAM ) >> + continue; >> + >> + end_pfn = PFN_UP(entry->addr + entry->size); >> + if ( end_pfn <= nr_pages ) >> + continue; >> + >> + navail = end_pfn - nr_pages; >> + nmap = navail > nr_holes ? nr_holes : navail; > > min()? > >> + nr_holes -= nmap; >> + start_pfn = PFN_DOWN(entry->addr) < nr_pages ? >> + nr_pages : PFN_DOWN(entry->addr); > > max_t()? Fixed both. >> + /* >> + * Populate this memory region using the pages >> + * previously removed by the MMIO holes. >> + */ >> + page_list_for_each ( page, &d->page_list ) >> + { >> + mfn = page_to_mfn(page); >> + if ( get_gpfn_from_mfn(mfn) != INVALID_M2P_ENTRY ) >> + continue; > > This being inside another loop, this is going to be unnecessarily > expensive on big memory systems without dom0_mem=, as you're > going to repeatedly skip a huge (and increasing) number of pages > here. So apart from needing to add a process_pending_softirqs() > call somewhere, I think you should have a cursor into the list > (which isn't going to change under your feet). > > Talking of which, Mukesh, I think the lack of periodic calls to > process_pending_softirqs() is also a latent problem in the function > without Roger's changes. Furthermore now that I look at it again I > wonder whether this whole thing shouldn't be SRAT based (when > there is an SRAT) in order to avoid mapping hotplug memory > regions as MMIO. I've added a call to process_pending_softirqs on pvh_add_mem_mapping and also one here, in case we are dealing with big holes. I will leave the SRAT stuff for later, since it's a much wider change probably. >> + >> + rc = guest_physmap_add_page(d, start_pfn, mfn, 0); >> + if ( rc != 0 ) >> + panic("Unable to add gpfn %#lx mfn %#lx to Dom0 physmap", >> + start_pfn, page_to_mfn(page)); > > Please print rc here too, to help diagnosing eventual problems. Also > please use "mfn" rather than "page_to_mfn(page)". Done. >> + start_pfn++; >> + if ( --nmap == 0 ) >> + break; >> + } >> + ASSERT(nmap == 0); >> + } >> + >> + ASSERT(nr_holes == 0); >> +} >> + >> +static __init void pvh_setup_e820(struct domain *d, unsigned long nr_pages) >> +{ >> + struct e820entry *entry, *entry_guest; >> + unsigned int i; >> + unsigned long pages, cur_pages = 0; >> + >> + /* >> + * Craft the e820 memory map for Dom0 based on the hardware e820 map. >> + */ >> + d->arch.e820 = xzalloc_array(struct e820entry, e820.nr_map); >> + if ( !d->arch.e820 ) >> + panic("Unable to allocate memory for Dom0 e820 map"); >> + entry_guest = d->arch.e820; >> + >> + /* Clamp e820 memory map to match the memory assigned to Dom0 */ >> + for ( i = 0, entry = e820.map; i < e820.nr_map; i++, entry++ ) >> + { >> + if ( entry->type != E820_RAM ) >> + { >> + *entry_guest = *entry; >> + goto next; >> + } >> + >> + if ( nr_pages == cur_pages ) >> + { >> + /* >> + * We already have all the assigned memory, >> + * skip this entry >> + */ >> + continue; >> + } >> + >> + *entry_guest = *entry; >> + pages = (entry_guest->size + PAGE_SIZE-1) >> PAGE_SHIFT; > > PFN_UP()? > >> + if ( (cur_pages + pages) > nr_pages ) >> + { >> + /* Truncate region */ >> + entry_guest->size = (nr_pages - cur_pages) << PAGE_SHIFT; >> + cur_pages = nr_pages; >> + } >> + else >> + { >> + cur_pages += pages; >> + } >> +next: > > Labels indented by at least on space please (avoiding diff's -p option > picking this up as hunk context in future patches). Both of the above comments are fixed, I'm going to send v3 now. Roger.