Re: [PATCH v2 1/4] dt-bindings: reserved-memory: Document "active" property

Re: [PATCH v2 1/4] dt-bindings: reserved-memory: Document "active" property

[Dmitry Osipenko]

04.09.2020 15:59, Thierry Reding пишет:
> From: Thierry Reding <treding@nvidia.com>
> 
> Reserved memory regions can be marked as "active" if hardware is
> expected to access the regions during boot and before the operating
> system can take control. One example where this is useful is for the
> operating system to infer whether the region needs to be identity-
> mapped through an IOMMU.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>  .../bindings/reserved-memory/reserved-memory.txt           | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt b/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
> index 4dd20de6977f..163d2927e4fc 100644
> --- a/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
> +++ b/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
> @@ -63,6 +63,13 @@ reusable (optional) - empty property
>        able to reclaim it back. Typically that means that the operating
>        system can use that region to store volatile or cached data that
>        can be otherwise regenerated or migrated elsewhere.
> +active (optional) - empty property
> +    - If this property is set for a reserved memory region, it indicates
> +      that some piece of hardware may be actively accessing this region.
> +      Should the operating system want to enable IOMMU protection for a
> +      device, all active memory regions must have been identity-mapped
> +      in order to ensure that non-quiescent hardware during boot can
> +      continue to access the memory.
>  
>  Linux implementation note:
>  - If a "linux,cma-default" property is present, then Linux will use the
> 

Hi,

Could you please explain what devices need this quirk? I see that you're
targeting Tegra SMMU driver, which means that it should be some pre-T186
device. Is this reservation needed for some device that has display
hardwired to a very specific IOMMU domain at the boot time?

If you're targeting devices that don't have IOMMU enabled by default at
the boot time, then this approach won't work for the existing devices
which won't ever get an updated bootloader. I think Robin Murphy already
suggested that we should simply create a dummy "identity" IOMMU domain
by default for the DRM/VDE devices and then replace it with an
explicitly created domain within the drivers.

Secondly, all NVIDIA bootloaders are passing tegra_fbmem=... via
kernel's cmdline with the physical location of the framebuffer in
memory. Maybe we could support this option?

Re: [PATCH v2 1/4] dt-bindings: reserved-memory: Document "active" property

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 5626 bytes --]

On Thu, Sep 24, 2020 at 04:23:59PM +0300, Dmitry Osipenko wrote:
> 04.09.2020 15:59, Thierry Reding пишет:
> > From: Thierry Reding <treding@nvidia.com>
> > 
> > Reserved memory regions can be marked as "active" if hardware is
> > expected to access the regions during boot and before the operating
> > system can take control. One example where this is useful is for the
> > operating system to infer whether the region needs to be identity-
> > mapped through an IOMMU.
> > 
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > ---
> >  .../bindings/reserved-memory/reserved-memory.txt           | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt b/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
> > index 4dd20de6977f..163d2927e4fc 100644
> > --- a/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
> > +++ b/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
> > @@ -63,6 +63,13 @@ reusable (optional) - empty property
> >        able to reclaim it back. Typically that means that the operating
> >        system can use that region to store volatile or cached data that
> >        can be otherwise regenerated or migrated elsewhere.
> > +active (optional) - empty property
> > +    - If this property is set for a reserved memory region, it indicates
> > +      that some piece of hardware may be actively accessing this region.
> > +      Should the operating system want to enable IOMMU protection for a
> > +      device, all active memory regions must have been identity-mapped
> > +      in order to ensure that non-quiescent hardware during boot can
> > +      continue to access the memory.
> >  
> >  Linux implementation note:
> >  - If a "linux,cma-default" property is present, then Linux will use the
> > 
> 
> Hi,
> 
> Could you please explain what devices need this quirk? I see that you're
> targeting Tegra SMMU driver, which means that it should be some pre-T186
> device.

Primarily I'm looking at Tegra210 and later, because on earlier devices
the bootloader doesn't consistently initialize display. I know that it
does on some devices, but not all of them. This same code should also
work on Tegra186 and later (with an ARM SMMU) although the situation is
slightly more complicated there because IOMMU translations will fault by
default long before these identity mappings can be established.

> Is this reservation needed for some device that has display
> hardwired to a very specific IOMMU domain at the boot time?

No, this is only used to convey information about the active framebuffer
to the kernel. In practice the DMA/IOMMU code will use this information
to establish a 1:1 mapping on whatever IOMMU domain that was picked for
display.

> If you're targeting devices that don't have IOMMU enabled by default at
> the boot time, then this approach won't work for the existing devices
> which won't ever get an updated bootloader.

If the devices don't use an IOMMU, then there should be no problem. The
extra reserved-memory nodes would still be necessary to ensure that the
kernel doesn't reuse the framebuffer memory for the slab allocator, but
if no IOMMU is used, then the display controller accessing the memory
isn't going to cause problems other than perhaps scanning out data that
is no longer a framebuffer.

There should also be no problem for devices with an old bootloader
because this code is triggered by the presence of a reserved-memory node
referenced via the memory-region property. Devices with an old
bootloader should continue to work as they did before. Although I
suppose they would start faulting once we enable DMA/IOMMU integration
for Tegra SMMU if they have a bootloader that does initialize display to
actively scan out during boot.

> I think Robin Murphy already suggested that we should simply create
> a dummy "identity" IOMMU domain by default for the DRM/VDE devices and
> then replace it with an explicitly created domain within the drivers.

I don't recall reading about that suggestion. So does this mean that for
certain devices we'd want to basically passthrough by default and then
at some point during boot take over with a properly managed IOMMU
domain?

The primary goal here is to move towards using the DMA API rather than
the IOMMU API directly, so we don't really have the option of replacing
with an explicitly created domain. Unless we have code in the DMA/IOMMU
code that does this somehow.

But I'm not sure what would be a good way to mark certain devices as
needing an identity domain by default. Do we still use the reserved-
memory node for that? That would still require some sort of flag to
specify which reserved-memory regions would need this identity mapping
because, as was pointed out in earlier review, some devices may have
reserved-memory regions that are not meant to be identity mapped.

> Secondly, all NVIDIA bootloaders are passing tegra_fbmem=... via
> kernel's cmdline with the physical location of the framebuffer in
> memory. Maybe we could support this option?

I'm not a big fan of that command-line option, but I also realize that
for older bootloaders that's probably the only option we have. I don't
suppose all of the devices support U-Boot? Because ideally we'd just
translate from tegra_fbmem=... to reserved-memory region there so that
we don't have to carry backwards-compatibility code for these purely
downstream bootloaders.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 1/4] dt-bindings: reserved-memory: Document "active" property

[Dmitry Osipenko]

