[PATCHv1 0/3] x86/xen: fixes and improvements to linear p2m

[PATCHv1 0/3] x86/xen: fixes and improvements to linear p2m

[David Vrabel]

The linear p2m changes in 3.19-rc1 is broken with some dom0
configurations.  While trying to fix it I also noticed that
get_phys_to_machine() could be optimized for the common case.

David

[PATCH 1/3] x86/xen: don't count how many PFNs are identity mapped

[David Vrabel]

This accounting is just used to print a diagnostic message that isn't
very useful.

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
---
 arch/x86/xen/setup.c |   27 +++++++++------------------
 1 file changed, 9 insertions(+), 18 deletions(-)

diff --git a/arch/x86/xen/setup.c b/arch/x86/xen/setup.c
index dfd77de..664dffc 100644
--- a/arch/x86/xen/setup.c
+++ b/arch/x86/xen/setup.c
@@ -229,15 +229,14 @@ static int __init xen_free_mfn(unsigned long mfn)
  * as a fallback if the remapping fails.
  */
 static void __init xen_set_identity_and_release_chunk(unsigned long start_pfn,
-	unsigned long end_pfn, unsigned long nr_pages, unsigned long *identity,
-	unsigned long *released)
+	unsigned long end_pfn, unsigned long nr_pages, unsigned long *released)
 {
-	unsigned long len = 0;
 	unsigned long pfn, end;
 	int ret;
 
 	WARN_ON(start_pfn > end_pfn);
 
+	/* Release pages first. */
 	end = min(end_pfn, nr_pages);
 	for (pfn = start_pfn; pfn < end; pfn++) {
 		unsigned long mfn = pfn_to_mfn(pfn);
@@ -250,16 +249,14 @@ static void __init xen_set_identity_and_release_chunk(unsigned long start_pfn,
 		WARN(ret != 1, "Failed to release pfn %lx err=%d\n", pfn, ret);
 
 		if (ret == 1) {
+			(*released)++;
 			if (!__set_phys_to_machine(pfn, INVALID_P2M_ENTRY))
 				break;
-			len++;
 		} else
 			break;
 	}
 
-	/* Need to release pages first */
-	*released += len;
-	*identity += set_phys_range_identity(start_pfn, end_pfn);
+	set_phys_range_identity(start_pfn, end_pfn);
 }
 
 /*
@@ -318,7 +315,6 @@ static void __init xen_do_set_identity_and_remap_chunk(
 	unsigned long ident_pfn_iter, remap_pfn_iter;
 	unsigned long ident_end_pfn = start_pfn + size;
 	unsigned long left = size;
-	unsigned long ident_cnt = 0;
 	unsigned int i, chunk;
 
 	WARN_ON(size == 0);
@@ -347,8 +343,7 @@ static void __init xen_do_set_identity_and_remap_chunk(
 		xen_remap_mfn = mfn;
 
 		/* Set identity map */
-		ident_cnt += set_phys_range_identity(ident_pfn_iter,
-			ident_pfn_iter + chunk);
+		set_phys_range_identity(ident_pfn_iter, ident_pfn_iter + chunk);
 
 		left -= chunk;
 	}
