On 1/20/21 2:18 PM, Pierre Morel wrote:
On 1/20/21 4:59 PM, Matthew Rosato wrote:
On 1/20/21 9:45 AM, Pierre Morel wrote:
On 1/20/21 3:03 PM, Matthew Rosato wrote:
On 1/20/21 4:12 AM, Pierre Morel wrote:
On 1/19/21 9:44 PM, Matthew Rosato wrote:
Today, ISM devices are completely disallowed for vfio-pci
passthrough as
QEMU rejects the device due to an (inappropriate) MSI-X check.
Removing
this fence, however, reveals additional deficiencies in the s390x PCI
interception layer that prevent ISM devices from working correctly.
Namely, ISM block write operations have particular requirements in
regards
to the alignment, size and order of writes performed that cannot be
guaranteed when breaking up write operations through the typical
vfio_pci_bar_rw paths. Furthermore, ISM requires that legacy/non-MIO
s390 PCI instructions are used, which is also not guaranteed when
the I/O
is passed through the typical userspace channels.
This patchset provides a set of fixes related to enabling ISM device
passthrough and includes patches to enable use of a new vfio
region that
will allow s390x PCI pass-through devices to perform s390 PCI
instructions
in such a way that the same instruction issued on the guest is
re-issued
on the host.
Associated kernel patchset:
https://lkml.org/lkml/2021/1/19/874
Changes from RFC -> v1:
- Refresh the header sync (built using Eric's 'update-linux-headers:
Include const.h' + manually removed pvrdma_ring.h again)
- Remove s390x/pci: fix pcistb length (already merged)
- Remove s390x/pci: Fix memory_region_access_valid call (already
merged)
- Fix bug: s390_pci_vfio_pcistb should use the pre-allocated PCISTB
buffer pcistb_buf rather than allocating/freeing its own.
- New patch: track the PFT (PCI Function Type) separately from
guest CLP
response data -- we tell the guest '0' for now due to limitations in
measurement block support, but we can still use the real value
provided via
the vfio CLP capabilities to make decisions.
- Use the PFT (pci function type) to determine when to use the region
for PCISTB/PCILG (only for ISM), rather than using the relaxed
alignment
bit.
- As a result, the pcistb_default is now updated to also handle the
possibility of relaxed alignment via 2 new functions,
pcistb_validate_write
and pcistb_write, which serve as wrappers to the memory_region calls.
- New patch, which partially restores the MSI-X fence for passthrough
devices... Could potentially be squashed with 's390x/pci: MSI-X
isn't
strictly required for passthrough' but left separately for now as
I felt it
needed a clear commit description of why we should still fence
this case.
Hi,
The choice of using the new VFIO region is made on the ISM PCI
function type (PFT), which makes the patch ISM specific, why don't
we use here the MIO bit common to any zPCI function and present in
kernel to make the choice?
As discussed during the RFC (and see my reply also to the kernel
set), the use of this region only works for devices that do not rely
on MSI-X interrupts. If we did as you suggest, other device types
like mlx would not receive MSI-X interrupts in the guest (And I did
indeed try variations where I used the special VFIO region for all
PCISTG/PCILG/PCISTB for various device types)
So the idea for now was to solve the specific problem at hand
(getting ISM devices working).
Sorry, if I missed or forgot some discussions, but I understood that
we are using this region to handle PCISTB instructions when the
device do not support MIO.
Don't we?
Sure thing - It's probably good to refresh the issue/rationale anyway
as we've had the holidays in between.
You are correct, a primary reason we need to resort to a separate VFIO
region for PCISTB (and PCILG) instructions for ISM devices is that
they do not support the MIO instruction set, yet the host kernel will
translate everything coming through the PCI I/O layer to MIO
instructions whenever that facility is available to the host (and not
purposely disabled). This issue is unique to vfio-pci/passthrough -
in the host, the ISM driver directly invokes functions in s390 pci
code to ensure that MIO instructions are not used.
QEMU intercepts and differentiates PCISTG and PCISTB.
The new hardware support both MIO and legacy PCISTB/PCISTG.
QEMU does not support MIO
My first interrogation is why should we translate legacy to MIO?
This is existing behavior of the s390 kernel PCI layer, not something
I'm introducing here.
So, as you say, QEMU does not support MIO, nor does it attempt to
translate anything to MIO. The thing is, to the host kernel, PCI I/O
coming from QEMU through the standard vfio-pci codepath just looks like
any other userspace PCI I/O coming in to the kernel. So the
memory_region operations against the vfio virtual address space bars in
QEMU then turn into iowrite/ioread operations in vfio-pci in the kernel,
which then subsequently end up in the s390 PCI kernel layer, and it's
there that everything is turned into an MIO operation if MIO is
available. That's not limited to vfio-pci, that's all PCI I/O in the
host (except for the ISM driver, which bypasses this behavior by
invoking s390 PCI kernel interfaces directly).
In an early (internal) version of the kernel component to this I floated
the idea of trying to determine whether or not MIO instructions could be
used for a given I/O operation, but the ideas I floated had various
flaws -- I'd invite Niklas to chime in with why this got squashed.
But anyway... If we were to solve that somehow, this would still leave
us with write operations capped at 8B and odd write pattern requirements
for ISM passthrough. Using a VFIO region to pass the operation directly
to the host kernel via a pinned page to overcome those limitations was
your idea actually :)
But OK, say we do need this for some obscure reason.
But this is not the only reason. There are additional reasons for
using this VFIO region:
1) ISM devices also don't support PCISTG instructions to certain
address spaces and PCISTB must be used regardless of operation
length. However the standard s390 PCI I/O path always uses PCISTG for
anything <=8B. Trying to determine whether a given I/O is intended for
an ISM device at that point in kernel code so as to use PCISTB instead
of PCISTG is the
OK, this is clear.
same problem as attempting to decide whether to use MIO vs non-MIO
instructions at that point.
humm, this is not exactly the same problem for me, but OK to choose to
handle it the same way.
The problem isn't the same BUT the information needed to solve the
problem is the same (for a given memory operation, knowing what
device/type it is intended for, which can therefore be used to determine
also whether it supports MIO or not).
2) It allows for much larger PCISTB operations (4K) than allowed via
the memory regions (loop of 8B operations).
OK
3) The above also has the added benefit of eliminating certain write
pattern requirements that are unique to ISM that would be introduced
if we split up the I/O into 8B chunks (if we can't write the whole
PCISTB in one go, ISM requires data written in a certain order for
some address spaces, or with certain bits on/off on the PCISTB
instruction to signify the state of the larger operation)
Yes, I suppose that the driver in the guest does it right and we need to
do the same.
I do not understand the relation between MSI-X and MIO.
Can you please explain?
There is not a relation between MSI-X and MIO really. Rather, this is
a case of the solution that is being offered here ONLY works for
devices that use MSI -- and ISM is a device that only supports MSI.
If you try to use this new VFIO region to pass I/O for an MSI-X
enabled device, the notifiers set up via vfio_msix_setup won't be
triggered because we are writing to the new VFIO region, not the
virtual bar regions that may have had notifiers setup as part of
vfio_msix_setup. This results in missing interrupts on MSI-X-enabled
vfio-pci devices.
These notifiers aren't a factor when the device is using MSI.
I find this strange but we do not need to discuss it.
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
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.
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'.
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.
