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

[Juergen Gross]

On 26/02/16 16:41, Daniel Kiper wrote:
> On Fri, Feb 26, 2016 at 03:28:21PM +0100, Juergen Gross wrote:
>> On 26/02/16 15:00, Daniel Kiper wrote:
>>> On Thu, Feb 25, 2016 at 04:33:46PM +0100, Juergen Gross wrote:
>>>> 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 <address@hidden>
>>>>>> ---
>>>>>> 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).
>>>
>>> OK, I was afraid of that. However, then grub_xen_alloc_final() is not final.
>>
>> Sure it is. Patch 11 just adds allocation to grub_xen_alloc_final(), not
>> after it.
> 
> Excerpt from patch 11:
> 
> @@ -474,6 +503,12 @@ grub_xen_boot (void)
>    err = grub_xen_alloc_final ();
>    if (err)
>      return err;
> +  if (xen_state.xen_inf.has_p2m_base)
> +    {
> +      err = grub_xen_p2m_alloc ();
> +      if (err)
> +        return err;
> +    }
> 

Hmph. No idea what I looked at when writing my previous reply.

>>> So, maybe it should be called grub_xen_alloc_boot_data() or something like 
>>> that.
> 
> Then my previous comment is still valid.

What about naming it grub_xen_alloc_kernel_mapping() ? This is what it
really does: It is allocating all not yet allocated areas which are to
be included in the initial kernel mapping.

Juergen