[PATCH 0/2] qemu: SMBIOS passing support

[PATCH 0/2] qemu: SMBIOS passing support

[Alex Williamson]

This series adds a new -smbios option for x86 that allows individual
SMBIOS entries to be passed into the guest VM.  This follows the same
basic path as the support for loading ACPI tables.  While SMBIOS is
independent of ACPI, I chose to add the smbios_entry_add() function to
acpi.c because they're both somewhat PC BIOS related (and ia64 can
support SMBIOS and might be able to make use of it there).

This feature allows the guest to see certain properties of the host if
configured correctly.  For instance, the system model and serial number
in the type 1 entry.  Obviously its only built at boot, so doesn't get
updated for migration scenarios.  User provided entries will supersede
generated entries, so care should be taken when passing entries which
describe physical properties, such as memory size and address ranges.
Thanks,

Alex 

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

[Alex Williamson]

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>
--

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:

[PATCH 2/2] qemu:bios: Read external SMBIOS entries from the VM

[Alex Williamson]

SMBIOS entries can be read from the VM using the same mechanism
as additional ACPI tables.  External entries will supercede
generated entries.

Signed-off-by: Alex Williamson <alex.williamson@hp.com>
--

diff --git a/bios/rombios32.c b/bios/rombios32.c
index 7be4216..f0e0f8c 100644
--- a/bios/rombios32.c
+++ b/bios/rombios32.c
@@ -471,6 +471,7 @@ void wrmsr_smp(uint32_t index, uint64_t val)
 #define QEMU_CFG_UUID       0x02
 #define QEMU_CFG_ARCH_LOCAL     0x8000
 #define QEMU_CFG_ACPI_TABLES  (QEMU_CFG_ARCH_LOCAL + 0)
+#define QEMU_CFG_SMBIOS_ENTRIES  (QEMU_CFG_ARCH_LOCAL + 1)
 
 int qemu_cfg_port;
 
@@ -519,6 +520,16 @@ static int acpi_load_table(int i, uint32_t addr, uint16_t *len)
     qemu_cfg_read((uint8_t*)addr, *len);
     return 0;
 }
+
+static uint16_t smbios_entries(void)
+{
+    uint16_t cnt;
+
+    qemu_cfg_select(QEMU_CFG_SMBIOS_ENTRIES);
+    qemu_cfg_read((uint8_t*)&cnt, sizeof(cnt));
+
+    return cnt;
+}
 #endif
 
 void uuid_probe(void)
