REGRESSION: iommu fails to take address limit into account

REGRESSION: iommu fails to take address limit into account

[Ard Biesheuvel]

Hello all,

I am looking into an issue where a platform device is wired to a
MMU-500, and for some reason (which is under investigation) the
platform device can not drive all address bits. I can work around this
by limiting the DMA mask to 40 bits in the driver. However, the IORT
table allows me to set the address limit as well, and so I was
expecting this to be taken into account by the SMMU driver.

When the iort/iommu layer sets up the DMA operations,
iommu_dma_init_domain() is entered with the expected values:

base == 0
size == 0x100_0000_0000

However, the iommu layer ends up generating IOVA addresses that have
bits [47:40] set (which is what the MMU-500 supports). Looking closer,
this is not surprising, given that the end_pfn variable that is
calculated in iommu_dma_init_domain() is no longer used after Zhen's
patch aa3ac9469c185 ("iommu/iova: Make dma_32bit_pfn implicit") was
applied.

So effectively, this is a regression, and I would like your help
figuring out how to go about fixing this.

Thanks,
Ard.

REGRESSION: iommu fails to take address limit into account

[Robin Murphy]

Hi Ard,

On 25/05/18 10:48, Ard Biesheuvel wrote:
> Hello all,
> 
> I am looking into an issue where a platform device is wired to a
> MMU-500, and for some reason (which is under investigation) the
> platform device can not drive all address bits. I can work around this
> by limiting the DMA mask to 40 bits in the driver. However, the IORT
> table allows me to set the address limit as well, and so I was
> expecting this to be taken into account by the SMMU driver.
> 
> When the iort/iommu layer sets up the DMA operations,
> iommu_dma_init_domain() is entered with the expected values:
> 
> base == 0
> size == 0x100_0000_0000
> 
> However, the iommu layer ends up generating IOVA addresses that have
> bits [47:40] set (which is what the MMU-500 supports). Looking closer,
> this is not surprising, given that the end_pfn variable that is
> calculated in iommu_dma_init_domain() is no longer used after Zhen's
> patch aa3ac9469c185 ("iommu/iova: Make dma_32bit_pfn implicit") was
> applied.
> 
> So effectively, this is a regression, and I would like your help
> figuring out how to go about fixing this.

Note that the size passed to iommu_dma_init_domain() has *never* been 
any kind of enforced limit, so nothing is actually regressing here. If 
the device master interface is natively >40 bits wide (such that the 
driver would expect to set a larger DMA mask) and the restriction is 
purely in the interconnect between device and SMMU, then that's the case 
which has always been broken.

I do have a long-standing plan of attack for fixing the underlying issue 
(of actually enforcing any dma mask restriction described by IORT or DT 
"dma-ranges"), but it's one of those things that's tied to a whole bunch 
of other rework and fixes in order to be viable. I'm about to go off on 
holiday for a while but I should hopefully have the bandwidth to start 
looking at this seriously once I get back (frankly it's been getting 
bumped down my to-do list for far too long now...)

Robin.

REGRESSION: iommu fails to take address limit into account

[Ard Biesheuvel]

On 25 May 2018 at 11:48, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> Hello all,
>
> I am looking into an issue where a platform device is wired to a
> MMU-500, and for some reason (which is under investigation) the
> platform device can not drive all address bits. I can work around this
> by limiting the DMA mask to 40 bits in the driver. However, the IORT
> table allows me to set the address limit as well, and so I was
> expecting this to be taken into account by the SMMU driver.
>
> When the iort/iommu layer sets up the DMA operations,
> iommu_dma_init_domain() is entered with the expected values:
>
> base == 0
> size == 0x100_0000_0000
>
> However, the iommu layer ends up generating IOVA addresses that have
> bits [47:40] set (which is what the MMU-500 supports). Looking closer,
> this is not surprising, given that the end_pfn variable that is
> calculated in iommu_dma_init_domain() is no longer used after Zhen's
> patch aa3ac9469c185 ("iommu/iova: Make dma_32bit_pfn implicit") was
> applied.
>
> So effectively, this is a regression, and I would like your help
> figuring out how to go about fixing this.
>

I have narrowed it down a bit further: even though the IOVA range
ignores the IORT address limit, the device's DMA mask is set
correctly. The only problem is that the driver (like all drivers
afaict) does not take into account the fact that its DMA mask has
already been set by the bus layer before its probe function is called.

I could add something like this

-       if (dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64)))
+       if (dma_get_mask(&pdev->dev) == DMA_BIT_MASK(32) &&
+           dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64)))
                dev_warn(&pdev->dev, "Failed to enable 64-bit DMA\n");

to only override the mask if it is not at its default value, but it
feels like papering over the problem given that most drivers appear to
ignore the preset mask as well.

REGRESSION: iommu fails to take address limit into account

[Robin Murphy]

