[PATCH 0/4] variable PHYS_OFFSET support

[PATCH 0/4] variable PHYS_OFFSET support

[Nicolas Pitre]

So here it is, in a form which I think should be ready for merging.

ARM mode is well tested.
Thumb2 mode is only compile tested at the moment.


Nicolas

[PATCH 1/4] ARM: runtime patching of __virt_to_phys() and __phys_to_virt()

[Nicolas Pitre]

On ARM it is common to find different offsets for the location of
physical memory.  In order to support multiple machine types with
a single kernel binary, we need to make PHYS_OFFSET a variable.
But turning PHYS_OFFSET into a global variable would  impact performance
of many hot paths.

In the context of __virt_to_phys() and __phys_to_virt(), we currently have:

	#define __virt_to_phys(x)  ((x) - PAGE_OFFSET + PHYS_OFFSET)

This normally translates into the following assembly instruction:

	add	rx, rx, #(PHYS_OFFSET - PAGE_OFFSET)

If we can assume that the difference between PHYS_OFFSET and PAGE_OFFSET
will always fit into 8 bits shifted to the MSBs, then we can easily patch
this difference into the corresponding assembly instructions at run time.
This is like saying that phys and virt offsets will always be at least
16 MB aligned which is a pretty safe assumption.

So the idea is to create a table of pointers to all those add instructions,
and have the early boot code to walk and patch them up before the kernel
gets to use them.  Result is equivalent to a variable PHYS_OFFSET with
next to zero performance impact compared to the constant PHYS_OFFSET.

Right now, the difference between PHYS_OFFSET and PAGE_OFFSET is determined
by the actual physical address the kernel is executing from upon start,
assuming that the kernel is located within the first 16 MB of RAM.

Thanks to Eric Miao and Russell King for their contributions to this patch.

Signed-off-by: Nicolas Pitre <nicolas.pitre@linaro.org>
---
 arch/arm/Kconfig              |    5 ++++
 arch/arm/include/asm/memory.h |   51 +++++++++++++++++++++++++++++++++--------
 arch/arm/kernel/head.S        |   35 ++++++++++++++++++++++++++++
 arch/arm/kernel/vmlinux.lds.S |    4 +++
 4 files changed, 85 insertions(+), 10 deletions(-)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index d56d21c0..136ed9b 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -187,6 +187,11 @@ config VECTORS_BASE
 	help
 	  The base address of exception vectors.
 
+config ARM_PATCH_PHYS_VIRT
+	bool
+	depends on EXPERIMENTAL
+	depends on !XIP && !THUMB2_KERNEL
+
 source "init/Kconfig"
 
 source "kernel/Kconfig.freezer"
diff --git a/arch/arm/include/asm/memory.h b/arch/arm/include/asm/memory.h
index 23c2e8e..2783ce2 100644
--- a/arch/arm/include/asm/memory.h
+++ b/arch/arm/include/asm/memory.h
@@ -133,16 +133,6 @@
 #endif
 
 /*
- * Physical vs virtual RAM address space conversion.  These are
- * private definitions which should NOT be used outside memory.h
- * files.  Use virt_to_phys/phys_to_virt/__pa/__va instead.
- */
-#ifndef __virt_to_phys
-#define __virt_to_phys(x)	((x) - PAGE_OFFSET + PHYS_OFFSET)
-#define __phys_to_virt(x)	((x) - PHYS_OFFSET + PAGE_OFFSET)
-#endif
-
-/*
  * Convert a physical address to a Page Frame Number and back
  */
 #define	__phys_to_pfn(paddr)	((paddr) >> PAGE_SHIFT)
@@ -157,6 +147,47 @@
 #ifndef __ASSEMBLY__
 
 /*
+ * Physical vs virtual RAM address space conversion.  These are
+ * private definitions which should NOT be used outside memory.h
+ * files.  Use virt_to_phys/phys_to_virt/__pa/__va instead.
+ */
+#ifdef CONFIG_ARM_PATCH_PHYS_VIRT
+
+#ifdef __virt_to_phys
+#error "this machine configuration uses complex __virt_to_phys/__phys_to_virt and cannot use CONFIG_ARM_PATCH_PHYS_VIRT"
+#endif
+
+#define __pv_stub(from,to,instr)			\
+	__asm__(					\
+	"1:	" instr "\t%0, %1, %2\n"		\
+	"	.pushsection .pv_table,\"a\"\n"		\
+	"	.long	1b\n"				\
+	"	.popsection"				\
+	: "=r" (to)					\
+	: "r" (from), "I" (1))
+
+static inline unsigned long __virt_to_phys(unsigned long x)
+{
+	unsigned long t;
+
+	__pv_stub(x, t, "add");
+	return t;
+}
+
+static inline unsigned long __phys_to_virt(unsigned long x)
+{
+	unsigned long t;
+
+	__pv_stub(x, t, "sub");
+	return t;
+}
+
+#else
+#define __virt_to_phys(x)	((x) - PAGE_OFFSET + PHYS_OFFSET)
+#define __phys_to_virt(x)	((x) - PHYS_OFFSET + PAGE_OFFSET)
+#endif
+
+/*
  * The DMA mask corresponding to the maximum bus address allocatable
  * using GFP_DMA.  The default here places no restriction on DMA
  * allocations.  This must be the smallest DMA mask in the system,
diff --git a/arch/arm/kernel/head.S b/arch/arm/kernel/head.S
index 6bd82d2..eaaf0ad 100644
--- a/arch/arm/kernel/head.S
+++ b/arch/arm/kernel/head.S
@@ -95,6 +95,9 @@ ENTRY(stext)
 #ifdef CONFIG_SMP_ON_UP
 	bl	__fixup_smp
 #endif
+#ifdef CONFIG_ARM_PATCH_PHYS_VIRT
+	bl	__fixup_pv_table
+#endif
 	bl	__create_page_tables
 
 	/*
@@ -433,4 +436,36 @@ smp_on_up:
 
 #endif
 
+#ifdef CONFIG_ARM_PATCH_PHYS_VIRT
+
+/* __fixup_pv_table - patch the stub instructions with the delta between
+ * PHYS_OFFSET and PAGE_OFFSET, which is assumed to be 16MiB aligned and
+ * can be expressed by an immediate shifter operand. The stub instruction
+ * has a form of '(add|sub) rd, rn, #imm'.
+ */
+__fixup_pv_table:
+	adr	r0, 1f
+	ldmia	r0, {r3-r5}
+	sub	r3, r0, r3	@ PHYS_OFFSET - PAGE_OFFSET
+	mov	r6, r3, lsr #24	@ constant for add/sub instructions
+	teq	r3, r6, lsl #24 @ must be 16MiB aligned
+	bne	__error
+	orr	r6, r6, #0x400	@ mask in rotate right 8 bits
+	add	r4, r4, r3
+	add	r5, r5, r3
+2:	cmp	r4, r5
+	ldrlo	r7, [r4], #4
+	ldrlo	ip, [r7, r3]
+	mov	ip, ip, lsr #12
+	orr	ip, r6, ip, lsl #12
+	strlo	ip, [r7, r3]
+	blo	2b
+	mov	pc, lr
+ENDPROC(__fixup_phys_virt)
+
+1:	.word	.
+	.word	__pv_table_begin
+	.word	__pv_table_end
+#endif
+
 #include "head-common.S"
diff --git a/arch/arm/kernel/vmlinux.lds.S b/arch/arm/kernel/vmlinux.lds.S
index cead889..fb32c9d 100644
--- a/arch/arm/kernel/vmlinux.lds.S
+++ b/arch/arm/kernel/vmlinux.lds.S
@@ -57,6 +57,10 @@ SECTIONS
 		__smpalt_end = .;
 #endif
 
