[PATCH v2 0/6] try improve native hotplug for pcie root ports

[PATCH v2 0/6] try improve native hotplug for pcie root ports

[Gerd Hoffmann]

v2:
 - rebase to latest master, solve conflicts.
 - drop 'RfC' from subject.

Gerd Hoffmann (6):
  pci: implement power state
  pcie: implement slot power control for pcie root ports
  pcie: add power indicator blink check
  pcie: factor out pcie_cap_slot_unplug()
  pcie: fast unplug when slot power is off
  pcie: expire pending delete

 include/hw/pci/pci.h   |  2 ++
 include/hw/qdev-core.h |  1 +
 hw/pci/pci.c           | 25 +++++++++++--
 hw/pci/pci_host.c      |  6 ++--
 hw/pci/pcie.c          | 79 +++++++++++++++++++++++++++++++++++-------
 softmmu/qdev-monitor.c |  4 ++-
 6 files changed, 100 insertions(+), 17 deletions(-)

-- 
2.33.1

[PATCH v2 1/6] pci: implement power state

[Gerd Hoffmann]

This allows to power off pci devices.  In "off" state the devices will
not be visible.  No pci config space access, no pci bar access, no dma.

Default state is "on", so this patch (alone) should not change behavior.

Use case:  Allows hotplug controllers implement slot power.  Hotplug
controllers doing so should set the inital power state for devices in
the ->plug callback.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 include/hw/pci/pci.h |  2 ++
 hw/pci/pci.c         | 25 +++++++++++++++++++++++--
 hw/pci/pci_host.c    |  6 ++++--
 3 files changed, 29 insertions(+), 4 deletions(-)

diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index 5c4016b9954a..e7cdf2d5ec5d 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -268,6 +268,7 @@ typedef struct PCIReqIDCache PCIReqIDCache;
 struct PCIDevice {
     DeviceState qdev;
     bool partially_hotplugged;
+    bool has_power;
 
     /* PCI config space */
     uint8_t *config;
@@ -908,5 +909,6 @@ extern const VMStateDescription vmstate_pci_device;
 }
 
 MSIMessage pci_get_msi_message(PCIDevice *dev, int vector);
+void pci_set_power(PCIDevice *pci_dev, bool state);
 
 #endif
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 4a84e478cef5..e5993c1ef52b 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -1380,6 +1380,9 @@ static void pci_update_mappings(PCIDevice *d)
             continue;
 
         new_addr = pci_bar_address(d, i, r->type, r->size);
+        if (!d->has_power) {
+            new_addr = PCI_BAR_UNMAPPED;
+        }
 
         /* This bar isn't changed */
         if (new_addr == r->addr)
@@ -1464,8 +1467,8 @@ void pci_default_write_config(PCIDevice *d, uint32_t addr, uint32_t val_in, int
     if (range_covers_byte(addr, l, PCI_COMMAND)) {
         pci_update_irq_disabled(d, was_irq_disabled);
         memory_region_set_enabled(&d->bus_master_enable_region,
-                                  pci_get_word(d->config + PCI_COMMAND)
-                                    & PCI_COMMAND_MASTER);
+                                  (pci_get_word(d->config + PCI_COMMAND)
+                                   & PCI_COMMAND_MASTER) && d->has_power);
     }
 
     msi_write_config(d, addr, val_in, l);
@@ -2182,6 +2185,8 @@ static void pci_qdev_realize(DeviceState *qdev, Error **errp)
         pci_qdev_unrealize(DEVICE(pci_dev));
         return;
     }
+
+    pci_set_power(pci_dev, true);
 }
 
 PCIDevice *pci_new_multifunction(int devfn, bool multifunction,
@@ -2853,6 +2858,22 @@ MSIMessage pci_get_msi_message(PCIDevice *dev, int vector)
     return msg;
 }
 