@@ -1966,7 +1977,7 @@ smbios_entry_point_init(void *start,
 /* Type 0 -- BIOS Information */
 #define RELEASE_DATE_STR "01/01/2007"
 static void *
-smbios_type_0_init(void *start)
+smbios_init_type_0(void *start)
 {
     struct smbios_type_0 *p = (struct smbios_type_0 *)start;
 
@@ -2002,7 +2013,7 @@ smbios_type_0_init(void *start)
 
 /* Type 1 -- System Information */
 static void *
-smbios_type_1_init(void *start)
+smbios_init_type_1(void *start)
 {
     struct smbios_type_1 *p = (struct smbios_type_1 *)start;
     p->header.type = 1;
@@ -2028,7 +2039,7 @@ smbios_type_1_init(void *start)
 
 /* Type 3 -- System Enclosure */
 static void *
-smbios_type_3_init(void *start)
+smbios_init_type_3(void *start)
 {
     struct smbios_type_3 *p = (struct smbios_type_3 *)start;
 
@@ -2058,7 +2069,7 @@ smbios_type_3_init(void *start)
 
 /* Type 4 -- Processor Information */
 static void *
-smbios_type_4_init(void *start, unsigned int cpu_number)
+smbios_init_type_4(void *start, unsigned int cpu_number)
 {
     struct smbios_type_4 *p = (struct smbios_type_4 *)start;
 
@@ -2098,7 +2109,7 @@ smbios_type_4_init(void *start, unsigned int cpu_number)
 
 /* Type 16 -- Physical Memory Array */
 static void *
-smbios_type_16_init(void *start, uint32_t memsize, int nr_mem_devs)
+smbios_init_type_16(void *start, uint32_t memsize, int nr_mem_devs)
 {
     struct smbios_type_16 *p = (struct smbios_type_16*)start;
 
@@ -2121,7 +2132,7 @@ smbios_type_16_init(void *start, uint32_t memsize, int nr_mem_devs)
 
 /* Type 17 -- Memory Device */
 static void *
-smbios_type_17_init(void *start, uint32_t memory_size_mb, int instance)
+smbios_init_type_17(void *start, uint32_t memory_size_mb, int instance)
 {
     struct smbios_type_17 *p = (struct smbios_type_17 *)start;
 
@@ -2151,7 +2162,7 @@ smbios_type_17_init(void *start, uint32_t memory_size_mb, int instance)
 
 /* Type 19 -- Memory Array Mapped Address */
 static void *
-smbios_type_19_init(void *start, uint32_t memory_size_mb, int instance)
+smbios_init_type_19(void *start, uint32_t memory_size_mb, int instance)
 {
     struct smbios_type_19 *p = (struct smbios_type_19 *)start;
 
@@ -2172,7 +2183,7 @@ smbios_type_19_init(void *start, uint32_t memory_size_mb, int instance)
 
 /* Type 20 -- Memory Device Mapped Address */
 static void *
-smbios_type_20_init(void *start, uint32_t memory_size_mb, int instance)
+smbios_init_type_20(void *start, uint32_t memory_size_mb, int instance)
 {
     struct smbios_type_20 *p = (struct smbios_type_20 *)start;
 
@@ -2196,7 +2207,7 @@ smbios_type_20_init(void *start, uint32_t memory_size_mb, int instance)
 
 /* Type 32 -- System Boot Information */
 static void *
-smbios_type_32_init(void *start)
+smbios_init_type_32(void *start)
 {
     struct smbios_type_32 *p = (struct smbios_type_32 *)start;
 
@@ -2214,7 +2225,7 @@ smbios_type_32_init(void *start)
 
 /* Type 127 -- End of Table */
 static void *
-smbios_type_127_init(void *start)
+smbios_init_type_127(void *start)
 {
     struct smbios_type_127 *p = (struct smbios_type_127 *)start;
 
@@ -2228,6 +2239,91 @@ smbios_type_127_init(void *start)
     return start + 2;
 }
 
+static int
+smbios_load_external(int type, char **p, char **q, unsigned *nr_structs,
+                     unsigned *max_struct_size)
+{
+#ifdef BX_QEMU
+    static uint64_t used_bitmap[4] = { 0 };
+    static uint16_t used_cnt = 0;
+    char *start = *p;
+    uint16_t len;
+    int i;
+
+    /* Keep track of the entry types we've already processed */
+    if (used_bitmap[(type >> 6) & 0x3] & (1ULL << (type & 0x3f)))
+        return 1;
+
+    /* Skip end markers, they could lead to bogus tables */
+    if (type == 127)
+        return 0;
+
+    /* Check if there are any tables left to report, also reset read index */
+    i = smbios_entries();
+    if (used_cnt == i)
+        return 0;
+
+    for (; i > 0; *q = *p, i--) {
+        int string_data;
+        qemu_cfg_read((uint8_t*)&len, sizeof(len));
+        if (!len)
+            continue;
+        if (len < sizeof(struct smbios_structure_header)) {
+            while (len--)
+                inb(QEMU_CFG_DATA_PORT); /* Invalid, skip to the next one */
+            continue;
+        }
+
+        qemu_cfg_read((uint8_t*)*p, sizeof(struct smbios_structure_header));
+        if (((struct smbios_structure_header *)*p)->type != type) {
+            len -= sizeof(struct smbios_structure_header);
+            while (len--)
+                inb(QEMU_CFG_DATA_PORT); /* skip to the next one */
+            continue;
+        }
+
+        /* Entries end with a double NULL char, if there's a string at
+         * the end (length is greater than formatted length), the string
+         * terminator provides the first NULL. */
+        string_data = (len > ((struct smbios_structure_header *)*p)->length);
+
+        /* Read the rest and terminate the entry */
+        len -= sizeof(struct smbios_structure_header);
+        *p += sizeof(struct smbios_structure_header);
+        qemu_cfg_read((uint8_t*)*p, len);
+        *p += len;
+        *((uint8_t*)*p) = 0;
+        (*p)++;
+        if (!string_data) {
+            *((uint8_t*)*p) = 0;
+            (*p)++;
+        }
+
+        (*nr_structs)++;
+        used_cnt++;
+        if (*p - *q > *max_struct_size)
+            *max_struct_size = *p - *q;
+
+        /* Mark that we've reported on this type */
+        used_bitmap[(type >> 6) & 0x3] |= (1ULL << (type & 0x3f));
+
+        /* A type 1 entry provides a UUID, but so does the QEMU_CFG_UUID
+         * port.  If the QEMU_CFG_UUID value is not zero, use it, otherwise
+         * use whatever was in the provided table. */
+        if (type == 1) {
+            const static uint8_t null_uuid[16] = { 0 };
+            if (memcmp(bios_uuid, null_uuid, 16)) {
+                struct smbios_type_1 *t = (struct smbios_type_1 *)*q;
+                memcpy(t->uuid, bios_uuid, 16);
+            }
+        }
+    }
+    return (start != *p);
+#else /* !BX_QEMU */
+    return 0;
+#endif
+}
+
 void smbios_init(void)
 {
     unsigned cpu_num, nr_structs = 0, max_struct_size = 0;
@@ -2246,34 +2342,39 @@ void smbios_init(void)
 
 	p = (char *)start + sizeof(struct smbios_entry_point);
 
-#define add_struct(fn) do{ \
-    q = (fn); \
-    nr_structs++; \
-    if ((q - p) > max_struct_size) \
-        max_struct_size = q - p; \
-    p = q; \
+#define add_struct(type, args...) do{ \
+    if (!smbios_load_external(type, &p, &q, &nr_structs, &max_struct_size)) { \
+        q = smbios_init_type_##type(args); \
+        nr_structs++; \
+        if ((q - p) > max_struct_size) \
+            max_struct_size = q - p; \
+        p = q; \
+    } \
 }while (0)
 
-    add_struct(smbios_type_0_init(p));
-    add_struct(smbios_type_1_init(p));
-    add_struct(smbios_type_3_init(p));
+    add_struct(0, p);
+    add_struct(1, p);
+    add_struct(3, p);
     for (cpu_num = 1; cpu_num <= smp_cpus; cpu_num++)
-        add_struct(smbios_type_4_init(p, cpu_num));
+        add_struct(4, p, cpu_num);
 
     /* Each 'memory device' covers up to 16GB of address space. */
     nr_mem_devs = (memsize + 0x3fff) >> 14;
-    add_struct(smbios_type_16_init(p, memsize, nr_mem_devs));
+    add_struct(16, p, memsize, nr_mem_devs);
     for ( i = 0; i < nr_mem_devs; i++ )
     {
         uint32_t dev_memsize = ((i == (nr_mem_devs - 1))
                                 ? (((memsize-1) & 0x3fff)+1) : 0x4000);
-        add_struct(smbios_type_17_init(p, dev_memsize, i));
-        add_struct(smbios_type_19_init(p, dev_memsize, i));
-        add_struct(smbios_type_20_init(p, dev_memsize, i));
+        add_struct(17, p, dev_memsize, i);
+        add_struct(19, p, dev_memsize, i);
+        add_struct(20, p, dev_memsize, i);
     }
 
-    add_struct(smbios_type_32_init(p));
-    add_struct(smbios_type_127_init(p));
+    add_struct(32, p);
+    /* Add any remaining provided entries before the end marker */
+    for (i = 0; i < 256; i++)
+        smbios_load_external(i, &p, &q, &nr_structs, &max_struct_size);
+    add_struct(127, p);
 
 #undef add_struct
 

Re: [PATCH 0/2] qemu: SMBIOS passing support

[Alex Williamson]

Is there any interest in this series?  Aside from copying host SMBIOS
entries, it also seems useful for providing information to the guest
about their virtual machine pool (perhaps via a type 3 entry), or
whatever other bits of data someone might find useful (type 11, OEM
string for instance).  Thanks,

Alex

On Mon, 2009-03-23 at 13:05 -0600, Alex Williamson wrote:
> This series adds a new -smbios option for x86 that allows individual
> SMBIOS entries to be passed into the guest VM.  This follows the same
> basic path as the support for loading ACPI tables.  While SMBIOS is
> independent of ACPI, I chose to add the smbios_entry_add() function to
> acpi.c because they're both somewhat PC BIOS related (and ia64 can
> support SMBIOS and might be able to make use of it there).
> 
> This feature allows the guest to see certain properties of the host if
> configured correctly.  For instance, the system model and serial number
> in the type 1 entry.  Obviously its only built at boot, so doesn't get
> updated for migration scenarios.  User provided entries will supersede
> generated entries, so care should be taken when passing entries which
> describe physical properties, such as memory size and address ranges.
> Thanks,
> 
> Alex 
> 

Re: [PATCH 0/2] qemu: SMBIOS passing support

[Gleb Natapov]

On Mon, Mar 30, 2009 at 07:59:36AM -0600, Alex Williamson wrote:
> 
> Is there any interest in this series?  Aside from copying host SMBIOS
> entries, it also seems useful for providing information to the guest
> about their virtual machine pool (perhaps via a type 3 entry), or
> whatever other bits of data someone might find useful (type 11, OEM
> string for instance).  Thanks,
> 
I think the patch is useful. Haven't looked at implementation though.

> Alex
> 
> On Mon, 2009-03-23 at 13:05 -0600, Alex Williamson wrote:
> > This series adds a new -smbios option for x86 that allows individual
> > SMBIOS entries to be passed into the guest VM.  This follows the same
> > basic path as the support for loading ACPI tables.  While SMBIOS is
> > independent of ACPI, I chose to add the smbios_entry_add() function to
> > acpi.c because they're both somewhat PC BIOS related (and ia64 can
> > support SMBIOS and might be able to make use of it there).
> > 
> > This feature allows the guest to see certain properties of the host if
> > configured correctly.  For instance, the system model and serial number
> > in the type 1 entry.  Obviously its only built at boot, so doesn't get
> > updated for migration scenarios.  User provided entries will supersede
> > generated entries, so care should be taken when passing entries which
> > describe physical properties, such as memory size and address ranges.
> > 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

--
			Gleb.

Re: [PATCH 0/2] qemu: SMBIOS passing support

[Daniel P. Berrange]

On Mon, Mar 30, 2009 at 07:59:36AM -0600, Alex Williamson wrote:
> 
> Is there any interest in this series?  Aside from copying host SMBIOS
> entries, it also seems useful for providing information to the guest
> about their virtual machine pool (perhaps via a type 3 entry), or
> whatever other bits of data someone might find useful (type 11, OEM
> string for instance).  Thanks,
> 
> Alex
> 
> On Mon, 2009-03-23 at 13:05 -0600, Alex Williamson wrote:
> > This series adds a new -smbios option for x86 that allows individual
> > SMBIOS entries to be passed into the guest VM.  This follows the same
> > basic path as the support for loading ACPI tables.  While SMBIOS is
> > independent of ACPI, I chose to add the smbios_entry_add() function to
> > acpi.c because they're both somewhat PC BIOS related (and ia64 can
> > support SMBIOS and might be able to make use of it there).
> > 
> > This feature allows the guest to see certain properties of the host if
> > configured correctly.  For instance, the system model and serial number
> > in the type 1 entry.  Obviously its only built at boot, so doesn't get
> > updated for migration scenarios.  User provided entries will supersede
> > generated entries, so care should be taken when passing entries which
> > describe physical properties, such as memory size and address ranges.
> > Thanks,

I can't help thinking that if we wish to provide metadata to guest OS
like system model, serial number, etc, then we'd be better off using
explicit named flags (or QEMU config file settings once that exists)

  -system-serial 2141241521  -system-model "Some Virtual Machine"

and have QEMU generate the neccessary SMBIOS data, or other equivalent 
data tables to suit the non-PC based machine types for which SMBIOS
is not relevant.

Regards,
Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

Re: [PATCH 0/2] qemu: SMBIOS passing support

[Avi Kivity]

Daniel P. Berrange wrote:
> I can't help thinking that if we wish to provide metadata to guest OS
> like system model, serial number, etc, then we'd be better off using
> explicit named flags (or QEMU config file settings once that exists)
>
>   -system-serial 2141241521  -system-model "Some Virtual Machine"
>
> and have QEMU generate the neccessary SMBIOS data, or other equivalent 
> data tables to suit the non-PC based machine types for which SMBIOS
> is not relevant.
>   

-smbios serial=blah,model=bleach ?


-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 0/2] qemu: SMBIOS passing support

[Alex Williamson]

On Mon, 2009-03-30 at 17:59 +0300, Avi Kivity wrote:
> Daniel P. Berrange wrote:
> > I can't help thinking that if we wish to provide metadata to guest OS
> > like system model, serial number, etc, then we'd be better off using
> > explicit named flags (or QEMU config file settings once that exists)
> >
> >   -system-serial 2141241521  -system-model "Some Virtual Machine"
> >
> > and have QEMU generate the neccessary SMBIOS data, or other equivalent 
> > data tables to suit the non-PC based machine types for which SMBIOS
> > is not relevant.
> >   
> 
> -smbios serial=blah,model=bleach ?
> 

Unfortunately that does make them smbios specific, while I think Daniel
is pointing out that several options may be useful on other platforms.

This is basically the same issue we have with -uuid already.  -uuid is a
non-smbios specific option, but rombios will incorporate the data when
it builds the type 1 entry.  I've retained this functionality, so that a
-uuid option will override the uuid in a passed in type 1 entry.  This
could be further extended with separate patches to provide serial or
model numbers generically, but allow them to override smbios values.
This seems complimentary to the patches in this series, but I don't
think it replaces all the functionality we get from a raw smbios entry
interface.  Thanks,

Alex

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

[Anthony Liguori]

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.


-- 
Regards,

Anthony Liguori

Re: [PATCH 2/2] qemu:bios: Read external SMBIOS entries from the VM

[Anthony Liguori]

Alex Williamson wrote:
> SMBIOS entries can be read from the VM using the same mechanism
> as additional ACPI tables.  External entries will supercede
> generated entries.
>
> Signed-off-by: Alex Williamson <alex.williamson@hp.com>
> +
> +        /* A type 1 entry provides a UUID, but so does the QEMU_CFG_UUID
> +         * port.  If the QEMU_CFG_UUID value is not zero, use it, otherwise
> +         * use whatever was in the provided table. */
> +        if (type == 1) {
> +            const static uint8_t null_uuid[16] = { 0 };
> +            if (memcmp(bios_uuid, null_uuid, 16)) {
> +                struct smbios_type_1 *t = (struct smbios_type_1 *)*q;
> +                memcpy(t->uuid, bios_uuid, 16);
> +            }
> +        }
>   

This is what I was getting at in my other post.  I'd sort of rather that 
a certain set of SMBIOS tables be specified at a higher level (like 
uuid, manufacture, vendor, etc.) and the blobs just be the OEM tables.  
I think that matches the work going on with the device configuration 
files more appropriately.

Regards,

Anthony Liguori

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

[Alex Williamson]

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

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

[Anthony Liguori]

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.

-- 
Regards,

Anthony Liguori

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

[Alex Williamson]

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

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

[Anthony Liguori]

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
>
>
>   


-- 
Regards,

Anthony Liguori