| Message ID | 20240913-reuse-v16-2-d016b4b4f616@daynix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/pci: SR-IOV related fixes and improvements | expand |
Hello, Adding : Harsh for QEMU/PPC pseries machine, Shivaprasad for KVM/PPC VFIO and IOMMU support. Could you please give us your feedback on these changes ? Thanks, C. On 9/13/24 05:44, Akihiko Odaki wrote: > 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; >
On 9/18/24 19:57, Cédric Le Goater wrote: > Hello, > > Adding : > > Harsh for QEMU/PPC pseries machine, > Shivaprasad for KVM/PPC VFIO and IOMMU support. > > Could you please give us your feedback on these changes ? > > Thanks, > > C. > > > > On 9/13/24 05:44, Akihiko Odaki wrote: >> 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; >> + } >> + While I will let Shivaprasad comment from IO perspective, I would like to suggest merging this condition with the error condition check preceding it. regards, Harsh >> err = spapr_dt_pci_device(p->sphb, pdev, p->fdt, p->offset); >> if (err < 0) { >> p->err = err; >> > >
On 9/18/24 7:57 PM, Cédric Le Goater wrote: > Hello, > > Adding : > > Harsh for QEMU/PPC pseries machine, > Shivaprasad for KVM/PPC VFIO and IOMMU support. > > Could you please give us your feedback on these changes ? > > Thanks, > > C. > > > > On 9/13/24 05:44, Akihiko Odaki wrote: >> Disabled means it is a disabled SR-IOV VF or it is powered off, and >> hidden from the guest. I see you are taking care of not powering on VFs in the following 8th patch in the series. Without it, this patch doesn't hold. Hope this patch and the 8th patch go together. Reviewed-by: Shivaprasad G Bhat <sbhat@linux.ibm.com> Thanks, Shivaprasad >> >> 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; >>
On 10/11/24 10:52 PM, Shivaprasad G Bhat wrote: > > On 9/18/24 7:57 PM, Cédric Le Goater wrote: >> Hello, >> >> Adding : >> >> Harsh for QEMU/PPC pseries machine, >> Shivaprasad for KVM/PPC VFIO and IOMMU support. >> >> Could you please give us your feedback on these changes ? >> >> Thanks, >> >> C. >> >> >> >> On 9/13/24 05:44, Akihiko Odaki wrote: >>> Disabled means it is a disabled SR-IOV VF or it is powered off, and >>> hidden from the guest. > > I see you are taking care of not powering on VFs in the following 8th > patch in > > the series. Without it, this patch doesn't hold. Hope this patch and > the 8th patch > > go together. > > > Reviewed-by: Shivaprasad G Bhat <sbhat@linux.ibm.com> > > While review/testing the patch again with the [8/13], I see the same check is needed in spapr_pci_dt_populate() before the call to spapr_dt_pci_device() to take care of the hotplug path. Kindly add the same there too. So, my Review-by would be with that. Thanks, Shivaprasad > Thanks, > > Shivaprasad > >>> >>> 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; >>>
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;
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(+)