[PATCH v2] Revert "MIPS: make CAC_ADDR and UNCAC_ADDR account for PHYS_OFFSET"

[PATCH v2] Revert "MIPS: make CAC_ADDR and UNCAC_ADDR account for PHYS_OFFSET"

[Steven J. Hill]

From: Leonid Yegoshin <Leonid.Yegoshin@imgtec.com>

This reverts commit 3f4579252aa166641861a64f1c2883365ca126c2. It is
invalid because the macros CAC_ADDR and UNCAC_ADDR have a kernel
virtual address as an argument and also returns a kernel virtual
address. Using and physical address PHYS_OFFSET is blatantly wrong
for a macro common to multiple platforms.

Signed-off-by: Leonid Yegoshin <Leonid.Yegoshin@imgtec.com>
Acked-by: Steven J. Hill <Steven.Hill@imgtec.com>
---
Changes in v2: Remove internal gerrit ID from commit message.

 arch/mips/include/asm/mach-ar7/spaces.h  |    7 +++++--
 arch/mips/include/asm/mach-ip28/spaces.h |    9 ++++++---
 arch/mips/include/asm/page.h             |    6 ++----
 3 files changed, 13 insertions(+), 9 deletions(-)

diff --git a/arch/mips/include/asm/mach-ar7/spaces.h b/arch/mips/include/asm/mach-ar7/spaces.h
index ac28f27..660ab64 100644
--- a/arch/mips/include/asm/mach-ar7/spaces.h
+++ b/arch/mips/include/asm/mach-ar7/spaces.h
@@ -14,8 +14,11 @@
  * This handles the memory map.
  * We handle pages at KSEG0 for kernels with 32 bit address space.
  */
-#define PAGE_OFFSET		0x94000000UL
-#define PHYS_OFFSET		0x14000000UL
+#define PAGE_OFFSET	_AC(0x94000000, UL)
+#define PHYS_OFFSET	_AC(0x14000000, UL)
+
+#define UNCAC_BASE	_AC(0xb4000000, UL)	/* 0xa0000000 + PHYS_OFFSET */
+#define IO_BASE		UNCAC_BASE
 
 #include <asm/mach-generic/spaces.h>
 
diff --git a/arch/mips/include/asm/mach-ip28/spaces.h b/arch/mips/include/asm/mach-ip28/spaces.h
index 5edf05d..5d6a764 100644
--- a/arch/mips/include/asm/mach-ip28/spaces.h
+++ b/arch/mips/include/asm/mach-ip28/spaces.h
@@ -11,11 +11,14 @@
 #ifndef _ASM_MACH_IP28_SPACES_H
 #define _ASM_MACH_IP28_SPACES_H
 
-#define CAC_BASE		0xa800000000000000
+#define CAC_BASE	_AC(0xa800000000000000, UL)
 
-#define HIGHMEM_START		(~0UL)
+#define HIGHMEM_START	(~0UL)
 
-#define PHYS_OFFSET		_AC(0x20000000, UL)
+#define PHYS_OFFSET	_AC(0x20000000, UL)
+
+#define UNCAC_BASE	_AC(0xc0000000, UL)     /* 0xa0000000 + PHYS_OFFSET */
+#define IO_BASE		UNCAC_BASE
 
 #include <asm/mach-generic/spaces.h>
 
diff --git a/arch/mips/include/asm/page.h b/arch/mips/include/asm/page.h
index f59552f..f6be474 100644
--- a/arch/mips/include/asm/page.h
+++ b/arch/mips/include/asm/page.h
@@ -205,10 +205,8 @@ extern int __virt_addr_valid(const volatile void *kaddr);
 #define VM_DATA_DEFAULT_FLAGS	(VM_READ | VM_WRITE | VM_EXEC | \
 				 VM_MAYREAD | VM_MAYWRITE | VM_MAYEXEC)
 
-#define UNCAC_ADDR(addr)	((addr) - PAGE_OFFSET + UNCAC_BASE +	\
-								PHYS_OFFSET)
-#define CAC_ADDR(addr)		((addr) - UNCAC_BASE + PAGE_OFFSET -	\
-								PHYS_OFFSET)
+#define UNCAC_ADDR(addr)	((addr) - PAGE_OFFSET + UNCAC_BASE)
+#define CAC_ADDR(addr)		((addr) - UNCAC_BASE + PAGE_OFFSET)
 
 #include <asm-generic/memory_model.h>
 #include <asm-generic/getorder.h>
