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

[Cédric Le Goater]

On 7/5/23 16:54, Matthew Rosato wrote:
> On 7/4/23 9:33 AM, Philippe Mathieu-Daudé wrote:
> > On 4/7/23 14:32, Cédric Le Goater wrote:
> > > On 7/4/23 14:09, Philippe Mathieu-Daudé wrote:
> > > > On 4/7/23 14:01, Cédric Le Goater wrote:
> > > > > It is useful to extend the number of available PCI devices to KVM guests
> > > > > for passthrough scenarios and also to expose these models to a different
> > > > > (big endian) architecture. Include models for Intel Ethernet adapters
> > > > > and one USB controller, which all support MSI-X. Devices only supporting
> > > > > INTx won't work on s390x.
> > > > >
> > > > > Signed-off-by: Cédric Le Goater <clg@redhat.com>
> > > > > ---
> > > > >
> > > > >    Tested under KVM as a machine device, under KVM nested as a passthrough
> > > > >    device
> > > > >
> > > > >    hw/s390x/Kconfig | 4 ++++
> > > > >    1 file changed, 4 insertions(+)
> > > > >
> > > > > diff --git a/hw/s390x/Kconfig b/hw/s390x/Kconfig
> > > > > index 5e7d8a2bae8b..7a82c58cdf6e 100644
> > > > > --- a/hw/s390x/Kconfig
> > > > > +++ b/hw/s390x/Kconfig
> > > > > @@ -10,3 +10,7 @@ config S390_CCW_VIRTIO
> > > > >        select SCLPCONSOLE
> > > > >        select VIRTIO_CCW
> > > > >        select MSI_NONBROKEN
> > > > > +    imply PCI_EXPRESS
> > > >
> > > > No, PCIe is a bus, which is implemented in s390-pci-bus.c;
> > > > S390_CCW_VIRTIO exposes this bus, so we Kconfig SELECT it.
> > > >
> > > > > +    imply E1000E_PCI_EXPRESS
> > > > > +    imply IGB_PCI_EXPRESS
> > > > > +    imply USB_XHCI_PCI
> > > >
> > > > These are devices you can plug on a PCIe bus, so Kconfig
> > > > IMPLY is correct.
> > >
> > > If I understand correctly, this should be ?
> > >
> > > @@ -5,8 +5,11 @@ config S390_CCW_VIRTIO
> > >        imply VFIO_AP
> > >        imply VFIO_CCW
> > >        imply WDT_DIAG288
> > > -    select PCI
> > > +    select PCI_EXPRESS
> > >        select S390_FLIC
> > >        select SCLPCONSOLE
> > >        select VIRTIO_CCW
> > >        select MSI_NONBROKEN
> > > +    imply E1000E_PCI_EXPRESS
> > > +    imply IGB_PCI_EXPRESS
> > > +    imply USB_XHCI_PCI
> >
> > This is how I'd write this patch. Note I have zero knowledge of zPCI.
>
> Indeed, our s390x PCI emulation is lacking in some places (e.g. missing legacy 
> interrupts as Thomas indicated in a prior thread) so we want to be selective 
> about what we enable.
>
> I have no strong objection to adding them as long as you've tested them.
>
> Based on the above comments, will there be a v3?  I don't have the imply'd 
> devices readily available for test but I did do some passthrough and virtio 
> sanity-testing with s390x hardware to make sure this changes doesn't regress 
> anything there.  I used the diff just above (select PCI_EXPRESS + imply*3)

Good. The VM I use for tests has the following PCI devices :

 0001:00:00.0 Ethernet controller: Mellanox Technologies MT27710 Family 
[ConnectX-4 Lx Virtual Function]
 0002:00:00.0 Non-VGA unclassified device: IBM Internal Shared Memory (ISM) 
virtual PCI device
 0003:00:00.0 Ethernet controller: Intel Corporation 82574L Gigabit Network 
Connection
 0004:00:00.0 Ethernet controller: Intel Corporation 82576 Gigabit Network 
Connection (rev 01)

The first two devices are passthrough devices from the LPAR. Then in the nested,
I simply pass one of the last two devices, emulated in QEMU. Both kernels have
the Intel net drivers to check connectivity with the LPAR. These are not shipped
by default.

Sending a v3.

Thanks,

C.