Re: [PATCH 1/2] qemu: Allow SMBIOS entries to be loaded and provided to the VM BIOS

[Alex Williamson <alex.williamson@hp.com>]

Hi Anthony,

On Mon, 2009-04-06 at 14:50 -0500, Anthony Liguori wrote:
> Alex Williamson wrote:
> 
> I know we have to support blobs because of OEM specific smbios entries, 
> but there are a number of common ones that it would probably be good to 
> specify in a less user-unfriendly way.  What do you think?

Yeah, I'll admit this is a pretty unfriendly interface.  I get from your
comment on the other part of the patch that you'd prefer not to get into
the mess of having both binary blobs and command line switches
augmenting the blobs.  This seems reasonable, but also means that we
need a way to fully define the tables we generate from the command line.
For a type 0 entry, that might mean the following set of switches:

-bios-version, -bios-date, -bios-characteristics, -bios-release

And for a type 1:

-system-manufacturer, -system-name, -system-version, -system-serial,
-system-sku, -system-family

type 3:

-chassis-manufacturer, -chassis-type, -chassis-version, -chassis-serial,
-chassis-asset, -chassis-oem

I'm sure I'm missing some, plus we might want to allow the memory and
processor entries to have some fields changed.  Do we want to add that
many switches and means to access them from the rombios?

> Anyway, comments below.
> 
> > diff --git a/hw/acpi.c b/hw/acpi.c
> > index 52f50a0..0bd93bf 100644
> > --- a/hw/acpi.c
> > +++ b/hw/acpi.c
> > @@ -915,3 +915,69 @@ out:
> >      }
> >      return -1;
> >  }
> > +
> > +char *smbios_entries;
> > +size_t smbios_entries_len;
> >   
> 
> I think an accessor would be better than making these variables global.

Ok

> > +int smbios_entry_add(const char *t)
> > +{
> >   
> 
> acpi.c is hardware emulation, I'd rather see the command line parsing 
> done somewhere else (like vl.c).

Ok.  acpi.c was just a convenient place to not bother architectures that
don't care about smbios.

> > +    struct stat s;
> > +    char file[1024], *p, *f, *n;
> > +    int fd, r;
> > +    size_t len, off;
> > +
> > +    f = (char *)t;
> > +    do {
> > +        n = strchr(f, ',');
> > +        if (n) {
> > +            strncpy(file, f, (n - f));
> > +            file[n - f] = '\0';
> > +            f = n + 1;
> > +        } else {
> > +            strcpy(file, f);
> > +            f += strlen(file);
> > +        }
> >   
> 
> I'm happy to just require multiple -smbios options.  I dislike 
> overloading with ','s even though we do it a lot in QEMU.

Yup, I didn't have it initially, but added it because I thought someone
might complain other qemu options allow it.

> > +        fd = open(file, O_RDONLY);
> > +        if (fd < 0)
> > +            return -1;
> > +
> > +        if (fstat(fd, &s) < 0) {
> > +            close(fd);
> > +            return -1;
> > +        }
> >   
> 
> May want to look at load_image/get_image_size.

Will do.  Thanks,

Alex