From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.71) id 1aZJNe-0001nf-Go for mharc-grub-devel@gnu.org; Fri, 26 Feb 2016 09:28:30 -0500 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52320) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aZJNc-0001nW-6B for grub-devel@gnu.org; Fri, 26 Feb 2016 09:28:29 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aZJNY-0007kT-Sm for grub-devel@gnu.org; Fri, 26 Feb 2016 09:28:28 -0500 Received: from mx2.suse.de ([195.135.220.15]:43356) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aZJNY-0007kL-IW for grub-devel@gnu.org; Fri, 26 Feb 2016 09:28:24 -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 108C0AB4B; Fri, 26 Feb 2016 14:28:22 +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> From: Juergen Gross Message-ID: <56D06105.3060104@suse.com> Date: Fri, 26 Feb 2016 15:28:21 +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: <20160226140000.GH3482@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, 26 Feb 2016 14:28:29 -0000 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. > So, maybe it should be called grub_xen_alloc_boot_data() or something like that. > >>>> + return GRUB_ERR_NONE; >>>> +} >>>> + >>>> +static grub_err_t >>>> grub_xen_boot (void) >>>> { >>>> grub_err_t err; >>>> @@ -330,13 +357,7 @@ grub_xen_boot (void) >>>> if (grub_xen_n_allocated_shared_pages) >>>> return grub_error (GRUB_ERR_BUG, "active grants"); >>>> >>>> - err = grub_xen_p2m_alloc (); >>>> - if (err) >>>> - return err; >>>> - err = grub_xen_special_alloc (); >>>> - if (err) >>>> - return err; >>>> - err = grub_xen_pt_alloc (); >>>> + err = grub_xen_alloc_final (); >>> >>> if (!xen_state.xen_inf.unmapped_initrd) >>> { >>> err = grub_xen_alloc_final (); >>> if (err) >>> goto fail; >>> } >>> >>> This way you avoid checks in grub_xen_p2m_alloc(), >>> grub_xen_special_alloc() and grub_xen_pt_alloc(). >> >> No. In case there is no initrd, but the kernel does support an unmapped >> initrd, I would omit allocating special pages, p2m and page tables. > > Ups... I have missed that. > >>>> if (err) >>>> return err; >>>> >>>> @@ -610,6 +631,13 @@ grub_cmd_initrd (grub_command_t cmd __attribute__ ((unused)), >>>> goto fail; >>>> } >>>> >>>> + if (xen_state.xen_inf.unmapped_initrd) >>>> + { >>>> + err = grub_xen_alloc_final (); >>>> + if (err) >>>> + goto fail; >>>> + } >>>> + >>>> if (grub_initrd_init (argc, argv, &initrd_ctx)) >>>> goto fail; >>>> >>>> @@ -627,14 +655,24 @@ grub_cmd_initrd (grub_command_t cmd __attribute__ ((unused)), >>>> goto fail; >>>> } >>>> >>>> - xen_state.next_start.mod_start = >>>> - xen_state.max_addr + xen_state.xen_inf.virt_base; >>>> - xen_state.next_start.mod_len = size; >>> >>> Leave "xen_state.next_start.mod_len = size;" as is here and... >>> >>>> - >>>> - xen_state.max_addr = ALIGN_UP (xen_state.max_addr + size, PAGE_SIZE); >>>> + if (xen_state.xen_inf.unmapped_initrd) >>>> + { >>>> + xen_state.next_start.flags |= SIF_MOD_START_PFN; >>>> + xen_state.next_start.mod_start = xen_state.max_addr >> PAGE_SHIFT; >>>> + xen_state.next_start.mod_len = size; >>> >>> ... do not add it here... >>> >>>> + } >>>> + else >>>> + { >>>> + xen_state.next_start.mod_start = >>>> + xen_state.max_addr + xen_state.xen_inf.virt_base; >>>> + xen_state.next_start.mod_len = size; >>> >>> ... and here... >>> >>>> + } >>> >>> ... and you do not need curly brackets then. >> >> I still do, as in the "if" part I still have two statements. > > Yep, but else stuff does not need one. Ah, okay. Too many different coding styles. Linux kernel would keep the braces. > >>>> grub_dprintf ("xen", "Initrd, addr=0x%x, size=0x%x\n", >>>> - (unsigned) xen_state.next_start.mod_start, (unsigned) size); >>>> + (unsigned) (xen_state.max_addr + xen_state.xen_inf.virt_base), >>>> + (unsigned) size); >>> >>> This should be printed only if !xen_state.xen_inf.unmapped_initrd. >> >> Really? Why? We still have an initrd. >> >>> Why do not use xen_state.next_start.mod_start here? >> >> It is the pfn in case of unmapped_initrd. > > Errr... I have looked at wrong line. Sorry about that. NP. Juergen