ACPI/IORT: Do not blindly trust DMA masks from firmware

Address issue observed on real world system with suboptimal IORT table where DMA masks of PCI devices would get set to 0 as result.

iort_dma_setup() would query the root complex'/named component IORT entry for a DMA mask, and use that over the one the device has been configured with earlier.

Ideally we want to use the minimum mask of what the IORT contains for the root complex and what the device was configured with.

Fixes 5ac65e8c8941 ("ACPI/IORT: Support address size limit for root complexes")

Signed-off-by Moritz Fischer <[email protected]>

---

Changes from v1:
- Changed warning to FW_BUG
- Warn for both Named Component or Root Complex
- Replaced min_not_zero() with min()

---
  drivers/acpi/arm64/iort.c | 14 ++++++++++++--
  1 file changed, 12 insertions(+), 2 deletions(-)

drivers/acpi/arm64/iort.c +10 -0

index d4eac6d7e9fb..2494138a6905 100644
@@ -1107,6 +1107,11 @@ static int nc_dma_get_range(struct device *dev, u64 *size)
 
  	ncomp = (struct acpi_iort_named_component *)node->node_data;
 
+	if (!ncomp->memory_address_limit) {
+		pr_warn(FW_BUG "Named component missing memory address limit\n");
+		return -EINVAL;
+	}
+
  	*size = ncomp->memory_address_limit >= 64 ? U64_MAX :
  			1ULL<<ncomp->memory_address_limit;
 

@@ -1126,6 +1131,11 @@ static int rc_dma_get_range(struct device *dev, u64 *size)
 
  	rc = (struct acpi_iort_root_complex *)node->node_data;
 
+	if (!rc->memory_address_limit) {
+		pr_warn(FW_BUG "Root complex missing memory address limit\n");
+		return -EINVAL;
+	}
+
  	*size = rc->memory_address_limit >= 64 ? U64_MAX :
  			1ULL<<rc->memory_address_limit;
 

Quoted 46 lines · Moritz Fischer

Address issue observed on real world system with suboptimal IORT table where DMA masks of PCI devices would get set to 0 as result.

iort_dma_setup() would query the root complex'/named component IORT entry for a DMA mask, and use that over the one the device has been configured with earlier.

Ideally we want to use the minimum mask of what the IORT contains for the root complex and what the device was configured with.

Fixes 5ac65e8c8941 ("ACPI/IORT: Support address size limit for root complexes")

Signed-off-by Moritz Fischer <[email protected]>

---

Changes from v1:
- Changed warning to FW_BUG
- Warn for both Named Component or Root Complex
- Replaced min_not_zero() with min()

---
  drivers/acpi/arm64/iort.c | 14 ++++++++++++--
  1 file changed, 12 insertions(+), 2 deletions(-)

drivers/acpi/arm64/iort.c +10 -0

index d4eac6d7e9fb..2494138a6905 100644
@@ -1107,6 +1107,11 @@ static int nc_dma_get_range(struct device *dev, u64 *size)
  	ncomp = (struct acpi_iort_named_component *)node->node_data;
+	if (!ncomp->memory_address_limit) {
+		pr_warn(FW_BUG "Named component missing memory address limit\n");
+		return -EINVAL;
+	}
+
  	*size = ncomp->memory_address_limit >= 64 ? U64_MAX :
  			1ULL<<ncomp->memory_address_limit;

@@ -1126,6 +1131,11 @@ static int rc_dma_get_range(struct device *dev, u64 *size)
  	rc = (struct acpi_iort_root_complex *)node->node_data;
+	if (!rc->memory_address_limit) {
+		pr_warn(FW_BUG "Root complex missing memory address limit\n");
+		return -EINVAL;
+	}
+
  	*size = rc->memory_address_limit >= 64 ? U64_MAX :
  			1ULL<<rc->memory_address_limit;

drivers/acpi/arm64/iort.c void iort_dma_setup(struct device *dev, u64 *dma_addr, u64 *dma_size) · line 1187

Quoted 7 lines

drivers/acpi/arm64/iort.c +2 -2

