On 2021-01-22 17:50, Moritz Fischer wrote:
Hi Robin,
On Fri, Jan 22, 2021 at 02:42:05PM +0000, Robin Murphy wrote:
On 2021-01-22 01:24, Moritz Fischer wrote:
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 <mdf@xxxxxxxxxx>
---
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(-)
diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
index d4eac6d7e9fb..2494138a6905 100644
--- a/drivers/acpi/arm64/iort.c
+++ b/drivers/acpi/arm64/iort.c
@@ -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);
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 <robin.murphy@xxxxxxx>
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.
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.
Robin.
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.
Thanks,
Robin.
}
*dma_addr = dmaaddr;
- Moritz
