Re: [PATCH for-9.2 v15 04/11] s390x/pci: Check for multifunction after device realization

[Akihiko Odaki <akihiko.odaki@daynix.com>]

On 2024/09/12 6:11, Matthew Rosato wrote:
> On 9/11/24 11:15 AM, Akihiko Odaki wrote:
>> On 2024/09/11 22:53, Matthew Rosato wrote:
>>> On 9/11/24 6:58 AM, Akihiko Odaki wrote:
>>>> On 2024/09/11 18:38, Cédric Le Goater wrote:
>>>>> +Matthew +Eric
>>>>>
>>>>> Side note for the maintainers :
>>>>>
>>>>> Before this change, the igb device, which is multifunction, was working
>>>>> fine under Linux.
>>>>>
>>>>> Was there a fix in Linux since :
>>>>>
>>>>>      57da367b9ec4 ("s390x/pci: forbid multifunction pci device")
>>>>>      6069bcdeacee ("s390x/pci: Move some hotplug checks to the pre_plug handler")
>>>>>
>>>>> ?
>>> The timing of those particular commits predates the linux s390 kernel support of multifunction/SR-IOV.  At that time it was simply not possible on s390.
>>
>> Is it OK to remove this check of multifunction now?
> 
> Yes, I think removing the check (which AFAIU was broken since 6069bcdeacee) will get us back to the behavior Cedric was seeing, where a device that reports multifunction in the config space is still allowed through but only the PF will be usable in the guest.

Commit 6069bcdeacee predates the introduction of SR-IOV (commit 
7c0fa8dff811 ["pcie: Add support for Single Root I/O Virtualization 
(SR/IOV)"]) so it did not break anything, strictly speaking. Ideally, we 
should have taken care of the check when introducing SR-IOV.

> 
>>
>> This code is not working properly with SR-IOV and misleading. It is better to remove the code if it does no good.
>>
>> It would be nice if anyone confirms multifunction works for s390x with the code removed.
> 
> Even if you remove the check, multifunction itself won't work in the s390x guest without these missing CLP pieces too.  When I have some time I'll hack something together to fabricate some CLP data and try it out, but it sounds like Cedric could use his setup to right now at least verify that the PF itself should remain usable in the guest (current behavior).

Well, it seems better to keep the check for non-SR-IOV multifunction 
devices while not enforcing the restriction for SR-IOV.

Multifunction devices without SR-IOV are created with explicit requests 
by specifying multifunction=on for PCI devices. Such requests cannot be 
fulfilled without multifunction CLP so we should reject them.

The situation is different for SR-IOV. SR-IOV is a feature inherent to 
specific type of devices and gets automatically enabled for these 
devices. It may make more sense to ignore just the SR-IOV part of such 
devices and keep the other functionalities working.

The current code implements such a behavior, but it is accidental and 
semantically wrong. I will correct the code to explicitly allow 
multifunction for SR-IOV.

Regards,
Akihiko Odaki