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

[Matthew Rosato <mjrosato@linux.ibm.com>]

On 1/22/21 11:46 AM, Cornelia Huck wrote:
> On Thu, 21 Jan 2021 13:06:24 -0500
> Matthew Rosato <mjrosato@linux.ibm.com> wrote:
> 
>> On 1/21/21 12:50 PM, Cornelia Huck wrote:
>>> On Thu, 21 Jan 2021 15:54:22 +0100
>>> Niklas Schnelle <schnelle@linux.ibm.com> wrote:
>>>    
>>>> On 1/21/21 3:46 PM, Pierre Morel wrote:
>>>>>
>>>>>
>>>>> On 1/21/21 2:37 PM, Niklas Schnelle wrote:
>>>>>>
>>>>>>
>>>>>> On 1/21/21 1:30 PM, Pierre Morel wrote:
>>>>>       
>>>>>>>>
>>>>>>>> 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.
>>>>>
>>>>>
>>>>> What does that change if we take one or the other solution considering the checking of MIO/MSIX/relax versus PFT?
>>>>
>>>>
>>>> If it's !MIO && !MSIX && relax_align I'm fine with that check but
>>>> then we should also add PCISTG to the region.
>>>>   
>>>
>>> Just to double check: that would today cover only ISM (which doesn't
>>> use PCISTG), correct?
>>>    
>>
>> Yes, correct -- So to summarize the proposal outlined is to use those
>> features to determine whether a device should be using the region or not
>> rather rather than strictly saying only PFT==ISM may use it.
>>
>> Practically speaking, ISM is the only device that fits the bill today
>> when you combine these things, and ISM does not use PCISTG -- so PCISTG
>> support was simply omitted from the region (prior versions before coming
>> upstream included it, was dropped since ISM doesn't use it).
>>
>> What Niklas suggests here is that, if we are going to be broad in
>> determining whether the region can be used for a given device vs saying
>> 'only PFT==ISM', then we can no longer assume that the device doesn't
>> use PCISTG and as such is suggesting the region should also include
>> PCISTG support as a means of future compatibility (to ensure non-MIO
>> PCISTG is issued for the device).
> 
> Yes, if we go the "check for features" route, including PCISTG makes
> sense.
> 
> However, I'm still not quite sure whether checking for ISM vs checking
> for features makes more sense in the long run. Is ISM that one weird
> device whose cousins you're not going to invite to the party, or is

...  Yes. :)

> there a possibility that there will be devices that could make use of
> the region for performance etc.?
> 

Also yes.  Well, potentially.  That's really what I wanted at the 
outset, but the fly in the ointment is that the region as I exploit it 
in QEMU today only works properly for devices without MSI-X, which 
frankly limits its general-purpose utility.  If we could lift that 
restriction (whether it be now or at a later point) then the region 
becomes quite beneficial for performance of any devices making frequent 
use of large PCISTB operations.