On Mon, 20 Apr 2020 at 13:23, Robin Murphy <robin.murphy@xxxxxxx> 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@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.
>
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.
