[RFC v1] hw/smbios: support for type 41 (onboard devices extended information)

Vincent Bernat posted 1 patch 3 years ago
Test checkpatch failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20210328205726.1330291-1-vincent@bernat.ch
Maintainers: "Michael S. Tsirkin" <mst@redhat.com>, Igor Mammedov <imammedo@redhat.com>
There is a newer version of this series
hw/smbios/smbios.c           | 117 +++++++++++++++++++++++++++++++++++
include/hw/firmware/smbios.h |  11 ++++
qemu-options.hx              |   7 ++-
3 files changed, 134 insertions(+), 1 deletion(-)
[RFC v1] hw/smbios: support for type 41 (onboard devices extended information)
Posted by Vincent Bernat 3 years ago
Type 41 defines the attributes of devices that are onboard. The
original intent was to imply the BIOS had some level of control over
the enablement of the associated devices.

If network devices are present in this table, by default, udev will
name the corresponding interfaces enoX, X being the instance number.
Without such information, udev will fallback to using the PCI ID and
this usually gives ens3 or ens4. This can be a bit annoying as the
name of the network card may depend on the order of options and may
change if a new PCI device is added earlier on the commande line.
Being able to provide SMBIOS type 41 entry ensure the name of the
interface won't change and helps the user guess the right name without
booting a first time.

This can be invoked with:

    $QEMU -netdev user,id=internet
          -device virtio-net-pci,mac=50:54:00:00:00:42,netdev=internet \
          -smbios type=41,designation=Onboard LAN,instance=1,kind=ethernet,pci=0000:00:09.0

Which results in the guest seeing dmidecode data and the interface
exposed as "eno1":

    $ dmidecode -t 41
    # dmidecode 3.3
    Getting SMBIOS data from sysfs.
    SMBIOS 2.8 present.Handle 0x2900, DMI type 41, 11 bytes
    Onboard Device
            Reference Designation: Onboard LAN
            Type: Ethernet
            Status: Enabled
            Type Instance: 1
            Bus Address: 0000:00:09.0
    $ udevadm info -p /sys/class/net/eno1 | grep ONBOARD
    E: ID_NET_NAME_ONBOARD=eno1
    E: ID_NET_LABEL_ONBOARD=Onboard LAN

The original plan was to directly provide a device and populate "kind"
and "pci" from the device. However, since the SMIBIOS tables are built
during argument evaluation, the information is not yet available.
I would welcome some guidance on how to implement this.

Signed-off-by: Vincent Bernat <vincent@bernat.ch>
---
 hw/smbios/smbios.c           | 117 +++++++++++++++++++++++++++++++++++
 include/hw/firmware/smbios.h |  11 ++++
 qemu-options.hx              |   7 ++-
 3 files changed, 134 insertions(+), 1 deletion(-)

diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
index f22c4f5b734e..b790045b007c 100644
--- a/hw/smbios/smbios.c
+++ b/hw/smbios/smbios.c
@@ -118,6 +118,32 @@ static struct {
     uint16_t speed;
 } type17;
 