+		__pv_table_begin = .;
+			*(.pv_table)
+		__pv_table_end = .;
+
 		INIT_SETUP(16)
 
 		INIT_CALLS
-- 
1.7.3.2.193.g78bbb

[PATCH 1/4] ARM: runtime patching of __virt_to_phys() and __phys_to_virt()

[Russell King - ARM Linux]

This is basically my patch with a few blank lines removed, a couple
of \n's also removed, a #error if __virt_to_phys is defined by a platform,
a minor tweak to the assembly and it being only usable on PXA.

I much prefer my patch over this as anyone can use it.  That's one of
the reasons why I arranged the code testing for __virt_to_phys as I
did, so the config option could be offered without having a big long
dependency list attached to it.

On Tue, Jan 04, 2011 at 03:20:05AM -0500, Nicolas Pitre wrote:
> On ARM it is common to find different offsets for the location of
> physical memory.  In order to support multiple machine types with
> a single kernel binary, we need to make PHYS_OFFSET a variable.
> But turning PHYS_OFFSET into a global variable would  impact performance
> of many hot paths.
> 
> In the context of __virt_to_phys() and __phys_to_virt(), we currently have:
> 
> 	#define __virt_to_phys(x)  ((x) - PAGE_OFFSET + PHYS_OFFSET)
> 
> This normally translates into the following assembly instruction:
> 
> 	add	rx, rx, #(PHYS_OFFSET - PAGE_OFFSET)
> 
> If we can assume that the difference between PHYS_OFFSET and PAGE_OFFSET
> will always fit into 8 bits shifted to the MSBs, then we can easily patch
> this difference into the corresponding assembly instructions at run time.
> This is like saying that phys and virt offsets will always be at least
> 16 MB aligned which is a pretty safe assumption.
> 
> So the idea is to create a table of pointers to all those add instructions,
> and have the early boot code to walk and patch them up before the kernel
> gets to use them.  Result is equivalent to a variable PHYS_OFFSET with
> next to zero performance impact compared to the constant PHYS_OFFSET.
> 
> Right now, the difference between PHYS_OFFSET and PAGE_OFFSET is determined
> by the actual physical address the kernel is executing from upon start,
> assuming that the kernel is located within the first 16 MB of RAM.
> 
> Thanks to Eric Miao and Russell King for their contributions to this patch.
> 
> Signed-off-by: Nicolas Pitre <nicolas.pitre@linaro.org>
> ---
>  arch/arm/Kconfig              |    5 ++++
>  arch/arm/include/asm/memory.h |   51 +++++++++++++++++++++++++++++++++--------
>  arch/arm/kernel/head.S        |   35 ++++++++++++++++++++++++++++
>  arch/arm/kernel/vmlinux.lds.S |    4 +++
>  4 files changed, 85 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index d56d21c0..136ed9b 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -187,6 +187,11 @@ config VECTORS_BASE
>  	help
>  	  The base address of exception vectors.
>  
> +config ARM_PATCH_PHYS_VIRT
> +	bool
> +	depends on EXPERIMENTAL
> +	depends on !XIP && !THUMB2_KERNEL
> +
>  source "init/Kconfig"
>  
>  source "kernel/Kconfig.freezer"
> diff --git a/arch/arm/include/asm/memory.h b/arch/arm/include/asm/memory.h
> index 23c2e8e..2783ce2 100644
> --- a/arch/arm/include/asm/memory.h
> +++ b/arch/arm/include/asm/memory.h
> @@ -133,16 +133,6 @@
>  #endif
>  
>  /*
> - * Physical vs virtual RAM address space conversion.  These are
> - * private definitions which should NOT be used outside memory.h
> - * files.  Use virt_to_phys/phys_to_virt/__pa/__va instead.
> - */
> -#ifndef __virt_to_phys
> -#define __virt_to_phys(x)	((x) - PAGE_OFFSET + PHYS_OFFSET)
> -#define __phys_to_virt(x)	((x) - PHYS_OFFSET + PAGE_OFFSET)
> -#endif
> -
> -/*
>   * Convert a physical address to a Page Frame Number and back
>   */
>  #define	__phys_to_pfn(paddr)	((paddr) >> PAGE_SHIFT)
> @@ -157,6 +147,47 @@
>  #ifndef __ASSEMBLY__
>  
>  /*
> + * Physical vs virtual RAM address space conversion.  These are
> + * private definitions which should NOT be used outside memory.h
> + * files.  Use virt_to_phys/phys_to_virt/__pa/__va instead.
> + */
> +#ifdef CONFIG_ARM_PATCH_PHYS_VIRT
> +
> +#ifdef __virt_to_phys
> +#error "this machine configuration uses complex __virt_to_phys/__phys_to_virt and cannot use CONFIG_ARM_PATCH_PHYS_VIRT"
> +#endif
> +
> +#define __pv_stub(from,to,instr)			\
> +	__asm__(					\
> +	"1:	" instr "\t%0, %1, %2\n"		\
> +	"	.pushsection .pv_table,\"a\"\n"		\
> +	"	.long	1b\n"				\
> +	"	.popsection"				\
> +	: "=r" (to)					\
> +	: "r" (from), "I" (1))
> +
> +static inline unsigned long __virt_to_phys(unsigned long x)
> +{
> +	unsigned long t;
> +
> +	__pv_stub(x, t, "add");
> +	return t;
> +}
> +
> +static inline unsigned long __phys_to_virt(unsigned long x)
> +{
> +	unsigned long t;
> +
> +	__pv_stub(x, t, "sub");
> +	return t;
> +}
> +
> +#else
> +#define __virt_to_phys(x)	((x) - PAGE_OFFSET + PHYS_OFFSET)
> +#define __phys_to_virt(x)	((x) - PHYS_OFFSET + PAGE_OFFSET)
> +#endif
> +
> +/*
>   * The DMA mask corresponding to the maximum bus address allocatable
>   * using GFP_DMA.  The default here places no restriction on DMA
>   * allocations.  This must be the smallest DMA mask in the system,
> diff --git a/arch/arm/kernel/head.S b/arch/arm/kernel/head.S
> index 6bd82d2..eaaf0ad 100644
> --- a/arch/arm/kernel/head.S
> +++ b/arch/arm/kernel/head.S
> @@ -95,6 +95,9 @@ ENTRY(stext)
>  #ifdef CONFIG_SMP_ON_UP
>  	bl	__fixup_smp
>  #endif
> +#ifdef CONFIG_ARM_PATCH_PHYS_VIRT
> +	bl	__fixup_pv_table
> +#endif
>  	bl	__create_page_tables
>  
>  	/*
> @@ -433,4 +436,36 @@ smp_on_up:
>  
>  #endif
>  
> +#ifdef CONFIG_ARM_PATCH_PHYS_VIRT
> +
> +/* __fixup_pv_table - patch the stub instructions with the delta between
> + * PHYS_OFFSET and PAGE_OFFSET, which is assumed to be 16MiB aligned and
> + * can be expressed by an immediate shifter operand. The stub instruction
> + * has a form of '(add|sub) rd, rn, #imm'.
> + */
> +__fixup_pv_table:
> +	adr	r0, 1f
> +	ldmia	r0, {r3-r5}
> +	sub	r3, r0, r3	@ PHYS_OFFSET - PAGE_OFFSET
> +	mov	r6, r3, lsr #24	@ constant for add/sub instructions
> +	teq	r3, r6, lsl #24 @ must be 16MiB aligned
> +	bne	__error
> +	orr	r6, r6, #0x400	@ mask in rotate right 8 bits
> +	add	r4, r4, r3
> +	add	r5, r5, r3
> +2:	cmp	r4, r5
> +	ldrlo	r7, [r4], #4
> +	ldrlo	ip, [r7, r3]
> +	mov	ip, ip, lsr #12
> +	orr	ip, r6, ip, lsl #12
> +	strlo	ip, [r7, r3]
> +	blo	2b
> +	mov	pc, lr
> +ENDPROC(__fixup_phys_virt)
> +
> +1:	.word	.
> +	.word	__pv_table_begin
> +	.word	__pv_table_end
> +#endif
> +
>  #include "head-common.S"
> diff --git a/arch/arm/kernel/vmlinux.lds.S b/arch/arm/kernel/vmlinux.lds.S
> index cead889..fb32c9d 100644
> --- a/arch/arm/kernel/vmlinux.lds.S
> +++ b/arch/arm/kernel/vmlinux.lds.S
> @@ -57,6 +57,10 @@ SECTIONS
>  		__smpalt_end = .;
>  #endif
>  
> +		__pv_table_begin = .;
> +			*(.pv_table)
> +		__pv_table_end = .;
> +
>  		INIT_SETUP(16)
>  
>  		INIT_CALLS
> -- 
> 1.7.3.2.193.g78bbb
> 

