[PATCH 1/5] Fix alignment of memory allocated by OFMEM's ofmem_malloc().

[Mark Cave-Ayland]

      A casual glance at the source code suggests that pointers returned by ofmem_malloc() are
aligned, but in fact they are not
because sizeof(alloc_desc_t) is added to the final pointer return value. Create some
additional padding after the previous value
of ofmem->next_malloc so we can store the alloc_desc_t underneath the final return
pointer in memory, thus ensuring that the
final pointer is aligned correctly.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland(a)siriusit.co.uk&gt;
---
 openbios-devel/libopenbios/ofmem_common.c |   21 +++++++++++++--------
 1 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/openbios-devel/libopenbios/ofmem_common.c
b/openbios-devel/libopenbios/ofmem_common.c
index 20a4cba..a4e199c 100644
--- a/openbios-devel/libopenbios/ofmem_common.c
+++ b/openbios-devel/libopenbios/ofmem_common.c
@@ -33,6 +33,11 @@ static inline size_t ALIGN_SIZE(size_t x, size_t a)
     return (x + a - 1) & ~(a-1);
 }
 
+static inline void * ALIGN_PTR(uintptr_t x, size_t a)
+{
+    return (void *)((x + a - 1) & ~(a-1));
+}
+
 static ucell get_ram_size( void )
 {
 	ofmem_t *ofmem = ofmem_arch_get_private();
@@ -102,29 +107,29 @@ void* ofmem_malloc( size_t size )
 
 	/* waste at most 4K by taking an entry from the freelist */
 	if( *pp && (**pp).size < size + 0x1000 ) {
-		ret = (char*)*pp + sizeof(alloc_desc_t);
-		memset( ret, 0, (**pp).size - sizeof(alloc_desc_t) );
+		ret = (char *)((uintptr_t)*pp + sizeof(alloc_desc_t));
+		memset( (char *)ret, 0, (**pp).size - sizeof(alloc_desc_t) );
 		*pp = (**pp).next;
-		return ret;
+		return (void *)ret;
 	}
 
 	top = ofmem_arch_get_heap_top();
 
-	if( pointer2cell(ofmem->next_malloc) + size > top ) {
+	ret = (char *)ALIGN_PTR((uintptr_t)ofmem->next_malloc + sizeof(alloc_desc_t),
CONFIG_OFMEM_MALLOC_ALIGN);
+	if( pointer2cell((void *)ret) + size > top ) {
 		printk("out of malloc memory (%x)!\n", size );
 		return NULL;
 	}
 
-	d = (alloc_desc_t*) ofmem->next_malloc;
+	d = (alloc_desc_t*)((uintptr_t)ret - sizeof(alloc_desc_t));
 	ofmem->next_malloc += size;
 
 	d->next = NULL;
 	d->size = size;
 
-	ret = (char*)d + sizeof(alloc_desc_t);
-	memset( ret, 0, size - sizeof(alloc_desc_t) );
+	memset( (char *)ret, 0, size - sizeof(alloc_desc_t) );
 
-	return ret;
+	return (void *)ret;
 }
 
 void ofmem_free( void *ptr )
-- 
1.7.1

[PATCH 2/5] Introduce ofmem_malloc_align() function for arbitrary alignment.

[Mark Cave-Ayland]

      Rework ofmem_malloc() so that it takes a second parameter specifying the alignment for the
allocation and rename it to
ofmem_malloc_align(). Then create ofmem_malloc() as a simple wrapper function onto
ofmem_malloc_align() using a default
alignment of CONFIG_OFMEM_MALLOC_ALIGN.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland(a)siriusit.co.uk&gt;
---
 openbios-devel/include/libopenbios/ofmem.h |    1 +
 openbios-devel/libopenbios/ofmem_common.c  |   11 ++++++++---
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/openbios-devel/include/libopenbios/ofmem.h
b/openbios-devel/include/libopenbios/ofmem.h
index 6472008..72247d9 100644
--- a/openbios-devel/include/libopenbios/ofmem.h
+++ b/openbios-devel/include/libopenbios/ofmem.h
@@ -76,6 +76,7 @@ extern int      	ofmem_map_page_range( phys_addr_t phys, ucell virt,
ucell size,
                                       ucell mode );
 
 /* malloc interface */
+extern void* ofmem_malloc_align( size_t size, int alignment );
 extern void* ofmem_malloc( size_t size );
 extern void  ofmem_free( void *ptr );
 extern void* ofmem_realloc( void *ptr, size_t size );
diff --git a/openbios-devel/libopenbios/ofmem_common.c
b/openbios-devel/libopenbios/ofmem_common.c
index a4e199c..e892260 100644
--- a/openbios-devel/libopenbios/ofmem_common.c
+++ b/openbios-devel/libopenbios/ofmem_common.c
@@ -86,7 +86,7 @@ print_trans( void )
 /* OF private allocations                                               */
 /************************************************************************/
 
-void* ofmem_malloc( size_t size )
+void* ofmem_malloc_align( size_t size, int alignment )
 {
 	ofmem_t *ofmem = ofmem_arch_get_private();
 	alloc_desc_t *d, **pp;
@@ -99,7 +99,7 @@ void* ofmem_malloc( size_t size )
 	if( !ofmem->next_malloc )
 		ofmem->next_malloc = (char*)ofmem_arch_get_malloc_base();
 
-	size = ALIGN_SIZE(size + sizeof(alloc_desc_t), CONFIG_OFMEM_MALLOC_ALIGN);
+	size = ALIGN_SIZE(size + sizeof(alloc_desc_t), alignment);
 
 	/* look in the freelist */
 	for( pp=&ofmem->mfree; *pp && (**pp).size < size; pp =
&(**pp).next ) {
@@ -115,7 +115,7 @@ void* ofmem_malloc( size_t size )
 
 	top = ofmem_arch_get_heap_top();
 
-	ret = (char *)ALIGN_PTR((uintptr_t)ofmem->next_malloc + sizeof(alloc_desc_t),
CONFIG_OFMEM_MALLOC_ALIGN);
+	ret = (char *)ALIGN_PTR((uintptr_t)ofmem->next_malloc + sizeof(alloc_desc_t),
alignment);
 	if( pointer2cell((void *)ret) + size > top ) {
 		printk("out of malloc memory (%x)!\n", size );
 		return NULL;
@@ -132,6 +132,11 @@ void* ofmem_malloc( size_t size )
 	return (void *)ret;
 }
 
+void* ofmem_malloc( size_t size )
+{
+	return ofmem_malloc_align( size, CONFIG_OFMEM_MALLOC_ALIGN );
+}
+
 void ofmem_free( void *ptr )
 {
 	ofmem_t *ofmem = ofmem_arch_get_private();
-- 
1.7.1

[PATCH 2/5] Introduce ofmem_malloc_align() function for arbitrary alignment.

[Blue Swirl]

      On Tue, Dec 21, 2010 at 10:38 AM, Mark Cave-Ayland
<mark.cave-ayland(a)siriusit.co.uk&gt; wrote:
  Rework ofmem_malloc() so that it takes a second
parameter specifying the alignment for the allocation and rename it to
 ofmem_malloc_align(). Then create ofmem_malloc() as a simple wrapper function onto
ofmem_malloc_align() using a default
 alignment of CONFIG_OFMEM_MALLOC_ALIGN.

 Signed-off-by: Mark Cave-Ayland <mark.cave-ayland(a)siriusit.co.uk&gt;
 ---
  openbios-devel/include/libopenbios/ofmem.h |    1 +
  openbios-devel/libopenbios/ofmem_common.c  |   11 ++++++++---
  2 files changed, 9 insertions(+), 3 deletions(-)

 diff --git a/openbios-devel/include/libopenbios/ofmem.h
b/openbios-devel/include/libopenbios/ofmem.h
 index 6472008..72247d9 100644
 --- a/openbios-devel/include/libopenbios/ofmem.h
 +++ b/openbios-devel/include/libopenbios/ofmem.h
 @@ -76,6 +76,7 @@ extern int            ofmem_map_page_range( phys_addr_t phys, ucell
virt, ucell size,
                                       ucell mode );

  /* malloc interface */
 +extern void* ofmem_malloc_align( size_t size, int alignment );
The standard function with this signature would be posix_memalign(),
so how about ofmem_memalign() or even ofmem_posix_memalign()?

 'extern' isn't useful for function prototypes.

   extern void* ofmem_malloc( size_t size );
  extern void  ofmem_free( void *ptr );
  extern void* ofmem_realloc( void *ptr, size_t size );
 diff --git a/openbios-devel/libopenbios/ofmem_common.c
b/openbios-devel/libopenbios/ofmem_common.c
 index a4e199c..e892260 100644
 --- a/openbios-devel/libopenbios/ofmem_common.c
 +++ b/openbios-devel/libopenbios/ofmem_common.c
 @@ -86,7 +86,7 @@ print_trans( void )
  /* OF private allocations                                               */
  /************************************************************************/

 -void* ofmem_malloc( size_t size )
 +void* ofmem_malloc_align( size_t size, int alignment )
  {
        ofmem_t *ofmem = ofmem_arch_get_private();
        alloc_desc_t *d, **pp;
 @@ -99,7 +99,7 @@ void* ofmem_malloc( size_t size )
        if( !ofmem->next_malloc )
                ofmem->next_malloc = (char*)ofmem_arch_get_malloc_base();

 -       size = ALIGN_SIZE(size + sizeof(alloc_desc_t), CONFIG_OFMEM_MALLOC_ALIGN);
 +       size = ALIGN_SIZE(size + sizeof(alloc_desc_t), alignment);

        /* look in the freelist */
        for( pp=&ofmem->mfree; *pp && (**pp).size < size; pp =
&(**pp).next ) {
 @@ -115,7 +115,7 @@ void* ofmem_malloc( size_t size )

        top = ofmem_arch_get_heap_top();

 -       ret = (char *)ALIGN_PTR((uintptr_t)ofmem->next_malloc + sizeof(alloc_desc_t),
CONFIG_OFMEM_MALLOC_ALIGN);
 +       ret = (char *)ALIGN_PTR((uintptr_t)ofmem->next_malloc + sizeof(alloc_desc_t),
alignment);
        if( pointer2cell((void *)ret) + size > top ) {
                printk("out of malloc memory (%x)!\n", size );
                return NULL;
 @@ -132,6 +132,11 @@ void* ofmem_malloc( size_t size )
        return (void *)ret;
  }

 +void* ofmem_malloc( size_t size )
 +{
 +       return ofmem_malloc_align( size, CONFIG_OFMEM_MALLOC_ALIGN );
 +}
 +
  void ofmem_free( void *ptr )
  {
        ofmem_t *ofmem = ofmem_arch_get_private();
 --
 1.7.1


 --
 OpenBIOS                 http://openbios.org/
 Mailinglist:  http://lists.openbios.org/mailman/listinfo
 Free your System - May the Forth be with you

[PATCH 2/5] Introduce ofmem_malloc_align() function for arbitrary alignment.

[Mark Cave-Ayland]

      Blue Swirl wrote:

   /* malloc
interface */
 +extern void* ofmem_malloc_align( size_t size, int alignment );
 
 The standard function with this signature would be posix_memalign(),
 so how about ofmem_memalign() or even ofmem_posix_memalign()?
ofmem_posix_memalign() seems to fit the convention better, so I'll go 
with that unless anyone has any objections?

   'extern' isn't useful for function
prototypes.
Yeah... again just copying the existing style of the file.


ATB,

Mark.

-- 
Mark Cave-Ayland - Senior Technical Architect
PostgreSQL - PostGIS
Sirius Corporation plc - control through freedom
http://www.siriusit.co.uk
t: [phone]

Sirius Labs: http://www.siriusit.co.uk/labs

[PATCH 3/5] Alter ofmem_malloc_align() so that the resulting pointer is aligned for both physical and virtual addresses.

[Mark Cave-Ayland]

      Some uses of ofmem_malloc_align() need to ensure that the alignment requirement is for
physical as well as virtual addresses,
e.g. for MMU page tables.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland(a)siriusit.co.uk&gt;
---
 openbios-devel/include/arch/ppc/io.h      |   20 ++++++++++++++++++++
 openbios-devel/libopenbios/ofmem_common.c |   11 ++++++++---
 2 files changed, 28 insertions(+), 3 deletions(-)

diff --git a/openbios-devel/include/arch/ppc/io.h b/openbios-devel/include/arch/ppc/io.h
index 3449c5b..20586e4 100644
--- a/openbios-devel/include/arch/ppc/io.h
+++ b/openbios-devel/include/arch/ppc/io.h
@@ -9,6 +9,26 @@
 extern char _start, _end;
 extern unsigned long virt_offset;
 
+static inline unsigned long
+va2pa(unsigned long va)
+{
+    if ((va >= (unsigned long)&_start) &&
+        (va < (unsigned long)&_end))
+        return va - virt_offset;
+    else
+        return va;
+}
+
+static inline unsigned long
+pa2va(unsigned long pa)
+{
+    if ((pa + virt_offset >= (unsigned long)&_start) &&
+        (pa + virt_offset < (unsigned long)&_end))
+        return pa + virt_offset;
+    else
+        return pa;
+}
+
 #define phys_to_virt(phys) ((void *) ((unsigned long) (phys) - virt_offset))
 #define virt_to_phys(virt) ((unsigned long) (virt) + virt_offset)
 
diff --git a/openbios-devel/libopenbios/ofmem_common.c
b/openbios-devel/libopenbios/ofmem_common.c
index e892260..50cd7d8 100644
--- a/openbios-devel/libopenbios/ofmem_common.c
+++ b/openbios-devel/libopenbios/ofmem_common.c
@@ -33,9 +33,9 @@ static inline size_t ALIGN_SIZE(size_t x, size_t a)
     return (x + a - 1) & ~(a-1);
 }
 
-static inline void * ALIGN_PTR(uintptr_t x, size_t a)
+static inline phys_addr_t ALIGN_PTR(phys_addr_t x, size_t a)
 {
-    return (void *)((x + a - 1) & ~(a-1));
+    return (x + a - 1) & ~(a-1);
 }
 
 static ucell get_ram_size( void )
@@ -92,6 +92,7 @@ void* ofmem_malloc_align( size_t size, int alignment )
 	alloc_desc_t *d, **pp;
 	char *ret;
 	ucell top;
+	phys_addr_t pa;
 
 	if( !size )
 		return NULL;
@@ -115,7 +116,11 @@ void* ofmem_malloc_align( size_t size, int alignment )
 
 	top = ofmem_arch_get_heap_top();
 
-	ret = (char *)ALIGN_PTR((uintptr_t)ofmem->next_malloc + sizeof(alloc_desc_t),
alignment);
+	/* Alignment should be on physical not virtual address */
+	pa = va2pa((uintptr_t)ofmem->next_malloc + sizeof(alloc_desc_t));
+	pa = ALIGN_PTR(pa, alignment);
+	ret = (char *)pa2va(pa);
+
 	if( pointer2cell((void *)ret) + size > top ) {
 		printk("out of malloc memory (%x)!\n", size );
 		return NULL;
-- 
1.7.1

[PATCH 3/5] Alter ofmem_malloc_align() so that the resulting pointer is aligned for both physical and virtual addresses.

[Blue Swirl]

      On Tue, Dec 21, 2010 at 10:38 AM, Mark Cave-Ayland
<mark.cave-ayland(a)siriusit.co.uk&gt; wrote:
  Some uses of ofmem_malloc_align() need to ensure that
the alignment requirement is for physical as well as virtual addresses,
 e.g. for MMU page tables.

 Signed-off-by: Mark Cave-Ayland <mark.cave-ayland(a)siriusit.co.uk&gt;
 ---
  openbios-devel/include/arch/ppc/io.h      |   20 ++++++++++++++++++++
  openbios-devel/libopenbios/ofmem_common.c |   11 ++++++++---
  2 files changed, 28 insertions(+), 3 deletions(-)

 diff --git a/openbios-devel/include/arch/ppc/io.h b/openbios-devel/include/arch/ppc/io.h
 index 3449c5b..20586e4 100644
 --- a/openbios-devel/include/arch/ppc/io.h
 +++ b/openbios-devel/include/arch/ppc/io.h
 @@ -9,6 +9,26 @@
  extern char _start, _end;
  extern unsigned long virt_offset;

 +static inline unsigned long
phys_addr_t?

  +va2pa(unsigned long va)
 +{
 +    if ((va >= (unsigned long)&_start) &&
 +        (va < (unsigned long)&_end))
 +        return va - virt_offset;
 +    else
 +        return va;
 +}
 +
 +static inline unsigned long
 +pa2va(unsigned long pa)
phys_addr_t pa?

  +{
 +    if ((pa + virt_offset >= (unsigned long)&_start) &&
 +        (pa + virt_offset < (unsigned long)&_end))
 +        return pa + virt_offset;
 +    else
 +        return pa;
 +}
 +
  #define phys_to_virt(phys) ((void *) ((unsigned long) (phys) - virt_offset))
  #define virt_to_phys(virt) ((unsigned long) (virt) + virt_offset)

 diff --git a/openbios-devel/libopenbios/ofmem_common.c
b/openbios-devel/libopenbios/ofmem_common.c
 index e892260..50cd7d8 100644
 --- a/openbios-devel/libopenbios/ofmem_common.c
 +++ b/openbios-devel/libopenbios/ofmem_common.c
 @@ -33,9 +33,9 @@ static inline size_t ALIGN_SIZE(size_t x, size_t a)
     return (x + a - 1) & ~(a-1);
  }

 -static inline void * ALIGN_PTR(uintptr_t x, size_t a)
 +static inline phys_addr_t ALIGN_PTR(phys_addr_t x, size_t a)
  {
 -    return (void *)((x + a - 1) & ~(a-1));
 +    return (x + a - 1) & ~(a-1);
  }

  static ucell get_ram_size( void )
 @@ -92,6 +92,7 @@ void* ofmem_malloc_align( size_t size, int alignment )
        alloc_desc_t *d, **pp;
        char *ret;
        ucell top;
 +       phys_addr_t pa;

        if( !size )
                return NULL;
 @@ -115,7 +116,11 @@ void* ofmem_malloc_align( size_t size, int alignment )

        top = ofmem_arch_get_heap_top();

 -       ret = (char *)ALIGN_PTR((uintptr_t)ofmem->next_malloc + sizeof(alloc_desc_t),
alignment);
 +       /* Alignment should be on physical not virtual address */
 +       pa = va2pa((uintptr_t)ofmem->next_malloc + sizeof(alloc_desc_t));
 +       pa = ALIGN_PTR(pa, alignment);
 +       ret = (char *)pa2va(pa);
 +
        if( pointer2cell((void *)ret) + size > top ) {
                printk("out of malloc memory (%x)!\n", size );
                return NULL;
 --
 1.7.1


 --
 OpenBIOS                 http://openbios.org/
 Mailinglist:  http://lists.openbios.org/mailman/listinfo
 Free your System - May the Forth be with you

[PATCH 3/5] Alter ofmem_malloc_align() so that the resulting pointer is aligned for both physical and virtual addresses.

[Mark Cave-Ayland]

      Blue Swirl wrote:

  diff --git
a/openbios-devel/include/arch/ppc/io.h b/openbios-devel/include/arch/ppc/io.h
 index 3449c5b..20586e4 100644
 --- a/openbios-devel/include/arch/ppc/io.h
 +++ b/openbios-devel/include/arch/ppc/io.h
 @@ -9,6 +9,26 @@
  extern char _start, _end;
  extern unsigned long virt_offset;

 +static inline unsigned long
 
 phys_addr_t?
 
  +va2pa(unsigned long va)
 +{
 +    if ((va >= (unsigned long)&_start) &&
 +        (va < (unsigned long)&_end))
 +        return va - virt_offset;
 +    else
 +        return va;
 +}
 +
 +static inline unsigned long
 +pa2va(unsigned long pa)
 
 phys_addr_t pa?
 
> +{
> +    if ((pa + virt_offset >= (unsigned long)&_start) &&
> +        (pa + virt_offset < (unsigned long)&_end))
> +        return pa + virt_offset;
> +    else
> +        return pa;
> +}
Currently none of the SPARC pa <-> va conversion functions use 
phys_addr_t so I just copied what was already there. Perhaps it would 
make sense to change these to phys_addr_t before this commit?


ATB,

Mark.

-- 
Mark Cave-Ayland - Senior Technical Architect
PostgreSQL - PostGIS
Sirius Corporation plc - control through freedom
http://www.siriusit.co.uk
t: [phone]

Sirius Labs: http://www.siriusit.co.uk/labs

[PATCH 3/5] Alter ofmem_malloc_align() so that the resulting pointer is aligned for both physical and virtual addresses.

[Andreas Färber]

      Am 21.12.2010 um 11:38 schrieb Mark Cave-Ayland:

  Some uses of ofmem_malloc_align() need to ensure that
the alignment  
 requirement is for physical as well as virtual addresses,
 e.g. for MMU page tables.

 Signed-off-by: Mark Cave-Ayland <mark.cave-ayland(a)siriusit.co.uk&gt;
 ---
 openbios-devel/include/arch/ppc/io.h      |   20 ++++++++++++++++++++
 openbios-devel/libopenbios/ofmem_common.c |   11 ++++++++---
 2 files changed, 28 insertions(+), 3 deletions(-)

 diff --git a/openbios-devel/include/arch/ppc/io.h b/openbios-devel/ 
 include/arch/ppc/io.h
 index 3449c5b..20586e4 100644
 --- a/openbios-devel/include/arch/ppc/io.h
 +++ b/openbios-devel/include/arch/ppc/io.h
 @@ -9,6 +9,26 @@
 extern char _start, _end;
 extern unsigned long virt_offset;

 +static inline unsigned long
 +va2pa(unsigned long va)
 +{
 +    if ((va >= (unsigned long)&_start) &&
 +        (va < (unsigned long)&_end))
 +        return va - virt_offset;
 +    else
 +        return va;
 +}
 +
 +static inline unsigned long
 +pa2va(unsigned long pa)
 +{
 +    if ((pa + virt_offset >= (unsigned long)&_start) &&
 +        (pa + virt_offset < (unsigned long)&_end))
 +        return pa + virt_offset;
 +    else
 +        return pa;
 +}
--verbose please! What is this supposed to do? Seems like a duplicate  
of ofmem.c:ea_to_phys() but different?

Andreas

  +
 #define phys_to_virt(phys) ((void *) ((unsigned long) (phys) -  
 virt_offset))
 #define virt_to_phys(virt) ((unsigned long) (virt) + virt_offset)

[PATCH 3/5] Alter ofmem_malloc_align() so that the resulting pointer is aligned for both physical and virtual addresses.

[Mark Cave-Ayland]

      Andreas Färber wrote:

  --verbose please! What is this supposed to do? Seems
like a duplicate of 
 ofmem.c:ea_to_phys() but different?
 
 Andreas
It's used to convert between physical/virtual addresses. For alignment 
purposes, we need to ensure that memory allocated to a specific 
alignment is aligned both physically and virtually and since I couldn't 
see these functions in the PPC io.h file I just added them.

 From what you're saying it sounds like the proper fix would be to 
rename the PPC functions ea_to_phys() (and also it's reverse mapping 
function) to match the SPARC definitions so they can be called from 
OFMEM. Does that sound reasonable?


ATB,

Mark.

-- 
Mark Cave-Ayland - Senior Technical Architect
PostgreSQL - PostGIS
Sirius Corporation plc - control through freedom
http://www.siriusit.co.uk
t: [phone]

Sirius Labs: http://www.siriusit.co.uk/labs

[PATCH 3/5] Alter ofmem_malloc_align() so that the resulting pointer is aligned for both physical and virtual addresses.

[Andreas Färber]

      Am 22.12.2010 um 11:38 schrieb Mark Cave-Ayland:

  Andreas Färber wrote:

  --verbose please! What is this supposed to do?
Seems like a  
 duplicate of ofmem.c:ea_to_phys() but different?
 Andreas
 It's used to convert between physical/virtual addresses. For  
 alignment purposes, we need to ensure that memory allocated to a  
 specific alignment is aligned both physically and virtually and  
 since I couldn't see these functions in the PPC io.h file I just  
 added them.

 From what you're saying it sounds like the proper fix would be to  
 rename the PPC functions ea_to_phys() (and also it's reverse mapping  
 function) to match the SPARC definitions so they can be called from  
 OFMEM. Does that sound reasonable?
No. The virtual address on ppc64 is something like 80 bits, whereas  
the effective address (64 bits) really is what we want, similar  
differences for 32-bit, so it shouldn't be renamed. It may need to be  
wrapped.

I don't think it belongs in io.h either. I wouldn't know where  
virt_offset is initialized, so I assume that during runtime (as  
opposed to forthstrap) it will be zero and all you're doing there is  
identity-mapping things, which buys us nothing.

We do have ofmem_arch_translate() or similar for the virtual-to- 
physical direction. Maybe we need another ofmem_* one for the opposite  
direction? sparc implementations could then call their *va* macros  
from there if it suits them. The way we map OpenBIOS into RAM in  
ea2phys() should guarantee physical alignment of aligned effective  
addresses, so that there's no need to consider it in other functions  
(this is an ugly hack, in theory OpenBIOS would need to be fully  
relocatable and just copied there).

Andreas

[PATCH 3/5] Alter ofmem_malloc_align() so that the resulting pointer is aligned for both physical and virtual addresses.

[Mark Cave-Ayland]

      Andreas Färber wrote:

  From what
you're saying it sounds like the proper fix would be to 
 rename the PPC functions ea_to_phys() (and also it's reverse mapping 
 function) to match the SPARC definitions so they can be called from 
 OFMEM. Does that sound reasonable?
 
 No. The virtual address on ppc64 is something like 80 bits, whereas the 
 effective address (64 bits) really is what we want, similar differences 
 for 32-bit, so it shouldn't be renamed. It may need to be wrapped.
 
 I don't think it belongs in io.h either. I wouldn't know where 
 virt_offset is initialized, so I assume that during runtime (as opposed 
 to forthstrap) it will be zero and all you're doing there is 
 identity-mapping things, which buys us nothing.
For SPARC, the equivalent is va_shift which is calculated at the very 
beginning of arch/<foo>/entry.S (i.e. just after the MMU is enabled).

Hmmm so it looks like virt_offset isn't set for PPC at all! This is 
definitely where my lack of understanding the PPC code shows up.

  We do have ofmem_arch_translate() or similar for the
virtual-to-physical 
 direction. Maybe we need another ofmem_* one for the opposite direction? 
 sparc implementations could then call their *va* macros from there if it 
 suits them. The way we map OpenBIOS into RAM in ea2phys() should 
 guarantee physical alignment of aligned effective addresses, so that 
 there's no need to consider it in other functions (this is an ugly hack, 
 in theory OpenBIOS would need to be fully relocatable and just copied 
 there).
As long as we have two functions that convert from phys to virt and virt 
to phys for each arch (preferably called pa2va and va2pa) then I'm 
happy. Note that this is not for the alignment of OpenBIOS itself, but 
for allow callers of ofmem_malloc() to request arbitrary alignment on 
requested regions. This is mainly because the various IOMMU/MMU page 
tables require this restriction on physical addresses at the same time 
for obvious reasons.


ATB,

Mark.

-- 
Mark Cave-Ayland - Senior Technical Architect
PostgreSQL - PostGIS
Sirius Corporation plc - control through freedom
http://www.siriusit.co.uk
t: [phone]

Sirius Labs: http://www.siriusit.co.uk/labs

[PATCH 3/5] Alter ofmem_malloc_align() so that the resulting pointer is aligned for both physical and virtual addresses.

[Mark Cave-Ayland]

      On 22/12/10 12:53, Andreas Färber wrote:

  No. The virtual address on ppc64 is something like 80
bits, whereas the
 effective address (64 bits) really is what we want, similar differences
 for 32-bit, so it shouldn't be renamed. It may need to be wrapped.

 I don't think it belongs in io.h either. I wouldn't know where
 virt_offset is initialized, so I assume that during runtime (as opposed
 to forthstrap) it will be zero and all you're doing there is
 identity-mapping things, which buys us nothing.

 We do have ofmem_arch_translate() or similar for the virtual-to-physical
 direction. Maybe we need another ofmem_* one for the opposite direction?
 sparc implementations could then call their *va* macros from there if it
 suits them. The way we map OpenBIOS into RAM in ea2phys() should
 guarantee physical alignment of aligned effective addresses, so that
 there's no need to consider it in other functions (this is an ugly hack,
 in theory OpenBIOS would need to be fully relocatable and just copied
 there).
Hi Andreas,

I've had a play at using the following code in arch/ppc/qemu/ofmem.c 
(compared to the original patch which placed it in io.h) and it seems to 
work for me in tests here. It's based upon the code in ea2phys().


/* Private functions for mapping between physical/virtual addresses */
inline phys_addr_t
va2pa(unsigned long va)
{
	if (va >= OF_CODE_START && va < OF_CODE_START + OF_CODE_SIZE) {
         	return (phys_addr_t)get_rom_base() - OF_CODE_START + va;
	} else {
		return (phys_addr_t)va;
     	}
}

inline unsigned long
pa2va(phys_addr_t pa)
{
     if ((pa - get_rom_base() + OF_CODE_START >= OF_CODE_START) &&
         (pa - get_rom_base() + OF_CODE_START < OF_CODE_START + 
OF_CODE_SIZE))
         return (unsigned long)pa - get_rom_base() + OF_CODE_START;
     else
         return (unsigned long)pa;
}


The main reason I'm trying to get a resolution for this is because I've 
made some improvements based upon previous comments, and this is the 
only part remaining before I feel this patchset is ready for commit. 
Once this is then complete, it unblocks the SPARC32 OFMEM patchset which 
would be really good to get in before the end of the holidays :)


ATB,

Mark.

-- 
Mark Cave-Ayland - Senior Technical Architect
PostgreSQL - PostGIS
Sirius Corporation plc - control through freedom
http://www.siriusit.co.uk
t: [phone]

Sirius Labs: http://www.siriusit.co.uk/labs

[PATCH 4/5] Introduce ofmem_arch_get_phys_top() for specifying the top of physical memory in OFMEM.

[Mark Cave-Ayland]

      At the same time, teach OFMEM to make use of it when calculating the /memory available
property.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland(a)siriusit.co.uk&gt;
---
 openbios-devel/arch/ppc/qemu/ofmem.c        |    7 +++++++
 openbios-devel/arch/sparc64/ofmem_sparc64.c |    7 +++++++
 openbios-devel/include/libopenbios/ofmem.h  |    1 +
 openbios-devel/libopenbios/ofmem_common.c   |    2 +-
 4 files changed, 16 insertions(+), 1 deletions(-)

diff --git a/openbios-devel/arch/ppc/qemu/ofmem.c b/openbios-devel/arch/ppc/qemu/ofmem.c
index c21a112..ef8cfc3 100644
--- a/openbios-devel/arch/ppc/qemu/ofmem.c
+++ b/openbios-devel/arch/ppc/qemu/ofmem.c
@@ -115,6 +115,13 @@ ucell ofmem_arch_get_virt_top(void)
     return IO_BASE;
 }
 
+phys_addr_t ofmem_arch_get_phys_top(void)
+{
+	ofmem_t *ofmem = ofmem_arch_get_private();
+
+	return ofmem->ramsize;
+}
+
 void ofmem_arch_unmap_pages(ucell virt, ucell size)
 {
     /* kill page mappings in provided range */
diff --git a/openbios-devel/arch/sparc64/ofmem_sparc64.c
b/openbios-devel/arch/sparc64/ofmem_sparc64.c
index 4d13038..0ec8632 100644
--- a/openbios-devel/arch/sparc64/ofmem_sparc64.c
+++ b/openbios-devel/arch/sparc64/ofmem_sparc64.c
@@ -63,6 +63,13 @@ ucell ofmem_arch_get_virt_top(void)
 	return (ucell)TOP_OF_RAM;
 }
 
+phys_addr_t ofmem_arch_get_phys_top(void)
+{
+	ofmem_t *ofmem = ofmem_arch_get_private();
+
+	return ofmem->ramsize;
+}
+
 retain_t *ofmem_arch_get_retained(void)
 {
 	/* Retained area is at the top of physical RAM */
diff --git a/openbios-devel/include/libopenbios/ofmem.h
b/openbios-devel/include/libopenbios/ofmem.h
index 72247d9..b7469c2 100644
--- a/openbios-devel/include/libopenbios/ofmem.h
+++ b/openbios-devel/include/libopenbios/ofmem.h
@@ -62,6 +62,7 @@ extern ofmem_t* 	ofmem_arch_get_private(void);
 extern void*    	ofmem_arch_get_malloc_base(void);
 extern ucell    	ofmem_arch_get_heap_top(void);
 extern ucell    	ofmem_arch_get_virt_top(void);
+extern phys_addr_t 	ofmem_arch_get_phys_top(void);
 extern retain_t*	ofmem_arch_get_retained(void);
 extern int			ofmem_arch_get_physaddr_cellsize(void);
 extern int			ofmem_arch_encode_physaddr(ucell *p, phys_addr_t value);
diff --git a/openbios-devel/libopenbios/ofmem_common.c
b/openbios-devel/libopenbios/ofmem_common.c
index 50cd7d8..f235e98 100644
--- a/openbios-devel/libopenbios/ofmem_common.c
+++ b/openbios-devel/libopenbios/ofmem_common.c
@@ -341,7 +341,7 @@ static void ofmem_update_translations( void )
 	ofmem_t *ofmem = ofmem_arch_get_private();
 
 	ofmem_update_memory_available(s_phandle_memory, ofmem->phys_range, 
-			&phys_range_prop, &phys_range_prop_size, &phys_range_prop_used,
get_ram_size());
+			&phys_range_prop, &phys_range_prop_size, &phys_range_prop_used,
ofmem_arch_get_phys_top());
 	ofmem_update_memory_available(s_phandle_mmu, ofmem->virt_range, 
 			&virt_range_prop, &virt_range_prop_size, &virt_range_prop_used, -1ULL);
 	ofmem_update_mmu_translations();
-- 
1.7.1

[PATCH 4/5] Introduce ofmem_arch_get_phys_top() for specifying the top of physical memory in OFMEM.

[Andreas Färber]

      Am 21.12.2010 um 11:38 schrieb Mark Cave-Ayland:

  At the same time, teach OFMEM to make use of it when
calculating  
 the /memory available property.

 Signed-off-by: Mark Cave-Ayland <mark.cave-ayland(a)siriusit.co.uk&gt;
 ---
 openbios-devel/arch/ppc/qemu/ofmem.c        |    7 +++++++
 openbios-devel/arch/sparc64/ofmem_sparc64.c |    7 +++++++
 openbios-devel/include/libopenbios/ofmem.h  |    1 +
 openbios-devel/libopenbios/ofmem_common.c   |    2 +-
 4 files changed, 16 insertions(+), 1 deletions(-)
  diff --git a/openbios-devel/libopenbios/ofmem_common.c
b/openbios- 
 devel/libopenbios/ofmem_common.c
 index 50cd7d8..f235e98 100644
 --- a/openbios-devel/libopenbios/ofmem_common.c
 +++ b/openbios-devel/libopenbios/ofmem_common.c
 @@ -341,7 +341,7 @@ static void ofmem_update_translations( void )
 	ofmem_t *ofmem = ofmem_arch_get_private();

 	ofmem_update_memory_available(s_phandle_memory, ofmem->phys_range,
 -			&phys_range_prop, &phys_range_prop_size, &phys_range_prop_used,  
 get_ram_size());
 +			&phys_range_prop, &phys_range_prop_size, &phys_range_prop_used,  
 ofmem_arch_get_phys_top());
