| Message ID | alpine.LFD.2.10.1311252254200.9667@knanqh.ubzr (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Nov 25, 2013 at 10:56:25PM -0500, Nicolas Pitre wrote: > What about simply doing the following instead, which I'm sure used to > work properly at some point: > > diff --git a/arch/arm/include/asm/memory.h b/arch/arm/include/asm/memory.h > index 9ecccc8650..2b8b8d3236 100644 > --- a/arch/arm/include/asm/memory.h > +++ b/arch/arm/include/asm/memory.h > @@ -239,6 +239,14 @@ static inline unsigned long __phys_to_virt(phys_addr_t x) > > #else > > +#ifndef PHYS_OFFSET > +#ifdef PLAT_PHYS_OFFSET > +#define PHYS_OFFSET PLAT_PHYS_OFFSET > +#else > +#define PHYS_OFFSET UL(CONFIG_PHYS_OFFSET) > +#endif > +#endif > + > static inline phys_addr_t __virt_to_phys(unsigned long x) > { > return (phys_addr_t)x - PAGE_OFFSET + PHYS_OFFSET; > @@ -253,14 +261,6 @@ static inline unsigned long __phys_to_virt(phys_addr_t x) > #endif > #endif /* __ASSEMBLY__ */ > > -#ifndef PHYS_OFFSET > -#ifdef PLAT_PHYS_OFFSET > -#define PHYS_OFFSET PLAT_PHYS_OFFSET > -#else > -#define PHYS_OFFSET UL(CONFIG_PHYS_OFFSET) > -#endif > -#endif And that makes PHYS_OFFSET undefined to assembly code - and we have references to it from said code.
On Tue, 26 Nov 2013, Russell King - ARM Linux wrote: > On Mon, Nov 25, 2013 at 10:56:25PM -0500, Nicolas Pitre wrote: > > What about simply doing the following instead, which I'm sure used to > > work properly at some point: > > > > diff --git a/arch/arm/include/asm/memory.h b/arch/arm/include/asm/memory.h > > index 9ecccc8650..2b8b8d3236 100644 > > --- a/arch/arm/include/asm/memory.h > > +++ b/arch/arm/include/asm/memory.h > > @@ -239,6 +239,14 @@ static inline unsigned long __phys_to_virt(phys_addr_t x) > > > > #else > > > > +#ifndef PHYS_OFFSET > > +#ifdef PLAT_PHYS_OFFSET > > +#define PHYS_OFFSET PLAT_PHYS_OFFSET > > +#else > > +#define PHYS_OFFSET UL(CONFIG_PHYS_OFFSET) > > +#endif > > +#endif > > + > > static inline phys_addr_t __virt_to_phys(unsigned long x) > > { > > return (phys_addr_t)x - PAGE_OFFSET + PHYS_OFFSET; > > @@ -253,14 +261,6 @@ static inline unsigned long __phys_to_virt(phys_addr_t x) > > #endif > > #endif /* __ASSEMBLY__ */ > > > > -#ifndef PHYS_OFFSET > > -#ifdef PLAT_PHYS_OFFSET > > -#define PHYS_OFFSET PLAT_PHYS_OFFSET > > -#else > > -#define PHYS_OFFSET UL(CONFIG_PHYS_OFFSET) > > -#endif > > -#endif > > And that makes PHYS_OFFSET undefined to assembly code - and we have > references to it from said code. Why does the kernel compile properly for me with this change then? Nicolas
On Tue, Nov 26, 2013 at 08:35:43AM -0500, Nicolas Pitre wrote: > On Tue, 26 Nov 2013, Russell King - ARM Linux wrote: > > > On Mon, Nov 25, 2013 at 10:56:25PM -0500, Nicolas Pitre wrote: > > > What about simply doing the following instead, which I'm sure used to > > > work properly at some point: > > > > > > diff --git a/arch/arm/include/asm/memory.h b/arch/arm/include/asm/memory.h > > > index 9ecccc8650..2b8b8d3236 100644 > > > --- a/arch/arm/include/asm/memory.h > > > +++ b/arch/arm/include/asm/memory.h > > > @@ -239,6 +239,14 @@ static inline unsigned long __phys_to_virt(phys_addr_t x) > > > > > > #else > > > > > > +#ifndef PHYS_OFFSET > > > +#ifdef PLAT_PHYS_OFFSET > > > +#define PHYS_OFFSET PLAT_PHYS_OFFSET > > > +#else > > > +#define PHYS_OFFSET UL(CONFIG_PHYS_OFFSET) > > > +#endif > > > +#endif > > > + > > > static inline phys_addr_t __virt_to_phys(unsigned long x) > > > { > > > return (phys_addr_t)x - PAGE_OFFSET + PHYS_OFFSET; > > > @@ -253,14 +261,6 @@ static inline unsigned long __phys_to_virt(phys_addr_t x) > > > #endif > > > #endif /* __ASSEMBLY__ */ > > > > > > -#ifndef PHYS_OFFSET > > > -#ifdef PLAT_PHYS_OFFSET > > > -#define PHYS_OFFSET PLAT_PHYS_OFFSET > > > -#else > > > -#define PHYS_OFFSET UL(CONFIG_PHYS_OFFSET) > > > -#endif > > > -#endif > > > > And that makes PHYS_OFFSET undefined to assembly code - and we have > > references to it from said code. > > Why does the kernel compile properly for me with this change then? Nicolas, Try using grep rather than just typing emails for the hell of it. Try looking at the patch I sent to fix this issue. Either of those will give you the appropriate clue necessary to answer your question.
On Tue, 26 Nov 2013, Russell King - ARM Linux wrote: > On Tue, Nov 26, 2013 at 08:35:43AM -0500, Nicolas Pitre wrote: > > On Tue, 26 Nov 2013, Russell King - ARM Linux wrote: > > > > > On Mon, Nov 25, 2013 at 10:56:25PM -0500, Nicolas Pitre wrote: > > > > What about simply doing the following instead, which I'm sure used to > > > > work properly at some point: > > > > > > > > diff --git a/arch/arm/include/asm/memory.h b/arch/arm/include/asm/memory.h > > > > index 9ecccc8650..2b8b8d3236 100644 > > > > --- a/arch/arm/include/asm/memory.h > > > > +++ b/arch/arm/include/asm/memory.h > > > > @@ -239,6 +239,14 @@ static inline unsigned long __phys_to_virt(phys_addr_t x) > > > > > > > > #else > > > > > > > > +#ifndef PHYS_OFFSET > > > > +#ifdef PLAT_PHYS_OFFSET > > > > +#define PHYS_OFFSET PLAT_PHYS_OFFSET > > > > +#else > > > > +#define PHYS_OFFSET UL(CONFIG_PHYS_OFFSET) > > > > +#endif > > > > +#endif > > > > + > > > > static inline phys_addr_t __virt_to_phys(unsigned long x) > > > > { > > > > return (phys_addr_t)x - PAGE_OFFSET + PHYS_OFFSET; > > > > @@ -253,14 +261,6 @@ static inline unsigned long __phys_to_virt(phys_addr_t x) > > > > #endif > > > > #endif /* __ASSEMBLY__ */ > > > > > > > > -#ifndef PHYS_OFFSET > > > > -#ifdef PLAT_PHYS_OFFSET > > > > -#define PHYS_OFFSET PLAT_PHYS_OFFSET > > > > -#else > > > > -#define PHYS_OFFSET UL(CONFIG_PHYS_OFFSET) > > > > -#endif > > > > -#endif > > > > > > And that makes PHYS_OFFSET undefined to assembly code - and we have > > > references to it from said code. > > > > Why does the kernel compile properly for me with this change then? > > Nicolas, > > Try using grep rather than just typing emails for the hell of it. > Try looking at the patch I sent to fix this issue. Either of those > will give you the appropriate clue necessary to answer your question. Your patch does s/PHYS_OFFSET/PLAT_PHYS_OFFSET/ in assembly code and defines PLAT_PHYS_OFFSET with CONFIG_PHYS_OFFSET when needed. Mine leaves PHYS_OFFSET as is in assembly code and defines PHYS_OFFSET with PLAT_PHYS_OFFSET or CONFIG_PHYS_OFFSET as appropriate, as it *used* to work before commit ca5a45c06c. The reason this commit broke things is because it moved __virt_to_phys() and __phys_to_virt() from macros to static inline. The macro version didn't need PHYS_OFFSET to be defined beforehand, which is not the case for static inline definitions. Hence my fix which is to simply move (one of) the definition of PHYS_OFFSET above its use by the mentioned static inline code. I also think it is a good thing _not_ to use PLAT_PHYS_OFFSET in assembly code. This was in fact done on purpose. The simple fact that PHYS_OFFSET, with some configuration, expands to something unappropriate for assembly code is a perfect mechanism to identify that something is wrong with that configuration. The reason is that no assembly code should be using PHYS_OFFSET when CONFIG_PATCH_PHYS_VIRT is defined. If it does then this is wrong. By moving that assembly code to PLAT_PHYS_OFFSET you are basically removing that sanity check mechanism since it might be possible for a platform to still define PLAT_PHYS_OFFSET in its mach/memory.h even if CONFIG_PATCH_PHYS_VIRT is defined. So, unless you identify a problem with the patch I sent, I still stand by my suggested fix. Nicolas
On Tue, Nov 26, 2013 at 12:26:49PM -0500, Nicolas Pitre wrote: > On Tue, 26 Nov 2013, Russell King - ARM Linux wrote: > > > On Tue, Nov 26, 2013 at 08:35:43AM -0500, Nicolas Pitre wrote: > > > On Tue, 26 Nov 2013, Russell King - ARM Linux wrote: > > > > > > > On Mon, Nov 25, 2013 at 10:56:25PM -0500, Nicolas Pitre wrote: > > > > > What about simply doing the following instead, which I'm sure used to > > > > > work properly at some point: > > > > > > > > > > diff --git a/arch/arm/include/asm/memory.h b/arch/arm/include/asm/memory.h > > > > > index 9ecccc8650..2b8b8d3236 100644 > > > > > --- a/arch/arm/include/asm/memory.h > > > > > +++ b/arch/arm/include/asm/memory.h > > > > > @@ -239,6 +239,14 @@ static inline unsigned long __phys_to_virt(phys_addr_t x) > > > > > > > > > > #else > > > > > > > > > > +#ifndef PHYS_OFFSET > > > > > +#ifdef PLAT_PHYS_OFFSET > > > > > +#define PHYS_OFFSET PLAT_PHYS_OFFSET > > > > > +#else > > > > > +#define PHYS_OFFSET UL(CONFIG_PHYS_OFFSET) > > > > > +#endif > > > > > +#endif > > > > > + > > > > > static inline phys_addr_t __virt_to_phys(unsigned long x) > > > > > { > > > > > return (phys_addr_t)x - PAGE_OFFSET + PHYS_OFFSET; > > > > > @@ -253,14 +261,6 @@ static inline unsigned long __phys_to_virt(phys_addr_t x) > > > > > #endif > > > > > #endif /* __ASSEMBLY__ */ > > > > > > > > > > -#ifndef PHYS_OFFSET > > > > > -#ifdef PLAT_PHYS_OFFSET > > > > > -#define PHYS_OFFSET PLAT_PHYS_OFFSET > > > > > -#else > > > > > -#define PHYS_OFFSET UL(CONFIG_PHYS_OFFSET) > > > > > -#endif > > > > > -#endif > > > > > > > > And that makes PHYS_OFFSET undefined to assembly code - and we have > > > > references to it from said code. > > > > > > Why does the kernel compile properly for me with this change then? > > > > Nicolas, > > > > Try using grep rather than just typing emails for the hell of it. > > Try looking at the patch I sent to fix this issue. Either of those > > will give you the appropriate clue necessary to answer your question. > > Your patch does s/PHYS_OFFSET/PLAT_PHYS_OFFSET/ in assembly code and > defines PLAT_PHYS_OFFSET with CONFIG_PHYS_OFFSET when needed. > > Mine leaves PHYS_OFFSET as is in assembly code and defines PHYS_OFFSET > with PLAT_PHYS_OFFSET or CONFIG_PHYS_OFFSET as appropriate, as it *used* > to work before commit ca5a45c06c. If manual inspection fails you, try building a nommu or XIP kernel. For crying out loud, you're moving the definition of PHYS_OFFSET to be _inside_ a #ifndef __ASSEMBLY__ .. #endif section. That's fscking obvious if you bother to read your own patch, because you move the definition to just _before_ some C code, and the assembler won't parse C code.
On Tue, 26 Nov 2013, Russell King - ARM Linux wrote: > On Tue, Nov 26, 2013 at 12:26:49PM -0500, Nicolas Pitre wrote: > > On Tue, 26 Nov 2013, Russell King - ARM Linux wrote: > > > > > On Tue, Nov 26, 2013 at 08:35:43AM -0500, Nicolas Pitre wrote: > > > > On Tue, 26 Nov 2013, Russell King - ARM Linux wrote: > > > > > > > > > On Mon, Nov 25, 2013 at 10:56:25PM -0500, Nicolas Pitre wrote: > > > > > > What about simply doing the following instead, which I'm sure used to > > > > > > work properly at some point: > > > > > > > > > > > > diff --git a/arch/arm/include/asm/memory.h b/arch/arm/include/asm/memory.h > > > > > > index 9ecccc8650..2b8b8d3236 100644 > > > > > > --- a/arch/arm/include/asm/memory.h > > > > > > +++ b/arch/arm/include/asm/memory.h > > > > > > @@ -239,6 +239,14 @@ static inline unsigned long __phys_to_virt(phys_addr_t x) > > > > > > > > > > > > #else > > > > > > > > > > > > +#ifndef PHYS_OFFSET > > > > > > +#ifdef PLAT_PHYS_OFFSET > > > > > > +#define PHYS_OFFSET PLAT_PHYS_OFFSET > > > > > > +#else > > > > > > +#define PHYS_OFFSET UL(CONFIG_PHYS_OFFSET) > > > > > > +#endif > > > > > > +#endif > > > > > > + > > > > > > static inline phys_addr_t __virt_to_phys(unsigned long x) > > > > > > { > > > > > > return (phys_addr_t)x - PAGE_OFFSET + PHYS_OFFSET; > > > > > > @@ -253,14 +261,6 @@ static inline unsigned long __phys_to_virt(phys_addr_t x) > > > > > > #endif > > > > > > #endif /* __ASSEMBLY__ */ > > > > > > > > > > > > -#ifndef PHYS_OFFSET > > > > > > -#ifdef PLAT_PHYS_OFFSET > > > > > > -#define PHYS_OFFSET PLAT_PHYS_OFFSET > > > > > > -#else > > > > > > -#define PHYS_OFFSET UL(CONFIG_PHYS_OFFSET) > > > > > > -#endif > > > > > > -#endif > > > > > > > > > > And that makes PHYS_OFFSET undefined to assembly code - and we have > > > > > references to it from said code. > > > > > > > > Why does the kernel compile properly for me with this change then? > > > > > > Nicolas, > > > > > > Try using grep rather than just typing emails for the hell of it. > > > Try looking at the patch I sent to fix this issue. Either of those > > > will give you the appropriate clue necessary to answer your question. > > > > Your patch does s/PHYS_OFFSET/PLAT_PHYS_OFFSET/ in assembly code and > > defines PLAT_PHYS_OFFSET with CONFIG_PHYS_OFFSET when needed. > > > > Mine leaves PHYS_OFFSET as is in assembly code and defines PHYS_OFFSET > > with PLAT_PHYS_OFFSET or CONFIG_PHYS_OFFSET as appropriate, as it *used* > > to work before commit ca5a45c06c. > > If manual inspection fails you, try building a nommu or XIP kernel. All right. I did a build test of course. What failed me is the difficulty nowdays to have the desired config symbols enabled (or left disabled for that matter) without other rules overriding manual choices. I think your patch is therefore the best solution. However, I'd suggest you include this as well to enforce configuration validity: #ifdef CONFIG_ARM_PATCH_PHYS_VIRT #undef PLAT_PHYS_OFFSET #endif The idea as I explained in my previous email is to prevent wrong usage. Nicolas
On Tue, Nov 26, 2013 at 01:41:08PM -0500, Nicolas Pitre wrote: > On Tue, 26 Nov 2013, Russell King - ARM Linux wrote: > > If manual inspection fails you, try building a nommu or XIP kernel. > > All right. I did a build test of course. What failed me is the > difficulty nowdays to have the desired config symbols enabled (or > left disabled for that matter) without other rules overriding manual > choices. This is precisely why reading and understanding the file (as I did) is much more important than chucking out emails on a mailing list. I'd already taken the time to see that there were _three_ levels if ifdef there and the problem case was buried in the depths of that. > I think your patch is therefore the best solution. However, I'd suggest > you include this as well to enforce configuration validity: > > #ifdef CONFIG_ARM_PATCH_PHYS_VIRT > #undef PLAT_PHYS_OFFSET > #endif > > The idea as I explained in my previous email is to prevent wrong usage. I don't believe it's necessary. Why? PLAT_PHYS_OFFSET gets defined in one of two ways: 1. We have a mach/memory.h 2. We have CONFIG_PHYS_OFFSET defined CONFIG_PHYS_OFFSET will only be defined if ARM_PATCH_PHYS_OFFSET has been disabled. So that leaves us with needing a mach/memory.h to define it. Now, if you consider that the majority of builds today use multiplatform which requires there that there be no mach/memory.h, and that requires the phys offset patching, we've already got way more than enough build coverage to find mis-uses, especially with the amount of building that Olof and myself do on the ARM kernel. Moreover, this is no different from what it is today. It hasn't suddenly become more visible. Hence, no matter what, such a change should not be part of fixing up the original breakage.
On Tue, 26 Nov 2013, Russell King - ARM Linux wrote: > On Tue, Nov 26, 2013 at 01:41:08PM -0500, Nicolas Pitre wrote: > > I think your patch is therefore the best solution. However, I'd suggest > > you include this as well to enforce configuration validity: > > > > #ifdef CONFIG_ARM_PATCH_PHYS_VIRT > > #undef PLAT_PHYS_OFFSET > > #endif > > > > The idea as I explained in my previous email is to prevent wrong usage. > > I don't believe it's necessary. Why? > > PLAT_PHYS_OFFSET gets defined in one of two ways: > > 1. We have a mach/memory.h > 2. We have CONFIG_PHYS_OFFSET defined > > CONFIG_PHYS_OFFSET will only be defined if ARM_PATCH_PHYS_OFFSET has been > disabled. So that leaves us with needing a mach/memory.h to define it. > > Now, if you consider that the majority of builds today use multiplatform > which requires there that there be no mach/memory.h, and that requires > the phys offset patching, we've already got way more than enough build > coverage to find mis-uses, especially with the amount of building that > Olof and myself do on the ARM kernel. I don't worry about multiplatform. It is more about those corner-case targets with a mach/memory.h where phys offset patching can be enabled. > Moreover, this is no different from what it is today. It hasn't suddenly > become more visible. Before commit ca5a45c06c a bad PHYS_OFFSET usage in assembly code was caught with a compilation error. Today bad and good uses of PHYS_OFFSET are caught with a compilation error. With your patch there is a risk for (the equivalent of) PHYS_OFFSET to be used wrongly without notice. I'm just arguing for bringing the same state of affair as it was before that commit which is the actual breakage origin. But in the end that doesn't matter much. As I said those are corner-case targets and very few people might still be using them. Nicolas
diff --git a/arch/arm/include/asm/memory.h b/arch/arm/include/asm/memory.h index 9ecccc8650..2b8b8d3236 100644 --- a/arch/arm/include/asm/memory.h +++ b/arch/arm/include/asm/memory.h @@ -239,6 +239,14 @@ static inline unsigned long __phys_to_virt(phys_addr_t x) #else +#ifndef PHYS_OFFSET +#ifdef PLAT_PHYS_OFFSET +#define PHYS_OFFSET PLAT_PHYS_OFFSET +#else +#define PHYS_OFFSET UL(CONFIG_PHYS_OFFSET) +#endif +#endif + static inline phys_addr_t __virt_to_phys(unsigned long x) { return (phys_addr_t)x - PAGE_OFFSET + PHYS_OFFSET; @@ -253,14 +261,6 @@ static inline unsigned long __phys_to_virt(phys_addr_t x) #endif #endif /* __ASSEMBLY__ */ -#ifndef PHYS_OFFSET -#ifdef PLAT_PHYS_OFFSET -#define PHYS_OFFSET PLAT_PHYS_OFFSET -#else -#define PHYS_OFFSET UL(CONFIG_PHYS_OFFSET) -#endif -#endif - #ifndef __ASSEMBLY__ /*