linux-kernel - Re: [PATCH v4] PCI/VGA: Make the vga_is_firmware

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <fb242a7e-b1ee-7be0-cbc4-be710e246279@loongson.cn>
Date:   Fri, 18 Aug 2023 18:20:44 +0800
From:   suijingfeng <suijingfeng@...ngson.cn>
To:     Bjorn Helgaas <helgaas@...nel.org>
Cc:     Bjorn Helgaas <bhelgaas@...gle.com>,
        loongson-kernel@...ts.loongnix.cn, linux-pci@...r.kernel.org,
        linux-riscv@...ts.infradead.org, linux-kernel@...r.kernel.org,
        dri-devel@...ts.freedesktop.org
Subject: Re: [PATCH v4] PCI/VGA: Make the vga_is_firmware_default() less
 arch-dependent

Hi,

On 2023/8/18 06:08, Bjorn Helgaas wrote:
>> Please note that before apply this patch, vgaarb can not select the
>> right boot vga due to weird logic introduced with the commit
>> 57fc7323a8e7c ("LoongArch: Add PCI controller support")
> If we need this reference to 57fc7323a8e7c, we need more specifics
> about what the "weird logic" is.  pci_fixup_vgadev() is the only
> obvious VGA connection, so I suppose it's related to that.
>
Yes, you are right.

The pci_fixup_vgadev() function will set the last VGA device enumerated as the default boot device.
By "the last" VGA device, I mean that this device has the largest PCI bus, domain, and function triple.
Thus, it is added to vgaarb in the end of all VGA device.
So that logic expresses that the last one added will be the default.
This probably is not what we want.

On the LS3A5000+LS7A1000 platform, the last VGA device is a S3 graphics 
(08:00.0). This GPU has two cores. Say the log below:

$ lspci | grep VGA

  00:06.1 VGA compatible controller: Loongson Technology LLC DC (Display Controller) (rev 01)
  03:00.0 VGA compatible controller: Advanced Micro Devices, Inc. [AMD/ATI] Caicos XT [Radeon HD 7470/8470 / R5 235/310 OEM]
  07:00.0 VGA compatible controller: S3 Graphics Ltd. Device 9070 (rev 01)
  08:00.0 VGA compatible controller: S3 Graphics Ltd. Device 9070 (rev 01)

[    0.361781] vgaarb: loaded
[    0.367838] pci 0000:00:06.1: Overriding boot device as 1002:6778
[    0.367841] pci 0000:00:06.1: Overriding boot device as 5333:9070
[    0.367843] pci 0000:00:06.1: Overriding boot device as 5333:9070

1) The "weird" logic completely overrides whatever decision VGAARB ever made.

It seems to say that the decision ever made by VGAARB is useless.
Well, I think VGAARB shouldn't endure this; VGAARB has to be small.

2) The results it gives are not correct either.

In the first testing example in my commit message,
it overrides the S3 graphics as the default boot VGA instead of the AMD/ATI GPU.
Actually, the firmware chooses the AMD/ATI GPU as the "frimware default".

3) It tries to make the decision for the end user instead of the firmware.

Therefore, that function is always wrong. Again, it's a policy, not a mechanism.

Since that already have been merge, I'm fine.
Maybe Huacai is busy, he might don't has the time to carry on a deep thinking.
But I think we should correct the mistake ever made,
let's merge this patch to make vgaarb great again ?

Well, that commit is not a dependency, I don't mind delete the referencing
to that commit. After all, I think my patch will be effective on other architectures.
Is additional testing on ARM64 and X86 is needed, if so I have to find the machine to
carry on the testing.