I like this change. Is get_ram_size() still needed elsewhere?

Andreas

[PATCH 4/5] Introduce ofmem_arch_get_phys_top() for specifying the top of physical memory in OFMEM.

[Mark Cave-Ayland]

      Andreas Färber wrote:

  I like this change. Is get_ram_size() still needed
elsewhere?
It's used in a couple of other places within OFMEM - it's more not 
knowing what should actually be in the properties rather than being a 
coding issue. Whichever way we find to be correct will be a really easy 
change :)


ATB,

Mark.

-- 
Mark Cave-Ayland - Senior Technical Architect
PostgreSQL - PostGIS
Sirius Corporation plc - control through freedom
http://www.siriusit.co.uk
t: [phone]

Sirius Labs: http://www.siriusit.co.uk/labs

[PATCH 5/5] Change OFMEM to allocate memory from the top of RAM downwards.

[Mark Cave-Ayland]

      At the moment OFMEM allocates memory from the bottom of RAM upwards, however many
loaders/kernels initialise themselves in lower
memory areas which can cause them to be overwritten by subsequent memory allocations.
Mimic the existing SPARC32 behaviour by
allocating physical and virtual memory from the top of RAM downwards.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland(a)siriusit.co.uk&gt;
---
 openbios-devel/libopenbios/ofmem_common.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/openbios-devel/libopenbios/ofmem_common.c
