[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v5 09/11] xen: add capability to load initrd outside of initi
From: |
Juergen Gross |
Subject: |
Re: [PATCH v5 09/11] xen: add capability to load initrd outside of initial mapping |
Date: |
Fri, 26 Feb 2016 15:28:21 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.6.0 |
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.
> So, maybe it should be called grub_xen_alloc_boot_data() or something like
> that.
>
>>>> + 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.
>
> Ups... I have missed that.
>
>>>> 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.
>
> Yep, but else stuff does not need one.
Ah, okay. Too many different coding styles. Linux kernel would keep
the braces.
>
>>>> 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.
>
> Errr... I have looked at wrong line. Sorry about that.
NP.
Juergen
- [PATCH v5 00/11] grub-xen: support booting huge pv-domains, Juergen Gross, 2016/02/25
- [PATCH v5 09/11] xen: add capability to load initrd outside of initial mapping, Juergen Gross, 2016/02/25
- Re: [PATCH v5 09/11] xen: add capability to load initrd outside of initial mapping, Daniel Kiper, 2016/02/25
- Re: [PATCH v5 09/11] xen: add capability to load initrd outside of initial mapping, Juergen Gross, 2016/02/25
- Re: [PATCH v5 09/11] xen: add capability to load initrd outside of initial mapping, Daniel Kiper, 2016/02/26
- Re: [PATCH v5 09/11] xen: add capability to load initrd outside of initial mapping,
Juergen Gross <=
- Re: [PATCH v5 09/11] xen: add capability to load initrd outside of initial mapping, Daniel Kiper, 2016/02/26
- Re: [PATCH v5 09/11] xen: add capability to load initrd outside of initial mapping, Juergen Gross, 2016/02/29
- Re: [PATCH v5 09/11] xen: add capability to load initrd outside of initial mapping, Daniel Kiper, 2016/02/29
- Re: [PATCH v5 09/11] xen: add capability to load initrd outside of initial mapping, Juergen Gross, 2016/02/29
- Re: [PATCH v5 09/11] xen: add capability to load initrd outside of initial mapping, Daniel Kiper, 2016/02/29
- [PATCH v5 03/11] xen: reduce number of global variables in xen loader, Juergen Gross, 2016/02/25
- [PATCH v5 07/11] xen: factor out allocation of special pages into separate function, Juergen Gross, 2016/02/25
- [PATCH v5 10/11] xen: modify page table construction, Juergen Gross, 2016/02/25
- [PATCH v5 06/11] xen: factor out p2m list allocation into separate function, Juergen Gross, 2016/02/25