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

[Juergen Gross <jgross@suse.com>]

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 <jgross@suse.com>
>> Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
>> ---
>> 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.

Juergen