b/openbios-devel/libopenbios/ofmem_common.c
index f235e98..97493e6 100644
--- a/openbios-devel/libopenbios/ofmem_common.c
+++ b/openbios-devel/libopenbios/ofmem_common.c
@@ -498,7 +498,7 @@ phys_addr_t ofmem_claim_phys( phys_addr_t phys, ucell size, ucell
align )
                 " align=" FMT_ucellx "\n",
                 phys, size, align);
 
-	return ofmem_claim_phys_( phys, size, align, 0, get_ram_size(), 0 );
+	return ofmem_claim_phys_( phys, size, align, 0, ofmem_arch_get_phys_top(), 1 );
 }
 
 static ucell ofmem_claim_virt_( ucell virt, ucell size, ucell align,
@@ -531,7 +531,7 @@ ucell ofmem_claim_virt( ucell virt, ucell size, ucell align )
 
 	/* printk("+ ofmem_claim virt %08lx %lx %ld\n", virt, size, align ); */
 	return ofmem_claim_virt_( virt, size, align,
-			get_ram_size(), ofmem_arch_get_virt_top(), 0 );
+			get_ram_size(), ofmem_arch_get_virt_top(), 1 );
 }
 
 /* if align != 0, phys is ignored. Returns -1 on error */
-- 
1.7.1

