[PATCH 0/3] iommu/vt-d bad RMRR workarounds

[PATCH 0/3] iommu/vt-d bad RMRR workarounds

[Barret Rhoden]

Hi -

Commit f036c7fa0ab6 ("iommu/vt-d: Check VT-d RMRR region in BIOS is
reported as reserved") caused a machine to fail to boot for me, but only
after a kexec.

Firmware provided an RMRR entry with base and end both == 0:

	DMAR: RMRR base: 0x00000000000000 end: 0x00000000000000

Yes, firmware should not do that.  I'd like to be able to handle it.

That bad entry was actually OK on a fresh boot, since the region of
0x0000..0x0001 ([start, end + 1)) was type RESERVED due to this e820
update call:

	e820: update [mem 0x00000000-0x00000fff] usable ==> reserved

However, after a kexec, for whatever reason that first entry changed from

	BIOS-e820: [mem 0x0000000000000000-0x000000000009cbff] usable

to

	BIOS-e820: [mem 0x0000000000000100-0x000000000009cbff] usable

It starts at 0x100, not 0x000.  Ultimately, the range for that bad RMRR
[0x0, 0x1) isn't in the e820 map at all after a kexec.

The existing code aborts all of the DMAR parsing, eventually my disk
drivers fail, I can't mount the root filesystem, etc.  If you're
curious, I get a bunch of these errors:

	ata2.00: qc timeout (cmd 0xec)

I can imagine a bunch of ways around this.

One option is to hook in a check for buggy RMRRs in intel-iommu.c.  If
the base and end are 0, just ignore the entry.  That works for my
specific buggy DMAR entry.  There might be other buggy entries out
there.  The docs specify some requirements on the base and end (called
limit) addresses.

Another option is to change the sanity check so that unmapped ranges are
considered OK.  That would work for my case, but only because we're
hiding the firmware bug: my DMAR has a bad RMRR that happens to fall into a
reserved or non-existent range.  The downside here is that we'd
presumably be setting up an IOMMU mapping for this bad RMRR.  But at
least it's not pointing to any RAM we're using.  (That's actually what
goes on in the current, non-kexec case for me.  Phys page 0 is marked
RESERVED, and I have an RMRR that points to it.)  This option also would
cover any buggy firmwares that use an actual RMRR that pointed to memory
that was omitted from the e820 map.

A third option: whenever the RMRR sanity check fails, just ignore it and
return 0.  Don't set up the rmrru.  Right now, we completely abort DMAR
processing.  If there was an actual device that needed to poke this
memory that failed the sanity check (meaning, not RESERVED, currently),
then we're already in trouble; that device could clobber RAM, right?  If
we're going to use the IOMMU, I'd rather the device be behind an IOMMU
with *no* mapping for the region, so it couldn't clobber whatever we
happened to put in that location.

I actually think all three options are reasonable ideas independently of
one another.  This patchset that does all three.  Please take at least
one of them.  =)  (May require a slight revision if you don't take all
of them).

Barret Rhoden (3):
  iommu/vt-d: skip RMRR entries that fail the sanity check
  iommu/vt-d: treat unmapped RMRR entries as sane
  iommu/vt-d: skip invalid RMRR entries

 arch/x86/include/asm/iommu.h |  2 ++
 drivers/iommu/intel-iommu.c  | 16 ++++++++++++++--
 2 files changed, 16 insertions(+), 2 deletions(-)

-- 
2.24.0.525.g8f36a354ae-goog

[PATCH 0/3] iommu/vt-d bad RMRR workarounds

[Barret Rhoden via iommu]

Hi -

Commit f036c7fa0ab6 ("iommu/vt-d: Check VT-d RMRR region in BIOS is
reported as reserved") caused a machine to fail to boot for me, but only
after a kexec.

Firmware provided an RMRR entry with base and end both == 0:

	DMAR: RMRR base: 0x00000000000000 end: 0x00000000000000

Yes, firmware should not do that.  I'd like to be able to handle it.

That bad entry was actually OK on a fresh boot, since the region of
0x0000..0x0001 ([start, end + 1)) was type RESERVED due to this e820
update call:

	e820: update [mem 0x00000000-0x00000fff] usable ==> reserved

However, after a kexec, for whatever reason that first entry changed from

	BIOS-e820: [mem 0x0000000000000000-0x000000000009cbff] usable

to

	BIOS-e820: [mem 0x0000000000000100-0x000000000009cbff] usable

It starts at 0x100, not 0x000.  Ultimately, the range for that bad RMRR
[0x0, 0x1) isn't in the e820 map at all after a kexec.

The existing code aborts all of the DMAR parsing, eventually my disk
drivers fail, I can't mount the root filesystem, etc.  If you're
curious, I get a bunch of these errors:

	ata2.00: qc timeout (cmd 0xec)

I can imagine a bunch of ways around this.

One option is to hook in a check for buggy RMRRs in intel-iommu.c.  If
the base and end are 0, just ignore the entry.  That works for my
specific buggy DMAR entry.  There might be other buggy entries out
there.  The docs specify some requirements on the base and end (called
limit) addresses.

Another option is to change the sanity check so that unmapped ranges are
considered OK.  That would work for my case, but only because we're
hiding the firmware bug: my DMAR has a bad RMRR that happens to fall into a
reserved or non-existent range.  The downside here is that we'd
presumably be setting up an IOMMU mapping for this bad RMRR.  But at
least it's not pointing to any RAM we're using.  (That's actually what
goes on in the current, non-kexec case for me.  Phys page 0 is marked
RESERVED, and I have an RMRR that points to it.)  This option also would
cover any buggy firmwares that use an actual RMRR that pointed to memory
that was omitted from the e820 map.

A third option: whenever the RMRR sanity check fails, just ignore it and
return 0.  Don't set up the rmrru.  Right now, we completely abort DMAR
processing.  If there was an actual device that needed to poke this
memory that failed the sanity check (meaning, not RESERVED, currently),
then we're already in trouble; that device could clobber RAM, right?  If
we're going to use the IOMMU, I'd rather the device be behind an IOMMU
with *no* mapping for the region, so it couldn't clobber whatever we
happened to put in that location.

I actually think all three options are reasonable ideas independently of
one another.  This patchset that does all three.  Please take at least
one of them.  =)  (May require a slight revision if you don't take all
of them).

