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

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

[Cédric Le Goater]

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
     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
 
-- 
2.41.0

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
>   

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

[Paolo Bonzini]

> 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

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

[Thomas Huth]

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

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

[Paolo Bonzini]

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

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

[Cédric Le Goater]

>> 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.

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
>