Re: [Xen-devel] [PATCH v2 1/6] xen: factor out p2m list allocation into separate function

[Juergen Gross <jgross@xxxxxxxx>]

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 <jgross@xxxxxxxx>
>> ---
>>  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).

> 
>>  #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.


Juergen


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel