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

[Matthew Rosato]

On 1/21/21 3: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 we have:
> > > devices supporting MIO and MSIX
> > > devices not supporting MIO nor MSIX
> > > devices not supporting the use of PCISTG to emulate PCISTB
> > >
> > > The first two are two different things indicated by two different 
> > > entries in the clp query PCI function response.
> > >
> > > The last one, we do not have an indicator as if the relaxed alignment 
> > > and length is set, PCISTB can not be emulated with PCISTG
>
>
> hum sorry, it seems I rewrote my sentence until it was wrong wrong!
> I wanted to say we DO HAVE an indicator with the relaxed bit...

That's actually not quite true though...  The relaxed bit does not 
directly imply that PCISTB cannot be emulated with PCISTG.  Rather, it 
more generally implies that PCISTB could be used instead of PCISTG as 
the length and alignment requirements for PCISTB are waived.  As far as 
I can tell, disallowing a PCISTG altogether is a trait that is unique to 
ISM and I don't see anywhere that it's otherwise indicated in 
architecture...  And in fact, for a given ISM device, certain address 
spaces (command) WILL accept a series of PCISTG issued in a particular 
manner in place of a PCISTB; meanwhile other ISM address spaces (data) 
will not accept any PCISTG ever.  :(  The ISM driver just always uses 
PCISTB.

So that is why I suggested type==ISM must require use of the region 
whereas other types that implement MSI could request (but not require) 
use of the region.

But yes, any other theoretical piece of hardware that also does not 
support MIO would have a similar region requirement.  I'll have a look 
at the MIO bit you referenced below and will have to verify that it does 
exactly what we expect for an ISM device.  Assuming yes, I will consider 
the following sort of checking for the next version of QEMU...

if (!MIO) {
        if (MSIX) {
                // passthrough disallowed
        }
        else {
                // region required, disallow if unavailable
        }
}
else if(RELAX && !MSIX) {
        // use region if available
}

Sound good?

All said, this would result in another bit passed from the kernel as CLP 
info and some small changes to patch 6 of this set to determine when to 
call s390_pci_get_zpci_io_region(pbdev) -- But just about everything 
else is unaffected, so @all please feel free to provide other review 
comments for the series in the meanwhile.


> > > What I mean with this is that considering the proposed implementation 
> > > and considering:
> > > MIO MSIX RELAX
> > >
> > > 0 0 1  -> must use the new region (ISM)
> > > 1 1 0  -> must use the standard VFIO region (MLX)
> > >
> > > we can discuss other 6 possibilities
> > >
> > > 0 0 0 -> must use the new region
> > > 0 1 0 -> NOOP
> > > 0 1 1 -> NOOP
> > > 1 0 0 -> can use any region
> > > 1 0 1 -> can use any region
> > > 1 1 1 -> NOOP
> > >
> > > In my opinion the test for using one region or another should be done 
> > > on these indicator instead of using the PFT. > This may offer us more 
> > > compatibility with other hardware we may not be
> > > aware of as today.
> >
> > This gets a little shaky, and goes both ways -- Using your list, a 
> > device that supports MIO, does not have MSI-X capability and doesn't 
> > support relaxed alignment (1 0 0 from above) can use any region -- but 
> > that may not always be true.  What if "other hardware we may not be 
> > aware of as today" includes future hardware that ONLY supports the MIO 
> > instruction set?  Then that device really can't use this region either.
>
>
> Right, but there is no bit in the CLP response for this case.
> Until there is one, the system is supposed to handle legacy instructions
>
> > But forgetting that possibility...  I think we can really simplify the 
> > above matrix down to a statement of "if device doesn't support MSI-X 
> > but DOES support non-MIO instructions, it can use the region."  I 
> > believe the latter half of that statement is implicit in the 
> > architecture today, so it's really then "if device doesn't support 
> > MSI-X, it can use the region".  There's just the caveat of, if the 
> > device is ISM, it changes from 'can use the region' to 'must use the 
> > region'.
>
>
> There can surely be simplifications.
>
> > 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