| Message ID | 20200420081131.16822-1-ardb@kernel.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | acpi: arm64/iort: Ensure DMA mask does not exceed device limit | expand |
[+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 >
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 >>
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.
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;
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(-)