| Message ID | 1456400017-5789-10-git-send-email-jgross@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <jgross@suse.com> > --- > 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(). > + > + 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(). > 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. > 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. Why do not use xen_state.next_start.mod_start here? Daniel
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 <jgross@suse.com> >> --- >> 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). > >> + >> + 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. > >> 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. > >> 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. Juergen
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 <jgross@suse.com> > >> --- > >> 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. 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. > >> 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. Daniel
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 <jgross@suse.com> >>>> --- >>>> 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
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 <jgross@suse.com> > >>>> --- > >>>> 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; + } > > So, maybe it should be called grub_xen_alloc_boot_data() or something like that. Then my previous comment is still valid. Daniel
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 <jgross@suse.com> >>>>>> --- >>>>>> 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. Juergen
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 <jgross@suse.com> > >>>>>> --- > >>>>>> 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. Daniel
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 <jgross@suse.com> >>>>>>>> --- >>>>>>>> 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
On Mon, Feb 29, 2016 at 04:49:04PM +0100, Juergen Gross wrote: > 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 <jgross@suse.com> > >>>>>>>> --- > >>>>>>>> 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. ;-) :-))) I thought about that one too! > 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. LGTM. Granted! Daniel
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; + + 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 (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; - - 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; + } + else + { + xen_state.next_start.mod_start = + xen_state.max_addr + xen_state.xen_inf.virt_base; + xen_state.next_start.mod_len = size; + } 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); + + xen_state.max_addr = ALIGN_UP (xen_state.max_addr + size, PAGE_SIZE); fail: grub_initrd_close (&initrd_ctx); @@ -686,6 +724,7 @@ grub_cmd_module (grub_command_t cmd __attribute__ ((unused)), if (!xen_state.module_info_page) { + xen_state.xen_inf.unmapped_initrd = 0; xen_state.n_modules = 0; xen_state.max_addr = ALIGN_UP (xen_state.max_addr, PAGE_SIZE); xen_state.modules_target_start = xen_state.max_addr; diff --git a/grub-core/loader/i386/xen_fileXX.c b/grub-core/loader/i386/xen_fileXX.c index 03215ca..8751174 100644 --- a/grub-core/loader/i386/xen_fileXX.c +++ b/grub-core/loader/i386/xen_fileXX.c @@ -261,6 +261,9 @@ parse_note (grub_elf_t elf, struct grub_xen_file_info *xi, descsz == 2 ? 2 : 3) == 0) xi->arch = GRUB_XEN_FILE_I386; break; + case XEN_ELFNOTE_MOD_START_PFN: + xi->unmapped_initrd = !!grub_le_to_cpu32(*(grub_uint32_t *) desc); + break; default: grub_dprintf ("xen", "unknown note type %d\n", nh->n_type); break; diff --git a/include/grub/xen_file.h b/include/grub/xen_file.h index 4b2ccba..ed749fa 100644 --- a/include/grub/xen_file.h +++ b/include/grub/xen_file.h @@ -36,6 +36,7 @@ struct grub_xen_file_info int has_note; int has_xen_guest; int extended_cr3; + int unmapped_initrd; enum { GRUB_XEN_FILE_I386 = 1,
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 <jgross@suse.com> --- 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(-)