[PATCH 5/5] Change OFMEM to allocate memory from the top of RAM downwards.

[Andreas Färber]

      Am 21.12.2010 um 11:38 schrieb Mark Cave-Ayland:

  At the moment OFMEM allocates memory from the bottom
of RAM upwards,  
 however many loaders/kernels initialise themselves in lower
 memory areas which can cause them to be overwritten by subsequent  
 memory allocations. Mimic the existing SPARC32 behaviour by
 allocating physical and virtual memory from the top of RAM downwards.

 Signed-off-by: Mark Cave-Ayland <mark.cave-ayland(a)siriusit.co.uk&gt;
While I'm not against reversing the direction, your above statement  
worries me. Our claim code should only claim free memory. So either  
the respective boot loader doesn't actually claim the memory it's  
using (the recent ppc bug that broke Haiku), or there's a bug in ofmem  
that you're hiding by this change. Both directions should in theory  
work, I believe.

Regards,
Andreas

[PATCH 5/5] Change OFMEM to allocate memory from the top of RAM downwards.

[Mark Cave-Ayland]

      Andreas Färber wrote:

  While I'm not against reversing the direction,
your above statement 
 worries me. Our claim code should only claim free memory. So either the 
 respective boot loader doesn't actually claim the memory it's using (the 
 recent ppc bug that broke Haiku), or there's a bug in ofmem that you're 
 hiding by this change. Both directions should in theory work, I believe.
