From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1LqxOt-0002Tu-Uf for qemu-devel@nongnu.org; Mon, 06 Apr 2009 18:34:44 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1LqxOp-0002Oq-FL for qemu-devel@nongnu.org; Mon, 06 Apr 2009 18:34:43 -0400 Received: from [199.232.76.173] (port=49706 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1LqxOp-0002Oi-6i for qemu-devel@nongnu.org; Mon, 06 Apr 2009 18:34:39 -0400 Received: from g4t0015.houston.hp.com ([15.201.24.18]:5112) by monty-python.gnu.org with esmtps (TLS-1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.60) (envelope-from ) id 1LqxOo-0003Au-O2 for qemu-devel@nongnu.org; Mon, 06 Apr 2009 18:34:39 -0400 From: Alex Williamson In-Reply-To: <49DA5CEA.7040209@us.ibm.com> References: <1237835133.7276.1107.camel@lappy> <1237835465.15558.4.camel@lappy> <49DA5CEA.7040209@us.ibm.com> Content-Type: text/plain Date: Mon, 06 Apr 2009 16:34:33 -0600 Message-Id: <1239057273.4726.76.camel@lappy> Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Subject: [Qemu-devel] Re: [PATCH 1/2] qemu: Allow SMBIOS entries to be loaded and provided to the VM BIOS Reply-To: qemu-devel@nongnu.org List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Anthony Liguori Cc: qemu-devel , kvm 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