On 25/05/18 11:35, Ard Biesheuvel wrote:
> On 25 May 2018 at 11:48, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>> Hello all,
>>
>> I am looking into an issue where a platform device is wired to a
>> MMU-500, and for some reason (which is under investigation) the
>> platform device can not drive all address bits. I can work around this
>> by limiting the DMA mask to 40 bits in the driver. However, the IORT
>> table allows me to set the address limit as well, and so I was
>> expecting this to be taken into account by the SMMU driver.
>>
>> When the iort/iommu layer sets up the DMA operations,
>> iommu_dma_init_domain() is entered with the expected values:
>>
>> base == 0
>> size == 0x100_0000_0000
>>
>> However, the iommu layer ends up generating IOVA addresses that have
>> bits [47:40] set (which is what the MMU-500 supports). Looking closer,
>> this is not surprising, given that the end_pfn variable that is
>> calculated in iommu_dma_init_domain() is no longer used after Zhen's
>> patch aa3ac9469c185 ("iommu/iova: Make dma_32bit_pfn implicit") was
>> applied.
>>
>> So effectively, this is a regression, and I would like your help
>> figuring out how to go about fixing this.
>>
> 
> I have narrowed it down a bit further: even though the IOVA range
> ignores the IORT address limit, the device's DMA mask is set
> correctly. The only problem is that the driver (like all drivers
> afaict) does not take into account the fact that its DMA mask has
> already been set by the bus layer before its probe function is called.
> 
> I could add something like this
> 
> -       if (dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64)))
> +       if (dma_get_mask(&pdev->dev) == DMA_BIT_MASK(32) &&
> +           dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64)))
>                  dev_warn(&pdev->dev, "Failed to enable 64-bit DMA\n");
> 
> to only override the mask if it is not at its default value, but it
> feels like papering over the problem given that most drivers appear to
> ignore the preset mask as well.

Yup, that's the crux of it - if, say, the firmware describes an explicit 
restriction to 32 bits, then drivers have no way to tell the difference 
between that and the default mask initialised by the bus code (which 
they *should* be able to widen), and at the moment neither does arch 
code or anyone else (and the current interface between arch code and 
firmware code does not help matters).

Robin.

REGRESSION: iommu fails to take address limit into account

[Ard Biesheuvel]

On 25 May 2018 at 12:49, Robin Murphy <robin.murphy@arm.com> wrote:
> On 25/05/18 11:35, Ard Biesheuvel wrote:
>>
>> On 25 May 2018 at 11:48, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>>>
>>> Hello all,
>>>
>>> I am looking into an issue where a platform device is wired to a
>>> MMU-500, and for some reason (which is under investigation) the
>>> platform device can not drive all address bits. I can work around this
>>> by limiting the DMA mask to 40 bits in the driver. However, the IORT
>>> table allows me to set the address limit as well, and so I was
>>> expecting this to be taken into account by the SMMU driver.
>>>
>>> When the iort/iommu layer sets up the DMA operations,
>>> iommu_dma_init_domain() is entered with the expected values:
>>>
>>> base == 0
>>> size == 0x100_0000_0000
>>>
>>> However, the iommu layer ends up generating IOVA addresses that have
>>> bits [47:40] set (which is what the MMU-500 supports). Looking closer,
>>> this is not surprising, given that the end_pfn variable that is
>>> calculated in iommu_dma_init_domain() is no longer used after Zhen's
>>> patch aa3ac9469c185 ("iommu/iova: Make dma_32bit_pfn implicit") was
>>> applied.
>>>
>>> So effectively, this is a regression, and I would like your help
>>> figuring out how to go about fixing this.
>>>
>>
>> I have narrowed it down a bit further: even though the IOVA range
>> ignores the IORT address limit, the device's DMA mask is set
>> correctly. The only problem is that the driver (like all drivers
>> afaict) does not take into account the fact that its DMA mask has
>> already been set by the bus layer before its probe function is called.
>>
>> I could add something like this
>>
>> -       if (dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64)))
>> +       if (dma_get_mask(&pdev->dev) == DMA_BIT_MASK(32) &&
>> +           dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64)))
>>                  dev_warn(&pdev->dev, "Failed to enable 64-bit DMA\n");
>>
>> to only override the mask if it is not at its default value, but it
>> feels like papering over the problem given that most drivers appear to
>> ignore the preset mask as well.
>
>
> Yup, that's the crux of it - if, say, the firmware describes an explicit
> restriction to 32 bits, then drivers have no way to tell the difference
> between that and the default mask initialised by the bus code (which they
> *should* be able to widen), and at the moment neither does arch code or
> anyone else (and the current interface between arch code and firmware code
> does not help matters).
>

OK, so I guess we need an additional bus limit field that is
initialized to ~0 and can only be narrowed, and take that into account
when dma_set_mask_and_coherent() is called. Not pretty, but at least
we won't have to fix all the drivers.