Okay - having played with this a bit further, it seems that it's the 
physical memory allocations that need to be reversed in order to prevent 
my Solaris 8 kernel from crashing mid-boot. Below is the log from my 
OFMEM-enabled SPARC32 OpenBIOS booting Solaris 8 with physical memory 
allocated from the bottom upwards:


OFMEM: ofmem_map_page_range ffe27000 -> d00000000 00001000 mode 0000003c
Configuration device id QEMU version 1 machine id 32
OFMEM: ofmem_map_page_range ffe26000 -> 010000000 00001000 mode 0000003c
OFMEM: ofmem_map_page_range ffe25000 -> 071100000 00001000 mode 0000003c
OFMEM: ofmem_map_page_range ffe24000 -> 071000000 00001000 mode 0000003c
OFMEM: ofmem_map_page_range ffe22000 -> 071200000 00002000 mode 0000003c
OFMEM: ofmem_map_page_range ffe21000 -> 071400000 00001000 mode 0000003c
OFMEM: ofmem_map_page_range ffe20000 -> 071910000 00001000 mode 0000003c
OFMEM: ofmem_map_page_range ffe1f000 -> 071f00000 00001000 mode 0000003c
OFMEM: ofmem_map_page_range ffe0e000 -> 071d00000 00011000 mode 0000003c
OFMEM: ofmem_map_page_range ffdfd000 -> 071e00000 00011000 mode 0000003c
OFMEM: ofmem_map_page_range ffdfc000 -> 007fff000 00001000 mode 0000003c
OFMEM: ofmem_map_page_range ffdfb000 -> 010001000 00001000 mode 0000003c
OFMEM: ofmem_map_page_range ffdfa000 -> 078400000 00001000 mode 0000003c
OFMEM: ofmem_map_page_range ffdf9000 -> 078800000 00001000 mode 0000003c
CPUs: 1 x FMI,MB86904
UUID: 00000000-0000-0000-0000-000000000000
Welcome to OpenBIOS v1.0 built on Dec 22 2010 01:15
   Type 'help' for detailed information