24.09.2020 17:01, Thierry Reding пишет:
> On Thu, Sep 24, 2020 at 04:23:59PM +0300, Dmitry Osipenko wrote:
>> 04.09.2020 15:59, Thierry Reding пишет:
>>> From: Thierry Reding <treding@nvidia.com>
>>>
>>> Reserved memory regions can be marked as "active" if hardware is
>>> expected to access the regions during boot and before the operating
>>> system can take control. One example where this is useful is for the
>>> operating system to infer whether the region needs to be identity-
>>> mapped through an IOMMU.
>>>
>>> Signed-off-by: Thierry Reding <treding@nvidia.com>
>>> ---
>>>  .../bindings/reserved-memory/reserved-memory.txt           | 7 +++++++
>>>  1 file changed, 7 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt b/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
>>> index 4dd20de6977f..163d2927e4fc 100644
>>> --- a/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
>>> +++ b/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
>>> @@ -63,6 +63,13 @@ reusable (optional) - empty property
>>>        able to reclaim it back. Typically that means that the operating
>>>        system can use that region to store volatile or cached data that
>>>        can be otherwise regenerated or migrated elsewhere.
>>> +active (optional) - empty property
>>> +    - If this property is set for a reserved memory region, it indicates
>>> +      that some piece of hardware may be actively accessing this region.
>>> +      Should the operating system want to enable IOMMU protection for a
>>> +      device, all active memory regions must have been identity-mapped
>>> +      in order to ensure that non-quiescent hardware during boot can
>>> +      continue to access the memory.
>>>  
>>>  Linux implementation note:
>>>  - If a "linux,cma-default" property is present, then Linux will use the
>>>
>>
>> Hi,
>>
>> Could you please explain what devices need this quirk? I see that you're
>> targeting Tegra SMMU driver, which means that it should be some pre-T186
>> device.
> 
> Primarily I'm looking at Tegra210 and later, because on earlier devices
> the bootloader doesn't consistently initialize display. I know that it
> does on some devices, but not all of them.

AFAIK, all tablet devices starting with Tegra20 that have display panel
are initializing display at a boot time for showing splash screen. This
includes all T20/T30/T114 tablets that are already supported by upstream
kernel.

> This same code should also
> work on Tegra186 and later (with an ARM SMMU) although the situation is
> slightly more complicated there because IOMMU translations will fault by
> default long before these identity mappings can be established.
> 
>> Is this reservation needed for some device that has display
>> hardwired to a very specific IOMMU domain at the boot time?
> 
> No, this is only used to convey information about the active framebuffer
> to the kernel. In practice the DMA/IOMMU code will use this information
> to establish a 1:1 mapping on whatever IOMMU domain that was picked for
> display.
> 
>> If you're targeting devices that don't have IOMMU enabled by default at
>> the boot time, then this approach won't work for the existing devices
>> which won't ever get an updated bootloader.
> 
> If the devices don't use an IOMMU, then there should be no problem. The
> extra reserved-memory nodes would still be necessary to ensure that the
> kernel doesn't reuse the framebuffer memory for the slab allocator, but
> if no IOMMU is used, then the display controller accessing the memory
> isn't going to cause problems other than perhaps scanning out data that
> is no longer a framebuffer.
> 
> There should also be no problem for devices with an old bootloader
> because this code is triggered by the presence of a reserved-memory node
> referenced via the memory-region property. Devices with an old
> bootloader should continue to work as they did before. Although I
> suppose they would start faulting once we enable DMA/IOMMU integration
> for Tegra SMMU if they have a bootloader that does initialize display to
> actively scan out during boot.
> 
>> I think Robin Murphy already suggested that we should simply create
>> a dummy "identity" IOMMU domain by default for the DRM/VDE devices and
>> then replace it with an explicitly created domain within the drivers.
> 
> I don't recall reading about that suggestion. So does this mean that for
> certain devices we'd want to basically passthrough by default and then
> at some point during boot take over with a properly managed IOMMU
> domain?

Yes, my understanding that this is what Robin suggested here:

https://lore.kernel.org/linux-iommu/cb12808b-7316-19db-7413-b7f852a6f8ae@arm.com/

> The primary goal here is to move towards using the DMA API rather than
> the IOMMU API directly, so we don't really have the option of replacing
> with an explicitly created domain. Unless we have code in the DMA/IOMMU
> code that does this somehow.
> 
> But I'm not sure what would be a good way to mark certain devices as
> needing an identity domain by default. Do we still use the reserved-
> memory node for that?

The reserved-memory indeed shouldn't be needed for resolving the
implicit IOMMU problem since we could mark certain devices within the
kernel IOMMU driver.

I haven't got around to trying to implement the implicit IOMMU support
yet, but I suppose we could implement the def_domain_type() hook in the
SMMU driver and then return IOMMU_DOMAIN_IDENTITY for the Display/VDE
devices. Then the Display/VDE drivers will take over the identity domain
and replace it with the explicit domain.

> That would still require some sort of flag to
> specify which reserved-memory regions would need this identity mapping
> because, as was pointed out in earlier review, some devices may have
> reserved-memory regions that are not meant to be identity mapped.

Please note that the reserved-memory approach also creates problem for
selection of a large CMA region if FB is located somewhere in a middle
of DRAM.

I already see that the FB's reserved-memory will break CMA for Nexus 7
and Acer A500 because CMA area overlaps with the bootloader's FB :)

Also keep in mind that initrd needs a location too and location usually
hardwired in a bootloader. Hence it increases pressure on the CMA selection.

>> Secondly, all NVIDIA bootloaders are passing tegra_fbmem=... via
>> kernel's cmdline with the physical location of the framebuffer in
>> memory. Maybe we could support this option?
> 
> I'm not a big fan of that command-line option, but I also realize that
> for older bootloaders that's probably the only option we have. I don't
> suppose all of the devices support U-Boot?

Majority of devices in a wild don't use u-boot and they have a
locked-down bootloader. Still it's possible to chain-load u-boot or
bypass the "security" and replace the bootloader, but these approaches
aren't widely supported because they take a lot of effort to be
implemented and maintained.

Even those devices that use proper u-boot usually never updating it and
are running some ancient version. You can't ignore all those people :)

> Because ideally we'd just
> translate from tegra_fbmem=... to reserved-memory region there so that
> we don't have to carry backwards-compatibility code for these purely
> downstream bootloaders.

IIRC, in the past Robin Murphy was suggesting to read out hardware state
early during kernel boot in order to find what regions are in use by
hardware.

I think it should be easy to do for the display controller since we
could check clock and PD states in order to decide whether DC's IO could
be accessed and then read out the FB pointer and size. I guess it should
take about hundred lines of code.

But the easiest way should be to ignore this trouble for devices that
have IOMMU disabled by default and simply allow display to show garbage.
Nobody ever complained about this for the past 7+ years :)

Hence implementing the dummy-identity domain support should be enough
for solving the problem, at least this should work for pre-T186 devices.

Re: [PATCH v2 1/4] dt-bindings: reserved-memory: Document "active" property

[Robin Murphy]

