Re: [Qemu-devel] [PATCH v1 1/7] pcihp: perform check for bus capability in pre_plug handler

[Igor Mammedov <imammedo@redhat.com>]

On Wed, 24 Oct 2018 12:19:24 +0200
David Hildenbrand <david@redhat.com> wrote:

> Perform the check in the pre_plug handler. In addition, we need the
> capability only if the device is actually hotplugged (and not created
> during machine initialization). This is a preparation for coldplugging
> devices via that hotplug handler.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>

Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  hw/acpi/pcihp.c         | 21 +++++++++++++++------
>  hw/acpi/piix4.c         | 16 ++++++++++++++--
>  include/hw/acpi/pcihp.h |  2 ++
>  3 files changed, 31 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> index 80d42e12ff..5e7cef173c 100644
> --- a/hw/acpi/pcihp.c
> +++ b/hw/acpi/pcihp.c
> @@ -217,17 +217,24 @@ void acpi_pcihp_reset(AcpiPciHpState *s)
>      acpi_pcihp_update(s);
>  }
>  
> -void acpi_pcihp_device_plug_cb(HotplugHandler *hotplug_dev, AcpiPciHpState *s,
> -                               DeviceState *dev, Error **errp)
> +void acpi_pcihp_device_pre_plug_cb(HotplugHandler *hotplug_dev,
> +                                   DeviceState *dev, Error **errp)
>  {
> -    PCIDevice *pdev = PCI_DEVICE(dev);
> -    int slot = PCI_SLOT(pdev->devfn);
> -    int bsel = acpi_pcihp_get_bsel(pci_get_bus(pdev));
> -    if (bsel < 0) {
> +    /* Only hotplugged devices need the hotplug capability. */
> +    if (dev->hotplugged &&
> +        acpi_pcihp_get_bsel(pci_get_bus(PCI_DEVICE(dev))) < 0) {
>          error_setg(errp, "Unsupported bus. Bus doesn't have property '"
>                     ACPI_PCIHP_PROP_BSEL "' set");
>          return;
>      }
> +}
> +
> +void acpi_pcihp_device_plug_cb(HotplugHandler *hotplug_dev, AcpiPciHpState *s,
> +                               DeviceState *dev, Error **errp)
> +{
> +    PCIDevice *pdev = PCI_DEVICE(dev);
> +    int slot = PCI_SLOT(pdev->devfn);
> +    int bsel;
>  
>      /* Don't send event when device is enabled during qemu machine creation:
>       * it is present on boot, no hotplug event is necessary. We do send an
> @@ -236,6 +243,8 @@ void acpi_pcihp_device_plug_cb(HotplugHandler *hotplug_dev, AcpiPciHpState *s,
>          return;
>      }
>  
> +    bsel = acpi_pcihp_get_bsel(pci_get_bus(pdev));
> +    g_assert(bsel >= 0);
>      s->acpi_pcihp_pci_status[bsel].up |= (1U << slot);
>      acpi_send_event(DEVICE(hotplug_dev), ACPI_PCI_HOTPLUG_STATUS);
>  }
> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> index e330f24c71..167afe2cea 100644
> --- a/hw/acpi/piix4.c
> +++ b/hw/acpi/piix4.c
> @@ -370,6 +370,18 @@ static void piix4_pm_powerdown_req(Notifier *n, void *opaque)
>      acpi_pm1_evt_power_down(&s->ar);
>  }
>  
> +static void piix4_device_pre_plug_cb(HotplugHandler *hotplug_dev,
> +                                    DeviceState *dev, Error **errp)
> +{
> +    if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
> +        acpi_pcihp_device_pre_plug_cb(hotplug_dev, dev, errp);
> +    } else if (!object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM) &&
> +               !object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
> +        error_setg(errp, "acpi: device plug request for not supported device"
> +                   " type: %s", object_get_typename(OBJECT(dev)));
> +    }
> +}
> +
>  static void piix4_device_plug_cb(HotplugHandler *hotplug_dev,
>                                   DeviceState *dev, Error **errp)
>  {
> @@ -392,8 +404,7 @@ static void piix4_device_plug_cb(HotplugHandler *hotplug_dev,
>              acpi_cpu_plug_cb(hotplug_dev, &s->cpuhp_state, dev, errp);
>          }
>      } else {
> -        error_setg(errp, "acpi: device plug request for not supported device"
> -                   " type: %s", object_get_typename(OBJECT(dev)));
> +        g_assert_not_reached();
>      }
>  }
>  
> @@ -702,6 +713,7 @@ static void piix4_pm_class_init(ObjectClass *klass, void *data)
>       */
>      dc->user_creatable = false;
>      dc->hotpluggable = false;
> +    hc->pre_plug = piix4_device_pre_plug_cb;
>      hc->plug = piix4_device_plug_cb;
>      hc->unplug_request = piix4_device_unplug_request_cb;
>      hc->unplug = piix4_device_unplug_cb;
> diff --git a/include/hw/acpi/pcihp.h b/include/hw/acpi/pcihp.h
> index 8a65f99fc8..ce31625850 100644
> --- a/include/hw/acpi/pcihp.h
> +++ b/include/hw/acpi/pcihp.h
> @@ -56,6 +56,8 @@ typedef struct AcpiPciHpState {
>  void acpi_pcihp_init(Object *owner, AcpiPciHpState *, PCIBus *root,
>                       MemoryRegion *address_space_io, bool bridges_enabled);
>  
> +void acpi_pcihp_device_pre_plug_cb(HotplugHandler *hotplug_dev,
> +                                   DeviceState *dev, Error **errp);
>  void acpi_pcihp_device_plug_cb(HotplugHandler *hotplug_dev, AcpiPciHpState *s,
>                                 DeviceState *dev, Error **errp);
>  void acpi_pcihp_device_unplug_cb(HotplugHandler *hotplug_dev, AcpiPciHpState *s,