* [PATCH] acpi: arm64/iort: Ensure DMA mask does not exceed device limit @ 2020-04-20 8:11 Ard Biesheuvel 2020-04-20 9:30 ` Lorenzo Pieralisi 0 siblings, 1 reply; 4+ messages in thread From: Ard Biesheuvel @ 2020-04-20 8:11 UTC (permalink / raw) To: linux-arm-kernel Cc: Ard Biesheuvel, linux-acpi, guohanjun, lorenzo.pieralisi, sudeep.holla When calculating the DMA mask from the address limit provided by the firmware, we add one to the ilog2() of the end address, and pass the result to DMA_BIT_MASK(). For an end address that is not a power-of-2 minus 1, this will result in the mask to be wider than the limit, and cover memory that is not addressable by the device. Instead, we should add 1 to 'end' before taking the log, so that a limit of, say, 0x3fffffff gets translated to a mask of 30, but any value below it gets translated to 29. Signed-off-by: Ard Biesheuvel <ardb@kernel.org> --- drivers/acpi/arm64/iort.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c index 7d04424189df..aab2f51eff14 100644 --- a/drivers/acpi/arm64/iort.c +++ b/drivers/acpi/arm64/iort.c @@ -1162,7 +1162,7 @@ void iort_dma_setup(struct device *dev, u64 *dma_addr, u64 *dma_size) * firmware. */ end = dmaaddr + size - 1; - mask = DMA_BIT_MASK(ilog2(end) + 1); + mask = DMA_BIT_MASK(ilog2(end + 1)); dev->bus_dma_limit = end; dev->coherent_dma_mask = mask; *dev->dma_mask = mask; -- 2.17.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] acpi: arm64/iort: Ensure DMA mask does not exceed device limit 2020-04-20 8:11 [PATCH] acpi: arm64/iort: Ensure DMA mask does not exceed device limit Ard Biesheuvel @ 2020-04-20 9:30 ` Lorenzo Pieralisi 2020-04-20 11:23 ` Robin Murphy 0 siblings, 1 reply; 4+ messages in thread From: Lorenzo Pieralisi @ 2020-04-20 9:30 UTC (permalink / raw) To: Ard Biesheuvel Cc: linux-acpi, guohanjun, robin.murphy, linux-arm-kernel, sudeep.holla [+Robin] On Mon, Apr 20, 2020 at 10:11:31AM +0200, Ard Biesheuvel wrote: > When calculating the DMA mask from the address limit provided by the > firmware, we add one to the ilog2() of the end address, and pass the > result to DMA_BIT_MASK(). > > For an end address that is not a power-of-2 minus 1, this will result > in the mask to be wider than the limit, and cover memory that is not > addressable by the device. Instead, we should add 1 to 'end' before > taking the log, so that a limit of, say, 0x3fffffff gets translated > to a mask of 30, but any value below it gets translated to 29. > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > --- > drivers/acpi/arm64/iort.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Need Robin's feedback on this - I was looking at: a7ba70f1787f ("dma-mapping: treat dev->bus_dma_mask as a DMA limit") I assume current code is *intended* but I shall let Robin comment on this. Thanks, Lorenzo > diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c > index 7d04424189df..aab2f51eff14 100644 > --- a/drivers/acpi/arm64/iort.c > +++ b/drivers/acpi/arm64/iort.c > @@ -1162,7 +1162,7 @@ void iort_dma_setup(struct device *dev, u64 *dma_addr, u64 *dma_size) > * firmware. > */ > end = dmaaddr + size - 1; > - mask = DMA_BIT_MASK(ilog2(end) + 1); > + mask = DMA_BIT_MASK(ilog2(end + 1)); > dev->bus_dma_limit = end; > dev->coherent_dma_mask = mask; > *dev->dma_mask = mask; > -- > 2.17.1 > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] acpi: arm64/iort: Ensure DMA mask does not exceed device limit 2020-04-20 9:30 ` Lorenzo Pieralisi @ 2020-04-20 11:23 ` Robin Murphy 2020-04-20 11:42 ` Ard Biesheuvel 0 siblings, 1 reply; 4+ messages in thread From: Robin Murphy @ 2020-04-20 11:23 UTC (permalink / raw) To: Lorenzo Pieralisi, Ard Biesheuvel Cc: linux-acpi, guohanjun, linux-arm-kernel, sudeep.holla On 2020-04-20 10:30 am, Lorenzo Pieralisi wrote: > [+Robin] > > On Mon, Apr 20, 2020 at 10:11:31AM +0200, Ard Biesheuvel wrote: >> When calculating the DMA mask from the address limit provided by the >> firmware, we add one to the ilog2() of the end address, and pass the >> result to DMA_BIT_MASK(). >> >> For an end address that is not a power-of-2 minus 1, this will result >> in the mask to be wider than the limit, and cover memory that is not >> addressable by the device. Instead, we should add 1 to 'end' before >> taking the log, so that a limit of, say, 0x3fffffff gets translated >> to a mask of 30, but any value below it gets translated to 29. >> >> Signed-off-by: Ard Biesheuvel <ardb@kernel.org> >> --- >> drivers/acpi/arm64/iort.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) > > Need Robin's feedback on this - I was looking at: > > a7ba70f1787f ("dma-mapping: treat dev->bus_dma_mask as a DMA limit") > > I assume current code is *intended* but I shall let Robin comment > on this. The device masks represent what bits the device is capable of driving, so rounding up is the correct and intended behaviour - if the interconnect address map imposes a non-power-of-two limit, say 3.75GB, and the device can physically access all of that, then claiming the device can't drive bit 31 and trying to prevent it from accessing the upper 1.75GB is nonsense. Although TBH none of this really matters much any more - as long as the limit is set correctly nothing bad will happen, and drivers are expected to replace these default masks anyway. In fact ancient drivers that still don't explicitly set their masks will be assuming the defaults are 32-bit, so replacing them with something potentially wider actually invites a whole other set of problems. In the case of of_dma_configure(), it kept the code that combines (*not* replaces) the default device masks with a limit-based mask because it didn't do any harm, but equally it should now be entirely unnecessary, and confusion like this seems like an argument for finally removing it. Robin. > > Thanks, > Lorenzo > >> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c >> index 7d04424189df..aab2f51eff14 100644 >> --- a/drivers/acpi/arm64/iort.c >> +++ b/drivers/acpi/arm64/iort.c >> @@ -1162,7 +1162,7 @@ void iort_dma_setup(struct device *dev, u64 *dma_addr, u64 *dma_size) >> * firmware. >> */ >> end = dmaaddr + size - 1; >> - mask = DMA_BIT_MASK(ilog2(end) + 1); >> + mask = DMA_BIT_MASK(ilog2(end + 1)); >> dev->bus_dma_limit = end; >> dev->coherent_dma_mask = mask; >> *dev->dma_mask = mask; >> -- >> 2.17.1 >> _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] acpi: arm64/iort: Ensure DMA mask does not exceed device limit 2020-04-20 11:23 ` Robin Murphy @ 2020-04-20 11:42 ` Ard Biesheuvel 0 siblings, 0 replies; 4+ messages in thread From: Ard Biesheuvel @ 2020-04-20 11:42 UTC (permalink / raw) To: Robin Murphy Cc: linux-acpi, Lorenzo Pieralisi, guohanjun, Linux ARM, sudeep.holla On Mon, 20 Apr 2020 at 13:23, Robin Murphy <robin.murphy@arm.com> wrote: > > On 2020-04-20 10:30 am, Lorenzo Pieralisi wrote: > > [+Robin] > > > > On Mon, Apr 20, 2020 at 10:11:31AM +0200, Ard Biesheuvel wrote: > >> When calculating the DMA mask from the address limit provided by the > >> firmware, we add one to the ilog2() of the end address, and pass the > >> result to DMA_BIT_MASK(). > >> > >> For an end address that is not a power-of-2 minus 1, this will result > >> in the mask to be wider than the limit, and cover memory that is not > >> addressable by the device. Instead, we should add 1 to 'end' before > >> taking the log, so that a limit of, say, 0x3fffffff gets translated > >> to a mask of 30, but any value below it gets translated to 29. > >> > >> Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > >> --- > >> drivers/acpi/arm64/iort.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > > > > Need Robin's feedback on this - I was looking at: > > > > a7ba70f1787f ("dma-mapping: treat dev->bus_dma_mask as a DMA limit") > > > > I assume current code is *intended* but I shall let Robin comment > > on this. > > The device masks represent what bits the device is capable of driving, > so rounding up is the correct and intended behaviour - if the > interconnect address map imposes a non-power-of-two limit, say 3.75GB, > and the device can physically access all of that, then claiming the > device can't drive bit 31 and trying to prevent it from accessing the > upper 1.75GB is nonsense. > Fair enough. > Although TBH none of this really matters much any more - as long as the > limit is set correctly nothing bad will happen, and drivers are expected > to replace these default masks anyway. In fact ancient drivers that > still don't explicitly set their masks will be assuming the defaults are > 32-bit, so replacing them with something potentially wider actually > invites a whole other set of problems. In the case of > of_dma_configure(), it kept the code that combines (*not* replaces) the > default device masks with a limit-based mask because it didn't do any > harm, but equally it should now be entirely unnecessary, and confusion > like this seems like an argument for finally removing it. > Indeed :-) Thanks for the explanation. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-04-20 11:42 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-04-20 8:11 [PATCH] acpi: arm64/iort: Ensure DMA mask does not exceed device limit Ard Biesheuvel 2020-04-20 9:30 ` Lorenzo Pieralisi 2020-04-20 11:23 ` Robin Murphy 2020-04-20 11:42 ` Ard Biesheuvel
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).