Regression (ARM) arch/arm/mm/init.c doesn't build without CONFIG_ARM_PATCH_PHYS_VIRT.

[Krzysztof Hałasa]

arch/arm/mm/init.c: In function 'setup_dma_zone':
arch/arm/mm/init.c:232:19: error: '__pv_phys_offset' undeclared (first use in this function)

Reverting the following commit fixes it:

commit 787b0d5c1ca7ff24feb6f92e4c7f4410ee7d81a8
Author: Santosh Shilimkar <santosh....@ti.com>
Date: Mon Dec 2 20:29:12 2013 +0100

ARM: 7908/1: mm: Fix the arm_dma_limit calculation

Current code is using PHYS_OFFSET to calculate the arm_dma_limit which
will lead to wrong calculations in cases where PHYS_OFFSET is updated
runtime.

So fix the code by using __pv_phys_offset instead of PHYS_OFFSET.

It seems PHYS_OFFSET is equal to __pv_phys_offset if the latter is
available:

arch/arm/include/asm/memory.h:#define PHYS_OFFSET __pv_phys_offset

Otherwise (without CONFIG_ARM_PATCH_PHYS_VIRT) PHYS_OFFSET is a
hard-coded CONFIG value, or whatever the platform/CPU code needs:

arch/arm/include/asm/memory.h:#define PHYS_OFFSET PLAT_PHYS_OFFSET

Perhaps the patch in question was needed at some point but I think the
situation had changed before it was commited.
--
Krzysztof Halasa

Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

[Russell King - ARM Linux]

On Tue, Dec 31, 2013 at 12:20:56PM +0100, Krzysztof Hałasa wrote:
> arch/arm/mm/init.c: In function 'setup_dma_zone':
> arch/arm/mm/init.c:232:19: error: '__pv_phys_offset' undeclared (first use in this function)
>
> Reverting the following commit fixes it:

Already fixed long before Christmas. Please use recent kernels.

[Krzysztof Hałasa]

Russell King - ARM Linux <li...@arm.linux.org.uk> writes:

> On Tue, Dec 31, 2013 at 12:20:56PM +0100, Krzysztof Hałasa wrote:
>> arch/arm/mm/init.c: In function 'setup_dma_zone':
>> arch/arm/mm/init.c:232:19: error: '__pv_phys_offset' undeclared (first use in this function)
>>
>> Reverting the following commit fixes it:
>
> Already fixed long before Christmas. Please use recent kernels.

Do you mean something other than Linus's tree? The problem I noted
is present in his tree as of Dec 30 - on both 3.13-rc6 and (then tip)
71ce176ee6ed1735b9a1160a5704a915d13849b1.

Or... do you mean the commit that added this regression?

--
Krzysztof Halasa

Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland

[Krzysztof Hałasa]

Russell, Santosh,

the unneeded commit causing regression is still in place. Please try to
compile an ARM kernel without CONFIG_ARM_PATCH_PHYS_VIRT and with
CONFIG_ZONE_DMA and see for yourself, if you don't believe me.

Please be aware that this commit fixes nothing, its only function is
causing the regression - so we don't lose anything by reverting it.

If the attached wasn't clear, what the defective commit presently does
is changing a perfectly valid code into a code referencing a variable
which (without CONFIG_ARM_PATCH_PHYS_VIRT set) doesn't at all exist.

With CONFIG_ARM_PATCH_PHYS_VIRT set, this commit does precisely nothing.

grep __pv_phys_offset * -r gives:

Broken by the commit in question:

arch/arm/mm/init.c: arm_dma_limit = __pv_phys_offset + arm_dma_zone_size - 1;

All of the following are only compiled/assembled with
CONFIG_ARM_PATCH_PHYS_VIRT set:

