Re: [PATCH v4] kconfig: Add PCIe devices to s390x machines

[Philippe Mathieu-Daudé]

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
>