Re: [PATCH v2 1/2] vfio/pci: fall back to platform ROM when pci_map_rom() fails

["Michael S. Tsirkin" <mst@redhat.com>]

On Tue, Nov 10, 2026 at 04:52:21PM -0700, Alex Williamson wrote:
> On Wed, 21 Oct 2026 04:20:32 -0400
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > When pci_resource_start() returns non-zero but pci_map_rom() fails
> > (ROM BAR exists but content is unreadable), the pdev->rom fallback
> > is unreachable because it's behind an else-if that only triggers
> > when pci_resource_start() is zero.
> > 
> > Try pdev->rom whenever the ROM BAR didn't produce a mapping, in both
> > the region info ioctl and the read path.
> 
> Here's the crazy thing, users actually update firmware through
> vfio-pci.  Therefore, what's an invalid ROM image on one check, may be
> a valid ROM image next round.  If we fall through to a different ROM
> provider, we might actually be presenting a changing view of the
> device.


This is out of spec anyway. The spec is pretty clear that _ROM if
present takes precedence over the ROM BAR.  I decided not to change that
if ROM BAR is valid to reduce the chance of breaking a working setup.

From ACPI 6.3, Section A.3.3 "_ROM (Get ROM Data)", page 1123:

	If _ROM is present, it is preferred over the image read through the
	standard PCI interface, in order to allow platform runtime firmware to
	provide re-configured ROM data via the method.


Would you like me to simply implement the spec? Use _ROM if there
even if pci rom bar is valid?


> I believe this is why we intentionally always report the physical ROM
> BAR when present and mapped, regardless of the contents.

No such setups should have _ROM, since rombar would then be ignored.

> QEMU, of course, has the ability to take the ROM as a file and override
> whatever is provided through the kernel interface.  This is how I had
> previously made use of a secondary GPU on a laptop, dumping the ACPI
> ROM to a file to provide directly to QEMU.

First we need a way to get the ROM from _ROM.

> 
> Does your GPU support a ROM BAR?  The v1 patch didn't have this and
> would have exposed the physical RAM BAR if present, so I'm guessing not.
> Thanks,
> 
> Alex

It has a bar but it is not accessible. _ROM overrides that, as per spec, and this
appears to be how nvidia drivers use it and expect to use it.


>  
> > Fixes: e021e6cbfb5a ("vfio/pci: Expose setup ROM at ROM bar when needed")
> > Cc: Yunxiang Li <Yunxiang.Li@amd.com>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> > 
> > Notes (changelog):
> >     v1->v2:
> >         bound reads by the ROM image rather than the BAR once the
> >         pdev->rom fallback is selected, so reads match the reported
> >         region size (a pread spanning the image used to fail with
> >         -EINVAL past the BAR size)
> >         add Fixes: tag
> > 
> >  drivers/vfio/pci/vfio_pci_core.c |  4 +++-
> >  drivers/vfio/pci/vfio_pci_rdwr.c | 32 ++++++++++++++++++++++++--------
> >  2 files changed, 27 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
> > index 050e7542952e..630474d03e3b 100644
> > --- a/drivers/vfio/pci/vfio_pci_core.c
> > +++ b/drivers/vfio/pci/vfio_pci_core.c
> > @@ -1128,7 +1128,9 @@ int vfio_pci_ioctl_get_region_info(struct vfio_device *core_vdev,
> >  				pci_unmap_rom(pdev, io);
> >  			}
> >  			vfio_pci_memory_unlock_and_restore(vdev, cmd);
> > -		} else if (pdev->rom && pdev->romlen) {
> > +		}
> > +
> > +		if (!info->size && pdev->rom && pdev->romlen) {
> >  			info->flags = VFIO_REGION_INFO_FLAG_READ;
> >  			/* Report BAR size as power of two. */
> >  			info->size = roundup_pow_of_two(pdev->romlen);
> > diff --git a/drivers/vfio/pci/vfio_pci_rdwr.c b/drivers/vfio/pci/vfio_pci_rdwr.c
> > index 3bfbb879a005..6e47d529cbb9 100644
> > --- a/drivers/vfio/pci/vfio_pci_rdwr.c
> > +++ b/drivers/vfio/pci/vfio_pci_rdwr.c
> > @@ -219,7 +219,8 @@ ssize_t vfio_pci_bar_rw(struct vfio_pci_core_device *vdev, char __user *buf,
> >  	int bar = VFIO_PCI_OFFSET_TO_INDEX(*ppos);
> >  	size_t x_start = 0, x_end = 0;
> >  	resource_size_t end;
> > -	void __iomem *io;
> > +	void __iomem *io = NULL;
> > +	bool rom_is_mapped = false;
> >  	struct resource *res = &vdev->pdev->resource[bar];
> >  	ssize_t done;
> >  	enum vfio_pci_io_width max_width = VFIO_PCI_IO_WIDTH_8;
> > @@ -231,11 +232,6 @@ ssize_t vfio_pci_bar_rw(struct vfio_pci_core_device *vdev, char __user *buf,
> >  	else
> >  		return -EINVAL;
> >  
> > -	if (pos >= end)
> > -		return -EINVAL;
> > -
> > -	count = min(count, (size_t)(end - pos));
> > -
> >  	if (bar == PCI_ROM_RESOURCE) {
> >  		/*
> >  		 * The ROM can fill less space than the BAR, so we start the
> > @@ -244,10 +240,30 @@ ssize_t vfio_pci_bar_rw(struct vfio_pci_core_device *vdev, char __user *buf,
> >  		 */
> >  		if (pci_resource_start(pdev, bar)) {
> >  			io = pci_map_rom(pdev, &x_start);
> > -		} else {
> > +			if (io)
> > +				rom_is_mapped = true;
> > +		}
> > +
> > +		/*
> > +		 * The BAR can also exist but be unreadable, e.g. on a GPU
> > +		 * that was never POSTed.  Fall back to the ROM image stashed
> > +		 * by the boot loader; the region then follows the image size
> > +		 * rather than the BAR, so bound the access only after the
> > +		 * source of the ROM content is known.
> > +		 */
> > +		if (!io && pdev->rom && pdev->romlen) {
> > +			end = roundup_pow_of_two(pdev->romlen);
> >  			io = ioremap(pdev->rom, pdev->romlen);
> >  			x_start = pdev->romlen;
> >  		}
> > +	}
> > +
> > +	if (pos >= end)
> > +		return -EINVAL;
> > +
> > +	count = min(count, (size_t)(end - pos));
> > +
> > +	if (bar == PCI_ROM_RESOURCE) {
> >  		if (!io)
> >  			return -ENOMEM;
> >  		x_end = end;
> > @@ -283,7 +299,7 @@ ssize_t vfio_pci_bar_rw(struct vfio_pci_core_device *vdev, char __user *buf,
> >  		*ppos += done;
> >  
> >  	if (bar == PCI_ROM_RESOURCE) {
> > -		if (pci_resource_start(pdev, bar))
> > +		if (rom_is_mapped)
> >  			pci_unmap_rom(pdev, io);
> >  		else
> >  			iounmap(io);