[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Matthew Rosato
Subject: Re: [PATCH 0/8] s390x/pci: Fixing s390 vfio-pci ISM support
Date: Wed, 20 Jan 2021 10:59:53 -0500
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.6.0

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:

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
- 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.


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.

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 same problem as attempting to decide whether to use MIO vs non-MIO instructions at that point. 2) It allows for much larger PCISTB operations (4K) than allowed via the memory regions (loop of 8B operations). 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)

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.

reply via email to

[Prev in Thread] Current Thread [Next in Thread]