@@ -371,7 +366,7 @@ static void __init xen_do_set_identity_and_remap_chunk(
 static unsigned long __init xen_set_identity_and_remap_chunk(
         const struct e820entry *list, size_t map_size, unsigned long start_pfn,
 	unsigned long end_pfn, unsigned long nr_pages, unsigned long remap_pfn,
-	unsigned long *identity, unsigned long *released)
+	unsigned long *released)
 {
 	unsigned long pfn;
 	unsigned long i = 0;
@@ -386,8 +381,7 @@ static unsigned long __init xen_set_identity_and_remap_chunk(
 		/* Do not remap pages beyond the current allocation */
 		if (cur_pfn >= nr_pages) {
 			/* Identity map remaining pages */
-			*identity += set_phys_range_identity(cur_pfn,
-				cur_pfn + size);
+			set_phys_range_identity(cur_pfn, cur_pfn + size);
 			break;
 		}
 		if (cur_pfn + size > nr_pages)
@@ -398,7 +392,7 @@ static unsigned long __init xen_set_identity_and_remap_chunk(
 		if (!remap_range_size) {
 			pr_warning("Unable to find available pfn range, not remapping identity pages\n");
 			xen_set_identity_and_release_chunk(cur_pfn,
-				cur_pfn + left, nr_pages, identity, released);
+				cur_pfn + left, nr_pages, released);
 			break;
 		}
 		/* Adjust size to fit in current e820 RAM region */
@@ -410,7 +404,6 @@ static unsigned long __init xen_set_identity_and_remap_chunk(
 		/* Update variables to reflect new mappings. */
 		i += size;
 		remap_pfn += size;
-		*identity += size;
 	}
 
 	/*
@@ -430,7 +423,6 @@ static void __init xen_set_identity_and_remap(
 	unsigned long *released)
 {
 	phys_addr_t start = 0;
-	unsigned long identity = 0;
 	unsigned long last_pfn = nr_pages;
 	const struct e820entry *entry;
 	unsigned long num_released = 0;
@@ -460,14 +452,13 @@ static void __init xen_set_identity_and_remap(
 				last_pfn = xen_set_identity_and_remap_chunk(
 						list, map_size, start_pfn,
 						end_pfn, nr_pages, last_pfn,
-						&identity, &num_released);
+						&num_released);
 			start = end;
 		}
 	}
 
 	*released = num_released;
 
-	pr_info("Set %ld page(s) to 1-1 mapping\n", identity);
 	pr_info("Released %ld page(s)\n", num_released);
 }
 
-- 
1.7.10.4

Re: [PATCH 1/3] x86/xen: don't count how many PFNs are identity mapped

[Juergen Gross]

On 01/07/2015 03:47 PM, David Vrabel wrote:
> This accounting is just used to print a diagnostic message that isn't
> very useful.
>
> Signed-off-by: David Vrabel <david.vrabel@citrix.com>

Reviewed-by: Juergen Gross <jgross@suse.com>

> ---
>   arch/x86/xen/setup.c |   27 +++++++++------------------
>   1 file changed, 9 insertions(+), 18 deletions(-)
>
> diff --git a/arch/x86/xen/setup.c b/arch/x86/xen/setup.c
> index dfd77de..664dffc 100644
> --- a/arch/x86/xen/setup.c
> +++ b/arch/x86/xen/setup.c
> @@ -229,15 +229,14 @@ static int __init xen_free_mfn(unsigned long mfn)
>    * as a fallback if the remapping fails.
>    */
>   static void __init xen_set_identity_and_release_chunk(unsigned long start_pfn,
> -	unsigned long end_pfn, unsigned long nr_pages, unsigned long *identity,
> -	unsigned long *released)
> +	unsigned long end_pfn, unsigned long nr_pages, unsigned long *released)
>   {
> -	unsigned long len = 0;
>   	unsigned long pfn, end;
>   	int ret;
>
>   	WARN_ON(start_pfn > end_pfn);
>
> +	/* Release pages first. */
>   	end = min(end_pfn, nr_pages);
>   	for (pfn = start_pfn; pfn < end; pfn++) {
>   		unsigned long mfn = pfn_to_mfn(pfn);
> @@ -250,16 +249,14 @@ static void __init xen_set_identity_and_release_chunk(unsigned long start_pfn,
>   		WARN(ret != 1, "Failed to release pfn %lx err=%d\n", pfn, ret);
>
>   		if (ret == 1) {
> +			(*released)++;
>   			if (!__set_phys_to_machine(pfn, INVALID_P2M_ENTRY))
>   				break;
> -			len++;
>   		} else
>   			break;
>   	}
>
> -	/* Need to release pages first */
> -	*released += len;
> -	*identity += set_phys_range_identity(start_pfn, end_pfn);
> +	set_phys_range_identity(start_pfn, end_pfn);
>   }
>
>   /*
> @@ -318,7 +315,6 @@ static void __init xen_do_set_identity_and_remap_chunk(
>   	unsigned long ident_pfn_iter, remap_pfn_iter;
>   	unsigned long ident_end_pfn = start_pfn + size;
>   	unsigned long left = size;
> -	unsigned long ident_cnt = 0;
>   	unsigned int i, chunk;
>
>   	WARN_ON(size == 0);
> @@ -347,8 +343,7 @@ static void __init xen_do_set_identity_and_remap_chunk(
>   		xen_remap_mfn = mfn;
>
>   		/* Set identity map */
> -		ident_cnt += set_phys_range_identity(ident_pfn_iter,
> -			ident_pfn_iter + chunk);
> +		set_phys_range_identity(ident_pfn_iter, ident_pfn_iter + chunk);
>
>   		left -= chunk;
>   	}
> @@ -371,7 +366,7 @@ static void __init xen_do_set_identity_and_remap_chunk(
>   static unsigned long __init xen_set_identity_and_remap_chunk(
>           const struct e820entry *list, size_t map_size, unsigned long start_pfn,
>   	unsigned long end_pfn, unsigned long nr_pages, unsigned long remap_pfn,
> -	unsigned long *identity, unsigned long *released)
> +	unsigned long *released)
>   {
>   	unsigned long pfn;
>   	unsigned long i = 0;
> @@ -386,8 +381,7 @@ static unsigned long __init xen_set_identity_and_remap_chunk(
>   		/* Do not remap pages beyond the current allocation */
>   		if (cur_pfn >= nr_pages) {
>   			/* Identity map remaining pages */
> -			*identity += set_phys_range_identity(cur_pfn,
> -				cur_pfn + size);
> +			set_phys_range_identity(cur_pfn, cur_pfn + size);
>   			break;
>   		}
>   		if (cur_pfn + size > nr_pages)
> @@ -398,7 +392,7 @@ static unsigned long __init xen_set_identity_and_remap_chunk(
>   		if (!remap_range_size) {
>   			pr_warning("Unable to find available pfn range, not remapping identity pages\n");
>   			xen_set_identity_and_release_chunk(cur_pfn,
> -				cur_pfn + left, nr_pages, identity, released);
> +				cur_pfn + left, nr_pages, released);
>   			break;
>   		}
>   		/* Adjust size to fit in current e820 RAM region */
> @@ -410,7 +404,6 @@ static unsigned long __init xen_set_identity_and_remap_chunk(
>   		/* Update variables to reflect new mappings. */
>   		i += size;
>   		remap_pfn += size;
> -		*identity += size;
>   	}
>
>   	/*
> @@ -430,7 +423,6 @@ static void __init xen_set_identity_and_remap(
>   	unsigned long *released)
>   {
>   	phys_addr_t start = 0;
> -	unsigned long identity = 0;
>   	unsigned long last_pfn = nr_pages;
>   	const struct e820entry *entry;
>   	unsigned long num_released = 0;
> @@ -460,14 +452,13 @@ static void __init xen_set_identity_and_remap(
>   				last_pfn = xen_set_identity_and_remap_chunk(
>   						list, map_size, start_pfn,
>   						end_pfn, nr_pages, last_pfn,
> -						&identity, &num_released);
> +						&num_released);
>   			start = end;
>   		}
>   	}
>
>   	*released = num_released;
>
> -	pr_info("Set %ld page(s) to 1-1 mapping\n", identity);
>   	pr_info("Released %ld page(s)\n", num_released);
>   }
>
>

[PATCH 2/3] x86/xen: add extra memory for remapped frames during setup

[David Vrabel]

If the non-RAM regions in the e820 memory map are larger than the size
of the initial balloon, a BUG was triggered as the frames are remaped
beyond the limit of the linear p2m.  The frames are remapped into the
initial balloon area (xen_extra_mem) but not enough of this is
available.

Ensure enough extra memory regions are added for these remapped
frames.

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
---
 arch/x86/xen/setup.c |   13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/arch/x86/xen/setup.c b/arch/x86/xen/setup.c
index 664dffc..feb6d86 100644
--- a/arch/x86/xen/setup.c
+++ b/arch/x86/xen/setup.c
@@ -366,7 +366,7 @@ static void __init xen_do_set_identity_and_remap_chunk(
 static unsigned long __init xen_set_identity_and_remap_chunk(
         const struct e820entry *list, size_t map_size, unsigned long start_pfn,
 	unsigned long end_pfn, unsigned long nr_pages, unsigned long remap_pfn,
-	unsigned long *released)
+	unsigned long *released, unsigned long *remapped)
 {
 	unsigned long pfn;
 	unsigned long i = 0;
@@ -404,6 +404,7 @@ static unsigned long __init xen_set_identity_and_remap_chunk(
 		/* Update variables to reflect new mappings. */
 		i += size;
 		remap_pfn += size;
+		*remapped += size;
 	}
 
 	/*
@@ -420,12 +421,13 @@ static unsigned long __init xen_set_identity_and_remap_chunk(
 
 static void __init xen_set_identity_and_remap(
 	const struct e820entry *list, size_t map_size, unsigned long nr_pages,
-	unsigned long *released)
+	unsigned long *released, unsigned long *remapped)
 {
 	phys_addr_t start = 0;
 	unsigned long last_pfn = nr_pages;
 	const struct e820entry *entry;
 	unsigned long num_released = 0;
+	unsigned long num_remapped = 0;
 	int i;
 
 	/*
@@ -452,12 +454,13 @@ static void __init xen_set_identity_and_remap(
 				last_pfn = xen_set_identity_and_remap_chunk(
 						list, map_size, start_pfn,
 						end_pfn, nr_pages, last_pfn,
-						&num_released);
+						&num_released, &num_remapped);
 			start = end;
 		}
 	}
 
 	*released = num_released;
+	*remapped = num_remapped;
 
 	pr_info("Released %ld page(s)\n", num_released);
 }
@@ -577,6 +580,7 @@ char * __init xen_memory_setup(void)
 	struct xen_memory_map memmap;
 	unsigned long max_pages;
 	unsigned long extra_pages = 0;
+	unsigned long remapped_pages;
 	int i;
 	int op;
 
@@ -626,9 +630,10 @@ char * __init xen_memory_setup(void)
 	 * underlying RAM.
 	 */
 	xen_set_identity_and_remap(map, memmap.nr_entries, max_pfn,
-				   &xen_released_pages);
+				   &xen_released_pages, &remapped_pages);
 
 	extra_pages += xen_released_pages;
+	extra_pages += remapped_pages;
 
 	/*
 	 * Clamp the amount of extra memory to a EXTRA_MEM_RATIO
-- 
1.7.10.4

Re: [PATCH 2/3] x86/xen: add extra memory for remapped frames during setup

[Juergen Gross]

On 01/07/2015 03:47 PM, David Vrabel wrote:
> If the non-RAM regions in the e820 memory map are larger than the size
> of the initial balloon, a BUG was triggered as the frames are remaped
> beyond the limit of the linear p2m.  The frames are remapped into the
> initial balloon area (xen_extra_mem) but not enough of this is
> available.
>
> Ensure enough extra memory regions are added for these remapped
> frames.
>
> Signed-off-by: David Vrabel <david.vrabel@citrix.com>

Reviewed-by: Juergen Gross <jgross@suse.com>

> ---
>   arch/x86/xen/setup.c |   13 +++++++++----
>   1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/xen/setup.c b/arch/x86/xen/setup.c
> index 664dffc..feb6d86 100644
> --- a/arch/x86/xen/setup.c
> +++ b/arch/x86/xen/setup.c
> @@ -366,7 +366,7 @@ static void __init xen_do_set_identity_and_remap_chunk(
>   static unsigned long __init xen_set_identity_and_remap_chunk(
>           const struct e820entry *list, size_t map_size, unsigned long start_pfn,
>   	unsigned long end_pfn, unsigned long nr_pages, unsigned long remap_pfn,
> -	unsigned long *released)
> +	unsigned long *released, unsigned long *remapped)
>   {
>   	unsigned long pfn;
>   	unsigned long i = 0;
> @@ -404,6 +404,7 @@ static unsigned long __init xen_set_identity_and_remap_chunk(
>   		/* Update variables to reflect new mappings. */
>   		i += size;
>   		remap_pfn += size;
> +		*remapped += size;
>   	}
>
>   	/*
> @@ -420,12 +421,13 @@ static unsigned long __init xen_set_identity_and_remap_chunk(
>
>   static void __init xen_set_identity_and_remap(
>   	const struct e820entry *list, size_t map_size, unsigned long nr_pages,
> -	unsigned long *released)
> +	unsigned long *released, unsigned long *remapped)
>   {
>   	phys_addr_t start = 0;
>   	unsigned long last_pfn = nr_pages;
>   	const struct e820entry *entry;
>   	unsigned long num_released = 0;
> +	unsigned long num_remapped = 0;
>   	int i;
>
>   	/*
> @@ -452,12 +454,13 @@ static void __init xen_set_identity_and_remap(
>   				last_pfn = xen_set_identity_and_remap_chunk(
>   						list, map_size, start_pfn,
>   						end_pfn, nr_pages, last_pfn,
> -						&num_released);
> +						&num_released, &num_remapped);
>   			start = end;
>   		}
>   	}
>
>   	*released = num_released;
> +	*remapped = num_remapped;
>
>   	pr_info("Released %ld page(s)\n", num_released);
>   }
> @@ -577,6 +580,7 @@ char * __init xen_memory_setup(void)
>   	struct xen_memory_map memmap;
>   	unsigned long max_pages;
>   	unsigned long extra_pages = 0;
> +	unsigned long remapped_pages;
>   	int i;
>   	int op;
>
> @@ -626,9 +630,10 @@ char * __init xen_memory_setup(void)
>   	 * underlying RAM.
>   	 */
>   	xen_set_identity_and_remap(map, memmap.nr_entries, max_pfn,
> -				   &xen_released_pages);
> +				   &xen_released_pages, &remapped_pages);
>
>   	extra_pages += xen_released_pages;
> +	extra_pages += remapped_pages;
>
>   	/*
>   	 * Clamp the amount of extra memory to a EXTRA_MEM_RATIO
>

[PATCH 3/3] x86/xen: optimize get_phys_to_machine()

[David Vrabel]

The page table walk is only needed to distinguish between identity and
missing, both of which have INVALID_P2M_ENTRY.

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
---
 arch/x86/xen/p2m.c |   30 ++++++++++++++++++------------
 1 file changed, 18 insertions(+), 12 deletions(-)

diff --git a/arch/x86/xen/p2m.c b/arch/x86/xen/p2m.c
index edbc7a6..a848201 100644
--- a/arch/x86/xen/p2m.c
+++ b/arch/x86/xen/p2m.c
@@ -405,8 +405,7 @@ void __init xen_vmalloc_p2m_tree(void)
 
 unsigned long get_phys_to_machine(unsigned long pfn)
 {
-	pte_t *ptep;
-	unsigned int level;
+	unsigned long mfn;
 
 	if (unlikely(pfn >= xen_p2m_size)) {
 		if (pfn < xen_max_p2m_pfn)
@@ -414,19 +413,26 @@ unsigned long get_phys_to_machine(unsigned long pfn)
 
 		return IDENTITY_FRAME(pfn);
 	}
+	
+	mfn = xen_p2m_addr[pfn];
 
-	ptep = lookup_address((unsigned long)(xen_p2m_addr + pfn), &level);
-	BUG_ON(!ptep || level != PG_LEVEL_4K);
+	if (unlikely(mfn == INVALID_P2M_ENTRY)) {
+		pte_t *ptep;
+		unsigned int level;
 
-	/*
-	 * The INVALID_P2M_ENTRY is filled in both p2m_*identity
-	 * and in p2m_*missing, so returning the INVALID_P2M_ENTRY
-	 * would be wrong.
-	 */
-	if (pte_pfn(*ptep) == PFN_DOWN(__pa(p2m_identity)))
-		return IDENTITY_FRAME(pfn);
+		ptep = lookup_address((unsigned long)(xen_p2m_addr + pfn), &level);
+		BUG_ON(!ptep || level != PG_LEVEL_4K);
+
+		/*
+		 * The INVALID_P2M_ENTRY is filled in both p2m_*identity
+		 * and in p2m_*missing, so returning the INVALID_P2M_ENTRY
+		 * would be wrong.
+		 */
+		if (pte_pfn(*ptep) == PFN_DOWN(__pa(p2m_identity)))
+			return IDENTITY_FRAME(pfn);
+	}
 
-	return xen_p2m_addr[pfn];
+	return mfn;
 }
 EXPORT_SYMBOL_GPL(get_phys_to_machine);
 
-- 
1.7.10.4

Re: [PATCH 3/3] x86/xen: optimize get_phys_to_machine()

[Juergen Gross]

On 01/07/2015 03:47 PM, David Vrabel wrote:
> The page table walk is only needed to distinguish between identity and
> missing, both of which have INVALID_P2M_ENTRY.

As get_phys_to_machine is called by __pfn_to_mfn() only which already
checks for mfn == INVALID_P2M_ENTRY this optimization will have an
effect only in the early boot case with pfn >= xen_p2m_size.

I doubt this is necessary.


Juergen

>
> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
> ---
>   arch/x86/xen/p2m.c |   30 ++++++++++++++++++------------
>   1 file changed, 18 insertions(+), 12 deletions(-)
>
> diff --git a/arch/x86/xen/p2m.c b/arch/x86/xen/p2m.c
> index edbc7a6..a848201 100644
> --- a/arch/x86/xen/p2m.c
> +++ b/arch/x86/xen/p2m.c
> @@ -405,8 +405,7 @@ void __init xen_vmalloc_p2m_tree(void)
>
>   unsigned long get_phys_to_machine(unsigned long pfn)
>   {
> -	pte_t *ptep;
> -	unsigned int level;
> +	unsigned long mfn;
>
>   	if (unlikely(pfn >= xen_p2m_size)) {
>   		if (pfn < xen_max_p2m_pfn)
> @@ -414,19 +413,26 @@ unsigned long get_phys_to_machine(unsigned long pfn)
>
>   		return IDENTITY_FRAME(pfn);
>   	}
> +	
> +	mfn = xen_p2m_addr[pfn];
>
> -	ptep = lookup_address((unsigned long)(xen_p2m_addr + pfn), &level);
> -	BUG_ON(!ptep || level != PG_LEVEL_4K);
> +	if (unlikely(mfn == INVALID_P2M_ENTRY)) {
> +		pte_t *ptep;
> +		unsigned int level;
>
> -	/*
> -	 * The INVALID_P2M_ENTRY is filled in both p2m_*identity
> -	 * and in p2m_*missing, so returning the INVALID_P2M_ENTRY
> -	 * would be wrong.
> -	 */
> -	if (pte_pfn(*ptep) == PFN_DOWN(__pa(p2m_identity)))
> -		return IDENTITY_FRAME(pfn);
> +		ptep = lookup_address((unsigned long)(xen_p2m_addr + pfn), &level);
> +		BUG_ON(!ptep || level != PG_LEVEL_4K);
> +
> +		/*
> +		 * The INVALID_P2M_ENTRY is filled in both p2m_*identity
> +		 * and in p2m_*missing, so returning the INVALID_P2M_ENTRY
> +		 * would be wrong.
> +		 */
> +		if (pte_pfn(*ptep) == PFN_DOWN(__pa(p2m_identity)))
> +			return IDENTITY_FRAME(pfn);
> +	}
>
> -	return xen_p2m_addr[pfn];
> +	return mfn;
>   }
>   EXPORT_SYMBOL_GPL(get_phys_to_machine);
>
>

Re: [PATCH 3/3] x86/xen: optimize get_phys_to_machine()

[David Vrabel]

On 07/01/15 15:18, Juergen Gross wrote:
> On 01/07/2015 03:47 PM, David Vrabel wrote:
>> The page table walk is only needed to distinguish between identity and
>> missing, both of which have INVALID_P2M_ENTRY.
> 
> As get_phys_to_machine is called by __pfn_to_mfn() only which already
> checks for mfn == INVALID_P2M_ENTRY this optimization will have an
> effect only in the early boot case with pfn >= xen_p2m_size.
> 
> I doubt this is necessary.

Doh. Now I remember suggesting this "optimization" before and getting
the same answer from you.

I'll drop this patch.

David