Re: [SeaBIOS] [PATCH v4 1/5] romfile_loader: utility to patch in-memory ROM files

[Michael S. Tsirkin]

On Tue, Sep 24, 2013 at 07:32:10PM -0400, Kevin O'Connor wrote:
> On Mon, Sep 23, 2013 at 09:20:25PM +0300, Michael S. Tsirkin wrote:
> > Add ability for a ROM file to point to
> > it's image in memory. When file is in memory,
> > add utility that can patch it, storing
> > pointers to one file within another file.
> >
> > This is not a lot of code: together with the follow-up patch to load
> > ACPI tables from ROM, it's about 1K extra.
> > Signed-off-by: Michael S. Tsirkin
> >
> > Makefile             |   2 +-
> >  src/romfile_loader.h |  72 +++++++++++++++++++++
> >  src/fw/paravirt.c    |   2 +
> >  src/romfile_loader.c | 176 +++++++++++++++++++++++++++++++++++++++++++++++++++
> >  4 files changed, 251 insertions(+), 1 deletion(-)
> >  create mode 100644 src/romfile_loader.h
> >  create mode 100644 src/romfile_loader.c
> >
> > diff --git a/Makefile b/Makefile
> > index 3984d35..ae36dba 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -23,7 +23,7 @@ SRC32FLAT=$(SRCBOTH) post.c memmap.c malloc.c pmm.c romfile.c optionroms.c \
> >      hw/usb-hub.c \
> >      fw/coreboot.c fw/lzmadecode.c fw/csm.c fw/biostables.c \
> >      fw/paravirt.c fw/shadow.c fw/pciinit.c fw/smm.c fw/mtrr.c fw/xen.c \
> > fw/acpi.c fw/mptable.c fw/pirtable.c fw/smbios.c
> > fw/acpi.c fw/mptable.c fw/pirtable.c fw/smbios.c romfile_loader.c
>
> romfile_loader is a firmware file and show go in the fw/ directory.
>
> [...]
> > --- a/src/fw/paravirt.c
> > +++ b/src/fw/paravirt.c
> > @@ -365,6 +365,8 @@ void qemu_cfg_init(void)
> >      for (e = 0; e < count; e++) {
> >          struct QemuCfgFile qfile;
> >          qemu_cfg_read(&qfile, sizeof(qfile));
> > if (!*qfile.name)
> > return;
>
> What is this for?
>
> SeaBIOS uses spaces and no tabs.
>
> [...]
> > +int romfile_loader_execute(const char *name)
> > +{
> > struct romfile_loader_entry_s *entry;
> > int size, offset = 0, nfiles = 0;
> > struct romfile_loader_files *files;
> > void *data = romfile_loadfile(name, &size);
> > if (!data)
> >    return -1;
> > /* Validate and count files */
> > for (offset = 0; offset < size; offset += sizeof(*entry)) {
> >    entry = data + offset;
> >    /* Check that entry fits in buffer. */
> >    if (offset + sizeof(*entry) > size) {
> >        warn_internalerror();
> >        return -1;
> >    }
> >    if (le32_to_cpu(entry->command) == ROMFILE_LOADER_COMMAND_ALLOCATE) {
> >        nfiles++;
> > }
> > }
> > files = malloc_tmp(sizeof(*files) + nfiles * sizeof(files->files[0]));
> > files->nfiles = 0;
>
> Nitpick, but this would be much simpler if it was just:
> files = malloc_tmp(size / sizeof(*entry) * sizeof(*files));

Wait, the reason I didn't do this is because it wastes
memory: it includes chunksum and pointer entries,
we really only want alloc entries.
See?
> Also, you should check that that the malloc succeeds.
>
> Otherwise, it looks okay to me.
>
> -Kevin