| Message ID | 1455177206-8974-5-git-send-email-jgross@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Feb 11, 2016 at 08:53:24AM +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> > --- > grub-core/loader/i386/xen.c | 56 ++++++++++++++++++++++++++++++-------- > grub-core/loader/i386/xen_fileXX.c | 3 ++ > include/grub/xen_file.h | 1 + > 3 files changed, 49 insertions(+), 11 deletions(-) > > diff --git a/grub-core/loader/i386/xen.c b/grub-core/loader/i386/xen.c > index 65cec27..0f41048 100644 > --- a/grub-core/loader/i386/xen.c > +++ b/grub-core/loader/i386/xen.c > @@ -307,15 +307,14 @@ grub_xen_pt_alloc (void) > } > > static grub_err_t > -grub_xen_boot (void) > +grub_xen_alloc_end (void) > { > grub_err_t err; > - grub_uint64_t nr_pages; > - struct gnttab_set_version gnttab_setver; > - grub_size_t i; > + static int called = 0; > > - if (grub_xen_n_allocated_shared_pages) > - return grub_error (GRUB_ERR_BUG, "active grants"); > + if (called) > + return GRUB_ERR_NONE; Why? > + called = 1; > > err = grub_xen_p2m_alloc (); > if (err) > @@ -327,6 +326,24 @@ grub_xen_boot (void) > if (err) > return err; > > + return GRUB_ERR_NONE; > +} > + > +static grub_err_t > +grub_xen_boot (void) > +{ > + grub_err_t err; > + grub_uint64_t nr_pages; > + struct gnttab_set_version gnttab_setver; > + grub_size_t i; > + > + if (grub_xen_n_allocated_shared_pages) > + return grub_error (GRUB_ERR_BUG, "active grants"); Why? > + err = grub_xen_alloc_end (); > + if (err) > + return err; > + > err = set_mfns (console_pfn); > if (err) > return err; > @@ -587,6 +604,13 @@ grub_cmd_initrd (grub_command_t cmd __attribute__ ((unused)), > goto fail; > } > > + if (xen_inf.unmapped_initrd) > + { > + err = grub_xen_alloc_end (); > + if (err) > + goto fail; > + } > + > if (grub_initrd_init (argc, argv, &initrd_ctx)) > goto fail; > > @@ -603,13 +627,22 @@ grub_cmd_initrd (grub_command_t cmd __attribute__ ((unused)), > goto fail; > } > > - next_start.mod_start = max_addr + xen_inf.virt_base; > - next_start.mod_len = size; > - > - max_addr = ALIGN_UP (max_addr + size, PAGE_SIZE); > + if (xen_inf.unmapped_initrd) > + { > + next_start.flags |= SIF_MOD_START_PFN; > + next_start.mod_start = max_addr >> PAGE_SHIFT; > + next_start.mod_len = size; > + } > + else > + { > + next_start.mod_start = max_addr + xen_inf.virt_base; > + next_start.mod_len = size; > + } > > grub_dprintf ("xen", "Initrd, addr=0x%x, size=0x%x\n", > - (unsigned) next_start.mod_start, (unsigned) size); > + (unsigned) (max_addr + xen_inf.virt_base), (unsigned) size); > + > + max_addr = ALIGN_UP (max_addr + size, PAGE_SIZE); > > fail: > grub_initrd_close (&initrd_ctx); > @@ -660,6 +693,7 @@ grub_cmd_module (grub_command_t cmd __attribute__ ((unused)), > > if (!xen_module_info_page) > { > + xen_inf.unmapped_initrd = 0; > n_modules = 0; > max_addr = ALIGN_UP (max_addr, PAGE_SIZE); > modules_target_start = max_addr; > diff --git a/grub-core/loader/i386/xen_fileXX.c b/grub-core/loader/i386/xen_fileXX.c > index 1ba5649..69fccd2 100644 > --- a/grub-core/loader/i386/xen_fileXX.c > +++ b/grub-core/loader/i386/xen_fileXX.c > @@ -253,6 +253,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 16: Could you define this a constant and use it here? Daniel
On 11/02/16 13:33, Daniel Kiper wrote: > On Thu, Feb 11, 2016 at 08:53:24AM +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> >> --- >> grub-core/loader/i386/xen.c | 56 ++++++++++++++++++++++++++++++-------- >> grub-core/loader/i386/xen_fileXX.c | 3 ++ >> include/grub/xen_file.h | 1 + >> 3 files changed, 49 insertions(+), 11 deletions(-) >> >> diff --git a/grub-core/loader/i386/xen.c b/grub-core/loader/i386/xen.c >> index 65cec27..0f41048 100644 >> --- a/grub-core/loader/i386/xen.c >> +++ b/grub-core/loader/i386/xen.c >> @@ -307,15 +307,14 @@ grub_xen_pt_alloc (void) >> } >> >> static grub_err_t >> -grub_xen_boot (void) >> +grub_xen_alloc_end (void) >> { >> grub_err_t err; >> - grub_uint64_t nr_pages; >> - struct gnttab_set_version gnttab_setver; >> - grub_size_t i; >> + static int called = 0; >> >> - if (grub_xen_n_allocated_shared_pages) >> - return grub_error (GRUB_ERR_BUG, "active grants"); >> + if (called) >> + return GRUB_ERR_NONE; > > Why? Did you look at the function? grub_xen_alloc_end() (some parts of the original grub_xen_boot()) is new and may be called multiple times. It was much easier to just call it and let it do what must be done only at the first time called. > >> + called = 1; >> >> err = grub_xen_p2m_alloc (); >> if (err) >> @@ -327,6 +326,24 @@ grub_xen_boot (void) >> if (err) >> return err; >> >> + return GRUB_ERR_NONE; >> +} >> + >> +static grub_err_t >> +grub_xen_boot (void) >> +{ >> + grub_err_t err; >> + grub_uint64_t nr_pages; >> + struct gnttab_set_version gnttab_setver; >> + grub_size_t i; >> + >> + if (grub_xen_n_allocated_shared_pages) >> + return grub_error (GRUB_ERR_BUG, "active grants"); > > Why? That's how it has been before. git just decided to generate the diff that way. > >> + err = grub_xen_alloc_end (); >> + if (err) >> + return err; >> + >> err = set_mfns (console_pfn); >> if (err) >> return err; >> @@ -587,6 +604,13 @@ grub_cmd_initrd (grub_command_t cmd __attribute__ ((unused)), >> goto fail; >> } >> >> + if (xen_inf.unmapped_initrd) >> + { >> + err = grub_xen_alloc_end (); >> + if (err) >> + goto fail; >> + } >> + >> if (grub_initrd_init (argc, argv, &initrd_ctx)) >> goto fail; >> >> @@ -603,13 +627,22 @@ grub_cmd_initrd (grub_command_t cmd __attribute__ ((unused)), >> goto fail; >> } >> >> - next_start.mod_start = max_addr + xen_inf.virt_base; >> - next_start.mod_len = size; >> - >> - max_addr = ALIGN_UP (max_addr + size, PAGE_SIZE); >> + if (xen_inf.unmapped_initrd) >> + { >> + next_start.flags |= SIF_MOD_START_PFN; >> + next_start.mod_start = max_addr >> PAGE_SHIFT; >> + next_start.mod_len = size; >> + } >> + else >> + { >> + next_start.mod_start = max_addr + xen_inf.virt_base; >> + next_start.mod_len = size; >> + } >> >> grub_dprintf ("xen", "Initrd, addr=0x%x, size=0x%x\n", >> - (unsigned) next_start.mod_start, (unsigned) size); >> + (unsigned) (max_addr + xen_inf.virt_base), (unsigned) size); >> + >> + max_addr = ALIGN_UP (max_addr + size, PAGE_SIZE); >> >> fail: >> grub_initrd_close (&initrd_ctx); >> @@ -660,6 +693,7 @@ grub_cmd_module (grub_command_t cmd __attribute__ ((unused)), >> >> if (!xen_module_info_page) >> { >> + xen_inf.unmapped_initrd = 0; >> n_modules = 0; >> max_addr = ALIGN_UP (max_addr, PAGE_SIZE); >> modules_target_start = max_addr; >> diff --git a/grub-core/loader/i386/xen_fileXX.c b/grub-core/loader/i386/xen_fileXX.c >> index 1ba5649..69fccd2 100644 >> --- a/grub-core/loader/i386/xen_fileXX.c >> +++ b/grub-core/loader/i386/xen_fileXX.c >> @@ -253,6 +253,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 16: > > Could you define this a constant and use it here? This would be the only case with a constant. All others are numeric as well. Juergen
On Thu, Feb 11, 2016 at 03:13:40PM +0100, Juergen Gross wrote: > On 11/02/16 13:33, Daniel Kiper wrote: > > On Thu, Feb 11, 2016 at 08:53:24AM +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> > >> --- > >> grub-core/loader/i386/xen.c | 56 ++++++++++++++++++++++++++++++-------- > >> grub-core/loader/i386/xen_fileXX.c | 3 ++ > >> include/grub/xen_file.h | 1 + > >> 3 files changed, 49 insertions(+), 11 deletions(-) > >> > >> diff --git a/grub-core/loader/i386/xen.c b/grub-core/loader/i386/xen.c > >> index 65cec27..0f41048 100644 > >> --- a/grub-core/loader/i386/xen.c > >> +++ b/grub-core/loader/i386/xen.c > >> @@ -307,15 +307,14 @@ grub_xen_pt_alloc (void) > >> } > >> > >> static grub_err_t > >> -grub_xen_boot (void) > >> +grub_xen_alloc_end (void) > >> { > >> grub_err_t err; > >> - grub_uint64_t nr_pages; > >> - struct gnttab_set_version gnttab_setver; > >> - grub_size_t i; > >> + static int called = 0; > >> > >> - if (grub_xen_n_allocated_shared_pages) > >> - return grub_error (GRUB_ERR_BUG, "active grants"); > >> + if (called) > >> + return GRUB_ERR_NONE; > > > > Why? > > Did you look at the function? grub_xen_alloc_end() (some parts of the > original grub_xen_boot()) is new and may be called multiple times. It > was much easier to just call it and let it do what must be done only > at the first time called. OK, but this means that you can use this loader only once. If you make a mistake in boot command line then you cannot fix it by calling this loader again because grub_xen_alloc_end() will fail. Am I right? > >> + called = 1; > >> > >> err = grub_xen_p2m_alloc (); > >> if (err) > >> @@ -327,6 +326,24 @@ grub_xen_boot (void) > >> if (err) > >> return err; > >> > >> + return GRUB_ERR_NONE; > >> +} > >> + > >> +static grub_err_t > >> +grub_xen_boot (void) > >> +{ > >> + grub_err_t err; > >> + grub_uint64_t nr_pages; > >> + struct gnttab_set_version gnttab_setver; > >> + grub_size_t i; > >> + > >> + if (grub_xen_n_allocated_shared_pages) > >> + return grub_error (GRUB_ERR_BUG, "active grants"); > > > > Why? > > That's how it has been before. git just decided to generate the diff > that way. Could you try --patience git option? > >> + err = grub_xen_alloc_end (); > >> + if (err) > >> + return err; > >> + > >> err = set_mfns (console_pfn); > >> if (err) > >> return err; > >> @@ -587,6 +604,13 @@ grub_cmd_initrd (grub_command_t cmd __attribute__ ((unused)), > >> goto fail; > >> } > >> > >> + if (xen_inf.unmapped_initrd) > >> + { > >> + err = grub_xen_alloc_end (); > >> + if (err) > >> + goto fail; > >> + } > >> + > >> if (grub_initrd_init (argc, argv, &initrd_ctx)) > >> goto fail; > >> > >> @@ -603,13 +627,22 @@ grub_cmd_initrd (grub_command_t cmd __attribute__ ((unused)), > >> goto fail; > >> } > >> > >> - next_start.mod_start = max_addr + xen_inf.virt_base; > >> - next_start.mod_len = size; > >> - > >> - max_addr = ALIGN_UP (max_addr + size, PAGE_SIZE); > >> + if (xen_inf.unmapped_initrd) > >> + { > >> + next_start.flags |= SIF_MOD_START_PFN; > >> + next_start.mod_start = max_addr >> PAGE_SHIFT; > >> + next_start.mod_len = size; > >> + } > >> + else > >> + { > >> + next_start.mod_start = max_addr + xen_inf.virt_base; > >> + next_start.mod_len = size; > >> + } > >> > >> grub_dprintf ("xen", "Initrd, addr=0x%x, size=0x%x\n", > >> - (unsigned) next_start.mod_start, (unsigned) size); > >> + (unsigned) (max_addr + xen_inf.virt_base), (unsigned) size); > >> + > >> + max_addr = ALIGN_UP (max_addr + size, PAGE_SIZE); > >> > >> fail: > >> grub_initrd_close (&initrd_ctx); > >> @@ -660,6 +693,7 @@ grub_cmd_module (grub_command_t cmd __attribute__ ((unused)), > >> > >> if (!xen_module_info_page) > >> { > >> + xen_inf.unmapped_initrd = 0; > >> n_modules = 0; > >> max_addr = ALIGN_UP (max_addr, PAGE_SIZE); > >> modules_target_start = max_addr; > >> diff --git a/grub-core/loader/i386/xen_fileXX.c b/grub-core/loader/i386/xen_fileXX.c > >> index 1ba5649..69fccd2 100644 > >> --- a/grub-core/loader/i386/xen_fileXX.c > >> +++ b/grub-core/loader/i386/xen_fileXX.c > >> @@ -253,6 +253,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 16: > > > > Could you define this a constant and use it here? > > This would be the only case with a constant. All others are numeric > as well. Ugh... So, please, please fix it at first. It is not an excuse if somebody before you made a mistake (let's call it in that way). Or at least put comments what these values mean. However, I prefer constants. Constants with comments (at least in header file) would be perfect. Daniel
On 11/02/16 18:25, Daniel Kiper wrote: > On Thu, Feb 11, 2016 at 03:13:40PM +0100, Juergen Gross wrote: >> On 11/02/16 13:33, Daniel Kiper wrote: >>> On Thu, Feb 11, 2016 at 08:53:24AM +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> >>>> --- >>>> grub-core/loader/i386/xen.c | 56 ++++++++++++++++++++++++++++++-------- >>>> grub-core/loader/i386/xen_fileXX.c | 3 ++ >>>> include/grub/xen_file.h | 1 + >>>> 3 files changed, 49 insertions(+), 11 deletions(-) >>>> >>>> diff --git a/grub-core/loader/i386/xen.c b/grub-core/loader/i386/xen.c >>>> index 65cec27..0f41048 100644 >>>> --- a/grub-core/loader/i386/xen.c >>>> +++ b/grub-core/loader/i386/xen.c >>>> @@ -307,15 +307,14 @@ grub_xen_pt_alloc (void) >>>> } >>>> >>>> static grub_err_t >>>> -grub_xen_boot (void) >>>> +grub_xen_alloc_end (void) >>>> { >>>> grub_err_t err; >>>> - grub_uint64_t nr_pages; >>>> - struct gnttab_set_version gnttab_setver; >>>> - grub_size_t i; >>>> + static int called = 0; >>>> >>>> - if (grub_xen_n_allocated_shared_pages) >>>> - return grub_error (GRUB_ERR_BUG, "active grants"); >>>> + if (called) >>>> + return GRUB_ERR_NONE; >>> >>> Why? >> >> Did you look at the function? grub_xen_alloc_end() (some parts of the >> original grub_xen_boot()) is new and may be called multiple times. It >> was much easier to just call it and let it do what must be done only >> at the first time called. > > OK, but this means that you can use this loader only once. If you make > a mistake in boot command line then you cannot fix it by calling this > loader again because grub_xen_alloc_end() will fail. Am I right? Yes, you are. I'll need to introduce some kind of state reset function which will re-enable grub_xen_alloc_end() and free all memory allocated. This function should be called on module unload and before initializing the relocator (this is how the other loaders are doing it). > >>>> + called = 1; >>>> >>>> err = grub_xen_p2m_alloc (); >>>> if (err) >>>> @@ -327,6 +326,24 @@ grub_xen_boot (void) >>>> if (err) >>>> return err; >>>> >>>> + return GRUB_ERR_NONE; >>>> +} >>>> + >>>> +static grub_err_t >>>> +grub_xen_boot (void) >>>> +{ >>>> + grub_err_t err; >>>> + grub_uint64_t nr_pages; >>>> + struct gnttab_set_version gnttab_setver; >>>> + grub_size_t i; >>>> + >>>> + if (grub_xen_n_allocated_shared_pages) >>>> + return grub_error (GRUB_ERR_BUG, "active grants"); >>> >>> Why? >> >> That's how it has been before. git just decided to generate the diff >> that way. > > Could you try --patience git option? I think I can. :-) > >>>> + err = grub_xen_alloc_end (); >>>> + if (err) >>>> + return err; >>>> + >>>> err = set_mfns (console_pfn); >>>> if (err) >>>> return err; >>>> @@ -587,6 +604,13 @@ grub_cmd_initrd (grub_command_t cmd __attribute__ ((unused)), >>>> goto fail; >>>> } >>>> >>>> + if (xen_inf.unmapped_initrd) >>>> + { >>>> + err = grub_xen_alloc_end (); >>>> + if (err) >>>> + goto fail; >>>> + } >>>> + >>>> if (grub_initrd_init (argc, argv, &initrd_ctx)) >>>> goto fail; >>>> >>>> @@ -603,13 +627,22 @@ grub_cmd_initrd (grub_command_t cmd __attribute__ ((unused)), >>>> goto fail; >>>> } >>>> >>>> - next_start.mod_start = max_addr + xen_inf.virt_base; >>>> - next_start.mod_len = size; >>>> - >>>> - max_addr = ALIGN_UP (max_addr + size, PAGE_SIZE); >>>> + if (xen_inf.unmapped_initrd) >>>> + { >>>> + next_start.flags |= SIF_MOD_START_PFN; >>>> + next_start.mod_start = max_addr >> PAGE_SHIFT; >>>> + next_start.mod_len = size; >>>> + } >>>> + else >>>> + { >>>> + next_start.mod_start = max_addr + xen_inf.virt_base; >>>> + next_start.mod_len = size; >>>> + } >>>> >>>> grub_dprintf ("xen", "Initrd, addr=0x%x, size=0x%x\n", >>>> - (unsigned) next_start.mod_start, (unsigned) size); >>>> + (unsigned) (max_addr + xen_inf.virt_base), (unsigned) size); >>>> + >>>> + max_addr = ALIGN_UP (max_addr + size, PAGE_SIZE); >>>> >>>> fail: >>>> grub_initrd_close (&initrd_ctx); >>>> @@ -660,6 +693,7 @@ grub_cmd_module (grub_command_t cmd __attribute__ ((unused)), >>>> >>>> if (!xen_module_info_page) >>>> { >>>> + xen_inf.unmapped_initrd = 0; >>>> n_modules = 0; >>>> max_addr = ALIGN_UP (max_addr, PAGE_SIZE); >>>> modules_target_start = max_addr; >>>> diff --git a/grub-core/loader/i386/xen_fileXX.c b/grub-core/loader/i386/xen_fileXX.c >>>> index 1ba5649..69fccd2 100644 >>>> --- a/grub-core/loader/i386/xen_fileXX.c >>>> +++ b/grub-core/loader/i386/xen_fileXX.c >>>> @@ -253,6 +253,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 16: >>> >>> Could you define this a constant and use it here? >> >> This would be the only case with a constant. All others are numeric >> as well. > > Ugh... So, please, please fix it at first. It is not an excuse > if somebody before you made a mistake (let's call it in that way). > Or at least put comments what these values mean. However, I prefer > constants. Constants with comments (at least in header file) would > be perfect. Okay, let me do some cleanup work on the xen loader: - add the possibility to call it multiple times (state reset, free the allocated memory) - merge all necessary global variables into one state structure, use local variables where possible - introduce lots of constants instead of using numerical values all over the code The first two items should go in before my series. The third is more a question of style, so it might block the complete series if I put it early in the series and such a change isn't regarded to be positive. Would you be okay if I put the constant introduction at the end of the series? This would result in readable code (at least I hope so) in case it is accepted and wouldn't block the functionality in case it is rejected. In case there is an early okay from the maintainer(s), I can do the constant introduction early, too, of course. Juergen
Hey Juergen, On Fri, Feb 12, 2016 at 07:25:02AM +0100, Juergen Gross wrote: [...] > Okay, let me do some cleanup work on the xen loader: > > - add the possibility to call it multiple times (state reset, free the > allocated memory) > - merge all necessary global variables into one state structure, use > local variables where possible > - introduce lots of constants instead of using numerical values all over > the code Make sense for me. > The first two items should go in before my series. The third is more a > question of style, so it might block the complete series if I put it > early in the series and such a change isn't regarded to be positive. > Would you be okay if I put the constant introduction at the end of the > series? This would result in readable code (at least I hope so) in case OK, let's do that in that way. > it is accepted and wouldn't block the functionality in case it is > rejected. In case there is an early okay from the maintainer(s), I can > do the constant introduction early, too, of course. Granted! Daniel
On 11.02.2016 15:13, Juergen Gross wrote: > On 11/02/16 13:33, Daniel Kiper wrote: >> On Thu, Feb 11, 2016 at 08:53:24AM +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> >>> --- >>> grub-core/loader/i386/xen.c | 56 ++++++++++++++++++++++++++++++-------- >>> grub-core/loader/i386/xen_fileXX.c | 3 ++ >>> include/grub/xen_file.h | 1 + >>> 3 files changed, 49 insertions(+), 11 deletions(-) >>> >>> diff --git a/grub-core/loader/i386/xen.c b/grub-core/loader/i386/xen.c >>> index 65cec27..0f41048 100644 >>> --- a/grub-core/loader/i386/xen.c >>> +++ b/grub-core/loader/i386/xen.c >>> @@ -307,15 +307,14 @@ grub_xen_pt_alloc (void) >>> } >>> >>> static grub_err_t >>> -grub_xen_boot (void) >>> +grub_xen_alloc_end (void) >>> { >>> grub_err_t err; >>> - grub_uint64_t nr_pages; >>> - struct gnttab_set_version gnttab_setver; >>> - grub_size_t i; >>> + static int called = 0; >>> >>> - if (grub_xen_n_allocated_shared_pages) >>> - return grub_error (GRUB_ERR_BUG, "active grants"); >>> + if (called) >>> + return GRUB_ERR_NONE; >> >> Why? > > Did you look at the function? grub_xen_alloc_end() (some parts of the > original grub_xen_boot()) is new and may be called multiple times. It > was much easier to just call it and let it do what must be done only > at the first time called. > What if a boot fails and then fallback kernel is loaded? >>> + if (grub_xen_n_allocated_shared_pages) >>> + return grub_error (GRUB_ERR_BUG, "active grants"); >> >> Why? > > That's how it has been before. git just decided to generate the diff > that way. > This is also needed to avoid passing control when some drivers are still active >>> + case 16: >> >> Could you define this a constant and use it here? > > This would be the only case with a constant. All others are numeric > as well. > I'm ok with not insisisting on using constants given current state but in general constants are preferable (yes, xen code isn't always clean)
On 12/02/16 13:24, Vladimir '?-coder/phcoder' Serbinenko wrote: > On 11.02.2016 15:13, Juergen Gross wrote: >> On 11/02/16 13:33, Daniel Kiper wrote: >>> On Thu, Feb 11, 2016 at 08:53:24AM +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> --- >>>> grub-core/loader/i386/xen.c | 56 > ++++++++++++++++++++++++++++++-------- >>>> grub-core/loader/i386/xen_fileXX.c | 3 ++ >>>> include/grub/xen_file.h | 1 + 3 files changed, 49 >>>> insertions(+), 11 deletions(-) >>>> >>>> diff --git a/grub-core/loader/i386/xen.c >>>> b/grub-core/loader/i386/xen.c index 65cec27..0f41048 100644 >>>> --- a/grub-core/loader/i386/xen.c +++ >>>> b/grub-core/loader/i386/xen.c @@ -307,15 +307,14 @@ >>>> grub_xen_pt_alloc (void) } >>>> >>>> static grub_err_t -grub_xen_boot (void) +grub_xen_alloc_end >>>> (void) { grub_err_t err; - grub_uint64_t nr_pages; - struct >>>> gnttab_set_version gnttab_setver; - grub_size_t i; + static >>>> int called = 0; >>>> >>>> - if (grub_xen_n_allocated_shared_pages) - return >>>> grub_error (GRUB_ERR_BUG, "active grants"); + if (called) + >>>> return GRUB_ERR_NONE; >>> >>> Why? >> >> Did you look at the function? grub_xen_alloc_end() (some parts of >> the original grub_xen_boot()) is new and may be called multiple >> times. It was much easier to just call it and let it do what must >> be done only at the first time called. >> > What if a boot fails and then fallback kernel is loaded? As already stated before: I'll add a patch handling freeing of memory in a reset function which will reset the "called" flag as well. >>>> + if (grub_xen_n_allocated_shared_pages) + return >>>> grub_error (GRUB_ERR_BUG, "active grants"); >>> >>> Why? >> >> That's how it has been before. git just decided to generate the >> diff that way. >> > This is also needed to avoid passing control when some drivers are > still active >>>> + case 16: >>> >>> Could you define this a constant and use it here? >> >> This would be the only case with a constant. All others are >> numeric as well. >> > I'm ok with not insisisting on using constants given current state > but in general constants are preferable (yes, xen code isn't always > clean) Okay, thanks for the confirmation. I'll add a patch (or better: some patches) which will clean up the xen code by introducing (lots of) constants. Juergen
diff --git a/grub-core/loader/i386/xen.c b/grub-core/loader/i386/xen.c index 65cec27..0f41048 100644 --- a/grub-core/loader/i386/xen.c +++ b/grub-core/loader/i386/xen.c @@ -307,15 +307,14 @@ grub_xen_pt_alloc (void) } static grub_err_t -grub_xen_boot (void) +grub_xen_alloc_end (void) { grub_err_t err; - grub_uint64_t nr_pages; - struct gnttab_set_version gnttab_setver; - grub_size_t i; + static int called = 0; - if (grub_xen_n_allocated_shared_pages) - return grub_error (GRUB_ERR_BUG, "active grants"); + if (called) + return GRUB_ERR_NONE; + called = 1; err = grub_xen_p2m_alloc (); if (err) @@ -327,6 +326,24 @@ grub_xen_boot (void) if (err) return err; + return GRUB_ERR_NONE; +} + +static grub_err_t +grub_xen_boot (void) +{ + grub_err_t err; + grub_uint64_t nr_pages; + struct gnttab_set_version gnttab_setver; + grub_size_t i; + + if (grub_xen_n_allocated_shared_pages) + return grub_error (GRUB_ERR_BUG, "active grants"); + + err = grub_xen_alloc_end (); + if (err) + return err; + err = set_mfns (console_pfn); if (err) return err; @@ -587,6 +604,13 @@ grub_cmd_initrd (grub_command_t cmd __attribute__ ((unused)), goto fail; } + if (xen_inf.unmapped_initrd) + { + err = grub_xen_alloc_end (); + if (err) + goto fail; + } + if (grub_initrd_init (argc, argv, &initrd_ctx)) goto fail; @@ -603,13 +627,22 @@ grub_cmd_initrd (grub_command_t cmd __attribute__ ((unused)), goto fail; } - next_start.mod_start = max_addr + xen_inf.virt_base; - next_start.mod_len = size; - - max_addr = ALIGN_UP (max_addr + size, PAGE_SIZE); + if (xen_inf.unmapped_initrd) + { + next_start.flags |= SIF_MOD_START_PFN; + next_start.mod_start = max_addr >> PAGE_SHIFT; + next_start.mod_len = size; + } + else + { + next_start.mod_start = max_addr + xen_inf.virt_base; + next_start.mod_len = size; + } grub_dprintf ("xen", "Initrd, addr=0x%x, size=0x%x\n", - (unsigned) next_start.mod_start, (unsigned) size); + (unsigned) (max_addr + xen_inf.virt_base), (unsigned) size); + + max_addr = ALIGN_UP (max_addr + size, PAGE_SIZE); fail: grub_initrd_close (&initrd_ctx); @@ -660,6 +693,7 @@ grub_cmd_module (grub_command_t cmd __attribute__ ((unused)), if (!xen_module_info_page) { + xen_inf.unmapped_initrd = 0; n_modules = 0; max_addr = ALIGN_UP (max_addr, PAGE_SIZE); modules_target_start = max_addr; diff --git a/grub-core/loader/i386/xen_fileXX.c b/grub-core/loader/i386/xen_fileXX.c index 1ba5649..69fccd2 100644 --- a/grub-core/loader/i386/xen_fileXX.c +++ b/grub-core/loader/i386/xen_fileXX.c @@ -253,6 +253,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 16: + 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> --- grub-core/loader/i386/xen.c | 56 ++++++++++++++++++++++++++++++-------- grub-core/loader/i386/xen_fileXX.c | 3 ++ include/grub/xen_file.h | 1 + 3 files changed, 49 insertions(+), 11 deletions(-)