@@ -1173,8 +1183,8 @@ void iort_dma_setup(struct device *dev, u64 *dma_addr, u64 *dma_size)
  		end = dmaaddr + size - 1;
  		mask = DMA_BIT_MASK(ilog2(end) + 1);
  		dev->bus_dma_limit = end;
-		dev->coherent_dma_mask = mask;
-		*dev->dma_mask = mask;
+		dev->coherent_dma_mask = min(dev->coherent_dma_mask, mask);
+		*dev->dma_mask = min(*dev->dma_mask, mask);

Oops, I got so distracted by the "not_zero" aspect in v1 that I ended up thinking purely about smaller-than-default masks, but of course this *does* matter the other way round. And it is what we've always done on the DT side, so at least it makes us consistent.

FWIW I've already started writing up a patch to kill off this bit entirely, but either way we still can't meaningfully interpret a supposed DMA limit of 0 bits in a table describing DMA-capable devices, so for this patch as a fix,

Reviewed-by Robin Murphy <[email protected]>

Thanks,
Robin.

Quoted 3 lines

drivers/acpi/arm64/iort.c +0 -0

  	}
  	*dma_addr = dmaaddr;
 

Hi Robin,

Quoted 65 lines · Robin Murphy

Quoted 46 lines · Moritz Fischer

Address issue observed on real world system with suboptimal IORT table where DMA masks of PCI devices would get set to 0 as result.

iort_dma_setup() would query the root complex'/named component IORT entry for a DMA mask, and use that over the one the device has been configured with earlier.

Ideally we want to use the minimum mask of what the IORT contains for the root complex and what the device was configured with.

Fixes 5ac65e8c8941 ("ACPI/IORT: Support address size limit for root complexes")

Signed-off-by Moritz Fischer <[email protected]>

---

Changes from v1:
- Changed warning to FW_BUG
- Warn for both Named Component or Root Complex
- Replaced min_not_zero() with min()

---
   drivers/acpi/arm64/iort.c | 14 ++++++++++++--
   1 file changed, 12 insertions(+), 2 deletions(-)

drivers/acpi/arm64/iort.c +10 -0

index d4eac6d7e9fb..2494138a6905 100644
@@ -1107,6 +1107,11 @@ static int nc_dma_get_range(struct device *dev, u64 *size)
   	ncomp = (struct acpi_iort_named_component *)node->node_data;
+	if (!ncomp->memory_address_limit) {
+		pr_warn(FW_BUG "Named component missing memory address limit\n");
+		return -EINVAL;
+	}
+
   	*size = ncomp->memory_address_limit >= 64 ? U64_MAX :
   			1ULL<<ncomp->memory_address_limit;

@@ -1126,6 +1131,11 @@ static int rc_dma_get_range(struct device *dev, u64 *size)
   	rc = (struct acpi_iort_root_complex *)node->node_data;
+	if (!rc->memory_address_limit) {
+		pr_warn(FW_BUG "Root complex missing memory address limit\n");
+		return -EINVAL;
+	}
+
   	*size = rc->memory_address_limit >= 64 ? U64_MAX :
   			1ULL<<rc->memory_address_limit;

drivers/acpi/arm64/iort.c void iort_dma_setup(struct device *dev, u64 *dma_addr, u64 *dma_size) · line 1187

Quoted 7 lines

drivers/acpi/arm64/iort.c +2 -2

@@ -1173,8 +1183,8 @@ void iort_dma_setup(struct device *dev, u64 *dma_addr, u64 *dma_size)
   		end = dmaaddr + size - 1;
   		mask = DMA_BIT_MASK(ilog2(end) + 1);
   		dev->bus_dma_limit = end;
-		dev->coherent_dma_mask = mask;
-		*dev->dma_mask = mask;
+		dev->coherent_dma_mask = min(dev->coherent_dma_mask, mask);
+		*dev->dma_mask = min(*dev->dma_mask, mask);

Oops, I got so distracted by the "not_zero" aspect in v1 that I ended up thinking purely about smaller-than-default masks, but of course this *does* matter the other way round. And it is what we've always done on the DT side, so at least it makes us consistent.

FWIW I've already started writing up a patch to kill off this bit entirely, but either way we still can't meaningfully interpret a supposed DMA limit of 0 bits in a table describing DMA-capable devices, so for this patch as a fix,