Barret Rhoden (3):
  iommu/vt-d: skip RMRR entries that fail the sanity check
  iommu/vt-d: treat unmapped RMRR entries as sane
  iommu/vt-d: skip invalid RMRR entries

 arch/x86/include/asm/iommu.h |  2 ++
 drivers/iommu/intel-iommu.c  | 16 ++++++++++++++--
 2 files changed, 16 insertions(+), 2 deletions(-)

-- 
2.24.0.525.g8f36a354ae-goog

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH 1/3] iommu/vt-d: skip RMRR entries that fail the sanity check

[Barret Rhoden]

RMRR entries describe memory regions that are DMA targets for devices
outside the kernel's control.

RMRR entries that fail the sanity check are pointing to regions of
memory that the firmware did not tell the kernel are reserved or
otherwise should not be used.

Instead of aborting DMAR processing, this commit skips these RMRR
entries.  They will not be mapped into the IOMMU, but the IOMMU can
still be utilized.  If anything, when the IOMMU is on, those devices
will not be able to clobber RAM that the kernel has allocated from those
regions.

Signed-off-by: Barret Rhoden <brho@google.com>
---
 drivers/iommu/intel-iommu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index f168cd8ee570..f7e09244c9e4 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -4316,7 +4316,7 @@ int __init dmar_parse_one_rmrr(struct acpi_dmar_header *header, void *arg)
 	rmrr = (struct acpi_dmar_reserved_memory *)header;
 	ret = arch_rmrr_sanity_check(rmrr);
 	if (ret)
-		return ret;
+		return 0;
 
 	rmrru = kzalloc(sizeof(*rmrru), GFP_KERNEL);
 	if (!rmrru)
-- 
2.24.0.525.g8f36a354ae-goog

[PATCH 1/3] iommu/vt-d: skip RMRR entries that fail the sanity check

[Barret Rhoden via iommu]

RMRR entries describe memory regions that are DMA targets for devices
outside the kernel's control.

RMRR entries that fail the sanity check are pointing to regions of
memory that the firmware did not tell the kernel are reserved or
otherwise should not be used.

Instead of aborting DMAR processing, this commit skips these RMRR
entries.  They will not be mapped into the IOMMU, but the IOMMU can
still be utilized.  If anything, when the IOMMU is on, those devices
will not be able to clobber RAM that the kernel has allocated from those
regions.

Signed-off-by: Barret Rhoden <brho@google.com>
---
 drivers/iommu/intel-iommu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index f168cd8ee570..f7e09244c9e4 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -4316,7 +4316,7 @@ int __init dmar_parse_one_rmrr(struct acpi_dmar_header *header, void *arg)
 	rmrr = (struct acpi_dmar_reserved_memory *)header;
 	ret = arch_rmrr_sanity_check(rmrr);
 	if (ret)
-		return ret;
+		return 0;
 
 	rmrru = kzalloc(sizeof(*rmrru), GFP_KERNEL);
 	if (!rmrru)
-- 
2.24.0.525.g8f36a354ae-goog

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH 2/3] iommu/vt-d: treat unmapped RMRR entries as sane

[Barret Rhoden]

The RMRR sanity check is to confirm that the memory pointed to by the
RMRR entry is not used by the kernel.  e820 RESERVED memory will not be
used.  However, there are ranges of physical memory that are not covered
by the e820 table at all.  The kernel will not use this memory, either.

This commit expands the sanity check to treat memory that is not in any
e820 entry as safe.

Signed-off-by: Barret Rhoden <brho@google.com>
---
 arch/x86/include/asm/iommu.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/x86/include/asm/iommu.h b/arch/x86/include/asm/iommu.h
index bf1ed2ddc74b..7e9f0c2f975f 100644
--- a/arch/x86/include/asm/iommu.h
+++ b/arch/x86/include/asm/iommu.h
@@ -20,6 +20,8 @@ arch_rmrr_sanity_check(struct acpi_dmar_reserved_memory *rmrr)
 
 	if (e820__mapped_all(start, end, E820_TYPE_RESERVED))
 		return 0;
+	if (!e820__mapped_any(start, end, 0))
+		return 0;
 
 	pr_err(FW_BUG "No firmware reserved region can cover this RMRR [%#018Lx-%#018Lx], contact BIOS vendor for fixes\n",
 	       start, end - 1);
-- 
2.24.0.525.g8f36a354ae-goog

[PATCH 2/3] iommu/vt-d: treat unmapped RMRR entries as sane

[Barret Rhoden via iommu]

The RMRR sanity check is to confirm that the memory pointed to by the
RMRR entry is not used by the kernel.  e820 RESERVED memory will not be
used.  However, there are ranges of physical memory that are not covered
by the e820 table at all.  The kernel will not use this memory, either.

This commit expands the sanity check to treat memory that is not in any
e820 entry as safe.

Signed-off-by: Barret Rhoden <brho@google.com>
---
 arch/x86/include/asm/iommu.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/x86/include/asm/iommu.h b/arch/x86/include/asm/iommu.h
index bf1ed2ddc74b..7e9f0c2f975f 100644
--- a/arch/x86/include/asm/iommu.h
+++ b/arch/x86/include/asm/iommu.h
@@ -20,6 +20,8 @@ arch_rmrr_sanity_check(struct acpi_dmar_reserved_memory *rmrr)
 
 	if (e820__mapped_all(start, end, E820_TYPE_RESERVED))
 		return 0;
+	if (!e820__mapped_any(start, end, 0))
+		return 0;
 
 	pr_err(FW_BUG "No firmware reserved region can cover this RMRR [%#018Lx-%#018Lx], contact BIOS vendor for fixes\n",
 	       start, end - 1);
-- 
2.24.0.525.g8f36a354ae-goog

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH 3/3] iommu/vt-d: skip invalid RMRR entries

[Barret Rhoden]

The VT-d docs specify requirements for the RMRR entries base and end
(called 'Limit' in the docs) addresses.

This commit will cause the DMAR processing to skip any RMRR entries
that do not meet these requirements with the expectation that firmware
is giving us junk.

