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@siriusit.co.uk --- 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 )
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@siriusit.co.uk --- 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();
On Tue, Dec 21, 2010 at 10:38 AM, Mark Cave-Ayland mark.cave-ayland@siriusit.co.uk 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@siriusit.co.uk
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
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.
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@siriusit.co.uk --- 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;
On Tue, Dec 21, 2010 at 10:38 AM, Mark Cave-Ayland mark.cave-ayland@siriusit.co.uk 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@siriusit.co.uk
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
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.
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@siriusit.co.uk
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)
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.
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
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.
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.
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@siriusit.co.uk --- 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();
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@siriusit.co.uk
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
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.
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@siriusit.co.uk --- 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 */
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@siriusit.co.uk
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
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.
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
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.
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.
On Tue, Dec 21, 2010 at 10:38 AM, Mark Cave-Ayland mark.cave-ayland@siriusit.co.uk 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@siriusit.co.uk
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
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.
-
Andreas Färber -
Blue Swirl -
Mark Cave-Ayland -
Tarl Neustaedter