[RFC patch] init default dma_ops to prepare intel_iommu_init failure

[RFC patch] init default dma_ops to prepare intel_iommu_init failure

[Luming Yu]

[-- Attachment #1: Type: text/plain, Size: 1350 bytes --]

Hi there,

On ia64 platform with dmar table and new upstream
kernel(config_dmar_default_on enabled),
if intel_iommu_init ends up without dma_ops initialized or other iommu
failures.
We got nothing to fall back. The following patch fixes it.

Please review. If make sense, please apply.

**The patch is enclosed in text attachment*
**Using web client to send the patch* *
**below is for review, please apply attached  patch*/

Thanks,
Luming

Signed-off-by: Yu Luming <luming.yu@intel.com>

arch/ia64/kernel/pci-swiotlb.c |   14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/arch/ia64/kernel/pci-swiotlb.c b/arch/ia64/kernel/pci-swiotlb.c
index 223abb1..d5dcaf5 100644
--- a/arch/ia64/kernel/pci-swiotlb.c
+++ b/arch/ia64/kernel/pci-swiotlb.c
@@ -46,15 +46,13 @@ void __init swiotlb_dma_init(void)

 void __init pci_swiotlb_init(void)
 {
-	if (!iommu_detected || iommu_pass_through) {
 #ifdef CONFIG_IA64_GENERIC
-		swiotlb = 1;
-		printk(KERN_INFO "PCI-DMA: Re-initialize machine vector.\n");
-		machvec_init("dig");
-		swiotlb_init();
-		dma_ops = &swiotlb_dma_ops;
+	swiotlb = 1;
+	printk(KERN_INFO "PCI-DMA: Re-initialize machine vector.\n");
+	machvec_init("dig");
+	swiotlb_init();
+	dma_ops = &swiotlb_dma_ops;
 #else
-		panic("Unable to find Intel IOMMU");
+	panic("Unable to find Intel IOMMU");
 #endif
-	}
 }

[-- Attachment #2: 5 --]
[-- Type: application/octet-stream, Size: 758 bytes --]

diff --git a/arch/ia64/kernel/pci-swiotlb.c b/arch/ia64/kernel/pci-swiotlb.c
index 223abb1..d5dcaf5 100644
--- a/arch/ia64/kernel/pci-swiotlb.c
+++ b/arch/ia64/kernel/pci-swiotlb.c
@@ -46,15 +46,13 @@ void __init swiotlb_dma_init(void)
 
 void __init pci_swiotlb_init(void)
 {
-	if (!iommu_detected || iommu_pass_through) {
 #ifdef CONFIG_IA64_GENERIC
-		swiotlb = 1;
-		printk(KERN_INFO "PCI-DMA: Re-initialize machine vector.\n");
-		machvec_init("dig");
-		swiotlb_init();
-		dma_ops = &swiotlb_dma_ops;
+	swiotlb = 1;
+	printk(KERN_INFO "PCI-DMA: Re-initialize machine vector.\n");
+	machvec_init("dig");
+	swiotlb_init();
+	dma_ops = &swiotlb_dma_ops;
 #else
-		panic("Unable to find Intel IOMMU");
+	panic("Unable to find Intel IOMMU");
 #endif
-	}
 }

Re: [RFC patch] init default dma_ops to prepare intel_iommu_init failure

[David Woodhouse]

On Wed, 2009-08-12 at 17:20 +0800, Luming Yu wrote:
> Hi there,
> 
> On ia64 platform with dmar table and new upstream
> kernel(config_dmar_default_on enabled),
> if intel_iommu_init ends up without dma_ops initialized or other iommu
> failures.
> We got nothing to fall back. The following patch fixes it.

Hm, don't we then end up with a 64MiB allocation for swiotlb buffers
which we don't need? We can't defer the allocation until we know -- I
think we need to come up with some way to give it _back_ if the swiotlb
turns out not to be needed? And we should do this for i386/x86_64 too.

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

Re: [RFC patch] init default dma_ops to prepare intel_iommu_init failure

[Fenghua Yu]

> Hi there,
> 
> On ia64 platform with dmar table and new upstream
> kernel(config_dmar_default_on enabled),
> if intel_iommu_init ends up without dma_ops initialized or other iommu
> failures.
> We got nothing to fall back. The following patch fixes it.
> 
x86/x86-64 also needs this fix. Please check the pci_swiotlb_init for x86/64.

> Please review. If make sense, please apply.
> 
> **The patch is enclosed in text attachment*
> **Using web client to send the patch* *
> **below is for review, please apply attached  patch*/
> 
> Thanks,
> Luming
> 
> Signed-off-by: Yu Luming <luming.yu@intel.com>
> 
> arch/ia64/kernel/pci-swiotlb.c |   14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/ia64/kernel/pci-swiotlb.c b/arch/ia64/kernel/pci-swiotlb.c
> index 223abb1..d5dcaf5 100644
> --- a/arch/ia64/kernel/pci-swiotlb.c
> +++ b/arch/ia64/kernel/pci-swiotlb.c
> @@ -46,15 +46,13 @@ void __init swiotlb_dma_init(void)
> 
>  void __init pci_swiotlb_init(void)
>  {
> -	if (!iommu_detected || iommu_pass_through) {

This check can not be removed. Otherwise, swiotlb_dma_ops will always override
previous dma_ops value.

I think you can change this check to something like:
	if (!dma_ops || iommu_pass_through) {
With this change, checking iommu_detected is unnecesary because checking dma_ops
covers it.

>  #ifdef CONFIG_IA64_GENERIC
> -		swiotlb = 1;
> -		printk(KERN_INFO "PCI-DMA: Re-initialize machine vector.\n");
> -		machvec_init("dig");
> -		swiotlb_init();
> -		dma_ops = &swiotlb_dma_ops;
> +	swiotlb = 1;
> +	printk(KERN_INFO "PCI-DMA: Re-initialize machine vector.\n");
> +	machvec_init("dig");
> +	swiotlb_init();
> +	dma_ops = &swiotlb_dma_ops;
>  #else
> -		panic("Unable to find Intel IOMMU");
> +	panic("Unable to find Intel IOMMU");
>  #endif
> -	}
>  }

Re: [RFC patch] init default dma_ops to prepare intel_iommu_init failure

[David Woodhouse]

On Wed, 2009-08-12 at 11:17 -0700, Fenghua Yu wrote:
> 
> >  void __init pci_swiotlb_init(void)
> >  {
> > -     if (!iommu_detected || iommu_pass_through) {
> 
> This check can not be removed. Otherwise, swiotlb_dma_ops will always override
> previous dma_ops value.
> 
> I think you can change this check to something like:
>         if (!dma_ops || iommu_pass_through) {
> With this change, checking iommu_detected is unnecesary because checking dma_ops
> covers it.

Hm, that iommu_pass_through should go away anyway. See commit 19943b0e3.

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

Re: [RFC patch] init default dma_ops to prepare intel_iommu_init failure

[FUJITA Tomonori]

On Wed, 12 Aug 2009 11:17:22 -0700
Fenghua Yu <fenghua.yu@intel.com> wrote:

> > Hi there,
> > 
> > On ia64 platform with dmar table and new upstream
> > kernel(config_dmar_default_on enabled),
> > if intel_iommu_init ends up without dma_ops initialized or other iommu
> > failures.
> > We got nothing to fall back. The following patch fixes it.
> > 
> x86/x86-64 also needs this fix. Please check the pci_swiotlb_init for x86/64.

x86 falls back into nommu_dam_ops (though it would be better to use
swiotlb on x86_64).


> > Please review. If make sense, please apply.
> > 
> > **The patch is enclosed in text attachment*
> > **Using web client to send the patch* *
> > **below is for review, please apply attached  patch*/
> > 
> > Thanks,
> > Luming
> > 
> > Signed-off-by: Yu Luming <luming.yu@intel.com>
> > 
> > arch/ia64/kernel/pci-swiotlb.c |   14 ++++++--------
> >  1 file changed, 6 insertions(+), 8 deletions(-)
> > 
> > diff --git a/arch/ia64/kernel/pci-swiotlb.c b/arch/ia64/kernel/pci-swiotlb.c
> > index 223abb1..d5dcaf5 100644
> > --- a/arch/ia64/kernel/pci-swiotlb.c
> > +++ b/arch/ia64/kernel/pci-swiotlb.c
> > @@ -46,15 +46,13 @@ void __init swiotlb_dma_init(void)
> > 
> >  void __init pci_swiotlb_init(void)
> >  {
> > -	if (!iommu_detected || iommu_pass_through) {
> 
> This check can not be removed. Otherwise, swiotlb_dma_ops will always override
> previous dma_ops value.

Yeah.


> I think you can change this check to something like:
> 	if (!dma_ops || iommu_pass_through) {

I don't think this work since pci_iommu_alloc() always sets up dma_ops
and calls pci_swiotlb_init().

All we need to do here is calling swiotlb_init() then we can use
swiotlb_dma_ops after intel_iommu_init() fails:

http://marc.info/?l=linux-kernel&m=125013354518721&w=2


> With this change, checking iommu_detected is unnecesary because checking dma_ops
> covers it.
> 
> >  #ifdef CONFIG_IA64_GENERIC
> > -		swiotlb = 1;
> > -		printk(KERN_INFO "PCI-DMA: Re-initialize machine vector.\n");
> > -		machvec_init("dig");
> > -		swiotlb_init();
> > -		dma_ops = &swiotlb_dma_ops;
> > +	swiotlb = 1;
> > +	printk(KERN_INFO "PCI-DMA: Re-initialize machine vector.\n");
> > +	machvec_init("dig");
> > +	swiotlb_init();
> > +	dma_ops = &swiotlb_dma_ops;
> >  #else
> > -		panic("Unable to find Intel IOMMU");
> > +	panic("Unable to find Intel IOMMU");
> >  #endif
> > -	}
> >  }
> 
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC patch] init default dma_ops to prepare intel_iommu_init failure

[Luming Yu]

>>
>> This check can not be removed. Otherwise, swiotlb_dma_ops will always override
>> previous dma_ops value.
>
> Yeah.

I think pci_swiotlb_init needs to be cleaned up for both x86_{32,64} and ia64.
it should be used to init default dma_ops, and the call site of it should be
as early as platform_dma_init in mem_init. SInce swiotlb_dma_ops is pitched as
default dma_ops for x86, ia64, we really don't need to let pci_swiotlb_init
know iommu_deteced, dmar_disabled or iommu_passthrough...and anything
like that..
Please note the major wrong assumption of the current implementation is
"iommu_deteced == iommu working" that I was trying to fix.

Re: [RFC patch] init default dma_ops to prepare intel_iommu_init failure

[FUJITA Tomonori]

On Thu, 13 Aug 2009 13:48:45 +0800
Luming Yu <luming.yu@gmail.com> wrote:

> >>
> >> This check can not be removed. Otherwise, swiotlb_dma_ops will always override
> >> previous dma_ops value.
> >
> > Yeah.
> 
> I think pci_swiotlb_init needs to be cleaned up for both x86_{32,64} and ia64.

Yeah, it has been on my todo list. But we can't clean up only
pci_swiotlb_init. We need to take care of the whole dma setup code.


> it should be used to init default dma_ops, and the call site of it should be
> as early as platform_dma_init in mem_init. SInce swiotlb_dma_ops is pitched as
> default dma_ops for x86, ia64,

It's more complicated. x86_32 should use nommu_dma_ops. x86_64 should
use nommu_dma_ops if swiotlb is not necessary.

About IA64, swiotlb is not always available (IA64_HP_ZX1 and
IA64_SIG_SN2).


> we really don't need to let pci_swiotlb_init
> know iommu_deteced, dmar_disabled or iommu_passthrough...and anything
> like that..

Yeah. That's one of reasons why I want to rewrite the IA64 and X86 dma
setup code again.


> Please note the major wrong assumption of the current implementation is
> "iommu_deteced == iommu working" that I was trying to fix.

Yeah, I see that. But it had been correct for long time; we had been
happy about the logic that if we detect iommu but it doesn't work then
we are dead.

The x86 dma setup code can enable both hardware IOMMU and
swiotlb. It's a bit similar to what you need for IA64.


Anyway, my patch should fix this problem. Have you tried it?

http://marc.info/?l=linux-kernel&m=125013354518721&w=2


It's fine for now, I think.

Re: [RFC patch] init default dma_ops to prepare intel_iommu_init failure

[FUJITA Tomonori]

On Thu, 13 Aug 2009 13:48:45 +0800
Luming Yu <luming.yu@gmail.com> wrote:

> >>
> >> This check can not be removed. Otherwise, swiotlb_dma_ops will always override
> >> previous dma_ops value.
> >
> > Yeah.
> 
> I think pci_swiotlb_init needs to be cleaned up for both x86_{32,64} and ia64.
> it should be used to init default dma_ops, and the call site of it should be
> as early as platform_dma_init in mem_init. SInce swiotlb_dma_ops is pitched as
> default dma_ops for x86, ia64, we really don't need to let pci_swiotlb_init
> know iommu_deteced, dmar_disabled or iommu_passthrough...and anything
> like that..
> Please note the major wrong assumption of the current implementation is
> "iommu_deteced == iommu working" that I was trying to fix.

How about this?

You can work around that assumption, I guess.


diff --git a/arch/ia64/kernel/pci-dma.c b/arch/ia64/kernel/pci-dma.c
index f6b1ff0..48caafa 100644
--- a/arch/ia64/kernel/pci-dma.c
+++ b/arch/ia64/kernel/pci-dma.c
@@ -20,6 +20,8 @@
 
 #include <asm/page.h>
 
+extern int swiotlb_late_init_with_default_size (size_t size);
+
 dma_addr_t bad_dma_address __read_mostly;
 EXPORT_SYMBOL(bad_dma_address);
 
@@ -44,11 +46,25 @@ struct device fallback_dev = {
 };
 
 extern struct dma_map_ops intel_dma_ops;
+extern struct dma_map_ops swiotlb_dma_ops;
 
 static int __init pci_iommu_init(void)
 {
-	if (iommu_detected)
-		intel_iommu_init();
+	int ret;
+
+	if (iommu_detected) {
+		ret = intel_iommu_init();
+		if (ret) {
+#ifdef CONFIG_IA64_GENERIC
+			dma_ops = &swiotlb_dma_ops;
+			if (swiotlb_late_init_with_default_size(64 * (1<<20)))
+				panic("failed to initialize swiotlb");
+			machvec_init("dig");
+#else
+			panic("Unable to find Intel IOMMU");
+#endif
+		}
+	}
 
 	return 0;
 }

Re: [RFC patch] init default dma_ops to prepare intel_iommu_init failure

[Luming Yu]

Thanks for another workaround, we should be working on deleting wrong
assumption of"iommu_deteced == iommu working", Not just workaround. My
patch could break something.. but it is just a start towards right
goal..

On Thu, Aug 13, 2009 at 2:52 PM, FUJITA
Tomonori<fujita.tomonori@lab.ntt.co.jp> wrote:
> On Thu, 13 Aug 2009 13:48:45 +0800
> Luming Yu <luming.yu@gmail.com> wrote:
>
>> >>
>> >> This check can not be removed. Otherwise, swiotlb_dma_ops will always override
>> >> previous dma_ops value.
>> >
>> > Yeah.
>>
>> I think pci_swiotlb_init needs to be cleaned up for both x86_{32,64} and ia64.
>> it should be used to init default dma_ops, and the call site of it should be
>> as early as platform_dma_init in mem_init. SInce swiotlb_dma_ops is pitched as
>> default dma_ops for x86, ia64, we really don't need to let pci_swiotlb_init
>> know iommu_deteced, dmar_disabled or iommu_passthrough...and anything
>> like that..
>> Please note the major wrong assumption of the current implementation is
>> "iommu_deteced == iommu working" that I was trying to fix.
>
> How about this?
>
> You can work around that assumption, I guess.
>
>
> diff --git a/arch/ia64/kernel/pci-dma.c b/arch/ia64/kernel/pci-dma.c
> index f6b1ff0..48caafa 100644
> --- a/arch/ia64/kernel/pci-dma.c
> +++ b/arch/ia64/kernel/pci-dma.c
> @@ -20,6 +20,8 @@
>
>  #include <asm/page.h>
>
> +extern int swiotlb_late_init_with_default_size (size_t size);
> +
>  dma_addr_t bad_dma_address __read_mostly;
>  EXPORT_SYMBOL(bad_dma_address);
>
> @@ -44,11 +46,25 @@ struct device fallback_dev = {
>  };
>
>  extern struct dma_map_ops intel_dma_ops;
> +extern struct dma_map_ops swiotlb_dma_ops;
>
>  static int __init pci_iommu_init(void)
>  {
> -       if (iommu_detected)
> -               intel_iommu_init();
> +       int ret;
> +
> +       if (iommu_detected) {
> +               ret = intel_iommu_init();
> +               if (ret) {
> +#ifdef CONFIG_IA64_GENERIC
> +                       dma_ops = &swiotlb_dma_ops;
> +                       if (swiotlb_late_init_with_default_size(64 * (1<<20)))
> +                               panic("failed to initialize swiotlb");
> +                       machvec_init("dig");
> +#else
> +                       panic("Unable to find Intel IOMMU");
> +#endif
> +               }
> +       }
>
>        return 0;
>  }
>

Re: [RFC patch] init default dma_ops to prepare intel_iommu_init failure

[FUJITA Tomonori]

On Thu, 13 Aug 2009 15:03:20 +0800
Luming Yu <luming.yu@gmail.com> wrote:

> Thanks for another workaround, we should be working on deleting wrong
> assumption of"iommu_deteced == iommu working", Not just workaround. My
> patch could break something.. but it is just a start towards right
> goal..

As I explained, your patch doesn't go toward the right goal.

And if you want to fix this problem right now, my workaround patch is
much better than yours; it doesn't waste 64MB.

Re: [RFC patch] init default dma_ops to prepare intel_iommu_init failure

[Luming Yu]

On Thu, Aug 13, 2009 at 3:14 PM, FUJITA
Tomonori<fujita.tomonori@lab.ntt.co.jp> wrote:
> On Thu, 13 Aug 2009 15:03:20 +0800
> Luming Yu <luming.yu@gmail.com> wrote:
>
>> Thanks for another workaround, we should be working on deleting wrong
>> assumption of"iommu_deteced == iommu working", Not just workaround. My
>> patch could break something.. but it is just a start towards right
>> goal..
>
> As I explained, your patch doesn't go toward the right goal.

The goal is to decouple dma_ops init and iommu_detect.
In that sense, my patch is right.

>
> And if you want to fix this problem right now, my workaround patch is
> much better than yours; it doesn't waste 64MB.

My patch needs some fixes to get something back when
assigning intel_dma_ops to dma_ops, since we don't want resource
associated with old dma_ops get lost as you and David pointed out.
Please note just pursuing workaround will end up with more pain
than that being relieved

Re: [RFC patch] init default dma_ops to prepare intel_iommu_init failure

[FUJITA Tomonori]

On Thu, 13 Aug 2009 15:37:54 +0800
Luming Yu <luming.yu@gmail.com> wrote:

> On Thu, Aug 13, 2009 at 3:14 PM, FUJITA
> Tomonori<fujita.tomonori@lab.ntt.co.jp> wrote:
> > On Thu, 13 Aug 2009 15:03:20 +0800
> > Luming Yu <luming.yu@gmail.com> wrote:
> >
> >> Thanks for another workaround, we should be working on deleting wrong
> >> assumption of"iommu_deteced == iommu working", Not just workaround. My
> >> patch could break something.. but it is just a start towards right
> >> goal..
> >
> > As I explained, your patch doesn't go toward the right goal.
> 
> The goal is to decouple dma_ops init and iommu_detect.
> In that sense, my patch is right.

No, your patch is a workaround too.

If you want swiotlb the default dma_ops for IA64, pci_swiotlb_init()
is the wrong place for swiotlb_init() because sba needs swiotlb too
for the same reason. Probably, we should always set platform_dma_init
to swiotlb_dma_init().


You want to fix this problem for 2.6.31? Or you think about 2.6.32 (or
later)?

Re: [RFC patch] init default dma_ops to prepare intel_iommu_init failure

[Luming Yu]

On Thu, Aug 13, 2009 at 4:03 PM, FUJITA
Tomonori<fujita.tomonori@lab.ntt.co.jp> wrote:
> On Thu, 13 Aug 2009 15:37:54 +0800
> Luming Yu <luming.yu@gmail.com> wrote:
>
>> On Thu, Aug 13, 2009 at 3:14 PM, FUJITA
>> Tomonori<fujita.tomonori@lab.ntt.co.jp> wrote:
>> > On Thu, 13 Aug 2009 15:03:20 +0800
>> > Luming Yu <luming.yu@gmail.com> wrote:
>> >
>> >> Thanks for another workaround, we should be working on deleting wrong
>> >> assumption of"iommu_deteced == iommu working", Not just workaround. My
>> >> patch could break something.. but it is just a start towards right
>> >> goal..
>> >
>> > As I explained, your patch doesn't go toward the right goal.
>>
>> The goal is to decouple dma_ops init and iommu_detect.
>> In that sense, my patch is right.
>
> No, your patch is a workaround too.
>
> If you want swiotlb the default dma_ops for IA64, pci_swiotlb_init()
> is the wrong place for swiotlb_init() because sba needs swiotlb too
> for the same reason. Probably, we should always set platform_dma_init
> to swiotlb_dma_init().
>
>
> You want to fix this problem for 2.6.31? Or you think about 2.6.32 (or
> later)?
>

I treat this as -32 or -33 stuff, because I don't see a real system is blocking
on the problem. I found this problem when I was trying to disable some drhd
for debugging a real iommu issue...that one has priority.

Re: [RFC patch] init default dma_ops to prepare intel_iommu_init failure

[David Woodhouse]

On Thu, 2009-08-13 at 16:11 +0800, Luming Yu wrote:
> I treat this as -32 or -33 stuff, because I don't see a real system is blocking
> on the problem. I found this problem when I was trying to disable some drhd
> for debugging a real iommu issue...that one has priority.

(The real bug you were chasing is fixed, isn't it? Or worked around, at
least, since the BIOS in question is still closed source and broken.)

But by talking about disabling _some_ drhd units, you've just reminded
me that the problem is far more interesting -- we can have the IOMMU
active for _some_ devices but not others.

I was toying with the idea of putting the intel_dma_ops into
dev->archdata for each pci device at boot or hotplug time, and leaving
the global variable alone. We can calculate the dev->iommu mapping at
that point too. And we can stop being quite so broken w.r.t. PCI
hotplug...

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

Re: [RFC patch] init default dma_ops to prepare intel_iommu_init failure

[FUJITA Tomonori]

On Thu, 13 Aug 2009 09:49:33 +0100
David Woodhouse <dwmw2@infradead.org> wrote:

> On Thu, 2009-08-13 at 16:11 +0800, Luming Yu wrote:
> > I treat this as -32 or -33 stuff, because I don't see a real system is blocking
> > on the problem. I found this problem when I was trying to disable some drhd
> > for debugging a real iommu issue...that one has priority.
> 
> (The real bug you were chasing is fixed, isn't it? Or worked around, at
> least, since the BIOS in question is still closed source and broken.)
> 
> But by talking about disabling _some_ drhd units, you've just reminded
> me that the problem is far more interesting -- we can have the IOMMU
> active for _some_ devices but not others.

It's very common. X86, POWER, IA64 can handle it (I might overlook
some).

About X86, Calgary IOMMU uses both archdata.dma_ops and the global
dma_ops.


> I was toying with the idea of putting the intel_dma_ops into
> dev->archdata for each pci device at boot or hotplug time, and leaving
> the global variable alone. We can calculate the dev->iommu mapping at
> that point too. And we can stop being quite so broken w.r.t. PCI
> hotplug...
> 
> -- 
> David Woodhouse                            Open Source Technology Centre
> David.Woodhouse@intel.com                              Intel Corporation
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC patch] init default dma_ops to prepare intel_iommu_init failure

[Luming Yu]

On Thu, Aug 13, 2009 at 4:49 PM, David Woodhouse<dwmw2@infradead.org> wrote:
> On Thu, 2009-08-13 at 16:11 +0800, Luming Yu wrote:
>> I treat this as -32 or -33 stuff, because I don't see a real system is blocking
>> on the problem. I found this problem when I was trying to disable some drhd
>> for debugging a real iommu issue...that one has priority.
>
> (The real bug you were chasing is fixed, isn't it? Or worked around, at
> least, since the BIOS in question is still closed source and broken.)

Yes the flood of dmar_fault on my HP compaq Dc7800u (my x86_64 linux
desktop) has been fixed. But I still have an iommu casued boot problem
on a ia64 box.

Re: [RFC patch] init default dma_ops to prepare intel_iommu_init failure

[David Woodhouse]

On Thu, 2009-08-13 at 17:00 +0800, Luming Yu wrote:
> Yes the flood of dmar_fault on my HP compaq Dc7800u (my x86_64 linux
> desktop) has been fixed. But I still have an iommu casued boot problem
> on a ia64 box.

Really? Even after http://git.kernel.org/linus/51b89f7a6615 ?]

Fenghua will be very interested if that's true. I'm not -- I just hate
you for having the hardware. :)

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

RE: [RFC patch] init default dma_ops to prepare intel_iommu_init failure

[Yu, Fenghua]

>-----Original Message-----
>From: David Woodhouse [mailto:dwmw2@infradead.org]
>Sent: Thursday, August 13, 2009 10:08 AM
>To: Luming Yu
>Cc: FUJITA Tomonori; Yu, Fenghua; Luck, Tony; linux-kernel@vger.kernel.org;
>akpm@linux-foundation.org
>Subject: Re: [RFC patch] init default dma_ops to prepare intel_iommu_init
>failure
>
>On Thu, 2009-08-13 at 17:00 +0800, Luming Yu wrote:
>> Yes the flood of dmar_fault on my HP compaq Dc7800u (my x86_64 linux
>> desktop) has been fixed. But I still have an iommu casued boot problem
>> on a ia64 box.
>
>Really? Even after http://git.kernel.org/linus/51b89f7a6615 ?]
>
>Fenghua will be very interested if that's true. I'm not -- I just hate
>you for having the hardware. :)
>

The latest upstream tree has three patches to fix the iommu boot issue on ia64. Except the above one, the other two patches are:
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=33041ec049d39a6e0463c7edc7b6f631d24559e3

http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=f532959b77e5e567c84c914cb7c7b07d2582448b

Please checkout the latest upstream tree and have a try.

Thanks.

-Fenghua

Re: [RFC patch] init default dma_ops to prepare intel_iommu_init failure

[Luming Yu]

On Fri, Aug 14, 2009 at 1:30 AM, Yu, Fenghua<fenghua.yu@intel.com> wrote:
>>-----Original Message-----
>>From: David Woodhouse [mailto:dwmw2@infradead.org]
>>Sent: Thursday, August 13, 2009 10:08 AM
>>To: Luming Yu
>>Cc: FUJITA Tomonori; Yu, Fenghua; Luck, Tony; linux-kernel@vger.kernel.org;
>>akpm@linux-foundation.org
>>Subject: Re: [RFC patch] init default dma_ops to prepare intel_iommu_init
>>failure
>>
>>On Thu, 2009-08-13 at 17:00 +0800, Luming Yu wrote:
>>> Yes the flood of dmar_fault on my HP compaq Dc7800u (my x86_64 linux
>>> desktop) has been fixed. But I still have an iommu casued boot problem
>>> on a ia64 box.
>>
>>Really? Even after http://git.kernel.org/linus/51b89f7a6615 ?]
>>
>>Fenghua will be very interested if that's true. I'm not -- I just hate
>>you for having the hardware. :)


-rc5 can't boot.


>>
>
> The latest upstream tree has three patches to fix the iommu boot issue on ia64. Except the above one, the other two patches are:
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=33041ec049d39a6e0463c7edc7b6f631d24559e3
>
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=f532959b77e5e567c84c914cb7c7b07d2582448b
>
> Please checkout the latest upstream tree and have a try.


-rc6 can boot. Thanks all for help. I'm satisfied so far. :-)

Re: [RFC patch] init default dma_ops to prepare intel_iommu_init failure

[FUJITA Tomonori]

On Wed, 12 Aug 2009 17:20:11 +0800
Luming Yu <luming.yu@gmail.com> wrote:

> Hi there,
> 
> On ia64 platform with dmar table and new upstream
> kernel(config_dmar_default_on enabled),
> if intel_iommu_init ends up without dma_ops initialized or other iommu
> failures.
> We got nothing to fall back. The following patch fixes it.

Hmm, I think that the current code should work.

If intel_iommu_init ends up without dma_ops initialized or other iommu
failures, then 'iommu_detected' should be zero.


Note that IA64_DIG_VTD doesn't enable SWIOTLB so there is fall
back. That's kinda the IA64 policy about non IA64_GENERIC kernel.


> Please review. If make sense, please apply.
> 
> **The patch is enclosed in text attachment*
> **Using web client to send the patch* *
> **below is for review, please apply attached  patch*/
> 
> Thanks,
> Luming
> 
> Signed-off-by: Yu Luming <luming.yu@intel.com>
> 
> arch/ia64/kernel/pci-swiotlb.c |   14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/ia64/kernel/pci-swiotlb.c b/arch/ia64/kernel/pci-swiotlb.c
> index 223abb1..d5dcaf5 100644
> --- a/arch/ia64/kernel/pci-swiotlb.c
> +++ b/arch/ia64/kernel/pci-swiotlb.c
> @@ -46,15 +46,13 @@ void __init swiotlb_dma_init(void)
> 
>  void __init pci_swiotlb_init(void)
>  {
> -	if (!iommu_detected || iommu_pass_through) {
>  #ifdef CONFIG_IA64_GENERIC
> -		swiotlb = 1;
> -		printk(KERN_INFO "PCI-DMA: Re-initialize machine vector.\n");
> -		machvec_init("dig");
> -		swiotlb_init();
> -		dma_ops = &swiotlb_dma_ops;
> +	swiotlb = 1;
> +	printk(KERN_INFO "PCI-DMA: Re-initialize machine vector.\n");
> +	machvec_init("dig");
> +	swiotlb_init();
> +	dma_ops = &swiotlb_dma_ops;
>  #else
> -		panic("Unable to find Intel IOMMU");
> +	panic("Unable to find Intel IOMMU");
>  #endif
> -	}
>  }

Re: [RFC patch] init default dma_ops to prepare intel_iommu_init failure

[Luming Yu]

On Thu, Aug 13, 2009 at 10:11 AM, FUJITA
Tomonori<fujita.tomonori@lab.ntt.co.jp> wrote:
> On Wed, 12 Aug 2009 17:20:11 +0800
> Luming Yu <luming.yu@gmail.com> wrote:
>
>> Hi there,
>>
>> On ia64 platform with dmar table and new upstream
>> kernel(config_dmar_default_on enabled),
>> if intel_iommu_init ends up without dma_ops initialized or other iommu
>> failures.
>> We got nothing to fall back. The following patch fixes it.
>
> Hmm, I think that the current code should work.

Without the patch, I encounters boot failure with the following .config

162-CONFIG_IA64_GENERIC=y
163-# CONFIG_IA64_DIG is not set
164:# CONFIG_IA64_DIG_VTD is not set

> Note that IA64_DIG_VTD doesn't enable SWIOTLB so there is fall
> back. That's kinda the IA64 policy about non IA64_GENERIC kernel.
>
how comes?

Re: [RFC patch] init default dma_ops to prepare intel_iommu_init failure

[FUJITA Tomonori]

On Thu, 13 Aug 2009 10:24:20 +0800
Luming Yu <luming.yu@gmail.com> wrote:

> On Thu, Aug 13, 2009 at 10:11 AM, FUJITA
> Tomonori<fujita.tomonori@lab.ntt.co.jp> wrote:
> > On Wed, 12 Aug 2009 17:20:11 +0800
> > Luming Yu <luming.yu@gmail.com> wrote:
> >
> >> Hi there,
> >>
> >> On ia64 platform with dmar table and new upstream
> >> kernel(config_dmar_default_on enabled),
> >> if intel_iommu_init ends up without dma_ops initialized or other iommu
> >> failures.
> >> We got nothing to fall back. The following patch fixes it.
> >
> > Hmm, I think that the current code should work.
> 
> Without the patch, I encounters boot failure with the following .config
> 
> 162-CONFIG_IA64_GENERIC=y
> 163-# CONFIG_IA64_DIG is not set
> 164:# CONFIG_IA64_DIG_VTD is not set

Duh, yeah, intel_iommu_init() fails, it doesn't work.

X86 falls back into nommu_dam_ops but IA64 has nothing.

pci_swiotlb_init() can't set swiotlb to 1 or overwrite dma_ops? If it
sets swiotlb to 1, intel_iommu_init() fails.

A quick fix could be something like this:


diff --git a/arch/ia64/include/asm/swiotlb.h b/arch/ia64/include/asm/swiotlb.h
index dcbaea7..21b076e 100644
--- a/arch/ia64/include/asm/swiotlb.h
+++ b/arch/ia64/include/asm/swiotlb.h
@@ -5,6 +5,7 @@
 #include <linux/swiotlb.h>
 
 extern int swiotlb_force;
+extern struct dma_map_ops swiotlb_dma_ops;
 
 #ifdef CONFIG_SWIOTLB
 extern int swiotlb;
diff --git a/arch/ia64/kernel/pci-dma.c b/arch/ia64/kernel/pci-dma.c
index f6b1ff0..344d783 100644
--- a/arch/ia64/kernel/pci-dma.c
+++ b/arch/ia64/kernel/pci-dma.c
@@ -11,6 +11,7 @@
 #include <asm/iommu.h>
 #include <asm/machvec.h>
 #include <linux/dma-mapping.h>
+#include <asm/swiotlb.h>
 
 #include <asm/system.h>
 
@@ -47,8 +48,13 @@ extern struct dma_map_ops intel_dma_ops;
 
 static int __init pci_iommu_init(void)
 {
-	if (iommu_detected)
-		intel_iommu_init();
+	int ret;
+
+	if (iommu_detected) {
+		ret = intel_iommu_init();
+		if (ret)
+			dma_ops = &swiotlb_dma_ops;
+	}
 
 	return 0;
 }
diff --git a/arch/ia64/kernel/pci-swiotlb.c b/arch/ia64/kernel/pci-swiotlb.c
index 223abb1..ad164cf 100644
--- a/arch/ia64/kernel/pci-swiotlb.c
+++ b/arch/ia64/kernel/pci-swiotlb.c
@@ -46,12 +46,14 @@ void __init swiotlb_dma_init(void)
 
 void __init pci_swiotlb_init(void)
 {
-	if (!iommu_detected || iommu_pass_through) {
+	/* for fallback */
+	swiotlb_init();
+
+	if (!dma_ops) {
 #ifdef CONFIG_IA64_GENERIC
 		swiotlb = 1;
 		printk(KERN_INFO "PCI-DMA: Re-initialize machine vector.\n");
 		machvec_init("dig");
-		swiotlb_init();
 		dma_ops = &swiotlb_dma_ops;
 #else
 		panic("Unable to find Intel IOMMU");

Re: [RFC patch] init default dma_ops to prepare intel_iommu_init failure

[FUJITA Tomonori]

On Thu, 13 Aug 2009 12:15:46 +0900
FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote:

> On Thu, 13 Aug 2009 10:24:20 +0800
> Luming Yu <luming.yu@gmail.com> wrote:
> 
> > On Thu, Aug 13, 2009 at 10:11 AM, FUJITA
> > Tomonori<fujita.tomonori@lab.ntt.co.jp> wrote:
> > > On Wed, 12 Aug 2009 17:20:11 +0800
> > > Luming Yu <luming.yu@gmail.com> wrote:
> > >
> > >> Hi there,
> > >>
> > >> On ia64 platform with dmar table and new upstream
> > >> kernel(config_dmar_default_on enabled),
> > >> if intel_iommu_init ends up without dma_ops initialized or other iommu
> > >> failures.
> > >> We got nothing to fall back. The following patch fixes it.
> > >
> > > Hmm, I think that the current code should work.
> > 
> > Without the patch, I encounters boot failure with the following .config
> > 
> > 162-CONFIG_IA64_GENERIC=y
> > 163-# CONFIG_IA64_DIG is not set
> > 164:# CONFIG_IA64_DIG_VTD is not set
> 
> Duh, yeah, intel_iommu_init() fails, it doesn't work.
> 
> X86 falls back into nommu_dam_ops but IA64 has nothing.
> 
> pci_swiotlb_init() can't set swiotlb to 1 or overwrite dma_ops? If it
> sets swiotlb to 1, intel_iommu_init() fails.
> 
> A quick fix could be something like this:

Oops, here's a good one.

diff --git a/arch/ia64/include/asm/swiotlb.h b/arch/ia64/include/asm/swiotlb.h
index dcbaea7..21b076e 100644
--- a/arch/ia64/include/asm/swiotlb.h
+++ b/arch/ia64/include/asm/swiotlb.h
@@ -5,6 +5,7 @@
 #include <linux/swiotlb.h>
 
 extern int swiotlb_force;
+extern struct dma_map_ops swiotlb_dma_ops;
 
 #ifdef CONFIG_SWIOTLB
 extern int swiotlb;
diff --git a/arch/ia64/kernel/pci-dma.c b/arch/ia64/kernel/pci-dma.c
index f6b1ff0..344d783 100644
--- a/arch/ia64/kernel/pci-dma.c
+++ b/arch/ia64/kernel/pci-dma.c
@@ -11,6 +11,7 @@
 #include <asm/iommu.h>
 #include <asm/machvec.h>
 #include <linux/dma-mapping.h>
+#include <asm/swiotlb.h>
 
 #include <asm/system.h>
 
@@ -47,8 +48,13 @@ extern struct dma_map_ops intel_dma_ops;
 
 static int __init pci_iommu_init(void)
 {
-	if (iommu_detected)
-		intel_iommu_init();
+	int ret;
+
+	if (iommu_detected) {
+		ret = intel_iommu_init();
+		if (ret)
+			dma_ops = &swiotlb_dma_ops;
+	}
 
 	return 0;
 }
diff --git a/arch/ia64/kernel/pci-swiotlb.c b/arch/ia64/kernel/pci-swiotlb.c
index 223abb1..fb06ef6 100644
--- a/arch/ia64/kernel/pci-swiotlb.c
+++ b/arch/ia64/kernel/pci-swiotlb.c
@@ -46,12 +46,14 @@ void __init swiotlb_dma_init(void)
 
 void __init pci_swiotlb_init(void)
 {
+	/* for fallback */
+	swiotlb_init();
+
 	if (!iommu_detected || iommu_pass_through) {
 #ifdef CONFIG_IA64_GENERIC
 		swiotlb = 1;
 		printk(KERN_INFO "PCI-DMA: Re-initialize machine vector.\n");
 		machvec_init("dig");
-		swiotlb_init();
 		dma_ops = &swiotlb_dma_ops;
 #else
 		panic("Unable to find Intel IOMMU");

Re: [RFC patch] init default dma_ops to prepare intel_iommu_init failure

[David Woodhouse]

On Thu, 2009-08-13 at 12:18 +0900, FUJITA Tomonori wrote:
> > Duh, yeah, intel_iommu_init() fails, it doesn't work.
> > 
> > X86 falls back into nommu_dam_ops but IA64 has nothing.
> > 
> > pci_swiotlb_init() can't set swiotlb to 1 or overwrite dma_ops? If it
> > sets swiotlb to 1, intel_iommu_init() fails.
> > 
> > A quick fix could be something like this:
> 
> Oops, here's a good one.

Closer, but don't we still want to free the swiotlb buffers if the iommu
init _does_ actually work?

And we want the same on x86 too -- we want swiotlb, not nommu.

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

Re: [RFC patch] init default dma_ops to prepare intel_iommu_init failure

[FUJITA Tomonori]

On Thu, 13 Aug 2009 09:26:12 +0100
David Woodhouse <dwmw2@infradead.org> wrote:

> On Thu, 2009-08-13 at 12:18 +0900, FUJITA Tomonori wrote:
> > > Duh, yeah, intel_iommu_init() fails, it doesn't work.
> > > 
> > > X86 falls back into nommu_dam_ops but IA64 has nothing.
> > > 
> > > pci_swiotlb_init() can't set swiotlb to 1 or overwrite dma_ops? If it
> > > sets swiotlb to 1, intel_iommu_init() fails.
> > > 
> > > A quick fix could be something like this:
> > 
> > Oops, here's a good one.
> 
> Closer, but don't we still want to free the swiotlb buffers if the iommu
> init _does_ actually work?

If we want to fix this right now, this should be applied:

http://marc.info/?l=linux-kernel&m=125014640502047&w=2

It allocates swiotlb buffer only if we actually use it.

But seems that this is not an urgent issue so I'll work on the IA64
and X86 dma bootup code again for 2.6.32 or 33.

> And we want the same on x86 too -- we want swiotlb, not nommu.