-- 
1.7.2.5

Re: [PATCH v2] Revert "MIPS: make CAC_ADDR and UNCAC_ADDR account for PHYS_OFFSET"

[Ralf Baechle]

On Thu, Jun 20, 2013 at 10:36:30AM -0500, Steven J. Hill wrote:

> From: Leonid Yegoshin <Leonid.Yegoshin@imgtec.com>
> 
> This reverts commit 3f4579252aa166641861a64f1c2883365ca126c2. It is
> invalid because the macros CAC_ADDR and UNCAC_ADDR have a kernel
> virtual address as an argument and also returns a kernel virtual
> address. Using and physical address PHYS_OFFSET is blatantly wrong
> for a macro common to multiple platforms.

While the patch itself is looking sane at a glance, I'm wondering if this
is fixing any actual bug or is just the result of a code review?

  Ralf

Re: [PATCH v2] Revert "MIPS: make CAC_ADDR and UNCAC_ADDR account for PHYS_OFFSET"

[Steven J. Hill]

On 06/26/2013 09:52 AM, Ralf Baechle wrote:
> On Thu, Jun 20, 2013 at 10:36:30AM -0500, Steven J. Hill wrote:
>
>> From: Leonid Yegoshin <Leonid.Yegoshin@imgtec.com>
>>
>> This reverts commit 3f4579252aa166641861a64f1c2883365ca126c2. It is
>> invalid because the macros CAC_ADDR and UNCAC_ADDR have a kernel
>> virtual address as an argument and also returns a kernel virtual
>> address. Using and physical address PHYS_OFFSET is blatantly wrong
>> for a macro common to multiple platforms.
>
> While the patch itself is looking sane at a glance, I'm wondering if this
> is fixing any actual bug or is just the result of a code review?
>
The new Aptiv cores are no longer bound by KSEGxxx address spaces. This 
means that the values for KUSEG, KSEG0, KSEG1, KSEG2 cannot be assumed 
to be static anymore, and some macros need to be changed accordingly.

-Steve

Re: [PATCH v2] Revert "MIPS: make CAC_ADDR and UNCAC_ADDR account for PHYS_OFFSET"

[Steven J. Hill]

On 06/26/2013 09:52 AM, Ralf Baechle wrote:
> On Thu, Jun 20, 2013 at 10:36:30AM -0500, Steven J. Hill wrote:
>
>> From: Leonid Yegoshin <Leonid.Yegoshin@imgtec.com>
>>
>> This reverts commit 3f4579252aa166641861a64f1c2883365ca126c2. It is
>> invalid because the macros CAC_ADDR and UNCAC_ADDR have a kernel
>> virtual address as an argument and also returns a kernel virtual
>> address. Using and physical address PHYS_OFFSET is blatantly wrong
>> for a macro common to multiple platforms.
>
> While the patch itself is looking sane at a glance, I'm wondering if this
> is fixing any actual bug or is just the result of a code review?
>
The new Aptiv cores are no longer bound by KSEGxxx address spaces. This 
means that the values for KUSEG, KSEG0, KSEG1, KSEG2 cannot be assumed 
to be static anymore, and some macros need to be changed accordingly.

-Steve

Re: [PATCH v2] Revert "MIPS: make CAC_ADDR and UNCAC_ADDR account for PHYS_OFFSET"

[Leonid Yegoshin]

This is a precursor for EVA specs implementation on Aptiv cores.

EVA has different virtual address sets for kernel and user space and it can use memory on different physical address location. For exam, on Malta it can use a natural 0x80000000, one our customer put memory into 0x40000000 etc.

- Leonid.


Ralf Baechle <ralf@linux-mips.org> wrote:


On Thu, Jun 20, 2013 at 10:36:30AM -0500, Steven J. Hill wrote:

> From: Leonid Yegoshin <Leonid.Yegoshin@imgtec.com>
>
> This reverts commit 3f4579252aa166641861a64f1c2883365ca126c2. It is
> invalid because the macros CAC_ADDR and UNCAC_ADDR have a kernel
> virtual address as an argument and also returns a kernel virtual
> address. Using and physical address PHYS_OFFSET is blatantly wrong
> for a macro common to multiple platforms.

While the patch itself is looking sane at a glance, I'm wondering if this
is fixing any actual bug or is just the result of a code review?

  Ralf

Re: [PATCH v2] Revert "MIPS: make CAC_ADDR and UNCAC_ADDR account for PHYS_OFFSET"

[Ralf Baechle]

