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

[Farhan Ali]

On 3/26/2026 6:03 AM, Niklas Schnelle wrote:
> On Wed, 2026-03-25 at 14:31 +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.
> >
> > Reviewed-by: Alexandra Winter <wintera@xxxxxxxxxxxxx>
> > Reviewed-by: Niklas Schnelle <schnelle@xxxxxxxxxxxxx>
> > Signed-off-by: Julian Ruess <julianr@xxxxxxxxxxxxx>
> > ---
> >   drivers/vfio/pci/Kconfig      |   2 +
> >   drivers/vfio/pci/Makefile     |   2 +
> >   drivers/vfio/pci/ism/Kconfig  |  10 ++
> >   drivers/vfio/pci/ism/Makefile |   3 +
> >   drivers/vfio/pci/ism/main.c   | 408 ++++++++++++++++++++++++++++++++++++++++++
> >   5 files changed, 425 insertions(+)
> --- snip ---
> > +
> > +static int ism_vfio_pci_ioctl_get_region_info(struct vfio_device *core_vdev,
> > +					      struct vfio_region_info *info,
> > +					      struct vfio_info_cap *caps)
> > +{
> > +	struct vfio_pci_core_device *vdev =
> > +		container_of(core_vdev, struct vfio_pci_core_device, vdev);
> > +	struct pci_dev *pdev = vdev->pdev;
> > +
> > +	switch (info->index) {
> > +	case VFIO_PCI_CONFIG_REGION_INDEX:
> > +		info->offset = ISM_VFIO_PCI_INDEX_TO_OFFSET(info->index);
> > +		info->size = pdev->cfg_size;
> > +		info->flags = VFIO_REGION_INFO_FLAG_READ |
> > +			      VFIO_REGION_INFO_FLAG_WRITE;
> > +		break;
> > +	case VFIO_PCI_BAR0_REGION_INDEX ... VFIO_PCI_BAR5_REGION_INDEX:
> > +		info->offset = ISM_VFIO_PCI_INDEX_TO_OFFSET(info->index);
> > +		info->size = pci_resource_len(pdev, info->index);
> > +		if (!info->size) {
> > +			info->flags = 0;
> > +			break;
> > +		}
> > +		info->flags = VFIO_REGION_INFO_FLAG_READ |
> > +			      VFIO_REGION_INFO_FLAG_WRITE;
> > +		break;
> > +	default:
> > +		info->offset = 0;
> > +		info->size = 0;
> > +		info->flags = 0;
> > +		return -EINVAL;
> Thinking more about this, the above default handling actually breaks
> additional regions such as the one added by Farhan for the error
> events. I think this needs to instead call the generic
> vfio_pci_ioctl_get_region_info() for other regions.

Note for error events we are using a VFIO device feature and not a 
region. At this point we actually don't have additional regions. I do 
agree that if we were to add a zpci specific region we would need to 
update here and ism_vfio_pci_rw() to allow region read/write ops. This 
ISM driver would also need to support all the possible callbacks in 
struct vfio_pci_regops.

Given that we don't have any additional regions yet, I am conflicted if 
it makes sense to add all the code to support additional regions without 
a good way to test it. Maybe its something we can defer when if we add 
additional regions? Thanks

Farhan


> > +	}
> > +	return 0;
> > +}
> --- snip ---