From: liulongfang <liulongfang@huawei.com>
To: Alex Williamson <alex.williamson@redhat.com>,
Jason Gunthorpe <jgg@ziepe.ca>
Cc: Gerd Bayer <gbayer@linux.ibm.com>,
Niklas Schnelle <schnelle@linux.ibm.com>, <kvm@vger.kernel.org>,
<linux-s390@vger.kernel.org>, Ankit Agrawal <ankita@nvidia.com>,
Yishai Hadas <yishaih@nvidia.com>,
Halil Pasic <pasic@linux.ibm.com>,
Julian Ruess <julianr@linux.ibm.com>
Subject: Re: [PATCH v3 1/3] vfio/pci: Extract duplicated code into macro
Date: Tue, 30 Apr 2024 16:16:05 +0800 [thread overview]
Message-ID: <8c1cb908-1de8-eac6-7afa-7495b23a7fe9@huawei.com> (raw)
In-Reply-To: <20240429161103.655b4010.alex.williamson@redhat.com>
On 2024/4/30 6:11, Alex Williamson wrote:
> On Mon, 29 Apr 2024 17:09:10 -0300
> Jason Gunthorpe <jgg@ziepe.ca> 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@redhat.com>
>>> Signed-off-by: Gerd Bayer <gbayer@linux.ibm.com>
>>> ---
>>> 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
>>
>
>
> .
>
next prev parent reply other threads:[~2024-04-30 8:16 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-25 16:56 [PATCH v3 0/3] vfio/pci: Support 8-byte PCI loads and stores Gerd Bayer
2024-04-25 16:56 ` [PATCH v3 1/3] vfio/pci: Extract duplicated code into macro Gerd Bayer
2024-04-29 16:31 ` Alex Williamson
2024-05-17 14:22 ` Gerd Bayer
2024-04-29 20:09 ` Jason Gunthorpe
2024-04-29 22:11 ` Alex Williamson
2024-04-29 22:33 ` Jason Gunthorpe
2024-04-30 8:16 ` liulongfang [this message]
2024-05-17 10:47 ` Ramesh Thomas
2024-04-25 16:56 ` [PATCH v3 2/3] vfio/pci: Support 8-byte PCI loads and stores Gerd Bayer
2024-04-29 16:31 ` Alex Williamson
2024-05-17 10:29 ` Ramesh Thomas
2024-05-20 9:02 ` Tian, Kevin
2024-04-25 16:56 ` [PATCH v3 3/3] vfio/pci: Continue to refactor vfio_pci_core_do_io_rw Gerd Bayer
2024-04-28 6:59 ` Tian, Kevin
2024-04-29 16:32 ` Alex Williamson
2024-05-17 11:41 ` Ramesh Thomas
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=8c1cb908-1de8-eac6-7afa-7495b23a7fe9@huawei.com \
--to=liulongfang@huawei.com \
--cc=alex.williamson@redhat.com \
--cc=ankita@nvidia.com \
--cc=gbayer@linux.ibm.com \
--cc=jgg@ziepe.ca \
--cc=julianr@linux.ibm.com \
--cc=kvm@vger.kernel.org \
--cc=linux-s390@vger.kernel.org \
--cc=pasic@linux.ibm.com \
--cc=schnelle@linux.ibm.com \
--cc=yishaih@nvidia.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).