| Message ID | 20240913-reuse-v16-3-d016b4b4f616@daynix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/pci: SR-IOV related fixes and improvements | expand |
Adding : Harsh for QEMU/PPC pseries machine, Shivaprasad for KVM/PPC VFIO and IOMMU support. Thanks, C. On 9/13/24 05:44, Akihiko Odaki wrote: > 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.", >
On 9/18/24 7:57 PM, Cédric Le Goater wrote: > Adding : > > Harsh for QEMU/PPC pseries machine, > Shivaprasad for KVM/PPC VFIO and IOMMU support. > > Thanks, > > C. > > > On 9/13/24 05:44, Akihiko Odaki wrote: >> 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) && I see there is history to this change. The reverted[1] virtio-net-pci SRIOV emulation support needed this as the VFs were explicitly specified with -device virtio-net-pci,sriov-pf=X property. I see the pre_plug handlers for the VFs cant be reached now with the reverted code base for the other devices(nvme and igb) supporting the SRIOV emulation. Do the VFs really reach this path in today's code base ? Other than the above workflow, the pre_plug() handlers wont be called for VFs when the echo X > /<sys-fs-pf-path>/sriov_numvfs inside the guest too. I don't see the workflow(PF automatically creating VFs) to hit this path. Could you clarify how? I see before the revert of virito-net-pci sriov use-case, the out of order VF hot|cold plug post PF are prevented here. Even if we allowed VFs to continue here, PFs were prevented in pcie_sriov_register_device() which is followed sequentially anyway. Now, as the pcie_sriov_register_device() is no longer there, this check actually makes sense as this would be the only place we avoid the out of order plugging. On a side note, for testing this fulky on PPC, we need more work on Qemu today as the open-sriov[2] is supported only on PowerVM. Thanks, Shivaprasad Reference : [1] - Atleast till commit b0fdaee5d1 [2] - https://lore.kernel.org/linuxppc-dev/20180105164552.36371-1-bryantly@linux.vnet.ibm.com/
On 2024/10/12 2:22, Shivaprasad G Bhat wrote: > On 9/18/24 7:57 PM, Cédric Le Goater wrote: >> Adding : >> >> Harsh for QEMU/PPC pseries machine, >> Shivaprasad for KVM/PPC VFIO and IOMMU support. >> >> Thanks, >> >> C. >> >> >> On 9/13/24 05:44, Akihiko Odaki wrote: >>> 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) && > > I see there is history to this change. The reverted[1] virtio-net-pci > SRIOV emulation support > > needed this as the VFs were explicitly specified with -device virtio- > net-pci,sriov-pf=X > > property. I see the pre_plug handlers for the VFs cant be reached now > with the reverted > > code base for the other devices(nvme and igb) supporting the SRIOV > emulation. > > > Do the VFs really reach this path in today's code base ? Other than the > above > > workflow, the pre_plug() handlers wont be called for VFs when the > > echo X > /<sys-fs-pf-path>/sriov_numvfs inside the guest too. I don't > see the > > workflow(PF automatically creating VFs) to hit this path. Could you > clarify how? > > > I see before the revert of virito-net-pci sriov use-case, the out of > order VF hot|cold > > plug post PF are prevented here. Even if we allowed VFs to continue > here, PFs were > > prevented in pcie_sriov_register_device() which is followed sequentially > anyway. Now, > > as the pcie_sriov_register_device() is no longer there, this check > actually makes > > sense as this would be the only place we avoid the out of order plugging. VFs are always plugged after its paired PF. Currently, VFs are plugged when the guest writes sriov_numvfs. With "[PATCH v16 08/13] pcie_sriov: Reuse SR-IOV VF device instances", which follows this patch, VFs will plug while the PF is being realized. I have no idea why you can't reproduce the issue by writing sriov_numvfs, but it is easy to reproduce it with "[PATCH v16 08/13] pcie_sriov: Reuse SR-IOV VF device instances" applied. You can use the following command: qemu-system-ppc64 -nographic -monitor stdio -serial none <<< 'device_add igb' It should say: Error: PCI: slot 18 function 0 already occupied by igbvf, additional functions can no longer be exposed to guest. Regards, Akihiko Odaki > > > On a side note, for testing this fulky on PPC, we need more work on Qemu > today as the > > open-sriov[2] is supported only on PowerVM. > > > Thanks, > > Shivaprasad > > > Reference : > > [1] - Atleast till commit b0fdaee5d1 > > [2] - https://lore.kernel.org/linuxppc-dev/20180105164552.36371-1- > bryantly@linux.vnet.ibm.com/ >
On 10/12/24 5:40 PM, Akihiko Odaki wrote: > On 2024/10/12 2:22, Shivaprasad G Bhat wrote: >> On 9/18/24 7:57 PM, Cédric Le Goater wrote: >>> Adding : >>> >>> Harsh for QEMU/PPC pseries machine, >>> Shivaprasad for KVM/PPC VFIO and IOMMU support. >>> >>> Thanks, >>> >>> C. >>> >>> >>> On 9/13/24 05:44, Akihiko Odaki wrote: >>>> 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) && >> >> I see there is history to this change. The reverted[1] virtio-net-pci >> SRIOV emulation support >> >> needed this as the VFs were explicitly specified with -device virtio- >> net-pci,sriov-pf=X >> >> property. I see the pre_plug handlers for the VFs cant be reached now >> with the reverted >> >> code base for the other devices(nvme and igb) supporting the SRIOV >> emulation. >> >> >> Do the VFs really reach this path in today's code base ? Other than >> the above >> >> workflow, the pre_plug() handlers wont be called for VFs when the >> >> echo X > /<sys-fs-pf-path>/sriov_numvfs inside the guest too. I don't >> see the >> >> workflow(PF automatically creating VFs) to hit this path. Could you >> clarify how? >> >> >> I see before the revert of virito-net-pci sriov use-case, the out of >> order VF hot|cold >> >> plug post PF are prevented here. Even if we allowed VFs to continue >> here, PFs were >> >> prevented in pcie_sriov_register_device() which is followed >> sequentially anyway. Now, >> >> as the pcie_sriov_register_device() is no longer there, this check >> actually makes >> >> sense as this would be the only place we avoid the out of order >> plugging. > > VFs are always plugged after its paired PF. Currently, VFs are plugged > when the guest writes sriov_numvfs. With "[PATCH v16 08/13] > pcie_sriov: Reuse SR-IOV VF device instances", which follows this > patch, VFs will plug while the PF is being realized. > Thanks, I was juggling between the reverted patches and the current ones and missed this patch. I see things fine now. Reviewed-by: Shivaprasad G Bhat <sbhat@linux.ibm.com> Tested-by: Shivaprasad G Bhat <sbhat@linux.ibm.com> > I have no idea why you can't reproduce the issue by writing sriov_numvfs, On PPC64, IOV resources are disabled and the sriov attributes in sysfs are not accessible without open-sriov support [2](not yet added on qemu). However, the config space is showing the IOV capabilities and the values fine with the patches. > but it is easy to reproduce it with "[PATCH v16 08/13] pcie_sriov: > Reuse SR-IOV VF device instances" applied. You can use the following > command: > qemu-system-ppc64 -nographic -monitor stdio -serial none <<< > 'device_add igb' > > It should say: > Error: PCI: slot 18 function 0 already occupied by igbvf, additional > functions can no longer be exposed to guest. > > Regards, > Akihiko Odaki > >> >> >> On a side note, for testing this fulky on PPC, we need more work on >> Qemu today as the >> >> open-sriov[2] is supported only on PowerVM. >> >> >> Thanks, >> >> Shivaprasad >> >> >> Reference : >> >> [1] - Atleast till commit b0fdaee5d1 >> >> [2] - https://lore.kernel.org/linuxppc-dev/20180105164552.36371-1- >> bryantly@linux.vnet.ibm.com/ >> >
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.",
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(-)