[Public]
> From: Alex Williamson <alex.williamson@xxxxxxxxxx>
> Sent: Thursday, December 12, 2024 18:01
>
> On Thu, 12 Dec 2024 15:50:49 -0500
> Yunxiang Li <Yunxiang.Li@xxxxxxx> wrote:
>
> > In the next patch the logic for reading ROM will get more complicated,
> > so decouple the ROM path from the normal path. Also check that for ROM
> > write is not allowed.
>
> This is already enforced by the caller. Vague references to the next patch don't
> make a lot of sense once commits are in the tree, this should describe what you're
> preparing for.
>
> >
> > Signed-off-by: Yunxiang Li <Yunxiang.Li@xxxxxxx>
> > ---
> > drivers/vfio/pci/vfio_pci_rdwr.c | 47
> > ++++++++++++++++----------------
> > 1 file changed, 24 insertions(+), 23 deletions(-)
> >
> > diff --git a/drivers/vfio/pci/vfio_pci_rdwr.c
> > b/drivers/vfio/pci/vfio_pci_rdwr.c
> > index a1eeacad82120..4bed9fd5af50f 100644
> > --- a/drivers/vfio/pci/vfio_pci_rdwr.c
> > +++ b/drivers/vfio/pci/vfio_pci_rdwr.c
> > @@ -236,10 +236,9 @@ ssize_t vfio_pci_bar_rw(struct vfio_pci_core_device
> *vdev, char __user *buf,
> > struct pci_dev *pdev = vdev->pdev;
> > loff_t pos = *ppos & VFIO_PCI_OFFSET_MASK;
> > int bar = VFIO_PCI_OFFSET_TO_INDEX(*ppos);
> > - size_t x_start = 0, x_end = 0;
> > + size_t x_start, x_end;
> > resource_size_t end;
> > void __iomem *io;
> > - struct resource *res = &vdev->pdev->resource[bar];
> > ssize_t done;
> >
> > if (pci_resource_start(pdev, bar))
> > @@ -253,41 +252,43 @@ ssize_t vfio_pci_bar_rw(struct vfio_pci_core_device
> *vdev, char __user *buf,
> > count = min(count, (size_t)(end - pos));
> >
> > if (bar == PCI_ROM_RESOURCE) {
> > + if (iswrite)
> > + return -EINVAL;
> > /*
> > * The ROM can fill less space than the BAR, so we start the
> > * excluded range at the end of the actual ROM. This makes
> > * filling large ROM BARs much faster.
> > */
> > io = pci_map_rom(pdev, &x_start);
> > - if (!io) {
> > - done = -ENOMEM;
> > - goto out;
> > - }
> > + if (!io)
> > + return -ENOMEM;
> > x_end = end;
> > +
> > + done = vfio_pci_core_do_io_rw(vdev, 1, io, buf, pos,
> > + count, x_start, x_end, 0);
> > +
> > + pci_unmap_rom(pdev, io);
> > } else {
> > - int ret = vfio_pci_core_setup_barmap(vdev, bar);
> > - if (ret) {
> > - done = ret;
> > - goto out;
> > - }
> > + done = vfio_pci_core_setup_barmap(vdev, bar);
> > + if (done)
> > + return done;
> >
> > io = vdev->barmap[bar];
> > - }
> >
> > - if (bar == vdev->msix_bar) {
> > - x_start = vdev->msix_offset;
> > - x_end = vdev->msix_offset + vdev->msix_size;
> > - }
> > + if (bar == vdev->msix_bar) {
> > + x_start = vdev->msix_offset;
> > + x_end = vdev->msix_offset + vdev->msix_size;
> > + } else {
> > + x_start = 0;
> > + x_end = 0;
> > + }
>
> There's a lot of semantic preference noise that obscures what you're actually trying
> to accomplish here, effectively this has only refactored the code to have separate
> calls to ..do_io_rw() for the ROM vs other case and therefore pushed the unmap
> into the ROM case, introducing various new exit paths.
Yes, the primary goal was to move the resource allocation and cleanup into one clause, otherwise there would be duplicated nested if clauses at the end just for cleanup. I didn't realize the caller was checking for write like that and then calling into this. It seems like it make more sense to have different helper functions, something like this. The two code path don't really have much in common other than they call do_io_rw at the end.
case VFIO_PCI_ROM_REGION_INDEX:
if (iswrite)
return -EINVAL;
return vfio_pci_rom_read(vdev, buf, count, ppos);
case VFIO_PCI_BAR0_REGION_INDEX ... VFIO_PCI_BAR5_REGION_INDEX:
return vfio_pci_bar_rw(vdev, buf, count, ppos, iswrite);
> >
> > - done = vfio_pci_core_do_io_rw(vdev, res->flags & IORESOURCE_MEM,
> io, buf, pos,
> > + done = vfio_pci_core_do_io_rw(vdev, pci_resource_flags(pdev, bar)
> &
> > +IORESOURCE_MEM, io, buf, pos,
>
> The line is too long already, now it's indented further and the wrapping needs to be
> adjusted.
>
> > count, x_start, x_end, iswrite);
> > -
> > - if (done >= 0)
> > - *ppos += done;
> > -
> > - if (bar == PCI_ROM_RESOURCE)
> > - pci_unmap_rom(pdev, io);
> > + }
> > out:
>
> Both goto's to this label were removed above, none added. Thanks,
>
> Alex
>
> > + if (done > 0)
> > + *ppos += done;
> > return done;
> > }
> >