[PATCH 1/4] ARM: runtime patching of __virt_to_phys() and __phys_to_virt()

[Nicolas Pitre]

On Tue, 4 Jan 2011, Russell King - ARM Linux wrote:

> This is basically my patch with a few blank lines removed, a couple
> of \n's also removed, a #error if __virt_to_phys is defined by a platform,
> a minor tweak to the assembly and it being only usable on PXA.
> 
> I much prefer my patch over this as anyone can use it.  That's one of
> the reasons why I arranged the code testing for __virt_to_phys as I
> did, so the config option could be offered without having a big long
> dependency list attached to it.

I don't think offering the option that people can turn on and not having 
the code effectively perform as expected is a good idea.  People might 
be expecting the feature to be there while in practice it is ignored 
which would lead to confusion.  Better to offer it so it can be selected 
as needed in combination with other features, such as 
CONFIG_AUTO_ZRELADDR (which in my opinion would be better hidden from 
user selection as well).

As to the authorship, since I drafted the original design, Eric Miao did 
the first implementation to validate the concept, and the code surviving 
is mostly yours, I didn't know who to singularly attribute the patch to 
in the author field.  I can put yourself there if you feel this is more 
appropriate.


Nicolas

[PATCH 1/4] ARM: runtime patching of __virt_to_phys() and __phys_to_virt()

[Russell King - ARM Linux]

On Tue, Jan 04, 2011 at 09:32:41AM -0500, Nicolas Pitre wrote:
> On Tue, 4 Jan 2011, Russell King - ARM Linux wrote:
> > This is basically my patch with a few blank lines removed, a couple
> > of \n's also removed, a #error if __virt_to_phys is defined by a platform,
> > a minor tweak to the assembly and it being only usable on PXA.
> > 
> > I much prefer my patch over this as anyone can use it.  That's one of
> > the reasons why I arranged the code testing for __virt_to_phys as I
> > did, so the config option could be offered without having a big long
> > dependency list attached to it.
> 
> I don't think offering the option that people can turn on and not having 
> the code effectively perform as expected is a good idea.  People might 
> be expecting the feature to be there while in practice it is ignored 
> which would lead to confusion.

Our aims are different then.  My aim is to move the code to a point where
it works for _everyone_ it possibly can - and theoretically that's every
platform except:

1. MSM due to their PHYS_OFFSET being 2MB aligned, rather than the more
   normal 256MB alignment.
2. Anyone with complex V:P mappings

(1) is dealt with easily by a dependency in the configuration preventing
the option being visible.  (2) is dealt with at runtime by ignoring the
configuration option - resulting in the p2v tables being empty.  The end
result will still run on the platform, but it won't do the relocation
stuff.  (2) could also be dealt with by adding the necessary dependencies
to the configuration option which is the longer term solution.

