Re: [PATCH v2 2/2] hw/smbios: retrieve PCI address from specified device for Type 41

["Daniel P. Berrangé" <berrange@redhat.com>]

On Thu, Apr 01, 2021 at 10:25:44AM +0200, Vincent Bernat wrote:
> Instead of specifying the PCI address manually, the device can be
> specified by ID:
> 
>     $QEMU -netdev user,id=internet
>           -device virtio-net-pci,mac=50:54:00:00:00:42,netdev=internet,id=internet-dev \
>           -smbios type=41,designation='Onboard LAN',instance=1,kind=ethernet,pcidev=internet-dev
> 
> The PCI segment is assumed to be 0. This should hold true for most
> cases.
> 
>     $ 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
> 
>     $ ip -brief a
>     lo               UNKNOWN        127.0.0.1/8 ::1/128
>     eno1             UP             10.0.2.14/24 fec0::5254:ff:fe00:42/64 fe80::5254:ff:fe00:42/64
> 
> Signed-off-by: Vincent Bernat <vincent@bernat.ch>
> ---
>  hw/smbios/smbios.c | 47 +++++++++++++++++++++-------------------------
>  qemu-options.hx    |  2 +-
>  2 files changed, 22 insertions(+), 27 deletions(-)

It doesn't really make sense to have this as a separate patch
when it is deleting half the code you added in the previous
patch. Just merge them together as one.

> 
> diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
> index 46a08652dff4..0f390e03453c 100644
> --- a/hw/smbios/smbios.c
> +++ b/hw/smbios/smbios.c
> @@ -27,6 +27,7 @@
>  #include "hw/firmware/smbios.h"
>  #include "hw/loader.h"
>  #include "hw/boards.h"
> +#include "hw/pci/pci.h"
>  #include "smbios_build.h"
>  
>  /* legacy structures and constants for <= 2.0 machines */
> @@ -134,12 +135,8 @@ static QEnumLookup type41_kind_lookup = {
>      .size = 10
>  };
>  struct type41_instance {
> -    const char *designation;
> +    const char *designation, *pcidev;
>      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);
> @@ -403,10 +400,9 @@ static const QemuOptDesc qemu_smbios_type41_opts[] = {
>          .type = QEMU_OPT_NUMBER,
>          .help = "device type instance",
>      },{
> -        .name = "pci",
> +        .name = "pcidev",
>          .type = QEMU_OPT_STRING,
>          .help = "PCI device",
> -        .def_value_str = "0:0.0",
>      },
>      { /* end of list */ }
>  };
> @@ -837,9 +833,23 @@ static void smbios_build_type_41_table(void)
>          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;
> +
> +        if (t41->pcidev) {
> +            PCIDevice *pdev = NULL;
> +            int rc = pci_qdev_find_device(t41->pcidev, &pdev);
> +            if (rc == 0) {
> +                /*
> +                 * TODO: Extract the appropriate value. Most of the
> +                 * time, this will be 0.
> +                 */
> +                t->segment_group_number = cpu_to_le16(0);

Hmm, tricky, as it requires interpreting the PCI topology. Wonder if
there's any helper that can do the hard work for you

> +                t->bus_number = pci_dev_bus_num(pdev);
> +                t->device_number = pdev->devfn;
> +            } else {
> +                fprintf(stderr, "%s: cannot find PCI device %s\n",
> +                        __func__, t41->pcidev);

This isn't terminating execution which looks like a bug.

Modify this method to have an 'Error **errp' parameter and
use 'error_setg' to report it.  You'll need to modify the
smbios_get_tables caller to have an 'Error **errp' too.

For the callers of smbios_get_tables(), you can then just pass
in '&error_fatal', to make it print the error + exit.

> +            }
> +        }
>  
>          SMBIOS_BUILD_TABLE_POST;
>          instance++;
> @@ -1301,7 +1311,6 @@ void smbios_entry_add(QemuOpts *opts, Error **errp)
>          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;
> @@ -1324,21 +1333,7 @@ void smbios_entry_add(QemuOpts *opts, Error **errp)
>                  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);
> +            save_opt(&t->pcidev, opts, "pcidev");
>  
>              QTAILQ_INSERT_TAIL(&type41, t, next);
>              return;
> diff --git a/qemu-options.hx b/qemu-options.hx
> index eb2de7c372c7..e6e54f9bd1f3 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -2371,7 +2371,7 @@ DEF("smbios", HAS_ARG, QEMU_OPTION_smbios,
>      "-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"
> -    "-smbios type=41[,designation=str][,kind=str][,instance=%d][,pci=%x:%x:%x.%x]\n"
> +    "-smbios type=41[,designation=str][,kind=str][,instance=%d][,pcidev=str]\n"
>      "                specify SMBIOS type 41 fields\n",
>      QEMU_ARCH_I386 | QEMU_ARCH_ARM)
>  SRST
> -- 
> 2.31.0
> 

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