On 2020-09-24 17:23, Dmitry Osipenko wrote:
> 24.09.2020 17:01, Thierry Reding пишет:
>> On Thu, Sep 24, 2020 at 04:23:59PM +0300, Dmitry Osipenko wrote:
>>> 04.09.2020 15:59, Thierry Reding пишет:
>>>> From: Thierry Reding <treding@nvidia.com>
>>>>
>>>> Reserved memory regions can be marked as "active" if hardware is
>>>> expected to access the regions during boot and before the operating
>>>> system can take control. One example where this is useful is for the
>>>> operating system to infer whether the region needs to be identity-
>>>> mapped through an IOMMU.
>>>>
>>>> Signed-off-by: Thierry Reding <treding@nvidia.com>
>>>> ---
>>>>   .../bindings/reserved-memory/reserved-memory.txt           | 7 +++++++
>>>>   1 file changed, 7 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt b/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
>>>> index 4dd20de6977f..163d2927e4fc 100644
>>>> --- a/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
>>>> +++ b/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
>>>> @@ -63,6 +63,13 @@ reusable (optional) - empty property
>>>>         able to reclaim it back. Typically that means that the operating
>>>>         system can use that region to store volatile or cached data that
>>>>         can be otherwise regenerated or migrated elsewhere.
>>>> +active (optional) - empty property
>>>> +    - If this property is set for a reserved memory region, it indicates
>>>> +      that some piece of hardware may be actively accessing this region.
>>>> +      Should the operating system want to enable IOMMU protection for a
>>>> +      device, all active memory regions must have been identity-mapped
>>>> +      in order to ensure that non-quiescent hardware during boot can
>>>> +      continue to access the memory.
>>>>   
>>>>   Linux implementation note:
>>>>   - If a "linux,cma-default" property is present, then Linux will use the
>>>>
>>>
>>> Hi,
>>>
>>> Could you please explain what devices need this quirk? I see that you're
>>> targeting Tegra SMMU driver, which means that it should be some pre-T186
>>> device.
>>
>> Primarily I'm looking at Tegra210 and later, because on earlier devices
>> the bootloader doesn't consistently initialize display. I know that it
>> does on some devices, but not all of them.
> 
> AFAIK, all tablet devices starting with Tegra20 that have display panel
> are initializing display at a boot time for showing splash screen. This
> includes all T20/T30/T114 tablets that are already supported by upstream
> kernel.
> 
>> This same code should also
>> work on Tegra186 and later (with an ARM SMMU) although the situation is
>> slightly more complicated there because IOMMU translations will fault by
>> default long before these identity mappings can be established.
>>
>>> Is this reservation needed for some device that has display
>>> hardwired to a very specific IOMMU domain at the boot time?
>>
>> No, this is only used to convey information about the active framebuffer
>> to the kernel. In practice the DMA/IOMMU code will use this information
>> to establish a 1:1 mapping on whatever IOMMU domain that was picked for
>> display.
>>
>>> If you're targeting devices that don't have IOMMU enabled by default at
>>> the boot time, then this approach won't work for the existing devices
>>> which won't ever get an updated bootloader.
>>
>> If the devices don't use an IOMMU, then there should be no problem. The
>> extra reserved-memory nodes would still be necessary to ensure that the
>> kernel doesn't reuse the framebuffer memory for the slab allocator, but
>> if no IOMMU is used, then the display controller accessing the memory
>> isn't going to cause problems other than perhaps scanning out data that
>> is no longer a framebuffer.
>>
>> There should also be no problem for devices with an old bootloader
>> because this code is triggered by the presence of a reserved-memory node
>> referenced via the memory-region property. Devices with an old
>> bootloader should continue to work as they did before. Although I
>> suppose they would start faulting once we enable DMA/IOMMU integration
>> for Tegra SMMU if they have a bootloader that does initialize display to
>> actively scan out during boot.
>>
>>> I think Robin Murphy already suggested that we should simply create
>>> a dummy "identity" IOMMU domain by default for the DRM/VDE devices and
>>> then replace it with an explicitly created domain within the drivers.
>>
>> I don't recall reading about that suggestion. So does this mean that for
>> certain devices we'd want to basically passthrough by default and then
>> at some point during boot take over with a properly managed IOMMU
>> domain?
> 
> Yes, my understanding that this is what Robin suggested here:
> 
> https://lore.kernel.org/linux-iommu/cb12808b-7316-19db-7413-b7f852a6f8ae@arm.com/

Just to clarify, what I was talking about there is largely orthogonal to 
the issue here. That was about systems with limited translation 
resources letting translation be specifically opt-in by IOMMU-aware 
drivers. It probably *would* happen to obviate the issue of disrupting 
live DMA at boot time on these particular Tegra platforms, but we still 
need something like Thierry's solution in general, since IOMMU drivers 
may have no other way to determine whether devices are active at boot 
and they have to take care to avoid breaking anything - e.g. SMMUv3 will 
at a bare minimum need to set up *some* form of valid stream table entry 
for the relevant device(s) right at the beginning where we first probe 
and reset the SMMU itself, regardless of what happens with domains and 
addresses later down the line.

>> The primary goal here is to move towards using the DMA API rather than
>> the IOMMU API directly, so we don't really have the option of replacing
>> with an explicitly created domain. Unless we have code in the DMA/IOMMU
>> code that does this somehow.
>>
>> But I'm not sure what would be a good way to mark certain devices as
>> needing an identity domain by default. Do we still use the reserved-
>> memory node for that?
> 
> The reserved-memory indeed shouldn't be needed for resolving the
> implicit IOMMU problem since we could mark certain devices within the
> kernel IOMMU driver.
> 
> I haven't got around to trying to implement the implicit IOMMU support
> yet, but I suppose we could implement the def_domain_type() hook in the
> SMMU driver and then return IOMMU_DOMAIN_IDENTITY for the Display/VDE
> devices. Then the Display/VDE drivers will take over the identity domain
> and replace it with the explicit domain.

FWIW I've already cooked up identity domain support for tegra-gart; I 
was planning on tackling it for tegra-smmu as well for the next version 
of my arm default domains series (which will be after the next -rc1 now 
since I'm just about to take some long-overdue holiday).

>> That would still require some sort of flag to
>> specify which reserved-memory regions would need this identity mapping
>> because, as was pointed out in earlier review, some devices may have
>> reserved-memory regions that are not meant to be identity mapped.
> 
> Please note that the reserved-memory approach also creates problem for
> selection of a large CMA region if FB is located somewhere in a middle
> of DRAM.
> 
> I already see that the FB's reserved-memory will break CMA for Nexus 7
> and Acer A500 because CMA area overlaps with the bootloader's FB :)
> 
> Also keep in mind that initrd needs a location too and location usually
> hardwired in a bootloader. Hence it increases pressure on the CMA selection.
> 
>>> Secondly, all NVIDIA bootloaders are passing tegra_fbmem=... via
>>> kernel's cmdline with the physical location of the framebuffer in
>>> memory. Maybe we could support this option?
>>
>> I'm not a big fan of that command-line option, but I also realize that
>> for older bootloaders that's probably the only option we have. I don't
>> suppose all of the devices support U-Boot?
> 
> Majority of devices in a wild don't use u-boot and they have a
> locked-down bootloader. Still it's possible to chain-load u-boot or
> bypass the "security" and replace the bootloader, but these approaches
> aren't widely supported because they take a lot of effort to be
> implemented and maintained.
> 
> Even those devices that use proper u-boot usually never updating it and
> are running some ancient version. You can't ignore all those people :)
> 
>> Because ideally we'd just
>> translate from tegra_fbmem=... to reserved-memory region there so that
>> we don't have to carry backwards-compatibility code for these purely
>> downstream bootloaders.
> 
> IIRC, in the past Robin Murphy was suggesting to read out hardware state
> early during kernel boot in order to find what regions are in use by
> hardware.

I doubt I suggested that in general, because I've always firmly believed 
it to be a terrible idea. I've debugged too many cases where firmware or 
kexec has inadvertently left DMA running and corrupted kernel memory, so 
in general we definitely *don't* want to blindly trust random hardware 
state. Anything I may have said in relation to Qualcomm's fundamentally 
broken hypervisor/bootloader setup should not be considered outside that 
specific context ;)

Robin.

