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.
I don't know what "stored in screen_info" means. Most of the addresses filled in by screen_info_resources() are hard-coded bus addresses specified by PCI and VGA specs. These are not just offsets "seen during boot"; these are legacy addresses that are not programmable via usual PCI BARs. They're hard-wired into VGA devices, so if we use the VGA frame buffer at 0xA0000, the 0xA0000 address must appear on the PCI bus.
VIDEO_TYPE_VLFB and VIDEO_TYPE_EFI are exceptions; I don't know how they work, but if they return addresses from firmware, I would expect them to be PCI bus addresses as well.
The helper pcibios_bus_to_resource() performs the relocation of the screen_info resource. The result now matches the I/O-memory resource of the PCI graphics device. As before, we store away the information necessary to update the information in screen_info.
Commit 78aa89d1dfba ("firmware/sysfb: Update screen_info for relocated EFI framebuffers") added the code for updating screen_info. It is based on similar functionality that pre-existed in efifb. Efifb uses a pointer to the PCI resource, while the newer code does a memcpy of the region. Hence efifb sees any updates to the PCI resource and avoids the issue.
v2:
- Fixed tags (Takashi, Ivan)
- Updated information on efifb
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de Reported-by: "Ivan T. Ivanov" iivanov@suse.de 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?
s/essentialy/essentially/
Tested-by: "Ivan T. Ivanov" iivanov@suse.de Fixes: 78aa89d1dfba ("firmware/sysfb: Update screen_info for relocated EFI framebuffers") Cc: dri-devel@lists.freedesktop.org Cc: stable@vger.kernel.org # v6.9+
drivers/video/screen_info_pci.c | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-)
diff --git a/drivers/video/screen_info_pci.c b/drivers/video/screen_info_pci.c index 6c5833517141..c46c75dc3fae 100644 --- a/drivers/video/screen_info_pci.c +++ b/drivers/video/screen_info_pci.c @@ -8,7 +8,7 @@ static struct pci_dev *screen_info_lfb_pdev; static size_t screen_info_lfb_bar; static resource_size_t screen_info_lfb_offset; -static struct resource screen_info_lfb_res = DEFINE_RES_MEM(0, 0); +static struct pci_bus_region screen_info_lfb_region; static bool __screen_info_relocation_is_valid(const struct screen_info *si, struct resource *pr) { @@ -31,7 +31,7 @@ void screen_info_apply_fixups(void) if (screen_info_lfb_pdev) { struct resource *pr = &screen_info_lfb_pdev->resource[screen_info_lfb_bar];
if (pr->start != screen_info_lfb_res.start) {
if (pr->start != screen_info_lfb_region.start) { if (__screen_info_relocation_is_valid(si, pr)) { /* * Only update base if we have an actual@@ -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.
pr = pci_find_resource(pdev, r); if (!pr) continue;@@ -85,7 +96,7 @@ static void screen_info_fixup_lfb(struct pci_dev *pdev) screen_info_lfb_pdev = pdev; screen_info_lfb_bar = pr - pdev->resource; screen_info_lfb_offset = r->start - pr->start;
memcpy(&screen_info_lfb_res, r, sizeof(screen_info_lfb_res));
memcpy(&screen_info_lfb_region, &bus_region, sizeof(screen_info_lfb_region));} DECLARE_PCI_FIXUP_CLASS_HEADER(PCI_ANY_ID, PCI_ANY_ID, PCI_BASE_CLASS_DISPLAY, 16, -- 2.49.0