Re: [PATCH 0/8] s390x/pci: Fixing s390 vfio-pci ISM support

[Niklas Schnelle]

On 1/21/21 1:30 PM, Pierre Morel wrote:
> 
> 
> On 1/21/21 10:58 AM, Niklas Schnelle wrote:
>>
>>
>> On 1/21/21 9:27 AM, Pierre Morel wrote:
>>>
>>>
>>> On 1/20/21 9:29 PM, Matthew Rosato wrote:
>>>> On 1/20/21 2:18 PM, Pierre Morel wrote:
>>>>>
>>>>>
>>> ...snip...
>>>
>>>>
>>>> So, I mean I can change the code to be more permissive in that way (allow 
>>>> any device that doesn't have MSI-X capability to at least attempt to use 
>>>> the region).  But the reality is that ISM specifically needs the region 
>>>> for successful pass through, so I don't see a reason to create a different 
>>>> bit for that vs just checking for the PFT in QEMU and using that value to 
>>>> decide whether or not region availability is a requirement for allowing 
>>>> the device to pass through.
>>>
>>>
>>> There is no need for a new bit to know if a device support MIO or not, as I 
>>> said before, there is already one in the CLP query PCI function response 
>>> and it is already used in the kernel zPCI architecture.
>>>
>>>
>>> It is not a big think to do and does not change the general architecture of 
>>> the patch, only the detection of which device is impacted to make it 
>>> generic instead of device dedicated.
>>>
>>> Regards,
>>> Pierre
>>
>> Just wanted to say that we've had a very similar discussion with
>> Cornelia end of last year and came to the conclusion that explicitly
>> matching the PFT is likely the safest bet:
>> https://lkml.org/lkml/2020/12/22/479
> 
> What I see there is a discussion on the relation between relaxed access and 
> MIO without explaining to Connie that we have in the kernel the possibility 
> to know if a device support MIO or not independently of it supports the 
> relaxed access.
> 
> The all point here is about taking decisions for the right reasons.
> 
> We have the possibility to take the decision based on functionalities and not 
> on a specific PCI function.

Yes but that goes both ways the functionality of the region has to match
that of the device and at least in it's current state the regions functionality
matches only ISM in a way that is so specific that it is very unlikely to match 
anything
else. For example it can't support a PCI device that requires non-MIO but
also MSI-X. In its current form it doesn't even support PCI Store only PCI Store
Block, we had that in an earlier version and it's trivial but then we get the 
MSI-X
problem.

> 
> 
> If we keep the PFT check, and we can do this of course, but is it a good 
> solution if it appears we have other PFT with the same functionalities?
> 
> Please note that this is a minor code change, keeping track of the MIO 
> support just as we keep track of the PFT and check on this instead of on the 
> PFT.

That is certainly true and I'm not strongly against matching on functionality
it just seems to me that it's too specific for that to make sense and
in that case I feel it's better to be clear about that and make it ISM
specific in name and functionality.

If we manage to find a fix for the MSI-X problem which I'd be really happy about
we can simply extend the regions functionality and reuse the same code
for a backwards compatibile ISM region and a more generic zPCI non-MIO
region that could even be used if the client (QEMU) uses non-MIO and
the device can do both as is the case for all current physical devices.

> 
> It does not modify the general architecture of the patch series neither in 
> kernel nor in QEMU at all.
> 
> 
> Pierre
>