Re: [PATCH v2 2/3] vfio/ism: Implement vfio_pci driver for ISM devices

[Niklas Schnelle]

On Tue, 2026-02-24 at 13:34 +0100, Julian Ruess wrote:
> Add a vfio_pci variant driver for the s390-specific Internal Shared
> Memory (ISM) devices used for inter-VM communication.
> 
> This enables the development of vfio-pci-based user space drivers for
> ISM devices.
> 
> On s390, kernel primitives such as ioread() and iowrite() are switched
> over from function handle based PCI load/stores instructions to PCI
> memory-I/O (MIO) loads/stores when these are available and not
> explicitly disabled. Since these instructions cannot be used with ISM
> devices, ensure that classic function handle-based PCI instructions are
> used instead.
> 
> The driver is still required even when MIO instructions are disabled, as
> the ISM device relies on the PCI store block (PCISTB) instruction to
> perform write operations.
> 
> Stores are not fragmented, therefore one ioctl corresponds to exactly
> one PCISTB instruction. User space must ensure to not write more than
> 4096 bytes at once to an ISM BAR which is the maximum payload of the
> PCISTB instruction.
> 
> Signed-off-by: Julian Ruess <julianr@xxxxxxxxxxxxx>
> ---
>  drivers/vfio/pci/Kconfig      |   2 +
>  drivers/vfio/pci/Makefile     |   2 +
>  drivers/vfio/pci/ism/Kconfig  |  11 ++
>  drivers/vfio/pci/ism/Makefile |   3 +
>  drivers/vfio/pci/ism/main.c   | 297 ++++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 315 insertions(+)
> 
--- snip ---
> +
> +static ssize_t ism_vfio_pci_do_io_r(struct vfio_pci_core_device *vdev,
> +				    char __user *buf, loff_t off, size_t count,
> +				    int bar)
> +{
> +	struct zpci_dev *zdev = to_zpci(vdev->pdev);
> +	ssize_t ret, done = 0;
> +	u64 req, length, tmp;
> +
> +	while (count) {
> +		if (count >= 8 && IS_ALIGNED(off, 8))
> +			length = 8;
> +		else if (count >= 4 && IS_ALIGNED(off, 4))
> +			length = 4;
> +		else if (count >= 2 && IS_ALIGNED(off, 2))
> +			length = 2;
> +		else
> +			length = 1;
> +		req = ZPCI_CREATE_REQ(READ_ONCE(zdev->fh), bar, length);
> +		/* use pcilg to prevent using MIO instructions */

I think this comment could be improved but it's not wrong either so no
strong opinion. Maybe something like:

/*
 * Use __zpci_load() to bypass automatic use of PCI MIO instructions
 * which are not supported on ISM devices
 */


> +		ret = __zpci_load(&tmp, req, off);
> +		if (ret)
> +			return ret;
> +		if (copy_to_user(buf, &tmp, length))
> +			return -EFAULT;
> +		count -= length;
> +		done += length;
> +		off += length;
> +		buf += length;
> +	}
> +	return done;
> +}
> +
> +static ssize_t ism_vfio_pci_do_io_w(struct vfio_pci_core_device *vdev,
> +				    char __user *buf, loff_t off, size_t count,
> +				    int bar)
> +{
> +	struct zpci_dev *zdev = to_zpci(vdev->pdev);
> +	void *data __free(kfree) = NULL;
> +	ssize_t ret;
> +	u64 req;
> +
> +	if (count > zdev->maxstbl)
> +		return -EINVAL;
> +	data = kzalloc(count, GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +	if (copy_from_user(data, buf, count))
> +		return -EFAULT;
> +	req = ZPCI_CREATE_REQ(READ_ONCE(zdev->fh), bar, count);
> +	ret = __zpci_store_block(data, req, off);

Note for the interested reader. ISM devices have relaxed alignment
rules on PCI Store Block so if you compare with other PCI Store Block
uses e.g. in memcpy_toio() don't be alarmed that this doesn't check the
alignment in the same way, or at all. Also we do still get error
returns if the access failed.

> +	if (ret)
> +		return ret;
> +	return count;
> +}
> 
--- snip ---
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("vfio-pci variant driver for the IBM Internal Shared Memory (ISM) device");
> +MODULE_AUTHOR("IBM Corporation");

I'm a bit biased here since I've been quite involved in your work and
the design decisions leading to this variant driver, but this looks
good to me. And in particular the low level PCI accesses  with ISM's
quirks in mind look fine to me.

Feel free to add:

Reviewed-by: Niklas Schnelle <schnelle@xxxxxxxxxxxxx>

Thanks,
Niklas