On 2024/4/30 6:11, Alex Williamson wrote:
> On Mon, 29 Apr 2024 17:09:10 -0300
> Jason Gunthorpe <jgg@xxxxxxxx> wrote:
>
>> On Thu, Apr 25, 2024 at 06:56:02PM +0200, Gerd Bayer wrote:
>>> vfio_pci_core_do_io_rw() repeats the same code for multiple access
>>> widths. Factor this out into a macro
>>>
>>> Suggested-by: Alex Williamson <alex.williamson@xxxxxxxxxx>
>>> Signed-off-by: Gerd Bayer <gbayer@xxxxxxxxxxxxx>
>>> ---
>>> drivers/vfio/pci/vfio_pci_rdwr.c | 106 ++++++++++++++-----------------
>>> 1 file changed, 46 insertions(+), 60 deletions(-)
>>>
>>> diff --git a/drivers/vfio/pci/vfio_pci_rdwr.c b/drivers/vfio/pci/vfio_pci_rdwr.c
>>> index 03b8f7ada1ac..3335f1b868b1 100644
>>> --- a/drivers/vfio/pci/vfio_pci_rdwr.c
>>> +++ b/drivers/vfio/pci/vfio_pci_rdwr.c
>>> @@ -90,6 +90,40 @@ VFIO_IOREAD(8)
>>> VFIO_IOREAD(16)
>>> VFIO_IOREAD(32)
>>>
>>> +#define VFIO_IORDWR(size) \
>>> +static int vfio_pci_core_iordwr##size(struct vfio_pci_core_device *vdev,\
>>> + bool iswrite, bool test_mem, \
>>> + void __iomem *io, char __user *buf, \
>>> + loff_t off, size_t *filled) \
>>> +{ \
>>> + u##size val; \
>>> + int ret; \
>>> + \
>>> + if (iswrite) { \
>>> + if (copy_from_user(&val, buf, sizeof(val))) \
>>> + return -EFAULT; \
>>> + \
>>> + ret = vfio_pci_core_iowrite##size(vdev, test_mem, \
>>> + val, io + off); \
>>> + if (ret) \
>>> + return ret; \
>>> + } else { \
>>> + ret = vfio_pci_core_ioread##size(vdev, test_mem, \
>>> + &val, io + off); \
>>> + if (ret) \
>>> + return ret; \
>>> + \
>>> + if (copy_to_user(buf, &val, sizeof(val))) \
>>> + return -EFAULT; \
>>> + } \
>>> + \
>>> + *filled = sizeof(val); \
>>> + return 0; \
>>> +} \
>>> +
>>> +VFIO_IORDWR(8)
>>> +VFIO_IORDWR(16)
>>> +VFIO_IORDWR(32)
>>
>> I'd suggest to try writing this without so many macros.
>>
>> This isn't very performance optimal already, we take a lock on every
>> iteration, so there isn't much point in inlining multiple copies of
>> everything to save an branch.
>
> These macros are to reduce duplicate code blocks and the errors that
Although simple and straightforward writing will result in more lines of code.
But it's not easy to squeeze in "extra" code.
The backdoor of "XZ Utils" is implanted through code complication.
Thanks.
Longfang.
> typically come from such duplication, as well as to provide type safe
> functions in the spirit of the ioread# and iowrite# helpers. It really
> has nothing to do with, nor is it remotely effective at saving a branch.
> Thanks,
>
> Alex
>
>> Push the sizing switch down to the bottom, start with a function like:
>>
>> static void __iowrite(const void *val, void __iomem *io, size_t len)
>> {
>> switch (len) {
>> case 8: {
>> #ifdef iowrite64 // NOTE this doesn't seem to work on x86?
>> if (IS_ENABLED(CONFIG_CPU_BIG_ENDIAN))
>> return iowrite64be(*(const u64 *)val, io);
>> return iowrite64(*(const u64 *)val, io);
>> #else
>> if (IS_ENABLED(CONFIG_CPU_BIG_ENDIAN)) {
>> iowrite32be(*(const u32 *)val, io);
>> iowrite32be(*(const u32 *)(val + 4), io + 4);
>> } else {
>> iowrite32(*(const u32 *)val, io);
>> iowrite32(*(const u32 *)(val + 4), io + 4);
>> }
>> return;
>> #endif
>> }
>>
>> case 4:
>> if (IS_ENABLED(CONFIG_CPU_BIG_ENDIAN))
>> return iowrite32be(*(const u32 *)val, io);
>> return iowrite32(*(const u32 *)val, io);
>> case 2:
>> if (IS_ENABLED(CONFIG_CPU_BIG_ENDIAN))
>> return iowrite16be(*(const u16 *)val, io);
>> return iowrite16(*(const u16 *)val, io);
>>
>> case 1:
>> return iowrite8(*(const u8 *)val, io);
>> }
>> }
>>
>> And then wrap it with the copy and the lock:
>>
>> static int do_iordwr(struct vfio_pci_core_device *vdev, bool test_mem,
>> const void __user *buf, void __iomem *io, size_t len,
>> bool iswrite)
>> {
>> u64 val;
>>
>> if (iswrite && copy_from_user(&val, buf, len))
>> return -EFAULT;
>>
>> if (test_mem) {
>> down_read(&vdev->memory_lock);
>> if (!__vfio_pci_memory_enabled(vdev)) {
>> up_read(&vdev->memory_lock);
>> return -EIO;
>> }
>> }
>>
>> if (iswrite)
>> __iowrite(&val, io, len);
>> else
>> __ioread(&val, io, len);
>>
>> if (test_mem)
>> up_read(&vdev->memory_lock);
>>
>> if (!iswrite && copy_to_user(buf, &val, len))
>> return -EFAULT;
>>
>> return 0;
>> }
>>
>> And then the loop can be simple:
>>
>> if (fillable) {
>> filled = num_bytes(fillable, off);
>> ret = do_iordwr(vdev, test_mem, buf, io + off, filled,
>> iswrite);
>> if (ret)
>> return ret;
>> } else {
>> filled = min(count, (size_t)(x_end - off));
>> /* Fill reads with -1, drop writes */
>> ret = fill_err(buf, filled);
>> if (ret)
>> return ret;
>> }
>>
>> Jason
>>
>
>
> .
>