On Wed, Jun 26, 2013 at 03:43:35PM +0000, Leonid Yegoshin wrote:

> This is a precursor for EVA specs implementation on Aptiv cores.
> 
> EVA has different virtual address sets for kernel and user space and it can use memory on different physical address location. For exam, on Malta it can use a natural 0x80000000, one our customer put memory into 0x40000000 etc.

Hmm...  Any significant reduction below 2GB sounds like opening a can of
worms with address space layout assumption in some application code.

I guess they were desperately looking to increase kernel memory, highmem
didn't fit the bill nor going 64 bit so this was the solution?

  Ralf

Re: [PATCH v2] Revert "MIPS: make CAC_ADDR and UNCAC_ADDR account for PHYS_OFFSET"

[Leonid Yegoshin]

Ralf,

EVA has actually INCREASE in user address space - I right now run system with 2GB phys memory and 3GB of user virtual memory address space. Work in progress is to verify that GLIBC accepts addresses above 2GB.

Yes, it is all about increasing phys and user memory and avoiding 64bits. Many solutions dont justify 64bit chip (chip space increase, performance degradation and increase in DMA addresses for devices).

- Leonid.


Ralf Baechle <ralf@linux-mips.org> wrote:


On Wed, Jun 26, 2013 at 03:43:35PM +0000, Leonid Yegoshin wrote:

> This is a precursor for EVA specs implementation on Aptiv cores.
>
> EVA has different virtual address sets for kernel and user space and it can use memory on different physical address location. For exam, on Malta it can use a natural 0x80000000, one our customer put memory into 0x40000000 etc.

Hmm...  Any significant reduction below 2GB sounds like opening a can of
worms with address space layout assumption in some application code.

I guess they were desperately looking to increase kernel memory, highmem
didn't fit the bill nor going 64 bit so this was the solution?

  Ralf

Re: [PATCH v2] Revert "MIPS: make CAC_ADDR and UNCAC_ADDR account for PHYS_OFFSET"

[Ralf Baechle]

On Wed, Jun 26, 2013 at 04:50:03PM +0000, Leonid Yegoshin wrote:

> EVA has actually INCREASE in user address space - I right now run system with 2GB phys memory and 3GB of user virtual memory address space. Work in progress is to verify that GLIBC accepts addresses above 2GB.

I took the 0x40000000 for a KSEG0-equivalent because you previously
mentioned the value of 0x80000000.

> Yes, it is all about increasing phys and user memory and avoiding 64bits. Many solutions dont justify 64bit chip (chip space increase, performance degradation and increase in DMA addresses for devices).

Fair enough - but in the end the increasing size of metadata and pagetables
which has to reside in lowmem will become the next bottleneck and highmem
I/O performance has never been great, is on most kernel developers shit list
and performance optimizations for highmem are getting killed whenever they
are getting into the way.

So I'd say EVA gives you something like 1.5GB of memory at most with good
performance and a 2GB userspace and something like 0.5GB, maybe 0.75GB
with a 3GB userspace.  Beyond that you need highmem and that's where things,
especially kernel programming get more complicated and slower.

  Ralf

Re: [PATCH v2] Revert "MIPS: make CAC_ADDR and UNCAC_ADDR account for PHYS_OFFSET"

[Leonid Yegoshin]

Ralf,

On 06/26/2013 10:50 AM, Ralf Baechle wrote:
> On Wed, Jun 26, 2013 at 04:50:03PM +0000, Leonid Yegoshin wrote:
>
>> EVA has actually INCREASE in user address space - I right now run system with 2GB phys memory and 3GB of user virtual memory address space. Work in progress is to verify that GLIBC accepts addresses above 2GB.
> I took the 0x40000000 for a KSEG0-equivalent because you previously
> mentioned the value of 0x80000000.

I wrote about kernel address layout. With EVA a user address layout is a 
different beast.
In EVA, user may have access, say [0x00000000 - 0xBFFFFFFF] through TLB and
kernel may have access, say [0x00000000 - 0xDFFFFFFF] unmapped. But 
segment shifts are applied to each KSEG.

>
>> Yes, it is all about increasing phys and user memory and avoiding 64bits. Many solutions dont justify 64bit chip (chip space increase, performance degradation and increase in DMA addresses for devices).
> Fair enough - but in the end the increasing size of metadata and pagetables
> which has to reside in lowmem will become the next bottleneck and highmem
> I/O performance has never been great, is on most kernel developers shit list
> and performance optimizations for highmem are getting killed whenever they
> are getting into the way.

