Re: [PATCH v2 4/6] xen: add capability to load initrd outside of initial mapping

[Juergen Gross <jgross@suse.com>]

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