| Message ID | 1237835465.15558.4.camel@lappy (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Alex Williamson wrote: > Create a new -smbios options that takes binary SMBIOS entries > to provide to the VM BIOS. The binary can be easily generated > using something like: > > dmidecode -t 1 -u | grep $'^\t\t[^"]' | xargs -n1 | \ > perl -lne 'printf "%c", hex($_)' > smbios_type_1.bin > > For some inventory tools, this makes the VM report the system > information for the host. One entry per binary file, multiple > files can be chained together as: > > -smbios file1,file2,... > > or specified independently: > > -smbios file1 -smbios file2 > > Signed-off-by: Alex Williamson <alex.williamson@hp.com> > Hi Alex, 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? 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. > +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). > + 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. > + 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.
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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Alex Williamson wrote: > 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 > You could go one level higher: -smbios type=0,bios-version='1.0',bios-date='2009/10/20' etc. > 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? > I think it's okay to start with some of the more common tables and provide the parsing in QEMU. We could then introduce humanize more tables down the road as people saw fit. At the end of the day, I'm most interested in the tables that are going to be frequently used by management applications. That is, the tables that are required for things like SVVP certification should be specified in a human readable format that QEMU can build reasonable defaults for and management tools can override. I'm torn between exposing the tables directly to the firmware or providing a higher level interface. I really don't like -uuid overriding a binary blob though so I'd prefer to avoid that. -uuid should only be respected if using the QEMU generated version of the SMBIOS table. I'll defer to whatever you think is better what is exposed in the firmware interface as I can see arguments for both.
On Mon, 2009-04-06 at 17:42 -0500, Anthony Liguori wrote: > Alex Williamson wrote: > > 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 > > > > You could go one level higher: > > -smbios type=0,bios-version='1.0',bios-date='2009/10/20' etc. That helps, but we have the same complexity in getting the data into the the bios. Adding a new QEMU_CFG_* for each field in every table we want to specify seems excessive. I'm half tempted to generate all the smbios entries in qemu and push them through a port to the bios. That would leave a lot of duplicate code in bochs though. Maybe the bios could read a stream of these through the port: uint16_t length; uint8_t type; /* 0: field, 1: table */ union { struct { uint8_t type; /* smbios entry type */ uint8_t offset; uint8_t data[]; } field; struct { uint8_t data[]; /* binary blob */ } table; } entry; We could convert uuid to use this too. The bios doesn't have a very effective means to seek through these, but maybe its not an issue as long as these entries are sparsely used. We could also use the table generation to enforce mutual exclusion between specifying fields and tables to avoid the uuid issue in the previous set. Other ideas? Thanks, Alex -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Alex Williamson wrote: > On Mon, 2009-04-06 at 17:42 -0500, Anthony Liguori wrote: > > That helps, but we have the same complexity in getting the data into the > the bios. Adding a new QEMU_CFG_* for each field in every table we want > to specify seems excessive. Right. > I'm half tempted to generate all the smbios > entries in qemu and push them through a port to the bios. That would > leave a lot of duplicate code in bochs though. Maybe the bios could > read a stream of these through the port: > > uint16_t length; > uint8_t type; /* 0: field, 1: table */ > union { > struct { > uint8_t type; /* smbios entry type */ > uint8_t offset; > uint8_t data[]; > } field; > struct { > uint8_t data[]; /* binary blob */ > } table; > } entry; > > We could convert uuid to use this too. Yes, this is what I was leaning toward too. > The bios doesn't have a very > effective means to seek through these, but maybe its not an issue as > long as these entries are sparsely used. We could also use the table > generation to enforce mutual exclusion between specifying fields and > tables to avoid the uuid issue in the previous set. Other ideas? > I'm pretty happy with this. The argument against it is that if we pass higher level information down via the FW interface, other types of FW (like OpenBIOS) could also use that information in a meaningful way. The problem is, beyond things like UUID, you start to get into pretty specific stuff and I'm not convinced it would all generalize very well. OEM tables are also impossible to represent this way. So yeah, plumbing the tables directly through to the BIOS seems to make sense to me. > Thanks, > > Alex > > >
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; + +int smbios_entry_add(const char *t) +{ + 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); + } + + fd = open(file, O_RDONLY); + if (fd < 0) + return -1; + + if (fstat(fd, &s) < 0) { + close(fd); + return -1; + } + + if (!smbios_entries) { + smbios_entries_len = sizeof(uint16_t); + smbios_entries = qemu_mallocz(smbios_entries_len); + } + + len = s.st_size; + smbios_entries = qemu_realloc(smbios_entries, smbios_entries_len + + len + sizeof(uint16_t)); + p = smbios_entries + smbios_entries_len; + + *(uint16_t *)p = cpu_to_le32(len); + p += sizeof(uint16_t); + + off = 0; + do { + r = read(fd, p + off, len); + if (r > 0) { + off += r; + len -= r; + } else if ((r < 0 && errno != EINTR) || r == 0) { + close(fd); + return -1; + } + } while (len); + + close(fd); + + smbios_entries_len += s.st_size + sizeof(uint16_t); + (*(uint16_t *)smbios_entries) = + cpu_to_le32(le32_to_cpu(*(uint16_t *)smbios_entries) + 1); + } while (*f); + + return 0; +} diff --git a/hw/pc.c b/hw/pc.c index 69f25f3..ec65e33 100644 --- a/hw/pc.c +++ b/hw/pc.c @@ -51,6 +51,7 @@ #define ACPI_DATA_SIZE 0x10000 #define BIOS_CFG_IOPORT 0x510 #define FW_CFG_ACPI_TABLES (FW_CFG_ARCH_LOCAL + 0) +#define FW_CFG_SMBIOS_ENTRIES (FW_CFG_ARCH_LOCAL + 1) #define MAX_IDE_BUS 2 @@ -442,6 +443,8 @@ static void bochs_bios_init(void) fw_cfg_add_i64(fw_cfg, FW_CFG_RAM_SIZE, (uint64_t)ram_size); fw_cfg_add_bytes(fw_cfg, FW_CFG_ACPI_TABLES, (uint8_t *)acpi_tables, acpi_tables_len); + fw_cfg_add_bytes(fw_cfg, FW_CFG_SMBIOS_ENTRIES, (uint8_t *)smbios_entries, + smbios_entries_len); } /* Generate an initial boot sector which sets state and jump to diff --git a/hw/pc.h b/hw/pc.h index 5b378d4..6c200b3 100644 --- a/hw/pc.h +++ b/hw/pc.h @@ -106,12 +106,15 @@ int ioport_get_a20(void); extern int acpi_enabled; extern char *acpi_tables; extern size_t acpi_tables_len; +extern char *smbios_entries; +extern size_t smbios_entries_len; i2c_bus *piix4_pm_init(PCIBus *bus, int devfn, uint32_t smb_io_base, qemu_irq sci_irq); void piix4_smbus_register_device(SMBusDevice *dev, uint8_t addr); void acpi_bios_init(void); int acpi_table_add(const char *table_desc); +int smbios_entry_add(const char *smbios_entry); /* hpet.c */ extern int no_hpet; diff --git a/vl.c b/vl.c index b62a2d4..372b83c 100644 --- a/vl.c +++ b/vl.c @@ -4061,6 +4061,7 @@ static void help(int exitcode) "-no-hpet disable HPET\n" "-acpitable [sig=str][,rev=n][,oem_id=str][,oem_table_id=str][,oem_rev=n][,asl_compiler_id=str][,asl_compiler_rev=n][,data=file1[:file2]...]\n" " ACPI table description\n" + "-smbios file1[,file2] SMBIOS entry\n" #endif "Linux boot specific:\n" "-kernel bzImage use 'bzImage' as kernel image\n" @@ -4201,6 +4202,7 @@ enum { QEMU_OPTION_no_acpi, QEMU_OPTION_no_hpet, QEMU_OPTION_acpitable, + QEMU_OPTION_smbios, /* Linux boot specific: */ QEMU_OPTION_kernel, @@ -4322,6 +4324,7 @@ static const QEMUOption qemu_options[] = { { "no-acpi", 0, QEMU_OPTION_no_acpi }, { "no-hpet", 0, QEMU_OPTION_no_hpet }, { "acpitable", HAS_ARG, QEMU_OPTION_acpitable }, + { "smbios", HAS_ARG, QEMU_OPTION_smbios }, #endif /* Linux boot specific: */ @@ -5152,6 +5155,12 @@ int main(int argc, char **argv, char **envp) exit(1); } break; + case QEMU_OPTION_smbios: + if(smbios_entry_add(optarg) < 0) { + fprintf(stderr, "Wrong smbios provided\n"); + exit(1); + } + break; #endif #ifdef USE_KQEMU case QEMU_OPTION_no_kqemu:
Create a new -smbios options that takes binary SMBIOS entries to provide to the VM BIOS. The binary can be easily generated using something like: dmidecode -t 1 -u | grep $'^\t\t[^"]' | xargs -n1 | \ perl -lne 'printf "%c", hex($_)' > smbios_type_1.bin For some inventory tools, this makes the VM report the system information for the host. One entry per binary file, multiple files can be chained together as: -smbios file1,file2,... or specified independently: -smbios file1 -smbios file2 Signed-off-by: Alex Williamson <alex.williamson@hp.com> -- -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html