EVA doesn't use HIGHMEM. Kernel has a direct access to all memory in, 
say 3GB (3.5GB?).
Malta model gives only 2GB because of PCI bridge loop problem.

>
> So I'd say EVA gives you something like 1.5GB of memory at most with good
> performance and a 2GB userspace and something like 0.5GB, maybe 0.75GB
> with a 3GB userspace.  Beyond that you need highmem and that's where things,
> especially kernel programming get more complicated and slower.

Ralf, PTE and page table sizes depends from page size and HUGE page 
table implementation.
With EVA the ratio of usable and service (PTE + page table) memory is 
the same as legacy MIPS
and independs from used user space size. Right now I am running SOAK 
tests + additional
"thrash" instance for 1500MB on 2GB physical Malta memory and see:

...
Thrash v0.3 thrashing over 1500 megabytes
...
vmstat
procs -----------memory---------- ---swap-- -----io---- --system-- 
----cpu----
  r  b   swpd   free   buff  cache   si   so    bi    bo in    cs us sy 
id wa
10  0      0 950480 252384 107856    0    0     1    18 166   132 75 25  
0  0

See: swap si/so == 0.

I use 16KB pages.

>
>    Ralf

- Leonid.

Re: [PATCH v2] Revert "MIPS: make CAC_ADDR and UNCAC_ADDR account for PHYS_OFFSET"

[Maciej W. Rozycki]

Ralf,

On Wed, 26 Jun 2013, Leonid Yegoshin wrote:

> > > EVA has actually INCREASE in user address space - I right now run system
> > > with 2GB phys memory and 3GB of user virtual memory address space. Work in
> > > progress is to verify that GLIBC accepts addresses above 2GB.
> > I took the 0x40000000 for a KSEG0-equivalent because you previously
> > mentioned the value of 0x80000000.
> 
> I wrote about kernel address layout. With EVA a user address layout is a
> different beast.
> In EVA, user may have access, say [0x00000000 - 0xBFFFFFFF] through TLB and
> kernel may have access, say [0x00000000 - 0xDFFFFFFF] unmapped. But segment
> shifts are applied to each KSEG.
> 
> > > Yes, it is all about increasing phys and user memory and avoiding 64bits.
> > > Many solutions dont justify 64bit chip (chip space increase, performance
> > > degradation and increase in DMA addresses for devices).
> > Fair enough - but in the end the increasing size of metadata and pagetables
> > which has to reside in lowmem will become the next bottleneck and highmem
> > I/O performance has never been great, is on most kernel developers shit list
> > and performance optimizations for highmem are getting killed whenever they
> > are getting into the way.
> 
> EVA doesn't use HIGHMEM. Kernel has a direct access to all memory in, say 3GB
> (3.5GB?).
> Malta model gives only 2GB because of PCI bridge loop problem.

 To complete the image, there is a set of new memory access instructions 
added (including but not limited to CACHE) that in the kernel mode 
separates accesses to the user space from accesses to the kernel space, 
i.e. the same virtual address can map differently depending on which 
instruction set it is used with.  I encourage you to have at least a skim 
over the most recent set of MIPS architecture manuals publicly available 
where it all is documented.

  Maciej

Re: [PATCH v2] Revert "MIPS: make CAC_ADDR and UNCAC_ADDR account for PHYS_OFFSET"

[Leonid Yegoshin]

On 06/26/2013 02:41 PM, Maciej W. Rozycki wrote:
> Ralf,
>   To complete the image, there is a set of new memory access instructions
> added (including but not limited to CACHE) that in the kernel mode
> separates accesses to the user space from accesses to the kernel space,
> i.e. the same virtual address can map differently depending on which
> instruction set it is used with.  I encourage you to have at least a skim
> over the most recent set of MIPS architecture manuals publicly available
> where it all is documented.
>
>    Maciej

Look into http://www.mips.com/auth/MD00091-2B-MIPS64PRA-AFP-03.52.pdf
(registration required). Read sections 4.13, 4.12 and 9.13-9.15.
Ignore anything for 64bit core.

Sorry, I asked David Lau to update MIPS32 actual docs but it can take time.
Right now it is from 2011 year.

- Leonid.

Re: [PATCH v2] Revert "MIPS: make CAC_ADDR and UNCAC_ADDR account for PHYS_OFFSET"

[Markos Chandras]