+void pci_set_power(PCIDevice *d, bool state)
+{
+    if (d->has_power == state) {
+        return;
+    }
+
+    d->has_power = state;
+    pci_update_mappings(d);
+    memory_region_set_enabled(&d->bus_master_enable_region,
+                              (pci_get_word(d->config + PCI_COMMAND)
+                               & PCI_COMMAND_MASTER) && d->has_power);
+    if (!d->has_power) {
+        pci_device_reset(d);
+    }
+}
+
 static const TypeInfo pci_device_type_info = {
     .name = TYPE_PCI_DEVICE,
     .parent = TYPE_DEVICE,
diff --git a/hw/pci/pci_host.c b/hw/pci/pci_host.c
index cf02f0d6a501..7beafd40a8e9 100644
--- a/hw/pci/pci_host.c
+++ b/hw/pci/pci_host.c
@@ -74,7 +74,8 @@ void pci_host_config_write_common(PCIDevice *pci_dev, uint32_t addr,
     /* non-zero functions are only exposed when function 0 is present,
      * allowing direct removal of unexposed functions.
      */
-    if (pci_dev->qdev.hotplugged && !pci_get_function_0(pci_dev)) {
+    if ((pci_dev->qdev.hotplugged && !pci_get_function_0(pci_dev)) ||
+        !pci_dev->has_power) {
         return;
     }
 
@@ -97,7 +98,8 @@ uint32_t pci_host_config_read_common(PCIDevice *pci_dev, uint32_t addr,
     /* non-zero functions are only exposed when function 0 is present,
      * allowing direct removal of unexposed functions.
      */
-    if (pci_dev->qdev.hotplugged && !pci_get_function_0(pci_dev)) {
+    if ((pci_dev->qdev.hotplugged && !pci_get_function_0(pci_dev)) ||
+        !pci_dev->has_power) {
         return ~0x0;
     }
 
-- 
2.33.1

[PATCH v2 2/6] pcie: implement slot power control for pcie root ports

[Gerd Hoffmann]

With this patch hot-plugged pci devices will only be visible to the
guest if the guests hotplug driver has enabled slot power.

This should fix the hot-plug race which one can hit when hot-plugging
a pci device at boot, while the guest is in the middle of the pci bus
scan.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/pci/pcie.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
index 914a9bf3d1f7..13d11a57c76f 100644
--- a/hw/pci/pcie.c
+++ b/hw/pci/pcie.c
@@ -366,6 +366,29 @@ static void hotplug_event_clear(PCIDevice *dev)
     }
 }
 
+static void pcie_set_power_device(PCIBus *bus, PCIDevice *dev, void *opaque)
+{
+    bool *power = opaque;
+
+    pci_set_power(dev, *power);
+}
+
+static void pcie_cap_update_power(PCIDevice *hotplug_dev)
+{
+    uint8_t *exp_cap = hotplug_dev->config + hotplug_dev->exp.exp_cap;
+    PCIBus *sec_bus = pci_bridge_get_sec_bus(PCI_BRIDGE(hotplug_dev));
+    uint32_t sltcap = pci_get_long(exp_cap + PCI_EXP_SLTCAP);
+    uint16_t sltctl = pci_get_word(exp_cap + PCI_EXP_SLTCTL);
+    bool power = true;
+
+    if (sltcap & PCI_EXP_SLTCAP_PCP) {
+        power = (sltctl & PCI_EXP_SLTCTL_PCC) == PCI_EXP_SLTCTL_PWR_ON;
+    }
+
+    pci_for_each_device(sec_bus, pci_bus_num(sec_bus),
+                        pcie_set_power_device, &power);
+}
+
 /*
  * A PCI Express Hot-Plug Event has occurred, so update slot status register
  * and notify OS of the event if necessary.
@@ -434,6 +457,7 @@ void pcie_cap_slot_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
             pci_word_test_and_set_mask(exp_cap + PCI_EXP_LNKSTA,
                                        PCI_EXP_LNKSTA_DLLLA);
         }
+        pcie_cap_update_power(hotplug_pdev);
         return;
     }
 
@@ -451,6 +475,7 @@ void pcie_cap_slot_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
         }
         pcie_cap_slot_event(hotplug_pdev,
                             PCI_EXP_HP_EV_PDC | PCI_EXP_HP_EV_ABP);
+        pcie_cap_update_power(hotplug_pdev);
     }
 }
 
@@ -625,6 +650,7 @@ void pcie_cap_slot_reset(PCIDevice *dev)
                                  PCI_EXP_SLTSTA_PDC |
                                  PCI_EXP_SLTSTA_ABP);
 
+    pcie_cap_update_power(dev);
     hotplug_event_update_event_status(dev);
 }
 
@@ -705,6 +731,7 @@ void pcie_cap_slot_write_config(PCIDevice *dev,
         pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTSTA,
                                        PCI_EXP_SLTSTA_PDC);
     }
+    pcie_cap_update_power(dev);
 
     hotplug_event_notify(dev);
 
@@ -731,6 +758,7 @@ int pcie_cap_slot_post_load(void *opaque, int version_id)
 {
     PCIDevice *dev = opaque;
     hotplug_event_update_event_status(dev);
+    pcie_cap_update_power(dev);
     return 0;
 }
 
-- 
2.33.1

[PATCH v2 3/6] pcie: add power indicator blink check

[Gerd Hoffmann]

Refuse to push the attention button in case the guest is busy with some
hotplug operation (as indicated by the power indicator blinking).

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/pci/pcie.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
index 13d11a57c76f..b92dbff11804 100644
--- a/hw/pci/pcie.c
+++ b/hw/pci/pcie.c
@@ -506,6 +506,7 @@ void pcie_cap_slot_unplug_request_cb(HotplugHandler *hotplug_dev,
     PCIDevice *hotplug_pdev = PCI_DEVICE(hotplug_dev);
     uint8_t *exp_cap = hotplug_pdev->config + hotplug_pdev->exp.exp_cap;
     uint32_t sltcap = pci_get_word(exp_cap + PCI_EXP_SLTCAP);
+    uint16_t sltctl = pci_get_word(exp_cap + PCI_EXP_SLTCTL);
 
     /* Check if hot-unplug is disabled on the slot */
     if ((sltcap & PCI_EXP_SLTCAP_HPC) == 0) {
@@ -521,6 +522,12 @@ void pcie_cap_slot_unplug_request_cb(HotplugHandler *hotplug_dev,
         return;
     }
 
+    if ((sltctl & PCI_EXP_SLTCTL_PIC) == PCI_EXP_SLTCTL_PWR_IND_BLINK) {
+        error_setg(errp, "Hot-unplug failed: "
+                   "guest is busy (power indicator blinking)");
+        return;
+    }
+
     dev->pending_deleted_event = true;
 
     /* In case user cancel the operation of multi-function hot-add,
-- 
2.33.1

[PATCH v2 4/6] pcie: factor out pcie_cap_slot_unplug()

[Gerd Hoffmann]

No functional change.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/pci/pcie.c | 32 ++++++++++++++++++++------------
 1 file changed, 20 insertions(+), 12 deletions(-)

diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
index b92dbff11804..959bf074b205 100644
--- a/hw/pci/pcie.c
+++ b/hw/pci/pcie.c
@@ -497,6 +497,25 @@ static void pcie_unplug_device(PCIBus *bus, PCIDevice *dev, void *opaque)
     object_unparent(OBJECT(dev));
 }
 
+static void pcie_cap_slot_do_unplug(PCIDevice *dev)
+{
+    PCIBus *sec_bus = pci_bridge_get_sec_bus(PCI_BRIDGE(dev));
+    uint8_t *exp_cap = dev->config + dev->exp.exp_cap;
+    uint32_t lnkcap = pci_get_long(exp_cap + PCI_EXP_LNKCAP);
+
+    pci_for_each_device_under_bus(sec_bus, pcie_unplug_device, NULL);
+
+    pci_word_test_and_clear_mask(exp_cap + PCI_EXP_SLTSTA,
+                                 PCI_EXP_SLTSTA_PDS);
+    if (dev->cap_present & QEMU_PCIE_LNKSTA_DLLLA ||
+        (lnkcap & PCI_EXP_LNKCAP_DLLLARC)) {
+        pci_word_test_and_clear_mask(exp_cap + PCI_EXP_LNKSTA,
+                                     PCI_EXP_LNKSTA_DLLLA);
+    }
+    pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTSTA,
+                               PCI_EXP_SLTSTA_PDC);
+}
+
 void pcie_cap_slot_unplug_request_cb(HotplugHandler *hotplug_dev,
                                      DeviceState *dev, Error **errp)
 {
@@ -676,7 +695,6 @@ void pcie_cap_slot_write_config(PCIDevice *dev,
     uint32_t pos = dev->exp.exp_cap;
     uint8_t *exp_cap = dev->config + pos;
     uint16_t sltsta = pci_get_word(exp_cap + PCI_EXP_SLTSTA);
-    uint32_t lnkcap = pci_get_long(exp_cap + PCI_EXP_LNKCAP);
 
     if (ranges_overlap(addr, len, pos + PCI_EXP_SLTSTA, 2)) {
         /*
@@ -726,17 +744,7 @@ void pcie_cap_slot_write_config(PCIDevice *dev,
         (val & PCI_EXP_SLTCTL_PIC_OFF) == PCI_EXP_SLTCTL_PIC_OFF &&
         (!(old_slt_ctl & PCI_EXP_SLTCTL_PCC) ||
         (old_slt_ctl & PCI_EXP_SLTCTL_PIC_OFF) != PCI_EXP_SLTCTL_PIC_OFF)) {
-        PCIBus *sec_bus = pci_bridge_get_sec_bus(PCI_BRIDGE(dev));
-        pci_for_each_device_under_bus(sec_bus, pcie_unplug_device, NULL);
-        pci_word_test_and_clear_mask(exp_cap + PCI_EXP_SLTSTA,
-                                     PCI_EXP_SLTSTA_PDS);
-        if (dev->cap_present & QEMU_PCIE_LNKSTA_DLLLA ||
-            (lnkcap & PCI_EXP_LNKCAP_DLLLARC)) {
-            pci_word_test_and_clear_mask(exp_cap + PCI_EXP_LNKSTA,
-                                         PCI_EXP_LNKSTA_DLLLA);
-        }
-        pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTSTA,
-                                       PCI_EXP_SLTSTA_PDC);
+        pcie_cap_slot_do_unplug(dev);
     }
     pcie_cap_update_power(dev);
 
-- 
2.33.1

[PATCH v2 5/6] pcie: fast unplug when slot power is off

[Gerd Hoffmann]

In case the slot is powered off (and the power indicator turned off too)
we can unplug right away, without round-trip to the guest.

Also clear pending attention button press, there is nothing to care
about any more.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/pci/pcie.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
index 959bf074b205..a930ac738a15 100644
--- a/hw/pci/pcie.c
+++ b/hw/pci/pcie.c
@@ -560,6 +560,16 @@ void pcie_cap_slot_unplug_request_cb(HotplugHandler *hotplug_dev,
         return;
     }
 
+    if (((sltctl & PCI_EXP_SLTCTL_PIC) == PCI_EXP_SLTCTL_PWR_IND_OFF) &&
+        ((sltctl & PCI_EXP_SLTCTL_PCC) == PCI_EXP_SLTCTL_PWR_OFF)) {
+        /* slot is powered off -> unplug without round-trip to the guest */
+        pcie_cap_slot_do_unplug(hotplug_pdev);
+        hotplug_event_notify(hotplug_pdev);
+        pci_word_test_and_clear_mask(exp_cap + PCI_EXP_SLTSTA,
+                                     PCI_EXP_SLTSTA_ABP);
+        return;
+    }
+
     pcie_cap_slot_push_attention_button(hotplug_pdev);
 }
 
-- 
2.33.1

[PATCH v2 6/6] pcie: expire pending delete

[Gerd Hoffmann]

Add an expire time for pending delete, once the time is over allow
pressing the attention button again.

This makes pcie hotplug behave more like acpi hotplug, where one can
try sending an 'device_del' monitor command again in case the guest
didn't respond to the first attempt.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 include/hw/qdev-core.h | 1 +
 hw/pci/pcie.c          | 2 ++
 softmmu/qdev-monitor.c | 4 +++-
 3 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 72622bd33706..20d3066595e4 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -181,6 +181,7 @@ struct DeviceState {
     char *canonical_path;
     bool realized;
     bool pending_deleted_event;
+    int64_t pending_deleted_expires_ms;
     QDict *opts;
     int hotplugged;
     bool allow_unplug_during_migration;
diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
index a930ac738a15..c5ed26633723 100644
--- a/hw/pci/pcie.c
+++ b/hw/pci/pcie.c
@@ -548,6 +548,8 @@ void pcie_cap_slot_unplug_request_cb(HotplugHandler *hotplug_dev,
     }
 
     dev->pending_deleted_event = true;
+    dev->pending_deleted_expires_ms =
+        qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + 5000; /* 5 secs */
 
     /* In case user cancel the operation of multi-function hot-add,
      * remove the function that is unexposed to guest individually,
diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
index b5aaae4b8cbe..cc402f18b933 100644
--- a/softmmu/qdev-monitor.c
+++ b/softmmu/qdev-monitor.c
@@ -937,7 +937,9 @@ void qmp_device_del(const char *id, Error **errp)
 {
     DeviceState *dev = find_device_state(id, errp);
     if (dev != NULL) {
-        if (dev->pending_deleted_event) {
+        if (dev->pending_deleted_event &&
+            (dev->pending_deleted_expires_ms == 0 ||
+             dev->pending_deleted_expires_ms > qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL))) {
             error_setg(errp, "Device %s is already in the "
                              "process of unplug", id);
             return;
-- 
2.33.1

Re: [PATCH v2 1/6] pci: implement power state

[Michael S. Tsirkin]

On Thu, Nov 11, 2021 at 02:08:54PM +0100, Gerd Hoffmann wrote:
> This allows to power off pci devices.  In "off" state the devices will
> not be visible.  No pci config space access, no pci bar access, no dma.
> 
> Default state is "on", so this patch (alone) should not change behavior.
> 
> Use case:  Allows hotplug controllers implement slot power.  Hotplug
> controllers doing so should set the inital power state for devices in
> the ->plug callback.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  include/hw/pci/pci.h |  2 ++
>  hw/pci/pci.c         | 25 +++++++++++++++++++++++--
>  hw/pci/pci_host.c    |  6 ++++--
>  3 files changed, 29 insertions(+), 4 deletions(-)
> 
> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index 5c4016b9954a..e7cdf2d5ec5d 100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -268,6 +268,7 @@ typedef struct PCIReqIDCache PCIReqIDCache;
>  struct PCIDevice {
>      DeviceState qdev;
>      bool partially_hotplugged;
> +    bool has_power;
>  
>      /* PCI config space */
>      uint8_t *config;
> @@ -908,5 +909,6 @@ extern const VMStateDescription vmstate_pci_device;
>  }
>  
>  MSIMessage pci_get_msi_message(PCIDevice *dev, int vector);
> +void pci_set_power(PCIDevice *pci_dev, bool state);
>  
>  #endif
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 4a84e478cef5..e5993c1ef52b 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -1380,6 +1380,9 @@ static void pci_update_mappings(PCIDevice *d)
>              continue;
>  
>          new_addr = pci_bar_address(d, i, r->type, r->size);
> +        if (!d->has_power) {
> +            new_addr = PCI_BAR_UNMAPPED;
> +        }
>  
>          /* This bar isn't changed */
>          if (new_addr == r->addr)

I am a bit confused about why this is necessary.
When power is removed device is reset, does not
this disable device memory automatically?


> @@ -1464,8 +1467,8 @@ void pci_default_write_config(PCIDevice *d, uint32_t addr, uint32_t val_in, int
>      if (range_covers_byte(addr, l, PCI_COMMAND)) {
>          pci_update_irq_disabled(d, was_irq_disabled);
>          memory_region_set_enabled(&d->bus_master_enable_region,
> -                                  pci_get_word(d->config + PCI_COMMAND)
> -                                    & PCI_COMMAND_MASTER);
> +                                  (pci_get_word(d->config + PCI_COMMAND)
> +                                   & PCI_COMMAND_MASTER) && d->has_power);
>      }
>  
>      msi_write_config(d, addr, val_in, l);
> @@ -2182,6 +2185,8 @@ static void pci_qdev_realize(DeviceState *qdev, Error **errp)
>          pci_qdev_unrealize(DEVICE(pci_dev));
>          return;
>      }
> +
> +    pci_set_power(pci_dev, true);
>  }
>  
>  PCIDevice *pci_new_multifunction(int devfn, bool multifunction,
> @@ -2853,6 +2858,22 @@ MSIMessage pci_get_msi_message(PCIDevice *dev, int vector)
>      return msg;
>  }
>  
> +void pci_set_power(PCIDevice *d, bool state)
> +{
> +    if (d->has_power == state) {
> +        return;
> +    }
> +
> +    d->has_power = state;
> +    pci_update_mappings(d);
> +    memory_region_set_enabled(&d->bus_master_enable_region,
> +                              (pci_get_word(d->config + PCI_COMMAND)
> +                               & PCI_COMMAND_MASTER) && d->has_power);
> +    if (!d->has_power) {
> +        pci_device_reset(d);
> +    }
> +}
> +
>  static const TypeInfo pci_device_type_info = {
>      .name = TYPE_PCI_DEVICE,
>      .parent = TYPE_DEVICE,
> diff --git a/hw/pci/pci_host.c b/hw/pci/pci_host.c
> index cf02f0d6a501..7beafd40a8e9 100644
> --- a/hw/pci/pci_host.c
> +++ b/hw/pci/pci_host.c
> @@ -74,7 +74,8 @@ void pci_host_config_write_common(PCIDevice *pci_dev, uint32_t addr,
>      /* non-zero functions are only exposed when function 0 is present,
>       * allowing direct removal of unexposed functions.
>       */
> -    if (pci_dev->qdev.hotplugged && !pci_get_function_0(pci_dev)) {
> +    if ((pci_dev->qdev.hotplugged && !pci_get_function_0(pci_dev)) ||
> +        !pci_dev->has_power) {
>          return;
>      }
>  
> @@ -97,7 +98,8 @@ uint32_t pci_host_config_read_common(PCIDevice *pci_dev, uint32_t addr,
>      /* non-zero functions are only exposed when function 0 is present,
>       * allowing direct removal of unexposed functions.
>       */
> -    if (pci_dev->qdev.hotplugged && !pci_get_function_0(pci_dev)) {
> +    if ((pci_dev->qdev.hotplugged && !pci_get_function_0(pci_dev)) ||
> +        !pci_dev->has_power) {
>          return ~0x0;
>      }
>  
> -- 
> 2.33.1

Re: [PATCH v2 1/6] pci: implement power state

[Michael S. Tsirkin]

On Thu, Nov 11, 2021 at 02:08:54PM +0100, Gerd Hoffmann wrote:
> This allows to power off pci devices.  In "off" state the devices will
> not be visible.  No pci config space access, no pci bar access, no dma.
> 
> Default state is "on", so this patch (alone) should not change behavior.
> 
> Use case:  Allows hotplug controllers implement slot power.  Hotplug
> controllers doing so should set the inital power state for devices in
> the ->plug callback.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  include/hw/pci/pci.h |  2 ++
>  hw/pci/pci.c         | 25 +++++++++++++++++++++++--
>  hw/pci/pci_host.c    |  6 ++++--
>  3 files changed, 29 insertions(+), 4 deletions(-)
> 
> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index 5c4016b9954a..e7cdf2d5ec5d 100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -268,6 +268,7 @@ typedef struct PCIReqIDCache PCIReqIDCache;
>  struct PCIDevice {
>      DeviceState qdev;
>      bool partially_hotplugged;
> +    bool has_power;
>  
>      /* PCI config space */
>      uint8_t *config;
> @@ -908,5 +909,6 @@ extern const VMStateDescription vmstate_pci_device;
>  }
>  
>  MSIMessage pci_get_msi_message(PCIDevice *dev, int vector);
> +void pci_set_power(PCIDevice *pci_dev, bool state);
>  
>  #endif
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 4a84e478cef5..e5993c1ef52b 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -1380,6 +1380,9 @@ static void pci_update_mappings(PCIDevice *d)
>              continue;
>  
>          new_addr = pci_bar_address(d, i, r->type, r->size);
> +        if (!d->has_power) {
> +            new_addr = PCI_BAR_UNMAPPED;
> +        }
>  
>          /* This bar isn't changed */
>          if (new_addr == r->addr)
> @@ -1464,8 +1467,8 @@ void pci_default_write_config(PCIDevice *d, uint32_t addr, uint32_t val_in, int
>      if (range_covers_byte(addr, l, PCI_COMMAND)) {
>          pci_update_irq_disabled(d, was_irq_disabled);
>          memory_region_set_enabled(&d->bus_master_enable_region,
> -                                  pci_get_word(d->config + PCI_COMMAND)
> -                                    & PCI_COMMAND_MASTER);
> +                                  (pci_get_word(d->config + PCI_COMMAND)
> +                                   & PCI_COMMAND_MASTER) && d->has_power);
>      }
>  
>      msi_write_config(d, addr, val_in, l);

Here, too - reset will clear bus master enable. Why do we need to
special-case has_power?


> @@ -2182,6 +2185,8 @@ static void pci_qdev_realize(DeviceState *qdev, Error **errp)
>          pci_qdev_unrealize(DEVICE(pci_dev));
>          return;
>      }
> +
> +    pci_set_power(pci_dev, true);
>  }
>  
>  PCIDevice *pci_new_multifunction(int devfn, bool multifunction,
> @@ -2853,6 +2858,22 @@ MSIMessage pci_get_msi_message(PCIDevice *dev, int vector)
>      return msg;
>  }
>  
> +void pci_set_power(PCIDevice *d, bool state)
> +{
> +    if (d->has_power == state) {
> +        return;
> +    }
> +
> +    d->has_power = state;
> +    pci_update_mappings(d);
> +    memory_region_set_enabled(&d->bus_master_enable_region,
> +                              (pci_get_word(d->config + PCI_COMMAND)
> +                               & PCI_COMMAND_MASTER) && d->has_power);
> +    if (!d->has_power) {
> +        pci_device_reset(d);
> +    }
> +}
> +
>  static const TypeInfo pci_device_type_info = {
>      .name = TYPE_PCI_DEVICE,
>      .parent = TYPE_DEVICE,
> diff --git a/hw/pci/pci_host.c b/hw/pci/pci_host.c
> index cf02f0d6a501..7beafd40a8e9 100644
> --- a/hw/pci/pci_host.c
> +++ b/hw/pci/pci_host.c
> @@ -74,7 +74,8 @@ void pci_host_config_write_common(PCIDevice *pci_dev, uint32_t addr,
>      /* non-zero functions are only exposed when function 0 is present,
>       * allowing direct removal of unexposed functions.
>       */
> -    if (pci_dev->qdev.hotplugged && !pci_get_function_0(pci_dev)) {
> +    if ((pci_dev->qdev.hotplugged && !pci_get_function_0(pci_dev)) ||
> +        !pci_dev->has_power) {
>          return;
>      }
>  
> @@ -97,7 +98,8 @@ uint32_t pci_host_config_read_common(PCIDevice *pci_dev, uint32_t addr,
>      /* non-zero functions are only exposed when function 0 is present,
>       * allowing direct removal of unexposed functions.
>       */
> -    if (pci_dev->qdev.hotplugged && !pci_get_function_0(pci_dev)) {
> +    if ((pci_dev->qdev.hotplugged && !pci_get_function_0(pci_dev)) ||
> +        !pci_dev->has_power) {
>          return ~0x0;
>      }
>  
> -- 
> 2.33.1

Re: [PATCH v2 1/6] pci: implement power state

[Gerd Hoffmann]

  Hi,

> >          new_addr = pci_bar_address(d, i, r->type, r->size);
> > +        if (!d->has_power) {
> > +            new_addr = PCI_BAR_UNMAPPED;
> > +        }
> >  
> >          /* This bar isn't changed */
> >          if (new_addr == r->addr)
> 
> I am a bit confused about why this is necessary.
> When power is removed device is reset, does not
> this disable device memory automatically?

Hmm.   While it clearly doesn't hurt it might not be needed indeed.  The
reset-on-poweroff should make sure both bars and dma are off, and due to
config space access being blocked the guest shouldn't be able to turn
them on while device power is off.

So, yes, in theory we should be able to drop this.  Assuming nothing in
qemu ever touches the bar mappings (host access to config space is not
blocked).

I'll have a look.

take care,
  Gerd