Re: [OpenBIOS] [PATCH] OBP Memory deallocation

[Mark Cave-Ayland]

On 09/02/14 22:59, Olivier Danet wrote:
> During boot, NextSTEP do memory allocation & deallocation:
>
> Implement dumb_memfree()
>
> Unmap freed memory (during boot, all the virtual memory is probed,
>
> so freed memory must be properly unmapped)
> The pointer given to dumb_memfree() does not always match the pointer
>
> returned by dumb_memalloc().
> In that case, try to find a memory area which includes the pointer address.
> Ugly, indeed...
>
> Signed-off-by : Olivier Danet
> Index: arch/sparc32/lib.c
>
> --- arch/sparc32/lib.c (révision 1257)
> +++ arch/sparc32/lib.c (copie de travail)
> @@ -239,10 +245,18 @@
> return obp_memalloc(va, size, align);
> }
>
> -void obp_dumb_memfree(__attribute__((unused))char *va,
> __attribute__((unused))unsigned sz)
>
> +void obp_dumb_memfree(char *va, unsigned size)
> {
> DPRINTF("obp_dumb_memfree 0x%p (size %d)\n", va, sz);
> phys_addr_t phys;
> ucell cellmode;
> DPRINTF("obp_dumb_memfree: virta 0x%x, sz %d\n", (unsigned int)va, size);
> phys = ofmem_translate(pointer2cell(va), &cellmode);
> ofmem_unmap(pointer2cell(va), size);
> ofmem_release_virt(pointer2cell(va), size);
> ofmem_release_phys(phys, size);
>
> }
>
> /* Data fault handling routines */
> Index: arch/sparc32/ofmem_sparc32.c
> ===================================================================
> --- arch/sparc32/ofmem_sparc32.c (révision 1257)
> +++ arch/sparc32/ofmem_sparc32.c (copie de travail)
> @@ -133,17 +133,24 @@
> /* Generate memory available property entry for Sparc32 */
> void ofmem_arch_create_available_entry(phandle_t ph, ucell *availentry,
> phys_addr_t start, ucell size)
> {
> int i = 0;
> int i;
> i += ofmem_arch_encode_physaddr(availentry, start);
> i = ofmem_arch_encode_physaddr(availentry, start);
>
> availentry[i] = size;
> }
>
> /* Unmap a set of pages */
> void ofmem_arch_unmap_pages(ucell virt, ucell size)
> {
> /* OFMEM re-maps the pages for us, so just ensure the TLB is in sync */
> srmmu_flush_whole_tlb();
> unsigned i;
> unsigned long pa;
> for (i = 0; i < size; i += PAGE_SIZE) {
> pa = find_pte(virt, 0);
> *(uint32_t *)pa = 0;
> virt += PAGE_SIZE;
> }
> srmmu_flush_whole_tlb();
>
> }
>
> /* Map a set of pages */
> Index: arch/sparc32/romvec.h
> ===================================================================
> --- arch/sparc32/romvec.h (révision 1257)
> +++ arch/sparc32/romvec.h (copie de travail)
> @@ -30,8 +30,8 @@
> int obp_inst2pkg_handler(int dev_desc);
> char *obp_dumb_memalloc(char *va, unsigned int size);
> char *obp_dumb_memalloc_handler(char *va, unsigned int size);
> -void obp_dumb_memfree(__attribute__((unused))char *va,
> __attribute__((unused))unsigned sz);
> -void obp_dumb_memfree_handler(__attribute__((unused))char *va,
> __attribute__((unused))unsigned sz);
> +void obp_dumb_memfree(char *va, unsigned size);
> +void obp_dumb_memfree_handler(char *va, unsigned size);
> char *obp_dumb_mmap(char *va, int which_io, unsigned int pa, unsigned
> int size);
> char *obp_dumb_mmap_handler(char *va, int which_io, unsigned int pa,
> unsigned int size);
> void obp_dumb_munmap(__attribute__((unused)) char *va,
> __attribute__((unused)) unsigned int size);
> Index: libopenbios/ofmem_common.c
> ===================================================================
> --- libopenbios/ofmem_common.c (révision 1257)
> +++ libopenbios/ofmem_common.c (copie de travail)
> @@ -887,16 +887,25 @@
> cr = *r;
> *r = (**r).next;
> free(cr);
>
> return;
> }
> cr = *r;
> for ( ; *r && ((**r).next)->start < ea; r = &(**r).next) {
> cr = *r;
> }
> for( ; *r && ((**r).next)->start < ea; r=&(**r).next ) {
> if (((**r).next)->start == ea) {
> /* Found matching entry */
> cr = (**r).next;
> (**r).next = cr->next;
> free(cr);
> } else {
> /* Not exactly matching, delete the previous entry, with start < ea */
> free(cr->next);
> cr->next = (*r)->next;
> return;
>
> }
> cr = (**r).next;
> (**r).next = cr->next;
> free(cr);
>
> }
> static int remove_range( phys_addr_t ea, ucell size, range_t **r )

The first 2 parts of this patch look fine; however the last part seems 
absolutely unbelievable(!). I wonder if this could be side-effect of 
your change to the memory allocation function? It would definitely be 
worth checking this again once you've had a look at my comments from the 
previous memory allocation patch.

ATB,

Mark.