Quoted 1 line

Reviewed-by Robin Murphy <[email protected]>

I think there's another issue the comparisons for revision should be against < 2 not < 1.

From what I could find DEN0049D (IORT) spec introduced the fields
(curiously the C doc seems to be missing).

DEN0049B specifies revision as '0', DEN0049C (missing?), DEN0049D specifies new fields for memory_size_limit and both Named Component and Root Complex nodes set revision to 2.

so I think it should be:

if (!node || node->revision < 2)

	return -ENODEV;

Only if we go past this and there is no address limit is it really a firmware bug.

Quoted 1 line

Quoted 6 lines

Thanks,
Robin.

Quoted 3 lines

drivers/acpi/arm64/iort.c +0 -0

   	}
   	*dma_addr = dmaaddr;
 

- Moritz

Quoted 68 lines · Moritz Fischer

Hi Robin,

Quoted 65 lines · Robin Murphy

Quoted 46 lines · Moritz Fischer

Address issue observed on real world system with suboptimal IORT table where DMA masks of PCI devices would get set to 0 as result.

iort_dma_setup() would query the root complex'/named component IORT entry for a DMA mask, and use that over the one the device has been configured with earlier.

Ideally we want to use the minimum mask of what the IORT contains for the root complex and what the device was configured with.

Fixes 5ac65e8c8941 ("ACPI/IORT: Support address size limit for root complexes")

Signed-off-by Moritz Fischer <[email protected]>

---

Changes from v1:
- Changed warning to FW_BUG
- Warn for both Named Component or Root Complex
- Replaced min_not_zero() with min()

---
   drivers/acpi/arm64/iort.c | 14 ++++++++++++--
   1 file changed, 12 insertions(+), 2 deletions(-)

drivers/acpi/arm64/iort.c +10 -0

index d4eac6d7e9fb..2494138a6905 100644
@@ -1107,6 +1107,11 @@ static int nc_dma_get_range(struct device *dev, u64 *size)
   	ncomp = (struct acpi_iort_named_component *)node->node_data;
+	if (!ncomp->memory_address_limit) {
+		pr_warn(FW_BUG "Named component missing memory address limit\n");
+		return -EINVAL;
+	}
+
   	*size = ncomp->memory_address_limit >= 64 ? U64_MAX :
   			1ULL<<ncomp->memory_address_limit;

@@ -1126,6 +1131,11 @@ static int rc_dma_get_range(struct device *dev, u64 *size)
   	rc = (struct acpi_iort_root_complex *)node->node_data;
+	if (!rc->memory_address_limit) {
+		pr_warn(FW_BUG "Root complex missing memory address limit\n");
+		return -EINVAL;
+	}
+
   	*size = rc->memory_address_limit >= 64 ? U64_MAX :
   			1ULL<<rc->memory_address_limit;

drivers/acpi/arm64/iort.c void iort_dma_setup(struct device *dev, u64 *dma_addr, u64 *dma_size) · line 1187

Quoted 7 lines

drivers/acpi/arm64/iort.c +2 -2

@@ -1173,8 +1183,8 @@ void iort_dma_setup(struct device *dev, u64 *dma_addr, u64 *dma_size)
   		end = dmaaddr + size - 1;
   		mask = DMA_BIT_MASK(ilog2(end) + 1);
   		dev->bus_dma_limit = end;
-		dev->coherent_dma_mask = mask;
-		*dev->dma_mask = mask;
+		dev->coherent_dma_mask = min(dev->coherent_dma_mask, mask);
+		*dev->dma_mask = min(*dev->dma_mask, mask);

Oops, I got so distracted by the "not_zero" aspect in v1 that I ended up thinking purely about smaller-than-default masks, but of course this *does* matter the other way round. And it is what we've always done on the DT side, so at least it makes us consistent.

FWIW I've already started writing up a patch to kill off this bit entirely, but either way we still can't meaningfully interpret a supposed DMA limit of 0 bits in a table describing DMA-capable devices, so for this patch as a fix,

Quoted 7 lines

Quoted 1 line

Reviewed-by Robin Murphy <[email protected]>

