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

[Matthew Rosato]

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)

Thanks,
Matt