Re: [PATCH V4 vfio 8/9] vfio/pci: Expose vfio_pci_iowrite/read##size()

Yishai Hadas
2023-12-03T14:14:30Z

On 30/11/2023 21:20, Alex Williamson wrote:
> On Wed, 29 Nov 2023 16:37:45 +0200
> Yishai Hadas <yishaih@nvidia.com> wrote:
>
>> Expose vfio_pci_iowrite/read##size() to let it be used by drivers.
>>
>> This functionality is needed to enable direct access to some physical
>> BAR of the device with the proper locks/checks in place.
>>
>> The next patches from this series will use this functionality on a data
>> path flow when a direct access to the BAR is needed.
>>
>> Signed-off-by: Yishai Hadas <yishaih@nvidia.com>
>> ---
>> drivers/vfio/pci/vfio_pci_rdwr.c | 10 ++++++----
>> include/linux/vfio_pci_core.h | 19 +++++++++++++++++++
>> 2 files changed, 25 insertions(+), 4 deletions(-)
>
> I don't follow the inconsistency between this and the previous patch.
> Why did we move and rename the code to setup the barmap but we export
> the ioread/write functions in place? Thanks,
>
> Alex

The mount of code for barmap setup was quite small compared to the
ioread/write functions.

However, I agree, we can be consistent here and export in both cases the
functions in place as part of vfio_pci_rdwr.c which is already part of
vfio_pci_core.ko

I may also rename in current patch vfio_pci_iowrite/read<xxx> to have
the 'core' prefix as part of the functions names (i.e.
vfio_pci_core_iowrite/read<xxx>) to be consistent with other exported
core functions and adapt the callers to this name.

Makes sense ?

Yishai

>
>
>> diff --git a/drivers/vfio/pci/vfio_pci_rdwr.c b/drivers/vfio/pci/vfio_pci_rdwr.c
>> index 6f08b3ecbb89..817ec9a89123 100644
>> --- a/drivers/vfio/pci/vfio_pci_rdwr.c
>> +++ b/drivers/vfio/pci/vfio_pci_rdwr.c
>> @@ -38,7 +38,7 @@
>> #define vfio_iowrite8 iowrite8
>>
>> #define VFIO_IOWRITE(size) \
>> -static int vfio_pci_iowrite##size(struct vfio_pci_core_device *vdev, \
>> +int vfio_pci_iowrite##size(struct vfio_pci_core_device *vdev, \
>> bool test_mem, u##size val, void __iomem *io) \
>> { \
>> if (test_mem) { \
>> @@ -55,7 +55,8 @@ static int vfio_pci_iowrite##size(struct vfio_pci_core_device *vdev, \
>> up_read(&vdev->memory_lock); \
>> \
>> return 0; \
>> -}
>> +} \
>> +EXPORT_SYMBOL_GPL(vfio_pci_iowrite##size);
>>
>> VFIO_IOWRITE(8)
>> VFIO_IOWRITE(16)
>> @@ -65,7 +66,7 @@ VFIO_IOWRITE(64)
>> #endif
>>
>> #define VFIO_IOREAD(size) \
>> -static int vfio_pci_ioread##size(struct vfio_pci_core_device *vdev, \
>> +int vfio_pci_ioread##size(struct vfio_pci_core_device *vdev, \
>> bool test_mem, u##size *val, void __iomem *io) \
>> { \
>> if (test_mem) { \
>> @@ -82,7 +83,8 @@ static int vfio_pci_ioread##size(struct vfio_pci_core_device *vdev, \
>> up_read(&vdev->memory_lock); \
>> \
>> return 0; \
>> -}
>> +} \
>> +EXPORT_SYMBOL_GPL(vfio_pci_ioread##size);
>>
>> VFIO_IOREAD(8)
>> VFIO_IOREAD(16)
>> diff --git a/include/linux/vfio_pci_core.h b/include/linux/vfio_pci_core.h
>> index 67ac58e20e1d..22c915317788 100644
>> --- a/include/linux/vfio_pci_core.h
>> +++ b/include/linux/vfio_pci_core.h
>> @@ -131,4 +131,23 @@ int vfio_pci_core_setup_barmap(struct vfio_pci_core_device *vdev, int bar);
>> pci_ers_result_t vfio_pci_core_aer_err_detected(struct pci_dev *pdev,
>> pci_channel_state_t state);
>>
>> +#define VFIO_IOWRITE_DECLATION(size) \
>> +int vfio_pci_iowrite##size(struct vfio_pci_core_device *vdev, \
>> + bool test_mem, u##size val, void __iomem *io);
>> +
>> +VFIO_IOWRITE_DECLATION(8)
>> +VFIO_IOWRITE_DECLATION(16)
>> +VFIO_IOWRITE_DECLATION(32)
>> +#ifdef iowrite64
>> +VFIO_IOWRITE_DECLATION(64)
>> +#endif
>> +
>> +#define VFIO_IOREAD_DECLATION(size) \
>> +int vfio_pci_ioread##size(struct vfio_pci_core_device *vdev, \
>> + bool test_mem, u##size *val, void __iomem *io);
>> +
>> +VFIO_IOREAD_DECLATION(8)
>> +VFIO_IOREAD_DECLATION(16)
>> +VFIO_IOREAD_DECLATION(32)
>> +
>> #endif /* VFIO_PCI_CORE_H */
>