On Wed, Sep 25, 2013 at 02:48:21AM +0300, Michael S. Tsirkin wrote:
> 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 <mst at redhat.com>
> > > ---
> > > 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, the math is wrong here.
What we want is header (sizeof(*files)) +
size of file array element multipled by number of files.
So we can replace nfiles with size / sizeof(*entry)
but that's not much simpler, is it?
> > Also, you should check that that the malloc succeeds.
> >
> > Otherwise, it looks okay to me.
> >
> > -Kevin