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

[Niklas Schnelle]

On Tue, 2026-03-10 at 13:20 -0600, Alex Williamson wrote:
> On Thu, 05 Mar 2026 13:20:14 +0100
> Julian Ruess <julianr@xxxxxxxxxxxxx> 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.
> > 
> > Reviewed-by: Niklas Schnelle <schnelle@xxxxxxxxxxxxx>
> > Signed-off-by: Julian Ruess <julianr@xxxxxxxxxxxxx>
> > ---
> > 
--- 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 __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))
> 
> Is there an endian issue here for 1/2/4 byte reads?  zpci_read_single()
> uses a u64 target for zpci_load(), but then casts the result into the
> destination buffer to place the low-order bytes.  AIUI, copy_to_user()
> would start from the high-order bytes of the u64.

Great find Alex, thank you! One should think that us s390 people would
be the ones extra careful with Endianess. I did a bit of digging how
this slipped through our review process and it seems we actually had it
right up to our internal v5 using a macro generated ism_read##size()
and using a temporary u##size. Then we messed up pulling the
copy_to_user() out of the individual ism_read() macros which then got
eliminated still without noticing the endianess issue. We actually also
don't need the sizes < 8 in the current user-space driver so didn't
notice in testing either. And as a side note LLM review also didn't
catch it. So unless vfio-pci requires reads < 8 bytes to work we'd opt
to just support the 8 byte case.

> 
> > +			return -EFAULT;
> > +		count -= length;
> > +		done += length;
> > +		off += length;
> > +		buf += length;
> > +	}
> > +	return done;
> > +}
--- snip ---
> > +
> > +static int ism_vfio_pci_probe(struct pci_dev *pdev,
> > +			      const struct pci_device_id *id)
> > +{
> > +	struct ism_vfio_pci_core_device *ivpcd;
> > +	struct zpci_dev *zdev = to_zpci(pdev);
> > +	int ret;
> > +
> > +	ivpcd = vfio_alloc_device(ism_vfio_pci_core_device, core_device.vdev,
> > +				  &pdev->dev, &ism_pci_ops);
> > +	if (IS_ERR(ivpcd))
> > +		return PTR_ERR(ivpcd);
> > +
> > +	store_block_cache = kmem_cache_create("store_block_cache",
> > +					      zdev->maxstbl, 0, 0, NULL);
> > +	if (!store_block_cache)
> > +		return -ENOMEM;
> 
> ivpcd is leaked here, we need a vfio_put_device().  Thanks,
> 
> Alex

Good find again. For completeness we also noticed another issue here in
internal review. The store_block_cache gets replaced and the old one
leaked when multiple devices are used with this driver. So we'll likely
go to a per-device kvmem_cache in v4.

> 
> > +
> > +	dev_set_drvdata(&pdev->dev, &ivpcd->core_device);
> > +	ret = vfio_pci_core_register_device(&ivpcd->core_device);
> > +	if (ret) {
> > +		kmem_cache_destroy(store_block_cache);
> > +		vfio_put_device(&ivpcd->core_device.vdev);
> > +	}
> > +
> > +	return ret;
> > +}
> >