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@xxxxxxxxxx>
---
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
