Re: [PATCH v2] video: screen_info: Relocate framebuffers behind PCI bridges

[Bjorn Helgaas <helgaas@kernel.org>]

On Mon, May 05, 2025 at 03:05:34PM +0200, Thomas Zimmermann wrote:
> Am 22.04.25 um 23:47 schrieb Bjorn Helgaas:
> > On Tue, Apr 22, 2025 at 09:49:57AM +0200, Thomas Zimmermann wrote:
> > > Apply bridge window offsets to screen_info framebuffers during
> > > relocation. Fixes invalid access to I/O memory.
> > > 
> > > Resources behind a PCI bridge can be located at a certain offset
> > > in the kernel's I/O range. The framebuffer memory range stored in
> > > screen_info refers to the offset as seen during boot (essentialy 0).
> > > During boot up, the kernel may assign a different memory offset to
> > > the bridge device and thereby relocating the framebuffer address of
> > > the PCI graphics device as seen by the kernel. The information in
> > > screen_info must be updated as well.
> > I can't see the bug report below, so I'm not sure what's happening
> > here.  Apparently the platform is one where PCI bus addresses are not
> > identical to CPU physical addresses.  On such platforms, the PCI host
> > bridge applies an offset between CPU and PCI addresses.  There are
> > several systems like that, but I'm not aware of any that change that
> > CPU->PCI bus address offset at runtime.
> > 
> > So I suspect the problem is not that the kernel has assigned a
> > different offset.  I think it's more likely that the hardware or
> > firmware has determined the offset before the kernel starts, and this
> > code just doesn't account for that.
> 
> Right, that's what I'm trying to say. I guess my explanation simply isn't
> clear.

Yeah, the part about the "kernel assigning a different offset" is a
bit misleading because the kernel doesn't actually assign or *change*
that offset; it only *discovers* the offset, typically from an ACPI
_TRA method or from device tree.

> > > Closes: https://bugzilla.suse.com/show_bug.cgi?id=1240696
> > This bug isn't public.  Can it be made public?  Or even better, a
> > report at https://bugzilla.kernel.org?
> 
> Try again, please. I've updated the settings of this bug report.

Works now, thanks!

> > > @@ -69,10 +69,21 @@ static void screen_info_fixup_lfb(struct pci_dev *pdev)
> > >   	for (i = 0; i < numres; ++i) {
> > >   		struct resource *r = &res[i];
> > > +		struct pci_bus_region bus_region = {
> > > +			.start = r->start,
> > > +			.end = r->end,
> > > +		};
> >
> > screen_info_resources() above fills in "struct resource res[]", but
> > that's not quite right.  A struct resource contains CPU addresses, and
> > screen_info_resources() fills in PCI bus addresses (0xa0000, etc).
> > 
> > struct pci_bus_region is meant to hold PCI bus addresses, so this
> > assignment gets them back where they should be.
> > 
> > >   		const struct resource *pr;
> > >   		if (!(r->flags & IORESOURCE_MEM))
> > >   			continue;
> > > +
> > > +		/*
> > > +		 * Translate the address to resource if the framebuffer
> > > +		 * is behind a PCI bridge.
> > > +		 */
> > > +		pcibios_bus_to_resource(pdev->bus, r, &bus_region);
> >
> > And this converts the PCI bus addresses to CPU addresses, so this
> > makes sense.
> > 
> > The comment might be a little misleading, though.  When PCI bus
> > addresses are different from CPU addresses, it's because the PCI host
> > bridge has applied an offset.  This only happens at the host bridge,
> > never at a PCI-PCI bridge (which is what "PCI bridge" usually means).
> > 
> > The commit log and comments could maybe be clarified, but this all
> > looks to me like it's doing the right thing in spite of abusing the
> > use of struct resource.
> 
> Thanks for reviewing. I'll try to clarify on the commit message. Not sure
> how to change the issue with struct pci_bus_region though.

Yeah, I don't know either.  screen_info_resources() takes a struct
resource pointer, but puts bus addresses into it.  That's misleading
at best, but it would be quite a bit more work to fix that.

I'm not sure we have a generic struct for bus addresses.  We have
struct pci_bus_region, but I'm not sure if screen_info is necessarily
specific to PCI.

Bjorn