Signed-off-by: Barret Rhoden <brho@google.com>
---
 drivers/iommu/intel-iommu.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index f7e09244c9e4..11322fefb883 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -4307,6 +4307,18 @@ static void __init init_iommu_pm_ops(void)
 static inline void init_iommu_pm_ops(void) {}
 #endif	/* CONFIG_PM */
 
+static int rmrr_validity_check(struct acpi_dmar_reserved_memory *rmrr)
+{
+	if ((rmrr->base_address & PAGE_MASK) ||
+	    (rmrr->end_address <= rmrr->base_address) ||
+	    ((rmrr->end_address - rmrr->base_address + 1) & PAGE_MASK)) {
+		pr_err(FW_BUG "Broken RMRR base: %#018Lx end: %#018Lx\n",
+		       rmrr->base_address, rmrr->end_address);
+		return -EINVAL;
+	}
+	return 0;
+}
+
 int __init dmar_parse_one_rmrr(struct acpi_dmar_header *header, void *arg)
 {
 	struct acpi_dmar_reserved_memory *rmrr;
@@ -4314,7 +4326,7 @@ int __init dmar_parse_one_rmrr(struct acpi_dmar_header *header, void *arg)
 	int ret;
 
 	rmrr = (struct acpi_dmar_reserved_memory *)header;
-	ret = arch_rmrr_sanity_check(rmrr);
+	ret = rmrr_validity_check(rmrr) || arch_rmrr_sanity_check(rmrr);
 	if (ret)
 		return 0;
 
-- 
2.24.0.525.g8f36a354ae-goog

[PATCH 3/3] iommu/vt-d: skip invalid RMRR entries

[Barret Rhoden via iommu]

The VT-d docs specify requirements for the RMRR entries base and end
(called 'Limit' in the docs) addresses.

This commit will cause the DMAR processing to skip any RMRR entries
that do not meet these requirements with the expectation that firmware
is giving us junk.

Signed-off-by: Barret Rhoden <brho@google.com>
---
 drivers/iommu/intel-iommu.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index f7e09244c9e4..11322fefb883 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -4307,6 +4307,18 @@ static void __init init_iommu_pm_ops(void)
 static inline void init_iommu_pm_ops(void) {}
 #endif	/* CONFIG_PM */
 
+static int rmrr_validity_check(struct acpi_dmar_reserved_memory *rmrr)
+{
+	if ((rmrr->base_address & PAGE_MASK) ||
+	    (rmrr->end_address <= rmrr->base_address) ||
+	    ((rmrr->end_address - rmrr->base_address + 1) & PAGE_MASK)) {
+		pr_err(FW_BUG "Broken RMRR base: %#018Lx end: %#018Lx\n",
+		       rmrr->base_address, rmrr->end_address);
+		return -EINVAL;
+	}
+	return 0;
+}
+
 int __init dmar_parse_one_rmrr(struct acpi_dmar_header *header, void *arg)
 {
 	struct acpi_dmar_reserved_memory *rmrr;
@@ -4314,7 +4326,7 @@ int __init dmar_parse_one_rmrr(struct acpi_dmar_header *header, void *arg)
 	int ret;
 
 	rmrr = (struct acpi_dmar_reserved_memory *)header;
-	ret = arch_rmrr_sanity_check(rmrr);
+	ret = rmrr_validity_check(rmrr) || arch_rmrr_sanity_check(rmrr);
 	if (ret)
 		return 0;
 
-- 
2.24.0.525.g8f36a354ae-goog

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 0/3] iommu/vt-d bad RMRR workarounds

[Lu Baolu]

Hi,

On 12/12/19 3:46 AM, Barret Rhoden via iommu wrote:
> I can imagine a bunch of ways around this.
> 
> One option is to hook in a check for buggy RMRRs in intel-iommu.c.  If
> the base and end are 0, just ignore the entry.  That works for my
> specific buggy DMAR entry.  There might be other buggy entries out
> there.  The docs specify some requirements on the base and end (called
> limit) addresses.
> 
> Another option is to change the sanity check so that unmapped ranges are
> considered OK.  That would work for my case, but only because we're
> hiding the firmware bug: my DMAR has a bad RMRR that happens to fall into a
> reserved or non-existent range.  The downside here is that we'd
> presumably be setting up an IOMMU mapping for this bad RMRR.  But at
> least it's not pointing to any RAM we're using.  (That's actually what
> goes on in the current, non-kexec case for me.  Phys page 0 is marked
> RESERVED, and I have an RMRR that points to it.)  This option also would
> cover any buggy firmwares that use an actual RMRR that pointed to memory
> that was omitted from the e820 map.
> 
> A third option: whenever the RMRR sanity check fails, just ignore it and
> return 0.  Don't set up the rmrru.  Right now, we completely abort DMAR
> processing.  If there was an actual device that needed to poke this
> memory that failed the sanity check (meaning, not RESERVED, currently),
> then we're already in trouble; that device could clobber RAM, right?  If
> we're going to use the IOMMU, I'd rather the device be behind an IOMMU
> with*no*  mapping for the region, so it couldn't clobber whatever we
> happened to put in that location.
> 
> I actually think all three options are reasonable ideas independently of
> one another.  This patchset that does all three.  Please take at least
> one of them.  =)  (May require a slight revision if you don't take all
> of them).

The VT-d spec defines the BIOS considerations about RMRR in section 8.4:

"
BIOS must report the RMRR reported memory addresses as reserved (or as
EFI runtime) in the system memory map returned through methods such as
INT15, EFI GetMemoryMap etc.
"

So we should treat it as firmware bug if the RMRR range is not mapped as
RESERVED in the system memory map table.

As for how should the driver handle this case, ignoring buggy RMRR with
a warning message might be a possible choice.

Best regards,
baolu

Re: [PATCH 0/3] iommu/vt-d bad RMRR workarounds

[Lu Baolu]

Hi,

