[PATCH for-9.2 v15 00/11] hw/pci: SR-IOV related fixes and improvements

[PATCH for-9.2 v15 00/11] hw/pci: SR-IOV related fixes and improvements

[Akihiko Odaki]

Supersedes: <20240714-rombar-v2-0-af1504ef55de@daynix.com>
("[PATCH v2 0/4] hw/pci: Convert rom_bar into OnOffAuto")

I submitted a RFC series[1] to add support for SR-IOV emulation to
virtio-net-pci. During the development of the series, I fixed some
trivial bugs and made improvements that I think are independently
useful. This series extracts those fixes and improvements from the RFC
series.

[1]: https://patchew.org/QEMU/20231210-sriov-v2-0-b959e8a6dfaf@daynix.com/

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
Changes in v15:
- Fixed variable shadowing in patch
  "pcie_sriov: Remove num_vfs from PCIESriovPF"
- Link to v14: https://lore.kernel.org/r/20240813-reuse-v14-0-4c15bc6ee0e6@daynix.com

Changes in v14:
- Dropped patch "pcie_sriov: Ensure VF function number does not
  overflow" as I found the restriction it imposes is unnecessary.
- Link to v13: https://lore.kernel.org/r/20240805-reuse-v13-0-aaeaa4d7dfd2@daynix.com

Changes in v13:
- Added patch "s390x/pci: Check for multifunction after device
  realization". I found SR-IOV devices, which are multifunction devices,
  are not supposed to work at all with s390x on QEMU.
- Link to v12: https://lore.kernel.org/r/20240804-reuse-v12-0-d3930c4111b2@daynix.com

Changes in v12:
- Changed to ignore invalid PCI_SRIOV_NUM_VF writes as done for
  PCI_SRIOV_CTRL_VFE.
- Updated the message for patch "hw/pci: Use -1 as the default value for
  rombar". (Markus Armbruster)
- Link to v11: https://lore.kernel.org/r/20240802-reuse-v11-0-fb83bb8c19fb@daynix.com

Changes in v11:
- Rebased.
- Dropped patch "hw/pci: Convert rom_bar into OnOffAuto".
- Added patch "hw/pci: Use -1 as the default value for rombar".
- Added for-9.2 to give a testing period for possible breakage with
  libvirt/s390x.
- Link to v10: https://lore.kernel.org/r/20240627-reuse-v10-0-7ca0b8ed3d9f@daynix.com

Changes in v10:
- Added patch "hw/ppc/spapr_pci: Do not reject VFs created after a PF".
- Added patch "hw/ppc/spapr_pci: Do not create DT for disabled PCI device".
- Added patch "hw/pci: Convert rom_bar into OnOffAuto".
- Dropped patch "hw/pci: Determine if rombar is explicitly enabled".
- Dropped patch "hw/qdev: Remove opts member".
- Link to v9: https://lore.kernel.org/r/20240315-reuse-v9-0-67aa69af4d53@daynix.com

Changes in v9:
- Rebased.
- Restored '#include "qapi/error.h"' (Michael S. Tsirkin)
- Added patch "pcie_sriov: Ensure VF function number does not overflow"
  to fix abortion with wrong PF addr.
- Link to v8: https://lore.kernel.org/r/20240228-reuse-v8-0-282660281e60@daynix.com

Changes in v8:
- Clarified that "hw/pci: Replace -1 with UINT32_MAX for romsize" is
  not a bug fix. (Markus Armbruster)
- Squashed patch "vfio: Avoid inspecting option QDict for rombar" into
  "hw/pci: Determine if rombar is explicitly enabled".
  (Markus Armbruster)
- Noted the minor semantics change for patch "hw/pci: Determine if
  rombar is explicitly enabled". (Markus Armbruster)
- Link to v7: https://lore.kernel.org/r/20240224-reuse-v7-0-29c14bcb952e@daynix.com

Changes in v7:
- Replaced -1 with UINT32_MAX when expressing uint32_t.
  (Markus Armbruster)
- Added patch "hw/pci: Replace -1 with UINT32_MAX for romsize".
- Link to v6: https://lore.kernel.org/r/20240220-reuse-v6-0-2e42a28b0cf2@daynix.com

Changes in v6:
- Fixed migration.
- Added patch "pcie_sriov: Do not manually unrealize".
- Restored patch "pcie_sriov: Release VFs failed to realize" that was
  missed in v5.
- Link to v5: https://lore.kernel.org/r/20240218-reuse-v5-0-e4fc1c19b5a9@daynix.com

Changes in v5:
- Added patch "hw/pci: Always call pcie_sriov_pf_reset()".
- Added patch "pcie_sriov: Reset SR-IOV extended capability".
- Removed a reference to PCI_SRIOV_CTRL_VFE in hw/nvme.
  (Michael S. Tsirkin)
- Noted the impact on the guest of patch "pcie_sriov: Do not reset
  NumVFs after unregistering VFs". (Michael S. Tsirkin)
- Changed to use pcie_sriov_num_vfs().
- Restored pci_set_power() and changed it to call pci_set_enabled() only
  for PFs with an expalanation. (Michael S. Tsirkin)
- Reordered patches.
- Link to v4: https://lore.kernel.org/r/20240214-reuse-v4-0-89ad093a07f4@daynix.com

Changes in v4:
- Reverted the change to pci_rom_bar_explicitly_enabled().
  (Michael S. Tsirkin)
- Added patch "pcie_sriov: Do not reset NumVFs after unregistering VFs".
- Added patch "hw/nvme: Refer to dev->exp.sriov_pf.num_vfs".
- Link to v3: https://lore.kernel.org/r/20240212-reuse-v3-0-8017b689ce7f@daynix.com

Changes in v3:
- Extracted patch "hw/pci: Use -1 as a default value for rombar" from
  patch "hw/pci: Determine if rombar is explicitly enabled"
  (Philippe Mathieu-Daudé)
- Added an audit result of PCIDevice::rom_bar to the message of patch
  "hw/pci: Use -1 as a default value for rombar"
  (Philippe Mathieu-Daudé)
- Link to v2: https://lore.kernel.org/r/20240210-reuse-v2-0-24ba2a502692@daynix.com

Changes in v2:
- Reset after enabling a function so that NVMe VF state gets updated.
- Link to v1: https://lore.kernel.org/r/20240203-reuse-v1-0-5be8c5ce6338@daynix.com

---
Akihiko Odaki (11):
      hw/pci: Rename has_power to enabled
      hw/ppc/spapr_pci: Do not create DT for disabled PCI device
      hw/ppc/spapr_pci: Do not reject VFs created after a PF
      s390x/pci: Check for multifunction after device realization
      pcie_sriov: Do not manually unrealize
      pcie_sriov: Reuse SR-IOV VF device instances
      pcie_sriov: Release VFs failed to realize
      pcie_sriov: Remove num_vfs from PCIESriovPF
      pcie_sriov: Register VFs after migration
      hw/pci: Use -1 as the default value for rombar
      hw/qdev: Remove opts member

 docs/pcie_sriov.txt         |   8 ++-
 include/hw/pci/pci.h        |   2 +-
 include/hw/pci/pci_device.h |  19 +++++-
 include/hw/pci/pcie_sriov.h |   9 +--
 include/hw/qdev-core.h      |   4 --
 hw/core/qdev.c              |   1 -
 hw/net/igb.c                |  13 +++-
 hw/nvme/ctrl.c              |  24 ++++---
 hw/pci/pci.c                |  23 ++++---
 hw/pci/pci_host.c           |   4 +-
 hw/pci/pcie_sriov.c         | 153 +++++++++++++++++++++++---------------------
 hw/ppc/spapr_pci.c          |   8 ++-
 hw/s390x/s390-pci-bus.c     |  14 ++--
 hw/vfio/pci.c               |   5 +-
 system/qdev-monitor.c       |  12 ++--
 hw/pci/trace-events         |   2 +-
 16 files changed, 175 insertions(+), 126 deletions(-)
---
base-commit: 31669121a01a14732f57c49400bc239cf9fd505f
change-id: 20240129-reuse-faae22b11934

Best regards,
-- 
Akihiko Odaki <akihiko.odaki@daynix.com>

[PATCH for-9.2 v15 01/11] hw/pci: Rename has_power to enabled

[Akihiko Odaki]

The renamed state will not only represent powering state of PFs, but
also represent SR-IOV VF enablement in the future.

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
 include/hw/pci/pci.h        |  7 ++++++-
 include/hw/pci/pci_device.h |  2 +-
 hw/pci/pci.c                | 14 +++++++-------
 hw/pci/pci_host.c           |  4 ++--
 4 files changed, 16 insertions(+), 11 deletions(-)

diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index eb26cac81098..fe04b4fafd04 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -678,6 +678,11 @@ static inline void pci_irq_pulse(PCIDevice *pci_dev)
 }
 
 MSIMessage pci_get_msi_message(PCIDevice *dev, int vector);
-void pci_set_power(PCIDevice *pci_dev, bool state);
+void pci_set_enabled(PCIDevice *pci_dev, bool state);
+
+static inline void pci_set_power(PCIDevice *pci_dev, bool state)
+{
+    pci_set_enabled(pci_dev, state);
+}
 
 #endif
diff --git a/include/hw/pci/pci_device.h b/include/hw/pci/pci_device.h
index 15694f248948..f38fb3111954 100644
--- a/include/hw/pci/pci_device.h
+++ b/include/hw/pci/pci_device.h
@@ -57,7 +57,7 @@ typedef struct PCIReqIDCache PCIReqIDCache;
 struct PCIDevice {
     DeviceState qdev;
     bool partially_hotplugged;
-    bool has_power;
+    bool enabled;
 
     /* PCI config space */
     uint8_t *config;
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index fab86d056721..b532888e8f6c 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -1525,7 +1525,7 @@ static void pci_update_mappings(PCIDevice *d)
             continue;
 
         new_addr = pci_bar_address(d, i, r->type, r->size);
-        if (!d->has_power) {
+        if (!d->enabled) {
             new_addr = PCI_BAR_UNMAPPED;
         }
 
@@ -1613,7 +1613,7 @@ void pci_default_write_config(PCIDevice *d, uint32_t addr, uint32_t val_in, int
         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) && d->has_power);
+                                   & PCI_COMMAND_MASTER) && d->enabled);
     }
 
     msi_write_config(d, addr, val_in, l);
@@ -2884,18 +2884,18 @@ MSIMessage pci_get_msi_message(PCIDevice *dev, int vector)
     return msg;
 }
 
-void pci_set_power(PCIDevice *d, bool state)
+void pci_set_enabled(PCIDevice *d, bool state)
 {
-    if (d->has_power == state) {
+    if (d->enabled == state) {
         return;
     }
 
-    d->has_power = state;
+    d->enabled = 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_COMMAND_MASTER) && d->enabled);
+    if (!d->enabled) {
         pci_device_reset(d);
     }
 }
