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.
> 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 <[email protected]>
> 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
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
Santosh Shilimkar <[email protected]> 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
On Monday 06 January 2014 05:08 PM, Krzysztof Hałasa wrote:
> Santosh Shilimkar <[email protected]> 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
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
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".
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".
On Monday 06 January 2014 08:11 PM, Russell King - ARM Linux wrote:
> 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.
>
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
On Tue, Jan 07, 2014 at 12:45:42PM -0500, Santosh Shilimkar wrote:
> On Monday 06 January 2014 08:11 PM, Russell King - ARM Linux wrote:
> > /--------------------------------------------------------------
> > | > > 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.
> >
> 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.
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".
Russell King - ARM Linux <[email protected]> writes:
> Okay, reverted.
Thanks.
--
Krzysztof Halasa
Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland