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

[Juergen Gross <jgross@suse.com>]

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