arch/arm/include/asm/memory.h:extern u64 __pv_phys_offset;
arch/arm/include/asm/memory.h:#define PHYS_OFFSET __pv_phys_offset
arch/arm/kernel/armksyms.c:EXPORT_SYMBOL(__pv_phys_offset);
arch/arm/kernel/head.S: add r6, r6, r3 @ adjust
__pv_phys_offset address
arch/arm/kernel/head.S: str r8, [r6, #LOW_OFFSET] @ save computed
PHYS_OFFSET to __pv_phys_offset
arch/arm/kernel/head.S:2: .long __pv_phys_offset
arch/arm/kernel/head.S: .globl __pv_phys_offset
arch/arm/kernel/head.S: .type __pv_phys_offset, %object
arch/arm/kernel/head.S:__pv_phys_offset:
arch/arm/kernel/head.S: .size __pv_phys_offset, . -__pv_phys_offset

In short, please revert 787b0d5c1ca7ff24feb6f92e4c7f4410ee7d81a8.
Or, do you want me to send a patch which does that?

HTH and TIA.

[Santosh Shilimkar]

On Monday 06 January 2014 02:28 PM, Krzysztof Hałasa wrote:
> Russell, Santosh,
>
> the unneeded commit causing regression is still in place. Please try to
> compile an ARM kernel without CONFIG_ARM_PATCH_PHYS_VIRT and with
> CONFIG_ZONE_DMA and see for yourself, if you don't believe me.
>
> Please be aware that this commit fixes nothing, its only function is
> causing the regression - so we don't lose anything by reverting it.
>

I am afraid you didn't understood what the fix is if you say above.
arm_dma_limit is broken without this fix for LPAE machines with
memory starting 4 GB physical boundary.

Regards,
Santosh

[Krzysztof Hałasa]

Santosh Shilimkar <santosh....@ti.com> writes:

> I am afraid you didn't understood what the fix is if you say above.
> arm_dma_limit is broken without this fix for LPAE machines with
> memory starting 4 GB physical boundary.

I wonder what CONFIG_ARM_PATCH_PHYS_VIRT and CONFIG_ZONE_DMA do they
use?

Of course, CONFIG_ZONE_DMA must be set, because otherwise the patch
changes a part disabled with #ifdef. CONFIG_ARM_PATCH_PHYS_VIRT must be
set because otherwise the kernel will not compile at all.

Do you accept the fact the configurations with CONFIG_ZONE_DMA and
without CONFIG_ARM_PATCH_PHYS_VIRT are now broken - by this very commit?

Now, what "fix" does this commit in its entirety do:

- arm_dma_limit = PHYS_OFFSET + arm_dma_zone_size - 1;
+ arm_dma_limit = __pv_phys_offset + arm_dma_zone_size - 1;

if under the above circumstances (CONFIG_*) in
arch/arm/include/asm/memory.h we have:

extern u64 __pv_phys_offset;
^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the only usable declaration of this variable
extern u64 __pv_offset;
extern void fixup_pv_table(const void *, unsigned long);
extern const void *__pv_table_begin, *__pv_table_end;

#define PHYS_OFFSET __pv_phys_offset
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

--
Krzysztof Halasa

Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland

[Santosh Shilimkar]

On Monday 06 January 2014 05:08 PM, Krzysztof Hałasa wrote:
> Santosh Shilimkar <santosh....@ti.com> writes:
>
>> I am afraid you didn't understood what the fix is if you say above.
>> arm_dma_limit is broken without this fix for LPAE machines with
>> memory starting 4 GB physical boundary.
>
> I wonder what CONFIG_ARM_PATCH_PHYS_VIRT and CONFIG_ZONE_DMA do they
> use?
>

You need to grep where the CONFIG_ARM_PATCH_PHYS_VIRT is used ;)

> Of course, CONFIG_ZONE_DMA must be set, because otherwise the patch
> changes a part disabled with #ifdef. CONFIG_ARM_PATCH_PHYS_VIRT must be
> set because otherwise the kernel will not compile at all.
>
> Do you accept the fact the configurations with CONFIG_ZONE_DMA and
> without CONFIG_ARM_PATCH_PHYS_VIRT are now broken - by this very commit?
>
> Now, what "fix" does this commit in its entirety do:
>
> - arm_dma_limit = PHYS_OFFSET + arm_dma_zone_size - 1;
> + arm_dma_limit = __pv_phys_offset + arm_dma_zone_size - 1;
>
> if under the above circumstances (CONFIG_*) in
> arch/arm/include/asm/memory.h we have:
>
> extern u64 __pv_phys_offset;
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the only usable declaration of this variable
> extern u64 __pv_offset;
> extern void fixup_pv_table(const void *, unsigned long);
> extern const void *__pv_table_begin, *__pv_table_end;
>
> #define PHYS_OFFSET __pv_phys_offset
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>