> I think it should be easy to do for the display controller since we
> could check clock and PD states in order to decide whether DC's IO could
> be accessed and then read out the FB pointer and size. I guess it should
> take about hundred lines of code.
> 
> But the easiest way should be to ignore this trouble for devices that
> have IOMMU disabled by default and simply allow display to show garbage.
> Nobody ever complained about this for the past 7+ years :)
> 
> Hence implementing the dummy-identity domain support should be enough
> for solving the problem, at least this should work for pre-T186 devices.
> 

Re: [PATCH v2 1/4] dt-bindings: reserved-memory: Document "active" property

[Dmitry Osipenko]

25.09.2020 15:39, Robin Murphy пишет:
...
>> IIRC, in the past Robin Murphy was suggesting to read out hardware state
>> early during kernel boot in order to find what regions are in use by
>> hardware.
> 
> I doubt I suggested that in general, because I've always firmly believed
> it to be a terrible idea. I've debugged too many cases where firmware or
> kexec has inadvertently left DMA running and corrupted kernel memory, so
> in general we definitely *don't* want to blindly trust random hardware
> state. Anything I may have said in relation to Qualcomm's fundamentally
> broken hypervisor/bootloader setup should not be considered outside that
> specific context ;)
> 
> Robin.
> 
>> I think it should be easy to do for the display controller since we
>> could check clock and PD states in order to decide whether DC's IO could
>> be accessed and then read out the FB pointer and size. I guess it should
>> take about hundred lines of code.

The active DMA is indeed very dangerous, but it's a bit less dangerous
in a case of read-only DMA.

I got another idea of how we could benefit from the active display
hardware. Maybe we could do the following:

1. Check whether display is active

2. Allocate CMA that matches the FB size

3. Create identity mapping for the CMA

4. Switch display framebuffer to our CMA

5. Create very early simple-framebuffer out of the CMA

6. Once Tegra DRM driver is loaded, it will kick out the simple-fb, and
thus, release temporal CMA and identity mapping.

This will provide us with a very early framebuffer output and it will
work on all devices out-of-the-box!

Re: [PATCH v2 1/4] dt-bindings: reserved-memory: Document "active" property

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 3362 bytes --]

On Fri, Sep 25, 2020 at 04:21:17PM +0300, Dmitry Osipenko wrote:
> 25.09.2020 15:39, Robin Murphy пишет:
> ...
> >> IIRC, in the past Robin Murphy was suggesting to read out hardware state
> >> early during kernel boot in order to find what regions are in use by
> >> hardware.
> > 
> > I doubt I suggested that in general, because I've always firmly believed
> > it to be a terrible idea. I've debugged too many cases where firmware or
> > kexec has inadvertently left DMA running and corrupted kernel memory, so
> > in general we definitely *don't* want to blindly trust random hardware
> > state. Anything I may have said in relation to Qualcomm's fundamentally
> > broken hypervisor/bootloader setup should not be considered outside that
> > specific context ;)
> > 
> > Robin.
> > 
> >> I think it should be easy to do for the display controller since we
> >> could check clock and PD states in order to decide whether DC's IO could
> >> be accessed and then read out the FB pointer and size. I guess it should
> >> take about hundred lines of code.
> 
> The active DMA is indeed very dangerous, but it's a bit less dangerous
> in a case of read-only DMA.
> 
> I got another idea of how we could benefit from the active display
> hardware. Maybe we could do the following:
> 
> 1. Check whether display is active
> 
> 2. Allocate CMA that matches the FB size
> 
> 3. Create identity mapping for the CMA
> 
> 4. Switch display framebuffer to our CMA
> 
> 5. Create very early simple-framebuffer out of the CMA
> 
> 6. Once Tegra DRM driver is loaded, it will kick out the simple-fb, and
> thus, release temporal CMA and identity mapping.
> 
> This will provide us with a very early framebuffer output and it will
> work on all devices out-of-the-box!

Well that's already kind of what this is trying to achieve, only
skipping the CMA step because the memory is already there and actively
being scanned out from. The problem with your sequence above is first
that you have to allocate from CMA, which means that this has to wait
until CMA becomes available. That's fairly early, but it's not
immediately there. Until you get to that point, there's always the
potential for the display controller to read out from memory that may
now be used for something else. As you said, read-only active DMA isn't
as dangerous as write DMA, but it's not very nice either.

Furthermore, your point 5. above requires device-specific knowledge and
as I mentioned earlier that requires a small, but not necessarily
trivial, device-specific driver to work, which is very impractical for
multi-platform kernels.

There's nothing preventing these reserved-memory regions from being
reused to implement simple-framebuffer. I could in fact imagine a fairly
simple extension to the existing simple-framebuffer binding that could
look like this for Tegra:

	dc@52000000 {
		compatible = "nvidia,tegra210-display", "simple-framebuffer";
		...
		memory-region = <&framebuffer>;
		width = <1920>;
		height = <1080>;
		stride = <7680>;
		format = "r8g8b8";
		...
	};

That's not dissimilar to what you're proposing above, except that it
moves everything before step 5. into the bootloader's responsibility and
therefore avoids the need for hardware-specific early display code in
the kernel.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 1/4] dt-bindings: reserved-memory: Document "active" property

[Dmitry Osipenko]

25.09.2020 15:39, Robin Murphy пишет:
...
>> Yes, my understanding that this is what Robin suggested here:
>>
>> https://lore.kernel.org/linux-iommu/cb12808b-7316-19db-7413-b7f852a6f8ae@arm.com/
>>
> 
> Just to clarify, what I was talking about there is largely orthogonal to
> the issue here. That was about systems with limited translation
> resources letting translation be specifically opt-in by IOMMU-aware
> drivers. It probably *would* happen to obviate the issue of disrupting
> live DMA at boot time on these particular Tegra platforms, but we still
> need something like Thierry's solution in general, since IOMMU drivers
> may have no other way to determine whether devices are active at boot
> and they have to take care to avoid breaking anything - e.g. SMMUv3 will
> at a bare minimum need to set up *some* form of valid stream table entry
> for the relevant device(s) right at the beginning where we first probe
> and reset the SMMU itself, regardless of what happens with domains and
> addresses later down the line.

Yes, I only meant that yours suggestion also should be useful here.
Anyways, thank you for the clarification :)

I agree that the Thierry's proposal is good! But it needs some more
thought yet because it's not very applicable to the current devices.

>>> The primary goal here is to move towards using the DMA API rather than
>>> the IOMMU API directly, so we don't really have the option of replacing
>>> with an explicitly created domain. Unless we have code in the DMA/IOMMU
>>> code that does this somehow.
>>>
>>> But I'm not sure what would be a good way to mark certain devices as
>>> needing an identity domain by default. Do we still use the reserved-
>>> memory node for that?
>>
>> The reserved-memory indeed shouldn't be needed for resolving the
>> implicit IOMMU problem since we could mark certain devices within the
>> kernel IOMMU driver.
>>
>> I haven't got around to trying to implement the implicit IOMMU support
>> yet, but I suppose we could implement the def_domain_type() hook in the
>> SMMU driver and then return IOMMU_DOMAIN_IDENTITY for the Display/VDE
>> devices. Then the Display/VDE drivers will take over the identity domain
>> and replace it with the explicit domain.
> 
> FWIW I've already cooked up identity domain support for tegra-gart; I
> was planning on tackling it for tegra-smmu as well for the next version
> of my arm default domains series (which will be after the next -rc1 now
> since I'm just about to take some long-overdue holiday).

Very nice! Maybe we will have some more food for the discussion by the
time you'll return. Have a good time!

Re: [PATCH v2 1/4] dt-bindings: reserved-memory: Document "active" property

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 11414 bytes --]