On 12/12/19 3:46 AM, Barret Rhoden via iommu wrote:
> I can imagine a bunch of ways around this.
> 
> One option is to hook in a check for buggy RMRRs in intel-iommu.c.  If
> the base and end are 0, just ignore the entry.  That works for my
> specific buggy DMAR entry.  There might be other buggy entries out
> there.  The docs specify some requirements on the base and end (called
> limit) addresses.
> 
> Another option is to change the sanity check so that unmapped ranges are
> considered OK.  That would work for my case, but only because we're
> hiding the firmware bug: my DMAR has a bad RMRR that happens to fall into a
> reserved or non-existent range.  The downside here is that we'd
> presumably be setting up an IOMMU mapping for this bad RMRR.  But at
> least it's not pointing to any RAM we're using.  (That's actually what
> goes on in the current, non-kexec case for me.  Phys page 0 is marked
> RESERVED, and I have an RMRR that points to it.)  This option also would
> cover any buggy firmwares that use an actual RMRR that pointed to memory
> that was omitted from the e820 map.
> 
> A third option: whenever the RMRR sanity check fails, just ignore it and
> return 0.  Don't set up the rmrru.  Right now, we completely abort DMAR
> processing.  If there was an actual device that needed to poke this
> memory that failed the sanity check (meaning, not RESERVED, currently),
> then we're already in trouble; that device could clobber RAM, right?  If
> we're going to use the IOMMU, I'd rather the device be behind an IOMMU
> with*no*  mapping for the region, so it couldn't clobber whatever we
> happened to put in that location.
> 
> I actually think all three options are reasonable ideas independently of
> one another.  This patchset that does all three.  Please take at least
> one of them.  =)  (May require a slight revision if you don't take all
> of them).

The VT-d spec defines the BIOS considerations about RMRR in section 8.4:

"
BIOS must report the RMRR reported memory addresses as reserved (or as
EFI runtime) in the system memory map returned through methods such as
INT15, EFI GetMemoryMap etc.
"

So we should treat it as firmware bug if the RMRR range is not mapped as
RESERVED in the system memory map table.

As for how should the driver handle this case, ignoring buggy RMRR with
a warning message might be a possible choice.

Best regards,
baolu
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 0/3] iommu/vt-d bad RMRR workarounds

[Barret Rhoden]

On 12/11/19 9:43 PM, Lu Baolu wrote:
> The VT-d spec defines the BIOS considerations about RMRR in section 8.4:
> 
> "
> BIOS must report the RMRR reported memory addresses as reserved (or as
> EFI runtime) in the system memory map returned through methods such as
> INT15, EFI GetMemoryMap etc.
> "
> 
> So we should treat it as firmware bug if the RMRR range is not mapped as
> RESERVED in the system memory map table.
> 
> As for how should the driver handle this case, ignoring buggy RMRR with
> a warning message might be a possible choice.

Agreed, firmware should not be doing this.  My first patch just skips 
those entries, instead of aborting DMAR processing, and keeps the warning.

So long as the machine still boots in a safe manner, I'm reasonably happy.

Thanks,

Barret

Re: [PATCH 0/3] iommu/vt-d bad RMRR workarounds

[Barret Rhoden via iommu]

On 12/11/19 9:43 PM, Lu Baolu wrote:
> The VT-d spec defines the BIOS considerations about RMRR in section 8.4:
> 
> "
> BIOS must report the RMRR reported memory addresses as reserved (or as
> EFI runtime) in the system memory map returned through methods such as
> INT15, EFI GetMemoryMap etc.
> "
> 
> So we should treat it as firmware bug if the RMRR range is not mapped as
> RESERVED in the system memory map table.
> 
> As for how should the driver handle this case, ignoring buggy RMRR with
> a warning message might be a possible choice.

Agreed, firmware should not be doing this.  My first patch just skips 
those entries, instead of aborting DMAR processing, and keeps the warning.

So long as the machine still boots in a safe manner, I'm reasonably happy.

Thanks,

Barret


_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 0/3] iommu/vt-d bad RMRR workarounds

[Lu Baolu]

On 12/13/19 10:31 PM, Barret Rhoden wrote:
> On 12/11/19 9:43 PM, Lu Baolu wrote:
>> The VT-d spec defines the BIOS considerations about RMRR in section 8.4:
>>
>> "
>> BIOS must report the RMRR reported memory addresses as reserved (or as
>> EFI runtime) in the system memory map returned through methods such as
>> INT15, EFI GetMemoryMap etc.
>> "
>>
>> So we should treat it as firmware bug if the RMRR range is not mapped as
>> RESERVED in the system memory map table.
>>
>> As for how should the driver handle this case, ignoring buggy RMRR with
>> a warning message might be a possible choice.
> 
> Agreed, firmware should not be doing this.  My first patch just skips 
> those entries, instead of aborting DMAR processing, and keeps the warning.
>

Hi Yian,

Does this work for you?

Best regards,
baolu


> So long as the machine still boots in a safe manner, I'm reasonably happy.
> 
> Thanks,
> 
> Barret
> 
> 

Re: [PATCH 0/3] iommu/vt-d bad RMRR workarounds

[Lu Baolu]

On 12/13/19 10:31 PM, Barret Rhoden wrote:
> On 12/11/19 9:43 PM, Lu Baolu wrote:
>> The VT-d spec defines the BIOS considerations about RMRR in section 8.4:
>>
>> "
>> BIOS must report the RMRR reported memory addresses as reserved (or as
>> EFI runtime) in the system memory map returned through methods such as
>> INT15, EFI GetMemoryMap etc.
>> "
>>
>> So we should treat it as firmware bug if the RMRR range is not mapped as
>> RESERVED in the system memory map table.
>>
>> As for how should the driver handle this case, ignoring buggy RMRR with
>> a warning message might be a possible choice.
> 
> Agreed, firmware should not be doing this.  My first patch just skips 
> those entries, instead of aborting DMAR processing, and keeps the warning.
>

Hi Yian,

Does this work for you?

Best regards,
baolu


> So long as the machine still boots in a safe manner, I'm reasonably happy.
> 
> Thanks,
> 
> Barret
> 
> 
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 1/3] iommu/vt-d: skip RMRR entries that fail the sanity check

[Chen, Yian]