I think there's another issue the comparisons for revision should be against < 2 not < 1.

From what I could find DEN0049D (IORT) spec introduced the fields
(curiously the C doc seems to be missing).

I guess it got lost in the documentation system move. FWIW I still have a copy of issue C, and root complex nodes are unchanged at revision 0 there.

Quoted 3 lines

DEN0049B specifies revision as '0', DEN0049C (missing?), DEN0049D specifies new fields for memory_size_limit and both Named Component and Root Complex nodes set revision to 2.

My copy of issue D says Root Complex nodes are at revision 1, with memory address size limit added.

(Note that Named Component nodes did bump to rev. 1 in issue C, then to rev. 2 in issue D)

Issue E bumped Root Complex nodes to revision 2 with the addition of the PRI flag, then E.a made a mess of everything by deprecating the revision numbers for individual tables - we probably need to deal with *that*, since otherwise we'll think new tables are back at rev. 0 again, but AFAICS the current check is correct for anything written against the first 5 releases.

Address issue observed on real world system with suboptimal IORT table where DMA masks of PCI devices would get set to 0 as result.

iort_dma_setup() would query the root complex'/named component IORT entry for a DMA mask, and use that over the one the device has been configured with earlier.

Ideally we want to use the minimum mask of what the IORT contains for the root complex and what the device was configured with.

Fixes 5ac65e8c8941 ("ACPI/IORT: Support address size limit for root complexes")

Signed-off-by Moritz Fischer <[email protected]>

---

Changes from v1:
- Changed warning to FW_BUG
- Warn for both Named Component or Root Complex
- Replaced min_not_zero() with min()

---
 drivers/acpi/arm64/iort.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

drivers/acpi/arm64/iort.c +12 -2

index d4eac6d7e9fb..2494138a6905 100644
@@ -1107,6 +1107,11 @@ static int nc_dma_get_range(struct device *dev, u64 *size)
 
 	ncomp = (struct acpi_iort_named_component *)node->node_data;
 
+	if (!ncomp->memory_address_limit) {
+		pr_warn(FW_BUG "Named component missing memory address limit\n");
+		return -EINVAL;
+	}
+
 	*size = ncomp->memory_address_limit >= 64 ? U64_MAX :
 			1ULL<<ncomp->memory_address_limit;
 

@@ -1126,6 +1131,11 @@ static int rc_dma_get_range(struct device *dev, u64 *size)
 
 	rc = (struct acpi_iort_root_complex *)node->node_data;
 
+	if (!rc->memory_address_limit) {
+		pr_warn(FW_BUG "Root complex missing memory address limit\n");
+		return -EINVAL;
+	}
+
 	*size = rc->memory_address_limit >= 64 ? U64_MAX :
 			1ULL<<rc->memory_address_limit;
 

@@ -1173,8 +1183,8 @@ void iort_dma_setup(struct device *dev, u64 *dma_addr, u64 *dma_size)
 		end = dmaaddr + size - 1;
 		mask = DMA_BIT_MASK(ilog2(end) + 1);
 		dev->bus_dma_limit = end;
-		dev->coherent_dma_mask = mask;
-		*dev->dma_mask = mask;
+		dev->coherent_dma_mask = min(dev->coherent_dma_mask, mask);
+		*dev->dma_mask = min(*dev->dma_mask, mask);
 	}
 
 	*dma_addr = dmaaddr;

Signature · 1 line

2.30.0

Address issue observed on real world system with suboptimal IORT table where DMA masks of PCI devices would get set to 0 as result.

iort_dma_setup() would query the root complex'/named component IORT entry for a DMA mask, and use that over the one the device has been configured with earlier.

[...]

Quoted 7 lines · Moritz Fischer

Address issue observed on real world system with suboptimal IORT table where DMA masks of PCI devices would get set to 0 as result.

iort_dma_setup() would query the root complex'/named component IORT entry for a DMA mask, and use that over the one the device has been configured with earlier.

Quoted 1 line

[...]

Applied to arm64 (for-next/fixes), thanks!

[1/1] ACPI/IORT: Do not blindly trust DMA masks from firmware

      https://git.kernel.org/arm64/c/a1df829ead58

Signature · 1 line

Catalin