On Fri, Sep 25, 2020 at 01:39:07PM +0100, Robin Murphy wrote:
> On 2020-09-24 17:23, Dmitry Osipenko wrote:
> > 24.09.2020 17:01, Thierry Reding пишет:
> > > On Thu, Sep 24, 2020 at 04:23:59PM +0300, Dmitry Osipenko wrote:
> > > > 04.09.2020 15:59, Thierry Reding пишет:
> > > > > From: Thierry Reding <treding@nvidia.com>
> > > > > 
> > > > > Reserved memory regions can be marked as "active" if hardware is
> > > > > expected to access the regions during boot and before the operating
> > > > > system can take control. One example where this is useful is for the
> > > > > operating system to infer whether the region needs to be identity-
> > > > > mapped through an IOMMU.
> > > > > 
> > > > > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > > > > ---
> > > > >   .../bindings/reserved-memory/reserved-memory.txt           | 7 +++++++
> > > > >   1 file changed, 7 insertions(+)
> > > > > 
> > > > > diff --git a/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt b/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
> > > > > index 4dd20de6977f..163d2927e4fc 100644
> > > > > --- a/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
> > > > > +++ b/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
> > > > > @@ -63,6 +63,13 @@ reusable (optional) - empty property
> > > > >         able to reclaim it back. Typically that means that the operating
> > > > >         system can use that region to store volatile or cached data that
> > > > >         can be otherwise regenerated or migrated elsewhere.
> > > > > +active (optional) - empty property
> > > > > +    - If this property is set for a reserved memory region, it indicates
> > > > > +      that some piece of hardware may be actively accessing this region.
> > > > > +      Should the operating system want to enable IOMMU protection for a
> > > > > +      device, all active memory regions must have been identity-mapped
> > > > > +      in order to ensure that non-quiescent hardware during boot can
> > > > > +      continue to access the memory.
> > > > >   Linux implementation note:
> > > > >   - If a "linux,cma-default" property is present, then Linux will use the
> > > > > 
> > > > 
> > > > Hi,
> > > > 
> > > > Could you please explain what devices need this quirk? I see that you're
> > > > targeting Tegra SMMU driver, which means that it should be some pre-T186
> > > > device.
> > > 
> > > Primarily I'm looking at Tegra210 and later, because on earlier devices
> > > the bootloader doesn't consistently initialize display. I know that it
> > > does on some devices, but not all of them.
> > 
> > AFAIK, all tablet devices starting with Tegra20 that have display panel
> > are initializing display at a boot time for showing splash screen. This
> > includes all T20/T30/T114 tablets that are already supported by upstream
> > kernel.
> > 
> > > This same code should also
> > > work on Tegra186 and later (with an ARM SMMU) although the situation is
> > > slightly more complicated there because IOMMU translations will fault by
> > > default long before these identity mappings can be established.
> > > 
> > > > Is this reservation needed for some device that has display
> > > > hardwired to a very specific IOMMU domain at the boot time?
> > > 
> > > No, this is only used to convey information about the active framebuffer
> > > to the kernel. In practice the DMA/IOMMU code will use this information
> > > to establish a 1:1 mapping on whatever IOMMU domain that was picked for
> > > display.
> > > 
> > > > If you're targeting devices that don't have IOMMU enabled by default at
> > > > the boot time, then this approach won't work for the existing devices
> > > > which won't ever get an updated bootloader.
> > > 
> > > If the devices don't use an IOMMU, then there should be no problem. The
> > > extra reserved-memory nodes would still be necessary to ensure that the
> > > kernel doesn't reuse the framebuffer memory for the slab allocator, but
> > > if no IOMMU is used, then the display controller accessing the memory
> > > isn't going to cause problems other than perhaps scanning out data that
> > > is no longer a framebuffer.
> > > 
> > > There should also be no problem for devices with an old bootloader
> > > because this code is triggered by the presence of a reserved-memory node
> > > referenced via the memory-region property. Devices with an old
> > > bootloader should continue to work as they did before. Although I
> > > suppose they would start faulting once we enable DMA/IOMMU integration
> > > for Tegra SMMU if they have a bootloader that does initialize display to
> > > actively scan out during boot.
> > > 
> > > > I think Robin Murphy already suggested that we should simply create
> > > > a dummy "identity" IOMMU domain by default for the DRM/VDE devices and
> > > > then replace it with an explicitly created domain within the drivers.
> > > 
> > > I don't recall reading about that suggestion. So does this mean that for
> > > certain devices we'd want to basically passthrough by default and then
> > > at some point during boot take over with a properly managed IOMMU
> > > domain?
> > 
> > Yes, my understanding that this is what Robin suggested here:
> > 
> > https://lore.kernel.org/linux-iommu/cb12808b-7316-19db-7413-b7f852a6f8ae@arm.com/
> 
> Just to clarify, what I was talking about there is largely orthogonal to the
> issue here. That was about systems with limited translation resources
> letting translation be specifically opt-in by IOMMU-aware drivers. It
> probably *would* happen to obviate the issue of disrupting live DMA at boot
> time on these particular Tegra platforms, but we still need something like
> Thierry's solution in general, since IOMMU drivers may have no other way to
> determine whether devices are active at boot and they have to take care to
> avoid breaking anything - e.g. SMMUv3 will at a bare minimum need to set up
> *some* form of valid stream table entry for the relevant device(s) right at
> the beginning where we first probe and reset the SMMU itself, regardless of
> what happens with domains and addresses later down the line.
> 
> > > The primary goal here is to move towards using the DMA API rather than
> > > the IOMMU API directly, so we don't really have the option of replacing
> > > with an explicitly created domain. Unless we have code in the DMA/IOMMU
> > > code that does this somehow.
> > > 
> > > But I'm not sure what would be a good way to mark certain devices as
> > > needing an identity domain by default. Do we still use the reserved-
> > > memory node for that?
> > 
> > The reserved-memory indeed shouldn't be needed for resolving the
> > implicit IOMMU problem since we could mark certain devices within the
> > kernel IOMMU driver.
> > 
> > I haven't got around to trying to implement the implicit IOMMU support
> > yet, but I suppose we could implement the def_domain_type() hook in the
> > SMMU driver and then return IOMMU_DOMAIN_IDENTITY for the Display/VDE
> > devices. Then the Display/VDE drivers will take over the identity domain
> > and replace it with the explicit domain.
> 
> FWIW I've already cooked up identity domain support for tegra-gart; I was
> planning on tackling it for tegra-smmu as well for the next version of my
> arm default domains series (which will be after the next -rc1 now since I'm
> just about to take some long-overdue holiday).
> 
> > > That would still require some sort of flag to
> > > specify which reserved-memory regions would need this identity mapping
> > > because, as was pointed out in earlier review, some devices may have
> > > reserved-memory regions that are not meant to be identity mapped.
> > 
> > Please note that the reserved-memory approach also creates problem for
> > selection of a large CMA region if FB is located somewhere in a middle
> > of DRAM.
> > 
> > I already see that the FB's reserved-memory will break CMA for Nexus 7
> > and Acer A500 because CMA area overlaps with the bootloader's FB :)
> > 
> > Also keep in mind that initrd needs a location too and location usually
> > hardwired in a bootloader. Hence it increases pressure on the CMA selection.
> > 
> > > > Secondly, all NVIDIA bootloaders are passing tegra_fbmem=... via
> > > > kernel's cmdline with the physical location of the framebuffer in
> > > > memory. Maybe we could support this option?
> > > 
> > > I'm not a big fan of that command-line option, but I also realize that
> > > for older bootloaders that's probably the only option we have. I don't
> > > suppose all of the devices support U-Boot?
> > 
> > Majority of devices in a wild don't use u-boot and they have a
> > locked-down bootloader. Still it's possible to chain-load u-boot or
> > bypass the "security" and replace the bootloader, but these approaches
> > aren't widely supported because they take a lot of effort to be
> > implemented and maintained.
> > 
> > Even those devices that use proper u-boot usually never updating it and
> > are running some ancient version. You can't ignore all those people :)
> > 
> > > Because ideally we'd just
> > > translate from tegra_fbmem=... to reserved-memory region there so that
> > > we don't have to carry backwards-compatibility code for these purely
> > > downstream bootloaders.
> > 
> > IIRC, in the past Robin Murphy was suggesting to read out hardware state
> > early during kernel boot in order to find what regions are in use by
> > hardware.
> 
> I doubt I suggested that in general, because I've always firmly believed it
> to be a terrible idea. I've debugged too many cases where firmware or kexec
> has inadvertently left DMA running and corrupted kernel memory, so in
> general we definitely *don't* want to blindly trust random hardware state.
> Anything I may have said in relation to Qualcomm's fundamentally broken
> hypervisor/bootloader setup should not be considered outside that specific
> context ;)

