Re: [PATCH v2 1/5] dt-bindings: reserved-memory: Document memory region specifier

[Dmitry Osipenko <digetx@gmail.com>]

01.07.2021 21:14, Thierry Reding пишет:
> On Tue, Jun 08, 2021 at 06:51:40PM +0200, Thierry Reding wrote:
>> On Fri, May 28, 2021 at 06:54:55PM +0200, Thierry Reding wrote:
>>> On Thu, May 20, 2021 at 05:03:06PM -0500, Rob Herring wrote:
>>>> On Fri, Apr 23, 2021 at 06:32:30PM +0200, Thierry Reding wrote:
>>>>> From: Thierry Reding <treding@nvidia.com>
>>>>>
>>>>> Reserved memory region phandle references can be accompanied by a
>>>>> specifier that provides additional information about how that specific
>>>>> reference should be treated.
>>>>>
>>>>> One use-case is to mark a memory region as needing an identity mapping
>>>>> in the system's IOMMU for the device that references the region. This is
>>>>> needed for example when the bootloader has set up hardware (such as a
>>>>> display controller) to actively access a memory region (e.g. a boot
>>>>> splash screen framebuffer) during boot. The operating system can use the
>>>>> identity mapping flag from the specifier to make sure an IOMMU identity
>>>>> mapping is set up for the framebuffer before IOMMU translations are
>>>>> enabled for the display controller.
>>>>>
>>>>> Signed-off-by: Thierry Reding <treding@nvidia.com>
>>>>> ---
>>>>>  .../reserved-memory/reserved-memory.txt       | 21 +++++++++++++++++++
>>>>>  include/dt-bindings/reserved-memory.h         |  8 +++++++
>>>>>  2 files changed, 29 insertions(+)
>>>>>  create mode 100644 include/dt-bindings/reserved-memory.h
>>>>
>>>> Sorry for being slow on this. I have 2 concerns.
>>>>
>>>> First, this creates an ABI issue. A DT with cells in 'memory-region' 
>>>> will not be understood by an existing OS. I'm less concerned about this 
>>>> if we address that with a stable fix. (Though I'm pretty sure we've 
>>>> naively added #?-cells in the past ignoring this issue.)
>>>
>>> A while ago I had proposed adding memory-region*s* as an alternative
>>> name for memory-region to make the naming more consistent with other
>>> types of properties (think clocks, resets, gpios, ...). If we added
>>> that, we could easily differentiate between the "legacy" cases where
>>> no #memory-region-cells was allowed and the new cases where it was.
>>>
>>>> Second, it could be the bootloader setting up the reserved region. If a 
>>>> node already has 'memory-region', then adding more regions is more 
>>>> complicated compared to adding new properties. And defining what each 
>>>> memory-region entry is or how many in schemas is impossible.
>>>
>>> It's true that updating the property gets a bit complicated, but it's
>>> not exactly rocket science. We really just need to splice the array. I
>>> have a working implemention for this in U-Boot.
>>>
>>> For what it's worth, we could run into the same issue with any new
>>> property that we add. Even if we renamed this to iommu-memory-region,
>>> it's still possible that a bootloader may have to update this property
>>> if it already exists (it could be hard-coded in DT, or it could have
>>> been added by some earlier bootloader or firmware).
>>>
>>>> Both could be addressed with a new property. Perhaps something like 
>>>> 'iommu-memory-region = <&phandle>;'. I think the 'iommu' prefix is 
>>>> appropriate given this is entirely because of the IOMMU being in the 
>>>> mix. I might feel differently if we had other uses for cells, but I 
>>>> don't really see it in this case. 
>>>
>>> I'm afraid that down the road we'll end up with other cases and then we
>>> might proliferate a number of *-memory-region properties with varying
>>> prefixes.
>>>
>>> I am aware of one other case where we might need something like this: on
>>> some Tegra SoCs we have audio processors that will access memory buffers
>>> using a DMA engine. These processors are booted from early firmware
>>> using firmware from system memory. In order to avoid trashing the
>>> firmware, we need to reserve memory. We can do this using reserved
>>> memory nodes. However, the audio DMA engine also uses the SMMU, so we
>>> need to make sure that the firmware memory is marked as reserved within
>>> the SMMU. This is similar to the identity mapping case, but not exactly
>>> the same. Instead of creating a 1:1 mapping, we just want that IOVA
>>> region to be reserved (i.e. IOMMU_RESV_RESERVED instead of
>>> IOMMU_RESV_DIRECT{,_RELAXABLE}).
>>>
>>> That would also fall into the IOMMU domain, but we can't reuse the
>>> iommu-memory-region property for that because then we don't have enough
>>> information to decide which type of reservation we need.
>>>
>>> We could obviously make iommu-memory-region take a specifier, but we
>>> could just as well use memory-regions in that case since we have
>>> something more generic anyway.
>>>
>>> With the #memory-region-cells proposal, we can easily extend the cell in
>>> the specifier with an additional MEMORY_REGION_IOMMU_RESERVE flag to
>>> take that other use case into account. If we than also change to the new
>>> memory-regions property name, we avoid the ABI issue (and we gain a bit
>>> of consistency while at it).
>>
>> Ping? Rob, do you want me to add this second use-case to the patch
>> series to make it more obvious that this isn't just a one-off thing? Or
>> how do we proceed?
> 
> Rob, given that additional use-case, do you want me to run with this
> proposal and send out an updated series?


What about variant with a "descriptor" properties that will describe
each region:

fb_desc: display-framebuffer-memory-descriptor {
	needs-identity-mapping;
}

display@52400000 {
	memory-region = <&fb ...>;
	memory-region-descriptor = <&fb_desc ...>;
};

It could be a more flexible/extendible variant.