Re: [Qemu-ppc] [Qemu-devel] [RFC PATCH v5 4/4] free the memory malloced by load_at()

[Yin Olivia-R63875]

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:address@hidden
> Sent: Monday, November 26, 2012 9:03 PM
> To: Yin Olivia-R63875
> Cc: Stuart Yoder; address@hidden; address@hidden
> 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:address@hidden
> >> Sent: Thursday, November 22, 2012 2:42 AM
> >> To: Stuart Yoder
> >> Cc: Yin Olivia-R63875; address@hidden; address@hidden
> >> 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<address@hidden>
> >> wrote:
> >>>> Signed-off-by: Olivia Yin<address@hidden>
> >>>> ---
> >>>>  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
> >>
> >
> >
>