From: Alex Williamson <alex.williamson@redhat.com>
To: Gerd Bayer <gbayer@linux.ibm.com>
Cc: Jason Gunthorpe <jgg@ziepe.ca>,
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 3/3] vfio/pci: Continue to refactor vfio_pci_core_do_io_rw
Date: Mon, 29 Apr 2024 10:32:01 -0600 [thread overview]
Message-ID: <20240429103201.7e07e502.alex.williamson@redhat.com> (raw)
In-Reply-To: <20240425165604.899447-4-gbayer@linux.ibm.com>
On Thu, 25 Apr 2024 18:56:04 +0200
Gerd Bayer <gbayer@linux.ibm.com> wrote:
> Convert if-elseif-chain into switch-case.
> Separate out and generalize the code from the if-clauses so the result
> can be used in the switch statement.
>
> Signed-off-by: Gerd Bayer <gbayer@linux.ibm.com>
> ---
> drivers/vfio/pci/vfio_pci_rdwr.c | 30 ++++++++++++++++++++++++------
> 1 file changed, 24 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/vfio/pci/vfio_pci_rdwr.c b/drivers/vfio/pci/vfio_pci_rdwr.c
> index 8ed06edaee23..634c00b03c71 100644
> --- a/drivers/vfio/pci/vfio_pci_rdwr.c
> +++ b/drivers/vfio/pci/vfio_pci_rdwr.c
> @@ -131,6 +131,20 @@ VFIO_IORDWR(32)
> VFIO_IORDWR(64)
#define MAX_FILL_SIZE 8
#else
#define MAX_FILL_SIZE 4
> #endif
>
> +static int fill_size(size_t fillable, loff_t off)
> +{
> + unsigned int fill_size;
unsigned int fill_size = MAX_FILL_SIZE;
> +#if defined(ioread64) && defined(iowrite64)
> + for (fill_size = 8; fill_size >= 0; fill_size /= 2) {
> +#else
> + for (fill_size = 4; fill_size >= 0; fill_size /= 2) {
> +#endif /* defined(ioread64) && defined(iowrite64) */
> + if (fillable >= fill_size && !(off % fill_size))
> + return fill_size;
> + }
> + return -1;
> +}
> +
> /*
> * Read or write from an __iomem region (MMIO or I/O port) with an excluded
> * range which is inaccessible. The excluded range drops writes and fills
> @@ -155,34 +169,38 @@ ssize_t vfio_pci_core_do_io_rw(struct vfio_pci_core_device *vdev, bool test_mem,
> else
> fillable = 0;
>
> + switch (fill_size(fillable, off)) {
> #if defined(ioread64) && defined(iowrite64)
> - if (fillable >= 8 && !(off % 8)) {
> + case 8:
> ret = vfio_pci_core_iordwr64(vdev, iswrite, test_mem,
> io, buf, off, &filled);
> if (ret)
> return ret;
> + break;
>
> - } else
AFAICT, avoiding this dangling else within the #ifdef is really the
only tangible advantage of conversion to a switch statement. Getting
rid of that alone while keeping, and actually increasing, the inline
ifdefs in the code doesn't seem worthwhile to me. I'd probably only go
this route if we could make vfio_pci_iordwr64() stubbed as a BUG_ON
when we don't have the ioread64 and iowrite64 accessors, in which case
the switch helper would never return 8 and the function would be
unreachable.
> #endif /* defined(ioread64) && defined(iowrite64) */
> - if (fillable >= 4 && !(off % 4)) {
> + case 4:
> ret = vfio_pci_core_iordwr32(vdev, iswrite, test_mem,
> io, buf, off, &filled);
> if (ret)
> return ret;
> + break;
>
> - } else if (fillable >= 2 && !(off % 2)) {
> + case 2:
> ret = vfio_pci_core_iordwr16(vdev, iswrite, test_mem,
> io, buf, off, &filled);
> if (ret)
> return ret;
> + break;
>
> - } else if (fillable) {
> + case 1:
> ret = vfio_pci_core_iordwr8(vdev, iswrite, test_mem,
> io, buf, off, &filled);
> if (ret)
> return ret;
> + break;
>
> - } else {
> + default:
This condition also seems a little more obfuscated without being
preceded by the 'if (fillable)' test, which might warrant handling
separate from the switch if we continue to pursue the switch
conversion. Thanks,
Alex
> /* Fill reads with -1, drop writes */
> filled = min(count, (size_t)(x_end - off));
> if (!iswrite) {
next prev parent reply other threads:[~2024-04-29 16:32 UTC|newest]
Thread overview: 25+ 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-05-21 15:47 ` Gerd Bayer
2024-04-30 8:16 ` liulongfang
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-21 15:50 ` Gerd Bayer
2024-05-17 10:29 ` Ramesh Thomas
2024-05-20 9:02 ` Tian, Kevin
2024-05-23 0:11 ` Ramesh Thomas
2024-05-23 21:52 ` Ramesh Thomas
2024-05-21 16:40 ` Gerd Bayer
2024-05-22 13:48 ` Jason Gunthorpe
2024-05-22 23:57 ` Ramesh Thomas
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 [this message]
2024-05-21 16:43 ` Gerd Bayer
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=20240429103201.7e07e502.alex.williamson@redhat.com \
--to=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).