Re: [PATCH] xen: correctly rebuild mfn list list after migration.

[Jeremy Fitzhardinge <jeremy@goop.org>]

 On 10/12/2010 03:14 AM, Ian Campbell wrote:
> Otherwise the second migration attempt fails because the mfn_list_list
> still refers to all the old mfns.
>
> We need to update the entires in both p2m_top_mfn and the mid_mfn
> pages which p2m_top_mfn refers to.
>
> In order to do this we need to keep track of the virtual addresses
> mapping the p2m_mid_mfn pages since we cannot rely on
> mfn_to_virt(p2m_top_mfn[idx]) since p2m_top_mfn[idx] will still
> contain the old MFN after a migration, which may now belong to another
> domain and hence have a different mapping in the m2p.
>
> Therefore add and maintain a third top level page, p2m_mid_mfn_p[],
> which tracks the virtual addresses of the mfns contained in
> p2m_top_mfn[].
>
> We also need to update the content of the p2m_mid_missing_mfn page on
> resume to refer to the page's new mfn.
>
> p2m_missing does not need updating since the migration process takes
> care of the leaf p2m pages for us.
>
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  arch/x86/xen/mmu.c |   49 ++++++++++++++++++++++++++++++++++++++-----------
>  1 files changed, 38 insertions(+), 11 deletions(-)
>
> diff --git a/arch/x86/xen/mmu.c b/arch/x86/xen/mmu.c
> index 16a8e25..8788064 100644
> --- a/arch/x86/xen/mmu.c
> +++ b/arch/x86/xen/mmu.c
> @@ -185,6 +185,8 @@ DEFINE_PER_CPU(unsigned long, xen_current_cr3);	 /* actual vcpu cr3 */
>   *    / \      / \         /           /
>   *  p2m p2m p2m p2m p2m p2m p2m ...
>   *
> + * The p2m_mid_mfn pages are mapped by p2m_mid_mfn_p.
> + *
>   * The p2m_top and p2m_top_mfn levels are limited to 1 page, so the
>   * maximum representable pseudo-physical address space is:
>   *  P2M_TOP_PER_PAGE * P2M_MID_PER_PAGE * P2M_PER_PAGE pages
> @@ -209,6 +211,7 @@ static RESERVE_BRK_ARRAY(unsigned long, p2m_mid_missing_mfn, P2M_MID_PER_PAGE);
>  
>  static RESERVE_BRK_ARRAY(unsigned long **, p2m_top, P2M_TOP_PER_PAGE);
>  static RESERVE_BRK_ARRAY(unsigned long, p2m_top_mfn, P2M_TOP_PER_PAGE);
> +static RESERVE_BRK_ARRAY(unsigned long *, p2m_mid_mfn_p, P2M_TOP_PER_PAGE);

This should be called "p2m_top_mfn_p" to have consistent naming, since
its at the top of the hierarchy and is indexed by topidx.  The "mid" in
the name makes it look like it should be indexed by mididx, but then it
wouldn't make sense to have just one of these (since there needs to be a
mid for each entry in top).  The fact that it points to mids is
irrelevant (or at least implied by the fact that its a top).

>  
>  RESERVE_BRK(p2m_mid, PAGE_SIZE * (MAX_DOMAIN_PAGES / (P2M_PER_PAGE * P2M_MID_PER_PAGE)));
>  RESERVE_BRK(p2m_mid_mfn, PAGE_SIZE * (MAX_DOMAIN_PAGES / (P2M_PER_PAGE * P2M_MID_PER_PAGE)));
> @@ -245,6 +248,14 @@ static void p2m_top_mfn_init(unsigned long *top)
>  		top[i] = virt_to_mfn(p2m_mid_missing_mfn);
>  }
>  
> +static void p2m_mid_mfn_p_init(unsigned long **top)
> +{
> +	unsigned i;
> +
> +	for (i = 0; i < P2M_TOP_PER_PAGE; i++)
> +		top[i] = p2m_mid_missing_mfn;
> +}
> +
>  static void p2m_mid_init(unsigned long **mid)
>  {
>  	unsigned i;
> @@ -301,15 +312,21 @@ EXPORT_SYMBOL(create_lookup_pte_addr);
>   */
>  void xen_build_mfn_list_list(void)
>  {
> -	unsigned pfn;
> +	unsigned long pfn;
>  
>  	/* Pre-initialize p2m_top_mfn to be completely missing */
>  	if (p2m_top_mfn == NULL) {
>  		p2m_mid_missing_mfn = extend_brk(PAGE_SIZE, PAGE_SIZE);
>  		p2m_mid_mfn_init(p2m_mid_missing_mfn);
>  
> +		p2m_mid_mfn_p = extend_brk(PAGE_SIZE, PAGE_SIZE);
> +		p2m_mid_mfn_p_init(p2m_mid_mfn_p);
> +
>  		p2m_top_mfn = extend_brk(PAGE_SIZE, PAGE_SIZE);
>  		p2m_top_mfn_init(p2m_top_mfn);
> +	} else {
> +		/* Reinitialise, mfn's all change after migration */
> +		p2m_mid_mfn_init(p2m_mid_missing_mfn);
>  	}
>  
>  	for (pfn = 0; pfn < xen_max_p2m_pfn; pfn += P2M_PER_PAGE) {
> @@ -322,14 +339,19 @@ void xen_build_mfn_list_list(void)
>  		mid = p2m_top[topidx];
>  
>  		/* Don't bother allocating any mfn mid levels if
> -		   they're just missing */
> -		if (mid[mididx] == p2m_missing)
> +		 * they're just missing, just update the stored mfn,
> +		 * since all could have changed over a migrate.
> +		 */
> +		if (mid == p2m_mid_missing) {
> +			p2m_top_mfn[topidx] = virt_to_mfn(p2m_mid_missing);
> +			pfn += P2M_MID_PER_PAGE - 1;

Why -1?  How does this interact with the "pfn += P2M_PER_PAGE" in the
for loop?  Should it be "(P2M_MID_PER_PAGE - 1) * P2M_PER_PAGE", with
the pfn += P2M_PER_PAGE moved to the bottom of the loop rather than in
the header?

Is this test even necessary?  Is it to save redundant re-testing of the
mid level?

    J