[RFC patch] return early if all dmar HW unit ignored

[RFC patch] return early if all dmar HW unit ignored

[Luming Yu]

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

Hello list,

When debugging an IOMMU problem, I noticed it should be much more safe
to return early than late if all remapping HW unit are ignored.
To figure out what device causes the iommu problem on my linux box:
hp-compaq dc7800, I tried to mark all dmar HW
unit ignored,but still got a lot of unxepcted dmar_fault. So I think
the proposed patch make sense. The next step is to
add a boot option to make dmar HW units selectable enable/disable individually.

Please review.

**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>

 intel-iommu.c |   26 +++++++++++++++++---------
 1 file changed, 17 insertions(+), 9 deletions(-)

diff --git a/drivers/pci/intel-iommu.c b/drivers/pci/intel-iommu.c
index ebc9b8d..713e206 100644
--- a/drivers/pci/intel-iommu.c
+++ b/drivers/pci/intel-iommu.c
@@ -2994,9 +2994,10 @@ static void __init iommu_exit_mempool(void)

 }

-static void __init init_no_remapping_devices(void)
+static int __init init_no_remapping_devices(void)
 {
 	struct dmar_drhd_unit *drhd;
+	int drhd_total = 0,drhd_ignored = 0;

 	for_each_drhd_unit(drhd) {
 		if (!drhd->include_all) {
@@ -3008,32 +3009,34 @@ static void __init init_no_remapping_devices(void)
 			if (i == drhd->devices_cnt)
 				drhd->ignored = 1;
 		}
+		drhd_total++;
 	}
-
-	if (dmar_map_gfx)
-		return;
-
 	for_each_drhd_unit(drhd) {
 		int i;
-		if (drhd->ignored || drhd->include_all)
+		if (drhd->ignored)
+			drhd_ignored++;
+		if (drhd->ignored || drhd->include_all) {
+			continue;
+		}
+		if (dmar_map_gfx)
 			continue;
-
 		for (i = 0; i < drhd->devices_cnt; i++)
 			if (drhd->devices[i] &&
 				!IS_GFX_DEVICE(drhd->devices[i]))
 				break;
-
 		if (i < drhd->devices_cnt)
 			continue;

 		/* bypass IOMMU if it is just for gfx devices */
 		drhd->ignored = 1;
+		drhd_ignored++;
 		for (i = 0; i < drhd->devices_cnt; i++) {
 			if (!drhd->devices[i])
 				continue;
 			drhd->devices[i]->dev.archdata.iommu = DUMMY_DEVICE_DOMAIN_INFO;
 		}
 	}
+	return (drhd_total == drhd_ignored);
 }

 #ifdef CONFIG_SUSPEND
@@ -3200,7 +3203,12 @@ int __init intel_iommu_init(void)
 	iommu_init_mempool();
 	dmar_init_reserved_ranges();

