[RFC,v5,4/4] free the memory malloced by load_at()

Signed-off-by: Olivia Yin <hong-hua.yin@freescale.com>
---
 hw/elf_ops.h |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

On Wed, Nov 21, 2012 at 8:38 AM, Olivia Yin <hong-hua.yin@freescale.com> wrote:
> Signed-off-by: Olivia Yin <hong-hua.yin@freescale.com>
> ---
>  hw/elf_ops.h |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
>
> diff --git a/hw/elf_ops.h b/hw/elf_ops.h
> index b346861..9c76a75 100644
> --- a/hw/elf_ops.h
> +++ b/hw/elf_ops.h
> @@ -178,6 +178,8 @@ static int glue(load_symbols, SZ)(struct elfhdr *ehdr, int fd, int must_swab,
>      s->disas_strtab = str;
>      s->next = syminfos;
>      syminfos = s;
> +    g_free(syms);
> +    g_free(str);
>      g_free(shdr_table);
>      return 0;
>   fail:

Olivia, as Alex pointed out there are references to syms and str in
the struct "s"....so you can't just free those I don't think.

The problem that leaves us with is that on every reset when we call
load_elf() that we re-load and re-malloc space for the symbols.

I think the solution may be to factor out the call to load_symbols()
from load_elf().   It looks like what load_symbols does in the end is
set the variable syminfos to point to the loaded symbol info.

If you factor load_symbols() out then in load_elf_32/64() you would do
something like:
      elf_phy_loader_32/64()
      load_symbols_32/64().

We don't need to be reloading symbols on every reset.

Alex, does that make sense?

Stuart

On 11/21/2012 07:39 PM, Stuart Yoder wrote:
> On Wed, Nov 21, 2012 at 8:38 AM, Olivia Yin<hong-hua.yin@freescale.com>  wrote:
>> Signed-off-by: Olivia Yin<hong-hua.yin@freescale.com>
>> ---
>>   hw/elf_ops.h |    2 ++
>>   1 files changed, 2 insertions(+), 0 deletions(-)
>>
>> diff --git a/hw/elf_ops.h b/hw/elf_ops.h
>> index b346861..9c76a75 100644
>> --- a/hw/elf_ops.h
>> +++ b/hw/elf_ops.h
>> @@ -178,6 +178,8 @@ static int glue(load_symbols, SZ)(struct elfhdr *ehdr, int fd, int must_swab,
>>       s->disas_strtab = str;
>>       s->next = syminfos;
>>       syminfos = s;
>> +    g_free(syms);
>> +    g_free(str);
>>       g_free(shdr_table);
>>       return 0;
>>    fail:
> Olivia, as Alex pointed out there are references to syms and str in
> the struct "s"....so you can't just free those I don't think.
>
> The problem that leaves us with is that on every reset when we call
> load_elf() that we re-load and re-malloc space for the symbols.
>
> I think the solution may be to factor out the call to load_symbols()
> from load_elf().   It looks like what load_symbols does in the end is
> set the variable syminfos to point to the loaded symbol info.
>
> If you factor load_symbols() out then in load_elf_32/64() you would do
> something like:
>        elf_phy_loader_32/64()
>        load_symbols_32/64().
>
> We don't need to be reloading symbols on every reset.
>
> Alex, does that make sense?

We can also mandate the caller of load_symbols to free the respective 
data :)


Alex

Hi Stuart & Alex,

"syms" and "str" could not be free since "symbols" is a global variable which
need stay in the memory during the whole life of VM. So it will not be free and 
reload when reset.

The only change is to the previous patch of elf loader (hw/elf_ops.h) as below:

@@ -232,7 +235,9 @@ static int glue(load_elf, SZ)(const char *name, int fd,
     if (pentry)
        *pentry = (uint64_t)(elf_sword)ehdr.e_entry;

-    glue(load_symbols, SZ)(&ehdr, fd, must_swab, clear_lsb);
+    if (!reset) {
+        glue(load_symbols, SZ)(&ehdr, fd, must_swab, clear_lsb);
+    }

     size = ehdr.e_phnum * sizeof(phdr[0]);
     lseek(fd, ehdr.e_phoff, SEEK_SET); 

Do you think it is reasonable?

Best Regards,
Olivia

> -----Original Message-----
> From: Alexander Graf [mailto:agraf@suse.de]
> Sent: Thursday, November 22, 2012 2:42 AM
> To: Stuart Yoder
> Cc: Yin Olivia-R63875; qemu-devel@nongnu.org; qemu-ppc@nongnu.org
> Subject: Re: [Qemu-devel] [RFC PATCH v5 4/4] free the memory malloced by
> load_at()
> 
> On 11/21/2012 07:39 PM, Stuart Yoder wrote:
> > On Wed, Nov 21, 2012 at 8:38 AM, Olivia Yin<hong-hua.yin@freescale.com>
> wrote:
> >> Signed-off-by: Olivia Yin<hong-hua.yin@freescale.com>
> >> ---
> >>   hw/elf_ops.h |    2 ++
> >>   1 files changed, 2 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/hw/elf_ops.h b/hw/elf_ops.h index b346861..9c76a75
> >> 100644
> >> --- a/hw/elf_ops.h
> >> +++ b/hw/elf_ops.h
> >> @@ -178,6 +178,8 @@ static int glue(load_symbols, SZ)(struct elfhdr
> *ehdr, int fd, int must_swab,
> >>       s->disas_strtab = str;
> >>       s->next = syminfos;
> >>       syminfos = s;
> >> +    g_free(syms);
> >> +    g_free(str);
> >>       g_free(shdr_table);
> >>       return 0;
> >>    fail:
> > Olivia, as Alex pointed out there are references to syms and str in
> > the struct "s"....so you can't just free those I don't think.
> >
> > The problem that leaves us with is that on every reset when we call
> > load_elf() that we re-load and re-malloc space for the symbols.
> >
> > I think the solution may be to factor out the call to load_symbols()
> > from load_elf().   It looks like what load_symbols does in the end is
> > set the variable syminfos to point to the loaded symbol info.
> >
> > If you factor load_symbols() out then in load_elf_32/64() you would do
> > something like:
> >        elf_phy_loader_32/64()
> >        load_symbols_32/64().
> >
> > We don't need to be reloading symbols on every reset.
> >
> > Alex, does that make sense?
> 
> We can also mandate the caller of load_symbols to free the respective
> data :)
> 
> 
> Alex
>

On 26.11.2012, at 02:53, Yin Olivia-R63875 wrote:

> Hi Stuart & Alex,
> 
> "syms" and "str" could not be free since "symbols" is a global variable which
> need stay in the memory during the whole life of VM. So it will not be free and 
> reload when reset.

Ah, that's used for the debug log output, right?

> The only change is to the previous patch of elf loader (hw/elf_ops.h) as below:
> 
> @@ -232,7 +235,9 @@ static int glue(load_elf, SZ)(const char *name, int fd,
>     if (pentry)
>        *pentry = (uint64_t)(elf_sword)ehdr.e_entry;
> 
> -    glue(load_symbols, SZ)(&ehdr, fd, must_swab, clear_lsb);
> +    if (!reset) {
> +        glue(load_symbols, SZ)(&ehdr, fd, must_swab, clear_lsb);
> +    }
> 
>     size = ehdr.e_phnum * sizeof(phdr[0]);
>     lseek(fd, ehdr.e_phoff, SEEK_SET); 
> 
> Do you think it is reasonable?

I think semantically we want to only load the symbols the first time we load the binary (read: not on reset), yes. Where does the reset variable you're using there come from?


Alex

> 
> Best Regards,
> Olivia
> 
>> -----Original Message-----
>> From: Alexander Graf [mailto:agraf@suse.de]
>> Sent: Thursday, November 22, 2012 2:42 AM
>> To: Stuart Yoder
>> Cc: Yin Olivia-R63875; qemu-devel@nongnu.org; qemu-ppc@nongnu.org
>> Subject: Re: [Qemu-devel] [RFC PATCH v5 4/4] free the memory malloced by
>> load_at()
>> 
>> On 11/21/2012 07:39 PM, Stuart Yoder wrote:
>>> On Wed, Nov 21, 2012 at 8:38 AM, Olivia Yin<hong-hua.yin@freescale.com>
>> wrote:
>>>> Signed-off-by: Olivia Yin<hong-hua.yin@freescale.com>
>>>> ---
>>>>  hw/elf_ops.h |    2 ++
>>>>  1 files changed, 2 insertions(+), 0 deletions(-)
>>>> 
>>>> diff --git a/hw/elf_ops.h b/hw/elf_ops.h index b346861..9c76a75
>>>> 100644
>>>> --- a/hw/elf_ops.h
>>>> +++ b/hw/elf_ops.h
>>>> @@ -178,6 +178,8 @@ static int glue(load_symbols, SZ)(struct elfhdr
>> *ehdr, int fd, int must_swab,
>>>>      s->disas_strtab = str;
>>>>      s->next = syminfos;
>>>>      syminfos = s;
>>>> +    g_free(syms);
>>>> +    g_free(str);
>>>>      g_free(shdr_table);
>>>>      return 0;
>>>>   fail:
>>> Olivia, as Alex pointed out there are references to syms and str in
>>> the struct "s"....so you can't just free those I don't think.
>>> 
>>> The problem that leaves us with is that on every reset when we call
>>> load_elf() that we re-load and re-malloc space for the symbols.
>>> 
>>> I think the solution may be to factor out the call to load_symbols()
>>> from load_elf().   It looks like what load_symbols does in the end is
>>> set the variable syminfos to point to the loaded symbol info.
>>> 
>>> If you factor load_symbols() out then in load_elf_32/64() you would do
>>> something like:
>>>       elf_phy_loader_32/64()
>>>       load_symbols_32/64().
>>> 
>>> We don't need to be reloading symbols on every reset.
>>> 
>>> Alex, does that make sense?
>> 
>> We can also mandate the caller of load_symbols to free the respective
>> data :)
>> 
>> 
>> Alex
>> 
> 
>

I added parameter 'reset' into the original load_elf32/64() in patch 3/4.
Reserve the most lines and rename this function as elf_phy_loader32/64().

-static int glue(load_elf, SZ)(const char *name, int fd,
+static int glue(elf_phy_loader, SZ)(const char *name, int fd,
                               uint64_t (*translate_fn)(void *, uint64_t),
                               void *translate_opaque,
                               int must_swab, uint64_t *pentry,
                               uint64_t *lowaddr, uint64_t *highaddr,
-                              int elf_machine, int clear_lsb)
+                              int elf_machine, int clear_lsb, int reset)

Then load_elf32/64() and elf_reset32/64() will call this function with different reset values.

static void glue(elf_reset, SZ)(void *opaque)
{
    ImageElf *elf = opaque;
    int fd;

    fd = open(elf->name, O_RDONLY | O_BINARY);
    glue(elf_phy_loader, SZ)(elf->name, fd, elf->fn, elf->opaque, elf->swab,
                             NULL, NULL, NULL, elf->machine, elf->lsb, 1);
}

static int glue(load_elf, SZ)(const char *name, int fd,
                              uint64_t (*translate_fn)(void *, uint64_t),
                              void *translate_opaque,
                              int must_swab, uint64_t *pentry,
                              uint64_t *lowaddr, uint64_t *highaddr,
                              int elf_machine, int clear_lsb)
{
    int ret;

    ret = glue(elf_phy_loader, SZ)(name, fd, (*translate_fn), translate_opaque,
                                   must_swab, pentry, lowaddr, highaddr,
                                   elf_machine, clear_lsb, 0);
    return ret;
}

Best Regards,
Olivia                                                        

> -----Original Message-----
> From: Alexander Graf [mailto:agraf@suse.de]
> Sent: Monday, November 26, 2012 9:03 PM
> To: Yin Olivia-R63875
> Cc: Stuart Yoder; qemu-devel@nongnu.org; qemu-ppc@nongnu.org
> Subject: Re: [Qemu-devel] [RFC PATCH v5 4/4] free the memory malloced by
> load_at()
> 
> 
> On 26.11.2012, at 02:53, Yin Olivia-R63875 wrote:
> 
> > Hi Stuart & Alex,
> >
> > "syms" and "str" could not be free since "symbols" is a global
> > variable which need stay in the memory during the whole life of VM. So
> > it will not be free and reload when reset.
> 
> Ah, that's used for the debug log output, right?
> 
> > The only change is to the previous patch of elf loader (hw/elf_ops.h)
> as below:
> >
> > @@ -232,7 +235,9 @@ static int glue(load_elf, SZ)(const char *name, int
> fd,
> >     if (pentry)
> >        *pentry = (uint64_t)(elf_sword)ehdr.e_entry;
> >
> > -    glue(load_symbols, SZ)(&ehdr, fd, must_swab, clear_lsb);
> > +    if (!reset) {
> > +        glue(load_symbols, SZ)(&ehdr, fd, must_swab, clear_lsb);
> > +    }
> >
> >     size = ehdr.e_phnum * sizeof(phdr[0]);
> >     lseek(fd, ehdr.e_phoff, SEEK_SET);
> >
> > Do you think it is reasonable?
> 
> I think semantically we want to only load the symbols the first time we
> load the binary (read: not on reset), yes. Where does the reset variable
> you're using there come from?
> 
> 
> Alex
> 
> >
> > Best Regards,
> > Olivia
> >
> >> -----Original Message-----
> >> From: Alexander Graf [mailto:agraf@suse.de]
> >> Sent: Thursday, November 22, 2012 2:42 AM
> >> To: Stuart Yoder
> >> Cc: Yin Olivia-R63875; qemu-devel@nongnu.org; qemu-ppc@nongnu.org
> >> Subject: Re: [Qemu-devel] [RFC PATCH v5 4/4] free the memory malloced
> >> by
> >> load_at()
> >>
> >> On 11/21/2012 07:39 PM, Stuart Yoder wrote:
> >>> On Wed, Nov 21, 2012 at 8:38 AM, Olivia
> >>> Yin<hong-hua.yin@freescale.com>
> >> wrote:
> >>>> Signed-off-by: Olivia Yin<hong-hua.yin@freescale.com>
> >>>> ---
> >>>>  hw/elf_ops.h |    2 ++
> >>>>  1 files changed, 2 insertions(+), 0 deletions(-)
> >>>>
> >>>> diff --git a/hw/elf_ops.h b/hw/elf_ops.h index b346861..9c76a75
> >>>> 100644
> >>>> --- a/hw/elf_ops.h
> >>>> +++ b/hw/elf_ops.h
> >>>> @@ -178,6 +178,8 @@ static int glue(load_symbols, SZ)(struct elfhdr
> >> *ehdr, int fd, int must_swab,
> >>>>      s->disas_strtab = str;
> >>>>      s->next = syminfos;
> >>>>      syminfos = s;
> >>>> +    g_free(syms);
> >>>> +    g_free(str);
> >>>>      g_free(shdr_table);
> >>>>      return 0;
> >>>>   fail:
> >>> Olivia, as Alex pointed out there are references to syms and str in
> >>> the struct "s"....so you can't just free those I don't think.
> >>>
> >>> The problem that leaves us with is that on every reset when we call
> >>> load_elf() that we re-load and re-malloc space for the symbols.
> >>>
> >>> I think the solution may be to factor out the call to load_symbols()
> >>> from load_elf().   It looks like what load_symbols does in the end is
> >>> set the variable syminfos to point to the loaded symbol info.
> >>>
> >>> If you factor load_symbols() out then in load_elf_32/64() you would
> >>> do something like:
> >>>       elf_phy_loader_32/64()
> >>>       load_symbols_32/64().
> >>>
> >>> We don't need to be reloading symbols on every reset.
> >>>
> >>> Alex, does that make sense?
> >>
> >> We can also mandate the caller of load_symbols to free the respective
> >> data :)
> >>
> >>
> >> Alex
> >>
> >
> >
>