I agree with this. I have no doubt that it could be done, technically,
but at the same time this is code that would have to run very early on
and therefore would have to be built-in. So even if that's just a couple
of hundred lines of code, it still means that we'd need that couple of
hundred lines of code for potentially each platform supported by a given
multi-platform kernel, and that's quickly going to add up.

So I think transporting this knowledge in device tree is a fair
compromise. The bootloader knows exactly whether the display hardware
was left active, so it's easy to update the device tree accordingly
before passing it to the kernel and it allows us to use generic code to
perform this quirking.

For Tegra specifically I'm not even sure this would work on all
generations. For example on Tegra186, the BPMP provides access to
clocks, resets and powergates. So the BPMP is needed to determine
whether or not the display hardware is active (you need to query clocks,
resets and powergates before accessing registers, because accessing
registers of an unclocked, in-reset or powergated hardware block
crashes). However, the BPMP is also typically behind the SMMU, so that
would make for a nice cyclic dependency.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 1/4] dt-bindings: reserved-memory: Document "active" property

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 10933 bytes --]

On Thu, Sep 24, 2020 at 07:23:34PM +0300, Dmitry Osipenko wrote:
> 24.09.2020 17:01, Thierry Reding пишет:
> > On Thu, Sep 24, 2020 at 04:23:59PM +0300, Dmitry Osipenko wrote:
> >> 04.09.2020 15:59, Thierry Reding пишет:
> >>> From: Thierry Reding <treding@nvidia.com>
> >>>
> >>> Reserved memory regions can be marked as "active" if hardware is
> >>> expected to access the regions during boot and before the operating
> >>> system can take control. One example where this is useful is for the
> >>> operating system to infer whether the region needs to be identity-
> >>> mapped through an IOMMU.
> >>>
> >>> Signed-off-by: Thierry Reding <treding@nvidia.com>
> >>> ---
> >>>  .../bindings/reserved-memory/reserved-memory.txt           | 7 +++++++
> >>>  1 file changed, 7 insertions(+)
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt b/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
> >>> index 4dd20de6977f..163d2927e4fc 100644
> >>> --- a/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
> >>> +++ b/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
> >>> @@ -63,6 +63,13 @@ reusable (optional) - empty property
> >>>        able to reclaim it back. Typically that means that the operating
> >>>        system can use that region to store volatile or cached data that
> >>>        can be otherwise regenerated or migrated elsewhere.
> >>> +active (optional) - empty property
> >>> +    - If this property is set for a reserved memory region, it indicates
> >>> +      that some piece of hardware may be actively accessing this region.
> >>> +      Should the operating system want to enable IOMMU protection for a
> >>> +      device, all active memory regions must have been identity-mapped
> >>> +      in order to ensure that non-quiescent hardware during boot can
> >>> +      continue to access the memory.
> >>>  
> >>>  Linux implementation note:
> >>>  - If a "linux,cma-default" property is present, then Linux will use the
> >>>
> >>
> >> Hi,
> >>
> >> Could you please explain what devices need this quirk? I see that you're
> >> targeting Tegra SMMU driver, which means that it should be some pre-T186
> >> device.
> > 
> > Primarily I'm looking at Tegra210 and later, because on earlier devices
> > the bootloader doesn't consistently initialize display. I know that it
> > does on some devices, but not all of them.
> 
> AFAIK, all tablet devices starting with Tegra20 that have display panel
> are initializing display at a boot time for showing splash screen. This
> includes all T20/T30/T114 tablets that are already supported by upstream
> kernel.
> 
> > This same code should also
> > work on Tegra186 and later (with an ARM SMMU) although the situation is
> > slightly more complicated there because IOMMU translations will fault by
> > default long before these identity mappings can be established.
> > 
> >> Is this reservation needed for some device that has display
> >> hardwired to a very specific IOMMU domain at the boot time?
> > 
> > No, this is only used to convey information about the active framebuffer
> > to the kernel. In practice the DMA/IOMMU code will use this information
> > to establish a 1:1 mapping on whatever IOMMU domain that was picked for
> > display.
> > 
> >> If you're targeting devices that don't have IOMMU enabled by default at
> >> the boot time, then this approach won't work for the existing devices
> >> which won't ever get an updated bootloader.
> > 
> > If the devices don't use an IOMMU, then there should be no problem. The
> > extra reserved-memory nodes would still be necessary to ensure that the
> > kernel doesn't reuse the framebuffer memory for the slab allocator, but
> > if no IOMMU is used, then the display controller accessing the memory
> > isn't going to cause problems other than perhaps scanning out data that
> > is no longer a framebuffer.
> > 
> > There should also be no problem for devices with an old bootloader
> > because this code is triggered by the presence of a reserved-memory node
> > referenced via the memory-region property. Devices with an old
> > bootloader should continue to work as they did before. Although I
> > suppose they would start faulting once we enable DMA/IOMMU integration
> > for Tegra SMMU if they have a bootloader that does initialize display to
> > actively scan out during boot.
> > 
> >> I think Robin Murphy already suggested that we should simply create
> >> a dummy "identity" IOMMU domain by default for the DRM/VDE devices and
> >> then replace it with an explicitly created domain within the drivers.
> > 
> > I don't recall reading about that suggestion. So does this mean that for
> > certain devices we'd want to basically passthrough by default and then
> > at some point during boot take over with a properly managed IOMMU
> > domain?
> 
> Yes, my understanding that this is what Robin suggested here:
> 
> https://lore.kernel.org/linux-iommu/cb12808b-7316-19db-7413-b7f852a6f8ae@arm.com/
> 
> > The primary goal here is to move towards using the DMA API rather than
> > the IOMMU API directly, so we don't really have the option of replacing
> > with an explicitly created domain. Unless we have code in the DMA/IOMMU
> > code that does this somehow.
> > 
> > But I'm not sure what would be a good way to mark certain devices as
> > needing an identity domain by default. Do we still use the reserved-
> > memory node for that?
> 
> The reserved-memory indeed shouldn't be needed for resolving the
> implicit IOMMU problem since we could mark certain devices within the
> kernel IOMMU driver.
> 
> I haven't got around to trying to implement the implicit IOMMU support
> yet, but I suppose we could implement the def_domain_type() hook in the
> SMMU driver and then return IOMMU_DOMAIN_IDENTITY for the Display/VDE
> devices. Then the Display/VDE drivers will take over the identity domain
> and replace it with the explicit domain.

