[PATCH] x86, aperture: Check for GART before accessing GART registers

[PATCH] x86, aperture: Check for GART before accessing GART registers

[Aravind Gopalakrishnan]

GART registers are not present in newer processors (Fam15h, Model 10h
and later). So, avoid accesses to GART registers in PCI config
space by returning early in early_gart_iommu_check() and
gart_iommu_hole_init() if GART is not available.

Refactoring the family check used in amd_nb into an inline function
so we can use it here as well as in amd_nb.c

Tested the patch on Fam10h and Fam15h Model 00h-fh and this code
runs fine. On Fam15h Model 60h-6fh and on Fam16h, we bail early
as they don't have GART.

Signed-off-by: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
Reviewed-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
---
 arch/x86/include/asm/amd_nb.h | 11 +++++++++++
 arch/x86/kernel/amd_nb.c      |  4 +---
 arch/x86/kernel/aperture_64.c |  4 ++--
 3 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/arch/x86/include/asm/amd_nb.h b/arch/x86/include/asm/amd_nb.h
index aaac3b2..864c8bd 100644
--- a/arch/x86/include/asm/amd_nb.h
+++ b/arch/x86/include/asm/amd_nb.h
@@ -98,11 +98,22 @@ static inline u16 amd_get_node_id(struct pci_dev *pdev)
 	return 0;
 }
 
+static inline bool amd_gart_present(void)
+{
+	/* GART present only on Fam15h upto model 0fh */
+	if (boot_cpu_data.x86 == 0xf || boot_cpu_data.x86 == 0x10 ||
+	    (boot_cpu_data.x86 == 0x15 && boot_cpu_data.x86_model < 0x10))
+		return true;
+
+	return false;
+}
+
 #else
 
 #define amd_nb_num(x)		0
 #define amd_nb_has_feature(x)	false
 #define node_to_amd_nb(x)	NULL
+#define amd_gart_present(x)	false
 
 #endif
 
diff --git a/arch/x86/kernel/amd_nb.c b/arch/x86/kernel/amd_nb.c
index 5caed1d..29fa475 100644
--- a/arch/x86/kernel/amd_nb.c
+++ b/arch/x86/kernel/amd_nb.c
@@ -89,9 +89,7 @@ int amd_cache_northbridges(void)
 			next_northbridge(link, amd_nb_link_ids);
 	}
 
-	/* GART present only on Fam15h upto model 0fh */
-	if (boot_cpu_data.x86 == 0xf || boot_cpu_data.x86 == 0x10 ||
-	    (boot_cpu_data.x86 == 0x15 && boot_cpu_data.x86_model < 0x10))
+	if (amd_gart_present())
 		amd_northbridges.flags |= AMD_NB_GART;
 
 	/*
diff --git a/arch/x86/kernel/aperture_64.c b/arch/x86/kernel/aperture_64.c
index 76164e1..1cb170b 100644
--- a/arch/x86/kernel/aperture_64.c
+++ b/arch/x86/kernel/aperture_64.c
@@ -262,7 +262,7 @@ void __init early_gart_iommu_check(void)
 	u64 aper_base = 0, last_aper_base = 0;
 	int aper_enabled = 0, last_aper_enabled = 0, last_valid = 0;
 
-	if (!early_pci_allowed())
+	if (!early_pci_allowed() || !amd_gart_present())
 		return;
 
 	/* This is mostly duplicate of iommu_hole_init */
@@ -356,7 +356,7 @@ int __init gart_iommu_hole_init(void)
 	int i, node;
 
 	if (gart_iommu_aperture_disabled || !fix_aperture ||
-	    !early_pci_allowed())
+	    !early_pci_allowed() || !amd_gart_present())
 		return -ENODEV;
 
 	pr_info("Checking aperture...\n");
-- 
1.9.1

Re: [PATCH] x86, aperture: Check for GART before accessing GART registers

[Borislav Petkov]

