| Message ID | 20230712080146.839113-1-clg@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v4] kconfig: Add PCIe devices to s390x machines | expand |
Hi Cédric, On 12/7/23 10:01, Cédric Le Goater wrote: > It is useful to extend the number of available PCIe devices to KVM guests > for passthrough scenarios and also to expose these models to a different > (big endian) architecture. Introduce a new config PCIE_DEVICES to select > models, Intel Ethernet adapters and one USB controller. These devices all > support MSI-X which is a requirement on s390x as legacy INTx are not > supported. > > Cc: Matthew Rosato <mjrosato@linux.ibm.com> > Cc: Paolo Bonzini <pbonzini@redhat.com> > Cc: Thomas Huth <thuth@redhat.com> > Signed-off-by: Cédric Le Goater <clg@redhat.com> > --- > > There could be a more general use of PCIE_DEVICES > > v4: Introduce PCIE_DEVICES > v3: PCI -> PCI_EXPRESS > v2: select -> imply > > configs/devices/s390x-softmmu/default.mak | 1 + > hw/net/Kconfig | 4 ++-- > hw/pci/Kconfig | 3 +++ > hw/s390x/Kconfig | 3 ++- > hw/usb/Kconfig | 2 +- > 5 files changed, 9 insertions(+), 4 deletions(-) > > diff --git a/configs/devices/s390x-softmmu/default.mak b/configs/devices/s390x-softmmu/default.mak > index f2287a133f36..2d5ff476e32a 100644 > --- a/configs/devices/s390x-softmmu/default.mak > +++ b/configs/devices/s390x-softmmu/default.mak > @@ -7,6 +7,7 @@ > #CONFIG_VFIO_CCW=n > #CONFIG_VIRTIO_PCI=n > #CONFIG_WDT_DIAG288=n > +#CONFIG_PCIE_DEVICE=n > > # Boards: > # > diff --git a/hw/net/Kconfig b/hw/net/Kconfig > index 98e00be4f937..7fcc0d7faa29 100644 > --- a/hw/net/Kconfig > +++ b/hw/net/Kconfig > @@ -41,12 +41,12 @@ config E1000_PCI > > config E1000E_PCI_EXPRESS > bool > - default y if PCI_DEVICES > + default y if PCI_DEVICES || PCIE_DEVICES There seems to be a pre-existing bug, shouldn't this be default y if PCIE_DEVICES ? (Cc'ing maintainers) > depends on PCI_EXPRESS && MSI_NONBROKEN > > config IGB_PCI_EXPRESS > bool > - default y if PCI_DEVICES > + default y if PCI_DEVICES || PCIE_DEVICES Similarly: default y if PCIE_DEVICES > depends on PCI_EXPRESS && MSI_NONBROKEN > > config RTL8139_PCI > diff --git a/hw/pci/Kconfig b/hw/pci/Kconfig > index 77f8b005ffb1..fe70902cd821 100644 > --- a/hw/pci/Kconfig > +++ b/hw/pci/Kconfig > @@ -8,6 +8,9 @@ config PCI_EXPRESS > config PCI_DEVICES > bool > > +config PCIE_DEVICES > + bool > + > config MSI_NONBROKEN > # selected by interrupt controllers that do not support MSI, > # or support it and have a good implementation. See commit > diff --git a/hw/s390x/Kconfig b/hw/s390x/Kconfig > index 454e0ff4b613..4c068d7960b9 100644 > --- a/hw/s390x/Kconfig > +++ b/hw/s390x/Kconfig > @@ -5,7 +5,8 @@ config S390_CCW_VIRTIO > imply VFIO_AP > imply VFIO_CCW > imply WDT_DIAG288 > - select PCI > + imply PCIE_DEVICES > + select PCI_EXPRESS I'm confused, TYPE_S390_PCI_HOST_BRIDGE exposes a PCI bus... At a minimum you'd need: -- >8 -- static const TypeInfo s390_pcihost_info = { .name = TYPE_S390_PCI_HOST_BRIDGE, - .parent = TYPE_PCI_HOST_BRIDGE, + .parent = TYPE_PCIE_HOST_BRIDGE, .instance_size = sizeof(S390pciState), .class_init = s390_pcihost_class_init, .interfaces = (InterfaceInfo[]) { --- Actually I can see: if (s390_pci_msix_init(pbdev) && !pbdev->interp) { error_setg(errp, "MSI-X support is mandatory " "in the S390 architecture"); return; } So this must be PCIe, not legacy PCI, right? > diff --git a/hw/usb/Kconfig b/hw/usb/Kconfig > index 0ec6def4b8b8..0f486764ed69 100644 > --- a/hw/usb/Kconfig > +++ b/hw/usb/Kconfig > @@ -36,7 +36,7 @@ config USB_XHCI > > config USB_XHCI_PCI > bool > - default y if PCI_DEVICES > + default y if PCI_DEVICES || PCIE_DEVICES TYPE_XHCI_PCI inherits TYPE_PCI_DEVICE and implements INTERFACE_PCIE_DEVICE, so this is OK. > depends on PCI > select USB_XHCI >
> diff --git a/configs/devices/s390x-softmmu/default.mak b/configs/devices/s390x-softmmu/default.mak > index f2287a133f36..2d5ff476e32a 100644 > --- a/configs/devices/s390x-softmmu/default.mak > +++ b/configs/devices/s390x-softmmu/default.mak > @@ -7,6 +7,7 @@ > #CONFIG_VFIO_CCW=n > #CONFIG_VIRTIO_PCI=n > #CONFIG_WDT_DIAG288=n > +#CONFIG_PCIE_DEVICE=n Should be CONFIG_PCIE_DEVICES; fixed and queued, thanks. Paolo
On 12/07/2023 14.11, Paolo Bonzini wrote: >> diff --git a/configs/devices/s390x-softmmu/default.mak b/configs/devices/s390x-softmmu/default.mak >> index f2287a133f36..2d5ff476e32a 100644 >> --- a/configs/devices/s390x-softmmu/default.mak >> +++ b/configs/devices/s390x-softmmu/default.mak >> @@ -7,6 +7,7 @@ >> #CONFIG_VFIO_CCW=n >> #CONFIG_VIRTIO_PCI=n >> #CONFIG_WDT_DIAG288=n >> +#CONFIG_PCIE_DEVICE=n > > Should be CONFIG_PCIE_DEVICES; fixed and queued, thanks. I think the patch is fine for now, but in the long run, I think we should make sure to mark PCIe-only devices only with PCIE_DEVICES, as Philippe suggested. So PCIe devices will then also not show up any more on targets that only provide a plain PCI bus (like hppa, I guess?). Thomas
On Wed, Jul 12, 2023 at 2:57 PM Thomas Huth <thuth@redhat.com> wrote: > I think the patch is fine for now, but in the long run, I think we should > make sure to mark PCIe-only devices only with PCIE_DEVICES, as Philippe > suggested. So PCIe devices will then also not show up any more on targets > that only provide a plain PCI bus (like hppa, I guess?). Or even "-M pc", I think. Paolo
>> diff --git a/hw/s390x/Kconfig b/hw/s390x/Kconfig >> index 454e0ff4b613..4c068d7960b9 100644 >> --- a/hw/s390x/Kconfig >> +++ b/hw/s390x/Kconfig >> @@ -5,7 +5,8 @@ config S390_CCW_VIRTIO >> imply VFIO_AP >> imply VFIO_CCW >> imply WDT_DIAG288 >> - select PCI >> + imply PCIE_DEVICES >> + select PCI_EXPRESS > > I'm confused, TYPE_S390_PCI_HOST_BRIDGE exposes a PCI bus... > At a minimum you'd need: > > -- >8 -- > static const TypeInfo s390_pcihost_info = { > .name = TYPE_S390_PCI_HOST_BRIDGE, > - .parent = TYPE_PCI_HOST_BRIDGE, > + .parent = TYPE_PCIE_HOST_BRIDGE, > .instance_size = sizeof(S390pciState), > .class_init = s390_pcihost_class_init, > .interfaces = (InterfaceInfo[]) { > --- > > Actually I can see: > > if (s390_pci_msix_init(pbdev) && !pbdev->interp) { > error_setg(errp, "MSI-X support is mandatory " > "in the S390 architecture"); > return; > } > > So this must be PCIe, not legacy PCI, right? It seems difficult to change now without breaking migration compatibility. C.
On 2023/07/12 19:48, Philippe Mathieu-Daudé wrote: > Hi Cédric, > > On 12/7/23 10:01, Cédric Le Goater wrote: >> It is useful to extend the number of available PCIe devices to KVM guests >> for passthrough scenarios and also to expose these models to a different >> (big endian) architecture. Introduce a new config PCIE_DEVICES to select >> models, Intel Ethernet adapters and one USB controller. These devices all >> support MSI-X which is a requirement on s390x as legacy INTx are not >> supported. >> >> Cc: Matthew Rosato <mjrosato@linux.ibm.com> >> Cc: Paolo Bonzini <pbonzini@redhat.com> >> Cc: Thomas Huth <thuth@redhat.com> >> Signed-off-by: Cédric Le Goater <clg@redhat.com> >> --- >> >> There could be a more general use of PCIE_DEVICES >> >> v4: Introduce PCIE_DEVICES >> v3: PCI -> PCI_EXPRESS >> v2: select -> imply >> configs/devices/s390x-softmmu/default.mak | 1 + >> hw/net/Kconfig | 4 ++-- >> hw/pci/Kconfig | 3 +++ >> hw/s390x/Kconfig | 3 ++- >> hw/usb/Kconfig | 2 +- >> 5 files changed, 9 insertions(+), 4 deletions(-) >> >> diff --git a/configs/devices/s390x-softmmu/default.mak >> b/configs/devices/s390x-softmmu/default.mak >> index f2287a133f36..2d5ff476e32a 100644 >> --- a/configs/devices/s390x-softmmu/default.mak >> +++ b/configs/devices/s390x-softmmu/default.mak >> @@ -7,6 +7,7 @@ >> #CONFIG_VFIO_CCW=n >> #CONFIG_VIRTIO_PCI=n >> #CONFIG_WDT_DIAG288=n >> +#CONFIG_PCIE_DEVICE=n >> # Boards: >> # >> diff --git a/hw/net/Kconfig b/hw/net/Kconfig >> index 98e00be4f937..7fcc0d7faa29 100644 >> --- a/hw/net/Kconfig >> +++ b/hw/net/Kconfig >> @@ -41,12 +41,12 @@ config E1000_PCI >> config E1000E_PCI_EXPRESS >> bool >> - default y if PCI_DEVICES >> + default y if PCI_DEVICES || PCIE_DEVICES > > There seems to be a pre-existing bug, shouldn't this be > > default y if PCIE_DEVICES > > ? I think you should leave this as is and instead add a config selected only when legacy PCI is available and make all legacy PCI devices depend on the config. This will prevent from selecting legacy PCI devices for s390x machines no matter if it's selected due to PCI_DEVICES or selected manually by the user (by mistake). > > (Cc'ing maintainers) > >> depends on PCI_EXPRESS && MSI_NONBROKEN >> config IGB_PCI_EXPRESS >> bool >> - default y if PCI_DEVICES >> + default y if PCI_DEVICES || PCIE_DEVICES > > Similarly: > > default y if PCIE_DEVICES > >> depends on PCI_EXPRESS && MSI_NONBROKEN >> config RTL8139_PCI >> diff --git a/hw/pci/Kconfig b/hw/pci/Kconfig >> index 77f8b005ffb1..fe70902cd821 100644 >> --- a/hw/pci/Kconfig >> +++ b/hw/pci/Kconfig >> @@ -8,6 +8,9 @@ config PCI_EXPRESS >> config PCI_DEVICES >> bool >> +config PCIE_DEVICES >> + bool >> + >> config MSI_NONBROKEN >> # selected by interrupt controllers that do not support MSI, >> # or support it and have a good implementation. See commit >> diff --git a/hw/s390x/Kconfig b/hw/s390x/Kconfig >> index 454e0ff4b613..4c068d7960b9 100644 >> --- a/hw/s390x/Kconfig >> +++ b/hw/s390x/Kconfig >> @@ -5,7 +5,8 @@ config S390_CCW_VIRTIO >> imply VFIO_AP >> imply VFIO_CCW >> imply WDT_DIAG288 >> - select PCI >> + imply PCIE_DEVICES >> + select PCI_EXPRESS > > I'm confused, TYPE_S390_PCI_HOST_BRIDGE exposes a PCI bus... > At a minimum you'd need: > > -- >8 -- > static const TypeInfo s390_pcihost_info = { > .name = TYPE_S390_PCI_HOST_BRIDGE, > - .parent = TYPE_PCI_HOST_BRIDGE, > + .parent = TYPE_PCIE_HOST_BRIDGE, > .instance_size = sizeof(S390pciState), > .class_init = s390_pcihost_class_init, > .interfaces = (InterfaceInfo[]) { > --- > > Actually I can see: > > if (s390_pci_msix_init(pbdev) && !pbdev->interp) { > error_setg(errp, "MSI-X support is mandatory " > "in the S390 architecture"); > return; > } > > So this must be PCIe, not legacy PCI, right? > >> diff --git a/hw/usb/Kconfig b/hw/usb/Kconfig >> index 0ec6def4b8b8..0f486764ed69 100644 >> --- a/hw/usb/Kconfig >> +++ b/hw/usb/Kconfig >> @@ -36,7 +36,7 @@ config USB_XHCI >> config USB_XHCI_PCI >> bool >> - default y if PCI_DEVICES >> + default y if PCI_DEVICES || PCIE_DEVICES > > TYPE_XHCI_PCI inherits TYPE_PCI_DEVICE and implements > INTERFACE_PCIE_DEVICE, so this is OK. > >> depends on PCI >> select USB_XHCI >
diff --git a/configs/devices/s390x-softmmu/default.mak b/configs/devices/s390x-softmmu/default.mak index f2287a133f36..2d5ff476e32a 100644 --- a/configs/devices/s390x-softmmu/default.mak +++ b/configs/devices/s390x-softmmu/default.mak @@ -7,6 +7,7 @@ #CONFIG_VFIO_CCW=n #CONFIG_VIRTIO_PCI=n #CONFIG_WDT_DIAG288=n +#CONFIG_PCIE_DEVICE=n # Boards: # diff --git a/hw/net/Kconfig b/hw/net/Kconfig index 98e00be4f937..7fcc0d7faa29 100644 --- a/hw/net/Kconfig +++ b/hw/net/Kconfig @@ -41,12 +41,12 @@ config E1000_PCI config E1000E_PCI_EXPRESS bool - default y if PCI_DEVICES + default y if PCI_DEVICES || PCIE_DEVICES depends on PCI_EXPRESS && MSI_NONBROKEN config IGB_PCI_EXPRESS bool - default y if PCI_DEVICES + default y if PCI_DEVICES || PCIE_DEVICES depends on PCI_EXPRESS && MSI_NONBROKEN config RTL8139_PCI diff --git a/hw/pci/Kconfig b/hw/pci/Kconfig index 77f8b005ffb1..fe70902cd821 100644 --- a/hw/pci/Kconfig +++ b/hw/pci/Kconfig @@ -8,6 +8,9 @@ config PCI_EXPRESS config PCI_DEVICES bool +config PCIE_DEVICES + bool + config MSI_NONBROKEN # selected by interrupt controllers that do not support MSI, # or support it and have a good implementation. See commit diff --git a/hw/s390x/Kconfig b/hw/s390x/Kconfig index 454e0ff4b613..4c068d7960b9 100644 --- a/hw/s390x/Kconfig +++ b/hw/s390x/Kconfig @@ -5,7 +5,8 @@ config S390_CCW_VIRTIO imply VFIO_AP imply VFIO_CCW imply WDT_DIAG288 - select PCI + imply PCIE_DEVICES + select PCI_EXPRESS select S390_FLIC select S390_FLIC_KVM if KVM select SCLPCONSOLE diff --git a/hw/usb/Kconfig b/hw/usb/Kconfig index 0ec6def4b8b8..0f486764ed69 100644 --- a/hw/usb/Kconfig +++ b/hw/usb/Kconfig @@ -36,7 +36,7 @@ config USB_XHCI config USB_XHCI_PCI bool - default y if PCI_DEVICES + default y if PCI_DEVICES || PCIE_DEVICES depends on PCI select USB_XHCI
It is useful to extend the number of available PCIe devices to KVM guests for passthrough scenarios and also to expose these models to a different (big endian) architecture. Introduce a new config PCIE_DEVICES to select models, Intel Ethernet adapters and one USB controller. These devices all support MSI-X which is a requirement on s390x as legacy INTx are not supported. Cc: Matthew Rosato <mjrosato@linux.ibm.com> Cc: Paolo Bonzini <pbonzini@redhat.com> Cc: Thomas Huth <thuth@redhat.com> Signed-off-by: Cédric Le Goater <clg@redhat.com> --- There could be a more general use of PCIE_DEVICES v4: Introduce PCIE_DEVICES v3: PCI -> PCI_EXPRESS v2: select -> imply configs/devices/s390x-softmmu/default.mak | 1 + hw/net/Kconfig | 4 ++-- hw/pci/Kconfig | 3 +++ hw/s390x/Kconfig | 3 ++- hw/usb/Kconfig | 2 +- 5 files changed, 9 insertions(+), 4 deletions(-)