Your grep actually missed the assembly files where the actual patching
happens. Please have a look at "arch/arm/kernel/head.S"

The "CONFIG_ARM_PATCH_PHYS_VIRT" is always enabled on almost all ARM builds
since its needed for multi-config kernel to work. May be its time to
make it default enabled but I let Russell comment if there is any
better way to handle this.

Regards,
Santosh

[Santosh Shilimkar]

On Monday 06 January 2014 05:39 PM, Russell King - ARM Linux wrote:

> On Mon, Jan 06, 2014 at 08:28:20PM +0100, Krzysztof Hałasa wrote:
>> Russell, Santosh,
>>
>> the unneeded commit causing regression is still in place. Please try to
>> compile an ARM kernel without CONFIG_ARM_PATCH_PHYS_VIRT and with
>> CONFIG_ZONE_DMA and see for yourself, if you don't believe me.
>>
>> Please be aware that this commit fixes nothing, its only function is
>> causing the regression - so we don't lose anything by reverting it.
>>
>> If the attached wasn't clear, what the defective commit presently does
>> is changing a perfectly valid code into a code referencing a variable
>> which (without CONFIG_ARM_PATCH_PHYS_VIRT set) doesn't at all exist.
>>
>> With CONFIG_ARM_PATCH_PHYS_VIRT set, this commit does precisely nothing.
>

> Right, so, with Assabet, which has CONFIG_DMA_ZONE=y and
> CONFIG_ARM_PATCH_PHYS_VIRT=y:
>
> $ make O=../build/assabet arch/arm/mm/init.i
>
> gives:

> arm_dma_limit = __pv_phys_offset + arm_dma_zone_size - 1;
>

> with or without Santosh's patch. So, with V2P patching in place, there's
> absolutely no functional difference.
>
> With CONFIG_ARM_PATCH_PHYS_VIRT=n, before Santosh's patch:
>
> arm_dma_limit = (0xc0000000UL) + arm_dma_zone_size - 1;
>
> After:

>
> arm_dma_limit = __pv_phys_offset + arm_dma_zone_size - 1;
>

> and this breaks the build because there is no __pv_phys_offset symbol.
>
> Now, the case which matters for Santosh is the first case - the one
> where CONFIG_ARM_PATCH_PHYS_VIRT=y. We can clearly see that after
> preprocessing, the results are 100% identical.
>
> Therefore, I find myself agreeing with Krzysztof that the commit is
> bad, has no functional change for the case it was proposed to solve,
> and needs to be reverted.
>
May be I missed your point but I ended up creating the patch because
the CMA reservation was failing on Keystone since the arm_dma_limit
wasn't right since it wasn't considering the actual __pv_phys_offset.

Isn't that an issue ?

Regards,
Santosh

[Russell King - ARM Linux]

On Mon, Jan 06, 2014 at 08:28:20PM +0100, Krzysztof Hałasa wrote:

> Russell, Santosh,
>
> the unneeded commit causing regression is still in place. Please try to
> compile an ARM kernel without CONFIG_ARM_PATCH_PHYS_VIRT and with
> CONFIG_ZONE_DMA and see for yourself, if you don't believe me.
>
> Please be aware that this commit fixes nothing, its only function is
> causing the regression - so we don't lose anything by reverting it.
>
> If the attached wasn't clear, what the defective commit presently does
> is changing a perfectly valid code into a code referencing a variable
> which (without CONFIG_ARM_PATCH_PHYS_VIRT set) doesn't at all exist.
>
> With CONFIG_ARM_PATCH_PHYS_VIRT set, this commit does precisely nothing.

Right, so, with Assabet, which has CONFIG_DMA_ZONE=y and
CONFIG_ARM_PATCH_PHYS_VIRT=y:

$ make O=../build/assabet arch/arm/mm/init.i