On Wed, Apr 01, 2015 at 09:32:08AM -0500, Aravind Gopalakrishnan wrote:
> GART registers are not present in newer processors (Fam15h, Model 10h
> and later). So, avoid accesses to GART registers in PCI config
> space by returning early in early_gart_iommu_check() and
> gart_iommu_hole_init() if GART is not available.
> 
> Refactoring the family check used in amd_nb into an inline function
> so we can use it here as well as in amd_nb.c
> 
> Tested the patch on Fam10h and Fam15h Model 00h-fh and this code
> runs fine. On Fam15h Model 60h-6fh and on Fam16h, we bail early
> as they don't have GART.
> 
> Signed-off-by: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
> Reviewed-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
> ---
>  arch/x86/include/asm/amd_nb.h | 11 +++++++++++
>  arch/x86/kernel/amd_nb.c      |  4 +---
>  arch/x86/kernel/aperture_64.c |  4 ++--
>  3 files changed, 14 insertions(+), 5 deletions(-)

Applied, thanks.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [PATCH] x86, aperture: Check for GART before accessing GART registers

[Ingo Molnar]

* Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com> wrote:

> GART registers are not present in newer processors (Fam15h, Model 10h
> and later). So, avoid accesses to GART registers in PCI config
> space by returning early in early_gart_iommu_check() and
> gart_iommu_hole_init() if GART is not available.

In what fashion did this problem manifest itself on real systems?

Thanks,

	Ingo

Re: [PATCH] x86, aperture: Check for GART before accessing GART registers

[Aravind Gopalakrishnan]

On 4/2/2015 5:01 AM, Ingo Molnar wrote:
> * Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com> wrote:
>
>> GART registers are not present in newer processors (Fam15h, Model 10h
>> and later). So, avoid accesses to GART registers in PCI config
>> space by returning early in early_gart_iommu_check() and
>> gart_iommu_hole_init() if GART is not available.
> In what fashion did this problem manifest itself on real systems?
>
>

This code doesn't break on existing processors.
There are some other side effects though..

We get "AGP:" messages on kernel logs like this-
[    0.000000] AGP: Node 0: aperture [bus addr 0x00000000-0x01ffffff] (32MB)
[    0.000000] AGP: Your BIOS doesn't leave a aperture memory hole
[    0.000000] AGP: Please enable the IOMMU option in the BIOS setup
[    0.000000] AGP: This costs you 64MB of RAM
[    0.000000] AGP: Mapping aperture over RAM [mem 
0xd4000000-0xd7ffffff] (65536KB)

These are just noise on processors which have no GART.
We can avoid calling allocate_aperture() and would not have to 
memblock_reserve() 64MB of RAM.
Also, we can avoid having to loop through all PCI buses, devices (twice) 
searching for AGP bridge if we bail out early.

For future processors though, we can't say how these registers will be 
implemented.
If we have to provide PCI IDs for them in amd_nb_misc_ids[] and this 
code ends up reading/writing something else then that'd be another problem.

Thanks,
-Aravind.

Re: [PATCH] x86, aperture: Check for GART before accessing GART registers

[Ingo Molnar]

* Aravind Gopalakrishnan <aravind.gopalakrishnan@amd.com> wrote:

> On 4/2/2015 5:01 AM, Ingo Molnar wrote:
> >* Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com> wrote:
> >
> >>GART registers are not present in newer processors (Fam15h, Model 10h
> >>and later). So, avoid accesses to GART registers in PCI config
> >>space by returning early in early_gart_iommu_check() and
> >>gart_iommu_hole_init() if GART is not available.
> >In what fashion did this problem manifest itself on real systems?
> >
> >
> 
> This code doesn't break on existing processors.
> There are some other side effects though..
> 
> We get "AGP:" messages on kernel logs like this-
> [    0.000000] AGP: Node 0: aperture [bus addr 0x00000000-0x01ffffff] (32MB)
> [    0.000000] AGP: Your BIOS doesn't leave a aperture memory hole
> [    0.000000] AGP: Please enable the IOMMU option in the BIOS setup
> [    0.000000] AGP: This costs you 64MB of RAM
> [    0.000000] AGP: Mapping aperture over RAM [mem 0xd4000000-0xd7ffffff]
> (65536KB)
> 
> These are just noise on processors which have no GART.

