agpgart: when telling user you'll corrupt his data, at least do it at KERN_CRIT

agpgart: when telling user you'll corrupt his data, at least do it at KERN_CRIT

[Pavel Machek]

IOMMU off means very bad stuff may happen, like data corruption on
your hard drives. At least tells users this is serious...

Plus, agp_amd64_init should go into .h file.

Signed-off-by: Pavel Machek <pavel@suse.cz>

diff --git a/arch/x86/kernel/pci-gart_64.c b/arch/x86/kernel/pci-gart_64.c
index faf3229..19e4f74 100644
--- a/arch/x86/kernel/pci-gart_64.c
+++ b/arch/x86/kernel/pci-gart_64.c
@@ -615,13 +615,11 @@ static __init int init_k8_gatt(struct ag
 
  nommu:
 	/* Should not happen anymore */
-	printk(KERN_ERR "PCI-DMA: More than 4GB of RAM and no IOMMU\n"
-	       KERN_ERR "PCI-DMA: 32bit PCI IO may malfunction.\n");
+	printk(KERN_CRIT "PCI-DMA: More than 4GB of RAM and no IOMMU\n"
+	       KERN_CRIT "PCI-DMA: 32bit PCI IO may malfunction.\n");
 	return -1;
 }
 
-extern int agp_amd64_init(void);
-
 static const struct dma_mapping_ops gart_dma_ops = {
 	.mapping_error			= NULL,
 	.map_single			= gart_map_single,
@@ -692,9 +690,9 @@ #endif
 	    !gart_iommu_aperture ||
 	    (no_agp && init_k8_gatt(&info) < 0)) {
 		if (end_pfn > MAX_DMA32_PFN) {
-			printk(KERN_ERR "WARNING more than 4GB of memory "
-					"but GART IOMMU not available.\n"
-			       KERN_ERR "WARNING 32bit PCI may malfunction.\n");
+			printk(KERN_CRIT "More than 4GB of memory "
+			       	          "but GART IOMMU not available.\n"
+			       KERN_CRIT "32bit PCI may malfunction.\n");
 		}
 		return;
 	}
diff --git a/include/asm-x86/agp.h b/include/asm-x86/agp.h
index e4004a9..657c25b 100644
--- a/include/asm-x86/agp.h
+++ b/include/asm-x86/agp.h
@@ -32,4 +32,7 @@ #define alloc_gatt_pages(order)		\
 #define free_gatt_pages(table, order)	\
 	free_pages((unsigned long)(table), (order))
 
+/* Only available in certain configs */
+extern int agp_amd64_init(void);
+
 #endif

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
pomozte zachranit klanovicky les:  http://www.ujezdskystrom.info/

Re: agpgart: when telling user you'll corrupt his data, at least do it at KERN_CRIT

[Andi Kleen]

On Fri, Mar 28, 2008 at 12:25:07PM +0100, Pavel Machek wrote:
> 
> IOMMU off means very bad stuff may happen, like data corruption on

Actually swiotlb takes over then in this case. The printk really
predates the introduction of swiotlb and could be actually removed.
That is why swiotlb is always SELECTed for gart too.

> your hard drives. At least tells users this is serious...

No there shouldn't be any data corruption with the default options.

Also especially hard disk drivers are expected (and generally do)
check the dma_map_sg() etc. return values and bail out if the IOMMU
fails e.g. due to overflow.

-Andi

Re: agpgart: when telling user you'll corrupt his data, at least do it at KERN_CRIT

[Ingo Molnar]

* Pavel Machek <pavel@ucw.cz> wrote:

> IOMMU off means very bad stuff may happen, like data corruption on 
> your hard drives. At least tells users this is serious...

ouch. Please at minimum lets turn this into a panic(), but best would be 
to trim memory in this case, hm?

	Ingo

Re: agpgart: when telling user you'll corrupt his data, at least do it at KERN_CRIT

[Jiri Kosina]

On Fri, 28 Mar 2008, Ingo Molnar wrote:

> > IOMMU off means very bad stuff may happen, like data corruption on 
> > your hard drives. At least tells users this is serious...
> ouch. Please at minimum lets turn this into a panic(), but best would be 
> to trim memory in this case, hm?

Are you suggesting to panic on all 64bit machines having more than 4G RAM 
without IOMMU turned on? Why, if swiotlb can still work nicely in such 
situations?

-- 
Jiri Kosina
SUSE Labs

Re: agpgart: when telling user you'll corrupt his data, at least do it at KERN_CRIT

[Ingo Molnar]

* Jiri Kosina <jkosina@suse.cz> wrote:

> On Fri, 28 Mar 2008, Ingo Molnar wrote:
> 
> > > IOMMU off means very bad stuff may happen, like data corruption on 
> > > your hard drives. At least tells users this is serious...
> > ouch. Please at minimum lets turn this into a panic(), but best would be 
> > to trim memory in this case, hm?
> 
> Are you suggesting to panic on all 64bit machines having more than 4G 
> RAM without IOMMU turned on? Why, if swiotlb can still work nicely in 
> such situations?

no, but we should avoid Pavel's disk corruption scenario ;-)

	Ingo

Re: agpgart: when telling user you'll corrupt his data, at least do it at KERN_CRIT

[Pavel Machek]

On Fri 2008-03-28 12:35:28, Ingo Molnar wrote:
> 
> * Pavel Machek <pavel@ucw.cz> wrote:
> 
> > IOMMU off means very bad stuff may happen, like data corruption on 
> > your hard drives. At least tells users this is serious...
> 
> ouch. Please at minimum lets turn this into a panic(), but best would be 
> to trim memory in this case, hm?