-	init_no_remapping_devices();
+	if(init_no_remapping_devices()) {
+		printk(KERN_ERR "IOMMU: all dmar HW unit ignored\n");
+		put_iova_domain(&reserved_iova_list);
+		iommu_exit_mempool();
+		return ret;
+	}

 	ret = init_dmars();
 	if (ret) {

[-- Attachment #2: 0.patch --]
[-- Type: application/octet-stream, Size: 1743 bytes --]

diff --git a/drivers/pci/intel-iommu.c b/drivers/pci/intel-iommu.c
index ebc9b8d..713e206 100644
--- a/drivers/pci/intel-iommu.c
+++ b/drivers/pci/intel-iommu.c
@@ -2994,9 +2994,10 @@ static void __init iommu_exit_mempool(void)
 
 }
 
-static void __init init_no_remapping_devices(void)
+static int __init init_no_remapping_devices(void)
 {
 	struct dmar_drhd_unit *drhd;
+	int drhd_total = 0,drhd_ignored = 0;
 
 	for_each_drhd_unit(drhd) {
 		if (!drhd->include_all) {
@@ -3008,32 +3009,34 @@ static void __init init_no_remapping_devices(void)
 			if (i == drhd->devices_cnt)
 				drhd->ignored = 1;
 		}
+		drhd_total++;
 	}
-
-	if (dmar_map_gfx)
-		return;
-
 	for_each_drhd_unit(drhd) {
 		int i;
-		if (drhd->ignored || drhd->include_all)
+		if (drhd->ignored)
+			drhd_ignored++;
+		if (drhd->ignored || drhd->include_all) {
+			continue;
+		}
+		if (dmar_map_gfx)
 			continue;
-
 		for (i = 0; i < drhd->devices_cnt; i++)
 			if (drhd->devices[i] &&
 				!IS_GFX_DEVICE(drhd->devices[i]))
 				break;
-
 		if (i < drhd->devices_cnt)
 			continue;
 
 		/* bypass IOMMU if it is just for gfx devices */
 		drhd->ignored = 1;
+		drhd_ignored++;
 		for (i = 0; i < drhd->devices_cnt; i++) {
 			if (!drhd->devices[i])
 				continue;
 			drhd->devices[i]->dev.archdata.iommu = DUMMY_DEVICE_DOMAIN_INFO;
 		}
 	}
+	return (drhd_total == drhd_ignored);
 }
 
 #ifdef CONFIG_SUSPEND
@@ -3200,7 +3203,12 @@ int __init intel_iommu_init(void)
 	iommu_init_mempool();
 	dmar_init_reserved_ranges();
 
-	init_no_remapping_devices();
+	if(init_no_remapping_devices()) {
+		printk(KERN_ERR "IOMMU: all dmar HW unit ignored\n");
+		put_iova_domain(&reserved_iova_list);
+		iommu_exit_mempool();
+		return ret;
+	}
 
 	ret = init_dmars();
 	if (ret) {

Re: [RFC patch] return early if all dmar HW unit ignored

[David Woodhouse]

On Fri, 2009-07-31 at 16:29 +0800, Luming Yu wrote:
> Hello list,
> 
> When debugging an IOMMU problem, I noticed it should be much more safe
> to return early than late if all remapping HW unit are ignored.
> To figure out what device causes the iommu problem on my linux box:
> hp-compaq dc7800, I tried to mark all dmar HW
> unit ignored,but still got a lot of unxepcted dmar_fault. So I think
> the proposed patch make sense. The next step is to
> add a boot option to make dmar HW units selectable enable/disable individually.

Why were you seeing faults if all dmar units were ignored? Surely that
shouldn't happen?

This patch doesn't make a lot of sense on its own -- can you show what
you were intending to do as the 'next step'?

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

Re: [RFC patch] return early if all dmar HW unit ignored

[Luming Yu]

David,

Sorry for late response.. I tried your patch that terminates dmar
table init if "one
DMAR reported at address xxxxx returns all = ones"
I know BIOS is very likely to do bad things, so I think your patch makes sense.
But I think the following info should look better..

Instead of ending up in parse DMAR table failure right after just one
drhd record,
all 4 are parsed. ( I applied your patch and the patch of this thread,
and changed your patch to set ignored flag)

$ dmesg | grep -i dmar
[    0.000000] ACPI: DMAR 000000007d2c225f 001A8 (v01 COMPAQ BEARLAKE
00000001      00000000)
[    0.161030] DMAR:Host address width 36
[    0.161032] DMAR:DRHD base: 0x000000fed90000 flags: 0x0
[    0.161045] WARNING: at drivers/pci/dmar.c:640 alloc_iommu+0xfc/0x230()
[    0.161049] Your BIOS is broken; DMAR reported at address fed90000
returns all = ones!
[    0.161070]  [<ffffffff81819ce9>] dmar_table_init+0x1b1/0x35a
[    0.161119] DMAR:DRHD base: 0x000000fed91000 flags: 0x0
[    0.161127] WARNING: at drivers/pci/dmar.c:640 alloc_iommu+0xfc/0x230()
[    0.161130] Your BIOS is broken; DMAR reported at address fed91000
returns all = ones!
[    0.161146]  [<ffffffff81819ce9>] dmar_table_init+0x1b1/0x35a
[    0.161183] DMAR:DRHD base: 0x000000fed92000 flags: 0x0
[    0.161191] WARNING: at drivers/pci/dmar.c:640 alloc_iommu+0xfc/0x230()
[    0.161194] Your BIOS is broken; DMAR reported at address fed92000
returns all = ones!
[    0.161210]  [<ffffffff81819ce9>] dmar_table_init+0x1b1/0x35a
[    0.161247] DMAR:DRHD base: 0x000000fed93000 flags: 0x1
[    0.161255] WARNING: at drivers/pci/dmar.c:640 alloc_iommu+0xfc/0x230()
[    0.161258] Your BIOS is broken; DMAR reported at address fed93000
returns all = ones!
[    0.161274]  [<ffffffff81819ce9>] dmar_table_init+0x1b1/0x35a
[    0.161310] DMAR:RMRR base: 0x0000007d600000 end: 0x0000007dffffff
[    0.161313] DMAR:RMRR base: 0x0000007d2d0000 end: 0x0000007d2d0fff
[    0.161315] DMAR:RMRR base: 0x0000007d2d1000 end: 0x0000007d2d1fff
[    0.161318] DMAR:RMRR base: 0x0000007d2d2000 end: 0x0000007d2d2fff
[    0.161320] DMAR:RMRR base: 0x0000007d2d3000 end: 0x0000007d2d3fff
[    0.161322] DMAR:RMRR base: 0x0000007d2d5000 end: 0x0000007d2d5fff
[    0.161325] DMAR:RMRR base: 0x0000007d2d6000 end: 0x0000007d2d6fff
[    0.161327] DMAR:RMRR base: 0x0000007d2d7000 end: 0x0000007d2d7fff
[    0.161329] DMAR:No ATSR found
[    0.161370] IOMMU: all dmar HW unit ignored


But I don't know why I were seeing faults if all dmar units were
ignored? Surely that
shouldn't happen? Will double check and investigate.

Thanks,
Luming


On Tue, Aug 4, 2009 at 3:09 PM, David Woodhouse<dwmw2@infradead.org> wrote:
> On Fri, 2009-07-31 at 16:29 +0800, Luming Yu wrote:
>> Hello list,
>>
>> When debugging an IOMMU problem, I noticed it should be much more safe
>> to return early than late if all remapping HW unit are ignored.
>> To figure out what device causes the iommu problem on my linux box:
>> hp-compaq dc7800, I tried to mark all dmar HW
>> unit ignored,but still got a lot of unxepcted dmar_fault. So I think
>> the proposed patch make sense. The next step is to
>> add a boot option to make dmar HW units selectable enable/disable individually.
>
> Why were you seeing faults if all dmar units were ignored? Surely that
> shouldn't happen?
>
> This patch doesn't make a lot of sense on its own -- can you show what
> you were intending to do as the 'next step'?
>
> --
> David Woodhouse                            Open Source Technology Centre
> David.Woodhouse@intel.com                              Intel Corporation
>
>

Re: [RFC patch] return early if all dmar HW unit ignored

[David Woodhouse]

On Mon, 2009-08-10 at 14:21 +0800, Luming Yu wrote:
> 
> But I don't know why I were seeing faults if all dmar units were
> ignored? Surely that shouldn't happen? Will double check and
> investigate.

It shouldn't happen -- I suspect that you didn't actually mark them
_all_ ignored? What happens if you modify my patch just to mark the
offending unit as ignored, but don't also apply your patch? Does it boot
then?

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

Re: [RFC patch] return early if all dmar HW unit ignored

[Luming Yu]

On Mon, Aug 10, 2009 at 5:13 PM, David Woodhouse<dwmw2@infradead.org> wrote:
> On Mon, 2009-08-10 at 14:21 +0800, Luming Yu wrote:
>>
>> But I don't know why I were seeing faults if all dmar units were
>> ignored? Surely that shouldn't happen? Will double check and
>> investigate.
>
> It shouldn't happen -- I suspect that you didn't actually mark them
> _all_ ignored? What happens if you modify my patch just to mark the
> offending unit as ignored, but don't also apply your patch? Does it boot
> then?

Just with your patch, it boot without any problem.
Add my patch in the thread, it boots too, and I see total 4 entries parsed...

Re: [RFC patch] return early if all dmar HW unit ignored

[Luming Yu]

On Mon, Aug 10, 2009 at 5:19 PM, Luming Yu<luming.yu@gmail.com> wrote:
> On Mon, Aug 10, 2009 at 5:13 PM, David Woodhouse<dwmw2@infradead.org> wrote:
>> On Mon, 2009-08-10 at 14:21 +0800, Luming Yu wrote:
>>>
>>> But I don't know why I were seeing faults if all dmar units were
>>> ignored? Surely that shouldn't happen? Will double check and
>>> investigate.
>>
>> It shouldn't happen -- I suspect that you didn't actually mark them
>> _all_ ignored? What happens if you modify my patch just to mark the
>> offending unit as ignored, but don't also apply your patch? Does it boot

I guess no but will check for sure tomorrow.


>> then?
>
> Just with your patch, it boot without any problem.
> Add my patch in the thread, it boots too, and I see total 4 entries parsed...

Re: [RFC patch] return early if all dmar HW unit ignored

[Luming Yu]

>>> It shouldn't happen -- I suspect that you didn't actually mark them

if it happens, it means there are bugs in code of handling ignore flag.

>>> _all_ ignored? What happens if you modify my patch just to mark the
>>> offending unit as ignored, but don't also apply your patch? Does it boot
> I guess no but will check for sure tomorrow.

Tested it didn't boot.