agreed.

> We can avoid calling allocate_aperture() and would not have to
> memblock_reserve() 64MB of RAM.
> Also, we can avoid having to loop through all PCI buses, devices (twice)
> searching for AGP bridge if we bail out early.

Makes sense. Mind adding this info to the changelog and resend?

Thanks,

	Ingo

Re: [PATCH] x86, aperture: Check for GART before accessing GART registers

[Aravind Gopalakrishnan]

On 4/2/2015 11:06 AM, Ingo Molnar wrote:
>
>
> We get "AGP:" messages on kernel logs like this-
> [    0.000000] AGP: Node 0: aperture [bus addr 0x00000000-0x01ffffff] (32MB)
> [    0.000000] AGP: Your BIOS doesn't leave a aperture memory hole
> [    0.000000] AGP: Please enable the IOMMU option in the BIOS setup
> [    0.000000] AGP: This costs you 64MB of RAM
> [    0.000000] AGP: Mapping aperture over RAM [mem 0xd4000000-0xd7ffffff]
> (65536KB)
>
> These are just noise on processors which have no GART.
> agreed.
>
>> We can avoid calling allocate_aperture() and would not have to
>> memblock_reserve() 64MB of RAM.
>> Also, we can avoid having to loop through all PCI buses, devices (twice)
>> searching for AGP bridge if we bail out early.
> Makes sense. Mind adding this info to the changelog and resend?
>
>

Sure, will do that and resend.

Thanks,
-Aravind.

Re: [PATCH] x86, aperture: Check for GART before accessing GART registers

[Borislav Petkov]

On Thu, Apr 02, 2015 at 11:23:17AM -0500, Aravind Gopalakrishnan wrote:
> Sure, will do that and resend.

No need - I can amend the local copy I have here.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [PATCH] x86, aperture: Check for GART before accessing GART registers

[Aravind Gopalakrishnan]

On 4/2/2015 11:53 AM, Borislav Petkov wrote:
> On Thu, Apr 02, 2015 at 11:23:17AM -0500, Aravind Gopalakrishnan wrote:
>> Sure, will do that and resend.
> No need - I can amend the local copy I have here.
>

Okay.

Thanks!
-Aravind.

Re: [PATCH] x86, aperture: Check for GART before accessing GART registers

[Borislav Petkov]

On Thu, Apr 02, 2015 at 12:04:21PM -0500, Aravind Gopalakrishnan wrote:
> >No need - I can amend the local copy I have here.

Here's what I did:

---
From: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
Date: Wed, 1 Apr 2015 09:32:08 -0500
Subject: [PATCH] x86/gart: Check for GART support before accessing GART
 registers
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

GART registers are not present in newer AMD processors (Fam15h,
Model 10h and later). So, avoid accesses to GART registers in PCI
config space by returning early in early_gart_iommu_check() and
gart_iommu_hole_init() if GART is not available.

Current code doesn't break on existing processors but there are some
side effects:

We get bogus AGP aperture messages which are simply noise on
GART-less processors:

  AGP: Node 0: aperture [bus addr 0x00000000-0x01ffffff] (32MB)
  AGP: Your BIOS doesn't leave a aperture memory hole
  AGP: Please enable the IOMMU option in the BIOS setup
  AGP: This costs you 64MB of RAM
  AGP: Mapping aperture over RAM [mem 0xd4000000-0xd7ffffff]

We can avoid calling allocate_aperture() and would not have to
wastefully reserve 64MB of RAM with memblock_reserve(). Also, we can
avoid having to loop through all PCI buses and devices twice, searching
for a non-existent AGP bridge if we bail out early.

Refactor the family check used in amd_nb into an inline function so we
can use it here as well as in amd_nb.c.

Tested the patch on Fam10h and Fam15h Model 00h-fh and this code runs
fine. On Fam15h, Models 60h-6fh and on Fam16h, we bail out early as they
don't have a GART.