Andi tells me we already fallback to swiotlb, so messages are not on
wrong loglevel; they contain confusing/obsolete text. 

							Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: agpgart: when telling user you'll corrupt his data, at least do it at KERN_CRIT

[Ingo Molnar]

* Pavel Machek <pavel@ucw.cz> wrote:

> On Fri 2008-03-28 12:35:28, Ingo Molnar wrote:
> > 
> > * Pavel Machek <pavel@ucw.cz> wrote:
> > 
> > > IOMMU off means very bad stuff may happen, like data corruption on 
> > > your hard drives. At least tells users this is serious...
> > 
> > ouch. Please at minimum lets turn this into a panic(), but best would be 
> > to trim memory in this case, hm?
> 
> Andi tells me we already fallback to swiotlb, so messages are not on 
> wrong loglevel; they contain confusing/obsolete text.

yes that is the theory - but how did your disk get corrupted in 
practice? ;-)

	Ingo

Re: agpgart: when telling user you'll corrupt his data, at least do it at KERN_CRIT

[Andi Kleen]

> yes that is the theory - but how did your disk get corrupted in 
> practice? ;-)

I suspect after wakeup due to the wrong ordering in resume (agp resume function
called too late).

The IOMMU code unfortunately doesn't know that the GART has gone away. 

The right fix for that is probably to move the GART resume code into
some shared file and make it a sysdev.

-Andi

Re: agpgart: when telling user you'll corrupt his data, at least do it at KERN_CRIT

[Pavel Machek]

On Fri 2008-03-28 15:13:48, Ingo Molnar wrote:
> 
> * Pavel Machek <pavel@ucw.cz> wrote:
> 
> > On Fri 2008-03-28 12:35:28, Ingo Molnar wrote:
> > > 
> > > * Pavel Machek <pavel@ucw.cz> wrote:
> > > 
> > > > IOMMU off means very bad stuff may happen, like data corruption on 
> > > > your hard drives. At least tells users this is serious...
> > > 
> > > ouch. Please at minimum lets turn this into a panic(), but best would be 
> > > to trim memory in this case, hm?
> > 
> > Andi tells me we already fallback to swiotlb, so messages are not on 
> > wrong loglevel; they contain confusing/obsolete text.
> 
> yes that is the theory - but how did your disk get corrupted in 
> practice? ;-)

Well, if you try suspend/resume, without iommu suspend/resume support
(which is not there in some cases), you get nasty data
corruption. ("PCI DMA will does not work").

Then, I saw [obsolete] printk(KERN_ERR) that told me that PCI DMA will
not work, and happily continued to boot.

I'll fix the messages.
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

agpgart: scary messages are fortunately obsolete

[Pavel Machek]

Fix obsolete printks in aperture-64. We used not to handle missing
agpgart, but we handle it okay now.

Signed-off-by: Pavel Machek <pavel@suse.cz>

diff --git a/arch/x86/kernel/pci-gart_64.c b/arch/x86/kernel/pci-gart_64.c
index faf3229..3939c6d 100644
--- a/arch/x86/kernel/pci-gart_64.c
+++ b/arch/x86/kernel/pci-gart_64.c
@@ -615,13 +615,13 @@ static __init int init_k8_gatt(struct ag
 
  nommu:
 	/* Should not happen anymore */
-	printk(KERN_ERR "PCI-DMA: More than 4GB of RAM and no IOMMU\n"
-	       KERN_ERR "PCI-DMA: 32bit PCI IO may malfunction.\n");
+	printk(KERN_WARNING "PCI-DMA: More than 4GB of RAM and no IOMMU\n"
+	       KERN_WARNING "falling back to iommu=soft.\n");
 	return -1;
 }
 
 extern int agp_amd64_init(void);
 
 static const struct dma_mapping_ops gart_dma_ops = {
 	.mapping_error			= NULL,
 	.map_single			= gart_map_single,
@@ -692,9 +690,9 @@ #endif
 	    !gart_iommu_aperture ||
 	    (no_agp && init_k8_gatt(&info) < 0)) {
 		if (end_pfn > MAX_DMA32_PFN) {
-			printk(KERN_ERR "WARNING more than 4GB of memory "
-					"but GART IOMMU not available.\n"
-			       KERN_ERR "WARNING 32bit PCI may malfunction.\n");
+			printk(KERN_WARNING "More than 4GB of memory "
+			       	          "but GART IOMMU not available.\n"
+			       KERN_WARNING "falling back to iommu=soft.\n");
 		}
 		return;
 	}

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: agpgart: scary messages are fortunately obsolete

[Andi Kleen]

>  	/* Should not happen anymore */
> -	printk(KERN_ERR "PCI-DMA: More than 4GB of RAM and no IOMMU\n"
> -	       KERN_ERR "PCI-DMA: 32bit PCI IO may malfunction.\n");
> +	printk(KERN_WARNING "PCI-DMA: More than 4GB of RAM and no IOMMU\n"
> +	       KERN_WARNING "falling back to iommu=soft.\n");

It might also fall back to another IOMMU (Calgary, Intel, ...) 
soft is just last resort.

You can probably just remove it.

-Andi

Re: agpgart: scary messages are fortunately obsolete

[Ingo Molnar]

* Pavel Machek <pavel@ucw.cz> wrote:

> Fix obsolete printks in aperture-64. We used not to handle missing 
> agpgart, but we handle it okay now.

thanks Pavel, applied.

	Ingo