| Message ID | 20210122012419.95010-1-mdf@kernel.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | [v2] ACPI/IORT: Do not blindly trust DMA masks from firmware | expand |
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@kernel.org> > --- > > 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@arm.com> Thanks, Robin. > } > > *dma_addr = dmaaddr; >
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@kernel.org> > > --- > > > > 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@arm.com> 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. > > Thanks, > Robin. > > > } > > *dma_addr = dmaaddr; > > - Moritz
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@kernel.org> >>> --- >>> >>> 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@arm.com> > > 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 >
On Fri, Jan 22, 2021 at 07:17:59PM +0000, Robin Murphy wrote: > 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@kernel.org> > > > > --- > > > > > > > > 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@arm.com> > > > > 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. Ok, yeah, I double checked this, you're right. Then patch should be fine as is. Thanks!
On Thu, Jan 21, 2021 at 05:24:19PM -0800, 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@kernel.org> > --- > > 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(-) Acked-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > 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); > } > > *dma_addr = dmaaddr; > -- > 2.30.0 >
On Thu, 21 Jan 2021 17:24:19 -0800, 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. > > [...] 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
Hi Catalin, On Wed, Jan 27, 2021 at 01:09:36PM +0000, Catalin Marinas wrote: > On Thu, 21 Jan 2021 17:24:19 -0800, 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. > > > > [...] > > 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 > > -- > Catalin > Should we consider this for stable, too? If so I can send an email to stable@ once it's in Linus' tree, since I'm the one forgetting to CC stable in the first place :) It would be nice if at least 5.10 would get this. Cheers, Moritz
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); } *dma_addr = dmaaddr;
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@kernel.org> --- 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(-)