On 26 June 2013 15:52, Ralf Baechle <ralf@linux-mips.org> wrote:
> On Thu, Jun 20, 2013 at 10:36:30AM -0500, Steven J. Hill wrote:
>
>> From: Leonid Yegoshin <Leonid.Yegoshin@imgtec.com>
>>
>> This reverts commit 3f4579252aa166641861a64f1c2883365ca126c2. It is
>> invalid because the macros CAC_ADDR and UNCAC_ADDR have a kernel
>> virtual address as an argument and also returns a kernel virtual
>> address. Using and physical address PHYS_OFFSET is blatantly wrong
>> for a macro common to multiple platforms.
>
> While the patch itself is looking sane at a glance, I'm wondering if this
> is fixing any actual bug or is just the result of a code review?
>
>   Ralf
>

I am afraid this commit[1] broke the build in
upstream-sfr/mips-for-linux-next with errors like this

arch/mips/include/asm/mach-generic/spaces.h:29:0: warning:
"UNCAC_BASE" redefined [enabled by default]
In file included from arch/mips/include/asm/addrspace.h:13:0,
                 from arch/mips/include/asm/barrier.h:11,
                 from arch/mips/include/asm/bitops.h:18,
                 from include/linux/bitops.h:22,
                 from include/linux/kernel.h:10,
                 from include/asm-generic/bug.h:13,
                 from arch/mips/include/asm/bug.h:41,
                 from include/linux/bug.h:4,
                 from include/linux/page-flags.h:9,
                 from kernel/bounds.c:9:
arch/mips/include/asm/mach-ar7/spaces.h:20:0: note: this is the
location of the previous definition

[1]: http://git.linux-mips.org/?p=ralf/upstream-sfr.git;a=commit;h=ed3ce16c3d2ba7cac321d29ec0a7d21408ea8437

--
Regards,
Markos Chandras

Re: [PATCH v2] Revert "MIPS: make CAC_ADDR and UNCAC_ADDR account for PHYS_OFFSET"

[Steven J. Hill]

On 07/03/2013 09:01 AM, Markos Chandras wrote:
>
> I am afraid this commit[1] broke the build in
> upstream-sfr/mips-for-linux-next with errors like this
>
> arch/mips/include/asm/mach-generic/spaces.h:29:0: warning:
> "UNCAC_BASE" redefined [enabled by default]
> In file included from arch/mips/include/asm/addrspace.h:13:0,
>                   from arch/mips/include/asm/barrier.h:11,
>                   from arch/mips/include/asm/bitops.h:18,
>                   from include/linux/bitops.h:22,
>                   from include/linux/kernel.h:10,
>                   from include/asm-generic/bug.h:13,
>                   from arch/mips/include/asm/bug.h:41,
>                   from include/linux/bug.h:4,
>                   from include/linux/page-flags.h:9,
>                   from kernel/bounds.c:9:
> arch/mips/include/asm/mach-ar7/spaces.h:20:0: note: this is the
> location of the previous definition
>
> [1]: http://git.linux-mips.org/?p=ralf/upstream-sfr.git;a=commit;h=ed3ce16c3d2ba7cac321d29ec0a7d21408ea8437
>
Let me rebase the patch and send another version.

Re: [PATCH v2] Revert "MIPS: make CAC_ADDR and UNCAC_ADDR account for PHYS_OFFSET"

[Steven J. Hill]

On 07/03/2013 09:01 AM, Markos Chandras wrote:
>
> I am afraid this commit[1] broke the build in
> upstream-sfr/mips-for-linux-next with errors like this
>
> arch/mips/include/asm/mach-generic/spaces.h:29:0: warning:
> "UNCAC_BASE" redefined [enabled by default]
> In file included from arch/mips/include/asm/addrspace.h:13:0,
>                   from arch/mips/include/asm/barrier.h:11,
>                   from arch/mips/include/asm/bitops.h:18,
>                   from include/linux/bitops.h:22,
>                   from include/linux/kernel.h:10,
>                   from include/asm-generic/bug.h:13,
>                   from arch/mips/include/asm/bug.h:41,
>                   from include/linux/bug.h:4,
>                   from include/linux/page-flags.h:9,
>                   from kernel/bounds.c:9:
> arch/mips/include/asm/mach-ar7/spaces.h:20:0: note: this is the
> location of the previous definition
>
> [1]: http://git.linux-mips.org/?p=ralf/upstream-sfr.git;a=commit;h=ed3ce16c3d2ba7cac321d29ec0a7d21408ea8437
>
Let me rebase the patch and send another version.