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

[Farhan Ali <alifm@linux.ibm.com>]

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@linux.ibm.com>
>> Reviewed-by: Niklas Schnelle <schnelle@linux.ibm.com>
>> Signed-off-by: Julian Ruess <julianr@linux.ibm.com>
>> ---
>>   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 ---