diff --git a/hw/pci/pci_host.c b/hw/pci/pci_host.c
index dfe6fe618401..0d82727cc9dd 100644
--- a/hw/pci/pci_host.c
+++ b/hw/pci/pci_host.c
@@ -86,7 +86,7 @@ void pci_host_config_write_common(PCIDevice *pci_dev, uint32_t addr,
      * allowing direct removal of unexposed functions.
      */
     if ((pci_dev->qdev.hotplugged && !pci_get_function_0(pci_dev)) ||
-        !pci_dev->has_power || is_pci_dev_ejected(pci_dev)) {
+        !pci_dev->enabled || is_pci_dev_ejected(pci_dev)) {
         return;
     }
 
@@ -111,7 +111,7 @@ uint32_t pci_host_config_read_common(PCIDevice *pci_dev, uint32_t addr,
      * allowing direct removal of unexposed functions.
      */
     if ((pci_dev->qdev.hotplugged && !pci_get_function_0(pci_dev)) ||
-        !pci_dev->has_power || is_pci_dev_ejected(pci_dev)) {
+        !pci_dev->enabled || is_pci_dev_ejected(pci_dev)) {
         return ~0x0;
     }
 

-- 
2.46.0

[PATCH for-9.2 v15 02/11] hw/ppc/spapr_pci: Do not create DT for disabled PCI device

[Akihiko Odaki]

Disabled means it is a disabled SR-IOV VF or it is powered off, and
hidden from the guest.

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
 hw/ppc/spapr_pci.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 7cf9904c3546..f63182a03c41 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1296,6 +1296,10 @@ static void spapr_dt_pci_device_cb(PCIBus *bus, PCIDevice *pdev,
         return;
     }
 
+    if (!pdev->enabled) {
+        return;
+    }
+
     err = spapr_dt_pci_device(p->sphb, pdev, p->fdt, p->offset);
     if (err < 0) {
         p->err = err;

-- 
2.46.0

[PATCH for-9.2 v15 03/11] hw/ppc/spapr_pci: Do not reject VFs created after a PF

[Akihiko Odaki]

A PF may automatically create VFs and the PF may be function 0.

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
 hw/ppc/spapr_pci.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index f63182a03c41..ed4454bbf79e 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1573,7 +1573,9 @@ static void spapr_pci_pre_plug(HotplugHandler *plug_handler,
      * hotplug, we do not allow functions to be hotplugged to a
      * slot that already has function 0 present
      */
-    if (plugged_dev->hotplugged && bus->devices[PCI_DEVFN(slotnr, 0)] &&
+    if (plugged_dev->hotplugged &&
+        !pci_is_vf(pdev) &&
+        bus->devices[PCI_DEVFN(slotnr, 0)] &&
         PCI_FUNC(pdev->devfn) != 0) {
         error_setg(errp, "PCI: slot %d function 0 already occupied by %s,"
                    " additional functions can no longer be exposed to guest.",

-- 
2.46.0

[PATCH for-9.2 v15 04/11] s390x/pci: Check for multifunction after device realization

[Akihiko Odaki]

The SR-IOV PFs set the multifunction bits during device realization so
check them after that. This forbids adding SR-IOV devices to s390x.

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
 hw/s390x/s390-pci-bus.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
index 3e57d5faca18..00b2c1f6157b 100644
--- a/hw/s390x/s390-pci-bus.c
+++ b/hw/s390x/s390-pci-bus.c
@@ -971,14 +971,7 @@ static void s390_pcihost_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
                     "this device");
     }
 
-    if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
-        PCIDevice *pdev = PCI_DEVICE(dev);
-
-        if (pdev->cap_present & QEMU_PCI_CAP_MULTIFUNCTION) {
-            error_setg(errp, "multifunction not supported in s390");
-            return;
-        }
-    } else if (object_dynamic_cast(OBJECT(dev), TYPE_S390_PCI_DEVICE)) {
+    if (object_dynamic_cast(OBJECT(dev), TYPE_S390_PCI_DEVICE)) {
         S390PCIBusDevice *pbdev = S390_PCI_DEVICE(dev);
 
         if (!s390_pci_alloc_idx(s, pbdev)) {
@@ -1069,6 +1062,11 @@ static void s390_pcihost_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
         pdev = PCI_DEVICE(dev);
 
+        if (pdev->cap_present & QEMU_PCI_CAP_MULTIFUNCTION) {
+            error_setg(errp, "multifunction not supported in s390");
+            return;
+        }
+
         if (!dev->id) {
             /* In the case the PCI device does not define an id */
             /* we generate one based on the PCI address         */

-- 
2.46.0

Re: [PATCH for-9.2 v15 04/11] s390x/pci: Check for multifunction after device realization

[Cédric Le Goater]

On 8/23/24 07:00, Akihiko Odaki wrote:
> The SR-IOV PFs set the multifunction bits during device realization so
> check them after that. This forbids adding SR-IOV devices to s390x.
> 
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>

May be add :

Fixes: 6069bcdeacee ("s390x/pci: Move some hotplug checks to the pre_plug handler")

?

Anyhow,

Tested-by: Cédric Le Goater <clg@redhat.com>
Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.


> ---
>   hw/s390x/s390-pci-bus.c | 14 ++++++--------
>   1 file changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
> index 3e57d5faca18..00b2c1f6157b 100644
> --- a/hw/s390x/s390-pci-bus.c
> +++ b/hw/s390x/s390-pci-bus.c
> @@ -971,14 +971,7 @@ static void s390_pcihost_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>                       "this device");
>       }
>   
> -    if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
> -        PCIDevice *pdev = PCI_DEVICE(dev);
> -
> -        if (pdev->cap_present & QEMU_PCI_CAP_MULTIFUNCTION) {
> -            error_setg(errp, "multifunction not supported in s390");
> -            return;
> -        }
> -    } else if (object_dynamic_cast(OBJECT(dev), TYPE_S390_PCI_DEVICE)) {
> +    if (object_dynamic_cast(OBJECT(dev), TYPE_S390_PCI_DEVICE)) {
>           S390PCIBusDevice *pbdev = S390_PCI_DEVICE(dev);
>   
>           if (!s390_pci_alloc_idx(s, pbdev)) {
> @@ -1069,6 +1062,11 @@ static void s390_pcihost_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>       } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
>           pdev = PCI_DEVICE(dev);
>   
> +        if (pdev->cap_present & QEMU_PCI_CAP_MULTIFUNCTION) {
> +            error_setg(errp, "multifunction not supported in s390");
> +            return;
> +        }
> +
>           if (!dev->id) {
>               /* In the case the PCI device does not define an id */
>               /* we generate one based on the PCI address         */
> 

Re: [PATCH for-9.2 v15 04/11] s390x/pci: Check for multifunction after device realization

[Cédric Le Goater]

+Matthew +Eric

Side note for the maintainers :

Before this change, the igb device, which is multifunction, was working
fine under Linux.

Was there a fix in Linux since :

   57da367b9ec4 ("s390x/pci: forbid multifunction pci device")
   6069bcdeacee ("s390x/pci: Move some hotplug checks to the pre_plug handler")

?

s390 PCI devices do not have extended capabilities, so the igb device
does not expose the SRIOV capability and only the PF is accessible but
it doesn't seem to be an issue. (Btw, CONFIG_PCI_IOV is set to y in the
default Linux config which is unexpected)

Thanks,

C.



On 8/23/24 07:00, Akihiko Odaki wrote:
> The SR-IOV PFs set the multifunction bits during device realization so
> check them after that. This forbids adding SR-IOV devices to s390x.
> 
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
>   hw/s390x/s390-pci-bus.c | 14 ++++++--------
>   1 file changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
> index 3e57d5faca18..00b2c1f6157b 100644
> --- a/hw/s390x/s390-pci-bus.c
> +++ b/hw/s390x/s390-pci-bus.c
> @@ -971,14 +971,7 @@ static void s390_pcihost_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>                       "this device");
>       }
>   
> -    if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
> -        PCIDevice *pdev = PCI_DEVICE(dev);
> -
> -        if (pdev->cap_present & QEMU_PCI_CAP_MULTIFUNCTION) {
> -            error_setg(errp, "multifunction not supported in s390");
> -            return;
> -        }
> -    } else if (object_dynamic_cast(OBJECT(dev), TYPE_S390_PCI_DEVICE)) {
> +    if (object_dynamic_cast(OBJECT(dev), TYPE_S390_PCI_DEVICE)) {
>           S390PCIBusDevice *pbdev = S390_PCI_DEVICE(dev);
>   
>           if (!s390_pci_alloc_idx(s, pbdev)) {
> @@ -1069,6 +1062,11 @@ static void s390_pcihost_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>       } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
>           pdev = PCI_DEVICE(dev);
>   
> +        if (pdev->cap_present & QEMU_PCI_CAP_MULTIFUNCTION) {
> +            error_setg(errp, "multifunction not supported in s390");
> +            return;
> +        }
> +
>           if (!dev->id) {
>               /* In the case the PCI device does not define an id */
>               /* we generate one based on the PCI address         */
> 

Re: [PATCH for-9.2 v15 04/11] s390x/pci: Check for multifunction after device realization

[Akihiko Odaki]

On 2024/09/11 18:38, Cédric Le Goater wrote:
> +Matthew +Eric
> 
> Side note for the maintainers :
> 
> Before this change, the igb device, which is multifunction, was working
> fine under Linux.
> 
> Was there a fix in Linux since :
> 
>    57da367b9ec4 ("s390x/pci: forbid multifunction pci device")
>    6069bcdeacee ("s390x/pci: Move some hotplug checks to the pre_plug 
> handler")
> 
> ?
> 
> s390 PCI devices do not have extended capabilities, so the igb device
> does not expose the SRIOV capability and only the PF is accessible but
> it doesn't seem to be an issue. (Btw, CONFIG_PCI_IOV is set to y in the
> default Linux config which is unexpected)

Doesn't s390x really see extended capabilities? hw/s390x/s390-pci-inst.c 
has a call pci_config_size() and pci_host_config_write_common(), which 
means it is exposing the whole PCI Express configuration space. Why 
can't s390x use extended capabilities then?

The best option for fix would be to replace the SR-IOV implementation 
with stub if s390x cannot use the SR-IOV capability. However I still 
need to know at what level I should change the implementation (e.g., is 
it fine to remove the entire capability, or should I keep the capability 
while writes to its registers no-op?)

Regards,
Akihiko Odaki

> 
> Thanks,
> 
> C.
> 
> 
> 
> On 8/23/24 07:00, Akihiko Odaki wrote:
>> The SR-IOV PFs set the multifunction bits during device realization so
>> check them after that. This forbids adding SR-IOV devices to s390x.
>>
>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>> ---
>>   hw/s390x/s390-pci-bus.c | 14 ++++++--------
>>   1 file changed, 6 insertions(+), 8 deletions(-)
>>
>> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
>> index 3e57d5faca18..00b2c1f6157b 100644
>> --- a/hw/s390x/s390-pci-bus.c
>> +++ b/hw/s390x/s390-pci-bus.c
>> @@ -971,14 +971,7 @@ static void s390_pcihost_pre_plug(HotplugHandler 
>> *hotplug_dev, DeviceState *dev,
>>                       "this device");
>>       }
>> -    if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
>> -        PCIDevice *pdev = PCI_DEVICE(dev);
>> -
>> -        if (pdev->cap_present & QEMU_PCI_CAP_MULTIFUNCTION) {
>> -            error_setg(errp, "multifunction not supported in s390");
>> -            return;
>> -        }
>> -    } else if (object_dynamic_cast(OBJECT(dev), TYPE_S390_PCI_DEVICE)) {
>> +    if (object_dynamic_cast(OBJECT(dev), TYPE_S390_PCI_DEVICE)) {
>>           S390PCIBusDevice *pbdev = S390_PCI_DEVICE(dev);
>>           if (!s390_pci_alloc_idx(s, pbdev)) {
>> @@ -1069,6 +1062,11 @@ static void s390_pcihost_plug(HotplugHandler 
>> *hotplug_dev, DeviceState *dev,
>>       } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
>>           pdev = PCI_DEVICE(dev);
>> +        if (pdev->cap_present & QEMU_PCI_CAP_MULTIFUNCTION) {
>> +            error_setg(errp, "multifunction not supported in s390");
>> +            return;
>> +        }
>> +
>>           if (!dev->id) {
>>               /* In the case the PCI device does not define an id */
>>               /* we generate one based on the PCI address         */
>>
> 

Re: [PATCH for-9.2 v15 04/11] s390x/pci: Check for multifunction after device realization

[Michael S. Tsirkin]

On Wed, Sep 11, 2024 at 07:58:15PM +0900, Akihiko Odaki wrote:
> On 2024/09/11 18:38, Cédric Le Goater wrote:
> > +Matthew +Eric
> > 
> > Side note for the maintainers :
> > 
> > Before this change, the igb device, which is multifunction, was working
> > fine under Linux.
> > 
> > Was there a fix in Linux since :
> > 
> >    57da367b9ec4 ("s390x/pci: forbid multifunction pci device")
> >    6069bcdeacee ("s390x/pci: Move some hotplug checks to the pre_plug
> > handler")
> > 
> > ?
> > 
> > s390 PCI devices do not have extended capabilities, so the igb device
> > does not expose the SRIOV capability and only the PF is accessible but
> > it doesn't seem to be an issue. (Btw, CONFIG_PCI_IOV is set to y in the
> > default Linux config which is unexpected)
> 
> Doesn't s390x really see extended capabilities? hw/s390x/s390-pci-inst.c has
> a call pci_config_size() and pci_host_config_write_common(), which means it
> is exposing the whole PCI Express configuration space. Why can't s390x use
> extended capabilities then?
> 
> The best option for fix would be to replace the SR-IOV implementation with
> stub if s390x cannot use the SR-IOV capability. However I still need to know
> at what level I should change the implementation (e.g., is it fine to remove
> the entire capability, or should I keep the capability while writes to its
> registers no-op?)
> 
> Regards,
> Akihiko Odaki

Note changing caps needs compat hacks for cross version migration to work.

> > 
> > Thanks,
> > 
> > C.
> > 
> > 
> > 
> > On 8/23/24 07:00, Akihiko Odaki wrote:
> > > The SR-IOV PFs set the multifunction bits during device realization so
> > > check them after that. This forbids adding SR-IOV devices to s390x.
> > > 
> > > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> > > ---
> > >   hw/s390x/s390-pci-bus.c | 14 ++++++--------
> > >   1 file changed, 6 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
> > > index 3e57d5faca18..00b2c1f6157b 100644
> > > --- a/hw/s390x/s390-pci-bus.c
> > > +++ b/hw/s390x/s390-pci-bus.c
> > > @@ -971,14 +971,7 @@ static void
> > > s390_pcihost_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> > >                       "this device");
> > >       }
> > > -    if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
> > > -        PCIDevice *pdev = PCI_DEVICE(dev);
> > > -
> > > -        if (pdev->cap_present & QEMU_PCI_CAP_MULTIFUNCTION) {
> > > -            error_setg(errp, "multifunction not supported in s390");
> > > -            return;
> > > -        }
> > > -    } else if (object_dynamic_cast(OBJECT(dev), TYPE_S390_PCI_DEVICE)) {
> > > +    if (object_dynamic_cast(OBJECT(dev), TYPE_S390_PCI_DEVICE)) {
> > >           S390PCIBusDevice *pbdev = S390_PCI_DEVICE(dev);
> > >           if (!s390_pci_alloc_idx(s, pbdev)) {
> > > @@ -1069,6 +1062,11 @@ static void s390_pcihost_plug(HotplugHandler
> > > *hotplug_dev, DeviceState *dev,
> > >       } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
> > >           pdev = PCI_DEVICE(dev);
> > > +        if (pdev->cap_present & QEMU_PCI_CAP_MULTIFUNCTION) {
> > > +            error_setg(errp, "multifunction not supported in s390");
> > > +            return;
> > > +        }
> > > +
> > >           if (!dev->id) {
> > >               /* In the case the PCI device does not define an id */
> > >               /* we generate one based on the PCI address         */
> > > 
> > 

Re: [PATCH for-9.2 v15 04/11] s390x/pci: Check for multifunction after device realization

[Matthew Rosato]

On 9/11/24 6:58 AM, Akihiko Odaki wrote:
> On 2024/09/11 18:38, Cédric Le Goater wrote:
>> +Matthew +Eric
>>
>> Side note for the maintainers :
>>
>> Before this change, the igb device, which is multifunction, was working
>> fine under Linux.
>>
>> Was there a fix in Linux since :
>>
>>    57da367b9ec4 ("s390x/pci: forbid multifunction pci device")
>>    6069bcdeacee ("s390x/pci: Move some hotplug checks to the pre_plug handler")
>>
>> ?
The timing of those particular commits predates the linux s390 kernel support of multifunction/SR-IOV.  At that time it was simply not possible on s390.

>>
>> s390 PCI devices do not have extended capabilities, so the igb device
>> does not expose the SRIOV capability and only the PF is accessible but
>> it doesn't seem to be an issue. (Btw, CONFIG_PCI_IOV is set to y in the
>> default Linux config which is unexpected)

The linux config option makes sense because the s390 kernel now supports SR-IOV/multifunction.

> 
> Doesn't s390x really see extended capabilities? hw/s390x/s390-pci-inst.c has a call pci_config_size() and pci_host_config_write_common(), which means it is exposing the whole PCI Express configuration space. Why can't s390x use extended capabilities then?
> 

So, rather than poking around in config space, s390 (and thus the s390 kernel) has an extra layer of 'capabilities' that it generally relies on to determine device functionality called 'CLP'.  Basically, there are pieces of CLP that are not currently generated (or forwarded from the host in the case of passthrough) by QEMU that would be needed by the guest to recognize the SRIOV/multifunction capability of a device, despite what config space has in it.  I suspect this is exactly why only the PF was available to your igb device then (missing CLP info made the device appear to not have multifunction capability as far as the s390 guest is concerned - fwiw adding CLP emulation to enable that is on our todo list).

Sounds like the short-term solution here would be to continue allowing the PF without multifunction being visible to the guest (so as to not regress prior functionality) and then aim for proper support after with the necessary CLP pieces. 

Thanks,
Matt

Re: [PATCH for-9.2 v15 04/11] s390x/pci: Check for multifunction after device realization

[Akihiko Odaki]

On 2024/09/11 22:53, Matthew Rosato wrote:
> On 9/11/24 6:58 AM, Akihiko Odaki wrote:
>> On 2024/09/11 18:38, Cédric Le Goater wrote:
>>> +Matthew +Eric
>>>
>>> Side note for the maintainers :
>>>
>>> Before this change, the igb device, which is multifunction, was working
>>> fine under Linux.
>>>
>>> Was there a fix in Linux since :
>>>
>>>     57da367b9ec4 ("s390x/pci: forbid multifunction pci device")
>>>     6069bcdeacee ("s390x/pci: Move some hotplug checks to the pre_plug handler")
>>>
>>> ?
> The timing of those particular commits predates the linux s390 kernel support of multifunction/SR-IOV.  At that time it was simply not possible on s390.

Is it OK to remove this check of multifunction now?

This code is not working properly with SR-IOV and misleading. It is 
better to remove the code if it does no good.

It would be nice if anyone confirms multifunction works for s390x with 
the code removed.

> 
>>>
>>> s390 PCI devices do not have extended capabilities, so the igb device
>>> does not expose the SRIOV capability and only the PF is accessible but
>>> it doesn't seem to be an issue. (Btw, CONFIG_PCI_IOV is set to y in the
>>> default Linux config which is unexpected)
> 
> The linux config option makes sense because the s390 kernel now supports SR-IOV/multifunction.
> 
>>
>> Doesn't s390x really see extended capabilities? hw/s390x/s390-pci-inst.c has a call pci_config_size() and pci_host_config_write_common(), which means it is exposing the whole PCI Express configuration space. Why can't s390x use extended capabilities then?
>>
> 
> So, rather than poking around in config space, s390 (and thus the s390 kernel) has an extra layer of 'capabilities' that it generally relies on to determine device functionality called 'CLP'.  Basically, there are pieces of CLP that are not currently generated (or forwarded from the host in the case of passthrough) by QEMU that would be needed by the guest to recognize the SRIOV/multifunction capability of a device, despite what config space has in it.  I suspect this is exactly why only the PF was available to your igb device then (missing CLP info made the device appear to not have multifunction capability as far as the s390 guest is concerned - fwiw adding CLP emulation to enable that is on our todo list).

What is expected to happen if you poke the configuration space anyway? I 
also wonder if there is some public documentation of CLP and relevant 
aspect of PCI support in s390x.

> 
> Sounds like the short-term solution here would be to continue allowing the PF without multifunction being visible to the guest (so as to not regress prior functionality) and then aim for proper support after with the necessary CLP pieces.

I agree; we can keep the PF working.

Regards,
Akihiko Odaki

Re: [PATCH for-9.2 v15 04/11] s390x/pci: Check for multifunction after device realization

[Matthew Rosato]

On 9/11/24 11:15 AM, Akihiko Odaki wrote:
> On 2024/09/11 22:53, Matthew Rosato wrote:
>> On 9/11/24 6:58 AM, Akihiko Odaki wrote:
>>> On 2024/09/11 18:38, Cédric Le Goater wrote:
>>>> +Matthew +Eric
>>>>
>>>> Side note for the maintainers :
>>>>
>>>> Before this change, the igb device, which is multifunction, was working
>>>> fine under Linux.
>>>>
>>>> Was there a fix in Linux since :
>>>>
>>>>     57da367b9ec4 ("s390x/pci: forbid multifunction pci device")
>>>>     6069bcdeacee ("s390x/pci: Move some hotplug checks to the pre_plug handler")
>>>>
>>>> ?
>> The timing of those particular commits predates the linux s390 kernel support of multifunction/SR-IOV.  At that time it was simply not possible on s390.
> 
> Is it OK to remove this check of multifunction now?

Yes, I think removing the check (which AFAIU was broken since 6069bcdeacee) will get us back to the behavior Cedric was seeing, where a device that reports multifunction in the config space is still allowed through but only the PF will be usable in the guest.

> 
> This code is not working properly with SR-IOV and misleading. It is better to remove the code if it does no good.
> 
> It would be nice if anyone confirms multifunction works for s390x with the code removed.

Even if you remove the check, multifunction itself won't work in the s390x guest without these missing CLP pieces too.  When I have some time I'll hack something together to fabricate some CLP data and try it out, but it sounds like Cedric could use his setup to right now at least verify that the PF itself should remain usable in the guest (current behavior).

> 
>>
>>>>
>>>> s390 PCI devices do not have extended capabilities, so the igb device
>>>> does not expose the SRIOV capability and only the PF is accessible but
>>>> it doesn't seem to be an issue. (Btw, CONFIG_PCI_IOV is set to y in the
>>>> default Linux config which is unexpected)
>>
>> The linux config option makes sense because the s390 kernel now supports SR-IOV/multifunction.
>>
>>>
>>> Doesn't s390x really see extended capabilities? hw/s390x/s390-pci-inst.c has a call pci_config_size() and pci_host_config_write_common(), which means it is exposing the whole PCI Express configuration space. Why can't s390x use extended capabilities then?
>>>
>>
>> So, rather than poking around in config space, s390 (and thus the s390 kernel) has an extra layer of 'capabilities' that it generally relies on to determine device functionality called 'CLP'.  Basically, there are pieces of CLP that are not currently generated (or forwarded from the host in the case of passthrough) by QEMU that would be needed by the guest to recognize the SRIOV/multifunction capability of a device, despite what config space has in it.  I suspect this is exactly why only the PF was available to your igb device then (missing CLP info made the device appear to not have multifunction capability as far as the s390 guest is concerned - fwiw adding CLP emulation to enable that is on our todo list).
> 
> What is expected to happen if you poke the configuration space anyway? I also wonder if there is some public documentation of CLP and relevant aspect of PCI support in s390x.

On s390, the contents of config space might be what is directly reported by the device or may have been modified by a firmware layer in between the device and the LPAR (logical partition that is the closest thing to bare-metal on s390 where you could run a linux host).  It's not that config space can't be read on s390 or anything like that, rather that the base s390 PCI kernel layer makes its decisions about multifunction based specifically on what is reported via CLP.  If CLP doesn't report that the device is multifunction capable (which it never does for a s390x QEMU guest today) then it is treated as not having multifunction support.

Unfortunately, the CLP details are not publicly documented.

> 
>>
>> Sounds like the short-term solution here would be to continue allowing the PF without multifunction being visible to the guest (so as to not regress prior functionality) and then aim for proper support after with the necessary CLP pieces.
> 
> I agree; we can keep the PF working.

Thanks!

Matt

Re: [PATCH for-9.2 v15 04/11] s390x/pci: Check for multifunction after device realization

[Akihiko Odaki]

On 2024/09/12 6:11, Matthew Rosato wrote:
> On 9/11/24 11:15 AM, Akihiko Odaki wrote:
>> On 2024/09/11 22:53, Matthew Rosato wrote:
>>> On 9/11/24 6:58 AM, Akihiko Odaki wrote:
>>>> On 2024/09/11 18:38, Cédric Le Goater wrote:
>>>>> +Matthew +Eric
>>>>>
>>>>> Side note for the maintainers :
>>>>>
>>>>> Before this change, the igb device, which is multifunction, was working
>>>>> fine under Linux.
>>>>>
>>>>> Was there a fix in Linux since :
>>>>>
>>>>>      57da367b9ec4 ("s390x/pci: forbid multifunction pci device")
>>>>>      6069bcdeacee ("s390x/pci: Move some hotplug checks to the pre_plug handler")
>>>>>
>>>>> ?
>>> The timing of those particular commits predates the linux s390 kernel support of multifunction/SR-IOV.  At that time it was simply not possible on s390.
>>
>> Is it OK to remove this check of multifunction now?
> 
> Yes, I think removing the check (which AFAIU was broken since 6069bcdeacee) will get us back to the behavior Cedric was seeing, where a device that reports multifunction in the config space is still allowed through but only the PF will be usable in the guest.

Commit 6069bcdeacee predates the introduction of SR-IOV (commit 
7c0fa8dff811 ["pcie: Add support for Single Root I/O Virtualization 
(SR/IOV)"]) so it did not break anything, strictly speaking. Ideally, we 
should have taken care of the check when introducing SR-IOV.

> 
>>
>> This code is not working properly with SR-IOV and misleading. It is better to remove the code if it does no good.
>>
>> It would be nice if anyone confirms multifunction works for s390x with the code removed.
> 
> Even if you remove the check, multifunction itself won't work in the s390x guest without these missing CLP pieces too.  When I have some time I'll hack something together to fabricate some CLP data and try it out, but it sounds like Cedric could use his setup to right now at least verify that the PF itself should remain usable in the guest (current behavior).

Well, it seems better to keep the check for non-SR-IOV multifunction 
devices while not enforcing the restriction for SR-IOV.

Multifunction devices without SR-IOV are created with explicit requests 
by specifying multifunction=on for PCI devices. Such requests cannot be 
fulfilled without multifunction CLP so we should reject them.

The situation is different for SR-IOV. SR-IOV is a feature inherent to 
specific type of devices and gets automatically enabled for these 
devices. It may make more sense to ignore just the SR-IOV part of such 
devices and keep the other functionalities working.

The current code implements such a behavior, but it is accidental and 
semantically wrong. I will correct the code to explicitly allow 
multifunction for SR-IOV.

Regards,
Akihiko Odaki

[PATCH for-9.2 v15 05/11] pcie_sriov: Do not manually unrealize

[Akihiko Odaki]

A device gets automatically unrealized when being unparented.

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
 hw/pci/pcie_sriov.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/hw/pci/pcie_sriov.c b/hw/pci/pcie_sriov.c
index e9b23221d713..499becd5273f 100644
--- a/hw/pci/pcie_sriov.c
+++ b/hw/pci/pcie_sriov.c
@@ -204,11 +204,7 @@ static void unregister_vfs(PCIDevice *dev)
     trace_sriov_unregister_vfs(dev->name, PCI_SLOT(dev->devfn),
                                PCI_FUNC(dev->devfn), num_vfs);
     for (i = 0; i < num_vfs; i++) {
-        Error *err = NULL;
         PCIDevice *vf = dev->exp.sriov_pf.vf[i];
-        if (!object_property_set_bool(OBJECT(vf), "realized", false, &err)) {
-            error_reportf_err(err, "Failed to unplug: ");
-        }
         object_unparent(OBJECT(vf));
         object_unref(OBJECT(vf));
     }

-- 
2.46.0

[PATCH for-9.2 v15 06/11] pcie_sriov: Reuse SR-IOV VF device instances

[Akihiko Odaki]

Disable SR-IOV VF devices by reusing code to power down PCI devices
instead of removing them when the guest requests to disable VFs. This
allows to realize devices and report VF realization errors at PF
realization time.

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
 docs/pcie_sriov.txt         |   8 ++--
 include/hw/pci/pci.h        |   5 ---
 include/hw/pci/pci_device.h |  15 +++++++
 include/hw/pci/pcie_sriov.h |   6 +--
 hw/net/igb.c                |  13 ++++--
 hw/nvme/ctrl.c              |  24 +++++++----
 hw/pci/pci.c                |   2 +-
 hw/pci/pcie_sriov.c         | 102 +++++++++++++++++++-------------------------
 8 files changed, 95 insertions(+), 80 deletions(-)

diff --git a/docs/pcie_sriov.txt b/docs/pcie_sriov.txt
index a47aad0bfab0..ab2142807f79 100644
--- a/docs/pcie_sriov.txt
+++ b/docs/pcie_sriov.txt
@@ -52,9 +52,11 @@ setting up a BAR for a VF.
       ...
 
       /* Add and initialize the SR/IOV capability */
-      pcie_sriov_pf_init(d, 0x200, "your_virtual_dev",
-                       vf_devid, initial_vfs, total_vfs,
-                       fun_offset, stride);
+      if (!pcie_sriov_pf_init(d, 0x200, "your_virtual_dev",
+                              vf_devid, initial_vfs, total_vfs,
+                              fun_offset, stride, errp)) {
+         return;
+      }
 
       /* Set up individual VF BARs (parameters as for normal BARs) */
       pcie_sriov_pf_init_vf_bar( ... )
diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index fe04b4fafd04..14a869eeaa71 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -680,9 +680,4 @@ static inline void pci_irq_pulse(PCIDevice *pci_dev)
 MSIMessage pci_get_msi_message(PCIDevice *dev, int vector);
 void pci_set_enabled(PCIDevice *pci_dev, bool state);
 
-static inline void pci_set_power(PCIDevice *pci_dev, bool state)
-{
-    pci_set_enabled(pci_dev, state);
-}
-
 #endif
diff --git a/include/hw/pci/pci_device.h b/include/hw/pci/pci_device.h
index f38fb3111954..1ff3ce94e25b 100644
--- a/include/hw/pci/pci_device.h
+++ b/include/hw/pci/pci_device.h
@@ -212,6 +212,21 @@ static inline uint16_t pci_get_bdf(PCIDevice *dev)
     return PCI_BUILD_BDF(pci_bus_num(pci_get_bus(dev)), dev->devfn);
 }
 
+static inline void pci_set_power(PCIDevice *pci_dev, bool state)
+{
+    /*
+     * Don't change the enabled state of VFs when powering on/off the device.
+     *
+     * When powering on, VFs must not be enabled immediately but they must
+     * wait until the guest configures SR-IOV.
+     * When powering off, their corresponding PFs will be reset and disable
+     * VFs.
+     */
+    if (!pci_is_vf(pci_dev)) {
+        pci_set_enabled(pci_dev, state);
+    }
+}
+
 uint16_t pci_requester_id(PCIDevice *dev);
 
 /* DMA access functions */
diff --git a/include/hw/pci/pcie_sriov.h b/include/hw/pci/pcie_sriov.h
index 450cbef6c201..70649236c18a 100644
--- a/include/hw/pci/pcie_sriov.h
+++ b/include/hw/pci/pcie_sriov.h
@@ -18,7 +18,6 @@
 typedef struct PCIESriovPF {
     uint16_t num_vfs;   /* Number of virtual functions created */
     uint8_t vf_bar_type[PCI_NUM_REGIONS];   /* Store type for each VF bar */
-    const char *vfname; /* Reference to the device type used for the VFs */
     PCIDevice **vf;     /* Pointer to an array of num_vfs VF devices */
 } PCIESriovPF;
 
@@ -27,10 +26,11 @@ typedef struct PCIESriovVF {
     uint16_t vf_number; /* Logical VF number of this function */
 } PCIESriovVF;
 
-void pcie_sriov_pf_init(PCIDevice *dev, uint16_t offset,
+bool pcie_sriov_pf_init(PCIDevice *dev, uint16_t offset,
                         const char *vfname, uint16_t vf_dev_id,
                         uint16_t init_vfs, uint16_t total_vfs,
-                        uint16_t vf_offset, uint16_t vf_stride);
+                        uint16_t vf_offset, uint16_t vf_stride,
+                        Error **errp);
 void pcie_sriov_pf_exit(PCIDevice *dev);
 
 /* Set up a VF bar in the SR/IOV bar area */
diff --git a/hw/net/igb.c b/hw/net/igb.c
index b92bba402e0d..b6ca2f1b8aee 100644
--- a/hw/net/igb.c
+++ b/hw/net/igb.c
@@ -446,9 +446,16 @@ static void igb_pci_realize(PCIDevice *pci_dev, Error **errp)
 
     pcie_ari_init(pci_dev, 0x150);
 
-    pcie_sriov_pf_init(pci_dev, IGB_CAP_SRIOV_OFFSET, TYPE_IGBVF,
-        IGB_82576_VF_DEV_ID, IGB_MAX_VF_FUNCTIONS, IGB_MAX_VF_FUNCTIONS,
-        IGB_VF_OFFSET, IGB_VF_STRIDE);
+    if (!pcie_sriov_pf_init(pci_dev, IGB_CAP_SRIOV_OFFSET,
+                            TYPE_IGBVF, IGB_82576_VF_DEV_ID,
+                            IGB_MAX_VF_FUNCTIONS, IGB_MAX_VF_FUNCTIONS,
+                            IGB_VF_OFFSET, IGB_VF_STRIDE,
+                            errp)) {
+        pcie_cap_exit(pci_dev);
+        igb_cleanup_msix(s);
+        msi_uninit(pci_dev);
+        return;
+    }
 
     pcie_sriov_pf_init_vf_bar(pci_dev, IGBVF_MMIO_BAR_IDX,
         PCI_BASE_ADDRESS_MEM_TYPE_64 | PCI_BASE_ADDRESS_MEM_PREFETCH,
diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index c6d4f61a47f9..e86ea2e7ce57 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -8271,7 +8271,8 @@ out:
     return pow2ceil(bar_size);
 }
 
-static void nvme_init_sriov(NvmeCtrl *n, PCIDevice *pci_dev, uint16_t offset)
+static bool nvme_init_sriov(NvmeCtrl *n, PCIDevice *pci_dev, uint16_t offset,
+                            Error **errp)
 {
     uint16_t vf_dev_id = n->params.use_intel_id ?
                          PCI_DEVICE_ID_INTEL_NVME : PCI_DEVICE_ID_REDHAT_NVME;
@@ -8280,12 +8281,17 @@ static void nvme_init_sriov(NvmeCtrl *n, PCIDevice *pci_dev, uint16_t offset)
                                       le16_to_cpu(cap->vifrsm),
                                       NULL, NULL);
 
-    pcie_sriov_pf_init(pci_dev, offset, "nvme", vf_dev_id,
-                       n->params.sriov_max_vfs, n->params.sriov_max_vfs,
-                       NVME_VF_OFFSET, NVME_VF_STRIDE);
+    if (!pcie_sriov_pf_init(pci_dev, offset, "nvme", vf_dev_id,
+                            n->params.sriov_max_vfs, n->params.sriov_max_vfs,
+                            NVME_VF_OFFSET, NVME_VF_STRIDE,
+                            errp)) {
+        return false;
+    }
 
     pcie_sriov_pf_init_vf_bar(pci_dev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY |
                               PCI_BASE_ADDRESS_MEM_TYPE_64, bar_size);
+
+    return true;
 }
 
 static int nvme_add_pm_capability(PCIDevice *pci_dev, uint8_t offset)
@@ -8410,6 +8416,12 @@ static bool nvme_init_pci(NvmeCtrl *n, PCIDevice *pci_dev, Error **errp)
         return false;
     }
 
+    if (!pci_is_vf(pci_dev) && n->params.sriov_max_vfs &&
+        !nvme_init_sriov(n, pci_dev, 0x120, errp)) {
+        msix_uninit(pci_dev, &n->bar0, &n->bar0);
+        return false;
+    }
+
     nvme_update_msixcap_ts(pci_dev, n->conf_msix_qsize);
 
     pcie_cap_deverr_init(pci_dev);
@@ -8439,10 +8451,6 @@ static bool nvme_init_pci(NvmeCtrl *n, PCIDevice *pci_dev, Error **errp)
         nvme_init_pmr(n, pci_dev);
     }
 
-    if (!pci_is_vf(pci_dev) && n->params.sriov_max_vfs) {
-        nvme_init_sriov(n, pci_dev, 0x120);
-    }
-
     return true;
 }
 
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index b532888e8f6c..5c0050e1786a 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -2895,7 +2895,7 @@ void pci_set_enabled(PCIDevice *d, bool state)
     memory_region_set_enabled(&d->bus_master_enable_region,
                               (pci_get_word(d->config + PCI_COMMAND)
                                & PCI_COMMAND_MASTER) && d->enabled);
-    if (!d->enabled) {
+    if (d->qdev.realized) {
         pci_device_reset(d);
     }
 }
diff --git a/hw/pci/pcie_sriov.c b/hw/pci/pcie_sriov.c
index 499becd5273f..4bffe6c97f66 100644
--- a/hw/pci/pcie_sriov.c
+++ b/hw/pci/pcie_sriov.c
@@ -20,15 +20,25 @@
 #include "qapi/error.h"
 #include "trace.h"
 
-static PCIDevice *register_vf(PCIDevice *pf, int devfn,
-                              const char *name, uint16_t vf_num);
-static void unregister_vfs(PCIDevice *dev);
+static void unparent_vfs(PCIDevice *dev, uint16_t total_vfs)
+{
+    for (uint16_t i = 0; i < total_vfs; i++) {
+        PCIDevice *vf = dev->exp.sriov_pf.vf[i];
+        object_unparent(OBJECT(vf));
+        object_unref(OBJECT(vf));
+    }
+    g_free(dev->exp.sriov_pf.vf);
+    dev->exp.sriov_pf.vf = NULL;
+}
 
-void pcie_sriov_pf_init(PCIDevice *dev, uint16_t offset,
+bool pcie_sriov_pf_init(PCIDevice *dev, uint16_t offset,
                         const char *vfname, uint16_t vf_dev_id,
                         uint16_t init_vfs, uint16_t total_vfs,
-                        uint16_t vf_offset, uint16_t vf_stride)
+                        uint16_t vf_offset, uint16_t vf_stride,
+                        Error **errp)
 {
+    BusState *bus = qdev_get_parent_bus(&dev->qdev);
+    int32_t devfn = dev->devfn + vf_offset;
     uint8_t *cfg = dev->config + offset;
     uint8_t *wmask;
 
@@ -36,7 +46,6 @@ void pcie_sriov_pf_init(PCIDevice *dev, uint16_t offset,
                         offset, PCI_EXT_CAP_SRIOV_SIZEOF);
     dev->exp.sriov_cap = offset;
     dev->exp.sriov_pf.num_vfs = 0;
-    dev->exp.sriov_pf.vfname = g_strdup(vfname);
     dev->exp.sriov_pf.vf = NULL;
 
     pci_set_word(cfg + PCI_SRIOV_VF_OFFSET, vf_offset);
@@ -69,13 +78,35 @@ void pcie_sriov_pf_init(PCIDevice *dev, uint16_t offset,
     pci_set_word(wmask + PCI_SRIOV_SYS_PGSIZE, 0x553);
 
     qdev_prop_set_bit(&dev->qdev, "multifunction", true);
+
+    dev->exp.sriov_pf.vf = g_new(PCIDevice *, total_vfs);
+
+    for (uint16_t i = 0; i < total_vfs; i++) {
+        PCIDevice *vf = pci_new(devfn, vfname);
+        vf->exp.sriov_vf.pf = dev;
+        vf->exp.sriov_vf.vf_number = i;
+
+        if (!qdev_realize(&vf->qdev, bus, errp)) {
+            unparent_vfs(dev, i);
+            return false;
+        }
+
+        /* set vid/did according to sr/iov spec - they are not used */
+        pci_config_set_vendor_id(vf->config, 0xffff);
+        pci_config_set_device_id(vf->config, 0xffff);
+
+        dev->exp.sriov_pf.vf[i] = vf;
+        devfn += vf_stride;
+    }
+
+    return true;
 }
 
 void pcie_sriov_pf_exit(PCIDevice *dev)
 {
-    unregister_vfs(dev);
-    g_free((char *)dev->exp.sriov_pf.vfname);
-    dev->exp.sriov_pf.vfname = NULL;
+    uint8_t *cfg = dev->config + dev->exp.sriov_cap;
+
+    unparent_vfs(dev, pci_get_word(cfg + PCI_SRIOV_TOTAL_VF));
 }
 
 void pcie_sriov_pf_init_vf_bar(PCIDevice *dev, int region_num,
@@ -141,38 +172,11 @@ void pcie_sriov_vf_register_bar(PCIDevice *dev, int region_num,
     }
 }
 
-static PCIDevice *register_vf(PCIDevice *pf, int devfn, const char *name,
-                              uint16_t vf_num)
-{
-    PCIDevice *dev = pci_new(devfn, name);
-    dev->exp.sriov_vf.pf = pf;
-    dev->exp.sriov_vf.vf_number = vf_num;
-    PCIBus *bus = pci_get_bus(pf);
-    Error *local_err = NULL;
-
-    qdev_realize(&dev->qdev, &bus->qbus, &local_err);
-    if (local_err) {
-        error_report_err(local_err);
-        return NULL;
-    }
-
-    /* set vid/did according to sr/iov spec - they are not used */
-    pci_config_set_vendor_id(dev->config, 0xffff);
-    pci_config_set_device_id(dev->config, 0xffff);
-
-    return dev;
-}
-
 static void register_vfs(PCIDevice *dev)
 {
     uint16_t num_vfs;
     uint16_t i;
     uint16_t sriov_cap = dev->exp.sriov_cap;
-    uint16_t vf_offset =
-        pci_get_word(dev->config + sriov_cap + PCI_SRIOV_VF_OFFSET);
-    uint16_t vf_stride =
-        pci_get_word(dev->config + sriov_cap + PCI_SRIOV_VF_STRIDE);
-    int32_t devfn = dev->devfn + vf_offset;
 
     assert(sriov_cap > 0);
     num_vfs = pci_get_word(dev->config + sriov_cap + PCI_SRIOV_NUM_VF);
@@ -180,18 +184,10 @@ static void register_vfs(PCIDevice *dev)
         return;
     }
 
-    dev->exp.sriov_pf.vf = g_new(PCIDevice *, num_vfs);
-
     trace_sriov_register_vfs(dev->name, PCI_SLOT(dev->devfn),
                              PCI_FUNC(dev->devfn), num_vfs);
     for (i = 0; i < num_vfs; i++) {
-        dev->exp.sriov_pf.vf[i] = register_vf(dev, devfn,
-                                              dev->exp.sriov_pf.vfname, i);
-        if (!dev->exp.sriov_pf.vf[i]) {
-            num_vfs = i;
-            break;
-        }
-        devfn += vf_stride;
+        pci_set_enabled(dev->exp.sriov_pf.vf[i], true);
     }
     dev->exp.sriov_pf.num_vfs = num_vfs;
 }
@@ -204,12 +200,8 @@ static void unregister_vfs(PCIDevice *dev)
     trace_sriov_unregister_vfs(dev->name, PCI_SLOT(dev->devfn),
                                PCI_FUNC(dev->devfn), num_vfs);
     for (i = 0; i < num_vfs; i++) {
-        PCIDevice *vf = dev->exp.sriov_pf.vf[i];
-        object_unparent(OBJECT(vf));
-        object_unref(OBJECT(vf));
+        pci_set_enabled(dev->exp.sriov_pf.vf[i], false);
     }
-    g_free(dev->exp.sriov_pf.vf);
-    dev->exp.sriov_pf.vf = NULL;
     dev->exp.sriov_pf.num_vfs = 0;
 }
 
@@ -231,14 +223,10 @@ void pcie_sriov_config_write(PCIDevice *dev, uint32_t address,
                              PCI_FUNC(dev->devfn), off, val, len);
 
     if (range_covers_byte(off, len, PCI_SRIOV_CTRL)) {
-        if (dev->exp.sriov_pf.num_vfs) {
-            if (!(val & PCI_SRIOV_CTRL_VFE)) {
-                unregister_vfs(dev);
-            }
+        if (val & PCI_SRIOV_CTRL_VFE) {
+            register_vfs(dev);
         } else {
-            if (val & PCI_SRIOV_CTRL_VFE) {
-                register_vfs(dev);
-            }
+            unregister_vfs(dev);
         }
     }
 }

-- 
2.46.0

[PATCH for-9.2 v15 07/11] pcie_sriov: Release VFs failed to realize

[Akihiko Odaki]

Release VFs failed to realize just as we do in unregister_vfs().

Fixes: 7c0fa8dff811 ("pcie: Add support for Single Root I/O Virtualization (SR/IOV)")
Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
 hw/pci/pcie_sriov.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/pci/pcie_sriov.c b/hw/pci/pcie_sriov.c
index 4bffe6c97f66..ac8c4013bc88 100644
--- a/hw/pci/pcie_sriov.c
+++ b/hw/pci/pcie_sriov.c
@@ -87,6 +87,8 @@ bool pcie_sriov_pf_init(PCIDevice *dev, uint16_t offset,
         vf->exp.sriov_vf.vf_number = i;
 
         if (!qdev_realize(&vf->qdev, bus, errp)) {
+            object_unparent(OBJECT(vf));
+            object_unref(vf);
             unparent_vfs(dev, i);
             return false;
         }

-- 
2.46.0

[PATCH for-9.2 v15 08/11] pcie_sriov: Remove num_vfs from PCIESriovPF

[Akihiko Odaki]

num_vfs is not migrated so use PCI_SRIOV_CTRL_VFE and PCI_SRIOV_NUM_VF
instead.

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
 include/hw/pci/pcie_sriov.h |  1 -
 hw/pci/pcie_sriov.c         | 38 +++++++++++++++++++++++++++-----------
 hw/pci/trace-events         |  2 +-
 3 files changed, 28 insertions(+), 13 deletions(-)

diff --git a/include/hw/pci/pcie_sriov.h b/include/hw/pci/pcie_sriov.h
index 70649236c18a..5148c5b77dd1 100644
--- a/include/hw/pci/pcie_sriov.h
+++ b/include/hw/pci/pcie_sriov.h
@@ -16,7 +16,6 @@
 #include "hw/pci/pci.h"
 
 typedef struct PCIESriovPF {
-    uint16_t num_vfs;   /* Number of virtual functions created */
     uint8_t vf_bar_type[PCI_NUM_REGIONS];   /* Store type for each VF bar */
     PCIDevice **vf;     /* Pointer to an array of num_vfs VF devices */
 } PCIESriovPF;
diff --git a/hw/pci/pcie_sriov.c b/hw/pci/pcie_sriov.c
index ac8c4013bc88..47028e150eac 100644
--- a/hw/pci/pcie_sriov.c
+++ b/hw/pci/pcie_sriov.c
@@ -45,7 +45,6 @@ bool pcie_sriov_pf_init(PCIDevice *dev, uint16_t offset,
     pcie_add_capability(dev, PCI_EXT_CAP_ID_SRIOV, 1,
                         offset, PCI_EXT_CAP_SRIOV_SIZEOF);
     dev->exp.sriov_cap = offset;
-    dev->exp.sriov_pf.num_vfs = 0;
     dev->exp.sriov_pf.vf = NULL;
 
     pci_set_word(cfg + PCI_SRIOV_VF_OFFSET, vf_offset);
@@ -182,29 +181,28 @@ static void register_vfs(PCIDevice *dev)
 
     assert(sriov_cap > 0);
     num_vfs = pci_get_word(dev->config + sriov_cap + PCI_SRIOV_NUM_VF);
-    if (num_vfs > pci_get_word(dev->config + sriov_cap + PCI_SRIOV_TOTAL_VF)) {
-        return;
-    }
 
     trace_sriov_register_vfs(dev->name, PCI_SLOT(dev->devfn),
                              PCI_FUNC(dev->devfn), num_vfs);
     for (i = 0; i < num_vfs; i++) {
         pci_set_enabled(dev->exp.sriov_pf.vf[i], true);
     }
-    dev->exp.sriov_pf.num_vfs = num_vfs;
+
+    pci_set_word(dev->wmask + sriov_cap + PCI_SRIOV_NUM_VF, 0);
 }
 
 static void unregister_vfs(PCIDevice *dev)
 {
-    uint16_t num_vfs = dev->exp.sriov_pf.num_vfs;
+    uint8_t *cfg = dev->config + dev->exp.sriov_cap;
     uint16_t i;
 
     trace_sriov_unregister_vfs(dev->name, PCI_SLOT(dev->devfn),
-                               PCI_FUNC(dev->devfn), num_vfs);
-    for (i = 0; i < num_vfs; i++) {
+                               PCI_FUNC(dev->devfn));
+    for (i = 0; i < pci_get_word(cfg + PCI_SRIOV_TOTAL_VF); i++) {
         pci_set_enabled(dev->exp.sriov_pf.vf[i], false);
     }
-    dev->exp.sriov_pf.num_vfs = 0;
+
+    pci_set_word(dev->wmask + dev->exp.sriov_cap + PCI_SRIOV_NUM_VF, 0xffff);
 }
 
 void pcie_sriov_config_write(PCIDevice *dev, uint32_t address,
@@ -230,6 +228,17 @@ void pcie_sriov_config_write(PCIDevice *dev, uint32_t address,
         } else {
             unregister_vfs(dev);
         }
+    } else if (range_covers_byte(off, len, PCI_SRIOV_NUM_VF)) {
+        uint8_t *cfg = dev->config + sriov_cap;
+        uint8_t *wmask = dev->wmask + sriov_cap;
+        uint16_t num_vfs = pci_get_word(cfg + PCI_SRIOV_NUM_VF);
+        uint16_t wmask_val = PCI_SRIOV_CTRL_MSE | PCI_SRIOV_CTRL_ARI;
+
+        if (num_vfs <= pci_get_word(cfg + PCI_SRIOV_TOTAL_VF)) {
+            wmask_val |= PCI_SRIOV_CTRL_VFE;
+        }
+
+        pci_set_word(wmask + PCI_SRIOV_CTRL, wmask_val);
     }
 }
 
@@ -246,6 +255,8 @@ void pcie_sriov_pf_reset(PCIDevice *dev)
     unregister_vfs(dev);
 
     pci_set_word(dev->config + sriov_cap + PCI_SRIOV_NUM_VF, 0);
+    pci_set_word(dev->wmask + sriov_cap + PCI_SRIOV_CTRL,
+                 PCI_SRIOV_CTRL_VFE | PCI_SRIOV_CTRL_MSE | PCI_SRIOV_CTRL_ARI);
 
     /*
      * Default is to use 4K pages, software can modify it
@@ -292,7 +303,7 @@ PCIDevice *pcie_sriov_get_pf(PCIDevice *dev)
 PCIDevice *pcie_sriov_get_vf_at_index(PCIDevice *dev, int n)
 {
     assert(!pci_is_vf(dev));
-    if (n < dev->exp.sriov_pf.num_vfs) {
+    if (n < pcie_sriov_num_vfs(dev)) {
         return dev->exp.sriov_pf.vf[n];
     }
     return NULL;
@@ -300,5 +311,10 @@ PCIDevice *pcie_sriov_get_vf_at_index(PCIDevice *dev, int n)
 
 uint16_t pcie_sriov_num_vfs(PCIDevice *dev)
 {
-    return dev->exp.sriov_pf.num_vfs;
+    uint16_t sriov_cap = dev->exp.sriov_cap;
+    uint8_t *cfg = dev->config + sriov_cap;
+
+    return sriov_cap &&
+           (pci_get_word(cfg + PCI_SRIOV_CTRL) & PCI_SRIOV_CTRL_VFE) ?
+           pci_get_word(cfg + PCI_SRIOV_NUM_VF) : 0;
 }
diff --git a/hw/pci/trace-events b/hw/pci/trace-events
index 19643aa8c6b0..e98f575a9d19 100644
--- a/hw/pci/trace-events
+++ b/hw/pci/trace-events
@@ -14,7 +14,7 @@ msix_write_config(char *name, bool enabled, bool masked) "dev %s enabled %d mask
 
 # hw/pci/pcie_sriov.c
 sriov_register_vfs(const char *name, int slot, int function, int num_vfs) "%s %02x:%x: creating %d vf devs"
-sriov_unregister_vfs(const char *name, int slot, int function, int num_vfs) "%s %02x:%x: Unregistering %d vf devs"
+sriov_unregister_vfs(const char *name, int slot, int function) "%s %02x:%x: Unregistering vf devs"
 sriov_config_write(const char *name, int slot, int fun, uint32_t offset, uint32_t val, uint32_t len) "%s %02x:%x: sriov offset 0x%x val 0x%x len %d"
 
 # pcie.c

-- 
2.46.0

[PATCH for-9.2 v15 09/11] pcie_sriov: Register VFs after migration

[Akihiko Odaki]

pcie_sriov doesn't have code to restore its state after migration, but
igb, which uses pcie_sriov, naively claimed its migration capability.

Add code to register VFs after migration and fix igb migration.

Fixes: 3a977deebe6b ("Intrdocue igb device emulation")
Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
 include/hw/pci/pcie_sriov.h | 2 ++
 hw/pci/pci.c                | 7 +++++++
 hw/pci/pcie_sriov.c         | 7 +++++++
 3 files changed, 16 insertions(+)

diff --git a/include/hw/pci/pcie_sriov.h b/include/hw/pci/pcie_sriov.h
index 5148c5b77dd1..c5d2d318d330 100644
--- a/include/hw/pci/pcie_sriov.h
+++ b/include/hw/pci/pcie_sriov.h
@@ -57,6 +57,8 @@ void pcie_sriov_pf_add_sup_pgsize(PCIDevice *dev, uint16_t opt_sup_pgsize);
 void pcie_sriov_config_write(PCIDevice *dev, uint32_t address,
                              uint32_t val, int len);
 
+void pcie_sriov_pf_post_load(PCIDevice *dev);
+
 /* Reset SR/IOV */
 void pcie_sriov_pf_reset(PCIDevice *dev);
 
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 5c0050e1786a..4c7be5295110 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -733,10 +733,17 @@ static bool migrate_is_not_pcie(void *opaque, int version_id)
     return !pci_is_express((PCIDevice *)opaque);
 }
 
+static int pci_post_load(void *opaque, int version_id)
+{
+    pcie_sriov_pf_post_load(opaque);
+    return 0;
+}
+
 const VMStateDescription vmstate_pci_device = {
     .name = "PCIDevice",
     .version_id = 2,
     .minimum_version_id = 1,
+    .post_load = pci_post_load,
     .fields = (const VMStateField[]) {
         VMSTATE_INT32_POSITIVE_LE(version_id, PCIDevice),
         VMSTATE_BUFFER_UNSAFE_INFO_TEST(config, PCIDevice,
diff --git a/hw/pci/pcie_sriov.c b/hw/pci/pcie_sriov.c
index 47028e150eac..a1cb1214af27 100644
--- a/hw/pci/pcie_sriov.c
+++ b/hw/pci/pcie_sriov.c
@@ -242,6 +242,13 @@ void pcie_sriov_config_write(PCIDevice *dev, uint32_t address,
     }
 }
 
+void pcie_sriov_pf_post_load(PCIDevice *dev)
+{
+    if (dev->exp.sriov_cap) {
+        register_vfs(dev);
+    }
+}
+
 
 /* Reset SR/IOV */
 void pcie_sriov_pf_reset(PCIDevice *dev)

-- 
2.46.0

[PATCH for-9.2 v15 10/11] hw/pci: Use -1 as the default value for rombar

[Akihiko Odaki]

vfio_pci_size_rom() distinguishes whether rombar is explicitly set to 1
by checking dev->opts, bypassing the QOM property infrastructure.

Use -1 as the default value for rombar to tell if the user explicitly
set it to 1. The property is also converted from unsigned to signed.
-1 is signed so it is safe to give it a new meaning. The values in
[2 ^ 31, 2 ^ 32) become invalid, but nobody should have typed these
values by chance.

Suggested-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---
 include/hw/pci/pci_device.h | 2 +-
 hw/pci/pci.c                | 2 +-
 hw/vfio/pci.c               | 5 ++---
 3 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/include/hw/pci/pci_device.h b/include/hw/pci/pci_device.h
index 1ff3ce94e25b..8fa845beee5e 100644
--- a/include/hw/pci/pci_device.h
+++ b/include/hw/pci/pci_device.h
@@ -148,7 +148,7 @@ struct PCIDevice {
     uint32_t romsize;
     bool has_rom;
     MemoryRegion rom;
-    uint32_t rom_bar;
+    int32_t rom_bar;
 
     /* INTx routing notifier */
     PCIINTxRoutingNotifier intx_routing_notifier;
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 4c7be5295110..d2eaf0c51dde 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -71,7 +71,7 @@ static Property pci_props[] = {
     DEFINE_PROP_PCI_DEVFN("addr", PCIDevice, devfn, -1),
     DEFINE_PROP_STRING("romfile", PCIDevice, romfile),
     DEFINE_PROP_UINT32("romsize", PCIDevice, romsize, UINT32_MAX),
-    DEFINE_PROP_UINT32("rombar",  PCIDevice, rom_bar, 1),
+    DEFINE_PROP_INT32("rombar",  PCIDevice, rom_bar, -1),
     DEFINE_PROP_BIT("multifunction", PCIDevice, cap_present,
                     QEMU_PCI_CAP_MULTIFUNCTION_BITNR, false),
     DEFINE_PROP_BIT("x-pcie-lnksta-dllla", PCIDevice, cap_present,
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 2407720c3530..dc53837eac73 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -1012,7 +1012,6 @@ static void vfio_pci_size_rom(VFIOPCIDevice *vdev)
 {
     uint32_t orig, size = cpu_to_le32((uint32_t)PCI_ROM_ADDRESS_MASK);
     off_t offset = vdev->config_offset + PCI_ROM_ADDRESS;
-    DeviceState *dev = DEVICE(vdev);
     char *name;
     int fd = vdev->vbasedev.fd;
 
@@ -1046,12 +1045,12 @@ static void vfio_pci_size_rom(VFIOPCIDevice *vdev)
     }
 
     if (vfio_opt_rom_in_denylist(vdev)) {
-        if (dev->opts && qdict_haskey(dev->opts, "rombar")) {
+        if (vdev->pdev.rom_bar > 0) {
             warn_report("Device at %s is known to cause system instability"
                         " issues during option rom execution",
                         vdev->vbasedev.name);
             error_printf("Proceeding anyway since user specified"
-                         " non zero value for rombar\n");
+                         " positive value for rombar\n");
         } else {
             warn_report("Rom loading for device at %s has been disabled"
                         " due to system instability issues",

-- 
2.46.0

[PATCH for-9.2 v15 11/11] hw/qdev: Remove opts member

[Akihiko Odaki]

It is no longer used.

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---
 include/hw/qdev-core.h |  4 ----
 hw/core/qdev.c         |  1 -
 system/qdev-monitor.c  | 12 +++++++-----
 3 files changed, 7 insertions(+), 10 deletions(-)

diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 77bfcbdf732a..a3757e6769f8 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -237,10 +237,6 @@ struct DeviceState {
      * @pending_deleted_expires_ms: optional timeout for deletion events
      */
     int64_t pending_deleted_expires_ms;
-    /**
-     * @opts: QDict of options for the device
-     */
-    QDict *opts;
     /**
      * @hotplugged: was device added after PHASE_MACHINE_READY?
      */
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index f3a996f57dee..2fc84699d432 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -706,7 +706,6 @@ static void device_finalize(Object *obj)
         dev->canonical_path = NULL;
     }
 
-    qobject_unref(dev->opts);
     g_free(dev->id);
 }
 
diff --git a/system/qdev-monitor.c b/system/qdev-monitor.c
index 6af6ef7d667f..3551989d5153 100644
--- a/system/qdev-monitor.c
+++ b/system/qdev-monitor.c
@@ -624,6 +624,7 @@ DeviceState *qdev_device_add_from_qdict(const QDict *opts,
     char *id;
     DeviceState *dev = NULL;
     BusState *bus = NULL;
+    QDict *properties;
 
     driver = qdict_get_try_str(opts, "driver");
     if (!driver) {
@@ -705,13 +706,14 @@ DeviceState *qdev_device_add_from_qdict(const QDict *opts,
     }
 
     /* set properties */
-    dev->opts = qdict_clone_shallow(opts);
-    qdict_del(dev->opts, "driver");
-    qdict_del(dev->opts, "bus");
-    qdict_del(dev->opts, "id");
+    properties = qdict_clone_shallow(opts);
+    qdict_del(properties, "driver");
+    qdict_del(properties, "bus");
+    qdict_del(properties, "id");
 
-    object_set_properties_from_keyval(&dev->parent_obj, dev->opts, from_json,
+    object_set_properties_from_keyval(&dev->parent_obj, properties, from_json,
                                       errp);
+    qobject_unref(properties);
     if (*errp) {
         goto err_del_dev;
     }

-- 
2.46.0

Re: [PATCH for-9.2 v15 00/11] hw/pci: SR-IOV related fixes and improvements

[Michael S. Tsirkin]

On Fri, Aug 23, 2024 at 02:00:37PM +0900, Akihiko Odaki wrote:
> Supersedes: <20240714-rombar-v2-0-af1504ef55de@daynix.com>
> ("[PATCH v2 0/4] hw/pci: Convert rom_bar into OnOffAuto")
> 
> I submitted a RFC series[1] to add support for SR-IOV emulation to
> virtio-net-pci. During the development of the series, I fixed some
> trivial bugs and made improvements that I think are independently
> useful. This series extracts those fixes and improvements from the RFC
> series.
> 
> [1]: https://patchew.org/QEMU/20231210-sriov-v2-0-b959e8a6dfaf@daynix.com/
> 
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>

I don't think Cédric's issues have been addressed, am I wrong?
Cédric, what is your take?



> ---
> Changes in v15:
> - Fixed variable shadowing in patch
>   "pcie_sriov: Remove num_vfs from PCIESriovPF"
> - Link to v14: https://lore.kernel.org/r/20240813-reuse-v14-0-4c15bc6ee0e6@daynix.com
> 
> Changes in v14:
> - Dropped patch "pcie_sriov: Ensure VF function number does not
>   overflow" as I found the restriction it imposes is unnecessary.
> - Link to v13: https://lore.kernel.org/r/20240805-reuse-v13-0-aaeaa4d7dfd2@daynix.com
> 
> Changes in v13:
> - Added patch "s390x/pci: Check for multifunction after device
>   realization". I found SR-IOV devices, which are multifunction devices,
>   are not supposed to work at all with s390x on QEMU.
> - Link to v12: https://lore.kernel.org/r/20240804-reuse-v12-0-d3930c4111b2@daynix.com
> 
> Changes in v12:
> - Changed to ignore invalid PCI_SRIOV_NUM_VF writes as done for
>   PCI_SRIOV_CTRL_VFE.
> - Updated the message for patch "hw/pci: Use -1 as the default value for
>   rombar". (Markus Armbruster)
> - Link to v11: https://lore.kernel.org/r/20240802-reuse-v11-0-fb83bb8c19fb@daynix.com
> 
> Changes in v11:
> - Rebased.
> - Dropped patch "hw/pci: Convert rom_bar into OnOffAuto".
> - Added patch "hw/pci: Use -1 as the default value for rombar".
> - Added for-9.2 to give a testing period for possible breakage with
>   libvirt/s390x.
> - Link to v10: https://lore.kernel.org/r/20240627-reuse-v10-0-7ca0b8ed3d9f@daynix.com
> 
> Changes in v10:
> - Added patch "hw/ppc/spapr_pci: Do not reject VFs created after a PF".
> - Added patch "hw/ppc/spapr_pci: Do not create DT for disabled PCI device".
> - Added patch "hw/pci: Convert rom_bar into OnOffAuto".
> - Dropped patch "hw/pci: Determine if rombar is explicitly enabled".
> - Dropped patch "hw/qdev: Remove opts member".
> - Link to v9: https://lore.kernel.org/r/20240315-reuse-v9-0-67aa69af4d53@daynix.com
> 
> Changes in v9:
> - Rebased.
> - Restored '#include "qapi/error.h"' (Michael S. Tsirkin)
> - Added patch "pcie_sriov: Ensure VF function number does not overflow"
>   to fix abortion with wrong PF addr.
> - Link to v8: https://lore.kernel.org/r/20240228-reuse-v8-0-282660281e60@daynix.com
> 
> Changes in v8:
> - Clarified that "hw/pci: Replace -1 with UINT32_MAX for romsize" is
>   not a bug fix. (Markus Armbruster)
> - Squashed patch "vfio: Avoid inspecting option QDict for rombar" into
>   "hw/pci: Determine if rombar is explicitly enabled".
>   (Markus Armbruster)
> - Noted the minor semantics change for patch "hw/pci: Determine if
>   rombar is explicitly enabled". (Markus Armbruster)
> - Link to v7: https://lore.kernel.org/r/20240224-reuse-v7-0-29c14bcb952e@daynix.com
> 
> Changes in v7:
> - Replaced -1 with UINT32_MAX when expressing uint32_t.
>   (Markus Armbruster)
> - Added patch "hw/pci: Replace -1 with UINT32_MAX for romsize".
> - Link to v6: https://lore.kernel.org/r/20240220-reuse-v6-0-2e42a28b0cf2@daynix.com
> 
> Changes in v6:
> - Fixed migration.
> - Added patch "pcie_sriov: Do not manually unrealize".
> - Restored patch "pcie_sriov: Release VFs failed to realize" that was
>   missed in v5.
> - Link to v5: https://lore.kernel.org/r/20240218-reuse-v5-0-e4fc1c19b5a9@daynix.com
> 
> Changes in v5:
> - Added patch "hw/pci: Always call pcie_sriov_pf_reset()".
> - Added patch "pcie_sriov: Reset SR-IOV extended capability".
> - Removed a reference to PCI_SRIOV_CTRL_VFE in hw/nvme.
>   (Michael S. Tsirkin)
> - Noted the impact on the guest of patch "pcie_sriov: Do not reset
>   NumVFs after unregistering VFs". (Michael S. Tsirkin)
> - Changed to use pcie_sriov_num_vfs().
> - Restored pci_set_power() and changed it to call pci_set_enabled() only
>   for PFs with an expalanation. (Michael S. Tsirkin)
> - Reordered patches.
> - Link to v4: https://lore.kernel.org/r/20240214-reuse-v4-0-89ad093a07f4@daynix.com
> 
> Changes in v4:
> - Reverted the change to pci_rom_bar_explicitly_enabled().
>   (Michael S. Tsirkin)
> - Added patch "pcie_sriov: Do not reset NumVFs after unregistering VFs".
> - Added patch "hw/nvme: Refer to dev->exp.sriov_pf.num_vfs".
> - Link to v3: https://lore.kernel.org/r/20240212-reuse-v3-0-8017b689ce7f@daynix.com
> 
> Changes in v3:
> - Extracted patch "hw/pci: Use -1 as a default value for rombar" from
>   patch "hw/pci: Determine if rombar is explicitly enabled"
>   (Philippe Mathieu-Daudé)
> - Added an audit result of PCIDevice::rom_bar to the message of patch
>   "hw/pci: Use -1 as a default value for rombar"
>   (Philippe Mathieu-Daudé)
> - Link to v2: https://lore.kernel.org/r/20240210-reuse-v2-0-24ba2a502692@daynix.com
> 
> Changes in v2:
> - Reset after enabling a function so that NVMe VF state gets updated.
> - Link to v1: https://lore.kernel.org/r/20240203-reuse-v1-0-5be8c5ce6338@daynix.com
> 
> ---
> Akihiko Odaki (11):
>       hw/pci: Rename has_power to enabled
>       hw/ppc/spapr_pci: Do not create DT for disabled PCI device
>       hw/ppc/spapr_pci: Do not reject VFs created after a PF
>       s390x/pci: Check for multifunction after device realization
>       pcie_sriov: Do not manually unrealize
>       pcie_sriov: Reuse SR-IOV VF device instances
>       pcie_sriov: Release VFs failed to realize
>       pcie_sriov: Remove num_vfs from PCIESriovPF
>       pcie_sriov: Register VFs after migration
>       hw/pci: Use -1 as the default value for rombar
>       hw/qdev: Remove opts member
> 
>  docs/pcie_sriov.txt         |   8 ++-
>  include/hw/pci/pci.h        |   2 +-
>  include/hw/pci/pci_device.h |  19 +++++-
>  include/hw/pci/pcie_sriov.h |   9 +--
>  include/hw/qdev-core.h      |   4 --
>  hw/core/qdev.c              |   1 -
>  hw/net/igb.c                |  13 +++-
>  hw/nvme/ctrl.c              |  24 ++++---
>  hw/pci/pci.c                |  23 ++++---
>  hw/pci/pci_host.c           |   4 +-
>  hw/pci/pcie_sriov.c         | 153 +++++++++++++++++++++++---------------------
>  hw/ppc/spapr_pci.c          |   8 ++-
>  hw/s390x/s390-pci-bus.c     |  14 ++--
>  hw/vfio/pci.c               |   5 +-
>  system/qdev-monitor.c       |  12 ++--
>  hw/pci/trace-events         |   2 +-
>  16 files changed, 175 insertions(+), 126 deletions(-)
> ---
> base-commit: 31669121a01a14732f57c49400bc239cf9fd505f
> change-id: 20240129-reuse-faae22b11934
> 
> Best regards,
> -- 
> Akihiko Odaki <akihiko.odaki@daynix.com>

Re: [PATCH for-9.2 v15 00/11] hw/pci: SR-IOV related fixes and improvements

[Akihiko Odaki]

On 2024/09/10 18:21, Michael S. Tsirkin wrote:
> On Fri, Aug 23, 2024 at 02:00:37PM +0900, Akihiko Odaki wrote:
>> Supersedes: <20240714-rombar-v2-0-af1504ef55de@daynix.com>
>> ("[PATCH v2 0/4] hw/pci: Convert rom_bar into OnOffAuto")
>>
>> I submitted a RFC series[1] to add support for SR-IOV emulation to
>> virtio-net-pci. During the development of the series, I fixed some
>> trivial bugs and made improvements that I think are independently
>> useful. This series extracts those fixes and improvements from the RFC
>> series.
>>
>> [1]: https://patchew.org/QEMU/20231210-sriov-v2-0-b959e8a6dfaf@daynix.com/
>>
>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> 
> I don't think Cédric's issues have been addressed, am I wrong?
> Cédric, what is your take?

I put the URI to Cédric's report here:
https://lore.kernel.org/r/75cbc7d9-b48e-4235-85cf-49dacf3c7483@redhat.com

This issue was dealt with patch "s390x/pci: Check for multifunction 
after device realization". I found that s390x on QEMU does not support 
multifunction and SR-IOV devices accidentally circumvent this 
restriction, which means igb was never supposed to work with s390x. The 
patch prevents adding SR-IOV devices to s390x to ensure the restriction 
is properly enforced.

Regards,
Akihiko Odaki

Re: [PATCH for-9.2 v15 00/11] hw/pci: SR-IOV related fixes and improvements

[Michael S. Tsirkin]

On Tue, Sep 10, 2024 at 06:33:01PM +0900, Akihiko Odaki wrote:
> On 2024/09/10 18:21, Michael S. Tsirkin wrote:
> > On Fri, Aug 23, 2024 at 02:00:37PM +0900, Akihiko Odaki wrote:
> > > Supersedes: <20240714-rombar-v2-0-af1504ef55de@daynix.com>
> > > ("[PATCH v2 0/4] hw/pci: Convert rom_bar into OnOffAuto")
> > > 
> > > I submitted a RFC series[1] to add support for SR-IOV emulation to
> > > virtio-net-pci. During the development of the series, I fixed some
> > > trivial bugs and made improvements that I think are independently
> > > useful. This series extracts those fixes and improvements from the RFC
> > > series.
> > > 
> > > [1]: https://patchew.org/QEMU/20231210-sriov-v2-0-b959e8a6dfaf@daynix.com/
> > > 
> > > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> > 
> > I don't think Cédric's issues have been addressed, am I wrong?
> > Cédric, what is your take?
> 
> I put the URI to Cédric's report here:
> https://lore.kernel.org/r/75cbc7d9-b48e-4235-85cf-49dacf3c7483@redhat.com
> 
> This issue was dealt with patch "s390x/pci: Check for multifunction after
> device realization". I found that s390x on QEMU does not support
> multifunction and SR-IOV devices accidentally circumvent this restriction,
> which means igb was never supposed to work with s390x. The patch prevents
> adding SR-IOV devices to s390x to ensure the restriction is properly
> enforced.
> 
> Regards,
> Akihiko Odaki

Cédric would appreciate your Tested-by/Reviewed-by.

-- 
MST

Re: [PATCH for-9.2 v15 00/11] hw/pci: SR-IOV related fixes and improvements

[Cédric Le Goater]

On 9/10/24 11:33, Akihiko Odaki wrote:
> On 2024/09/10 18:21, Michael S. Tsirkin wrote:
>> On Fri, Aug 23, 2024 at 02:00:37PM +0900, Akihiko Odaki wrote:
>>> Supersedes: <20240714-rombar-v2-0-af1504ef55de@daynix.com>
>>> ("[PATCH v2 0/4] hw/pci: Convert rom_bar into OnOffAuto")
>>>
>>> I submitted a RFC series[1] to add support for SR-IOV emulation to
>>> virtio-net-pci. During the development of the series, I fixed some
>>> trivial bugs and made improvements that I think are independently
>>> useful. This series extracts those fixes and improvements from the RFC
>>> series.
>>>
>>> [1]: https://patchew.org/QEMU/20231210-sriov-v2-0-b959e8a6dfaf@daynix.com/
>>>
>>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>>
>> I don't think Cédric's issues have been addressed, am I wrong?
>> Cédric, what is your take?
> 
> I put the URI to Cédric's report here:
> https://lore.kernel.org/r/75cbc7d9-b48e-4235-85cf-49dacf3c7483@redhat.com
> 
> This issue was dealt with patch "s390x/pci: Check for multifunction after device realization". I found that s390x on QEMU does not support multifunction and SR-IOV devices accidentally circumvent this restriction, which means igb was never supposed to work with s390x. The patch prevents adding SR-IOV devices to s390x to ensure the restriction is properly enforced.

yes, indeed and it seems to fix :

   6069bcdeacee ("s390x/pci: Move some hotplug checks to the pre_plug handler")

I will update patch 4.


Thanks,

C.


That said, the igb device worked perfectly fine under the s390x machine.

Re: [PATCH for-9.2 v15 00/11] hw/pci: SR-IOV related fixes and improvements

[Michael S. Tsirkin]

On Tue, Sep 10, 2024 at 03:21:54PM +0200, Cédric Le Goater wrote:
> On 9/10/24 11:33, Akihiko Odaki wrote:
> > On 2024/09/10 18:21, Michael S. Tsirkin wrote:
> > > On Fri, Aug 23, 2024 at 02:00:37PM +0900, Akihiko Odaki wrote:
> > > > Supersedes: <20240714-rombar-v2-0-af1504ef55de@daynix.com>
> > > > ("[PATCH v2 0/4] hw/pci: Convert rom_bar into OnOffAuto")
> > > > 
> > > > I submitted a RFC series[1] to add support for SR-IOV emulation to
> > > > virtio-net-pci. During the development of the series, I fixed some
> > > > trivial bugs and made improvements that I think are independently
> > > > useful. This series extracts those fixes and improvements from the RFC
> > > > series.
> > > > 
> > > > [1]: https://patchew.org/QEMU/20231210-sriov-v2-0-b959e8a6dfaf@daynix.com/
> > > > 
> > > > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> > > 
> > > I don't think Cédric's issues have been addressed, am I wrong?
> > > Cédric, what is your take?
> > 
> > I put the URI to Cédric's report here:
> > https://lore.kernel.org/r/75cbc7d9-b48e-4235-85cf-49dacf3c7483@redhat.com
> > 
> > This issue was dealt with patch "s390x/pci: Check for multifunction after device realization". I found that s390x on QEMU does not support multifunction and SR-IOV devices accidentally circumvent this restriction, which means igb was never supposed to work with s390x. The patch prevents adding SR-IOV devices to s390x to ensure the restriction is properly enforced.
> 
> yes, indeed and it seems to fix :
> 
>   6069bcdeacee ("s390x/pci: Move some hotplug checks to the pre_plug handler")
> 
> I will update patch 4.
> 
> 
> Thanks,
> 
> C.
> 
> 
> That said, the igb device worked perfectly fine under the s390x machine.

And it works for you after this patchset, yes?

Re: [PATCH for-9.2 v15 00/11] hw/pci: SR-IOV related fixes and improvements

[Cédric Le Goater]

On 9/10/24 15:34, Michael S. Tsirkin wrote:
> On Tue, Sep 10, 2024 at 03:21:54PM +0200, Cédric Le Goater wrote:
>> On 9/10/24 11:33, Akihiko Odaki wrote:
>>> On 2024/09/10 18:21, Michael S. Tsirkin wrote:
>>>> On Fri, Aug 23, 2024 at 02:00:37PM +0900, Akihiko Odaki wrote:
>>>>> Supersedes: <20240714-rombar-v2-0-af1504ef55de@daynix.com>
>>>>> ("[PATCH v2 0/4] hw/pci: Convert rom_bar into OnOffAuto")
>>>>>
>>>>> I submitted a RFC series[1] to add support for SR-IOV emulation to
>>>>> virtio-net-pci. During the development of the series, I fixed some
>>>>> trivial bugs and made improvements that I think are independently
>>>>> useful. This series extracts those fixes and improvements from the RFC
>>>>> series.
>>>>>
>>>>> [1]: https://patchew.org/QEMU/20231210-sriov-v2-0-b959e8a6dfaf@daynix.com/
>>>>>
>>>>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>>>>
>>>> I don't think Cédric's issues have been addressed, am I wrong?
>>>> Cédric, what is your take?
>>>
>>> I put the URI to Cédric's report here:
>>> https://lore.kernel.org/r/75cbc7d9-b48e-4235-85cf-49dacf3c7483@redhat.com
>>>
>>> This issue was dealt with patch "s390x/pci: Check for multifunction after device realization". I found that s390x on QEMU does not support multifunction and SR-IOV devices accidentally circumvent this restriction, which means igb was never supposed to work with s390x. The patch prevents adding SR-IOV devices to s390x to ensure the restriction is properly enforced.
>>
>> yes, indeed and it seems to fix :
>>
>>    6069bcdeacee ("s390x/pci: Move some hotplug checks to the pre_plug handler")
>>
>> I will update patch 4.
>>
>>
>> Thanks,
>>
>> C.
>>
>>
>> That said, the igb device worked perfectly fine under the s390x machine.
> 
> And it works for you after this patchset, yes?

ah no, IGB is not an available device for the s390x machine anymore :

   qemu-system-s390x: -device igb,netdev=net1,mac=C0:FF:EE:00:00:13: multifunction not supported in s390

This is what commit 57da367b9ec4 ("s390x/pci: forbid multifunction
pci device") initially required (and later broken by 6069bcdeacee).
So I guess we are fine with the expected behavior.

Thanks,

C.

Re: [PATCH for-9.2 v15 00/11] hw/pci: SR-IOV related fixes and improvements

[Michael S. Tsirkin]

On Tue, Sep 10, 2024 at 04:13:14PM +0200, Cédric Le Goater wrote:
> On 9/10/24 15:34, Michael S. Tsirkin wrote:
> > On Tue, Sep 10, 2024 at 03:21:54PM +0200, Cédric Le Goater wrote:
> > > On 9/10/24 11:33, Akihiko Odaki wrote:
> > > > On 2024/09/10 18:21, Michael S. Tsirkin wrote:
> > > > > On Fri, Aug 23, 2024 at 02:00:37PM +0900, Akihiko Odaki wrote:
> > > > > > Supersedes: <20240714-rombar-v2-0-af1504ef55de@daynix.com>
> > > > > > ("[PATCH v2 0/4] hw/pci: Convert rom_bar into OnOffAuto")
> > > > > > 
> > > > > > I submitted a RFC series[1] to add support for SR-IOV emulation to
> > > > > > virtio-net-pci. During the development of the series, I fixed some
> > > > > > trivial bugs and made improvements that I think are independently
> > > > > > useful. This series extracts those fixes and improvements from the RFC
> > > > > > series.
> > > > > > 
> > > > > > [1]: https://patchew.org/QEMU/20231210-sriov-v2-0-b959e8a6dfaf@daynix.com/
> > > > > > 
> > > > > > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> > > > > 
> > > > > I don't think Cédric's issues have been addressed, am I wrong?
> > > > > Cédric, what is your take?
> > > > 
> > > > I put the URI to Cédric's report here:
> > > > https://lore.kernel.org/r/75cbc7d9-b48e-4235-85cf-49dacf3c7483@redhat.com
> > > > 
> > > > This issue was dealt with patch "s390x/pci: Check for multifunction after device realization". I found that s390x on QEMU does not support multifunction and SR-IOV devices accidentally circumvent this restriction, which means igb was never supposed to work with s390x. The patch prevents adding SR-IOV devices to s390x to ensure the restriction is properly enforced.
> > > 
> > > yes, indeed and it seems to fix :
> > > 
> > >    6069bcdeacee ("s390x/pci: Move some hotplug checks to the pre_plug handler")
> > > 
> > > I will update patch 4.
> > > 
> > > 
> > > Thanks,
> > > 
> > > C.
> > > 
> > > 
> > > That said, the igb device worked perfectly fine under the s390x machine.
> > 
> > And it works for you after this patchset, yes?
> 
> ah no, IGB is not an available device for the s390x machine anymore :
> 
>   qemu-system-s390x: -device igb,netdev=net1,mac=C0:FF:EE:00:00:13: multifunction not supported in s390


So patch 4 didn't relly help.


> This is what commit 57da367b9ec4 ("s390x/pci: forbid multifunction
> pci device") initially required (and later broken by 6069bcdeacee).
> So I guess we are fine with the expected behavior.
> 
> Thanks,
> 
> C.

Better to fix than to guess if there are users, I think.

-- 
MST

Re: [PATCH for-9.2 v15 00/11] hw/pci: SR-IOV related fixes and improvements

[Akihiko Odaki]

On 2024/09/11 0:27, Michael S. Tsirkin wrote:
> On Tue, Sep 10, 2024 at 04:13:14PM +0200, Cédric Le Goater wrote:
>> On 9/10/24 15:34, Michael S. Tsirkin wrote:
>>> On Tue, Sep 10, 2024 at 03:21:54PM +0200, Cédric Le Goater wrote:
>>>> On 9/10/24 11:33, Akihiko Odaki wrote:
>>>>> On 2024/09/10 18:21, Michael S. Tsirkin wrote:
>>>>>> On Fri, Aug 23, 2024 at 02:00:37PM +0900, Akihiko Odaki wrote:
>>>>>>> Supersedes: <20240714-rombar-v2-0-af1504ef55de@daynix.com>
>>>>>>> ("[PATCH v2 0/4] hw/pci: Convert rom_bar into OnOffAuto")
>>>>>>>
>>>>>>> I submitted a RFC series[1] to add support for SR-IOV emulation to
>>>>>>> virtio-net-pci. During the development of the series, I fixed some
>>>>>>> trivial bugs and made improvements that I think are independently
>>>>>>> useful. This series extracts those fixes and improvements from the RFC
>>>>>>> series.
>>>>>>>
>>>>>>> [1]: https://patchew.org/QEMU/20231210-sriov-v2-0-b959e8a6dfaf@daynix.com/
>>>>>>>
>>>>>>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>>>>>>
>>>>>> I don't think Cédric's issues have been addressed, am I wrong?
>>>>>> Cédric, what is your take?
>>>>>
>>>>> I put the URI to Cédric's report here:
>>>>> https://lore.kernel.org/r/75cbc7d9-b48e-4235-85cf-49dacf3c7483@redhat.com
>>>>>
>>>>> This issue was dealt with patch "s390x/pci: Check for multifunction after device realization". I found that s390x on QEMU does not support multifunction and SR-IOV devices accidentally circumvent this restriction, which means igb was never supposed to work with s390x. The patch prevents adding SR-IOV devices to s390x to ensure the restriction is properly enforced.
>>>>
>>>> yes, indeed and it seems to fix :
>>>>
>>>>     6069bcdeacee ("s390x/pci: Move some hotplug checks to the pre_plug handler")
>>>>
>>>> I will update patch 4.
>>>>
>>>>
>>>> Thanks,
>>>>
>>>> C.
>>>>
>>>>
>>>> That said, the igb device worked perfectly fine under the s390x machine.
>>>
>>> And it works for you after this patchset, yes?
>>
>> ah no, IGB is not an available device for the s390x machine anymore :
>>
>>    qemu-system-s390x: -device igb,netdev=net1,mac=C0:FF:EE:00:00:13: multifunction not supported in s390
> 
> 
> So patch 4 didn't relly help.
> 
> 
>> This is what commit 57da367b9ec4 ("s390x/pci: forbid multifunction
>> pci device") initially required (and later broken by 6069bcdeacee).
>> So I guess we are fine with the expected behavior.
>>
>> Thanks,
>>
>> C.
> 
> Better to fix than to guess if there are users, I think.

Yes, but it will require some knowledge of s390x, which I cannot provide.

Commit 57da367b9ec4 ("s390x/pci: forbid multifunction pci device") says 
having a multifunction device will make the guest spin forever. That is 
not what Cédric observed with igb so it may no longer be relevant, but I 
cannot be sure that the problem is resolved without understanding how 
multifunction devices are supposed to work with s390x.

Ideally someone with s390x expertise should check relevant hardware 
documentation and confirm QEMU properly implements mutlifunction 
devices. Let's keep the restriction until then.

Regards,
Akihiko Odaki

Re: [PATCH for-9.2 v15 00/11] hw/pci: SR-IOV related fixes and improvements

[Michael S. Tsirkin]

On Wed, Sep 11, 2024 at 12:05:46PM +0900, Akihiko Odaki wrote:
> On 2024/09/11 0:27, Michael S. Tsirkin wrote:
> > On Tue, Sep 10, 2024 at 04:13:14PM +0200, Cédric Le Goater wrote:
> > > On 9/10/24 15:34, Michael S. Tsirkin wrote:
> > > > On Tue, Sep 10, 2024 at 03:21:54PM +0200, Cédric Le Goater wrote:
> > > > > On 9/10/24 11:33, Akihiko Odaki wrote:
> > > > > > On 2024/09/10 18:21, Michael S. Tsirkin wrote:
> > > > > > > On Fri, Aug 23, 2024 at 02:00:37PM +0900, Akihiko Odaki wrote:
> > > > > > > > Supersedes: <20240714-rombar-v2-0-af1504ef55de@daynix.com>
> > > > > > > > ("[PATCH v2 0/4] hw/pci: Convert rom_bar into OnOffAuto")
> > > > > > > > 
> > > > > > > > I submitted a RFC series[1] to add support for SR-IOV emulation to
> > > > > > > > virtio-net-pci. During the development of the series, I fixed some
> > > > > > > > trivial bugs and made improvements that I think are independently
> > > > > > > > useful. This series extracts those fixes and improvements from the RFC
> > > > > > > > series.
> > > > > > > > 
> > > > > > > > [1]: https://patchew.org/QEMU/20231210-sriov-v2-0-b959e8a6dfaf@daynix.com/
> > > > > > > > 
> > > > > > > > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> > > > > > > 
> > > > > > > I don't think Cédric's issues have been addressed, am I wrong?
> > > > > > > Cédric, what is your take?
> > > > > > 
> > > > > > I put the URI to Cédric's report here:
> > > > > > https://lore.kernel.org/r/75cbc7d9-b48e-4235-85cf-49dacf3c7483@redhat.com
> > > > > > 
> > > > > > This issue was dealt with patch "s390x/pci: Check for multifunction after device realization". I found that s390x on QEMU does not support multifunction and SR-IOV devices accidentally circumvent this restriction, which means igb was never supposed to work with s390x. The patch prevents adding SR-IOV devices to s390x to ensure the restriction is properly enforced.
> > > > > 
> > > > > yes, indeed and it seems to fix :
> > > > > 
> > > > >     6069bcdeacee ("s390x/pci: Move some hotplug checks to the pre_plug handler")
> > > > > 
> > > > > I will update patch 4.
> > > > > 
> > > > > 
> > > > > Thanks,
> > > > > 
> > > > > C.
> > > > > 
> > > > > 
> > > > > That said, the igb device worked perfectly fine under the s390x machine.
> > > > 
> > > > And it works for you after this patchset, yes?
> > > 
> > > ah no, IGB is not an available device for the s390x machine anymore :
> > > 
> > >    qemu-system-s390x: -device igb,netdev=net1,mac=C0:FF:EE:00:00:13: multifunction not supported in s390
> > 
> > 
> > So patch 4 didn't relly help.
> > 
> > 
> > > This is what commit 57da367b9ec4 ("s390x/pci: forbid multifunction
> > > pci device") initially required (and later broken by 6069bcdeacee).
> > > So I guess we are fine with the expected behavior.
> > > 
> > > Thanks,
> > > 
> > > C.
> > 
> > Better to fix than to guess if there are users, I think.
> 
> Yes, but it will require some knowledge of s390x, which I cannot provide.
> 
> Commit 57da367b9ec4 ("s390x/pci: forbid multifunction pci device") says
> having a multifunction device will make the guest spin forever. That is not
> what Cédric observed with igb so it may no longer be relevant, but I cannot
> be sure that the problem is resolved without understanding how multifunction
> devices are supposed to work with s390x.
> 
> Ideally someone with s390x expertise should check relevant hardware
> documentation and confirm QEMU properly implements mutlifunction devices.

The fact is, QEMU already does what most users want from it. So the
first rule whenever adding a new feature is not breaking old
functionality.  I know, it's annoying, as some of it is held together by
duct tape.  We have a friendly community so if you ask nicely, you
usually can get help. You'd probably have to be a bit more specific
with your questions.

> Let's keep the restriction until then.
> 
> Regards,
> Akihiko Odaki


Not introducing regressions is a hard rule, sorry.

-- 
MST

Re: [PATCH for-9.2 v15 00/11] hw/pci: SR-IOV related fixes and improvements

[Cédric Le Goater]

>> Better to fix than to guess if there are users, I think.
> 
> Yes, but it will require some knowledge of s390x, which I cannot provide.
> 
> Commit 57da367b9ec4 ("s390x/pci: forbid multifunction pci device") says having a multifunction device will make the guest spin forever. That is not what Cédric observed with igb so it may no longer be relevant, but I cannot be sure that the problem is resolved without understanding how multifunction devices are supposed to work with s390x.
> 
> Ideally someone with s390x expertise should check relevant hardware documentation and confirm QEMU properly implements mutlifunction devices. 

I just cc'ed the s390x PCI maintainers on patch 4. Let's see.

Thanks,

C.