Actually, the plan (and part of the reason for this patch) is to
transition the display driver over to using the DMA API rather than
creating IOMMU domains explicitly.

Explicit IOMMU usage currently works around this prior to Tegra186
because IOMMU translations are not enabled until after the device has
been attached to the IOMMU, at which point a mapping will already have
been created. It's also part of the reason why we don't support DMA-
type domains yet on Tegra SMMU, because as soon as we do, this would
cause a fault storm during boot.

On Tegra186 this same problem exists because the driver does support DMA
type domains and hence the kernel will try to set those up for display
during early boot and before the driver has had a chance to set up
mappings (or quiesce the display hardware).

> > That would still require some sort of flag to
> > specify which reserved-memory regions would need this identity mapping
> > because, as was pointed out in earlier review, some devices may have
> > reserved-memory regions that are not meant to be identity mapped.
> 
> Please note that the reserved-memory approach also creates problem for
> selection of a large CMA region if FB is located somewhere in a middle
> of DRAM.
> 
> I already see that the FB's reserved-memory will break CMA for Nexus 7
> and Acer A500 because CMA area overlaps with the bootloader's FB :)
> 
> Also keep in mind that initrd needs a location too and location usually
> hardwired in a bootloader. Hence it increases pressure on the CMA selection.

I do understand those issues, but there's really not a lot we can do
about it. It's wrong for CMA to overlap with the framebuffer from which
the display hardware just keeps scanning out, so all that these reserved
memory nodes do is actually fix a long-standing bug (depending on how
paranoid you are, you might even consider it a security hole).

For older devices that don't have a bootloader that properly defines
this memory as reserved, they should be able to continue to use a CMA
that overlaps with the framebuffer and work just like before.

> >> Secondly, all NVIDIA bootloaders are passing tegra_fbmem=... via
> >> kernel's cmdline with the physical location of the framebuffer in
> >> memory. Maybe we could support this option?
> > 
> > I'm not a big fan of that command-line option, but I also realize that
> > for older bootloaders that's probably the only option we have. I don't
> > suppose all of the devices support U-Boot?
> 
> Majority of devices in a wild don't use u-boot and they have a
> locked-down bootloader. Still it's possible to chain-load u-boot or
> bypass the "security" and replace the bootloader, but these approaches
> aren't widely supported because they take a lot of effort to be
> implemented and maintained.
> 
> Even those devices that use proper u-boot usually never updating it and
> are running some ancient version. You can't ignore all those people :)

I'm not trying to ignore all those people, but at the same time I don't
think legacy devices about which we can't do much should prevent new
devices from doing the right thing and properly reserving memory that
the kernel isn't supposed to use.

You already suggested that we could choose an identity domain by default
for VDE and display for this. That sounds like a good compromise to me,
but I think we should do that only on platforms where we can't implement
this properly.

> > Because ideally we'd just
> > translate from tegra_fbmem=... to reserved-memory region there so that
> > we don't have to carry backwards-compatibility code for these purely
> > downstream bootloaders.
> 
> IIRC, in the past Robin Murphy was suggesting to read out hardware state
> early during kernel boot in order to find what regions are in use by
> hardware.
> 
> I think it should be easy to do for the display controller since we
> could check clock and PD states in order to decide whether DC's IO could
> be accessed and then read out the FB pointer and size. I guess it should
> take about hundred lines of code.
> 
> But the easiest way should be to ignore this trouble for devices that
> have IOMMU disabled by default and simply allow display to show garbage.
> Nobody ever complained about this for the past 7+ years :)
> 
> Hence implementing the dummy-identity domain support should be enough
> for solving the problem, at least this should work for pre-T186 devices.

I'd still like to do this properly at least on Tegra210 as well. It
should be possible to come up with a set of criteria where the dummy
identity domain should be used and where it shouldn't. I think that's
the easiest part of this and can easily be implemented as a couple-of-
lines quirk in some kernel driver.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 1/4] dt-bindings: reserved-memory: Document "active" property

[Dmitry Osipenko]

