[OpenBIOS] [PATCH] OBP Memory deallocation

[Olivier Danet]

On 26/03/2014 16:18, Mark Cave-Ayland wrote:
> On 10/02/14 01:40, Mark Cave-Ayland wrote:
> 
>> 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 <odanet at caramail.com>
>>>
>>> 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.
> 
> I spent a bit of time looking at this over the weekend, and during some testing was surprised to see that OFMEM's unmap_pages() would do the right thing in that it would locate the "correct" memory region, even if the specified boundaries weren't exactly the same as those used during creation. This meant that the failure was purely caused by remove_range_() being unable to handle the subsequent release with the same parameters.
> 
> With some further work, I came up with the attached patch which uses the same algorithm as unmap_page_range() to match regions for removal. I'm more confident in this method since it was written by Igor (rather than me!), gives a consistent behaviour between the two sets of functions, and as an extra plus is smaller to boot.
> 
> A quick spin around my SPARC32/SPARC64 test suite shows no regressions, so please test and let me know what you think.
> 
> 
> ATB,
> 
> Mark.
Hello Mark
I tested the patch and found no regression either,.. and it pleases NextStep.

Good !

Olivier