Re: [Qemu-devel] [PATCH 2/4] qdev/qbus: Add hidden device support

[Michael S. Tsirkin]

On Fri, May 17, 2019 at 02:58:18PM +0200, Jens Freimann wrote:
> This adds support for hiding a device to the qbus and qdev APIs.
> qdev_device_add() is modified to check for a standby argument in the
> option string. A DeviceListener callback should_be_hidden() is added. It
> can be used by a standby device to inform qdev that this device should
> not be added now. The standby device handler can store the device
> options to plug the device in at a later point in time.
> 
> Signed-off-by: Jens Freimann <address@hidden>


I really like this approach. I think is has value beyond failover:
e.g. if the PCI bus is powered off then devices on it should
also be invisible. Right now we kind of work around this
but we could switch to this API down the road.

> ---
>  hw/core/qdev.c         | 19 +++++++++++++++++++
>  hw/vfio/pci.c          |  1 +
>  hw/vfio/pci.h          |  1 +
>  include/hw/qdev-core.h |  9 +++++++++
>  qdev-monitor.c         | 41 ++++++++++++++++++++++++++++++++++++++---
>  vl.c                   |  6 ++++--
>  6 files changed, 72 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 98cdaa6bf7..d55fe00ae7 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -211,6 +211,25 @@ void device_listener_unregister(DeviceListener *listener)
>      QTAILQ_REMOVE(&device_listeners, listener, link);
>  }
>  
> +bool qdev_should_hide_device(QemuOpts *opts, Error **errp)
> +{
> +    bool res = false;
> +    bool match_found = false;
> +
> +    DeviceListener *listener;
> +
> +    QTAILQ_FOREACH(listener, &device_listeners, link) {
> +       if (listener->should_be_hidden) {
> +            listener->should_be_hidden(listener, opts, &match_found, &res);
> +        }
> +
> +        if (match_found) {
> +            break;
> +        }
> +    }
> +    return res;
> +}
> +
>  void qdev_set_legacy_instance_id(DeviceState *dev, int alias_id,
>                                   int required_for_version)
>  {
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 8cecb53d5c..835249c61d 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -3215,6 +3215,7 @@ static Property vfio_pci_dev_properties[] = {
>                              display, ON_OFF_AUTO_OFF),
>      DEFINE_PROP_UINT32("xres", VFIOPCIDevice, display_xres, 0),
>      DEFINE_PROP_UINT32("yres", VFIOPCIDevice, display_yres, 0),
> +    DEFINE_PROP_STRING("standby", VFIOPCIDevice, standby),
>      DEFINE_PROP_UINT32("x-intx-mmap-timeout-ms", VFIOPCIDevice,
>                         intx.mmap_timeout, 1100),
>      DEFINE_PROP_BIT("x-vga", VFIOPCIDevice, features,
> diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
> index cfcd1a81b8..1a87f91889 100644
> --- a/hw/vfio/pci.h
> +++ b/hw/vfio/pci.h
> @@ -135,6 +135,7 @@ typedef struct VFIOPCIDevice {
>      PCIHostDeviceAddress host;
>      EventNotifier err_notifier;
>      EventNotifier req_notifier;
> +    char *standby;
>      int (*resetfn)(struct VFIOPCIDevice *);
>      uint32_t vendor_id;
>      uint32_t device_id;
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index 5437395779..d54d3ae62a 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -158,6 +158,13 @@ struct DeviceState {
>  struct DeviceListener {
>      void (*realize)(DeviceListener *listener, DeviceState *dev);
>      void (*unrealize)(DeviceListener *listener, DeviceState *dev);
> +    /*
> +     * This callback is called just upon init of the DeviceState
> +     * and can be used by a standby device for informing qdev if this
> +     * device should be hidden by checking the device opts
> +     */
> +    void (*should_be_hidden)(DeviceListener *listener, QemuOpts *device_opts,
> +            bool *match_found, bool *res);
>      QTAILQ_ENTRY(DeviceListener) link;
>  };
>  
> @@ -454,4 +461,6 @@ static inline bool qbus_is_hotpluggable(BusState *bus)
>  void device_listener_register(DeviceListener *listener);
>  void device_listener_unregister(DeviceListener *listener);
>  
> +bool qdev_should_hide_device(QemuOpts *opts, Error **errp);
> +
>  #endif
> diff --git a/qdev-monitor.c b/qdev-monitor.c
> index 9cce8b93c2..a81226529a 100644
> --- a/qdev-monitor.c
> +++ b/qdev-monitor.c
> @@ -32,8 +32,10 @@
>  #include "qemu/help_option.h"
>  #include "qemu/option.h"
>  #include "qemu/qemu-print.h"
> +#include "qemu/option_int.h"
>  #include "sysemu/block-backend.h"
>  #include "migration/misc.h"
> +#include "migration/migration.h"
>  
>  /*
>   * Aliases were a bad idea from the start.  Let's keep them
> @@ -561,14 +563,45 @@ void qdev_set_id(DeviceState *dev, const char *id)
>      }
>  }
>  
> +static int is_failover_device(void *opaque, const char *name, const char 
> *value,
> +                        Error **errp)
> +{
> +    if (strcmp(name, "standby") == 0) {
> +        QemuOpts *opts = (QemuOpts *)opaque;
> +
> +        if (qdev_should_hide_device(opts, errp) && errp && !*errp) {
> +            return 1;
> +        } else if (errp && *errp) {
> +            return -1;
> +        }
> +    }
> +
> +    return 0;
> +}
> +
> +static bool should_hide_device(QemuOpts *opts, Error **err)
> +{
> +    if (qemu_opt_foreach(opts, is_failover_device, opts, err) == 0) {
> +        return false;
> +    }
> +    return true;
> +}
> +
>  DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
>  {
>      DeviceClass *dc;
>      const char *driver, *path;
> -    DeviceState *dev;
> +    DeviceState *dev = NULL;
>      BusState *bus = NULL;
>      Error *err = NULL;
>  
> +    if (opts && should_hide_device(opts, &err)) {
> +        if (err) {
> +            goto err_del_dev;
> +        }
> +        return NULL;
> +    }
> +
>      driver = qemu_opt_get(opts, "driver");
>      if (!driver) {
>          error_setg(errp, QERR_MISSING_PARAMETER, "driver");
> @@ -640,8 +673,10 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error 
> **errp)
>  
>  err_del_dev:
>      error_propagate(errp, err);
> -    object_unparent(OBJECT(dev));
> -    object_unref(OBJECT(dev));
> +    if (dev) {
> +        object_unparent(OBJECT(dev));
> +        object_unref(OBJECT(dev));
> +    }
>      return NULL;
>  }
>  
> diff --git a/vl.c b/vl.c
> index b6709514c1..4b5b878275 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2355,10 +2355,12 @@ static int device_init_func(void *opaque, QemuOpts 
> *opts, Error **errp)
>      DeviceState *dev;
>  
>      dev = qdev_device_add(opts, errp);
> -    if (!dev) {
> +    if (!dev && *errp) {
> +        error_report_err(*errp);
>          return -1;
> +    } else if (dev) {
> +        object_unref(OBJECT(dev));
>      }
> -    object_unref(OBJECT(dev));
>      return 0;
>  }
>  
> -- 
> 2.21.0