| Message ID | 20240823-reuse-v15-0-eddcb960e289@daynix.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | hw/pci: SR-IOV related fixes and improvements | expand |
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>
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
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.
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.
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?
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.
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.
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
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.
>> 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.
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,