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(-)
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 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 <[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(-)
>
> 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 <[email protected]>
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 <[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(-)
> >
> > 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 <[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.
>
> 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 <[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(-)
> > > >
> > > > 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 <[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.
>
> > 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 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 <[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(-)
>>>
>>> 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 <[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.
> 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 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 <[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(-)
Acked-by: Lorenzo Pieralisi <[email protected]>
> 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
--
Catalin
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