On 12/11/2019 11:46 AM, Barret Rhoden wrote:
> RMRR entries describe memory regions that are DMA targets for devices
> outside the kernel's control.
>
> RMRR entries that fail the sanity check are pointing to regions of
> memory that the firmware did not tell the kernel are reserved or
> otherwise should not be used.
>
> Instead of aborting DMAR processing, this commit skips these RMRR
> entries.  They will not be mapped into the IOMMU, but the IOMMU can
> still be utilized.  If anything, when the IOMMU is on, those devices
> will not be able to clobber RAM that the kernel has allocated from those
> regions.
>
> Signed-off-by: Barret Rhoden <brho@google.com>
> ---
>   drivers/iommu/intel-iommu.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
> index f168cd8ee570..f7e09244c9e4 100644
> --- a/drivers/iommu/intel-iommu.c
> +++ b/drivers/iommu/intel-iommu.c
> @@ -4316,7 +4316,7 @@ int __init dmar_parse_one_rmrr(struct acpi_dmar_header *header, void *arg)
>   	rmrr = (struct acpi_dmar_reserved_memory *)header;
>   	ret = arch_rmrr_sanity_check(rmrr);
>   	if (ret)
> -		return ret;
> +		return 0;
>   
>   	rmrru = kzalloc(sizeof(*rmrru), GFP_KERNEL);
>   	if (!rmrru)
Parsing rmrr function should report the error to caller. The behavior to 
response the error can be
chose  by the caller in the calling stack, for example, 
dmar_walk_remapping_entries().
A concern is that ignoring a detected firmware bug might have a 
potential side impact though
it seemed safe for your case.

Thanks,
Yian

Re: [PATCH 1/3] iommu/vt-d: skip RMRR entries that fail the sanity check

[Chen, Yian]

On 12/11/2019 11:46 AM, Barret Rhoden wrote:
> RMRR entries describe memory regions that are DMA targets for devices
> outside the kernel's control.
>
> RMRR entries that fail the sanity check are pointing to regions of
> memory that the firmware did not tell the kernel are reserved or
> otherwise should not be used.
>
> Instead of aborting DMAR processing, this commit skips these RMRR
> entries.  They will not be mapped into the IOMMU, but the IOMMU can
> still be utilized.  If anything, when the IOMMU is on, those devices
> will not be able to clobber RAM that the kernel has allocated from those
> regions.
>
> Signed-off-by: Barret Rhoden <brho@google.com>
> ---
>   drivers/iommu/intel-iommu.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
> index f168cd8ee570..f7e09244c9e4 100644
> --- a/drivers/iommu/intel-iommu.c
> +++ b/drivers/iommu/intel-iommu.c
> @@ -4316,7 +4316,7 @@ int __init dmar_parse_one_rmrr(struct acpi_dmar_header *header, void *arg)
>   	rmrr = (struct acpi_dmar_reserved_memory *)header;
>   	ret = arch_rmrr_sanity_check(rmrr);
>   	if (ret)
> -		return ret;
> +		return 0;
>   
>   	rmrru = kzalloc(sizeof(*rmrru), GFP_KERNEL);
>   	if (!rmrru)
Parsing rmrr function should report the error to caller. The behavior to 
response the error can be
chose  by the caller in the calling stack, for example, 
dmar_walk_remapping_entries().
A concern is that ignoring a detected firmware bug might have a 
potential side impact though
it seemed safe for your case.

Thanks,
Yian
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 0/3] iommu/vt-d bad RMRR workarounds

[Chen, Yian]

On 12/13/2019 5:52 PM, Lu Baolu wrote:
>
> On 12/13/19 10:31 PM, Barret Rhoden wrote:
>> On 12/11/19 9:43 PM, Lu Baolu wrote:
>>> The VT-d spec defines the BIOS considerations about RMRR in section 
>>> 8.4:
>>>
>>> "
>>> BIOS must report the RMRR reported memory addresses as reserved (or as
>>> EFI runtime) in the system memory map returned through methods such as
>>> INT15, EFI GetMemoryMap etc.
>>> "
>>>
>>> So we should treat it as firmware bug if the RMRR range is not 
>>> mapped as
>>> RESERVED in the system memory map table.
>>>
>>> As for how should the driver handle this case, ignoring buggy RMRR with
>>> a warning message might be a possible choice.
>>
>> Agreed, firmware should not be doing this.  My first patch just skips 
>> those entries, instead of aborting DMAR processing, and keeps the 
>> warning.
>>
>
> Hi Yian,
>
> Does this work for you?
>
> Best regards,
> baolu
>
>
I made a comment in the the patch email "[PATCH 1/3] iommu/vt-d: skip 
RMRR entries that fail the sanity check "
thanks,
Yian

>> So long as the machine still boots in a safe manner, I'm reasonably 
>> happy.
>>
>> Thanks,
>>
>> Barret
>>
>>

Re: [PATCH 0/3] iommu/vt-d bad RMRR workarounds

[Chen, Yian]

On 12/13/2019 5:52 PM, Lu Baolu wrote:
>
> On 12/13/19 10:31 PM, Barret Rhoden wrote:
>> On 12/11/19 9:43 PM, Lu Baolu wrote:
>>> The VT-d spec defines the BIOS considerations about RMRR in section 
>>> 8.4:
>>>
>>> "
>>> BIOS must report the RMRR reported memory addresses as reserved (or as
>>> EFI runtime) in the system memory map returned through methods such as
>>> INT15, EFI GetMemoryMap etc.
>>> "
>>>
>>> So we should treat it as firmware bug if the RMRR range is not 
>>> mapped as
>>> RESERVED in the system memory map table.
>>>
>>> As for how should the driver handle this case, ignoring buggy RMRR with
>>> a warning message might be a possible choice.
>>
>> Agreed, firmware should not be doing this.  My first patch just skips 
>> those entries, instead of aborting DMAR processing, and keeps the 
>> warning.
>>
>
> Hi Yian,
>
> Does this work for you?
>
> Best regards,
> baolu
>
>
I made a comment in the the patch email "[PATCH 1/3] iommu/vt-d: skip 
RMRR entries that fail the sanity check "
thanks,
Yian

>> So long as the machine still boots in a safe manner, I'm reasonably 
>> happy.
>>
>> Thanks,
>>
>> Barret
>>
>>

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 1/3] iommu/vt-d: skip RMRR entries that fail the sanity check

[Barret Rhoden]