Signed-off-by: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
Reviewed-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
Cc: Jörg Rödel <joro@8bytes.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: x86-ml <x86@kernel.org>
Link: http://lkml.kernel.org/r/1427898728-3434-1-git-send-email-Aravind.Gopalakrishnan@amd.com
Signed-off-by: 
---
 arch/x86/include/asm/amd_nb.h | 11 +++++++++++
 arch/x86/kernel/amd_nb.c      |  4 +---
 arch/x86/kernel/aperture_64.c |  4 ++--
 3 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/arch/x86/include/asm/amd_nb.h b/arch/x86/include/asm/amd_nb.h
index aaac3b2fb746..864c8bd8cbe4 100644
--- a/arch/x86/include/asm/amd_nb.h
+++ b/arch/x86/include/asm/amd_nb.h
@@ -98,11 +98,22 @@ static inline u16 amd_get_node_id(struct pci_dev *pdev)
 	return 0;
 }
 
+static inline bool amd_gart_present(void)
+{
+	/* GART present only on Fam15h upto model 0fh */
+	if (boot_cpu_data.x86 == 0xf || boot_cpu_data.x86 == 0x10 ||
+	    (boot_cpu_data.x86 == 0x15 && boot_cpu_data.x86_model < 0x10))
+		return true;
+
+	return false;
+}
+
 #else
 
 #define amd_nb_num(x)		0
 #define amd_nb_has_feature(x)	false
 #define node_to_amd_nb(x)	NULL
+#define amd_gart_present(x)	false
 
 #endif
 
diff --git a/arch/x86/kernel/amd_nb.c b/arch/x86/kernel/amd_nb.c
index 5caed1dd7ccf..29fa475ec518 100644
--- a/arch/x86/kernel/amd_nb.c
+++ b/arch/x86/kernel/amd_nb.c
@@ -89,9 +89,7 @@ int amd_cache_northbridges(void)
 			next_northbridge(link, amd_nb_link_ids);
 	}
 
-	/* GART present only on Fam15h upto model 0fh */
-	if (boot_cpu_data.x86 == 0xf || boot_cpu_data.x86 == 0x10 ||
-	    (boot_cpu_data.x86 == 0x15 && boot_cpu_data.x86_model < 0x10))
+	if (amd_gart_present())
 		amd_northbridges.flags |= AMD_NB_GART;
 
 	/*
diff --git a/arch/x86/kernel/aperture_64.c b/arch/x86/kernel/aperture_64.c
index 76164e173a24..1cb170b06853 100644
--- a/arch/x86/kernel/aperture_64.c
+++ b/arch/x86/kernel/aperture_64.c
@@ -262,7 +262,7 @@ void __init early_gart_iommu_check(void)
 	u64 aper_base = 0, last_aper_base = 0;
 	int aper_enabled = 0, last_aper_enabled = 0, last_valid = 0;
 
-	if (!early_pci_allowed())
+	if (!early_pci_allowed() || !amd_gart_present())
 		return;
 
 	/* This is mostly duplicate of iommu_hole_init */
@@ -356,7 +356,7 @@ int __init gart_iommu_hole_init(void)
 	int i, node;
 
 	if (gart_iommu_aperture_disabled || !fix_aperture ||
-	    !early_pci_allowed())
+	    !early_pci_allowed() || !amd_gart_present())
 		return -ENODEV;
 
 	pr_info("Checking aperture...\n");
-- 
2.3.3

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [PATCH] x86, aperture: Check for GART before accessing GART registers

[Ingo Molnar]

* Borislav Petkov <bp@suse.de> wrote:

> On Thu, Apr 02, 2015 at 12:04:21PM -0500, Aravind Gopalakrishnan wrote:
> > >No need - I can amend the local copy I have here.
> 
> Here's what I did:
> 
> ---
> From: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
> Date: Wed, 1 Apr 2015 09:32:08 -0500
> Subject: [PATCH] x86/gart: Check for GART support before accessing GART
>  registers
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
> 
> GART registers are not present in newer AMD processors (Fam15h,
> Model 10h and later). So, avoid accesses to GART registers in PCI
> config space by returning early in early_gart_iommu_check() and
> gart_iommu_hole_init() if GART is not available.
> 
> Current code doesn't break on existing processors but there are some
> side effects:
> 
> We get bogus AGP aperture messages which are simply noise on
> GART-less processors:
> 
>   AGP: Node 0: aperture [bus addr 0x00000000-0x01ffffff] (32MB)
>   AGP: Your BIOS doesn't leave a aperture memory hole

