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

["Julian Ruess" <julianr@linux.ibm.com>]

On Fri Feb 27, 2026 at 4:52 PM CET, Alexandra Winter wrote:
>
>
> On 24.02.26 13:34, Julian Ruess wrote:
> [...]
>> diff --git a/drivers/vfio/pci/ism/main.c b/drivers/vfio/pci/ism/main.c
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..5f9674f6dd1d44888c4e1e416d05edfd89fd09fe
>> --- /dev/null
>> +++ b/drivers/vfio/pci/ism/main.c
>> @@ -0,0 +1,297 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * vfio-ISM driver for s390
>> + *
>> + * Copyright IBM Corp.
>> + */
>> +
>> +#include "../vfio_pci_priv.h"
>> +
>> +#define ISM_VFIO_PCI_OFFSET_SHIFT   48
>> +#define ISM_VFIO_PCI_OFFSET_TO_INDEX(off) (off >> ISM_VFIO_PCI_OFFSET_SHIFT)
>> +#define ISM_VFIO_PCI_INDEX_TO_OFFSET(index) ((u64)(index) << ISM_VFIO_PCI_OFFSET_SHIFT)
>> +#define ISM_VFIO_PCI_OFFSET_MASK (((u64)(1) << ISM_VFIO_PCI_OFFSET_SHIFT) - 1)
>> +
>> +struct ism_vfio_pci_core_device {
>> +	struct vfio_pci_core_device core_device;
>> +};
>> +
>> +static int ism_pci_open_device(struct vfio_device *core_vdev)
>> +{
>> +	struct ism_vfio_pci_core_device *ivdev;
>> +	struct vfio_pci_core_device *vdev;
>
>
> Why do you need 'struct ism_vfio_pci_core_device'?
> Unlike other vfio_pci variant drivers your struct only has a single member.
> I see it is used below as well, but couldn't you directly use 'struct vfio_pci_core_device'
> in all places?

Theoretically yes, but functions like vfio_alloc_device() expect this parent
struct.

>
>
>> +	int ret;
>> +
>> +	ivdev = container_of(core_vdev, struct ism_vfio_pci_core_device,
>> +			     core_device.vdev);
>> +	vdev = &ivdev->core_device;
>> +
>> +	ret = vfio_pci_core_enable(vdev);
>> +	if (ret)
>> +		return ret;
>> +
>> +	vfio_pci_core_finish_enable(vdev);
>> +	return 0;
>> +}
>> +
>> +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;
>
> As you know something like
>> +		if (count >= 8 && IS_ALIGNED(off, 8)) {
>> +			length = 8;
> 		} else {
> 			unsigned missing_bytes = 8 - (off % 8);
> 			length = min(count, missing_bytes);
> 		}
>
> would suffice to fullfill the requirements for pcilg to BARs.
> But your code works as well.

I think my version is easier to read and better aligned to common code.

>
>> +		req = ZPCI_CREATE_REQ(READ_ONCE(zdev->fh), bar, length);
>> +		/* use pcilg to prevent using MIO instructions */
>> +		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;
>
> I think you also need to check for off+count not crossing 4k

Thanks! Will introduce this check in the next version.

>
>
>> +	data = kzalloc(count, GFP_KERNEL);
>
>
> Where do you free 'data' ?

void *data __free(kfree) = NULL;

>
>> +	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);
>> +	if (ret)
>> +		return ret;
>> +	return count;
>> +}
>> +