Trying cdrom:d...
Not a bootable ELF image
Loading a.out image...
Loaded 7680 bytes
entry point is 0x4000
bootpath: /iommu/sbus/espdma/esp/sd@2,0:d

Jumping to entry point 00004000 for type 00000005...
switching to new context:
OFMEM: ofmem_claim phys=ffffffffffffffff size=00040000 align=00000008
OFMEM: ofmem_claim phys returned 000000000
OFMEM: ofmem_claim_virt virt=f0040000 size=00040000 align=00000000
OFMEM: ofmem_map_page_range f0040000 -> 000000000 00040000 mode 000000bc
OFMEM: ofmem_claim phys=ffffffffffffffff size=00019000 align=00000008
OFMEM: ofmem_claim phys returned 000040000
OFMEM: ofmem_claim_virt virt=f0240000 size=00019000 align=00000000
OFMEM: ofmem_map_page_range f0240000 -> 000040000 00019000 mode 000000bc
OFMEM: ofmem_claim phys=ffffffffffffffff size=00007000 align=00000008
OFMEM: ofmem_claim phys returned 000059000
OFMEM: ofmem_claim_virt virt=f0080000 size=00007000 align=00000000
OFMEM: ofmem_map_page_range f0080000 -> 000059000 00007000 mode 000000bc
OFMEM: ofmem_claim phys=ffffffffffffffff size=00001000 align=00000008
OFMEM: ofmem_claim phys returned 000060000
OFMEM: ofmem_claim_virt virt=f0087000 size=00001000 align=00000000
OFMEM: ofmem_map_page_range f0087000 -> 000060000 00001000 mode 000000bc
OFMEM: ofmem_claim phys=ffffffffffffffff size=00001000 align=00000008
OFMEM: ofmem_claim phys returned 000061000
OFMEM: ofmem_claim_virt virt=f0088000 size=00001000 align=00000000
OFMEM: ofmem_map_page_range f0088000 -> 000061000 00001000 mode 000000bc
OFMEM: ofmem_claim phys=ffffffffffffffff size=00003000 align=00000008
OFMEM: ofmem_claim phys returned 000062000
OFMEM: ofmem_claim_virt virt=f0089000 size=00003000 align=00000000
OFMEM: ofmem_map_page_range f0089000 -> 000062000 00003000 mode 000000bc
OFMEM: ofmem_claim phys=ffffffffffffffff size=00003000 align=00000008
OFMEM: ofmem_claim phys returned 000065000
OFMEM: ofmem_claim_virt virt=f008c000 size=00003000 align=00000000
OFMEM: ofmem_map_page_range f008c000 -> 000065000 00003000 mode 000000bc
OFMEM: ofmem_claim phys=ffffffffffffffff size=00001000 align=00000008
OFMEM: ofmem_claim phys returned 000068000
OFMEM: ofmem_claim_virt virt=f0259000 size=00001000 align=00000000
OFMEM: ofmem_map_page_range f0259000 -> 000068000 00001000 mode 000000bc
OFMEM: ofmem_claim phys=ffffffffffffffff size=00001000 align=00000008
OFMEM: ofmem_claim phys returned 000069000
OFMEM: ofmem_claim_virt virt=f025a000 size=00001000 align=00000000
OFMEM: ofmem_map_page_range f025a000 -> 000069000 00001000 mode 000000bc
OFMEM: ofmem_claim phys=ffffffffffffffff size=00001000 align=00000008
OFMEM: ofmem_claim phys returned 00006a000
OFMEM: ofmem_claim_virt virt=f025b000 size=00001000 align=00000000
OFMEM: ofmem_map_page_range f025b000 -> 00006a000 00001000 mode 000000bc
OFMEM: ofmem_claim phys=ffffffffffffffff size=00001000 align=00000008
OFMEM: ofmem_claim phys returned 00006b000
OFMEM: ofmem_claim_virt virt=f025c000 size=00001000 align=00000000
OFMEM: ofmem_map_page_range f025c000 -> 00006b000 00001000 mode 000000bc
OFMEM: ofmem_claim phys=ffffffffffffffff size=000c9000 align=00000008
OFMEM: ofmem_claim phys returned 00006c000
OFMEM: ofmem_claim_virt virt=f008f000 size=000c9000 align=00000000
OFMEM: ofmem_map_page_range f008f000 -> 00006c000 000c9000 mode 000000bc
OFMEM: ofmem_claim phys=ffffffffffffffff size=00018000 align=00000008
OFMEM: ofmem_claim phys returned 000135000
OFMEM: ofmem_claim_virt virt=f025d000 size=00018000 align=00000000
OFMEM: ofmem_map_page_range f025d000 -> 000135000 00018000 mode 000000bc