hah, someone should fix the typo in that message:

	s/a a/a

>   AGP: Please enable the IOMMU option in the BIOS setup
>   AGP: This costs you 64MB of RAM
>   AGP: Mapping aperture over RAM [mem 0xd4000000-0xd7ffffff]
> 
> We can avoid calling allocate_aperture() and would not have to
> wastefully reserve 64MB of RAM with memblock_reserve(). Also, we can
> avoid having to loop through all PCI buses and devices twice, searching
> for a non-existent AGP bridge if we bail out early.
> 
> Refactor the family check used in amd_nb into an inline function so we

s/amd_nb/amd_nb.c

>  
> +static inline bool amd_gart_present(void)
> +{
> +	/* GART present only on Fam15h upto model 0fh */

s/h u/h, u

> +	if (amd_gart_present())
>  		amd_northbridges.flags |= AMD_NB_GART;
>  
>  	/*
> diff --git a/arch/x86/kernel/aperture_64.c b/arch/x86/kernel/aperture_64.c
> index 76164e173a24..1cb170b06853 100644
> --- a/arch/x86/kernel/aperture_64.c
> +++ b/arch/x86/kernel/aperture_64.c
> @@ -262,7 +262,7 @@ void __init early_gart_iommu_check(void)
>  	u64 aper_base = 0, last_aper_base = 0;
>  	int aper_enabled = 0, last_aper_enabled = 0, last_valid = 0;
>  
> -	if (!early_pci_allowed())
> +	if (!early_pci_allowed() || !amd_gart_present())
>  		return;
>  
>  	/* This is mostly duplicate of iommu_hole_init */
> @@ -356,7 +356,7 @@ int __init gart_iommu_hole_init(void)
>  	int i, node;
>  
>  	if (gart_iommu_aperture_disabled || !fix_aperture ||
> -	    !early_pci_allowed())
> +	    !early_pci_allowed() || !amd_gart_present())
>  		return -ENODEV;

So what happens if !early_pci_allowed() but the GART is present? We'll 
set amd_northbridges.flags |= AMD_NB_GART, but won't run any of the 
setup code in aperture_64.c, right? Is that a valid setup?

Thanks,

	Ingo

Re: [PATCH] x86, aperture: Check for GART before accessing GART registers

[Aravind Gopalakrishnan]

On 4/2/2015 1:19 PM, Ingo Molnar wrote:

>
>> +	if (amd_gart_present())
>>   		amd_northbridges.flags |= AMD_NB_GART;
>>   
>>   	/*
>> diff --git a/arch/x86/kernel/aperture_64.c b/arch/x86/kernel/aperture_64.c
>> index 76164e173a24..1cb170b06853 100644
>> --- a/arch/x86/kernel/aperture_64.c
>> +++ b/arch/x86/kernel/aperture_64.c
>> @@ -262,7 +262,7 @@ void __init early_gart_iommu_check(void)
>>   	u64 aper_base = 0, last_aper_base = 0;
>>   	int aper_enabled = 0, last_aper_enabled = 0, last_valid = 0;
>>   
>> -	if (!early_pci_allowed())
>> +	if (!early_pci_allowed() || !amd_gart_present())
>>   		return;
>>   
>>   	/* This is mostly duplicate of iommu_hole_init */
>> @@ -356,7 +356,7 @@ int __init gart_iommu_hole_init(void)
>>   	int i, node;
>>   
>>   	if (gart_iommu_aperture_disabled || !fix_aperture ||
>> -	    !early_pci_allowed())
>> +	    !early_pci_allowed() || !amd_gart_present())
>>   		return -ENODEV;
> So what happens if !early_pci_allowed() but the GART is present? We'll
> set amd_northbridges.flags |= AMD_NB_GART, but won't run any of the
> setup code in aperture_64.c, right? Is that a valid setup?
>

