From: Joerg Roedel <[email protected]>
If a device has an exclusion range specified in the IVRS
table, this region needs to be reserved in the iova-domain
of that device. This hasn't happened until now and can cause
data corruption on data transfered with these devices.
Treat exclusion ranges as reserved regions in the iommu-core
to fix the problem.
Fixes: be2a022c0dd0 ('x86, AMD IOMMU: add functions to parse IOMMU memory mapping requirements for devices')
Signed-off-by: Joerg Roedel <[email protected]>
---
drivers/iommu/amd_iommu.c | 9 ++++++---
drivers/iommu/amd_iommu_init.c | 7 ++++---
2 files changed, 10 insertions(+), 6 deletions(-)
diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index 21cb088d6687..2a8d2806d5b9 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -3169,21 +3169,24 @@ static void amd_iommu_get_resv_regions(struct device *dev,
return;
list_for_each_entry(entry, &amd_iommu_unity_map, list) {
+ int type, prot = 0;
size_t length;
- int prot = 0;
if (devid < entry->devid_start || devid > entry->devid_end)
continue;
+ type = IOMMU_RESV_DIRECT;
length = entry->address_end - entry->address_start;
if (entry->prot & IOMMU_PROT_IR)
prot |= IOMMU_READ;
if (entry->prot & IOMMU_PROT_IW)
prot |= IOMMU_WRITE;
+ if (entry->prot & (1 << 2))
+ /* Exclusion range */
+ type = IOMMU_RESV_RESERVED;
region = iommu_alloc_resv_region(entry->address_start,
- length, prot,
- IOMMU_RESV_DIRECT);
+ length, prot, type);
if (!region) {
dev_err(dev, "Out of memory allocating dm-regions\n");
return;
diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
index f773792d77fd..1b1378619fc9 100644
--- a/drivers/iommu/amd_iommu_init.c
+++ b/drivers/iommu/amd_iommu_init.c
@@ -2013,6 +2013,9 @@ static int __init init_unity_map_range(struct ivmd_header *m)
if (e == NULL)
return -ENOMEM;
+ if (m->flags & IVMD_FLAG_EXCL_RANGE)
+ init_exclusion_range(m);
+
switch (m->type) {
default:
kfree(e);
@@ -2059,9 +2062,7 @@ static int __init init_memory_definitions(struct acpi_table_header *table)
while (p < end) {
m = (struct ivmd_header *)p;
- if (m->flags & IVMD_FLAG_EXCL_RANGE)
- init_exclusion_range(m);
- else if (m->flags & IVMD_FLAG_UNITY_MAP)
+ if (m->flags & (IVMD_FLAG_UNITY_MAP | IVMD_FLAG_EXCL_RANGE))
init_unity_map_range(m);
p += m->length;
--
2.16.4
On 3/28/19 5:44 AM, Joerg Roedel wrote:
> From: Joerg Roedel <[email protected]>
>
> If a device has an exclusion range specified in the IVRS
> table, this region needs to be reserved in the iova-domain
> of that device. This hasn't happened until now and can cause
> data corruption on data transfered with these devices.
>
> Treat exclusion ranges as reserved regions in the iommu-core
> to fix the problem.
>
> Fixes: be2a022c0dd0 ('x86, AMD IOMMU: add functions to parse IOMMU memory mapping requirements for devices')
> Signed-off-by: Joerg Roedel <[email protected]>
> ---
> drivers/iommu/amd_iommu.c | 9 ++++++---
> drivers/iommu/amd_iommu_init.c | 7 ++++---
> 2 files changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
> index 21cb088d6687..2a8d2806d5b9 100644
> --- a/drivers/iommu/amd_iommu.c
> +++ b/drivers/iommu/amd_iommu.c
> @@ -3169,21 +3169,24 @@ static void amd_iommu_get_resv_regions(struct device *dev,
> return;
>
> list_for_each_entry(entry, &amd_iommu_unity_map, list) {
> + int type, prot = 0;
> size_t length;
> - int prot = 0;
>
> if (devid < entry->devid_start || devid > entry->devid_end)
> continue;
>
> + type = IOMMU_RESV_DIRECT;
> length = entry->address_end - entry->address_start;
> if (entry->prot & IOMMU_PROT_IR)
> prot |= IOMMU_READ;
> if (entry->prot & IOMMU_PROT_IW)
> prot |= IOMMU_WRITE;
> + if (entry->prot & (1 << 2))
Could we add
#define IOMMU_WRITE_EXCL (1 << 2)
to amd_iommu_types.h?
The bit is documented in the AMD IOMMU spec, so this seems safe...
> + /* Exclusion range */
> + type = IOMMU_RESV_RESERVED;
>
> region = iommu_alloc_resv_region(entry->address_start,
> - length, prot,
> - IOMMU_RESV_DIRECT);
> + length, prot, type);
> if (!region) {
> dev_err(dev, "Out of memory allocating dm-regions\n");
> return;
> diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
> index f773792d77fd..1b1378619fc9 100644
> --- a/drivers/iommu/amd_iommu_init.c
> +++ b/drivers/iommu/amd_iommu_init.c
> @@ -2013,6 +2013,9 @@ static int __init init_unity_map_range(struct ivmd_header *m)
> if (e == NULL)
> return -ENOMEM;
>
> + if (m->flags & IVMD_FLAG_EXCL_RANGE)
> + init_exclusion_range(m);
> +
> switch (m->type) {
> default:
> kfree(e);
> @@ -2059,9 +2062,7 @@ static int __init init_memory_definitions(struct acpi_table_header *table)
>
> while (p < end) {
> m = (struct ivmd_header *)p;
> - if (m->flags & IVMD_FLAG_EXCL_RANGE)
> - init_exclusion_range(m);
> - else if (m->flags & IVMD_FLAG_UNITY_MAP)
> + if (m->flags & (IVMD_FLAG_UNITY_MAP | IVMD_FLAG_EXCL_RANGE))
> init_unity_map_range(m);
>
> p += m->length;
>
The problem I see here is that if, for some untold reason, there is more
than one exclusion range emitted, where only the last one wins in the
hardware, we'd still end up with more than one range reserved in the
IOVA tree. Clearly, this is extremely unlikely, but wouldn't we want to
protect against that sort of misuse/mistake?
I could be missing something.
Hi Gary,
On Thu, Mar 28, 2019 at 02:52:19PM +0000, Gary R Hook wrote:
> On 3/28/19 5:44 AM, Joerg Roedel wrote:
> > + if (entry->prot & (1 << 2))
>
> Could we add #define IOMMU_WRITE_EXCL (1 << 2) to amd_iommu_types.h?
Yes, I replace that magic number with a descriptive name.
> The problem I see here is that if, for some untold reason, there is more
> than one exclusion range emitted, where only the last one wins in the
> hardware, we'd still end up with more than one range reserved in the
> IOVA tree. Clearly, this is extremely unlikely, but wouldn't we want to
> protect against that sort of misuse/mistake?
>
> I could be missing something.
No, you are not, this could still be a problem. Until now it isn't,
because this week was the first time I have every seen an AMD IOMMU
system making use of exclusion ranges, and it doesn't have this problem.
But this problem has been in the code even before this patch and needs
to be addressed separatly. I think it is the best option to cancel IOMMU
initialization when the IVRS table defines conflicting exclusion ranges
for a single IOMMU instance.
Regards,
Joerg
On 3/29/19 9:51 AM, Joerg Roedel wrote:
> Hi Gary,
>
> On Thu, Mar 28, 2019 at 02:52:19PM +0000, Gary R Hook wrote:
>> On 3/28/19 5:44 AM, Joerg Roedel wrote:
>>> + if (entry->prot & (1 << 2))
>>
>> Could we add #define IOMMU_WRITE_EXCL (1 << 2) to amd_iommu_types.h?
>
> Yes, I replace that magic number with a descriptive name.
Super; thanks.
>> The problem I see here is that if, for some untold reason, there is more
>> than one exclusion range emitted, where only the last one wins in the
>> hardware, we'd still end up with more than one range reserved in the
>> IOVA tree. Clearly, this is extremely unlikely, but wouldn't we want to
>> protect against that sort of misuse/mistake?
>>
>> I could be missing something.
>
> No, you are not, this could still be a problem. Until now it isn't,
> because this week was the first time I have every seen an AMD IOMMU
> system making use of exclusion ranges, and it doesn't have this problem.
>
> But this problem has been in the code even before this patch and needs
> to be addressed separatly. I think it is the best option to cancel IOMMU
> initialization when the IVRS table defines conflicting exclusion ranges
> for a single IOMMU instance.
Really? Interesting. May I ask who, as I've not seen it yet, either?
This change accomplishes the goal of setting exclusion + reserving the
IOVA range, and is verified with testing? Cool. One wonders if they've
considered a unity range?
Agree that addressing the uniqueness issue separately is fine. I'd
probably prefer "first one wins" until IOVA tree clean-up could be
implemented for "last one wins".... but I'm seeing that there are at
least two spots in the code that need attention. I may go experiment
with this.
All told, this is really about handling a corner case, as the likelihood
of a BIOS trying to set up >1 exclusion ranges seems improbable, if not
impossible.
grh
Tested on a Dell PowerEdge R7425 system on which this problem is easily reproducible.
Tested-by: Stuart Hayes <[email protected]>
Tested on a Dell PowerEdge R7425 system on which this problem is easily reproducible.
Tested-by: Stuart Hayes <[email protected]>