(hangs at this point)


The Solaris boot loader appears to be claiming specific virtual 
addresses, but claiming the physical addresses from the pool. The last 
physical address allocated from the pool before the crash is 0x135000 
which I think may be overwriting the kernel itself as I have something 
in the back of my mind that says the kernel is loaded at 0x100000 - Blue 
any ideas?


ATB,

Mark.

-- 
Mark Cave-Ayland - Senior Technical Architect
PostgreSQL - PostGIS
Sirius Corporation plc - control through freedom
http://www.siriusit.co.uk
t: [phone]

Sirius Labs: http://www.siriusit.co.uk/labs

[PATCH 5/5] Change OFMEM to allocate memory from the top of RAM downwards.

[Andreas Färber]

      Am 22.12.2010 um 11:49 schrieb Mark Cave-Ayland:

  Andreas Färber wrote:

  While I'm not against reversing the
direction, your above statement  
 worries me. Our claim code should only claim free memory. So either  
 the respective boot loader doesn't actually claim the memory it's  
 using (the recent ppc bug that broke Haiku), or there's a bug in  
 ofmem that you're hiding by this change. Both directions should in  
 theory work, I believe.
 Okay - having played with this a bit further, it seems that it's the  
 physical memory allocations that need to be reversed in order to  
 prevent my Solaris 8 kernel from crashing mid-boot. Below is the log  
 from my OFMEM-enabled SPARC32 OpenBIOS booting Solaris 8 with  
 physical memory allocated from the bottom upwards:
[...]
  OFMEM: ofmem_claim phys=ffffffffffffffff size=00018000
align=00000008
 OFMEM: ofmem_claim phys returned 000135000
 OFMEM: ofmem_claim_virt virt=f025d000 size=00018000 align=00000000
 OFMEM: ofmem_map_page_range f025d000 -> 000135000 00018000 mode  
 000000bc

 (hangs at this point)


 The Solaris boot loader appears to be claiming specific virtual  
 addresses, but claiming the physical addresses from the pool. The  
 last physical address allocated from the pool before the crash is  
 0x135000 which I think may be overwriting the kernel itself as I  
 have something in the back of my mind that says the kernel is loaded  
 at 0x100000 - Blue any ideas?
As a general piece of advice I would suggest to turn on debugging for  
the CIF. I would assume that the client simply calls "claim", then  
something might be wrong with the mapping to ofmem. Or Solaris makes  
specific assumptions, which Tarl might be able to answer. Maybe you  
need to claim specific parts of memory during initial setup so that  
they are exempt.

Andreas

[PATCH 5/5] Change OFMEM to allocate memory from the top of RAM downwards.

[Mark Cave-Ayland]

      Andreas Färber wrote:

  As a general piece of advice I would suggest to turn
on debugging for 
 the CIF. I would assume that the client simply calls "claim", then 
 something might be wrong with the mapping to ofmem. Or Solaris makes 
 specific assumptions, which Tarl might be able to answer. Maybe you need 
 to claim specific parts of memory during initial setup so that they are 
 exempt.
Okay. SPARC32 doesn't have CIF, rather it has a block of function 
pointers (called romvec) which used instead. Attached is the same output 
with romvec debugging enabled.


ATB,

Mark.

-- 
Mark Cave-Ayland - Senior Technical Architect
PostgreSQL - PostGIS
Sirius Corporation plc - control through freedom
http://www.siriusit.co.uk
t: [phone]

Sirius Labs: http://www.siriusit.co.uk/labs

[PATCH 5/5] Change OFMEM to allocate memory from the top of RAM downwards.

[Tarl Neustaedter]

      On 2010-12-22 7:26 AM, Andreas Färber wrote:
 
 As a general piece of advice I would suggest to turn on debugging for the 
 CIF. I would assume that the client simply calls "claim", then something 
 might be wrong with the mapping to ofmem. Or Solaris makes specific 
 assumptions, which Tarl might be able to answer. Maybe you need to claim 
 specific parts of memory during initial setup so that they are exempt.

I'm sure Solaris makes assumptions about memory regions, but they aren't 
documented anywhere I know of. OBP allocates physical memory from top 
downwards, and Solaris is supposed to avoid any OBP memory allocations by 
respecting the /memory and /virtual-memory nodes.

If we're running into a problem with virtual allocations, Solaris does
"know" 
that OBP lives in F000.0000 and above. It expects to allocate specific virtual 
addresses for itself (and 0x10.0000 does sound right for the initial kernel 
address), but these should also be kept from conflicting by the 
/virtual-memory properties. What happens if a specific virtual address that it 
expects to use is already occupied is an interesting question.

Looking at the OFMEM trace, the claim seems to go just fine (last claim phys 
was given 0x13.5000, last claim virt took 0xf025.d000), it was after the 
map_page_range mapping that physical memory to virtual address 0xf025.d000 
that we got stuck.

  OFMEM: ofmem_claim phys=ffffffffffffffff size=00018000
align=00000008
 OFMEM: ofmem_claim phys returned 000135000
 OFMEM: ofmem_claim_virt virt=f025d000 size=00018000 align=00000000
 OFMEM: ofmem_map_page_range f025d000 -> 000135000 00018000 mode 000000bc
That should be mapping f025.d000 through f027.5000 as virtual addresses of 
physical 13.5000 through 14.d000, which would certainly be suspicious in a Sun 
piece of hardware.

 From 0xf000.0000 through 0xf010.000 is a copy of the OBP prom binary (starts 
with "OBMD"), followed by OBP expanding itself into memory. At f020.0000 is 
(again, in our systems), the uncompressed binary - I recall f020.0000 is the 
actual trap vector. On current Sun systems, trying to use f025.d000 would 
overwrite OBP itself, currently in the middle of fb8 video interface code.

Looking at stuff in a real system, I don't see "unoccupied" virtual
addresses 
until about f040.0000 - if you can figure out who is coming up with the 
addresses of f025.d000 and (worse, earlier) f008.c000 , that might help figure 
out what's going on.

[PATCH 1/5] Fix alignment of memory allocated by OFMEM's ofmem_malloc().

[Blue Swirl]

      On Tue, Dec 21, 2010 at 10:38 AM, Mark Cave-Ayland
<mark.cave-ayland(a)siriusit.co.uk&gt; wrote:
  A casual glance at the source code suggests that
pointers returned by ofmem_malloc() are aligned, but in fact they are not
 because sizeof(alloc_desc_t) is added to the final pointer return value. Create some
additional padding after the previous value
 of ofmem->next_malloc so we can store the alloc_desc_t underneath the final return
pointer in memory, thus ensuring that the
 final pointer is aligned correctly.

 Signed-off-by: Mark Cave-Ayland <mark.cave-ayland(a)siriusit.co.uk&gt;
 ---
  openbios-devel/libopenbios/ofmem_common.c |   21 +++++++++++++--------
  1 files changed, 13 insertions(+), 8 deletions(-)

 diff --git a/openbios-devel/libopenbios/ofmem_common.c
b/openbios-devel/libopenbios/ofmem_common.c
 index 20a4cba..a4e199c 100644
 --- a/openbios-devel/libopenbios/ofmem_common.c
 +++ b/openbios-devel/libopenbios/ofmem_common.c
 @@ -33,6 +33,11 @@ static inline size_t ALIGN_SIZE(size_t x, size_t a)
     return (x + a - 1) & ~(a-1);
  }

 +static inline void * ALIGN_PTR(uintptr_t x, size_t a)
Uppercase name suggests a macro, please use lowercase.

  +{
 +    return (void *)((x + a - 1) & ~(a-1));
I'd add spaces inside a-1.

  +}
 +
  static ucell get_ram_size( void )
  {
        ofmem_t *ofmem = ofmem_arch_get_private();
 @@ -102,29 +107,29 @@ void* ofmem_malloc( size_t size )

        /* waste at most 4K by taking an entry from the freelist */
        if( *pp && (**pp).size < size + 0x1000 ) {
 -               ret = (char*)*pp + sizeof(alloc_desc_t);
 -               memset( ret, 0, (**pp).size - sizeof(alloc_desc_t) );
 +               ret = (char *)((uintptr_t)*pp + sizeof(alloc_desc_t));
 +               memset( (char *)ret, 0, (**pp).size - sizeof(alloc_desc_t) );
There are a lot of char* and void* casts, perhaps type of ret should
be void * to avoid some of them. At least (char *)ret shouldn't be
necessary anyway.

                 *pp = (**pp).next;
 -               return ret;
 +               return (void *)ret;
        }

        top = ofmem_arch_get_heap_top();

 -       if( pointer2cell(ofmem->next_malloc) + size > top ) {
 +       ret = (char *)ALIGN_PTR((uintptr_t)ofmem->next_malloc + sizeof(alloc_desc_t),
CONFIG_OFMEM_MALLOC_ALIGN);
 +       if( pointer2cell((void *)ret) + size > top ) {
                printk("out of malloc memory (%x)!\n", size );
                return NULL;
        }

 -       d = (alloc_desc_t*) ofmem->next_malloc;
 +       d = (alloc_desc_t*)((uintptr_t)ret - sizeof(alloc_desc_t));
        ofmem->next_malloc += size;

        d->next = NULL;
        d->size = size;

 -       ret = (char*)d + sizeof(alloc_desc_t);
 -       memset( ret, 0, size - sizeof(alloc_desc_t) );
 +       memset( (char *)ret, 0, size - sizeof(alloc_desc_t) );

 -       return ret;
 +       return (void *)ret;
  }

  void ofmem_free( void *ptr )
 --
 1.7.1


 --
 OpenBIOS                 http://openbios.org/
 Mailinglist:  http://lists.openbios.org/mailman/listinfo
 Free your System - May the Forth be with you

[PATCH 1/5] Fix alignment of memory allocated by OFMEM's ofmem_malloc().

[Mark Cave-Ayland]

      Blue Swirl wrote:

  +static inline
void * ALIGN_PTR(uintptr_t x, size_t a)
 
 Uppercase name suggests a macro, please use lowercase.
Yeah... this is a bit tricky as the style in OFMEM is different from 
elsewhere. My thinking was to try and match the existing style and then 
possibly attempt a tidy-up later after things are bedded in just to keep 
the commits cleaner.

  +{
 +    return (void *)((x + a - 1) & ~(a-1));
 
 I'd add spaces inside a-1.
Okay - will do.

  +}
 +
  static ucell get_ram_size( void )
  {
        ofmem_t *ofmem = ofmem_arch_get_private();
 @@ -102,29 +107,29 @@ void* ofmem_malloc( size_t size )

        /* waste at most 4K by taking an entry from the freelist */
        if( *pp && (**pp).size < size + 0x1000 ) {
 -               ret = (char*)*pp + sizeof(alloc_desc_t);
 -               memset( ret, 0, (**pp).size - sizeof(alloc_desc_t) );
 +               ret = (char *)((uintptr_t)*pp + sizeof(alloc_desc_t));
 +               memset( (char *)ret, 0, (**pp).size - sizeof(alloc_desc_t) );
 
 There are a lot of char* and void* casts, perhaps type of ret should
 be void * to avoid some of them. At least (char *)ret shouldn't be
 necessary anyway.
I did think about that, but wasn't sure if this would cause funny 
pointer maths bugs. If you think that would be okay, I can give it a go.


ATB,

Mark.

-- 
Mark Cave-Ayland - Senior Technical Architect
PostgreSQL - PostGIS
Sirius Corporation plc - control through freedom
http://www.siriusit.co.uk
t: [phone]

Sirius Labs: http://www.siriusit.co.uk/labs