It might be a valid setup. But it would work correctly only if BIOS did 
the right thing with setting up aperture space.

If !early_pci_allowed() and BIOS did not setup aperture correctly, that 
would have caused problems already.
But it has not been an issue so far right?

Thanks,
-Aravind.

Re: [PATCH] x86, aperture: Check for GART before accessing GART registers

[Borislav Petkov]

On Mon, Apr 06, 2015 at 06:10:22PM -0500, Aravind Gopalakrishnan wrote:
> >So what happens if !early_pci_allowed() but the GART is present? We'll
> >set amd_northbridges.flags |= AMD_NB_GART, but won't run any of the
> >setup code in aperture_64.c, right? Is that a valid setup?
> 
> It might be a valid setup. But it would work correctly only if BIOS did the
> right thing with setting up aperture space.
> 
> If !early_pci_allowed() and BIOS did not setup aperture correctly, that
> would have caused problems already.
> But it has not been an issue so far right?

I think the right question to ask is:

What are we doing when there's no GART?

So what you probably want to do is:

	if (!amd_gart_present())
		return -ENODEV;

*before* the compound check.

Now, if we have detected a GART, i.e., amd_gart_present() is TRUE, the
code will behave as it used to behave before.

This should be the least intrusive change going forward IMHO.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [PATCH] x86, aperture: Check for GART before accessing GART registers

[Aravind Gopalakrishnan]

On 4/7/2015 7:34 AM, Borislav Petkov wrote:
> On Mon, Apr 06, 2015 at 06:10:22PM -0500, Aravind Gopalakrishnan wrote:
>>> So what happens if !early_pci_allowed() but the GART is present? We'll
>>> set amd_northbridges.flags |= AMD_NB_GART, but won't run any of the
>>> setup code in aperture_64.c, right? Is that a valid setup?
>> It might be a valid setup. But it would work correctly only if BIOS did the
>> right thing with setting up aperture space.
>>
>> If !early_pci_allowed() and BIOS did not setup aperture correctly, that
>> would have caused problems already.
>> But it has not been an issue so far right?
> I think the right question to ask is:
>
> What are we doing when there's no GART?
>
> So what you probably want to do is:
>
> 	if (!amd_gart_present())
> 		return -ENODEV;
>
> *before* the compound check.
>
> Now, if we have detected a GART, i.e., amd_gart_present() is TRUE, the
> code will behave as it used to behave before.
>
> This should be the least intrusive change going forward IMHO.
>

Okay. I'll do that and correct the typos Ingo pointed out earlier and 
resend.

Thanks,
-Aravind.

Re: [PATCH] x86, aperture: Check for GART before accessing GART registers

[Borislav Petkov]

On Tue, Apr 07, 2015 at 09:46:26AM -0500, Aravind Gopalakrishnan wrote:
> Okay. I'll do that and correct the typos Ingo pointed out earlier and
> resend.

Btw, I think you should do the same in early_gart_iommu_check() too.

Doing the testing this way would mean that we first are testing for GART
hw presence and then do the rest of checks. If no GART hw, the rest of
the checks are meaningless.

Also, when testing do a "pci=noearly" boot which should make

	!early_pci_allowed()

true and thus test that path too.

Thanks.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [PATCH] x86, aperture: Check for GART before accessing GART registers

[Aravind Gopalakrishnan]

On 4/7/2015 9:57 AM, Borislav Petkov wrote:
> On Tue, Apr 07, 2015 at 09:46:26AM -0500, Aravind Gopalakrishnan wrote:
>> Okay. I'll do that and correct the typos Ingo pointed out earlier and
>> resend.
> Btw, I think you should do the same in early_gart_iommu_check() too.
>
> Doing the testing this way would mean that we first are testing for GART
> hw presence and then do the rest of checks. If no GART hw, the rest of
> the checks are meaningless.
>
> Also, when testing do a "pci=noearly" boot which should make
>
> 	!early_pci_allowed()
>
> true and thus test that path too.
>

