[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 1/6] xen: factor out p2m list allocation into separate fun
From: |
Juergen Gross |
Subject: |
Re: [PATCH v2 1/6] xen: factor out p2m list allocation into separate function |
Date: |
Fri, 12 Feb 2016 07:27:54 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.5.0 |
On 11/02/16 18:09, Daniel Kiper wrote:
> On Thu, Feb 11, 2016 at 01:38:10PM +0100, Juergen Gross wrote:
>> On 11/02/16 13:19, Daniel Kiper wrote:
>>> On Thu, Feb 11, 2016 at 08:53:21AM +0100, Juergen Gross wrote:
>>>> Do the p2m list allocation of the to be loaded kernel in a separate
>>>> function. This will allow doing the p2m list allocation at different
>>>> times of the boot preparations depending on the features the kernel
>>>> is supporting.
>>>>
>>>> While at this remove superfluous setting of first_p2m_pfn and
>>>> nr_p2m_frames as those are needed only in case of the p2m list not
>>>> being mapped by the initial kernel mapping.
>>>>
>>>> Signed-off-by: Juergen Gross <address@hidden>
>>>> ---
>>>> grub-core/loader/i386/xen.c | 70
>>>> ++++++++++++++++++++++++++-------------------
>>>> 1 file changed, 40 insertions(+), 30 deletions(-)
>>>>
>>>> diff --git a/grub-core/loader/i386/xen.c b/grub-core/loader/i386/xen.c
>>>> index c4d9689..42ed7c7 100644
>>>> --- a/grub-core/loader/i386/xen.c
>>>> +++ b/grub-core/loader/i386/xen.c
>>>> @@ -52,6 +52,8 @@ static struct grub_xen_file_info xen_inf;
>>>> static struct xen_multiboot_mod_list *xen_module_info_page;
>>>> static grub_uint64_t modules_target_start;
>>>> static grub_size_t n_modules;
>>>> +static struct grub_relocator_xen_state state;
>>>> +static grub_xen_mfn_t *virt_mfn_list;
>>>
>>> Do we strongly need this as globals? I suppose that
>>> both of them could be local to grub_xen_boot.
>>
>> This would require passing the state pointer to many other functions.
>> Same applies to virt_mfn_list.
>>
>> I just followed the style used in the source already: variables used in
>> multiple functions are mostly defined globally (there are even some
>> which are used in one function only).
>
> Well, I do not like that style but if maintainer do not object I will
> do not complain more here about that.
>
>>>> #define PAGE_SIZE 4096
>>>> #define MAX_MODULES (PAGE_SIZE / sizeof (struct xen_multiboot_mod_list))
>>>> @@ -166,7 +168,7 @@ generate_page_table (grub_uint64_t *where,
>>>> grub_uint64_t paging_start,
>>>> }
>>>>
>>>> static grub_err_t
>>>> -set_mfns (grub_xen_mfn_t * new_mfn_list, grub_xen_mfn_t pfn)
>>>> +set_mfns (grub_xen_mfn_t pfn)
>>>> {
>>>> grub_xen_mfn_t i, t;
>>>> grub_xen_mfn_t cn_pfn = -1, st_pfn = -1;
>>>> @@ -175,32 +177,32 @@ set_mfns (grub_xen_mfn_t * new_mfn_list,
>>>> grub_xen_mfn_t pfn)
>>>>
>>>> for (i = 0; i < grub_xen_start_page_addr->nr_pages; i++)
>>>> {
>>>> - if (new_mfn_list[i] == grub_xen_start_page_addr->console.domU.mfn)
>>>> + if (virt_mfn_list[i] == grub_xen_start_page_addr->console.domU.mfn)
>>>> cn_pfn = i;
>>>> - if (new_mfn_list[i] == grub_xen_start_page_addr->store_mfn)
>>>> + if (virt_mfn_list[i] == grub_xen_start_page_addr->store_mfn)
>>>> st_pfn = i;
>>>> }
>>>> if (cn_pfn == (grub_xen_mfn_t)-1)
>>>> return grub_error (GRUB_ERR_BUG, "no console");
>>>> if (st_pfn == (grub_xen_mfn_t)-1)
>>>> return grub_error (GRUB_ERR_BUG, "no store");
>>>> - t = new_mfn_list[pfn];
>>>> - new_mfn_list[pfn] = new_mfn_list[cn_pfn];
>>>> - new_mfn_list[cn_pfn] = t;
>>>> - t = new_mfn_list[pfn + 1];
>>>> - new_mfn_list[pfn + 1] = new_mfn_list[st_pfn];
>>>> - new_mfn_list[st_pfn] = t;
>>>> -
>>>> - m2p_updates[0].ptr = page2offset (new_mfn_list[pfn]) |
>>>> MMU_MACHPHYS_UPDATE;
>>>> + t = virt_mfn_list[pfn];
>>>> + virt_mfn_list[pfn] = virt_mfn_list[cn_pfn];
>>>> + virt_mfn_list[cn_pfn] = t;
>>>> + t = virt_mfn_list[pfn + 1];
>>>> + virt_mfn_list[pfn + 1] = virt_mfn_list[st_pfn];
>>>> + virt_mfn_list[st_pfn] = t;
>>>> +
>>>> + m2p_updates[0].ptr = page2offset (virt_mfn_list[pfn]) |
>>>> MMU_MACHPHYS_UPDATE;
>>>> m2p_updates[0].val = pfn;
>>>> m2p_updates[1].ptr =
>>>> - page2offset (new_mfn_list[pfn + 1]) | MMU_MACHPHYS_UPDATE;
>>>> + page2offset (virt_mfn_list[pfn + 1]) | MMU_MACHPHYS_UPDATE;
>>>> m2p_updates[1].val = pfn + 1;
>>>> m2p_updates[2].ptr =
>>>> - page2offset (new_mfn_list[cn_pfn]) | MMU_MACHPHYS_UPDATE;
>>>> + page2offset (virt_mfn_list[cn_pfn]) | MMU_MACHPHYS_UPDATE;
>>>> m2p_updates[2].val = cn_pfn;
>>>> m2p_updates[3].ptr =
>>>> - page2offset (new_mfn_list[st_pfn]) | MMU_MACHPHYS_UPDATE;
>>>> + page2offset (virt_mfn_list[st_pfn]) | MMU_MACHPHYS_UPDATE;
>>>> m2p_updates[3].val = st_pfn;
>>>>
>>>> grub_xen_mmu_update (m2p_updates, 4, NULL, DOMID_SELF);
>>>> @@ -209,34 +211,43 @@ set_mfns (grub_xen_mfn_t * new_mfn_list,
>>>> grub_xen_mfn_t pfn)
>>>> }
>>>>
>>>> static grub_err_t
>>>> +grub_xen_p2m_alloc (void)
>>>> +{
>>>> + grub_relocator_chunk_t ch;
>>>> + grub_size_t p2msize;
>>>> + grub_err_t err;
>>>> +
>>>> + state.mfn_list = max_addr;
>>>> + next_start.mfn_list = max_addr + xen_inf.virt_base;
>>>> + p2msize = sizeof (grub_xen_mfn_t) * grub_xen_start_page_addr->nr_pages;
>>>> + err = grub_relocator_alloc_chunk_addr (relocator, &ch, max_addr,
>>>> p2msize);
>>>
>>> Hmmm.... Where relocator is defined?
>>
>> First global variable in this source.
>>
>>>
>>>> + if (err)
>>>> + return err;
>>>> + virt_mfn_list = get_virtual_current_address (ch);
>>>> + grub_memcpy (virt_mfn_list,
>>>> + (void *) grub_xen_start_page_addr->mfn_list, p2msize);
>>>> + max_addr = ALIGN_UP (max_addr + p2msize, PAGE_SIZE);
>>>> +
>>>> + return GRUB_ERR_NONE;
>>>> +}
>>>> +
>>>> +static grub_err_t
>>>> grub_xen_boot (void)
>>>> {
>>>> - struct grub_relocator_xen_state state;
>>>> grub_relocator_chunk_t ch;
>>>> grub_err_t err;
>>>> - grub_size_t pgtsize;
>>>> struct start_info *nst;
>>>> grub_uint64_t nr_info_pages;
>>>> grub_uint64_t nr_pages, nr_pt_pages, nr_need_pages;
>>>> struct gnttab_set_version gnttab_setver;
>>>> - grub_xen_mfn_t *new_mfn_list;
>>>> grub_size_t i;
>>>>
>>>> if (grub_xen_n_allocated_shared_pages)
>>>> return grub_error (GRUB_ERR_BUG, "active grants");
>>>>
>>>> - state.mfn_list = max_addr;
>>>> - next_start.mfn_list = max_addr + xen_inf.virt_base;
>>>> - next_start.first_p2m_pfn = max_addr >> PAGE_SHIFT; /* Is this
>>>> right? */
>>>> - pgtsize = sizeof (grub_xen_mfn_t) * grub_xen_start_page_addr->nr_pages;
>>>> - err = grub_relocator_alloc_chunk_addr (relocator, &ch, max_addr,
>>>> pgtsize);
>>>> - next_start.nr_p2m_frames = (pgtsize + PAGE_SIZE - 1) >> PAGE_SHIFT;
>>>> + err = grub_xen_p2m_alloc ();
>>>> if (err)
>>>> return err;
>>>> - new_mfn_list = get_virtual_current_address (ch);
>>>> - grub_memcpy (new_mfn_list,
>>>> - (void *) grub_xen_start_page_addr->mfn_list, pgtsize);
>>>> - max_addr = ALIGN_UP (max_addr + pgtsize, PAGE_SIZE);
>>>>
>>>> err = grub_relocator_alloc_chunk_addr (relocator, &ch,
>>>
>>> Ditto. I think that this could be passed as a grub_xen_boot() argument.
>>> Otherwise this appear from nowhere and reading/understanding is more
>>> difficult.
>>
>> Huh? How that? grub_xen_boot() can't have any parameters (see prototype
>> of grub_loader_set() which is used with grub_xen_boot() as parameter).
>>
>> BTW: I didn't introduce this kind of usage of relocator.
>
> OK, I have checked other loaders and it looks that they use relocator in
> the same way. Let's leave it as is. However, this does not waive my
> concerns expressed above in regards to other global variables.
See my reply to patch 4. I'll change my series.
Juergen
[PATCH v2 1/6] xen: factor out p2m list allocation into separate function, Juergen Gross, 2016/02/11
[PATCH v2 3/6] xen: factor out allocation of page tables into separate function, Juergen Gross, 2016/02/11
[PATCH v2 5/6] xen: modify page table construction, Juergen Gross, 2016/02/11