Re: [SeaBIOS] [PATCH] Remove the pmm handle argument from _malloc().

[Don Slutz]

On 12/27/13 11:35, Kevin O'Connor wrote:
> The PMM handle argument will almost always be 0xffffffff.  Use
> separate code for the few rare cases where it may not be the default
> value.  Gcc produces better code if _malloc() only requires three
> parameters.
> Signed-off-by: Kevin O'Connor
>
> src/fw/csm.c            |  4 ++--
>   src/fw/romfile_loader.c |  2 +-
>   src/malloc.c            | 23 ++++++++++++++++++-----
>   src/malloc.h            | 23 ++++++++++++-----------
>   src/pmm.c               | 19 ++++++++++++-------
>   5 files changed, 45 insertions(+), 26 deletions(-)
>
> diff --git a/src/fw/csm.c b/src/fw/csm.c
> index dfb0d12..a44ed26 100644
> --- a/src/fw/csm.c
> +++ b/src/fw/csm.c
> @@ -257,9 +257,9 @@ handle_csm_0006(struct bregs *regs)
>           size, align, region);
>   if (region & 2)
>    chunk = _malloc(&ZoneLow, MALLOC_DEFAULT_HANDLE, size, align);
>    chunk = _malloc(&ZoneLow, size, align);
> if (!chunk && (region & 1))
>    chunk = _malloc(&ZoneFSeg, MALLOC_DEFAULT_HANDLE, size, align);
>    chunk = _malloc(&ZoneFSeg, size, align);
>
> dprintf(3, "Legacy16GetTableAddress size %x align %x region %d yields %p\n",
>     size, align, region, chunk);
>
> diff --git a/src/fw/romfile_loader.c b/src/fw/romfile_loader.c
> index 325a0fa..f4b17ff 100644
> --- a/src/fw/romfile_loader.c
> +++ b/src/fw/romfile_loader.c
> @@ -57,7 +57,7 @@ static void romfile_loader_allocate(struct romfile_loader_entry_s *entry,
>       file->file = romfile_find(entry->alloc_file);
>       if (!file->file || !file->file->size)
>           return;
> data = _malloc(zone, MALLOC_DEFAULT_HANDLE, file->file->size, alloc_align);
> data = _malloc(zone, file->file->size, alloc_align);
>  if (!data) {
>      warn_noalloc();
>      return;
>
> diff --git a/src/malloc.c b/src/malloc.c
> index 6ef418a..c4cb171 100644
> --- a/src/malloc.c
> +++ b/src/malloc.c
> @@ -225,7 +225,7 @@ zonelow_expand(u32 size, u32 align, struct allocinfo_s *fill)
>
> // Allocate memory from the given zone and track it as a PMM allocation
>   void * __malloc
> -_malloc(struct zone_s *zone, u32 handle, u32 size, u32 align)
> +_malloc(struct zone_s *zone, u32 size, u32 align)
>   {
>       ASSERT32FLAT();
>       if (!size)
> @@ -240,6 +240,7 @@ _malloc(struct zone_s *zone, u32 handle, u32 size, u32 align)
>           if (!detail)
>               return NULL;
>       }
>
> detail->handle = MALLOC_DEFAULT_HANDLE;
>
> // Find and reserve space for main allocation
>  void *data = allocSpace(zone, size, align, &detail->datainfo);
>
> @@ -250,9 +251,8 @@ _malloc(struct zone_s *zone, u32 handle, u32 size, u32 align)
>           return NULL;
>       }
> dprintf(8, "_malloc zone=%p handle=%x size=%d align=%x ret=%p (detail=%p)\n"
>        , zone, handle, size, align, data, detail);
> detail->handle = handle;
>
> dprintf(8, "_malloc zone=%p size=%d align=%x ret=%p (detail=%p)\n"
>        , zone, size, align, data, detail);
>
> return data;
>
> }
>
> @@ -296,9 +296,22 @@ malloc_getspace(struct zone_s *zone)
>       return maxspace - reserve;
>   }
>
> +// Set a handle associated with an allocation.
> +void
> +malloc_sethandle(void *data, u32 handle)
> +{
> ASSERT32FLAT();
> struct allocinfo_s *info = findAlloc(data);
> if (!info || data == (void*)info || data == info->dataend)
>    return;
> struct allocdetail_s *detail = container_of(
>    info, struct allocdetail_s, datainfo);
> detail->handle = handle;

I would think a "dprint(8" should be here like:

dprintf(8, "malloc_sethandle handle=%x data=%p (detail=%p)\n"
             , handle, data, detail);

-Don Slutz
> +}
> // Find the data block allocated with _malloc with a given handle.
> void *
>
> -malloc_find(u32 handle)
> +malloc_findhandle(u32 handle)
>   {
>       int i;
>       for (i=0; i<ARRAY_SIZE(Zones); i++) {
> diff --git a/src/malloc.h b/src/malloc.h
> index af8a21d..2bcb5bf 100644
> --- a/src/malloc.h
> +++ b/src/malloc.h
> @@ -15,29 +15,30 @@ void malloc_preinit(void);
>   extern u32 LegacyRamSize;
>   void malloc_init(void);
>   void malloc_prepboot(void);
> -void *_malloc(struct zone_s *zone, u32 handle, u32 size, u32 align);
> +void *_malloc(struct zone_s *zone, u32 size, u32 align);
>   int _free(void *data);
>   u32 malloc_getspace(struct zone_s *zone);
> -void *malloc_find(u32 handle);
> +void malloc_sethandle(void *data, u32 handle);
> +void *malloc_findhandle(u32 handle);
>
> #define MALLOC_DEFAULT_HANDLE 0xFFFFFFFF
>   // Minimum alignment of malloc'd memory
>   #define MALLOC_MIN_ALIGN 16
>   // Helper functions for memory allocation.
>   static inline void *malloc_low(u32 size) {
> return _malloc(&ZoneLow, MALLOC_DEFAULT_HANDLE, size, MALLOC_MIN_ALIGN);
> return _malloc(&ZoneLow, size, MALLOC_MIN_ALIGN);
> }
> static inline void *malloc_high(u32 size) {
> return _malloc(&ZoneHigh, MALLOC_DEFAULT_HANDLE, size, MALLOC_MIN_ALIGN);
> return _malloc(&ZoneHigh, size, MALLOC_MIN_ALIGN);
> }
> static inline void *malloc_fseg(u32 size) {
> return _malloc(&ZoneFSeg, MALLOC_DEFAULT_HANDLE, size, MALLOC_MIN_ALIGN);
> return _malloc(&ZoneFSeg, size, MALLOC_MIN_ALIGN);
> }
> static inline void *malloc_tmplow(u32 size) {
> return _malloc(&ZoneTmpLow, MALLOC_DEFAULT_HANDLE, size, MALLOC_MIN_ALIGN);
> return _malloc(&ZoneTmpLow, size, MALLOC_MIN_ALIGN);
> }
> static inline void *malloc_tmphigh(u32 size) {
> return _malloc(&ZoneTmpHigh, MALLOC_DEFAULT_HANDLE, size, MALLOC_MIN_ALIGN);
> return _malloc(&ZoneTmpHigh, size, MALLOC_MIN_ALIGN);
> }
> static inline void *malloc_tmp(u32 size) {
>  void *ret = malloc_tmphigh(size);
>
> @@ -46,16 +47,16 @@ static inline void *malloc_tmp(u32 size) {
>       return malloc_tmplow(size);
>   }
>   static inline void *memalign_low(u32 align, u32 size) {
> return _malloc(&ZoneLow, MALLOC_DEFAULT_HANDLE, size, align);
> return _malloc(&ZoneLow, size, align);
> }
> static inline void *memalign_high(u32 align, u32 size) {
> return _malloc(&ZoneHigh, MALLOC_DEFAULT_HANDLE, size, align);
> return _malloc(&ZoneHigh, size, align);
> }
> static inline void *memalign_tmplow(u32 align, u32 size) {
> return _malloc(&ZoneTmpLow, MALLOC_DEFAULT_HANDLE, size, align);
> return _malloc(&ZoneTmpLow, size, align);
> }
> static inline void *memalign_tmphigh(u32 align, u32 size) {
> return _malloc(&ZoneTmpHigh, MALLOC_DEFAULT_HANDLE, size, align);
> return _malloc(&ZoneTmpHigh, size, align);
> }
> static inline void *memalign_tmp(u32 align, u32 size) {
>  void *ret = memalign_tmphigh(align, size);
>
> diff --git a/src/pmm.c b/src/pmm.c
> index be03bdb..304faab 100644
> --- a/src/pmm.c
> +++ b/src/pmm.c
> @@ -65,21 +65,26 @@ handle_pmm00(u16 *args)
>           if (align < MALLOC_MIN_ALIGN)
>               align = MALLOC_MIN_ALIGN;
>       }
> void *data;
>  switch (flags & 3) {
>  default:
>  case 0:
>      return 0;
>  case 1:
>    return (u32)_malloc(lowzone, handle, size, align);
>    data = _malloc(lowzone, size, align);
>    break;
> case 2:
>    return (u32)_malloc(highzone, handle, size, align);
>    data = _malloc(highzone, size, align);
>    break;
> case 3: {
>    void *data = _malloc(lowzone, handle, size, align);
>    if (data)
>        return (u32)data;
>    return (u32)_malloc(highzone, handle, size, align);
>    data = _malloc(lowzone, size, align);
>    if (!data)
>        data = _malloc(highzone, size, align);
> }
> }
>
> if (data && handle != MALLOC_DEFAULT_HANDLE)
>    malloc_sethandle(data, handle);
>
> return (u32)data;
> }
>
> // PMM - find
>
> @@ -90,7 +95,7 @@ handle_pmm01(u16 *args)
>       dprintf(3, "pmm01: handle=%x\n", handle);
>       if (handle == MALLOC_DEFAULT_HANDLE)
>           return 0;
> return (u32)malloc_find(handle);
>
> return (u32)malloc_findhandle(handle);
> }
>
> // PMM - deallocate

[PATCH] Remove the pmm handle argument from _malloc().

[Kevin O'Connor]

On Fri, Dec 27, 2013 at 05:07:17PM -0500, Don Slutz wrote:
> On 12/27/13 11:35, Kevin O'Connor wrote:
> > The PMM handle argument will almost always be 0xffffffff.  Use
> > separate code for the few rare cases where it may not be the default
> > value.  Gcc produces better code if _malloc() only requires three
> > parameters.

[...]
> > +// Set a handle associated with an allocation.
> > +void
> > +malloc_sethandle(void *data, u32 handle)
> > +{
> > ASSERT32FLAT();
> > struct allocinfo_s *info = findAlloc(data);
> > if (!info || data == (void*)info || data == info->dataend)
> >    return;
> > struct allocdetail_s *detail = container_of(
> >    info, struct allocdetail_s, datainfo);
> > detail->handle = handle;
>
> I would think a "dprint(8" should be here like:
> dprintf(8, "malloc_sethandle handle=%x data=%p (detail=%p)\n"
>         , handle, data, detail);

Thanks for reviewing.  The only place that calls malloc_sethandle
already shows the handle (handle_pmm00) and the rest of the info is
displayed by _malloc().

-Kevin

[PATCH] Remove the pmm handle argument from _malloc().

[Don Slutz]

On 12/27/13 17:19, Kevin O'Connor wrote:
> On Fri, Dec 27, 2013 at 05:07:17PM -0500, Don Slutz wrote:
> > On 12/27/13 11:35, Kevin O'Connor wrote:
> > > The PMM handle argument will almost always be 0xffffffff.  Use
> > > separate code for the few rare cases where it may not be the default
> > > value.  Gcc produces better code if _malloc() only requires three
> > > parameters.
>
> [...]
> > > +// Set a handle associated with an allocation.
> > > +void
> > > +malloc_sethandle(void *data, u32 handle)
> > > +{
> > > ASSERT32FLAT();
> > > struct allocinfo_s *info = findAlloc(data);
> > > if (!info || data == (void*)info || data == info->dataend)
> > >    return;
> > > struct allocdetail_s *detail = container_of(
> > >    info, struct allocdetail_s, datainfo);
> > > detail->handle = handle;
> >
> > I would think a "dprint(8" should be here like:
> >  dprintf(8, "malloc_sethandle handle=%x data=%p (detail=%p)\n"
> >          , handle, data, detail);
>
> Thanks for reviewing.  The only place that calls malloc_sethandle
> already shows the handle (handle_pmm00) and the rest of the info is
> displayed by _malloc().
>
> -Kevin

Ok, you can add my:

Reviewed-by: Don Slutz

-Don Slutz