| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <511b7382-d4b7-4d87-9695-f621c0a60b4e@suse.de>
Date: Mon, 23 Jun 2025 12:43:32 +0200
From: Thomas Zimmermann <tzimmermann@...e.de>
To: Mario Limonciello <superm1@...nel.org>,
Bjorn Helgaas <bhelgaas@...gle.com>
Cc: Alex Deucher <alexander.deucher@....com>,
Christian König <christian.koenig@....com>,
David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>,
Lukas Wunner <lukas@...ner.de>,
Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
Maxime Ripard <mripard@...nel.org>, David Woodhouse <dwmw2@...radead.org>,
Lu Baolu <baolu.lu@...ux.intel.com>, Joerg Roedel <joro@...tes.org>,
Will Deacon <will@...nel.org>, Robin Murphy <robin.murphy@....com>,
Alex Williamson <alex.williamson@...hat.com>,
Jaroslav Kysela <perex@...ex.cz>, Takashi Iwai <tiwai@...e.com>,
"open list:DRM DRIVERS" <dri-devel@...ts.freedesktop.org>,
open list <linux-kernel@...r.kernel.org>,
"open list:INTEL IOMMU (VT-d)" <iommu@...ts.linux.dev>,
"open list:PCI SUBSYSTEM" <linux-pci@...r.kernel.org>,
"open list:VFIO DRIVER" <kvm@...r.kernel.org>,
"open list:SOUND" <linux-sound@...r.kernel.org>,
Daniel Dadap <ddadap@...dia.com>,
Mario Limonciello <mario.limonciello@....com>
Subject: Re: [PATCH v3 6/7] PCI/VGA: Move check for firmware default out of
VGA arbiter
Hi
Am 21.06.25 um 00:17 schrieb Mario Limonciello:
> On 6/20/2025 3:45 AM, Thomas Zimmermann wrote:
>> Hi
>>
>> Am 20.06.25 um 04:49 schrieb Mario Limonciello:
>>> From: Mario Limonciello <mario.limonciello@....com>
>>>
>>> The x86 specific check for whether a framebuffer belongs to a device
>>> works for display devices as well as VGA devices. Callers to
>>> video_is_primary_device() can benefit from checking non-VGA display
>>> devices.
>>>
>>> Move the x86 specific check into x86 specific code, and adjust VGA
>>> arbiter to call that code as well. This allows fbcon to find the
>>> right PCI device on systems that don't have VGA devices.
>>>
>>> Suggested-by: Thomas Zimmermann <tzimmermann@...e.de>
>>> Signed-off-by: Mario Limonciello <mario.limonciello@....com>
>>> ---
>>> arch/x86/video/video-common.c | 28 +++++++++++++++++++++++++++
>>> drivers/pci/vgaarb.c | 36
>>> ++---------------------------------
>>> 2 files changed, 30 insertions(+), 34 deletions(-)
>>>
>>> diff --git a/arch/x86/video/video-common.c b/arch/x86/video/video-
>>> common.c
>>> index 81fc97a2a837a..718116e35e450 100644
>>> --- a/arch/x86/video/video-common.c
>>> +++ b/arch/x86/video/video-common.c
>>> @@ -9,6 +9,7 @@
>>> #include <linux/module.h>
>>> #include <linux/pci.h>
>>> +#include <linux/screen_info.h>
>>> #include <linux/vgaarb.h>
>>> #include <asm/video.h>
>>> @@ -27,13 +28,40 @@ EXPORT_SYMBOL(pgprot_framebuffer);
>>> bool video_is_primary_device(struct device *dev)
>>
>> I'm not sure I understand this patch. video_is_primary_device()
>> already exists for 3 architectures, including x86. [1] Adding it here
>> should produce an error. (?)
>
> I wasn't adding a new implementation of it, I was augmenting the x86
> implementation.
Indeed. Apologies, I must have somehow misread the patch. So this is
essentially doing what I proposed.
>
> But I guess based on your below point it just needs to call
> screen_info_pci_dev().
Yeah, the helper already does everything necessary.
>>
>> [1] https://elixir.bootlin.com/linux/v6.15.2/A/ident/
>> video_is_primary_device
>>
>> The code on x86 is
>>
>> bool <https://elixir.bootlin.com/linux/v6.15.2/C/ident/
>> bool>video_is_primary_device
>> <https://elixir.bootlin.com/linux/v6.15.2/
>> C/ident/video_is_primary_device>(structdevice <https://
>> elixir.bootlin.com/linux/v6.15.2/C/ident/device>*dev) { structpci_dev
>> <https://elixir.bootlin.com/linux/v6.15.2/C/ident/pci_dev>*pdev; if(!
>> dev_is_pci <https://elixir.bootlin.com/linux/v6.15.2/C/ident/
>> dev_is_pci>(dev)) returnfalse
>> <https://elixir.bootlin.com/linux/v6.15.2/ C/ident/false>;
>> pdev=to_pci_dev <https://elixir.bootlin.com/linux/
>> v6.15.2/C/ident/to_pci_dev>(dev); return(pdev==vga_default_device
>> <https://elixir.bootlin.com/linux/v6.15.2/C/ident/vga_default_device>());
>> }
>>
>> I was thinking about extending it to test for additional properties,
>> like this
>>
>> bool <https://elixir.bootlin.com/linux/v6.15.2/C/ident/
>> bool>video_is_primary_device
>> <https://elixir.bootlin.com/linux/v6.15.2/
>> C/ident/video_is_primary_device>(structdevice <https://
>> elixir.bootlin.com/linux/v6.15.2/C/ident/device>*dev) { structpci_dev
>> <https://elixir.bootlin.com/linux/v6.15.2/C/ident/pci_dev>*pdev; if(!
>> dev_is_pci <https://elixir.bootlin.com/linux/v6.15.2/C/ident/
>> dev_is_pci>(dev)) returnfalse
>> <https://elixir.bootlin.com/linux/v6.15.2/ C/ident/false>;
>> pdev=to_pci_dev <https://elixir.bootlin.com/linux/
>> v6.15.2/C/ident/to_pci_dev>(dev); if(pdev==vga_default_device
>> <https://
>> elixir.bootlin.com/linux/v6.15.2/C/ident/vga_default_device>())
>> return true for_each_pci_dev() { // test if display and could be
>> primary. } return false; // nothing found }
>>
>
> The above looks like some bad copy / paste. Could you clarify?
Oh, well. I really messed up my reply. :D
What I meant is what you already implemented, but with the existing helper:
bool video_is_primary_device(dev)
{
if (dev == vga_default_device())
return true
if (dev == screen_info_pci_device())
return true
return false
}
One thing to keep in minds is that video_is_primary_device() currently
returns false by default. IDK if that's a problem for user space, but
user space should at least pick a reasonable fallback in that case.
Best regards
Thomas
>
>>
>> This would then be called from per-device sysfs code that export a
>> property similar to boot_vga (such as boot_display).
>
> Here's the other idea I had in mind.
>
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index 268c69daa4d57..8535950b4c0f3 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -30,6 +30,7 @@
> #include <linux/msi.h>
> #include <linux/of.h>
> #include <linux/aperture.h>
> +#include <asm/video.h>
> #include "pci.h"
>
> #ifndef ARCH_PCI_DEV_GROUPS
> @@ -679,6 +680,13 @@ const struct attribute_group *pcibus_groups[] = {
> NULL,
> };
>
> +static ssize_t boot_console_show(struct device *dev, struct
> device_attribute *attr,
> + char *buf)
> +{
> + return sysfs_emit(buf, "%u\n", video_is_primary_device(dev));
> +}
> +static DEVICE_ATTR_RO(boot_console);
> +
> static ssize_t boot_vga_show(struct device *dev, struct
> device_attribute *attr,
> char *buf)
> {
> @@ -1698,6 +1706,7 @@ late_initcall(pci_sysfs_init);
>
> static struct attribute *pci_dev_dev_attrs[] = {
> &dev_attr_boot_vga.attr,
> + &dev_attr_boot_console.attr,
> NULL,
> };
>
> @@ -1710,6 +1719,9 @@ static umode_t pci_dev_attrs_are_visible(struct
> kobject *kobj,
> if (a == &dev_attr_boot_vga.attr && pci_is_vga(pdev))
> return a->mode;
>
> + if (a == &dev_attr_boot_console.attr && pci_is_display(pdev))
> + return a->mode;
> +
> return 0;
> }
>
>
>>
>>
>> The issue is currently just an x86 problem, but I can imagine
>> something similar happening on ARM. There we'd have to go through the
>> DT tree to figure out the primary device. That's a problem for a
>> later patch set, but we should keep this in mind.
>
> I think that the sysfs file idea above would work for any arch.
>
>>
>>> {
>>> + u64 base = screen_info.lfb_base;
>>> + u64 size = screen_info.lfb_size;
>>> struct pci_dev *pdev;
>>> + struct resource *r;
>>> + u64 limit;
>>> if (!dev_is_pci(dev))
>>> return false;
>>> pdev = to_pci_dev(dev);
>>> + if (!pci_is_display(pdev))
>>> + return false;
>>> +
>>> + /* Select the device owning the boot framebuffer if there is
>>> one */
>>> + if (screen_info.capabilities & VIDEO_CAPABILITY_64BIT_BASE)
>>> + base |= (u64)screen_info.ext_lfb_base << 32;
>>> +
>>> + limit = base + size;
>>> +
>>> + /* Does firmware framebuffer belong to us? */
>>> + pci_dev_for_each_resource(pdev, r) {
>>> + if (resource_type(r) != IORESOURCE_MEM)
>>> + continue;
>>> +
>>> + if (!r->start || !r->end)
>>> + continue;
>>> +
>>> + if (base < r->start || limit >= r->end)
>>> + continue;
>>> +
>>> + return true;
>>> + }
>>> +
>>
>> You can drop all this code and call screen_info_pci_dev() instead. I
>> simply never got to update vgaarb to use it.
>
> 👍
>
>>
>> [2] https://elixir.bootlin.com/linux/v6.15.2/source/drivers/video/
>> screen_info_pci.c#L109
>>
>>> return (pdev == vga_default_device());
>>> }
>>> EXPORT_SYMBOL(video_is_primary_device);
>>> diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
>>> index 78748e8d2dbae..15ab58c70b016 100644
>>> --- a/drivers/pci/vgaarb.c
>>> +++ b/drivers/pci/vgaarb.c
>>> @@ -26,12 +26,12 @@
>>> #include <linux/poll.h>
>>> #include <linux/miscdevice.h>
>>> #include <linux/slab.h>
>>> -#include <linux/screen_info.h>
>>> #include <linux/vt.h>
>>> #include <linux/console.h>
>>> #include <linux/acpi.h>
>>> #include <linux/uaccess.h>
>>> #include <linux/vgaarb.h>
>>> +#include <asm/video.h>
>>> static void vga_arbiter_notify_clients(void);
>>> @@ -554,38 +554,6 @@ void vga_put(struct pci_dev *pdev, unsigned int
>>> rsrc)
>>> }
>>> EXPORT_SYMBOL(vga_put);
>>> -static bool vga_is_firmware_default(struct pci_dev *pdev)
>>> -{
>>> -#if defined(CONFIG_X86)
>>> - u64 base = screen_info.lfb_base;
>>> - u64 size = screen_info.lfb_size;
>>> - struct resource *r;
>>> - u64 limit;
>>> -
>>> - /* Select the device owning the boot framebuffer if there is
>>> one */
>>> -
>>> - if (screen_info.capabilities & VIDEO_CAPABILITY_64BIT_BASE)
>>> - base |= (u64)screen_info.ext_lfb_base << 32;
>>> -
>>> - limit = base + size;
>>> -
>>> - /* Does firmware framebuffer belong to us? */
>>> - pci_dev_for_each_resource(pdev, r) {
>>> - if (resource_type(r) != IORESOURCE_MEM)
>>> - continue;
>>> -
>>> - if (!r->start || !r->end)
>>> - continue;
>>> -
>>> - if (base < r->start || limit >= r->end)
>>> - continue;
>>> -
>>> - return true;
>>> - }
>>> -#endif
>>> - return false;
>>> -}
>>> -
>>> static bool vga_arb_integrated_gpu(struct device *dev)
>>> {
>>> #if defined(CONFIG_ACPI)
>>> @@ -623,7 +591,7 @@ static bool vga_is_boot_device(struct vga_device
>>> *vgadev)
>>> if (boot_vga && boot_vga->is_firmware_default)
>>> return false;
>>> - if (vga_is_firmware_default(pdev)) {
>>> + if (video_is_primary_device(&pdev->dev)) {
>>
>> Maybe not change this because you don't want to end up with non-VGA
>> devices here.
>
> 👍
>
>>
>> Best regards
>> Thomas
>>
>>> vgadev->is_firmware_default = true;
>>> return true;
>>> }
>>
>
--
--
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)
Powered by blists - more mailing lists