From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.71) id 1aaQ4Q-0008J8-RH for mharc-grub-devel@gnu.org; Mon, 29 Feb 2016 10:49:14 -0500 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34548) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aaQ4O-0008I6-E2 for grub-devel@gnu.org; Mon, 29 Feb 2016 10:49:13 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aaQ4J-00015a-Ds for grub-devel@gnu.org; Mon, 29 Feb 2016 10:49:12 -0500 Received: from mx2.suse.de ([195.135.220.15]:45540) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aaQ4J-00015U-24 for grub-devel@gnu.org; Mon, 29 Feb 2016 10:49:07 -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 82914ACA1; Mon, 29 Feb 2016 15:49:05 +0000 (UTC) Subject: Re: [PATCH v5 09/11] xen: add capability to load initrd outside of initial mapping To: Daniel Kiper References: <1456400017-5789-1-git-send-email-jgross@suse.com> <1456400017-5789-10-git-send-email-jgross@suse.com> <20160225124757.GB3482@olila.local.net-space.pl> <56CF1EDA.1000304@suse.com> <20160226140000.GH3482@olila.local.net-space.pl> <56D06105.3060104@suse.com> <20160226154126.GJ3482@olila.local.net-space.pl> <56D400FE.9010907@suse.com> <20160229154337.GB3500@olila.local.net-space.pl> From: Juergen Gross Message-ID: <56D46870.9090508@suse.com> Date: Mon, 29 Feb 2016 16:49:04 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.6.0 MIME-Version: 1.0 In-Reply-To: <20160229154337.GB3500@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: Mon, 29 Feb 2016 15:49:13 -0000 On 29/02/16 16:43, Daniel Kiper wrote: > On Mon, Feb 29, 2016 at 09:27:42AM +0100, Juergen Gross wrote: >> On 26/02/16 16:41, Daniel Kiper wrote: >>> On Fri, Feb 26, 2016 at 03:28:21PM +0100, Juergen Gross wrote: >>>> On 26/02/16 15:00, Daniel Kiper wrote: >>>>> On Thu, Feb 25, 2016 at 04:33:46PM +0100, Juergen Gross wrote: >>>>>> On 25/02/16 13:47, Daniel Kiper wrote: >>>>>>> On Thu, Feb 25, 2016 at 12:33:35PM +0100, Juergen Gross wrote: >>>>>>>> Modern pvops linux kernels support an initrd not covered by the initial >>>>>>>> mapping. This capability is flagged by an elf-note. >>>>>>>> >>>>>>>> In case the elf-note is set by the kernel don't place the initrd into >>>>>>>> the initial mapping. This will allow to load larger initrds and/or >>>>>>>> support domains with larger memory, as the initial mapping is limited >>>>>>>> to 2GB and it is containing the p2m list. >>>>>>>> >>>>>>>> Signed-off-by: Juergen Gross >>>>>>>> --- >>>>>>>> V5: let call grub_xen_alloc_final() all subfunctions unconditionally >>>>>>>> and let them decide whether they need to do anything >>>>>>>> V4: rename grub_xen_alloc_end() to grub_xen_alloc_final() >>>>>>>> --- >>>>>>>> grub-core/loader/i386/xen.c | 65 ++++++++++++++++++++++++++++++-------- >>>>>>>> grub-core/loader/i386/xen_fileXX.c | 3 ++ >>>>>>>> include/grub/xen_file.h | 1 + >>>>>>>> 3 files changed, 56 insertions(+), 13 deletions(-) >>>>>>>> >>>>>>>> diff --git a/grub-core/loader/i386/xen.c b/grub-core/loader/i386/xen.c >>>>>>>> index 2e12763..466f0c0 100644 >>>>>>>> --- a/grub-core/loader/i386/xen.c >>>>>>>> +++ b/grub-core/loader/i386/xen.c >>>>>>>> @@ -228,6 +228,9 @@ grub_xen_p2m_alloc (void) >>>>>>>> grub_size_t p2msize; >>>>>>>> grub_err_t err; >>>>>>>> >>>>>>>> + if (xen_state.virt_mfn_list) >>>>>>>> + return GRUB_ERR_NONE; >>>>>>>> + >>>>>>>> xen_state.state.mfn_list = xen_state.max_addr; >>>>>>>> xen_state.next_start.mfn_list = >>>>>>>> xen_state.max_addr + xen_state.xen_inf.virt_base; >>>>>>>> @@ -250,6 +253,9 @@ grub_xen_special_alloc (void) >>>>>>>> grub_relocator_chunk_t ch; >>>>>>>> grub_err_t err; >>>>>>>> >>>>>>>> + if (xen_state.virt_start_info) >>>>>>>> + return GRUB_ERR_NONE; >>>>>>>> + >>>>>>>> err = grub_relocator_alloc_chunk_addr (xen_state.relocator, &ch, >>>>>>>> xen_state.max_addr, >>>>>>>> sizeof (xen_state.next_start)); >>>>>>>> @@ -281,6 +287,9 @@ grub_xen_pt_alloc (void) >>>>>>>> grub_uint64_t nr_info_pages; >>>>>>>> grub_uint64_t nr_pages, nr_pt_pages, nr_need_pages; >>>>>>>> >>>>>>>> + if (xen_state.virt_pgtable) >>>>>>>> + return GRUB_ERR_NONE; >>>>>>>> + >>>>>>>> xen_state.next_start.pt_base = >>>>>>>> xen_state.max_addr + xen_state.xen_inf.virt_base; >>>>>>>> xen_state.state.paging_start = xen_state.max_addr >> PAGE_SHIFT; >>>>>>>> @@ -320,6 +329,24 @@ grub_xen_pt_alloc (void) >>>>>>>> } >>>>>>>> >>>>>>>> static grub_err_t >>>>>>>> +grub_xen_alloc_final (void) >>>>>>>> +{ >>>>>>>> + grub_err_t err; >>>>>>>> + >>>>>>>> + err = grub_xen_p2m_alloc (); >>>>>>>> + if (err) >>>>>>>> + return err; >>>>>>>> + err = grub_xen_special_alloc (); >>>>>>>> + if (err) >>>>>>>> + return err; >>>>>>>> + err = grub_xen_pt_alloc (); >>>>>>>> + if (err) >>>>>>>> + return err; >>>>>>> >>>>>>> Could you move grub_xen_p2m_alloc() here? This way grub_xen_alloc_final() >>>>>>> will be real final in patch 11 and you do not need an extra condition >>>>>>> around grub_xen_p2m_alloc(). >>>>>> >>>>>> No. Page tables must be allocated after p2m unless p2m is outside of >>>>>> initial kernel mapping (patch 11). >>>>> >>>>> OK, I was afraid of that. However, then grub_xen_alloc_final() is not final. >>>> >>>> Sure it is. Patch 11 just adds allocation to grub_xen_alloc_final(), not >>>> after it. >>> >>> Excerpt from patch 11: >>> >>> @@ -474,6 +503,12 @@ grub_xen_boot (void) >>> err = grub_xen_alloc_final (); >>> if (err) >>> return err; >>> + if (xen_state.xen_inf.has_p2m_base) >>> + { >>> + err = grub_xen_p2m_alloc (); >>> + if (err) >>> + return err; >>> + } >>> >> >> Hmph. No idea what I looked at when writing my previous reply. >> >>>>> So, maybe it should be called grub_xen_alloc_boot_data() or something like that. >>> >>> Then my previous comment is still valid. >> >> What about naming it grub_xen_alloc_kernel_mapping() ? This is what it >> really does: It is allocating all not yet allocated areas which are to >> be included in the initial kernel mapping. > > This function does three things: > - allocate memory for p2m array, > - allocate special pages (e.g. console), > - allocate memory for page tables. > > Three of them are part of boot data described by struct start_info. > Well, there are also other required stuff to properly boot PV guest. > So, I can agree that grub_xen_alloc_boot_data() is not perfect but > I cannot find anything better. Additionally, IMO grub_xen_alloc_kernel_mapping() > is not right choice too. It suggests that this functions does anything > with kernel mapping. Yep, it does, however, this is only one third > of this function. So, I tend to use my proposal or anything better > which is short and quite well describes what is going in this > function as whole. Yeah. grub_xen_alloc_stuff() with a nice comment. ;-) Okay, lets stick with grub_xen_alloc_boot_data() and add a comment explaining that the function will allocate everything not yet allocated and covered by the initial page tables. Juergen