+static QEnumLookup type41_kind_lookup = {
+    .array = (const char *const[]) {
+        "other",
+        "unknown",
+        "video",
+        "scsi",
+        "ethernet",
+        "tokenring",
+        "sound",
+        "pata",
+        "sata",
+        "sas",
+    },
+    .size = 10
+};
+struct type41_instance {
+    const char *designation;
+    uint8_t instance, kind;
+    struct {
+        uint16_t segment;
+        uint8_t bus, device;
+    } pci;
+    QTAILQ_ENTRY(type41_instance) next;
+};
+static QTAILQ_HEAD(, type41_instance) type41 = QTAILQ_HEAD_INITIALIZER(type41);
+
 static QemuOptsList qemu_smbios_opts = {
     .name = "smbios",
     .head = QTAILQ_HEAD_INITIALIZER(qemu_smbios_opts.head),
@@ -358,6 +384,33 @@ static const QemuOptDesc qemu_smbios_type17_opts[] = {
     { /* end of list */ }
 };
 
+static const QemuOptDesc qemu_smbios_type41_opts[] = {
+    {
+        .name = "type",
+        .type = QEMU_OPT_NUMBER,
+        .help = "SMBIOS element type",
+    },{
+        .name = "designation",
+        .type = QEMU_OPT_STRING,
+        .help = "reference designation string",
+    },{
+        .name = "kind",
+        .type = QEMU_OPT_STRING,
+        .help = "device type",
+        .def_value_str = "other",
+    },{
+        .name = "instance",
+        .type = QEMU_OPT_NUMBER,
+        .help = "device type instance",
+    },{
+        .name = "pci",
+        .type = QEMU_OPT_STRING,
+        .help = "PCI device",
+        .def_value_str = "0:0.0",
+    },
+    { /* end of list */ }
+};
+
 static void smbios_register_config(void)
 {
     qemu_add_opts(&qemu_smbios_opts);
@@ -773,6 +826,26 @@ static void smbios_build_type_32_table(void)
     SMBIOS_BUILD_TABLE_POST;
 }
 
+static void smbios_build_type_41_table(void)
+{
+    unsigned instance = 0;
+    struct type41_instance *t41;
+
+    QTAILQ_FOREACH(t41, &type41, next) {
+        SMBIOS_BUILD_TABLE_PRE(41, 0x2900 + instance, true);
+
+        SMBIOS_TABLE_SET_STR(41, reference_designation_str, t41->designation);
+        t->device_type = t41->kind;
+        t->device_type_instance = t41->instance;
+        t->segment_group_number = cpu_to_le16(t41->pci.segment);
+        t->bus_number = t41->pci.bus;
+        t->device_number = t41->pci.device;
+
+        SMBIOS_BUILD_TABLE_POST;
+        instance++;
+    }
+}
+
 static void smbios_build_type_127_table(void)
 {
     SMBIOS_BUILD_TABLE_PRE(127, 0x7F00, true); /* required */
@@ -928,6 +1001,7 @@ void smbios_get_tables(MachineState *ms,
 
         smbios_build_type_32_table();
         smbios_build_type_38_table();
+        smbios_build_type_41_table();
         smbios_build_type_127_table();
 
         smbios_validate_table(ms);
@@ -1224,6 +1298,49 @@ void smbios_entry_add(QemuOpts *opts, Error **errp)
             save_opt(&type17.part, opts, "part");
             type17.speed = qemu_opt_get_number(opts, "speed", 0);
             return;
+        case 41: {
+            struct type41_instance *t;
+            Error *local_err = NULL;
+            int pseg, pbus, pdevice, pfunction;
+
+            if (!qemu_opts_validate(opts, qemu_smbios_type41_opts, errp)) {
+                return;
+            }
+            if ((t = calloc(1, sizeof(struct type41_instance))) == NULL) {
+                error_setg(errp,
+                           "Unable to allocate memory for a new type 41 instance");
+                return;
+            }
+
+            save_opt(&t->designation, opts, "designation");
+            t->kind = qapi_enum_parse(&type41_kind_lookup,
+                                      qemu_opt_get(opts, "kind"),
+                                      0, &local_err) + 1;
+            t->kind |= 0x80;     /* enabled */
+            if (local_err != NULL) {
+                error_propagate(errp, local_err);
+                free(t);
+                return;
+            }
+            t->instance = qemu_opt_get_number(opts, "instance", 1);
+            if (sscanf(qemu_opt_get(opts, "pci"), "%x:%x:%x.%x",
+                       &pseg,
+                       &pbus,
+                       &pdevice,
+                       &pfunction) != 4) {
+                error_setg(errp, "unable to parse %s: %s",
+                           qemu_opt_get(opts, "pci"),
+                           g_strerror(errno));
+                free(t);
+                return;
+            }
+            t->pci.segment = pseg;
+            t->pci.bus = pbus;
+            t->pci.device = ((uint8_t)pdevice << 3) + ((uint8_t)pfunction & 0x7);
+
+            QTAILQ_INSERT_TAIL(&type41, t, next);
+            return;
+        }
         default:
             error_setg(errp,
                        "Don't know how to build fields for SMBIOS type %ld",
diff --git a/include/hw/firmware/smbios.h b/include/hw/firmware/smbios.h
index 02a0ced0a09f..4504dd03c303 100644
--- a/include/hw/firmware/smbios.h
+++ b/include/hw/firmware/smbios.h
@@ -258,6 +258,17 @@ struct smbios_type_32 {
     uint8_t boot_status;
 } QEMU_PACKED;
 
+/* SMBIOS type 41 - Onboard Devices Extended Information */
+struct smbios_type_41 {
+    struct smbios_structure_header header;
+    uint8_t reference_designation_str;
+    uint8_t device_type;
+    uint8_t device_type_instance;
+    uint16_t segment_group_number;
+    uint8_t bus_number;
+    uint8_t device_number;
+} QEMU_PACKED;
+
 /* SMBIOS type 127 -- End-of-table */
 struct smbios_type_127 {
     struct smbios_structure_header header;
diff --git a/qemu-options.hx b/qemu-options.hx
index fd21002bd61d..eb2de7c372c7 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -2370,7 +2370,9 @@ DEF("smbios", HAS_ARG, QEMU_OPTION_smbios,
     "                specify SMBIOS type 11 fields\n"
     "-smbios type=17[,loc_pfx=str][,bank=str][,manufacturer=str][,serial=str]\n"
     "               [,asset=str][,part=str][,speed=%d]\n"
-    "                specify SMBIOS type 17 fields\n",
+    "                specify SMBIOS type 17 fields\n"
+    "-smbios type=41[,designation=str][,kind=str][,instance=%d][,pci=%x:%x:%x.%x]\n"
+    "                specify SMBIOS type 41 fields\n",
     QEMU_ARCH_I386 | QEMU_ARCH_ARM)
 SRST
 ``-smbios file=binary``
@@ -2432,6 +2434,9 @@ SRST
 
 ``-smbios type=17[,loc_pfx=str][,bank=str][,manufacturer=str][,serial=str][,asset=str][,part=str][,speed=%d]``
     Specify SMBIOS type 17 fields
+
+``-smbios type=41[,designation=str][,kind=str][,instance=%d][,dev=str]``
+    Specify SMBIOS type 41 fields
 ERST
 
 DEFHEADING()
-- 
2.31.0


Re: [RFC v1] hw/smbios: support for type 41 (onboard devices extended information)
Posted by no-reply@patchew.org 3 years ago
Patchew URL: https://patchew.org/QEMU/20210328205726.1330291-1-vincent@bernat.ch/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20210328205726.1330291-1-vincent@bernat.ch
Subject: [RFC v1] hw/smbios: support for type 41 (onboard devices extended information)

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]         patchew/20210328205726.1330291-1-vincent@bernat.ch -> patchew/20210328205726.1330291-1-vincent@bernat.ch
Switched to a new branch 'test'
e30795d hw/smbios: support for type 41 (onboard devices extended information)

=== OUTPUT BEGIN ===
ERROR: do not use assignment in if condition
#175: FILE: hw/smbios/smbios.c:1309:
+            if ((t = calloc(1, sizeof(struct type41_instance))) == NULL) {

WARNING: line over 80 characters
#205: FILE: hw/smbios/smbios.c:1339:
+            t->pci.device = ((uint8_t)pdevice << 3) + ((uint8_t)pfunction & 0x7);

total: 1 errors, 1 warnings, 183 lines checked

Commit e30795d491a0 (hw/smbios: support for type 41 (onboard devices extended information)) has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20210328205726.1330291-1-vincent@bernat.ch/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
Re: [RFC v1] hw/smbios: support for type 41 (onboard devices extended information)
Posted by Daniel P. Berrangé 2 years, 12 months ago
On Sun, Mar 28, 2021 at 10:57:26PM +0200, Vincent Bernat wrote:
> Type 41 defines the attributes of devices that are onboard. The
> original intent was to imply the BIOS had some level of control over
> the enablement of the associated devices.
> 
> If network devices are present in this table, by default, udev will
> name the corresponding interfaces enoX, X being the instance number.
> Without such information, udev will fallback to using the PCI ID and
> this usually gives ens3 or ens4. This can be a bit annoying as the
> name of the network card may depend on the order of options and may
> change if a new PCI device is added earlier on the commande line.
> Being able to provide SMBIOS type 41 entry ensure the name of the
> interface won't change and helps the user guess the right name without
> booting a first time.
> 
> This can be invoked with:
> 
>     $QEMU -netdev user,id=internet
>           -device virtio-net-pci,mac=50:54:00:00:00:42,netdev=internet \
>           -smbios type=41,designation=Onboard LAN,instance=1,kind=ethernet,pci=0000:00:09.0
> 
> Which results in the guest seeing dmidecode data and the interface
> exposed as "eno1":
> 
>     $ dmidecode -t 41
>     # dmidecode 3.3
>     Getting SMBIOS data from sysfs.
>     SMBIOS 2.8 present.Handle 0x2900, DMI type 41, 11 bytes
>     Onboard Device
>             Reference Designation: Onboard LAN
>             Type: Ethernet
>             Status: Enabled
>             Type Instance: 1
>             Bus Address: 0000:00:09.0
>     $ udevadm info -p /sys/class/net/eno1 | grep ONBOARD
>     E: ID_NET_NAME_ONBOARD=eno1
>     E: ID_NET_LABEL_ONBOARD=Onboard LAN
> 
> The original plan was to directly provide a device and populate "kind"
> and "pci" from the device. However, since the SMIBIOS tables are built
> during argument evaluation, the information is not yet available.
> I would welcome some guidance on how to implement this.

I'm not sure I see the problem you're describing here, could
you elaborate ?

I see SMBIOS tables are built by  smbios_get_tables() method.
This is called from qemu_init(), after all arguents have been
processed and devices have been created.

It seems like this should allow SMBIOS tables to be auto-populated
from the NICs listed in -device args previously.


Note, if we're going to auto-populate the SMBIOS type 41 tabes
from -device args, then we'll need to make this behaviour
configurable via a property, so that we can ensure this only
applies to new machine types.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|


Re: [RFC v1] hw/smbios: support for type 41 (onboard devices extended information)
Posted by Vincent Bernat 2 years, 12 months ago
 ❦ 30 mars 2021 11:35 +01, Daniel P. Berrangé:

>> If network devices are present in this table, by default, udev will
>> name the corresponding interfaces enoX, X being the instance number.
>> Without such information, udev will fallback to using the PCI ID and
>> this usually gives ens3 or ens4. This can be a bit annoying as the
>> name of the network card may depend on the order of options and may
>> change if a new PCI device is added earlier on the commande line.
>> Being able to provide SMBIOS type 41 entry ensure the name of the
>> interface won't change and helps the user guess the right name without
>> booting a first time.
>> 
>> This can be invoked with:
>> 
>>     $QEMU -netdev user,id=internet
>>           -device virtio-net-pci,mac=50:54:00:00:00:42,netdev=internet \
>>           -smbios type=41,designation=Onboard LAN,instance=1,kind=ethernet,pci=0000:00:09.0
>> 
>> Which results in the guest seeing dmidecode data and the interface
>> exposed as "eno1":
>> 
>>     $ dmidecode -t 41
>>     # dmidecode 3.3
>>     Getting SMBIOS data from sysfs.
>>     SMBIOS 2.8 present.Handle 0x2900, DMI type 41, 11 bytes
>>     Onboard Device
>>             Reference Designation: Onboard LAN
>>             Type: Ethernet
>>             Status: Enabled
>>             Type Instance: 1
>>             Bus Address: 0000:00:09.0
>>     $ udevadm info -p /sys/class/net/eno1 | grep ONBOARD
>>     E: ID_NET_NAME_ONBOARD=eno1
>>     E: ID_NET_LABEL_ONBOARD=Onboard LAN
>> 
>> The original plan was to directly provide a device and populate "kind"
>> and "pci" from the device. However, since the SMIBIOS tables are built
>> during argument evaluation, the information is not yet available.
>> I would welcome some guidance on how to implement this.
>
> I'm not sure I see the problem you're describing here, could
> you elaborate ?
>
> I see SMBIOS tables are built by  smbios_get_tables() method.
> This is called from qemu_init(), after all arguents have been
> processed and devices have been created.

OK, I was mistaken. I'll try to retrieve the information here then.

> It seems like this should allow SMBIOS tables to be auto-populated
> from the NICs listed in -device args previously.
>
>
> Note, if we're going to auto-populate the SMBIOS type 41 tabes
> from -device args, then we'll need to make this behaviour
> configurable via a property, so that we can ensure this only
> applies to new machine types.

I didn't plan for something automatic, just being able to specify a PCI
device in the -smbios arguments and have the PCI location automatically
filled from that.
-- 
Keep it simple to make it faster.
            - The Elements of Programming Style (Kernighan & Plauger)