gives:

arm_dma_limit = __pv_phys_offset + arm_dma_zone_size - 1;

with or without Santosh's patch. So, with V2P patching in place, there's
absolutely no functional difference.

With CONFIG_ARM_PATCH_PHYS_VIRT=n, before Santosh's patch:

arm_dma_limit = (0xc0000000UL) + arm_dma_zone_size - 1;

After:

arm_dma_limit = __pv_phys_offset + arm_dma_zone_size - 1;

and this breaks the build because there is no __pv_phys_offset symbol.

Now, the case which matters for Santosh is the first case - the one
where CONFIG_ARM_PATCH_PHYS_VIRT=y. We can clearly see that after
preprocessing, the results are 100% identical.

Therefore, I find myself agreeing with Krzysztof that the commit is
bad, has no functional change for the case it was proposed to solve,
and needs to be reverted.

--
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up. Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

[Russell King - ARM Linux]

On Mon, Jan 06, 2014 at 06:42:13PM -0500, Santosh Shilimkar wrote:
> On Monday 06 January 2014 05:39 PM, Russell King - ARM Linux wrote:
> > On Mon, Jan 06, 2014 at 08:28:20PM +0100, Krzysztof Hałasa wrote:
> >> Russell, Santosh,
> >>
> >> the unneeded commit causing regression is still in place. Please try to
> >> compile an ARM kernel without CONFIG_ARM_PATCH_PHYS_VIRT and with
> >> CONFIG_ZONE_DMA and see for yourself, if you don't believe me.
> >>
> >> Please be aware that this commit fixes nothing, its only function is
> >> causing the regression - so we don't lose anything by reverting it.
> >>
> >> If the attached wasn't clear, what the defective commit presently does
> >> is changing a perfectly valid code into a code referencing a variable
> >> which (without CONFIG_ARM_PATCH_PHYS_VIRT set) doesn't at all exist.
> >>
> >> With CONFIG_ARM_PATCH_PHYS_VIRT set, this commit does precisely nothing.
> >

/--------------------------------------------------------------

| > > Right, so, with Assabet, which has CONFIG_DMA_ZONE=y and
| > > CONFIG_ARM_PATCH_PHYS_VIRT=y:
| > >
| > > $ make O=../build/assabet arch/arm/mm/init.i
| > >
| > > gives:
| > > arm_dma_limit = __pv_phys_offset + arm_dma_zone_size - 1;
| > >
| > > with or without Santosh's patch.

\--------------------------------------------------------------

> May be I missed your point but I ended up creating the patch because
> the CMA reservation was failing on Keystone since the arm_dma_limit
> wasn't right since it wasn't considering the actual __pv_phys_offset.
>
> Isn't that an issue ?

See the above. Your patch has _no_ effect what so ever when
CONFIG_ARM_PATCH_PHYS_VIRT=y - which you have on the Keystone, right?

If you don't believe me, ask make to produce arch/arm/mm/init.i, which
is the output from the preprocessor, comparing the resulting generated
file both with and without your patch applied.

--
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up. Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

[Santosh Shilimkar]

Looks like you are right. I did two fixes to have right arm_dma_limit in
below order.

1. 787b0d5 {ARM: 7908/1: mm: Fix the arm_dma_limit calculation}
2. 7c92732 {ARM: 7909/1: mm: Call setup_dma_zone() post early_paging_init()}

But with 2 alone the issue gets fixed since with ARM_PATCH_PHYS_VIRT and
below pre-processor, the PHYS_OFFSET also gets an updated value. I never
realised that 1 becomes redundant after second patch while creating them.

#define PHYS_OFFSET __pv_phys_offset

So indeed, 787b0d5{ARM: 7908/1: mm: Fix the arm_dma_limit calculation}
won't be needed anymore and can be reverted. Sorry it took some time
for me to reach to your conclusion.

Regards,
Santosh

[Russell King - ARM Linux]

Okay, reverted.

--
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up. Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

[Krzysztof Hałasa]

Russell King - ARM Linux <li...@arm.linux.org.uk> writes:

> Okay, reverted.

Thanks.

--
Krzysztof Halasa

Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland