Re: [PATCH v5 09/11] xen: add capability to load initrd outside of initial mapping

[Juergen Gross <jgross@suse.com>]

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