[PATCH v2] video: screen_info: Relocate framebuffers behind PCI bridges
Thomas Zimmermann
tzimmermann at suse.de
Mon May 5 13:05:34 UTC 2025
Hi
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.
>
> 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.
Exactly. These addresses are determined by Grub, stored in the
screen_info field of the kernel's boot params, and then transferred to
the kernel. [1] Somewhere between storing them and using them in the
kernel, the offset is applied to the address range. The content in
screen_info is never being updated. That's what this patch does.
[1]
https://elixir.bootlin.com/linux/v6.14.5/source/arch/x86/include/uapi/asm/bootparam.h#L117
>
>> 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 at suse.de>
>> Reported-by: "Ivan T. Ivanov" <iivanov at 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?
Try again, please. I've updated the settings of this bug report.
>
> s/essentialy/essentially/
>
>> Tested-by: "Ivan T. Ivanov" <iivanov at suse.de>
>> Fixes: 78aa89d1dfba ("firmware/sysfb: Update screen_info for relocated EFI framebuffers")
>> Cc: dri-devel at lists.freedesktop.org
>> Cc: <stable at 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.
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.
Best regards
Thomas
>
>> 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
>>
--
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)
More information about the dri-devel
mailing list