lkml.org 
[lkml]   [2019]   [Dec]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 1/3] iommu/vt-d: skip RMRR entries that fail the sanity check
From
Date


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
>

\
 
 \ /
  Last update: 2019-12-17 20:20    [W:0.045 / U:1.648 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site