Here are results from further testing:
1. on platforms with both iommu and gart
     - with pci=noearly, we break out of init routines in aperture_64.c 
early. amd_iommu_init() will run through it's init routine.
         - if amd_iommu_init() fails somewhere, we fall back to 
gart_iommu_init()
         - gart_iommu_init() fails since gart_iommu_aperture is not set.
         - fall back to swiotlb.
     - with amd_iommu=off
         - init routines in aperture_64.c run fine as both 
amd_gart_present() and early_pci_allowed() are true
         -  amd_iommu_detect() fails due to command line arg.
         - fall back to gart iommu
     - with pci=noearly and amd_iommu=off
         - break out of aperture_64.c init routines, and 
amd_iommu_detect() fails.
         - fall back to swiotlb

2. on platform with no gart but iommu present,
     - pci=noearly option is not relevant as we break before that due to 
!amd_gart_present()
     - if amd_iommu_init() fails somewhere, we fall back to swiotlb, 
else use iommu

3. on platforms with no gart and no iommu
     - use swiotlb regardless of any command line options passed

Thanks,
-Aravind.

Re: [PATCH] x86, aperture: Check for GART before accessing GART registers

[Borislav Petkov]

On Tue, Apr 07, 2015 at 03:34:19PM -0500, Aravind Gopalakrishnan wrote:
> On 4/7/2015 9:57 AM, Borislav Petkov wrote:
> >On Tue, Apr 07, 2015 at 09:46:26AM -0500, Aravind Gopalakrishnan wrote:
> >>Okay. I'll do that and correct the typos Ingo pointed out earlier and
> >>resend.
> >Btw, I think you should do the same in early_gart_iommu_check() too.
> >
> >Doing the testing this way would mean that we first are testing for GART
> >hw presence and then do the rest of checks. If no GART hw, the rest of
> >the checks are meaningless.
> >
> >Also, when testing do a "pci=noearly" boot which should make
> >
> >	!early_pci_allowed()
> >
> >true and thus test that path too.
> >
> 
> Here are results from further testing:
> 1. on platforms with both iommu and gart
>     - with pci=noearly, we break out of init routines in aperture_64.c
> early. amd_iommu_init() will run through it's init routine.
>         - if amd_iommu_init() fails somewhere, we fall back to
> gart_iommu_init()
>         - gart_iommu_init() fails since gart_iommu_aperture is not set.
>         - fall back to swiotlb.
>     - with amd_iommu=off
>         - init routines in aperture_64.c run fine as both amd_gart_present()
> and early_pci_allowed() are true
>         -  amd_iommu_detect() fails due to command line arg.
>         - fall back to gart iommu
>     - with pci=noearly and amd_iommu=off
>         - break out of aperture_64.c init routines, and amd_iommu_detect()
> fails.
>         - fall back to swiotlb
> 
> 2. on platform with no gart but iommu present,
>     - pci=noearly option is not relevant as we break before that due to
> !amd_gart_present()
>     - if amd_iommu_init() fails somewhere, we fall back to swiotlb, else use
> iommu
> 
> 3. on platforms with no gart and no iommu
>     - use swiotlb regardless of any command line options passed

I don't see anything out of the ordinary but then again I don't know
this code so...

@joro, can you please double-check?

Thanks.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [PATCH] x86, aperture: Check for GART before accessing GART registers

[Jörg-Volker Peetz]

Hi,

tried this patch on my HP Pavilion notebook wiht AMD Phenom II processor. The
BIOS has no IOMMU option.
With this patch I'm still seeing the AGP messages mention in the patch description.
Is this processor affected at all? /proc/cpuinfo says "cpu family" 16. That is 10h?

Regards,
jvp.