05.11.2020 18:49, Thierry Reding пишет:
> On Thu, Sep 24, 2020 at 07:23:34PM +0300, Dmitry Osipenko wrote:
>> 24.09.2020 17:01, Thierry Reding пишет:
>>> On Thu, Sep 24, 2020 at 04:23:59PM +0300, Dmitry Osipenko wrote:
>>>> 04.09.2020 15:59, Thierry Reding пишет:
>>>>> From: Thierry Reding <treding@nvidia.com>
>>>>>
>>>>> Reserved memory regions can be marked as "active" if hardware is
>>>>> expected to access the regions during boot and before the operating
>>>>> system can take control. One example where this is useful is for the
>>>>> operating system to infer whether the region needs to be identity-
>>>>> mapped through an IOMMU.
>>>>>
>>>>> Signed-off-by: Thierry Reding <treding@nvidia.com>
>>>>> ---
>>>>>  .../bindings/reserved-memory/reserved-memory.txt           | 7 +++++++
>>>>>  1 file changed, 7 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt b/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
>>>>> index 4dd20de6977f..163d2927e4fc 100644
>>>>> --- a/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
>>>>> +++ b/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
>>>>> @@ -63,6 +63,13 @@ reusable (optional) - empty property
>>>>>        able to reclaim it back. Typically that means that the operating
>>>>>        system can use that region to store volatile or cached data that
>>>>>        can be otherwise regenerated or migrated elsewhere.
>>>>> +active (optional) - empty property
>>>>> +    - If this property is set for a reserved memory region, it indicates
>>>>> +      that some piece of hardware may be actively accessing this region.
>>>>> +      Should the operating system want to enable IOMMU protection for a
>>>>> +      device, all active memory regions must have been identity-mapped
>>>>> +      in order to ensure that non-quiescent hardware during boot can
>>>>> +      continue to access the memory.
>>>>>  
>>>>>  Linux implementation note:
>>>>>  - If a "linux,cma-default" property is present, then Linux will use the
>>>>>
>>>>
>>>> Hi,
>>>>
>>>> Could you please explain what devices need this quirk? I see that you're
>>>> targeting Tegra SMMU driver, which means that it should be some pre-T186
>>>> device.
>>>
>>> Primarily I'm looking at Tegra210 and later, because on earlier devices
>>> the bootloader doesn't consistently initialize display. I know that it
>>> does on some devices, but not all of them.
>>
>> AFAIK, all tablet devices starting with Tegra20 that have display panel
>> are initializing display at a boot time for showing splash screen. This
>> includes all T20/T30/T114 tablets that are already supported by upstream
>> kernel.
>>
>>> This same code should also
>>> work on Tegra186 and later (with an ARM SMMU) although the situation is
>>> slightly more complicated there because IOMMU translations will fault by
>>> default long before these identity mappings can be established.
>>>
>>>> Is this reservation needed for some device that has display
>>>> hardwired to a very specific IOMMU domain at the boot time?
>>>
>>> No, this is only used to convey information about the active framebuffer
>>> to the kernel. In practice the DMA/IOMMU code will use this information
>>> to establish a 1:1 mapping on whatever IOMMU domain that was picked for
>>> display.
>>>
>>>> If you're targeting devices that don't have IOMMU enabled by default at
>>>> the boot time, then this approach won't work for the existing devices
>>>> which won't ever get an updated bootloader.
>>>
>>> If the devices don't use an IOMMU, then there should be no problem. The
>>> extra reserved-memory nodes would still be necessary to ensure that the
>>> kernel doesn't reuse the framebuffer memory for the slab allocator, but
>>> if no IOMMU is used, then the display controller accessing the memory
>>> isn't going to cause problems other than perhaps scanning out data that
>>> is no longer a framebuffer.
>>>
>>> There should also be no problem for devices with an old bootloader
>>> because this code is triggered by the presence of a reserved-memory node
>>> referenced via the memory-region property. Devices with an old
>>> bootloader should continue to work as they did before. Although I
>>> suppose they would start faulting once we enable DMA/IOMMU integration
>>> for Tegra SMMU if they have a bootloader that does initialize display to
>>> actively scan out during boot.
>>>
>>>> I think Robin Murphy already suggested that we should simply create
>>>> a dummy "identity" IOMMU domain by default for the DRM/VDE devices and
>>>> then replace it with an explicitly created domain within the drivers.
>>>
>>> I don't recall reading about that suggestion. So does this mean that for
>>> certain devices we'd want to basically passthrough by default and then
>>> at some point during boot take over with a properly managed IOMMU
>>> domain?
>>
>> Yes, my understanding that this is what Robin suggested here:
>>
>> https://lore.kernel.org/linux-iommu/cb12808b-7316-19db-7413-b7f852a6f8ae@arm.com/
>>
>>> The primary goal here is to move towards using the DMA API rather than
>>> the IOMMU API directly, so we don't really have the option of replacing
>>> with an explicitly created domain. Unless we have code in the DMA/IOMMU
>>> code that does this somehow.
>>>
>>> But I'm not sure what would be a good way to mark certain devices as
>>> needing an identity domain by default. Do we still use the reserved-
>>> memory node for that?
>>
>> The reserved-memory indeed shouldn't be needed for resolving the
>> implicit IOMMU problem since we could mark certain devices within the
>> kernel IOMMU driver.
>>
>> I haven't got around to trying to implement the implicit IOMMU support
>> yet, but I suppose we could implement the def_domain_type() hook in the
>> SMMU driver and then return IOMMU_DOMAIN_IDENTITY for the Display/VDE
>> devices. Then the Display/VDE drivers will take over the identity domain
>> and replace it with the explicit domain.
> 
> Actually, the plan (and part of the reason for this patch) is to
> transition the display driver over to using the DMA API rather than
> creating IOMMU domains explicitly.
> 
> Explicit IOMMU usage currently works around this prior to Tegra186
> because IOMMU translations are not enabled until after the device has
> been attached to the IOMMU, at which point a mapping will already have
> been created. It's also part of the reason why we don't support DMA-
> type domains yet on Tegra SMMU, because as soon as we do, this would
> cause a fault storm during boot.
> 
> On Tegra186 this same problem exists because the driver does support DMA
> type domains and hence the kernel will try to set those up for display
> during early boot and before the driver has had a chance to set up
> mappings (or quiesce the display hardware).
> 
>>> That would still require some sort of flag to
>>> specify which reserved-memory regions would need this identity mapping
>>> because, as was pointed out in earlier review, some devices may have
>>> reserved-memory regions that are not meant to be identity mapped.
>>
>> Please note that the reserved-memory approach also creates problem for
>> selection of a large CMA region if FB is located somewhere in a middle
>> of DRAM.
>>
>> I already see that the FB's reserved-memory will break CMA for Nexus 7
>> and Acer A500 because CMA area overlaps with the bootloader's FB :)
>>
>> Also keep in mind that initrd needs a location too and location usually
>> hardwired in a bootloader. Hence it increases pressure on the CMA selection.
> 
> I do understand those issues, but there's really not a lot we can do
> about it. It's wrong for CMA to overlap with the framebuffer from which
> the display hardware just keeps scanning out, so all that these reserved
> memory nodes do is actually fix a long-standing bug (depending on how
> paranoid you are, you might even consider it a security hole).
> 
> For older devices that don't have a bootloader that properly defines
> this memory as reserved, they should be able to continue to use a CMA
> that overlaps with the framebuffer and work just like before.
> 
>>>> Secondly, all NVIDIA bootloaders are passing tegra_fbmem=... via
>>>> kernel's cmdline with the physical location of the framebuffer in
>>>> memory. Maybe we could support this option?
>>>
>>> I'm not a big fan of that command-line option, but I also realize that
>>> for older bootloaders that's probably the only option we have. I don't
>>> suppose all of the devices support U-Boot?
>>
>> Majority of devices in a wild don't use u-boot and they have a
>> locked-down bootloader. Still it's possible to chain-load u-boot or
>> bypass the "security" and replace the bootloader, but these approaches
>> aren't widely supported because they take a lot of effort to be
>> implemented and maintained.
>>
>> Even those devices that use proper u-boot usually never updating it and
>> are running some ancient version. You can't ignore all those people :)
> 
> I'm not trying to ignore all those people, but at the same time I don't
> think legacy devices about which we can't do much should prevent new
> devices from doing the right thing and properly reserving memory that
> the kernel isn't supposed to use.
> 
> You already suggested that we could choose an identity domain by default
> for VDE and display for this. That sounds like a good compromise to me,
> but I think we should do that only on platforms where we can't implement
> this properly.
> 
>>> Because ideally we'd just
>>> translate from tegra_fbmem=... to reserved-memory region there so that
>>> we don't have to carry backwards-compatibility code for these purely
>>> downstream bootloaders.
>>
>> IIRC, in the past Robin Murphy was suggesting to read out hardware state
>> early during kernel boot in order to find what regions are in use by
>> hardware.
>>
>> I think it should be easy to do for the display controller since we
>> could check clock and PD states in order to decide whether DC's IO could
>> be accessed and then read out the FB pointer and size. I guess it should
>> take about hundred lines of code.
>>
>> But the easiest way should be to ignore this trouble for devices that
>> have IOMMU disabled by default and simply allow display to show garbage.
>> Nobody ever complained about this for the past 7+ years :)
>>
>> Hence implementing the dummy-identity domain support should be enough
>> for solving the problem, at least this should work for pre-T186 devices.
> 
> I'd still like to do this properly at least on Tegra210 as well. It
> should be possible to come up with a set of criteria where the dummy
> identity domain should be used and where it shouldn't. I think that's
> the easiest part of this and can easily be implemented as a couple-of-
> lines quirk in some kernel driver.

Any solution should be fine as long as it doesn't break older devices.
If you'll have patches that will needs testing, then please feel free to
ping me.