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

[Juergen Gross <jgross@suse.com>]

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