[PATCH] iommu/amd: Reserve exclusion range in iova-domain

[PATCH] iommu/amd: Reserve exclusion range in iova-domain

[Joerg Roedel]

From: Joerg Roedel <jroedel@suse.de>

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 <jroedel@suse.de>
---
 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

Re: [PATCH] iommu/amd: Reserve exclusion range in iova-domain

[Gary R Hook]

On 3/28/19 5:44 AM, Joerg Roedel wrote:
> From: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>
> 
> 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 <jroedel-l3A5Bk7waGM@public.gmane.org>
> ---
>   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.

Re: [PATCH] iommu/amd: Reserve exclusion range in iova-domain

[Gary R Hook]

On 3/28/19 5:44 AM, Joerg Roedel wrote:
> From: Joerg Roedel <jroedel@suse.de>
> 
> 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 <jroedel@suse.de>
> ---
>   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.

Re: [PATCH] iommu/amd: Reserve exclusion range in iova-domain

[Joerg Roedel]

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

Re: [PATCH] iommu/amd: Reserve exclusion range in iova-domain

[Gary R Hook]

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

Re: [PATCH] iommu/amd: Reserve exclusion range in iova-domain

[Stuart Hayes]

Tested on a Dell PowerEdge R7425 system on which this problem is easily reproducible.

Tested-by: Stuart Hayes <stuart.w.hayes@gmail.com>

Re: [PATCH] iommu/amd: Reserve exclusion range in iova-domain

[Stuart Hayes]

Tested on a Dell PowerEdge R7425 system on which this problem is easily reproducible.

Tested-by: Stuart Hayes <stuart.w.hayes@gmail.com>