Borislav Petkov wrote on 04/02/2015 19:17:
> On Thu, Apr 02, 2015 at 12:04:21PM -0500, Aravind Gopalakrishnan wrote:
>>> No need - I can amend the local copy I have here.
> 
> Here's what I did:
> 
> ---
> From: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
> Date: Wed, 1 Apr 2015 09:32:08 -0500
> Subject: [PATCH] x86/gart: Check for GART support before accessing GART
>  registers
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
> 
> GART registers are not present in newer AMD processors (Fam15h,
> Model 10h and later). So, avoid accesses to GART registers in PCI
> config space by returning early in early_gart_iommu_check() and
> gart_iommu_hole_init() if GART is not available.
> 
> Current code doesn't break on existing processors but there are some
> side effects:
> 
> We get bogus AGP aperture messages which are simply noise on
> GART-less processors:
> 
>   AGP: Node 0: aperture [bus addr 0x00000000-0x01ffffff] (32MB)
>   AGP: Your BIOS doesn't leave a aperture memory hole
>   AGP: Please enable the IOMMU option in the BIOS setup
>   AGP: This costs you 64MB of RAM
>   AGP: Mapping aperture over RAM [mem 0xd4000000-0xd7ffffff]
> 
<snip>

Re: [PATCH] x86, aperture: Check for GART before accessing GART registers

[Borislav Petkov]

On Tue, Apr 14, 2015 at 01:15:43PM +0200, Jörg-Volker Peetz wrote:
> Hi,

Hi,

please do not top-post.

> tried this patch on my HP Pavilion notebook wiht AMD Phenom II processor. The
> BIOS has no IOMMU option.
> With this patch I'm still seeing the AGP messages mention in the patch description.
> Is this processor affected at all?

Your CPU has an AGP bridge but the BIOS doesn't leave an aperture so the
kernel has to do it. Read the message.

> /proc/cpuinfo says "cpu family" 16. That is 10h?

Yes.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [PATCH] x86, aperture: Check for GART before accessing GART registers

[Jörg-Volker Peetz]

Hi,

Borislav Petkov wrote on 04/14/2015 13:33:
> On Tue, Apr 14, 2015 at 01:15:43PM +0200, Jörg-Volker Peetz wrote:
>> Hi,
> 
> Hi,
> 
> please do not top-post.
> 
>> tried this patch on my HP Pavilion notebook wiht AMD Phenom II processor. The
>> BIOS has no IOMMU option.
>> With this patch I'm still seeing the AGP messages mention in the patch description.
>> Is this processor affected at all?
> 
> Your CPU has an AGP bridge but the BIOS doesn't leave an aperture so the
> kernel has to do it. Read the message.
> 
>> /proc/cpuinfo says "cpu family" 16. That is 10h?
> 
> Yes.
> 

thank you for the explanation. Somehow I thought I had a newer CPU w/o GART
registers, but I misread the patch. Maybe wishful thinking, in the hope to save
64 MB :-(
Sorry for the noise.
-- 
Regards,
jvp.

Re: [PATCH] x86, aperture: Check for GART before accessing GART registers

[Borislav Petkov]

On Tue, Apr 14, 2015 at 08:22:48PM +0200, Jörg-Volker Peetz wrote:
> thank you for the explanation. Somehow I thought I had a newer CPU w/o
> GART registers, but I misread the patch. Maybe wishful thinking, in
> the hope to save 64 MB :-(

64MB out of how much? Does it even matter?

You could turn off CONFIG_GART_IOMMU and not build aperture_64.c at all.

I think then you'll fall back to swiotlb or whatever and that will most
likely slow down DMA ... the story was something like that, at least,
memory's hazy. Joerg's on CC, he'll correct me if I'm talking bull.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [PATCH] x86, aperture: Check for GART before accessing GART registers

[Joerg Roedel]

On Tue, Apr 14, 2015 at 08:36:43PM +0200, Borislav Petkov wrote:
> 64MB out of how much? Does it even matter?
> 
> You could turn off CONFIG_GART_IOMMU and not build aperture_64.c at all.
> 
> I think then you'll fall back to swiotlb or whatever and that will most
> likely slow down DMA ... the story was something like that, at least,
> memory's hazy. Joerg's on CC, he'll correct me if I'm talking bull.

Using SWIOTLB will also eat 64MB of RAM, so it doesn't matter if you use
it instead of the GART driver. So whether your machine has GART or not,
it will cost you 64MB of RAM for DMA remapping (unless you have a real
IOMMU).


	Joerg