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