On 12/16/19 2:07 PM, Chen, Yian wrote:
> 
> 
> On 12/11/2019 11:46 AM, Barret Rhoden wrote:
>> RMRR entries describe memory regions that are DMA targets for devices
>> outside the kernel's control.
>>
>> RMRR entries that fail the sanity check are pointing to regions of
>> memory that the firmware did not tell the kernel are reserved or
>> otherwise should not be used.
>>
>> Instead of aborting DMAR processing, this commit skips these RMRR
>> entries.  They will not be mapped into the IOMMU, but the IOMMU can
>> still be utilized.  If anything, when the IOMMU is on, those devices
>> will not be able to clobber RAM that the kernel has allocated from those
>> regions.
>>
>> Signed-off-by: Barret Rhoden <brho@google.com>
>> ---
>>   drivers/iommu/intel-iommu.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
>> index f168cd8ee570..f7e09244c9e4 100644
>> --- a/drivers/iommu/intel-iommu.c
>> +++ b/drivers/iommu/intel-iommu.c
>> @@ -4316,7 +4316,7 @@ int __init dmar_parse_one_rmrr(struct 
>> acpi_dmar_header *header, void *arg)
>>       rmrr = (struct acpi_dmar_reserved_memory *)header;
>>       ret = arch_rmrr_sanity_check(rmrr);
>>       if (ret)
>> -        return ret;
>> +        return 0;
>>       rmrru = kzalloc(sizeof(*rmrru), GFP_KERNEL);
>>       if (!rmrru)
> Parsing rmrr function should report the error to caller. The behavior to 
> response the error can be
> chose  by the caller in the calling stack, for example, 
> dmar_walk_remapping_entries().
> A concern is that ignoring a detected firmware bug might have a 
> potential side impact though
> it seemed safe for your case.

That's a little difficult given the current code.  Once we are in
dmar_walk_remapping_entries(), the specific function (parse_one_rmrr) is 
called via callback:

	ret = cb->cb[iter->type](iter, cb->arg[iter->type]);
	if (ret)
		return ret;

If there's an error of any sort, it aborts the walk.  Handling the 
specific errors here is difficult, since we don't know what the errors 
mean to the specific callback.  Is there some errno we can use that 
means "there was a problem, but it's not so bad that you have to abort, 
but I figured you ought to know"?  Not that I think that's a good idea.

The knowledge of whether or not a specific error is worth aborting all 
DMAR functionality is best known inside the specific callback.  The only 
handling to do is print a warning and either skip it or abort.

I think skipping the entry for a bad RMRR is better than aborting 
completely, though I understand if people don't like that.  It's 
debatable.  By aborting, we lose the ability to use the IOMMU at all, 
but we are still in a situation where the devices using the RMRR regions 
might be clobbering kernel memory, right?  Using the IOMMU (with no 
mappings for the bad RMRRs) would stop those devices from clobbering memory.

