| Message ID | 20240425165604.899447-4-gbayer@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vfio/pci: Support 8-byte PCI loads and stores | expand |
> From: Gerd Bayer <gbayer@linux.ibm.com> > Sent: Friday, April 26, 2024 12:56 AM > > @@ -131,6 +131,20 @@ VFIO_IORDWR(32) > VFIO_IORDWR(64) > #endif > > +static int fill_size(size_t fillable, loff_t off) > +{ > + unsigned int 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; this is unreachable with "fill_size >= 0" in the loop. shouldn't it be: #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)) break; } return fill_size; > > + 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; the check on 'ret' can be moved out of the switch to be generic.
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) {
On 4/25/2024 9:56 AM, Gerd Bayer 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) > #endif > > +static int fill_size(size_t fillable, loff_t off) > +{ > + unsigned int 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) { 0 anyway reaches default case, so loop condition can be "fill_size > 0" or at the start of function return 0 or -1 if fillable <= 0 > +#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; This check and returning is common for all cases except default. Maybe ret can be initialized to 0 before the switch block and do the check and return after the switch block. > + break; > > - } else > #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: > /* Fill reads with -1, drop writes */ > filled = min(count, (size_t)(x_end - off)); > if (!iswrite) {
On Mon, 2024-04-29 at 10:32 -0600, Alex Williamson wrote: > 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) { > > Well, overall this sounds like it creates more headaches than it tries to solve - and that is a strong hint to not do it. I'll drop this further refactoring in the next version. Thanks, Gerd
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) #endif +static int fill_size(size_t fillable, loff_t off) +{ + unsigned int 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 #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: /* Fill reads with -1, drop writes */ filled = min(count, (size_t)(x_end - off)); if (!iswrite) {
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(-)