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
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 <[email protected]>
---
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
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
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
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
>
>
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