Re: [PATCH v7 0/9] ACPI/IORT: Support for IORT RMR node

Robin Murphy
2022-01-25T19:31:00Z

On 2022-01-25 13:00, Shameerali Kolothum Thodi wrote:
> Hi Robin/Lorenzo,
>
>> -----Original Message-----
>> From: iommu [mailto:iommu-bounces@lists.linux-foundation.org] On Behalf
>> Of Shameer Kolothum
>> Sent: 05 August 2021 09:07
>> To: linux-arm-kernel@lists.infradead.org; linux-acpi@vger.kernel.org;
>> iommu@lists.linux-foundation.org
>> Cc: robin.murphy@arm.com; jon@solid-run.com; Linuxarm
>> <linuxarm@huawei.com>; steven.price@arm.com; Guohanjun (Hanjun Guo)
>> <guohanjun@huawei.com>; yangyicong <yangyicong@huawei.com>;
>> Sami.Mujawar@arm.com; will@kernel.org; wanghuiqiang
>> <wanghuiqiang@huawei.com>
>> Subject: [PATCH v7 0/9] ACPI/IORT: Support for IORT RMR node
>>
>> Hi,
>>
>> The series adds support to IORT RMR nodes specified in IORT
>> Revision E.b -ARM DEN 0049E[0]. RMR nodes are used to describe
>> memory ranges that are used by endpoints and require a unity
>> mapping in SMMU.
>>
>> We have faced issues with 3408iMR RAID controller cards which
>> fail to boot when SMMU is enabled. This is because these
>> controllers make use of host memory for various caching related
>> purposes and when SMMU is enabled the iMR firmware fails to
>> access these memory regions as there is no mapping for them.
>> IORT RMR provides a way for UEFI to describe and report these
>> memory regions so that the kernel can make a unity mapping for
>> these in SMMU.
>>
>> Change History:
>>
>> v6 --> v7
>>
>> The only change from v6 is the fix pointed out by Steve to
>> the SMMUv2 SMR bypass install in patch #8.
>>
>> Thanks to the Tested-by tags by Laurentiu with SMMUv2 and
>> Hanjun/Huiqiang with SMMUv3 for v6. I haven't added the tags
>> yet as the series still needs more review[1].
>>
>> Feedback and tests on this series is very much appreciated.
>
> Since we have an update to IORT spec(E.c) now[1] and includes additional
> attributes/flags for the RMR node, I am planning to respin this series soon.
>
> Going through the new spec, I have a few queries,
>
> The memory range attributes can now be described as one of the following,
>
> 0x00: Device-nGnRnE memory
> 0x01: Device-nGnRE memory
> 0x02: Device-nGRE memory
> 0x03: Device-GRE memory
> 0x04: Normal Inner Non-cacheable Outer Non-cacheable
> 0x05: Normal Inner Write-back Outer Write-back Inner Shareable
>
> I am not sure how this needs to be captured and used in the kernel. Is there
> any intention of using these fine-grained attributes in the kernel now
> or a generic mapping of the above to the struct iommu_rev_region prot field
> is enough? i.e., something like,
>
> {
> ....
> prot = IOMMU_READ | IOMMU_WRITE;
>
> if (rmr_attr == normal_mem) // 0x05
> prot |= IOMMU_CACHE;
>
> if (rmr_attr == device_mem) { //0x00 - 0x03
> prot |= IOMMU_MMIO;
> prot |= IOMMU_NOEXEC;
> }
> ....
> }

Yup, pretty much that, except don't bother with IOMMU_NOEXEC. We can't
reliably infer it - e.g. on an AXI-based interconnect AxCACHE and AxPROT
are entirely orthogonal, so a Device-type read with the "Instruction
access" hint is perfectly legal - and in the common IORT code we're not
in a position to second-guess what any given RMR might represent for
whatever agent is accessing it.

All we can reasonably do here is map the Device types to IOMMU_MMIO and
Write-back to IOMMU_CACHE, and if anyone ever does want to insist that
that's not sufficient, then they're welcome to send patches to make the
IOMMU API more expressive :)

> Similarly for the 'flags' field, the new 'Access Privilege' is intended to set the
> IOMMU_PRIV ?

Yes, exactly!

Cheers,
Robin.

>
> Please let me know.
>
> Thanks,
> Shameer
>
> [1] https://developer.arm.com/documentation/den0049/ec/?lang=en
>