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

[Akihiko Odaki]

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