Lastly, marking the option as 'EXPERIMENTAL' is there to convey that it
may not work for everyone, and people should expect things not to work if
they enable such an option (and report when that's the case.)

Another reason why selecting this option is wrong is that it is incompatible
with XIP.  If you're going to unconditionally enable it for platforms like
PXA, make sure you strip out all of PXA's XIP support before you do so,
otherwise you'll build a kernel which has absolutely no way of ever booting.

> As to the authorship, since I drafted the original design, Eric Miao did 
> the first implementation to validate the concept, and the code surviving 
> is mostly yours, I didn't know who to singularly attribute the patch to 
> in the author field.  I can put yourself there if you feel this is more 
> appropriate.

The correct thing to do is to ensure that it has Eric's and my sign-offs.
If you compare Eric's to my version, mine has a fair amount of changes.

[PATCH 1/4] ARM: runtime patching of __virt_to_phys() and __phys_to_virt()

[Nicolas Pitre]

On Tue, 4 Jan 2011, Russell King - ARM Linux wrote:

> On Tue, Jan 04, 2011 at 09:32:41AM -0500, Nicolas Pitre wrote:
> > On Tue, 4 Jan 2011, Russell King - ARM Linux wrote:
> > > This is basically my patch with a few blank lines removed, a couple
> > > of \n's also removed, a #error if __virt_to_phys is defined by a platform,
> > > a minor tweak to the assembly and it being only usable on PXA.
> > > 
> > > I much prefer my patch over this as anyone can use it.  That's one of
> > > the reasons why I arranged the code testing for __virt_to_phys as I
> > > did, so the config option could be offered without having a big long
> > > dependency list attached to it.
> > 
> > I don't think offering the option that people can turn on and not having 
> > the code effectively perform as expected is a good idea.  People might 
> > be expecting the feature to be there while in practice it is ignored 
> > which would lead to confusion.
> 
> Our aims are different then.  My aim is to move the code to a point where
> it works for _everyone_ it possibly can - and theoretically that's every
> platform except:
> 
> 1. MSM due to their PHYS_OFFSET being 2MB aligned, rather than the more
>    normal 256MB alignment.
> 2. Anyone with complex V:P mappings

I completely agree with that goal.  But I'd prefer for those platforms 
which are not yet supported by this feature not to be able to compile 
rather than silently ignore the feature and not behave as expected.

> (1) is dealt with easily by a dependency in the configuration preventing
> the option being visible.  (2) is dealt with at runtime by ignoring the
> configuration option - resulting in the p2v tables being empty.  The end
> result will still run on the platform, but it won't do the relocation
> stuff.  (2) could also be dealt with by adding the necessary dependencies
> to the configuration option which is the longer term solution.

Since (2) is not supported yet with this config option selected, I think 
it is best to simply #error the build.

> Lastly, marking the option as 'EXPERIMENTAL' is there to convey that it
> may not work for everyone, and people should expect things not to work if
> they enable such an option (and report when that's the case.)

Sure, hence my #error in the patch which is even easier to diagnose and 
self explanatory.

And in fact I think that this would indeed be simpler to just fall back 
to a global variable for PHYS_OFFSET when a platform defines its own 
p2v/v2p mapping.  This way, the goal of this feature would be 
universally available.

OTOH, one of the long term goal for this is to be able to support more 
than one SOC family with the same kernel binary.  I don't think it is 
worth extending this support to be able to also include those platforms 
with a complex v2p and p2v translation, as the overhead for all included 
platforms would simply kill the advantages of having a single kernel 
binary.  So I still don't see a big compelling reason to support (2) at 
all with this, except maybe only for kdump purposes.

> Another reason why selecting this option is wrong is that it is incompatible
> with XIP.  If you're going to unconditionally enable it for platforms like
> PXA, make sure you strip out all of PXA's XIP support before you do so,
> otherwise you'll build a kernel which has absolutely no way of ever booting.

Well, my idea is more about a global config option that makes sense for 
the user, like "physically position independent kernel binary" or 
something like that.  This option would of course be dependent on !XIP 
and !ZBOOT_ROM, and would in turn select AUTO_ZRELADDR and 
ARM_PATCH_PHYS_VIRT.  In fact, maybe those options should all be 
consolidated under a single generic config option expressing the goal to 
be achieved instead of the implementation strategy.

> > As to the authorship, since I drafted the original design, Eric Miao did 
> > the first implementation to validate the concept, and the code surviving 
> > is mostly yours, I didn't know who to singularly attribute the patch to 
> > in the author field.  I can put yourself there if you feel this is more 
> > appropriate.
> 
> The correct thing to do is to ensure that it has Eric's and my 
> sign-offs. 

Fair enough.

> If you compare Eric's to my version, mine has a fair amount of 
> changes.

Yep, and I'm now adding to the mix.  So this is a good idea to clarify 
this issue.


Nicolas

[PATCH 1/4] ARM: runtime patching of __virt_to_phys() and __phys_to_virt()

[Russell King - ARM Linux]

On Tue, Jan 04, 2011 at 12:50:28PM -0500, Nicolas Pitre wrote:
> On Tue, 4 Jan 2011, Russell King - ARM Linux wrote:
> > Our aims are different then.  My aim is to move the code to a point where
> > it works for _everyone_ it possibly can - and theoretically that's every
> > platform except:
> > 
> > 1. MSM due to their PHYS_OFFSET being 2MB aligned, rather than the more
> >    normal 256MB alignment.
> > 2. Anyone with complex V:P mappings
> 
> I completely agree with that goal.  But I'd prefer for those platforms 
> which are not yet supported by this feature not to be able to compile 
> rather than silently ignore the feature and not behave as expected.
> 
> > (1) is dealt with easily by a dependency in the configuration preventing
> > the option being visible.  (2) is dealt with at runtime by ignoring the
> > configuration option - resulting in the p2v tables being empty.  The end
> > result will still run on the platform, but it won't do the relocation
> > stuff.  (2) could also be dealt with by adding the necessary dependencies
> > to the configuration option which is the longer term solution.
> 
> Since (2) is not supported yet with this config option selected, I think 
> it is best to simply #error the build.
> 
> > Lastly, marking the option as 'EXPERIMENTAL' is there to convey that it
> > may not work for everyone, and people should expect things not to work if
> > they enable such an option (and report when that's the case.)
> 
> Sure, hence my #error in the patch which is even easier to diagnose and 
> self explanatory.

You're making a mountain out of a mole hill.  At present, there is one
platform which defines its own complex v:p mapping and that is Realview,
but only when sparsemem is enabled.  As already mentioned, MSM is the
only other platform which can't use this method.  So that's a simple
dependency line against the config.

The other breakages are use of PHYS_OFFSET as an initializer which is a
build-error inducing failure, and adopting the approach I outlined in my
4 patch set results in many of those going away before we get support for
this merged - even better, if PHYS_OFFSET were always to be variable-like,
then we'd stop any new uses even appearing.

> And in fact I think that this would indeed be simpler to just fall back 
> to a global variable for PHYS_OFFSET when a platform defines its own 
> p2v/v2p mapping.  This way, the goal of this feature would be 
> universally available.

Not really.  Platforms define their own mapping because it's not a simple
addition or subtraction, but because it's a complex non-linear conversion.

#define __phys_to_virt(phys)                                            \
        ((phys) >= 0x80000000 ? (phys) - 0x80000000 + PAGE_OFFSET2 :    \
         (phys) >= 0x20000000 ? (phys) - 0x20000000 + PAGE_OFFSET1 :    \
         (phys) + PAGE_OFFSET)

#define __virt_to_phys(virt)                                            \
         ((virt) >= PAGE_OFFSET2 ? (virt) - PAGE_OFFSET2 + 0x80000000 : \
          (virt) >= PAGE_OFFSET1 ? (virt) - PAGE_OFFSET1 + 0x20000000 : \
          (virt) - PAGE_OFFSET)

This doesn't lend itself in any way to a variable-based PHYS_OFFSET, and
could never be subsituted code-wise at run time without significant
effort.

In fact, platforms which have complex V:P mappings can _never_ be a part
of a kernel which has this feature enabled.

[PATCH 1/4] ARM: runtime patching of __virt_to_phys() and __phys_to_virt()

[David Brown]

On Tue, Jan 04 2011, Russell King - ARM Linux wrote:

> On Tue, Jan 04, 2011 at 12:50:28PM -0500, Nicolas Pitre wrote:

> You're making a mountain out of a mole hill.  At present, there is one
> platform which defines its own complex v:p mapping and that is Realview,
> but only when sparsemem is enabled.  As already mentioned, MSM is the
> only other platform which can't use this method.  So that's a simple
> dependency line against the config.

This is unfortunate, however, for us, since it would keep us having all
of our targets having to be built exclusively.

Any idea how much it would hurt other targets to have the
__virt_to_phys() and __phys_to_virt() have a 16-bit fixup, even if only
the upper 8 bits are used?

David

[PATCH 1/4] ARM: runtime patching of __virt_to_phys() and __phys_to_virt()

[Nicolas Pitre]

On Tue, 4 Jan 2011, David Brown wrote:

> On Tue, Jan 04 2011, Russell King - ARM Linux wrote:
> 
> > On Tue, Jan 04, 2011 at 12:50:28PM -0500, Nicolas Pitre wrote:
> 
> > You're making a mountain out of a mole hill.  At present, there is one
> > platform which defines its own complex v:p mapping and that is Realview,
> > but only when sparsemem is enabled.  As already mentioned, MSM is the
> > only other platform which can't use this method.  So that's a simple
> > dependency line against the config.
> 
> This is unfortunate, however, for us, since it would keep us having all
> of our targets having to be built exclusively.
> 
> Any idea how much it would hurt other targets to have the
> __virt_to_phys() and __phys_to_virt() have a 16-bit fixup, even if only
> the upper 8 bits are used?

Probably not that much.  But let's make it work satisfactorily for the 
general case first, and then we might consider and test variations that 
could accommodate msm.


Nicolas

[PATCH 1/4] ARM: runtime patching of __virt_to_phys() and __phys_to_virt()

[David Brown]

On Tue, Jan 04 2011, Nicolas Pitre wrote:

> On Tue, 4 Jan 2011, David Brown wrote:
>
>> Any idea how much it would hurt other targets to have the
>> __virt_to_phys() and __phys_to_virt() have a 16-bit fixup, even if only
>> the upper 8 bits are used?
>
> Probably not that much.  But let's make it work satisfactorily for the 
> general case first, and then we might consider and test variations that 
> could accommodate msm.

Sounds like a good plan.  We've got quite a few other things to clean up
before we can build for more than one arch anyway.

David

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[PATCH 1/4] ARM: runtime patching of __virt_to_phys() and __phys_to_virt()

[Russell King - ARM Linux]

On Tue, Jan 04, 2011 at 11:00:05AM -0800, David Brown wrote:
> On Tue, Jan 04 2011, Nicolas Pitre wrote:
> 
> > On Tue, 4 Jan 2011, David Brown wrote:
> >
> >> Any idea how much it would hurt other targets to have the
> >> __virt_to_phys() and __phys_to_virt() have a 16-bit fixup, even if only
> >> the upper 8 bits are used?
> >
> > Probably not that much.  But let's make it work satisfactorily for the 
> > general case first, and then we might consider and test variations that 
> > could accommodate msm.
> 
> Sounds like a good plan.  We've got quite a few other things to clean up
> before we can build for more than one arch anyway.

Actually, we can do this quite easily.  Having a config symbol which
gets enabled when MSM is added, which then enables two consecutive
__pv_fixups gives us 16-bits to play with.

As I originally intended with my implementation, the value field of the
instruction can be used to identify what this fixup is about - and we
can put that to use by selecting either bits 31-24 or 23-16 of the
offset value.

This still allows optimizations to happen with instruction scheduling -
eg, for the MSM kernel I've just built with this enabled:

c000b320:       e2844001        add     r4, r4, #1      ; 0x1
c000b324:       e59f3264        ldr     r3, [pc, #612]  ; c000b590 <setup_arch+0x690>
c000b328:       e2844000        add     r4, r4, #0      ; 0x0
c000b32c:       e2833001        add     r3, r3, #1      ; 0x1
c000b330:       e59f5228        ldr     r5, [pc, #552]  ; c000b560 <setup_arch+0x660>
c000b334:       e2833000        add     r3, r3, #0      ; 0x0

The first and second adds comprise one translation, the second pair are
a second translation, and as you can see, the compiler scheduled the
loads inbetween.

[PATCH 1/4] ARM: runtime patching of __virt_to_phys() and __phys_to_virt()

[Nicolas Pitre]

On Tue, 4 Jan 2011, Russell King - ARM Linux wrote:

> On Tue, Jan 04, 2011 at 12:50:28PM -0500, Nicolas Pitre wrote:
> > On Tue, 4 Jan 2011, Russell King - ARM Linux wrote:
> > > Our aims are different then.  My aim is to move the code to a point where
> > > it works for _everyone_ it possibly can - and theoretically that's every
> > > platform except:
> > > 
> > > 1. MSM due to their PHYS_OFFSET being 2MB aligned, rather than the more
> > >    normal 256MB alignment.
> > > 2. Anyone with complex V:P mappings
> > 
> > I completely agree with that goal.  But I'd prefer for those platforms 
> > which are not yet supported by this feature not to be able to compile 
> > rather than silently ignore the feature and not behave as expected.
> > 
> > > (1) is dealt with easily by a dependency in the configuration preventing
> > > the option being visible.  (2) is dealt with at runtime by ignoring the
> > > configuration option - resulting in the p2v tables being empty.  The end
> > > result will still run on the platform, but it won't do the relocation
> > > stuff.  (2) could also be dealt with by adding the necessary dependencies
> > > to the configuration option which is the longer term solution.
> > 
> > Since (2) is not supported yet with this config option selected, I think 
> > it is best to simply #error the build.
> > 
> > > Lastly, marking the option as 'EXPERIMENTAL' is there to convey that it
> > > may not work for everyone, and people should expect things not to work if
> > > they enable such an option (and report when that's the case.)
> > 
> > Sure, hence my #error in the patch which is even easier to diagnose and 
> > self explanatory.
> 
> You're making a mountain out of a mole hill.  At present, there is one
> platform which defines its own complex v:p mapping and that is Realview,
> but only when sparsemem is enabled.  As already mentioned, MSM is the
> only other platform which can't use this method.  So that's a simple
> dependency line against the config.

OK, agreed.

The #error might still be a good idea as a stop gate if some future 
platforms also define a complex v:p mapping and the Kbuild dependency is 
missed, just like we have things like:

#if __LINUX_ARM_ARCH__ < 6
#error SMP not supported on pre-ARMv6 CPUs
#endif

> The other breakages are use of PHYS_OFFSET as an initializer which is a
> build-error inducing failure, and adopting the approach I outlined in my
> 4 patch set results in many of those going away before we get support for
> this merged - even better, if PHYS_OFFSET were always to be variable-like,
> then we'd stop any new uses even appearing.

Completely agreed.

> > And in fact I think that this would indeed be simpler to just fall back 
> > to a global variable for PHYS_OFFSET when a platform defines its own 
> > p2v/v2p mapping.  This way, the goal of this feature would be 
> > universally available.
> 
> Not really.  Platforms define their own mapping because it's not a simple
> addition or subtraction, but because it's a complex non-linear conversion.
> 
> #define __phys_to_virt(phys)                                            \
>         ((phys) >= 0x80000000 ? (phys) - 0x80000000 + PAGE_OFFSET2 :    \
>          (phys) >= 0x20000000 ? (phys) - 0x20000000 + PAGE_OFFSET1 :    \
>          (phys) + PAGE_OFFSET)
> 
> #define __virt_to_phys(virt)                                            \
>          ((virt) >= PAGE_OFFSET2 ? (virt) - PAGE_OFFSET2 + 0x80000000 : \
>           (virt) >= PAGE_OFFSET1 ? (virt) - PAGE_OFFSET1 + 0x20000000 : \
>           (virt) - PAGE_OFFSET)

Indeed, no point trying to support that.

> This doesn't lend itself in any way to a variable-based PHYS_OFFSET, and
> could never be subsituted code-wise at run time without significant
> effort.
> 
> In fact, platforms which have complex V:P mappings can _never_ be a part
> of a kernel which has this feature enabled.

I said the same already, perhaps not as strongly.

If we _wanted_ to support such platforms in a common kernel binary, then 
we would have to fall back to function pointers.  The end goal would be 
accomplished through a different implementation.  I don't think it is 
worth going there though.


Nicolas

[PATCH 2/4] ARM: make PHYS_OFFSET actually variable

[Nicolas Pitre]

Previous patch made the implementation of __virt_to_phys() and
__phys_to_virt() assuming PHYS_OFFSET was variable.  Let's make
PHYS_OFFSET actually variable by defining it in terms of __virt_to_phys().

There is a catch with this approach, as PHYS_OFFSET cannot be used as
a static initializer anymore.  Only one such case was found in generic
code and removed, and that is in the declaration of the init_tags
structure (those who wish to use CONFIG_ARM_PATCH_PHYS_VIRT are better
not to rely on that init_tags  default at all anyway, or they'll get
what they deserve).

Signed-off-by: Nicolas Pitre <nicolas.pitre@linaro.org>

PAGE_OFFSET)
---
 arch/arm/include/asm/memory.h |    3 +++
 arch/arm/kernel/setup.c       |    2 ++
 2 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/arch/arm/include/asm/memory.h b/arch/arm/include/asm/memory.h
index 2783ce2..acc3126 100644
--- a/arch/arm/include/asm/memory.h
+++ b/arch/arm/include/asm/memory.h
@@ -182,6 +182,9 @@ static inline unsigned long __phys_to_virt(unsigned long x)
 	return t;
 }
 
+#undef PHYS_OFFSET
+#define PHYS_OFFSET		__virt_to_phys(PAGE_OFFSET)
+
 #else
 #define __virt_to_phys(x)	((x) - PAGE_OFFSET + PHYS_OFFSET)
 #define __phys_to_virt(x)	((x) - PHYS_OFFSET + PAGE_OFFSET)
diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c
index 336f14e..7502bc1 100644
--- a/arch/arm/kernel/setup.c
+++ b/arch/arm/kernel/setup.c
@@ -703,8 +703,10 @@ static struct init_tags {
 } init_tags __initdata = {
 	{ tag_size(tag_core), ATAG_CORE },
 	{ 1, PAGE_SIZE, 0xff },
+#ifndef CONFIG_ARM_PATCH_PHYS_VIRT
 	{ tag_size(tag_mem32), ATAG_MEM },
 	{ MEM_SIZE, PHYS_OFFSET },
+#endif
 	{ 0, ATAG_NONE }
 };
 
-- 
1.7.3.2.193.g78bbb

[PATCH 2/4] ARM: make PHYS_OFFSET actually variable

[Russell King - ARM Linux]

On Tue, Jan 04, 2011 at 03:20:06AM -0500, Nicolas Pitre wrote:
> diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c
> index 336f14e..7502bc1 100644
> --- a/arch/arm/kernel/setup.c
> +++ b/arch/arm/kernel/setup.c
> @@ -703,8 +703,10 @@ static struct init_tags {
>  } init_tags __initdata = {
>  	{ tag_size(tag_core), ATAG_CORE },
>  	{ 1, PAGE_SIZE, 0xff },
> +#ifndef CONFIG_ARM_PATCH_PHYS_VIRT
>  	{ tag_size(tag_mem32), ATAG_MEM },
>  	{ MEM_SIZE, PHYS_OFFSET },
> +#endif

Removing the definition is not really on - it's there as a fallback and
removing fallbacks is not a good idea unless you can prove beyond doubt
that it'll never be used.

As that depends on the boot loader, it's something that can never be
proven.  So, arrange for it to be initialized at runtime, just like I
will be doing in my revised version I'm working on which I detailed in
a previous email.

[PATCH 2/4] ARM: make PHYS_OFFSET actually variable

[Nicolas Pitre]

On Tue, 4 Jan 2011, Russell King - ARM Linux wrote:

> On Tue, Jan 04, 2011 at 03:20:06AM -0500, Nicolas Pitre wrote:
> > diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c
> > index 336f14e..7502bc1 100644
> > --- a/arch/arm/kernel/setup.c
> > +++ b/arch/arm/kernel/setup.c
> > @@ -703,8 +703,10 @@ static struct init_tags {
> >  } init_tags __initdata = {
> >  	{ tag_size(tag_core), ATAG_CORE },
> >  	{ 1, PAGE_SIZE, 0xff },
> > +#ifndef CONFIG_ARM_PATCH_PHYS_VIRT
> >  	{ tag_size(tag_mem32), ATAG_MEM },
> >  	{ MEM_SIZE, PHYS_OFFSET },
> > +#endif
> 
> Removing the definition is not really on - it's there as a fallback and
> removing fallbacks is not a good idea unless you can prove beyond doubt
> that it'll never be used.
> 
> As that depends on the boot loader, it's something that can never be
> proven.  So, arrange for it to be initialized at runtime, just like I
> will be doing in my revised version I'm working on which I detailed in
> a previous email.

Agreed.

I'll wait for that revised version before working on this further.


Nicolas

[PATCH 3/4] ARM: module support for CONFIG_ARM_PATCH_PHYS_VIRT

[Nicolas Pitre]

Thanks to Russell King for his contribution to this patch.

Signed-off-by: Nicolas Pitre <nicolas.pitre@linaro.org>
---
 arch/arm/include/asm/memory.h |    4 ++++
 arch/arm/kernel/head.S        |   35 +++++++++++++++++++++++++++++------
 arch/arm/kernel/module.c      |   13 +++++++++++++
 3 files changed, 46 insertions(+), 6 deletions(-)

diff --git a/arch/arm/include/asm/memory.h b/arch/arm/include/asm/memory.h
index acc3126..f83be97 100644
--- a/arch/arm/include/asm/memory.h
+++ b/arch/arm/include/asm/memory.h
@@ -185,6 +185,10 @@ static inline unsigned long __phys_to_virt(unsigned long x)
 #undef PHYS_OFFSET
 #define PHYS_OFFSET		__virt_to_phys(PAGE_OFFSET)
 
+extern void fixup_pv_table(void *table_start, void *table_end, long v2p_offset);
+#define fixup_pv_table(start, end) \
+	fixup_pv_table(start, end, PHYS_OFFSET - PAGE_OFFSET)
+
 #else
 #define __virt_to_phys(x)	((x) - PAGE_OFFSET + PHYS_OFFSET)
 #define __phys_to_virt(x)	((x) - PHYS_OFFSET + PAGE_OFFSET)
diff --git a/arch/arm/kernel/head.S b/arch/arm/kernel/head.S
index eaaf0ad..621c792 100644
--- a/arch/arm/kernel/head.S
+++ b/arch/arm/kernel/head.S
@@ -447,12 +447,22 @@ __fixup_pv_table:
 	adr	r0, 1f
 	ldmia	r0, {r3-r5}
 	sub	r3, r0, r3	@ PHYS_OFFSET - PAGE_OFFSET
+	add	r4, r4, r3
+	add	r5, r5, r3
 	mov	r6, r3, lsr #24	@ constant for add/sub instructions
 	teq	r3, r6, lsl #24 @ must be 16MiB aligned
 	bne	__error
+	b	__fixup_pv_table_loop
+ENDPROC(__fixup_phys_virt)
+
+1:	.word	.
+	.word	__pv_table_begin
+	.word	__pv_table_end
+
+	.pushsection .text
+
+ENTRY(__fixup_pv_table_loop)
 	orr	r6, r6, #0x400	@ mask in rotate right 8 bits
-	add	r4, r4, r3
-	add	r5, r5, r3
 2:	cmp	r4, r5
 	ldrlo	r7, [r4], #4
 	ldrlo	ip, [r7, r3]
@@ -461,11 +471,24 @@ __fixup_pv_table:
 	strlo	ip, [r7, r3]
 	blo	2b
 	mov	pc, lr
-ENDPROC(__fixup_phys_virt)
+ENDPROC(__fixup_phys_virt_loop)
+
+/*
+ * This is the C callable version used to fixup modules.
+ * void fixup_pv_table(void *table_start, void *table_end, long v2p_offset)
+ */
+ENTRY(fixup_pv_table)
+	stmfd	sp!, {r4 - r7, lr}
+	mov	r3, #0		@ offset (zero as we're in virtual space)
+	mov	r4, r0		@ loop start
+	mov	r5, r1		@ loop end
+	mov     r6, r2, lsr #24	@ constant for add/sub instructions		
+	bl      __fixup_pv_table_loop
+	ldmfd	sp!, {r4 - r7, pc}
+ENDPROC(fixup_pv_table)
+
+	.popsection
 
-1:	.word	.
-	.word	__pv_table_begin
-	.word	__pv_table_end
 #endif
 
 #include "head-common.S"
diff --git a/arch/arm/kernel/module.c b/arch/arm/kernel/module.c
index d9bd786..6fef8d2 100644
--- a/arch/arm/kernel/module.c
+++ b/arch/arm/kernel/module.c
@@ -329,6 +329,19 @@ int
 module_finalize(const Elf32_Ehdr *hdr, const Elf_Shdr *sechdrs,
 		struct module *module)
 {
+#ifdef CONFIG_ARM_PATCH_PHYS_VIRT
+	char *secstrings = (void *)hdr + sechdrs[hdr->e_shstrndx].sh_offset;
+        const Elf_Shdr *s;
+
+	for (s = sechdrs; s < sechdrs + hdr->e_shnum; s++)
+		if (!strcmp(".pv_table", secstrings + s->sh_name)) {
+			void *pv_table_begin = (void *)s->sh_addr;
+			void *pv_table_end = pv_table_begin + s->sh_size;
+			fixup_pv_table(pv_table_begin, pv_table_end);
+			break;
+		}
+#endif
+
 	register_unwind_tables(module);
 	return 0;
 }
-- 
1.7.3.2.193.g78bbb

[PATCH 3/4] ARM: module support for CONFIG_ARM_PATCH_PHYS_VIRT

[Russell King - ARM Linux]

On Tue, Jan 04, 2011 at 03:20:07AM -0500, Nicolas Pitre wrote:
> Thanks to Russell King for his contribution to this patch.
> 
> Signed-off-by: Nicolas Pitre <nicolas.pitre@linaro.org>

This should all be part of the patch introducing the patching - without
it you will end up with a kernel which has the dynamic P:V translation
support but the modules will be buggered.

[PATCH 4/4] ARM: support for Thumb-2 instructions with CONFIG_ARM_PATCH_PHYS_VIRT

[Nicolas Pitre]

Signed-off-by: Nicolas Pitre <nicolas.pitre@linaro.org>
---
 arch/arm/Kconfig              |    3 +--
 arch/arm/include/asm/memory.h |    5 +++--
 arch/arm/kernel/head.S        |   31 +++++++++++++++++++++++++++----
 3 files changed, 31 insertions(+), 8 deletions(-)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 136ed9b..feb374a 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -189,8 +189,7 @@ config VECTORS_BASE
 
 config ARM_PATCH_PHYS_VIRT
 	bool
-	depends on EXPERIMENTAL
-	depends on !XIP && !THUMB2_KERNEL
+	depends on !XIP && EXPERIMENTAL
 
 source "init/Kconfig"
 
diff --git a/arch/arm/include/asm/memory.h b/arch/arm/include/asm/memory.h
index f83be97..80a939d 100644
--- a/arch/arm/include/asm/memory.h
+++ b/arch/arm/include/asm/memory.h
@@ -157,14 +157,15 @@
 #error "this machine configuration uses complex __virt_to_phys/__phys_to_virt and cannot use CONFIG_ARM_PATCH_PHYS_VIRT"
 #endif
 
+/* the stub constant 0xff000000 is used to force the required insn encoding */
 #define __pv_stub(from,to,instr)			\
 	__asm__(					\
-	"1:	" instr "\t%0, %1, %2\n"		\
+	"1:	" instr "\t%0, %1, #0xff000000\n"	\
 	"	.pushsection .pv_table,\"a\"\n"		\
 	"	.long	1b\n"				\
 	"	.popsection"				\
 	: "=r" (to)					\
-	: "r" (from), "I" (1))
+	: "r" (from))
 
 static inline unsigned long __virt_to_phys(unsigned long x)
 {
diff --git a/arch/arm/kernel/head.S b/arch/arm/kernel/head.S
index 621c792..397271d 100644
--- a/arch/arm/kernel/head.S
+++ b/arch/arm/kernel/head.S
@@ -447,11 +447,11 @@ __fixup_pv_table:
 	adr	r0, 1f
 	ldmia	r0, {r3-r5}
 	sub	r3, r0, r3	@ PHYS_OFFSET - PAGE_OFFSET
+	movs	r6, r3, lsl #8	@ must be 16MiB aligned
+	bne	__error
 	add	r4, r4, r3
 	add	r5, r5, r3
-	mov	r6, r3, lsr #24	@ constant for add/sub instructions
-	teq	r3, r6, lsl #24 @ must be 16MiB aligned
-	bne	__error
+	mov	r6, r3
 	b	__fixup_pv_table_loop
 ENDPROC(__fixup_phys_virt)
 
@@ -462,6 +462,8 @@ ENDPROC(__fixup_phys_virt)
 	.pushsection .text
 
 ENTRY(__fixup_pv_table_loop)
+#ifndef CONFIG_THUMB2_KERNEL
+	mov	r6, r6, lsr #24	@ constant for add/sub instructions
 	orr	r6, r6, #0x400	@ mask in rotate right 8 bits
 2:	cmp	r4, r5
 	ldrlo	r7, [r4], #4
@@ -471,6 +473,27 @@ ENTRY(__fixup_pv_table_loop)
 	strlo	ip, [r7, r3]
 	blo	2b
 	mov	pc, lr
+#else
+	teq	r6, #0
+	beq	2f
+	clz	r7, r6
+	lsr	r6, #24
+	lsl	r6, r7
+	bic	r6, r6, #0x3080
+	lsrs	r7, #1
+	orrcs	r6, r6, #0x80
+	orr	r6, r6, r7, lsl #12
+	orr	r6, r6, #0x4000
+2:	cmp	r4, r5
+	bxhs	lr
+	ldr	r7, [r4], #4
+	add	r7, r3
+	ldr	ip, [r7, #2]
+	and	ip, ip, #0x0f00
+	orr	ip, ip, r6
+	str	ip, [r7, #2]
+	b	2b
+#endif
 ENDPROC(__fixup_phys_virt_loop)
 
 /*
@@ -482,7 +505,7 @@ ENTRY(fixup_pv_table)
 	mov	r3, #0		@ offset (zero as we're in virtual space)
 	mov	r4, r0		@ loop start
 	mov	r5, r1		@ loop end
-	mov     r6, r2, lsr #24	@ constant for add/sub instructions		
+	mov     r6, r2		@ PHYS_OFFSET - PAGE_OFFSET
 	bl      __fixup_pv_table_loop
 	ldmfd	sp!, {r4 - r7, pc}
 ENDPROC(fixup_pv_table)
-- 
1.7.3.2.193.g78bbb

[PATCH 4/4] ARM: support for Thumb-2 instructions with CONFIG_ARM_PATCH_PHYS_VIRT

[Dave Martin]

Hi,

On Tue, Jan 4, 2011 at 2:20 AM, Nicolas Pitre <nico@fluxnic.net> wrote:
> Signed-off-by: Nicolas Pitre <nicolas.pitre@linaro.org>
> ---
>  arch/arm/Kconfig              |    3 +--
>  arch/arm/include/asm/memory.h |    5 +++--
>  arch/arm/kernel/head.S        |   31 +++++++++++++++++++++++++++----
>  3 files changed, 31 insertions(+), 8 deletions(-)
>
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index 136ed9b..feb374a 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -189,8 +189,7 @@ config VECTORS_BASE
>
>  config ARM_PATCH_PHYS_VIRT
>        bool
> -       depends on EXPERIMENTAL
> -       depends on !XIP && !THUMB2_KERNEL
> +       depends on !XIP && EXPERIMENTAL
>
>  source "init/Kconfig"
>
> diff --git a/arch/arm/include/asm/memory.h b/arch/arm/include/asm/memory.h
> index f83be97..80a939d 100644
> --- a/arch/arm/include/asm/memory.h
> +++ b/arch/arm/include/asm/memory.h
> @@ -157,14 +157,15 @@
>  #error "this machine configuration uses complex __virt_to_phys/__phys_to_virt and cannot use CONFIG_ARM_PATCH_PHYS_VIRT"
>  #endif
>
> +/* the stub constant 0xff000000 is used to force the required insn encoding */
>  #define __pv_stub(from,to,instr)                       \
>        __asm__(                                        \
> -       "1:     " instr "\t%0, %1, %2\n"                \
> +       "1:     " instr "\t%0, %1, #0xff000000\n"       \
>        "       .pushsection .pv_table,\"a\"\n"         \
>        "       .long   1b\n"                           \
>        "       .popsection"                            \
>        : "=r" (to)                                     \
> -       : "r" (from), "I" (1))
> +       : "r" (from))
>
>  static inline unsigned long __virt_to_phys(unsigned long x)
>  {
> diff --git a/arch/arm/kernel/head.S b/arch/arm/kernel/head.S
> index 621c792..397271d 100644
> --- a/arch/arm/kernel/head.S
> +++ b/arch/arm/kernel/head.S
> @@ -447,11 +447,11 @@ __fixup_pv_table:
>        adr     r0, 1f
>        ldmia   r0, {r3-r5}
>        sub     r3, r0, r3      @ PHYS_OFFSET - PAGE_OFFSET
> +       movs    r6, r3, lsl #8  @ must be 16MiB aligned
> +       bne     __error
>        add     r4, r4, r3
>        add     r5, r5, r3
> -       mov     r6, r3, lsr #24 @ constant for add/sub instructions
> -       teq     r3, r6, lsl #24 @ must be 16MiB aligned
> -       bne     __error
> +       mov     r6, r3
>        b       __fixup_pv_table_loop
>  ENDPROC(__fixup_phys_virt)
>
> @@ -462,6 +462,8 @@ ENDPROC(__fixup_phys_virt)
>        .pushsection .text
>
>  ENTRY(__fixup_pv_table_loop)
> +#ifndef CONFIG_THUMB2_KERNEL
> +       mov     r6, r6, lsr #24 @ constant for add/sub instructions
>        orr     r6, r6, #0x400  @ mask in rotate right 8 bits
>  2:     cmp     r4, r5
>        ldrlo   r7, [r4], #4
> @@ -471,6 +473,27 @@ ENTRY(__fixup_pv_table_loop)
>        strlo   ip, [r7, r3]
>        blo     2b
>        mov     pc, lr
> +#else
> +       teq     r6, #0
> +       beq     2f
> +       clz     r7, r6
> +       lsr     r6, #24
> +       lsl     r6, r7
> +       bic     r6, r6, #0x3080

Should bits 12-13 of r6 ever be nonzero here? The code already throws
an error of the p2v offset is not a multiple of 16 MiB; i.e., (r6 &
~0xff000000) == 0, so r6 >> (24 - clz(r6)) must be in the range
0..0xff.

Of course, clearing the extra bits again doesn't matter...

> +       lsrs    r7, #1
> +       orrcs   r6, r6, #0x80
> +       orr     r6, r6, r7, lsl #12
> +       orr     r6, r6, #0x4000
> +2:     cmp     r4, r5
> +       bxhs    lr
> +       ldr     r7, [r4], #4
> +       add     r7, r3
> +       ldr     ip, [r7, #2]
> +       and     ip, ip, #0x0f00
> +       orr     ip, ip, r6
> +       str     ip, [r7, #2]
> +       b       2b
> +#endif
>  ENDPROC(__fixup_phys_virt_loop)
>
>  /*
> @@ -482,7 +505,7 @@ ENTRY(fixup_pv_table)
>        mov     r3, #0          @ offset (zero as we're in virtual space)
>        mov     r4, r0          @ loop start
>        mov     r5, r1          @ loop end
> -       mov     r6, r2, lsr #24 @ constant for add/sub instructions
> +       mov     r6, r2          @ PHYS_OFFSET - PAGE_OFFSET
>        bl      __fixup_pv_table_loop
>        ldmfd   sp!, {r4 - r7, pc}
>  ENDPROC(fixup_pv_table)

I've not been able to test this code yet, but it looks to me like it
should work for the Thumb-2 case.

Cheers
---Dave

[PATCH 4/4] ARM: support for Thumb-2 instructions with CONFIG_ARM_PATCH_PHYS_VIRT

[Nicolas Pitre]

On Mon, 10 Jan 2011, Dave Martin wrote:

> Hi,
> 
> On Tue, Jan 4, 2011 at 2:20 AM, Nicolas Pitre <nico@fluxnic.net> wrote:
> > @@ -462,6 +462,8 @@ ENDPROC(__fixup_phys_virt)
> >        .pushsection .text
> >
> >  ENTRY(__fixup_pv_table_loop)
> > +#ifndef CONFIG_THUMB2_KERNEL
> > +       mov     r6, r6, lsr #24 @ constant for add/sub instructions
> >        orr     r6, r6, #0x400  @ mask in rotate right 8 bits
> >  2:     cmp     r4, r5
> >        ldrlo   r7, [r4], #4
> > @@ -471,6 +473,27 @@ ENTRY(__fixup_pv_table_loop)
> >        strlo   ip, [r7, r3]
> >        blo     2b
> >        mov     pc, lr
> > +#else
> > +       teq     r6, #0
> > +       beq     2f
> > +       clz     r7, r6
> > +       lsr     r6, #24
> > +       lsl     r6, r7
> > +       bic     r6, r6, #0x3080
> 
> Should bits 12-13 of r6 ever be nonzero here? The code already throws
> an error of the p2v offset is not a multiple of 16 MiB; i.e., (r6 &
> ~0xff000000) == 0, so r6 >> (24 - clz(r6)) must be in the range
> 0..0xff.

Yes, they should be cleared already.  This is probably a leftover from a 
previous version.

I need to rework this part entirely now anyway to go on top of Russell's 
latest version.

> I've not been able to test this code yet, but it looks to me like it
> should work for the Thumb-2 case.

Thanks for looking at it.


Nicolas

[PATCH 4/4] ARM: support for Thumb-2 instructions with CONFIG_ARM_PATCH_PHYS_VIRT

[Russell King - ARM Linux]

On Mon, Jan 10, 2011 at 05:45:44PM -0500, Nicolas Pitre wrote:
> I need to rework this part entirely now anyway to go on top of Russell's 
> latest version.

I've made a few changes recently to it to fix a few quirks.  Once the
merge window is over, I'll push out new patches and tree.  Until then
it wouldn't be fair on SFR/linux-next to push new code stuff out during
the merge window.

[PATCH 4/4] ARM: support for Thumb-2 instructions with CONFIG_ARM_PATCH_PHYS_VIRT

[Nicolas Pitre]

On Mon, 10 Jan 2011, Russell King - ARM Linux wrote:

> On Mon, Jan 10, 2011 at 05:45:44PM -0500, Nicolas Pitre wrote:
> > I need to rework this part entirely now anyway to go on top of Russell's 
> > latest version.
> 
> I've made a few changes recently to it to fix a few quirks.  Once the
> merge window is over, I'll push out new patches and tree.  Until then
> it wouldn't be fair on SFR/linux-next to push new code stuff out during
> the merge window.

Maybe you could push those into a branch which is not pulled into 
linux-next?


Nicolas

[PATCH 0/4] variable PHYS_OFFSET support

[Russell King - ARM Linux]

On Tue, Jan 04, 2011 at 03:20:04AM -0500, Nicolas Pitre wrote:
> So here it is, in a form which I think should be ready for merging.
> 
> ARM mode is well tested.
> Thumb2 mode is only compile tested at the moment.

I don't believe this is ready for merging - there's still work to be
done with the early assembly code using PHYS_OFFSET.  That currently
doesn't _appear_ to be a problem because it still uses the old platform
specific setting of PHYS_OFFSET - and that needs careful thought as:

#if (PHYS_OFFSET & 0x001fffff)
#error "PHYS_OFFSET must be at an even 2MiB boundary!"
#endif

has been eroded from 16MB downwards to 2MB over time because of
platform setups - eg, MSM for example.

As we're having to store the p:v offset (it's cheaper to store it than
your way of making PHYS_OFFSET depend on the translation and PAGE_OFFSET)
then we might as well also store what we think is the physical offset
and use that for PHYS_OFFSET too once the assembly code issue has been
sorted.

So, what I suggest is a different patch ordering:

1. Fix the assembly code not to rely upon a fixed PHYS_OFFSET (is this
   even possible with the 2MB requirement?)

2. Fix the initializers in platform code (ignoring MSM).

3. Rename PHYS_OFFSET to be PLAT_PHYS_OFFSET in platform memory.h files,
   defining PHYS_OFFSET in asm/memory.h to be PLAT_PHYS_OFFSET.  (We
   could make PHYS_OFFSET at this point be a variable, which'd stop any
   new initializers using PHYS_OFFSET - which I think would be beneficial
   anyway.)

4. Introduce P2V patching _with_ the module support.  With this patch
   create __pv_phys_offset, and if P2V patching is enabled, redefine
   PHYS_OFFSET to be this variable.  Note that __pv_phys_offset would
   need to be exported to modules.

So, I don't think it's ready for this coming merge window.

[PATCH 0/4] variable PHYS_OFFSET support

[Nicolas Pitre]

On Tue, 4 Jan 2011, Russell King - ARM Linux wrote:

> So, I don't think it's ready for this coming merge window.

OK, agreed.

We could also take the time to make the kdump kernel benefit from this 
feature at the same time, removing the need from a special build of the 
kernel for this purpose, which would be a direct application.


Nicolas