From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.71) id 1aU7D0-0002PP-Dp for mharc-grub-devel@gnu.org; Fri, 12 Feb 2016 01:28:02 -0500 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55639) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aU7Cy-0002P3-13 for grub-devel@gnu.org; Fri, 12 Feb 2016 01:28:01 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aU7Cu-0002X1-PP for grub-devel@gnu.org; Fri, 12 Feb 2016 01:27:59 -0500 Received: from mx2.suse.de ([195.135.220.15]:56260) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aU7Cu-0002Wv-E6 for grub-devel@gnu.org; Fri, 12 Feb 2016 01:27:56 -0500 X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay1.suse.de (charybdis-ext.suse.de [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id 30CEDAB5D; Fri, 12 Feb 2016 06:27:55 +0000 (UTC) Subject: Re: [PATCH v2 1/6] xen: factor out p2m list allocation into separate function To: Daniel Kiper References: <1455177206-8974-1-git-send-email-jgross@suse.com> <1455177206-8974-2-git-send-email-jgross@suse.com> <20160211121954.GA3482@olila.local.net-space.pl> <56BC80B2.4070102@suse.com> <20160211170955.GF3482@olila.local.net-space.pl> From: Juergen Gross Message-ID: <56BD7B6A.90108@suse.com> Date: Fri, 12 Feb 2016 07:27:54 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.5.0 MIME-Version: 1.0 In-Reply-To: <20160211170955.GF3482@olila.local.net-space.pl> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x (no timestamps) [generic] X-Received-From: 195.135.220.15 Cc: grub-devel@gnu.org, phcoder@gmail.com, mchang@suse.com, xen-devel@lists.xen.org X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.14 Precedence: list Reply-To: The development of GNU GRUB List-Id: The development of GNU GRUB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 12 Feb 2016 06:28:01 -0000 On 11/02/16 18:09, Daniel Kiper wrote: > On Thu, Feb 11, 2016 at 01:38:10PM +0100, Juergen Gross wrote: >> On 11/02/16 13:19, Daniel Kiper wrote: >>> On Thu, Feb 11, 2016 at 08:53:21AM +0100, Juergen Gross wrote: >>>> Do the p2m list allocation of the to be loaded kernel in a separate >>>> function. This will allow doing the p2m list allocation at different >>>> times of the boot preparations depending on the features the kernel >>>> is supporting. >>>> >>>> While at this remove superfluous setting of first_p2m_pfn and >>>> nr_p2m_frames as those are needed only in case of the p2m list not >>>> being mapped by the initial kernel mapping. >>>> >>>> Signed-off-by: Juergen Gross >>>> --- >>>> grub-core/loader/i386/xen.c | 70 ++++++++++++++++++++++++++------------------- >>>> 1 file changed, 40 insertions(+), 30 deletions(-) >>>> >>>> diff --git a/grub-core/loader/i386/xen.c b/grub-core/loader/i386/xen.c >>>> index c4d9689..42ed7c7 100644 >>>> --- a/grub-core/loader/i386/xen.c >>>> +++ b/grub-core/loader/i386/xen.c >>>> @@ -52,6 +52,8 @@ static struct grub_xen_file_info xen_inf; >>>> static struct xen_multiboot_mod_list *xen_module_info_page; >>>> static grub_uint64_t modules_target_start; >>>> static grub_size_t n_modules; >>>> +static struct grub_relocator_xen_state state; >>>> +static grub_xen_mfn_t *virt_mfn_list; >>> >>> Do we strongly need this as globals? I suppose that >>> both of them could be local to grub_xen_boot. >> >> This would require passing the state pointer to many other functions. >> Same applies to virt_mfn_list. >> >> I just followed the style used in the source already: variables used in >> multiple functions are mostly defined globally (there are even some >> which are used in one function only). > > Well, I do not like that style but if maintainer do not object I will > do not complain more here about that. > >>>> #define PAGE_SIZE 4096 >>>> #define MAX_MODULES (PAGE_SIZE / sizeof (struct xen_multiboot_mod_list)) >>>> @@ -166,7 +168,7 @@ generate_page_table (grub_uint64_t *where, grub_uint64_t paging_start, >>>> } >>>> >>>> static grub_err_t >>>> -set_mfns (grub_xen_mfn_t * new_mfn_list, grub_xen_mfn_t pfn) >>>> +set_mfns (grub_xen_mfn_t pfn) >>>> { >>>> grub_xen_mfn_t i, t; >>>> grub_xen_mfn_t cn_pfn = -1, st_pfn = -1; >>>> @@ -175,32 +177,32 @@ set_mfns (grub_xen_mfn_t * new_mfn_list, grub_xen_mfn_t pfn) >>>> >>>> for (i = 0; i < grub_xen_start_page_addr->nr_pages; i++) >>>> { >>>> - if (new_mfn_list[i] == grub_xen_start_page_addr->console.domU.mfn) >>>> + if (virt_mfn_list[i] == grub_xen_start_page_addr->console.domU.mfn) >>>> cn_pfn = i; >>>> - if (new_mfn_list[i] == grub_xen_start_page_addr->store_mfn) >>>> + if (virt_mfn_list[i] == grub_xen_start_page_addr->store_mfn) >>>> st_pfn = i; >>>> } >>>> if (cn_pfn == (grub_xen_mfn_t)-1) >>>> return grub_error (GRUB_ERR_BUG, "no console"); >>>> if (st_pfn == (grub_xen_mfn_t)-1) >>>> return grub_error (GRUB_ERR_BUG, "no store"); >>>> - t = new_mfn_list[pfn]; >>>> - new_mfn_list[pfn] = new_mfn_list[cn_pfn]; >>>> - new_mfn_list[cn_pfn] = t; >>>> - t = new_mfn_list[pfn + 1]; >>>> - new_mfn_list[pfn + 1] = new_mfn_list[st_pfn]; >>>> - new_mfn_list[st_pfn] = t; >>>> - >>>> - m2p_updates[0].ptr = page2offset (new_mfn_list[pfn]) | MMU_MACHPHYS_UPDATE; >>>> + t = virt_mfn_list[pfn]; >>>> + virt_mfn_list[pfn] = virt_mfn_list[cn_pfn]; >>>> + virt_mfn_list[cn_pfn] = t; >>>> + t = virt_mfn_list[pfn + 1]; >>>> + virt_mfn_list[pfn + 1] = virt_mfn_list[st_pfn]; >>>> + virt_mfn_list[st_pfn] = t; >>>> + >>>> + m2p_updates[0].ptr = page2offset (virt_mfn_list[pfn]) | MMU_MACHPHYS_UPDATE; >>>> m2p_updates[0].val = pfn; >>>> m2p_updates[1].ptr = >>>> - page2offset (new_mfn_list[pfn + 1]) | MMU_MACHPHYS_UPDATE; >>>> + page2offset (virt_mfn_list[pfn + 1]) | MMU_MACHPHYS_UPDATE; >>>> m2p_updates[1].val = pfn + 1; >>>> m2p_updates[2].ptr = >>>> - page2offset (new_mfn_list[cn_pfn]) | MMU_MACHPHYS_UPDATE; >>>> + page2offset (virt_mfn_list[cn_pfn]) | MMU_MACHPHYS_UPDATE; >>>> m2p_updates[2].val = cn_pfn; >>>> m2p_updates[3].ptr = >>>> - page2offset (new_mfn_list[st_pfn]) | MMU_MACHPHYS_UPDATE; >>>> + page2offset (virt_mfn_list[st_pfn]) | MMU_MACHPHYS_UPDATE; >>>> m2p_updates[3].val = st_pfn; >>>> >>>> grub_xen_mmu_update (m2p_updates, 4, NULL, DOMID_SELF); >>>> @@ -209,34 +211,43 @@ set_mfns (grub_xen_mfn_t * new_mfn_list, grub_xen_mfn_t pfn) >>>> } >>>> >>>> static grub_err_t >>>> +grub_xen_p2m_alloc (void) >>>> +{ >>>> + grub_relocator_chunk_t ch; >>>> + grub_size_t p2msize; >>>> + grub_err_t err; >>>> + >>>> + state.mfn_list = max_addr; >>>> + next_start.mfn_list = max_addr + xen_inf.virt_base; >>>> + p2msize = sizeof (grub_xen_mfn_t) * grub_xen_start_page_addr->nr_pages; >>>> + err = grub_relocator_alloc_chunk_addr (relocator, &ch, max_addr, p2msize); >>> >>> Hmmm.... Where relocator is defined? >> >> First global variable in this source. >> >>> >>>> + if (err) >>>> + return err; >>>> + virt_mfn_list = get_virtual_current_address (ch); >>>> + grub_memcpy (virt_mfn_list, >>>> + (void *) grub_xen_start_page_addr->mfn_list, p2msize); >>>> + max_addr = ALIGN_UP (max_addr + p2msize, PAGE_SIZE); >>>> + >>>> + return GRUB_ERR_NONE; >>>> +} >>>> + >>>> +static grub_err_t >>>> grub_xen_boot (void) >>>> { >>>> - struct grub_relocator_xen_state state; >>>> grub_relocator_chunk_t ch; >>>> grub_err_t err; >>>> - grub_size_t pgtsize; >>>> struct start_info *nst; >>>> grub_uint64_t nr_info_pages; >>>> grub_uint64_t nr_pages, nr_pt_pages, nr_need_pages; >>>> struct gnttab_set_version gnttab_setver; >>>> - grub_xen_mfn_t *new_mfn_list; >>>> grub_size_t i; >>>> >>>> if (grub_xen_n_allocated_shared_pages) >>>> return grub_error (GRUB_ERR_BUG, "active grants"); >>>> >>>> - state.mfn_list = max_addr; >>>> - next_start.mfn_list = max_addr + xen_inf.virt_base; >>>> - next_start.first_p2m_pfn = max_addr >> PAGE_SHIFT; /* Is this right? */ >>>> - pgtsize = sizeof (grub_xen_mfn_t) * grub_xen_start_page_addr->nr_pages; >>>> - err = grub_relocator_alloc_chunk_addr (relocator, &ch, max_addr, pgtsize); >>>> - next_start.nr_p2m_frames = (pgtsize + PAGE_SIZE - 1) >> PAGE_SHIFT; >>>> + err = grub_xen_p2m_alloc (); >>>> if (err) >>>> return err; >>>> - new_mfn_list = get_virtual_current_address (ch); >>>> - grub_memcpy (new_mfn_list, >>>> - (void *) grub_xen_start_page_addr->mfn_list, pgtsize); >>>> - max_addr = ALIGN_UP (max_addr + pgtsize, PAGE_SIZE); >>>> >>>> err = grub_relocator_alloc_chunk_addr (relocator, &ch, >>> >>> Ditto. I think that this could be passed as a grub_xen_boot() argument. >>> Otherwise this appear from nowhere and reading/understanding is more >>> difficult. >> >> Huh? How that? grub_xen_boot() can't have any parameters (see prototype >> of grub_loader_set() which is used with grub_xen_boot() as parameter). >> >> BTW: I didn't introduce this kind of usage of relocator. > > OK, I have checked other loaders and it looks that they use relocator in > the same way. Let's leave it as is. However, this does not waive my > concerns expressed above in regards to other global variables. See my reply to patch 4. I'll change my series. Juergen