[PATCH v5 1/5] x86/quirks: Fix stolen detection with integrated + discrete GPU

[PATCH v5 1/5] x86/quirks: Fix stolen detection with integrated + discrete GPU

[Lucas De Marchi]

early_pci_scan_bus() does a depth-first traversal, possibly calling
the quirk functions for each device based on vendor, device and class
from early_qrk table. intel_graphics_quirks() however uses PCI_ANY_ID
and does additional filtering in the quirk.

If there is an Intel integrated + discrete GPU the quirk may be called
first for the discrete GPU based on the PCI topology. Then we will fail
to reserve the system stolen memory for the integrated GPU, because we
will already have marked the quirk as "applied".

This was reproduced in a setup with Alderlake-P (integrated) + DG2
(discrete), with the following PCI topology:

	- 00:01.0 Bridge
	  `- 03:00.0 DG2
	- 00:02.0 Integrated GPU

So, stop using the QFLAG_APPLY_ONCE flag, replacing it with a static
local variable. We can set this variable in the right place, inside
intel_graphics_quirks(), only when the quirk was actually applied, i.e.
when we find the integrated GPU based on the intel_early_ids table.

Cc: stable@vger.kernel.org
Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---

v5: apply fix before the refactor

 arch/x86/kernel/early-quirks.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/early-quirks.c b/arch/x86/kernel/early-quirks.c
index 1ca3a56fdc2d..de9a76eb544e 100644
--- a/arch/x86/kernel/early-quirks.c
+++ b/arch/x86/kernel/early-quirks.c
@@ -589,10 +589,14 @@ intel_graphics_stolen(int num, int slot, int func,
 
 static void __init intel_graphics_quirks(int num, int slot, int func)
 {
+	static bool quirk_applied __initdata;
 	const struct intel_early_ops *early_ops;
 	u16 device;
 	int i;
 
+	if (quirk_applied)
+		return;
+
 	device = read_pci_config_16(num, slot, func, PCI_DEVICE_ID);
 
 	for (i = 0; i < ARRAY_SIZE(intel_early_ids); i++) {
@@ -605,6 +609,8 @@ static void __init intel_graphics_quirks(int num, int slot, int func)
 
 		intel_graphics_stolen(num, slot, func, early_ops);
 
+		quirk_applied = true;
+
 		return;
 	}
 }
@@ -705,7 +711,7 @@ static struct chipset early_qrk[] __initdata = {
 	{ PCI_VENDOR_ID_INTEL, 0x3406, PCI_CLASS_BRIDGE_HOST,
 	  PCI_BASE_CLASS_BRIDGE, 0, intel_remapping_check },
 	{ PCI_VENDOR_ID_INTEL, PCI_ANY_ID, PCI_CLASS_DISPLAY_VGA, PCI_ANY_ID,
-	  QFLAG_APPLY_ONCE, intel_graphics_quirks },
+	  0, intel_graphics_quirks },
 	/*
 	 * HPET on the current version of the Baytrail platform has accuracy
 	 * problems: it will halt in deep idle state - so we disable it.
-- 
2.34.1

[PATCH v5 2/5] x86/quirks: Stop using QFLAG_APPLY_ONCE in via_bugs()

[Lucas De Marchi]

Adopt the same approach as in intel_graphics_quirks(), with a static
local variable, to control when the quirk has already been applied.
However, contrary to intel_graphics_quirks() here we always set it as
applied as soon as it's called to avoid changing the current behavior
that is not failing.

After converting other users, it will allow us to remove all the logic
handling the flags.

Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
 arch/x86/kernel/early-quirks.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/early-quirks.c b/arch/x86/kernel/early-quirks.c
index de9a76eb544e..59cc67aace93 100644
--- a/arch/x86/kernel/early-quirks.c
+++ b/arch/x86/kernel/early-quirks.c
@@ -57,6 +57,13 @@ static void __init fix_hypertransport_config(int num, int slot, int func)
 static void __init via_bugs(int  num, int slot, int func)
 {
 #ifdef CONFIG_GART_IOMMU
+	static bool quirk_applied __initdata;
+
+	if (quirk_applied)
+		return;
+
+	quirk_applied = true;
+
 	if ((max_pfn > MAX_DMA32_PFN ||  force_iommu) &&
 	    !gart_iommu_aperture_allowed) {
 		printk(KERN_INFO
@@ -697,7 +704,7 @@ static struct chipset early_qrk[] __initdata = {
 	{ PCI_VENDOR_ID_NVIDIA, PCI_ANY_ID,
 	  PCI_CLASS_BRIDGE_PCI, PCI_ANY_ID, QFLAG_APPLY_ONCE, nvidia_bugs },
 	{ PCI_VENDOR_ID_VIA, PCI_ANY_ID,
-	  PCI_CLASS_BRIDGE_PCI, PCI_ANY_ID, QFLAG_APPLY_ONCE, via_bugs },
+	  PCI_CLASS_BRIDGE_PCI, PCI_ANY_ID, 0, via_bugs },
 	{ PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_K8_NB,
 	  PCI_CLASS_BRIDGE_HOST, PCI_ANY_ID, 0, fix_hypertransport_config },
 	{ PCI_VENDOR_ID_ATI, PCI_DEVICE_ID_ATI_IXP400_SMBUS,
-- 
2.34.1

[PATCH v5 3/5] x86/quirks: Stop using QFLAG_APPLY_ONCE in nvidia_bugs()

[Lucas De Marchi]

Adopt the same approach as in intel_graphics_quirks(), with a static
local variable, to control when the quirk has already been applied.
However, contrary to intel_graphics_quirks(), here we always set it as
applied as soon as it's called to avoid changing the current behavior
that is not failing.

This is the last user of the flags, so we can cleanup the early-quirks,
removing all the flags logic later.

Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
 arch/x86/kernel/early-quirks.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/early-quirks.c b/arch/x86/kernel/early-quirks.c
index 59cc67aace93..7c70977737de 100644
--- a/arch/x86/kernel/early-quirks.c
+++ b/arch/x86/kernel/early-quirks.c
@@ -88,6 +88,13 @@ static void __init nvidia_bugs(int num, int slot, int func)
 {
 #ifdef CONFIG_ACPI
 #ifdef CONFIG_X86_IO_APIC
+	static bool quirk_applied __initdata;
+
+	if (quirk_applied)
+		return;
+
+	quirk_applied = true;
+
 	/*
 	 * Only applies to Nvidia root ports (bus 0) and not to
 	 * Nvidia graphics cards with PCI ports on secondary buses.
@@ -702,7 +709,7 @@ struct chipset {
 
 static struct chipset early_qrk[] __initdata = {
 	{ PCI_VENDOR_ID_NVIDIA, PCI_ANY_ID,
-	  PCI_CLASS_BRIDGE_PCI, PCI_ANY_ID, QFLAG_APPLY_ONCE, nvidia_bugs },
+	  PCI_CLASS_BRIDGE_PCI, PCI_ANY_ID, 0, nvidia_bugs },
 	{ PCI_VENDOR_ID_VIA, PCI_ANY_ID,
 	  PCI_CLASS_BRIDGE_PCI, PCI_ANY_ID, 0, via_bugs },
 	{ PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_K8_NB,
-- 
2.34.1

[PATCH v5 4/5] x86/quirks: Remove unused logic for flags

[Lucas De Marchi]

The flags were only used to mark the quirk as applied when it was
requested to be called only once. Now all the users were converted to
use a static local variable, so this logic can be removed.

Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
 arch/x86/kernel/early-quirks.c | 35 ++++++++++++++--------------------
 1 file changed, 14 insertions(+), 21 deletions(-)

diff --git a/arch/x86/kernel/early-quirks.c b/arch/x86/kernel/early-quirks.c
index 7c70977737de..1db4d92f8a85 100644
--- a/arch/x86/kernel/early-quirks.c
+++ b/arch/x86/kernel/early-quirks.c
@@ -695,37 +695,33 @@ static void __init apple_airport_reset(int bus, int slot, int func)
 	early_iounmap(mmio, BCM4331_MMIO_SIZE);
 }
 
-#define QFLAG_APPLY_ONCE 	0x1
-#define QFLAG_APPLIED		0x2
-#define QFLAG_DONE		(QFLAG_APPLY_ONCE|QFLAG_APPLIED)
 struct chipset {
 	u32 vendor;
 	u32 device;
 	u32 class;
 	u32 class_mask;
-	u32 flags;
 	void (*f)(int num, int slot, int func);
 };
 
 static struct chipset early_qrk[] __initdata = {
 	{ PCI_VENDOR_ID_NVIDIA, PCI_ANY_ID,
-	  PCI_CLASS_BRIDGE_PCI, PCI_ANY_ID, 0, nvidia_bugs },
+	  PCI_CLASS_BRIDGE_PCI, PCI_ANY_ID, nvidia_bugs },
 	{ PCI_VENDOR_ID_VIA, PCI_ANY_ID,
-	  PCI_CLASS_BRIDGE_PCI, PCI_ANY_ID, 0, via_bugs },
+	  PCI_CLASS_BRIDGE_PCI, PCI_ANY_ID, via_bugs },
 	{ PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_K8_NB,
-	  PCI_CLASS_BRIDGE_HOST, PCI_ANY_ID, 0, fix_hypertransport_config },
+	  PCI_CLASS_BRIDGE_HOST, PCI_ANY_ID, fix_hypertransport_config },
 	{ PCI_VENDOR_ID_ATI, PCI_DEVICE_ID_ATI_IXP400_SMBUS,
-	  PCI_CLASS_SERIAL_SMBUS, PCI_ANY_ID, 0, ati_bugs },
+	  PCI_CLASS_SERIAL_SMBUS, PCI_ANY_ID, ati_bugs },
 	{ PCI_VENDOR_ID_ATI, PCI_DEVICE_ID_ATI_SBX00_SMBUS,
-	  PCI_CLASS_SERIAL_SMBUS, PCI_ANY_ID, 0, ati_bugs_contd },
+	  PCI_CLASS_SERIAL_SMBUS, PCI_ANY_ID, ati_bugs_contd },
 	{ PCI_VENDOR_ID_INTEL, 0x3403, PCI_CLASS_BRIDGE_HOST,
-	  PCI_BASE_CLASS_BRIDGE, 0, intel_remapping_check },
+	  PCI_BASE_CLASS_BRIDGE, intel_remapping_check },
 	{ PCI_VENDOR_ID_INTEL, 0x3405, PCI_CLASS_BRIDGE_HOST,
-	  PCI_BASE_CLASS_BRIDGE, 0, intel_remapping_check },
+	  PCI_BASE_CLASS_BRIDGE, intel_remapping_check },
 	{ PCI_VENDOR_ID_INTEL, 0x3406, PCI_CLASS_BRIDGE_HOST,
-	  PCI_BASE_CLASS_BRIDGE, 0, intel_remapping_check },
+	  PCI_BASE_CLASS_BRIDGE, intel_remapping_check },
 	{ PCI_VENDOR_ID_INTEL, PCI_ANY_ID, PCI_CLASS_DISPLAY_VGA, PCI_ANY_ID,
-	  0, intel_graphics_quirks },
+	  intel_graphics_quirks },
 	/*
 	 * HPET on the current version of the Baytrail platform has accuracy
 	 * problems: it will halt in deep idle state - so we disable it.
@@ -735,9 +731,9 @@ static struct chipset early_qrk[] __initdata = {
 	 *    http://www.intel.com/content/dam/www/public/us/en/documents/datasheets/atom-z8000-datasheet-vol-1.pdf
 	 */
 	{ PCI_VENDOR_ID_INTEL, 0x0f00,
-		PCI_CLASS_BRIDGE_HOST, PCI_ANY_ID, 0, force_disable_hpet},
+	  PCI_CLASS_BRIDGE_HOST, PCI_ANY_ID, force_disable_hpet},
 	{ PCI_VENDOR_ID_BROADCOM, 0x4331,
-	  PCI_CLASS_NETWORK_OTHER, PCI_ANY_ID, 0, apple_airport_reset},
+	  PCI_CLASS_NETWORK_OTHER, PCI_ANY_ID, apple_airport_reset},
 	{}
 };
 
@@ -778,12 +774,9 @@ static int __init check_dev_quirk(int num, int slot, int func)
 			((early_qrk[i].device == PCI_ANY_ID) ||
 			(early_qrk[i].device == device)) &&
 			(!((early_qrk[i].class ^ class) &
-			    early_qrk[i].class_mask))) {
-				if ((early_qrk[i].flags &
-				     QFLAG_DONE) != QFLAG_DONE)
-					early_qrk[i].f(num, slot, func);
-				early_qrk[i].flags |= QFLAG_APPLIED;
-			}
+			    early_qrk[i].class_mask)))
+				early_qrk[i].f(num, slot, func);
+
 	}
 
 	type = read_pci_config_byte(num, slot, func,
-- 
2.34.1

[PATCH v5 5/5] x86/quirks: Improve line wrap on quirk conditions

[Lucas De Marchi]

Remove extra parenthesis and wrap lines so it's easier to read what are
the conditions being checked. The call to the hook also had an extra
indentation: remove here to conform to coding style.

Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 arch/x86/kernel/early-quirks.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kernel/early-quirks.c b/arch/x86/kernel/early-quirks.c
index 1db4d92f8a85..996e3cbc1c5f 100644
--- a/arch/x86/kernel/early-quirks.c
+++ b/arch/x86/kernel/early-quirks.c
@@ -769,14 +769,12 @@ static int __init check_dev_quirk(int num, int slot, int func)
 	device = read_pci_config_16(num, slot, func, PCI_DEVICE_ID);
 
 	for (i = 0; early_qrk[i].f != NULL; i++) {
-		if (((early_qrk[i].vendor == PCI_ANY_ID) ||
-			(early_qrk[i].vendor == vendor)) &&
-			((early_qrk[i].device == PCI_ANY_ID) ||
-			(early_qrk[i].device == device)) &&
-			(!((early_qrk[i].class ^ class) &
-			    early_qrk[i].class_mask)))
-				early_qrk[i].f(num, slot, func);
-
+		if ((early_qrk[i].vendor == PCI_ANY_ID ||
+		     early_qrk[i].vendor == vendor) &&
+		    (early_qrk[i].device == PCI_ANY_ID ||
+		     early_qrk[i].device == device) &&
+		    !((early_qrk[i].class ^ class) & early_qrk[i].class_mask))
+			early_qrk[i].f(num, slot, func);
 	}
 
 	type = read_pci_config_byte(num, slot, func,
-- 
2.34.1

Re: [PATCH v5 1/5] x86/quirks: Fix stolen detection with integrated + discrete GPU

[Borislav Petkov]

n Thu, Jan 13, 2022 at 04:28:39PM -0800, Lucas De Marchi wrote:
> early_pci_scan_bus() does a depth-first traversal, possibly calling
> the quirk functions for each device based on vendor, device and class
> from early_qrk table. intel_graphics_quirks() however uses PCI_ANY_ID
> and does additional filtering in the quirk.
> 
> If there is an Intel integrated + discrete GPU the quirk may be called
> first for the discrete GPU based on the PCI topology. Then we will fail
> to reserve the system stolen memory for the integrated GPU, because we
> will already have marked the quirk as "applied".

Who is "we"?

Please use passive voice in your commit message: no "we" or "I", etc,
and describe your changes in imperative mood.

Bottom line is: personal pronouns are ambiguous in text, especially with
so many parties/companies/etc developing the kernel so let's avoid them
please.

> This was reproduced in a setup with Alderlake-P (integrated) + DG2
> (discrete), with the following PCI topology:
> 
> 	- 00:01.0 Bridge
> 	  `- 03:00.0 DG2
> 	- 00:02.0 Integrated GPU
> 
> So, stop using the QFLAG_APPLY_ONCE flag, replacing it with a static
> local variable. We can set this variable in the right place, inside
> intel_graphics_quirks(), only when the quirk was actually applied, i.e.
> when we find the integrated GPU based on the intel_early_ids table.
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> ---
> 
> v5: apply fix before the refactor
> 
>  arch/x86/kernel/early-quirks.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/early-quirks.c b/arch/x86/kernel/early-quirks.c
> index 1ca3a56fdc2d..de9a76eb544e 100644
> --- a/arch/x86/kernel/early-quirks.c
> +++ b/arch/x86/kernel/early-quirks.c
> @@ -589,10 +589,14 @@ intel_graphics_stolen(int num, int slot, int func,
>  
>  static void __init intel_graphics_quirks(int num, int slot, int func)
>  {
> +	static bool quirk_applied __initdata;
>  	const struct intel_early_ops *early_ops;
>  	u16 device;
>  	int i;
>  
> +	if (quirk_applied)
> +		return;
> +
>  	device = read_pci_config_16(num, slot, func, PCI_DEVICE_ID);
>  
>  	for (i = 0; i < ARRAY_SIZE(intel_early_ids); i++) {
> @@ -605,6 +609,8 @@ static void __init intel_graphics_quirks(int num, int slot, int func)
>  
>  		intel_graphics_stolen(num, slot, func, early_ops);
>  
> +		quirk_applied = true;
> +
>  		return;
>  	}

So I wonder: why can't you simply pass in a static struct chipset *
pointer into the early_qrk[i].f function and in there you can set
QFLAG_APPLIED or so, so that you can mark that the quirk is applied by
using the nice, per-quirk flags someone has already added instead of
this ugly static variable?

Patch 3 especially makes me go, huh?

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v5 1/5] x86/quirks: Fix stolen detection with integrated + discrete GPU

[Lucas De Marchi]

On Tue, Jan 18, 2022 at 10:40:50AM +0100, Borislav Petkov wrote:
>n Thu, Jan 13, 2022 at 04:28:39PM -0800, Lucas De Marchi wrote:
>> early_pci_scan_bus() does a depth-first traversal, possibly calling
>> the quirk functions for each device based on vendor, device and class
>> from early_qrk table. intel_graphics_quirks() however uses PCI_ANY_ID
>> and does additional filtering in the quirk.
>>
>> If there is an Intel integrated + discrete GPU the quirk may be called
>> first for the discrete GPU based on the PCI topology. Then we will fail
>> to reserve the system stolen memory for the integrated GPU, because we
>> will already have marked the quirk as "applied".
>
>Who is "we"?
>
>Please use passive voice in your commit message: no "we" or "I", etc,
>and describe your changes in imperative mood.
>
>Bottom line is: personal pronouns are ambiguous in text, especially with
>so many parties/companies/etc developing the kernel so let's avoid them
>please.

I had the impression the subject/title should be imperative, with it
more relaxed in the body. It seems we have one more difference among
subsystems and I will adapt on next submissions to x86.

To clarify, "we" here means whoever is reading and following the code
path. It has the same connotation as the others in
'git log --grep "we\s"'.  From a quick grep it seems Linus merges a lot
of pull requests using that language and he himself uses it in commit
messages. Example: commit 054aa8d439b9 ("fget: check that the fd still
exists after getting a ref to it"). I was also surprised he also uses it
in the first person in some commits.

>
>> This was reproduced in a setup with Alderlake-P (integrated) + DG2
>> (discrete), with the following PCI topology:
>>
>> 	- 00:01.0 Bridge
>> 	  `- 03:00.0 DG2
>> 	- 00:02.0 Integrated GPU
>>
>> So, stop using the QFLAG_APPLY_ONCE flag, replacing it with a static
>> local variable. We can set this variable in the right place, inside
>> intel_graphics_quirks(), only when the quirk was actually applied, i.e.
>> when we find the integrated GPU based on the intel_early_ids table.
>>
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
>> ---
>>
>> v5: apply fix before the refactor
>>
>>  arch/x86/kernel/early-quirks.c | 8 +++++++-
>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/kernel/early-quirks.c b/arch/x86/kernel/early-quirks.c
>> index 1ca3a56fdc2d..de9a76eb544e 100644
>> --- a/arch/x86/kernel/early-quirks.c
>> +++ b/arch/x86/kernel/early-quirks.c
>> @@ -589,10 +589,14 @@ intel_graphics_stolen(int num, int slot, int func,
>>
>>  static void __init intel_graphics_quirks(int num, int slot, int func)
>>  {
>> +	static bool quirk_applied __initdata;
>>  	const struct intel_early_ops *early_ops;
>>  	u16 device;
>>  	int i;
>>
>> +	if (quirk_applied)
>> +		return;
>> +
>>  	device = read_pci_config_16(num, slot, func, PCI_DEVICE_ID);
>>
>>  	for (i = 0; i < ARRAY_SIZE(intel_early_ids); i++) {
>> @@ -605,6 +609,8 @@ static void __init intel_graphics_quirks(int num, int slot, int func)
>>
>>  		intel_graphics_stolen(num, slot, func, early_ops);
>>
>> +		quirk_applied = true;
>> +
>>  		return;
>>  	}
>
>So I wonder: why can't you simply pass in a static struct chipset *
>pointer into the early_qrk[i].f function and in there you can set
>QFLAG_APPLIED or so, so that you can mark that the quirk is applied by
>using the nice, per-quirk flags someone has already added instead of
>this ugly static variable?

It seems you prefer v1. See 20211218061313.100571-1-lucas.demarchi@intel.com

Although in the review Bjorn suggested just splitting the commit, it was
also mentioned that the PCI subsystem has no such logic in its
equivalent pci_do_fixups(): a quirk/fixup needing that should instead
use a static local.

After checking the code again I seconded his suggestion and adapted
on subsequent versions. Besides his comment on PCI subsystem I had these
motivations:

1) From a total o 11 quirks, only 3 were actually using that logic and 1
of them was wrong (the one being fixed here with the called vs applied
logic)

2) The resources these conditions are protecting are global to the
system: they all end up setting a variable - just having a static local
protecting the function from being called more than once seemed
appropriate to avoid that.

3) Even arch/x86 uses that for the PCI fixups
(arch/x86/kernel/quirks.c). It uses the logic "resource has already
been set" as opposed to adding static local since, but it seems similar
approach to me.


>
>Patch 3 especially makes me go, huh?

What is special about patch 3? Maybe the way it was split in v5 vs v4
made it not so clear: intention was not to change the current behavior
since it has been like this for 15+ years with no bug that I know of.
The previous approach would not call acpi_table_parse() twice for
example.

thanks
Lucas De Marchi

>
>-- 
>Regards/Gruss,
>    Boris.
>
>https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v5 1/5] x86/quirks: Fix stolen detection with integrated + discrete GPU

[Borislav Petkov]

On Tue, Jan 18, 2022 at 08:36:56AM -0800, Lucas De Marchi wrote:
> I had the impression the subject/title should be imperative, with it
> more relaxed in the body. It seems we have one more difference among
> subsystems and I will adapt on next submissions to x86.

We have written it down properly, in case it explains it better:

"The tip tree maintainers set value on following these rules, especially
on the request to write changelogs in imperative mood and not
impersonating code or the execution of it. This is not just a whim of
the maintainers. Changelogs written in abstract words are more precise
and tend to be less confusing than those written in the form of novels."

from Documentation/process/maintainer-tip.rst

> > So I wonder: why can't you simply pass in a static struct chipset *
> > pointer into the early_qrk[i].f function and in there you can set
> > QFLAG_APPLIED or so, so that you can mark that the quirk is applied by
> > using the nice, per-quirk flags someone has already added instead of
> > this ugly static variable?
> 
> It seems you prefer v1. See 20211218061313.100571-1-lucas.demarchi@intel.com

I do?

I don't see there:

	early_qrk[i].f(&early_qrk[i], num, slot, func)

so that the ->f callback can set the flags. Or at least the flags passed
in.

If it is not clear what I mean, pls say so and I'll try to produce an
example diff ontop.

> Although in the review Bjorn suggested just splitting the commit, it was
> also mentioned that the PCI subsystem has no such logic in its
> equivalent pci_do_fixups(): a quirk/fixup needing that should instead
> use a static local.

Why?

There's perfectly nice ->flags there for exactly stuff like that. static
vars are ugly and should be avoided if possible.

> What is special about patch 3?

Nothing special. It is just ugly.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [Intel-gfx] [PATCH v5 1/5] x86/quirks: Fix stolen detection with integrated + discrete GPU

[Bjorn Helgaas]

On Tue, Jan 18, 2022 at 06:26:48PM +0100, Borislav Petkov wrote:
> On Tue, Jan 18, 2022 at 08:36:56AM -0800, Lucas De Marchi wrote:
> > I had the impression the subject/title should be imperative, with it
> > more relaxed in the body. It seems we have one more difference among
> > subsystems and I will adapt on next submissions to x86.
> 
> We have written it down properly, in case it explains it better:
> 
> "The tip tree maintainers set value on following these rules, especially
> on the request to write changelogs in imperative mood and not
> impersonating code or the execution of it. This is not just a whim of
> the maintainers. Changelogs written in abstract words are more precise
> and tend to be less confusing than those written in the form of novels."
> 
> from Documentation/process/maintainer-tip.rst

Thanks for writing this down!  I do the same for PCI.  I suspect this
is a pretty conservative style that would be acceptable tree-wide even
if not required everywhere.

> > Although in the review Bjorn suggested just splitting the commit, it was
> > also mentioned that the PCI subsystem has no such logic in its
> > equivalent pci_do_fixups(): a quirk/fixup needing that should instead
> > use a static local.
> 
> Why?

I don't really care much one way or the other.  I think the simplest
approach is to remove QFLAG_APPLY_ONCE from intel_graphics_quirks()
and do nothing else, as I suggested here:

  https://lore.kernel.org/r/20220113000805.GA295089@bhelgaas

Unfortunately that didn't occur to me until I'd already suggested more
complicated things that no longer seem worthwhile to me.

The static variable might be ugly, but it does seem to be what
intel_graphics_quirks() wants -- a "do this at most once per system
but we don't know exactly which device" situation.

> There's perfectly nice ->flags there for exactly stuff like that. static
> vars are ugly and should be avoided if possible.

Bjorn

Re: [Intel-gfx] [PATCH v5 1/5] x86/quirks: Fix stolen detection with integrated + discrete GPU

[Borislav Petkov]

On Tue, Jan 18, 2022 at 11:58:53AM -0600, Bjorn Helgaas wrote:
> Thanks for writing this down!  I do the same for PCI.  I suspect this
> is a pretty conservative style that would be acceptable tree-wide even
> if not required everywhere.

Yeah, although that is an uphill battle. People do love their personal
pronouns in commit messages even if it reads weird. And don't get
me wrong - I used to do it too but tglx started with this passive
formulation and now I see how it is a lot less intrusive and keeps the
focus on the issue at hand.

> I don't really care much one way or the other.  I think the simplest
> approach is to remove QFLAG_APPLY_ONCE from intel_graphics_quirks()
> and do nothing else, as I suggested here:
> 
>   https://lore.kernel.org/r/20220113000805.GA295089@bhelgaas
> 
> Unfortunately that didn't occur to me until I'd already suggested more
> complicated things that no longer seem worthwhile to me.
> 
> The static variable might be ugly, but it does seem to be what
> intel_graphics_quirks() wants -- a "do this at most once per system
> but we don't know exactly which device" situation.

I see.

Yeah, keeping it solely inside intel_graphics_quirks() and maybe with a
comment ontop, why it is done, is simple. I guess if more quirks need
this once-thing people might have to consider a more sensible scheme - I
was just objecting to sprinkling those static vars everywhere.

But your call. :)

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [Intel-gfx] [PATCH v5 1/5] x86/quirks: Fix stolen detection with integrated + discrete GPU

[Bjorn Helgaas]

On Tue, Jan 18, 2022 at 07:37:29PM +0100, Borislav Petkov wrote:
> On Tue, Jan 18, 2022 at 11:58:53AM -0600, Bjorn Helgaas wrote:
> > I don't really care much one way or the other.  I think the simplest
> > approach is to remove QFLAG_APPLY_ONCE from intel_graphics_quirks()
> > and do nothing else, as I suggested here:
> > 
> >   https://lore.kernel.org/r/20220113000805.GA295089@bhelgaas
> > 
> > Unfortunately that didn't occur to me until I'd already suggested more
> > complicated things that no longer seem worthwhile to me.
> > 
> > The static variable might be ugly, but it does seem to be what
> > intel_graphics_quirks() wants -- a "do this at most once per system
> > but we don't know exactly which device" situation.
> 
> I see.
> 
> Yeah, keeping it solely inside intel_graphics_quirks() and maybe with a
> comment ontop, why it is done, is simple. I guess if more quirks need
> this once-thing people might have to consider a more sensible scheme - I
> was just objecting to sprinkling those static vars everywhere.
> 
> But your call. :)

Haha :)  I was hoping not to touch it myself because I think this
whole stolen memory thing is kind of nasty.  It's not clear to me why
we need it at all, or why we have to keep all this device-specific
logic in the kernel, or why it has to be an early quirk as opposed to
a regular PCI quirk.  We had a thread [1] about it a while ago but I
don't think anything got resolved.

But to try to make forward progress, I applied patch 1/5 (actually,
the updated one from [2]) to my pci/misc branch with the updated
commit log and code comments below.

IMO it's probably not worth doing patches 2-5 since I don't think
they fix anything and I'm not sure it improves readability.  I'm sorry
because it was my mistake to encourage that route initially.

I think we could still get this into v5.17 since it's small and the
merge window is still open.

[1] https://lore.kernel.org/linux-pci/20201104120506.172447-1-tejaskumarx.surendrakumar.upadhyay@intel.com/
[2] https://lore.kernel.org/r/20220118190558.2ququ4vdfjuahicm@ldmartin-desk2

commit 7329e2608d04 ("x86/gpu: Reserve stolen memory for first integrated Intel GPU")
Author: Lucas De Marchi <lucas.demarchi@intel.com>
Date:   Thu Jan 13 16:28:39 2022 -0800

    x86/gpu: Reserve stolen memory for first integrated Intel GPU
    
    "Stolen memory" is memory set aside for use by an Intel integrated GPU.
    The intel_graphics_quirks() early quirk reserves this memory when it is
    called for a GPU that appears in the intel_early_ids[] table of integrated
    GPUs.
    
    Previously intel_graphics_quirks() was marked as QFLAG_APPLY_ONCE, so it
    was called only for the first Intel GPU found.  If a discrete GPU happened
    to be enumerated first, intel_graphics_quirks() was called for it but not
    for any integrated GPU found later.  Therefore, stolen memory for such an
    integrated GPU was never reserved.
    
    For example, this problem occurs in this Alderlake-P (integrated) + DG2
    (discrete) topology where the DG2 is found first, but stolen memory is
    associated with the integrated GPU:
    
      - 00:01.0 Bridge
        `- 03:00.0 DG2 discrete GPU
      - 00:02.0 Integrated GPU (with stolen memory)
    
    Remove the QFLAG_APPLY_ONCE flag and call intel_graphics_quirks() for every
    Intel GPU.  Reserve stolen memory for the first GPU that appears in
    intel_early_ids[].
    
    [bhelgaas: commit log, add code comment, squash in
    https://lore.kernel.org/r/20220118190558.2ququ4vdfjuahicm@ldmartin-desk2]
    Link: https://lore.kernel.org/r/20220114002843.2083382-1-lucas.demarchi@intel.com
    Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
    Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
    Cc: stable@vger.kernel.org

diff --git a/arch/x86/kernel/early-quirks.c b/arch/x86/kernel/early-quirks.c
index 391a4e2b8604..8690fab95ae4 100644
--- a/arch/x86/kernel/early-quirks.c
+++ b/arch/x86/kernel/early-quirks.c
@@ -515,6 +515,7 @@ static const struct intel_early_ops gen11_early_ops __initconst = {
 	.stolen_size = gen9_stolen_size,
 };
 
+/* Intel integrated GPUs for which we need to reserve "stolen memory" */
 static const struct pci_device_id intel_early_ids[] __initconst = {
 	INTEL_I830_IDS(&i830_early_ops),
 	INTEL_I845G_IDS(&i845_early_ops),
@@ -591,6 +592,13 @@ static void __init intel_graphics_quirks(int num, int slot, int func)
 	u16 device;
 	int i;
 
+	/*
+	 * Reserve "stolen memory" for an integrated GPU.  If we've already
+	 * found one, there's nothing to do for other (discrete) GPUs.
+	 */
+	if (resource_size(&intel_graphics_stolen_res))
+		return;
+
 	device = read_pci_config_16(num, slot, func, PCI_DEVICE_ID);
 
 	for (i = 0; i < ARRAY_SIZE(intel_early_ids); i++) {
@@ -703,7 +711,7 @@ static struct chipset early_qrk[] __initdata = {
 	{ PCI_VENDOR_ID_INTEL, 0x3406, PCI_CLASS_BRIDGE_HOST,
 	  PCI_BASE_CLASS_BRIDGE, 0, intel_remapping_check },
 	{ PCI_VENDOR_ID_INTEL, PCI_ANY_ID, PCI_CLASS_DISPLAY_VGA, PCI_ANY_ID,
-	  QFLAG_APPLY_ONCE, intel_graphics_quirks },
+	  0, intel_graphics_quirks },
 	/*
 	 * HPET on the current version of the Baytrail platform has accuracy
 	 * problems: it will halt in deep idle state - so we disable it.

Re: [Intel-gfx] [PATCH v5 1/5] x86/quirks: Fix stolen detection with integrated + discrete GPU

[Borislav Petkov]

On Tue, Jan 18, 2022 at 02:01:45PM -0600, Bjorn Helgaas wrote:
> Haha :)  I was hoping not to touch it myself because I think this
> whole stolen memory thing is kind of nasty.  It's not clear to me why
> we need it at all, or why we have to keep all this device-specific
> logic in the kernel, or why it has to be an early quirk as opposed to
> a regular PCI quirk.  We had a thread [1] about it a while ago but I
> don't think anything got resolved.

/me goes and skims over it.

OMG, what a mess. And OEM BIOS is involved.

Makes me wanna run away screaming.

> But to try to make forward progress, I applied patch 1/5 (actually,
> the updated one from [2]) to my pci/misc branch with the updated
> commit log and code comments below.

Yap.

And if that stops the continuous stream of new-platform-addition
patches, even better.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [Intel-gfx] [PATCH v5 1/5] x86/quirks: Fix stolen detection with integrated + discrete GPU

[Lucas De Marchi]

On Tue, Jan 18, 2022 at 02:01:45PM -0600, Bjorn Helgaas wrote:
>On Tue, Jan 18, 2022 at 07:37:29PM +0100, Borislav Petkov wrote:
>> On Tue, Jan 18, 2022 at 11:58:53AM -0600, Bjorn Helgaas wrote:
>> > I don't really care much one way or the other.  I think the simplest
>> > approach is to remove QFLAG_APPLY_ONCE from intel_graphics_quirks()
>> > and do nothing else, as I suggested here:
>> >
>> >   https://lore.kernel.org/r/20220113000805.GA295089@bhelgaas
>> >
>> > Unfortunately that didn't occur to me until I'd already suggested more
>> > complicated things that no longer seem worthwhile to me.
>> >
>> > The static variable might be ugly, but it does seem to be what
>> > intel_graphics_quirks() wants -- a "do this at most once per system
>> > but we don't know exactly which device" situation.
>>
>> I see.
>>
>> Yeah, keeping it solely inside intel_graphics_quirks() and maybe with a
>> comment ontop, why it is done, is simple. I guess if more quirks need
>> this once-thing people might have to consider a more sensible scheme - I
>> was just objecting to sprinkling those static vars everywhere.
>>
>> But your call. :)
>
>Haha :)  I was hoping not to touch it myself because I think this
>whole stolen memory thing is kind of nasty.  It's not clear to me why
>we need it at all, or why we have to keep all this device-specific
>logic in the kernel, or why it has to be an early quirk as opposed to
>a regular PCI quirk.  We had a thread [1] about it a while ago but I
>don't think anything got resolved.

I was reading that thread again and thinking what we could do to try to
resolve this. I will reply on that thread.

>But to try to make forward progress, I applied patch 1/5 (actually,
>the updated one from [2]) to my pci/misc branch with the updated
>commit log and code comments below.

thanks. I found the wording in the title odd as when I read "first" it
gives me the impression it's saying there could be more, which is not
possible.  Anyway, not a big thing. Thanks for rewording it.

Lucas De Marchi

Re: [Intel-gfx] [PATCH v5 1/5] x86/quirks: Fix stolen detection with integrated + discrete GPU

[Bjorn Helgaas]

On Wed, Jan 19, 2022 at 12:30:04PM -0800, Lucas De Marchi wrote:
> On Tue, Jan 18, 2022 at 02:01:45PM -0600, Bjorn Helgaas wrote:

> > Haha :)  I was hoping not to touch it myself because I think this
> > whole stolen memory thing is kind of nasty.  It's not clear to me why
> > we need it at all, or why we have to keep all this device-specific
> > logic in the kernel, or why it has to be an early quirk as opposed to
> > a regular PCI quirk.  We had a thread [1] about it a while ago but I
> > don't think anything got resolved.
> 
> I was reading that thread again and thinking what we could do to try to
> resolve this. I will reply on that thread.

Great!  I hope there's some way around this.

> > But to try to make forward progress, I applied patch 1/5 (actually,
> > the updated one from [2]) to my pci/misc branch with the updated
> > commit log and code comments below.
> 
> thanks. I found the wording in the title odd as when I read "first" it
> gives me the impression it's saying there could be more, which is not
> possible.

I said "first integrated GPU" because Linux doesn't control what
devices are in the system; it just has to deal with whatever it finds.
All one can tell from the code is that if we find one or more devices
that appear in intel_early_ids[], we reserve stolen memory for the
first such device.

System-specific knowledge might tell you that there should only be one
integrated GPU, but there's no constraint like that in Linux.

Bjorn

Re: [PATCH v5 1/5] x86/quirks: Fix stolen detection with integrated + discrete GPU

[Lucas De Marchi]

On Tue, Jan 18, 2022 at 06:26:48PM +0100, Borislav Petkov wrote:
>On Tue, Jan 18, 2022 at 08:36:56AM -0800, Lucas De Marchi wrote:
>> I had the impression the subject/title should be imperative, with it
>> more relaxed in the body. It seems we have one more difference among
>> subsystems and I will adapt on next submissions to x86.
>
>We have written it down properly, in case it explains it better:
>
>"The tip tree maintainers set value on following these rules, especially
>on the request to write changelogs in imperative mood and not
>impersonating code or the execution of it. This is not just a whim of
>the maintainers. Changelogs written in abstract words are more precise
>and tend to be less confusing than those written in the form of novels."
>
>from Documentation/process/maintainer-tip.rst

nice, thanks. I had missed this. It certainly makes it easier to adapt
the style when crossing subystems

>> > So I wonder: why can't you simply pass in a static struct chipset *
>> > pointer into the early_qrk[i].f function and in there you can set
>> > QFLAG_APPLIED or so, so that you can mark that the quirk is applied by
>> > using the nice, per-quirk flags someone has already added instead of
>> > this ugly static variable?
>>
>> It seems you prefer v1. See 20211218061313.100571-1-lucas.demarchi@intel.com
>
>I do?
>
>I don't see there:
>
>	early_qrk[i].f(&early_qrk[i], num, slot, func)
>
>so that the ->f callback can set the flags. Or at least the flags passed
>in.

Indeed not exactly the same. In v1 we have

	applied = early_qrk[i].f(num, slot, func);

because I was trying to keep the logic that uses and the one that checks
the value in the same place. With your suggestion the logic to set the
flag would need to move to the called functions, while checking for the
flag would continue to be in the caller.

>If it is not clear what I mean, pls say so and I'll try to produce an
>example diff ontop.
>
>> Although in the review Bjorn suggested just splitting the commit, it was
>> also mentioned that the PCI subsystem has no such logic in its
>> equivalent pci_do_fixups(): a quirk/fixup needing that should instead
>> use a static local.
>
>Why?

I think to make it similar how the PCI fixups work. Anyway, do you
prefer that I change the QFLAG_APPLY_ONCE as above (including
nvidia_bugs() and ati_bugs()) or a very minimal fix like below and
nothing else?

------8<------
diff --git a/arch/x86/kernel/early-quirks.c b/arch/x86/kernel/early-quirks.c
index 391a4e2b8604..7b2a3230c42a 100644
--- a/arch/x86/kernel/early-quirks.c
+++ b/arch/x86/kernel/early-quirks.c
@@ -591,6 +591,13 @@ static void __init intel_graphics_quirks(int num, int slot, int func)
  	u16 device;
  	int i;
  
+	/*
+	 * Already reserved for integrated graphics, nothing to do for other
+	 * (discrete) cards.
+	 */
+	if (resource_size(&intel_graphics_stolen_res))
+		return;
+
  	device = read_pci_config_16(num, slot, func, PCI_DEVICE_ID);
  
  	for (i = 0; i < ARRAY_SIZE(intel_early_ids); i++) {
@@ -703,7 +710,7 @@ static struct chipset early_qrk[] __initdata = {
  	{ PCI_VENDOR_ID_INTEL, 0x3406, PCI_CLASS_BRIDGE_HOST,
  	  PCI_BASE_CLASS_BRIDGE, 0, intel_remapping_check },
  	{ PCI_VENDOR_ID_INTEL, PCI_ANY_ID, PCI_CLASS_DISPLAY_VGA, PCI_ANY_ID,
-	  QFLAG_APPLY_ONCE, intel_graphics_quirks },
+	  0, intel_graphics_quirks },
  	/*
  	 * HPET on the current version of the Baytrail platform has accuracy
  	 * problems: it will halt in deep idle state - so we disable it.
------8<------


thanks
Lucas De Marchi

Re: [PATCH v5 1/5] x86/quirks: Fix stolen detection with integrated + discrete GPU

[Borislav Petkov]

On Tue, Jan 18, 2022 at 11:05:58AM -0800, Lucas De Marchi wrote:
> I think to make it similar how the PCI fixups work. Anyway, do you
> prefer that I change the QFLAG_APPLY_ONCE as above (including
> nvidia_bugs() and ati_bugs()) or a very minimal fix like below and
> nothing else?
> 
> ------8<------
> diff --git a/arch/x86/kernel/early-quirks.c b/arch/x86/kernel/early-quirks.c
> index 391a4e2b8604..7b2a3230c42a 100644
> --- a/arch/x86/kernel/early-quirks.c
> +++ b/arch/x86/kernel/early-quirks.c
> @@ -591,6 +591,13 @@ static void __init intel_graphics_quirks(int num, int slot, int func)
>  	u16 device;
>  	int i;
> +	/*
> +	 * Already reserved for integrated graphics, nothing to do for other
> +	 * (discrete) cards.
> +	 */
> +	if (resource_size(&intel_graphics_stolen_res))
> +		return;
> +
>  	device = read_pci_config_16(num, slot, func, PCI_DEVICE_ID);
>  	for (i = 0; i < ARRAY_SIZE(intel_early_ids); i++) {
> @@ -703,7 +710,7 @@ static struct chipset early_qrk[] __initdata = {
>  	{ PCI_VENDOR_ID_INTEL, 0x3406, PCI_CLASS_BRIDGE_HOST,
>  	  PCI_BASE_CLASS_BRIDGE, 0, intel_remapping_check },
>  	{ PCI_VENDOR_ID_INTEL, PCI_ANY_ID, PCI_CLASS_DISPLAY_VGA, PCI_ANY_ID,
> -	  QFLAG_APPLY_ONCE, intel_graphics_quirks },
> +	  0, intel_graphics_quirks },
>  	/*
>  	 * HPET on the current version of the Baytrail platform has accuracy
>  	 * problems: it will halt in deep idle state - so we disable it.
> ------8<------

This looks even cleaner to me but that's Bjorn's call in the end.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette