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

[Juergen Gross]

On 22/02/16 13:24, Daniel Kiper wrote:
> On Mon, Feb 22, 2016 at 10:18:38AM +0100, Juergen Gross wrote:
>> On 22/02/16 09:42, Daniel Kiper wrote:
>>> On Mon, Feb 22, 2016 at 07:03:17AM +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>
>>>> Reviewed-by: Daniel Kiper <address@hidden>
>>>> ---
>>>> V4: rename grub_xen_alloc_end() to grub_xen_alloc_final()
>>>> ---
>>>>  grub-core/loader/i386/xen.c        | 61 
>>>> ++++++++++++++++++++++++++++++--------
>>>>  grub-core/loader/i386/xen_fileXX.c |  3 ++
>>>>  include/grub/xen_file.h            |  1 +
>>>>  3 files changed, 52 insertions(+), 13 deletions(-)
>>>>
>>>> diff --git a/grub-core/loader/i386/xen.c b/grub-core/loader/i386/xen.c
>>>> index 2e12763..22a94ae 100644
>>>> --- a/grub-core/loader/i386/xen.c
>>>> +++ b/grub-core/loader/i386/xen.c
>>>> @@ -58,6 +58,7 @@ struct xen_loader_state {
>>>>    grub_uint64_t modules_target_start;
>>>>    grub_size_t n_modules;
>>>>    int loaded;
>>>> +  int alloc_end_called;
>>>
>>> alloc_end_called -> alloc_final_called
>>>
>>>>  };
>>>>
>>>>  static struct xen_loader_state xen_state;
>>>> @@ -320,6 +321,28 @@ grub_xen_pt_alloc (void)
>>>>  }
>>>>
>>>>  static grub_err_t
>>>> +grub_xen_alloc_final (void)
>>>> +{
>>>> +  grub_err_t err;
>>>> +
>>>> +  if (xen_state.alloc_end_called)
>>>> +    return GRUB_ERR_NONE;
>>>> +  xen_state.alloc_end_called = 1;
>>>
>>> This is not nice. What happens if grub_xen_p2m_alloc() allocate what
>>> is needed and grub_xen_special_alloc() fails?
>>
>> Then grub_xen_alloc_final will fail resulting in boot failure of the
>> new kernel. The memory allocated by the single functions will be freed
>> in case a new boot is attempted (patch 1 and 2).
>>
>>> Maybe grub_xen_p2m_alloc(), grub_xen_special_alloc() and grub_xen_pt_alloc()
>>> should check itself that required regions are allocated properly
>>> (if (something != NULL)) and do nothing if yes.
>>
>> Hmm, with grub_xen_reset() doing appropriate cleanups this would be an
>> option. I think I'll change it as you are suggesting.
> 
> It looks that we can completely avoid that stuff. I think that
> grub_xen_alloc_final() should be called conditionally in both
> places/cases. That is all.

Hmm. I think this is less clear, especially taking patch 11 into
account. And I wouldn't like to tie e.g. page table allocation to
something like "if (!xen_state.virt_start_info)" which would result
from that change, now that I've removed xen_state.alloc_end_called as
you suggested.

Juergen