Re: [PATCH 37/36] machine: introduce MachineInitPhase

[Igor Mammedov]

On Fri, 27 Nov 2020 07:00:21 -0500
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Generalize the qdev_hotplug variable to the different phases of
> machine initialization.  We would like to allow different
> monitor commands depending on the phase.

new machine states need more documentation
+ a couple more comments below

> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>       This has to be the last patch because patch 36 is where
>       hw/core/machine.c takes care of setting qdev_hotplug to true.
> 
>       And this explains the weird [PATCH 00/37] in the previous
>       version of the RFC. :)
> 
>  hw/core/machine-qmp-cmds.c |  6 +++---
>  hw/core/machine.c          |  3 ++-
>  hw/core/qdev.c             | 16 ++++++++++++++--
>  hw/pci/pci.c               |  2 +-
>  hw/usb/core.c              |  2 +-
>  hw/virtio/virtio-iommu.c   |  2 +-
>  include/hw/qdev-core.h     | 12 +++++++++++-
>  monitor/hmp.c              |  2 +-
>  softmmu/qdev-monitor.c     | 24 +++++++++++++-----------
>  softmmu/vl.c               |  4 +++-
>  ui/console.c               |  2 +-
>  11 files changed, 51 insertions(+), 24 deletions(-)
> 
> diff --git a/hw/core/machine-qmp-cmds.c b/hw/core/machine-qmp-cmds.c
> index cb9387c5f5..6cbeb688c4 100644
> --- a/hw/core/machine-qmp-cmds.c
> +++ b/hw/core/machine-qmp-cmds.c
> @@ -286,9 +286,9 @@ HotpluggableCPUList *qmp_query_hotpluggable_cpus(Error 
> **errp)
>  
>  void qmp_set_numa_node(NumaOptions *cmd, Error **errp)
>  {
> -    if (qdev_hotplug) {
> -         error_setg(errp, "The command is permitted only before the machine 
> has been created");
> -         return;
> +    if (phase_check(PHASE_MACHINE_READY)) {
that's what I was afraid of in 26/36,
it should be PHASE_MACHINE_INITIALIZED

> +        error_setg(errp, "The command is permitted only before the machine 
> has been created");
> +        return;
>      }
>  
>      set_numa_options(MACHINE(qdev_get_machine()), cmd, errp);
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 025c4f9749..a71cb23ecf 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -1171,6 +1171,7 @@ void machine_run_board_init(MachineState *machine)
>      }
>  
>      machine_class->init(machine);
> +    phase_advance(PHASE_MACHINE_INITIALIZED);
>  }
>  
>  static NotifierList machine_init_done_notifiers =
> @@ -1204,7 +1205,7 @@ void qdev_machine_creation_done(void)
>       * ok, initial machine setup is done, starting from now we can
>       * only create hotpluggable devices
>       */
> -    qdev_hotplug = true;
> +    phase_advance(PHASE_MACHINE_READY);
>      qdev_assert_realized_properly();
>  
>      /* TODO: once all bus devices are qdevified, this should be done
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index bc5df8ce69..beb35879c6 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -41,7 +41,6 @@
>  #include "migration/vmstate.h"
>  #include "trace.h"
>  
> -bool qdev_hotplug = false;
>  static bool qdev_hot_added = false;
>  bool qdev_hot_removed = false;
>  
> @@ -1023,7 +1022,7 @@ static void device_initfn(Object *obj)
>  {
>      DeviceState *dev = DEVICE(obj);
>  
> -    if (qdev_hotplug) {
> +    if (phase_check(PHASE_MACHINE_READY)) {
>          dev->hotplugged = 1;
>          qdev_hot_added = true;
>      }
> @@ -1267,6 +1266,19 @@ Object *qdev_get_machine(void)
>      return dev;
>  }
>  
> +static MachineInitPhase machine_phase;
> +
> +bool phase_check(MachineInitPhase phase)
> +{
> +    return machine_phase >= phase;
> +}
> +
> +void phase_advance(MachineInitPhase phase)
> +{
> +    assert(machine_phase == phase - 1);
> +    machine_phase = phase;
> +}
> +
>  static const TypeInfo device_type_info = {
>      .name = TYPE_DEVICE,
>      .parent = TYPE_OBJECT,
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 9424231542..d4349ea577 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -1062,7 +1062,7 @@ static PCIDevice *do_pci_register_device(PCIDevice 
> *pci_dev,
>      address_space_init(&pci_dev->bus_master_as,
>                         &pci_dev->bus_master_container_region, pci_dev->name);
>  
> -    if (qdev_hotplug) {
> +    if (phase_check(PHASE_MACHINE_READY)) {
>          pci_init_bus_master(pci_dev);
>      }
>      pci_dev->irq_state = 0;
> diff --git a/hw/usb/core.c b/hw/usb/core.c
> index 5234dcc73f..e960036f4d 100644
> --- a/hw/usb/core.c
> +++ b/hw/usb/core.c
> @@ -97,7 +97,7 @@ void usb_wakeup(USBEndpoint *ep, unsigned int stream)
>      USBDevice *dev = ep->dev;
>      USBBus *bus = usb_bus_from_device(dev);
>  
> -    if (!qdev_hotplug) {
> +    if (!phase_check(PHASE_MACHINE_READY)) {
>          /*
>           * This is machine init cold plug.  No need to wakeup anyone,
>           * all devices will be reset anyway.  And trying to wakeup can
> diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
> index fc5c75d693..8370fd80d7 100644
> --- a/hw/virtio/virtio-iommu.c
> +++ b/hw/virtio/virtio-iommu.c
> @@ -928,7 +928,7 @@ static int 
> virtio_iommu_set_page_size_mask(IOMMUMemoryRegion *mr,
>       * accept it. Having a different masks is possible but the guest will use
>       * sub-optimal block sizes, so warn about it.
>       */
> -    if (qdev_hotplug) {
> +    if (phase_check(PHASE_MACHINE_READY)) {
>          int new_granule = ctz64(new_mask);
>          int cur_granule = ctz64(cur_mask);
>  
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index 6446846752..05e7e7bc45 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -821,7 +821,6 @@ Object *qdev_get_machine(void);
>  /* FIXME: make this a link<> */
>  bool qdev_set_parent_bus(DeviceState *dev, BusState *bus, Error **errp);
>  
> -extern bool qdev_hotplug;
>  extern bool qdev_hot_removed;
>  
>  char *qdev_get_dev_path(DeviceState *dev);
> @@ -847,4 +846,15 @@ void device_listener_unregister(DeviceListener 
> *listener);
>   */
>  bool qdev_should_hide_device(QemuOpts *opts);
>  
> +typedef enum MachineInitPhase {
> +    PHASE_NO_MACHINE,
> +    PHASE_MACHINE_CREATED,
> +    PHASE_ACCEL_CREATED,
> +    PHASE_MACHINE_INITIALIZED,
> +    PHASE_MACHINE_READY,
> +} MachineInitPhase;
Perhaps add doc comments here describing phases?

> +
> +extern bool phase_check(MachineInitPhase phase);
> +extern void phase_advance(MachineInitPhase phase);
> +
>  #endif
> diff --git a/monitor/hmp.c b/monitor/hmp.c
> index f2fe192d69..6c0b33a0b1 100644
> --- a/monitor/hmp.c
> +++ b/monitor/hmp.c
> @@ -216,7 +216,7 @@ static bool cmd_can_preconfig(const HMPCommand *cmd)
>  
>  static bool cmd_available(const HMPCommand *cmd)
>  {
> -    return qdev_hotplug || cmd_can_preconfig(cmd);
> +    return phase_check(PHASE_MACHINE_READY) || cmd_can_preconfig(cmd);
>  }
>  
>  static void help_cmd_dump_one(Monitor *mon,
> diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
> index e967d13bd0..184fe317af 100644
> --- a/softmmu/qdev-monitor.c
> +++ b/softmmu/qdev-monitor.c
> @@ -244,7 +244,7 @@ static DeviceClass *qdev_get_device_class(const char 
> **driver, Error **errp)
>  
>      dc = DEVICE_CLASS(oc);
>      if (!dc->user_creatable ||
> -        (qdev_hotplug && !dc->hotpluggable)) {
> +        (phase_check(PHASE_MACHINE_READY) && !dc->hotpluggable)) {
>          error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "driver",
>                     "pluggable device type");
>          return NULL;
> @@ -637,7 +637,7 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
>      }
>      hide = should_hide_device(opts);
>  
> -    if ((hide || qdev_hotplug) && bus && !qbus_is_hotpluggable(bus)) {
> +    if ((hide || phase_check(PHASE_MACHINE_READY)) && bus && 
> !qbus_is_hotpluggable(bus)) {
>          error_setg(errp, QERR_BUS_NO_HOTPLUG, bus->name);
>          return NULL;
>      }
> @@ -655,15 +655,17 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error 
> **errp)
>      dev = qdev_new(driver);
>  
>      /* Check whether the hotplug is allowed by the machine */
> -    if (qdev_hotplug && !qdev_hotplug_allowed(dev, errp)) {
> -        goto err_del_dev;
> -    }
> +    if (phase_check(PHASE_MACHINE_READY)) {
> +        if (!qdev_hotplug_allowed(dev, errp)) {
> +            goto err_del_dev;
> +        }
>  
> -    if (!bus && qdev_hotplug && !qdev_get_machine_hotplug_handler(dev)) {
> -        /* No bus, no machine hotplug handler --> device is not hotpluggable 
> */
> -        error_setg(errp, "Device '%s' can not be hotplugged on this machine",
> -                   driver);
> -        goto err_del_dev;
> +        if (!bus && !qdev_get_machine_hotplug_handler(dev)) {
> +            /* No bus, no machine hotplug handler --> device is not 
> hotpluggable */
> +            error_setg(errp, "Device '%s' can not be hotplugged on this 
> machine",
> +                       driver);
> +            goto err_del_dev;
> +        }
>      }
>  
>      qdev_set_id(dev, qemu_opts_id(opts));
> @@ -1001,7 +1003,7 @@ int qemu_global_option(const char *str)
>  
>  bool qmp_command_available(const QmpCommand *cmd, Error **errp)
>  {
> -    if (!qdev_hotplug &&
> +    if (!phase_check(PHASE_MACHINE_READY) &&
>          !(cmd->options & QCO_ALLOW_PRECONFIG)) {
>          error_setg(errp, "The command '%s' is permitted only after machine 
> initialization has completed",
>                     cmd->name);
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index 1fde4a17a9..2dd5c2c775 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -2451,7 +2451,7 @@ static void qemu_machine_creation_done(void)
>  
>  void qmp_x_exit_preconfig(Error **errp)
>  {
> -    if (qdev_hotplug) {
> +    if (phase_check(PHASE_MACHINE_READY)) {
>          error_setg(errp, "The command is permitted only before machine 
> initialization");
>          return;
>      }
> @@ -3428,12 +3428,14 @@ void qemu_init(int argc, char **argv, char **envp)
>      qemu_create_early_backends();
>  
>      qemu_apply_machine_options();
> +    phase_advance(PHASE_MACHINE_CREATED);
>  
>      /*
>       * Note: uses machine properties such as kernel-irqchip, must run
>       * after machine_set_property().
>       */
>      configure_accelerators(argv[0]);
> +    phase_advance(PHASE_ACCEL_CREATED);
>  
>      /*
>       * Beware, QOM objects created before this point miss global and
> diff --git a/ui/console.c b/ui/console.c
> index e07d2c380d..3a98135daa 100644
> --- a/ui/console.c
> +++ b/ui/console.c
> @@ -1343,7 +1343,7 @@ static QemuConsole *new_console(DisplayState *ds, 
> console_type_t console_type,
>      if (QTAILQ_EMPTY(&consoles)) {
>          s->index = 0;
>          QTAILQ_INSERT_TAIL(&consoles, s, next);
> -    } else if (console_type != GRAPHIC_CONSOLE || qdev_hotplug) {
> +    } else if (console_type != GRAPHIC_CONSOLE || 
> phase_check(PHASE_MACHINE_READY)) {
>          QemuConsole *last = QTAILQ_LAST(&consoles);
>          s->index = last->index + 1;
>          QTAILQ_INSERT_TAIL(&consoles, s, next);