Regardless, I have two other patches in this series that could resolve 
the problem for me and probably other people.  I'd just like at least 
one of the three patches to get merged so that my machine boots when the 
original commit f036c7fa0ab6 ("iommu/vt-d: Check VT-d RMRR region in 
BIOS is reported as reserved") gets released.

Thanks,

Barret

Re: [PATCH 1/3] iommu/vt-d: skip RMRR entries that fail the sanity check

[Barret Rhoden via iommu]

On 12/16/19 2:07 PM, Chen, Yian wrote:
> 
> 
> On 12/11/2019 11:46 AM, Barret Rhoden wrote:
>> RMRR entries describe memory regions that are DMA targets for devices
>> outside the kernel's control.
>>
>> RMRR entries that fail the sanity check are pointing to regions of
>> memory that the firmware did not tell the kernel are reserved or
>> otherwise should not be used.
>>
>> Instead of aborting DMAR processing, this commit skips these RMRR
>> entries.  They will not be mapped into the IOMMU, but the IOMMU can
>> still be utilized.  If anything, when the IOMMU is on, those devices
>> will not be able to clobber RAM that the kernel has allocated from those
>> regions.
>>
>> Signed-off-by: Barret Rhoden <brho@google.com>
>> ---
>>   drivers/iommu/intel-iommu.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
>> index f168cd8ee570..f7e09244c9e4 100644
>> --- a/drivers/iommu/intel-iommu.c
>> +++ b/drivers/iommu/intel-iommu.c
>> @@ -4316,7 +4316,7 @@ int __init dmar_parse_one_rmrr(struct 
>> acpi_dmar_header *header, void *arg)
>>       rmrr = (struct acpi_dmar_reserved_memory *)header;
>>       ret = arch_rmrr_sanity_check(rmrr);
>>       if (ret)
>> -        return ret;
>> +        return 0;
>>       rmrru = kzalloc(sizeof(*rmrru), GFP_KERNEL);
>>       if (!rmrru)
> Parsing rmrr function should report the error to caller. The behavior to 
> response the error can be
> chose  by the caller in the calling stack, for example, 
> dmar_walk_remapping_entries().
> A concern is that ignoring a detected firmware bug might have a 
> potential side impact though
> it seemed safe for your case.

That's a little difficult given the current code.  Once we are in
dmar_walk_remapping_entries(), the specific function (parse_one_rmrr) is 
called via callback:

	ret = cb->cb[iter->type](iter, cb->arg[iter->type]);
	if (ret)
		return ret;

If there's an error of any sort, it aborts the walk.  Handling the 
specific errors here is difficult, since we don't know what the errors 
mean to the specific callback.  Is there some errno we can use that 
means "there was a problem, but it's not so bad that you have to abort, 
but I figured you ought to know"?  Not that I think that's a good idea.

The knowledge of whether or not a specific error is worth aborting all 
DMAR functionality is best known inside the specific callback.  The only 
handling to do is print a warning and either skip it or abort.

I think skipping the entry for a bad RMRR is better than aborting 
completely, though I understand if people don't like that.  It's 
debatable.  By aborting, we lose the ability to use the IOMMU at all, 
but we are still in a situation where the devices using the RMRR regions 
might be clobbering kernel memory, right?  Using the IOMMU (with no 
mappings for the bad RMRRs) would stop those devices from clobbering memory.

Regardless, I have two other patches in this series that could resolve 
the problem for me and probably other people.  I'd just like at least 
one of the three patches to get merged so that my machine boots when the 
original commit f036c7fa0ab6 ("iommu/vt-d: Check VT-d RMRR region in 
BIOS is reported as reserved") gets released.

Thanks,

Barret

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 1/3] iommu/vt-d: skip RMRR entries that fail the sanity check

[Chen, Yian]

On 12/16/2019 11:35 AM, Barret Rhoden wrote:
> On 12/16/19 2:07 PM, Chen, Yian wrote:
>>
>>
>> On 12/11/2019 11:46 AM, Barret Rhoden wrote:
>>> RMRR entries describe memory regions that are DMA targets for devices
>>> outside the kernel's control.
>>>
>>> RMRR entries that fail the sanity check are pointing to regions of
>>> memory that the firmware did not tell the kernel are reserved or
>>> otherwise should not be used.
>>>
>>> Instead of aborting DMAR processing, this commit skips these RMRR
>>> entries.  They will not be mapped into the IOMMU, but the IOMMU can
>>> still be utilized.  If anything, when the IOMMU is on, those devices
>>> will not be able to clobber RAM that the kernel has allocated from 
>>> those
>>> regions.
>>>
>>> Signed-off-by: Barret Rhoden <brho@google.com>
>>> ---
>>>   drivers/iommu/intel-iommu.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
>>> index f168cd8ee570..f7e09244c9e4 100644
>>> --- a/drivers/iommu/intel-iommu.c
>>> +++ b/drivers/iommu/intel-iommu.c
>>> @@ -4316,7 +4316,7 @@ int __init dmar_parse_one_rmrr(struct 
>>> acpi_dmar_header *header, void *arg)
>>>       rmrr = (struct acpi_dmar_reserved_memory *)header;
>>>       ret = arch_rmrr_sanity_check(rmrr);
>>>       if (ret)
>>> -        return ret;
>>> +        return 0;
>>>       rmrru = kzalloc(sizeof(*rmrru), GFP_KERNEL);
>>>       if (!rmrru)
>> Parsing rmrr function should report the error to caller. The behavior 
>> to response the error can be
>> chose  by the caller in the calling stack, for example, 
>> dmar_walk_remapping_entries().
>> A concern is that ignoring a detected firmware bug might have a 
>> potential side impact though
>> it seemed safe for your case.
>
> That's a little difficult given the current code.  Once we are in
> dmar_walk_remapping_entries(), the specific function (parse_one_rmrr) 
> is called via callback:
>
>     ret = cb->cb[iter->type](iter, cb->arg[iter->type]);
>     if (ret)
>         return ret;
>
> If there's an error of any sort, it aborts the walk.  Handling the 
> specific errors here is difficult, since we don't know what the errors 
> mean to the specific callback.  Is there some errno we can use that 
> means "there was a problem, but it's not so bad that you have to 
> abort, but I figured you ought to know"?  Not that I think that's a 
> good idea.
>
> The knowledge of whether or not a specific error is worth aborting all 
> DMAR functionality is best known inside the specific callback.  The 
> only handling to do is print a warning and either skip it or abort.
>
> I think skipping the entry for a bad RMRR is better than aborting 
> completely, though I understand if people don't like that.  It's 
> debatable.  By aborting, we lose the ability to use the IOMMU at all, 
> but we are still in a situation where the devices using the RMRR 
> regions might be clobbering kernel memory, right?  Using the IOMMU 
> (with no mappings for the bad RMRRs) would stop those devices from 
> clobbering memory.
>
> Regardless, I have two other patches in this series that could resolve 
> the problem for me and probably other people.  I'd just like at least 
> one of the three patches to get merged so that my machine boots when 
> the original commit f036c7fa0ab6 ("iommu/vt-d: Check VT-d RMRR region 
> in BIOS is reported as reserved") gets released.
>
when a firmware bug appears, the potential problem may beyond the scope 
of its visible impacts so that introducing a workaround in official 
implementation should be considered very carefully.

If the workaround is really needed at this point, I would recommend 
adding a WARN_TAINT with TAINT_FIRMWARE_WORKAROUND, to tell the 
workaround is in the place.

Thanks
Yian

> Thanks,
>
> Barret
>

Re: [PATCH 1/3] iommu/vt-d: skip RMRR entries that fail the sanity check

[Chen, Yian]

On 12/16/2019 11:35 AM, Barret Rhoden wrote:
> On 12/16/19 2:07 PM, Chen, Yian wrote:
>>
>>
>> On 12/11/2019 11:46 AM, Barret Rhoden wrote:
>>> RMRR entries describe memory regions that are DMA targets for devices
>>> outside the kernel's control.
>>>
>>> RMRR entries that fail the sanity check are pointing to regions of
>>> memory that the firmware did not tell the kernel are reserved or
>>> otherwise should not be used.
>>>
>>> Instead of aborting DMAR processing, this commit skips these RMRR
>>> entries.  They will not be mapped into the IOMMU, but the IOMMU can
>>> still be utilized.  If anything, when the IOMMU is on, those devices
>>> will not be able to clobber RAM that the kernel has allocated from 
>>> those
>>> regions.
>>>
>>> Signed-off-by: Barret Rhoden <brho@google.com>
>>> ---
>>>   drivers/iommu/intel-iommu.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
>>> index f168cd8ee570..f7e09244c9e4 100644
>>> --- a/drivers/iommu/intel-iommu.c
>>> +++ b/drivers/iommu/intel-iommu.c
>>> @@ -4316,7 +4316,7 @@ int __init dmar_parse_one_rmrr(struct 
>>> acpi_dmar_header *header, void *arg)
>>>       rmrr = (struct acpi_dmar_reserved_memory *)header;
>>>       ret = arch_rmrr_sanity_check(rmrr);
>>>       if (ret)
>>> -        return ret;
>>> +        return 0;
>>>       rmrru = kzalloc(sizeof(*rmrru), GFP_KERNEL);
>>>       if (!rmrru)
>> Parsing rmrr function should report the error to caller. The behavior 
>> to response the error can be
>> chose  by the caller in the calling stack, for example, 
>> dmar_walk_remapping_entries().
>> A concern is that ignoring a detected firmware bug might have a 
>> potential side impact though
>> it seemed safe for your case.
>
> That's a little difficult given the current code.  Once we are in
> dmar_walk_remapping_entries(), the specific function (parse_one_rmrr) 
> is called via callback:
>
>     ret = cb->cb[iter->type](iter, cb->arg[iter->type]);
>     if (ret)
>         return ret;
>
> If there's an error of any sort, it aborts the walk.  Handling the 
> specific errors here is difficult, since we don't know what the errors 
> mean to the specific callback.  Is there some errno we can use that 
> means "there was a problem, but it's not so bad that you have to 
> abort, but I figured you ought to know"?  Not that I think that's a 
> good idea.
>
> The knowledge of whether or not a specific error is worth aborting all 
> DMAR functionality is best known inside the specific callback.  The 
> only handling to do is print a warning and either skip it or abort.
>
> I think skipping the entry for a bad RMRR is better than aborting 
> completely, though I understand if people don't like that.  It's 
> debatable.  By aborting, we lose the ability to use the IOMMU at all, 
> but we are still in a situation where the devices using the RMRR 
> regions might be clobbering kernel memory, right?  Using the IOMMU 
> (with no mappings for the bad RMRRs) would stop those devices from 
> clobbering memory.
>
> Regardless, I have two other patches in this series that could resolve 
> the problem for me and probably other people.  I'd just like at least 
> one of the three patches to get merged so that my machine boots when 
> the original commit f036c7fa0ab6 ("iommu/vt-d: Check VT-d RMRR region 
> in BIOS is reported as reserved") gets released.
>
when a firmware bug appears, the potential problem may beyond the scope 
of its visible impacts so that introducing a workaround in official 
implementation should be considered very carefully.

If the workaround is really needed at this point, I would recommend 
adding a WARN_TAINT with TAINT_FIRMWARE_WORKAROUND, to tell the 
workaround is in the place.

Thanks
Yian

> Thanks,
>
> Barret
>

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 1/3] iommu/vt-d: skip RMRR entries that fail the sanity check

[Barret Rhoden]

On 12/17/19 2:19 PM, Chen, Yian wrote:
>> Regardless, I have two other patches in this series that could resolve 
>> the problem for me and probably other people.  I'd just like at least 
>> one of the three patches to get merged so that my machine boots when 
>> the original commit f036c7fa0ab6 ("iommu/vt-d: Check VT-d RMRR region 
>> in BIOS is reported as reserved") gets released.
>>
> when a firmware bug appears, the potential problem may beyond the scope 
> of its visible impacts so that introducing a workaround in official 
> implementation should be considered very carefully.

Agreed.  I think that in the RMRR case, it wouldn't surprise me if these 
problems are already occurring, and we just didn't know about it, so I'd 
like to think about sane workarounds.  I only noticed it on a kexec. 
Not sure how many people with similarly-broken firmware are kexecing 
kernels on linus/master kernels yet.

Specifically, my firmware reports an RMRR with start == 0 and end == 0 
(end should be page-aligned-minus-one).  The only reason commit 
f036c7fa0ab6 didn't catch it on a full reboot is that trim_bios_range() 
reserved the first page, assuming that the BIOS meant to reserve it but 
just didn't tell us in the e820 map.  My firmware didn't mark that first 
page E820_RESERVED.  On a kexec, the range that got trimmed was 
0x100-0xfff instead of 0x000-0xfff.  In both cases, the kernel won't use 
the region the broken RMRR points to, but in the kexec case, it wasn't 
E820_RESERVED, so the new commit aborted the DMAR setup.

> If the workaround is really needed at this point, I would recommend 
> adding a WARN_TAINT with TAINT_FIRMWARE_WORKAROUND, to tell the 
> workaround is in the place.

Sounds good.  I can rework the patchset so that whenever I skip an RMRR 
entry or whatnot, I'll put in a WARN_TAINT.  I see a few other examples 
in dmar.c to work from.

If any of the three changes are too aggressive, I'm OK with you all 
taking just one of them.  I'd like to be able to kexec with the new 
kernel.  I'm likely not the only one with bad firmware, and any bug that 
only shows up on a kexec often a pain to detect.

Thanks,

Barret

Re: [PATCH 1/3] iommu/vt-d: skip RMRR entries that fail the sanity check

[Barret Rhoden via iommu]

On 12/17/19 2:19 PM, Chen, Yian wrote:
>> Regardless, I have two other patches in this series that could resolve 
>> the problem for me and probably other people.  I'd just like at least 
>> one of the three patches to get merged so that my machine boots when 
>> the original commit f036c7fa0ab6 ("iommu/vt-d: Check VT-d RMRR region 
>> in BIOS is reported as reserved") gets released.
>>
> when a firmware bug appears, the potential problem may beyond the scope 
> of its visible impacts so that introducing a workaround in official 
> implementation should be considered very carefully.

Agreed.  I think that in the RMRR case, it wouldn't surprise me if these 
problems are already occurring, and we just didn't know about it, so I'd 
like to think about sane workarounds.  I only noticed it on a kexec. 
Not sure how many people with similarly-broken firmware are kexecing 
kernels on linus/master kernels yet.

Specifically, my firmware reports an RMRR with start == 0 and end == 0 
(end should be page-aligned-minus-one).  The only reason commit 
f036c7fa0ab6 didn't catch it on a full reboot is that trim_bios_range() 
reserved the first page, assuming that the BIOS meant to reserve it but 
just didn't tell us in the e820 map.  My firmware didn't mark that first 
page E820_RESERVED.  On a kexec, the range that got trimmed was 
0x100-0xfff instead of 0x000-0xfff.  In both cases, the kernel won't use 
the region the broken RMRR points to, but in the kexec case, it wasn't 
E820_RESERVED, so the new commit aborted the DMAR setup.

> If the workaround is really needed at this point, I would recommend 
> adding a WARN_TAINT with TAINT_FIRMWARE_WORKAROUND, to tell the 
> workaround is in the place.

Sounds good.  I can rework the patchset so that whenever I skip an RMRR 
entry or whatnot, I'll put in a WARN_TAINT.  I see a few other examples 
in dmar.c to work from.

If any of the three changes are too aggressive, I'm OK with you all 
taking just one of them.  I'd like to be able to kexec with the new 
kernel.  I'm likely not the only one with bad firmware, and any bug that 
only shows up on a kexec often a pain to detect.

Thanks,

Barret

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu