ioremap() use with specific mailbox drivers and SCMI

ioremap() use with specific mailbox drivers and SCMI

[Florian Fainelli]

Hi Sudeep,
	
I am looking at the following files:

arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts:
        memory at 0 {
                device_type = "memory";
                reg = <0x00000000 0x00000000 0x00000000 0x05e00000>,
                      <0x00000000 0x05f00000 0x00000000 0x00001000>,
                      <0x00000000 0x05f02000 0x00000000 0x00efd000>,
                      <0x00000000 0x06e00000 0x00000000 0x0060f000>,
                      <0x00000000 0x07410000 0x00000000 0x1aaf0000>,
                      <0x00000000 0x22000000 0x00000000 0x1c000000>;
        };

arch/arm64/boot/dts/hisilicon/hi6220.dtsi:
                mailbox: mailbox at f7510000 {
                        compatible = "hisilicon,hi6220-mbox";
                        reg = <0x0 0xf7510000 0x0 0x1000>, /* IPC_S */
                              <0x0 0x06dff800 0x0 0x0800>; /* Mailbox
buffer */
                        interrupts = <GIC_SPI 94 IRQ_TYPE_LEVEL_HIGH>;
                        #mbox-cells = <3>;
                };

and this is quite an interesting approach, although it appears to be
quite fragile and simplistic to me for a number of reasons:

- if the mailbox at f7510000 was ever to be relocated within a parent "bus"
node that had a "ranges" property mapping the parent space from
0xf000_0000 to produce offset relative nodes, the translation for its
"reg" properties would apply relative to both cells, which means that
now, the second cell may no longer point to DRAM anymore. This can be
worked around by keeping that particular node outside of the "bus" node,
but it is essentially means we mixed address spaces within the "reg"
property

- punching holes in DRAM by omitting this mailbox's shared buffer from
the top-level memory "reg" property is extremely error prone, and seems
to be a shortcut/hack to allow the ARM/ARM64 implementations to
successfully call ioremap() against these regions. I am not commenting
about the other regions mentioned in the DTS, but pstore is possibly
another example where using ioremap() is convenient, but still not quite
correct.

Using a reserved-memory entry would be far less error prone, would
clearly show the intents of the specific regions being declared, and
could, through the use of additional properties (e.g: "map", as opposed
to "no-map") indicate a possible need to map that DRAM region into
kernel virtual memory and using e.g: memremap() APIs.

Also the semantics of ioremap() are not particularly portable from one
architecture to another whereas memremap() is IMHO.

Thank you
-- 
Florian

ioremap() use with specific mailbox drivers and SCMI

[Florian Fainelli]

+Sudeep as originally intended

On 04/19/2018 09:25 AM, Florian Fainelli wrote:
> Hi Sudeep,
> 	
> I am looking at the following files:
> 
> arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts:
>         memory at 0 {
>                 device_type = "memory";
>                 reg = <0x00000000 0x00000000 0x00000000 0x05e00000>,
>                       <0x00000000 0x05f00000 0x00000000 0x00001000>,
>                       <0x00000000 0x05f02000 0x00000000 0x00efd000>,
>                       <0x00000000 0x06e00000 0x00000000 0x0060f000>,
>                       <0x00000000 0x07410000 0x00000000 0x1aaf0000>,
>                       <0x00000000 0x22000000 0x00000000 0x1c000000>;
>         };
> 
> arch/arm64/boot/dts/hisilicon/hi6220.dtsi:
>                 mailbox: mailbox at f7510000 {
>                         compatible = "hisilicon,hi6220-mbox";
>                         reg = <0x0 0xf7510000 0x0 0x1000>, /* IPC_S */
>                               <0x0 0x06dff800 0x0 0x0800>; /* Mailbox
> buffer */
>                         interrupts = <GIC_SPI 94 IRQ_TYPE_LEVEL_HIGH>;
>                         #mbox-cells = <3>;
>                 };
> 
> and this is quite an interesting approach, although it appears to be
> quite fragile and simplistic to me for a number of reasons:
> 
> - if the mailbox at f7510000 was ever to be relocated within a parent "bus"
> node that had a "ranges" property mapping the parent space from
> 0xf000_0000 to produce offset relative nodes, the translation for its
> "reg" properties would apply relative to both cells, which means that
> now, the second cell may no longer point to DRAM anymore. This can be
> worked around by keeping that particular node outside of the "bus" node,
> but it is essentially means we mixed address spaces within the "reg"
> property
> 
> - punching holes in DRAM by omitting this mailbox's shared buffer from
> the top-level memory "reg" property is extremely error prone, and seems
> to be a shortcut/hack to allow the ARM/ARM64 implementations to
> successfully call ioremap() against these regions. I am not commenting
> about the other regions mentioned in the DTS, but pstore is possibly
> another example where using ioremap() is convenient, but still not quite
> correct.
> 
> Using a reserved-memory entry would be far less error prone, would
> clearly show the intents of the specific regions being declared, and
> could, through the use of additional properties (e.g: "map", as opposed
> to "no-map") indicate a possible need to map that DRAM region into
> kernel virtual memory and using e.g: memremap() APIs.
> 
> Also the semantics of ioremap() are not particularly portable from one
> architecture to another whereas memremap() is IMHO.
> 
> Thank you
> 

-- 
Florian

ioremap() use with specific mailbox drivers and SCMI

[Sudeep Holla]

+Arnd,

On 19/04/18 17:35, Florian Fainelli wrote:
> +Sudeep as originally intended
> 
> On 04/19/2018 09:25 AM, Florian Fainelli wrote:
>> Hi Sudeep,
>> 	
>> I am looking at the following files:
>>
>> arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts:
>>         memory at 0 {
>>                 device_type = "memory";
>>                 reg = <0x00000000 0x00000000 0x00000000 0x05e00000>,
>>                       <0x00000000 0x05f00000 0x00000000 0x00001000>,
>>                       <0x00000000 0x05f02000 0x00000000 0x00efd000>,
>>                       <0x00000000 0x06e00000 0x00000000 0x0060f000>,
>>                       <0x00000000 0x07410000 0x00000000 0x1aaf0000>,
>>                       <0x00000000 0x22000000 0x00000000 0x1c000000>;
>>         };
>>
>> arch/arm64/boot/dts/hisilicon/hi6220.dtsi:
>>                 mailbox: mailbox at f7510000 {
>>                         compatible = "hisilicon,hi6220-mbox";
>>                         reg = <0x0 0xf7510000 0x0 0x1000>, /* IPC_S */
>>                               <0x0 0x06dff800 0x0 0x0800>; /* Mailbox
>> buffer */
>>                         interrupts = <GIC_SPI 94 IRQ_TYPE_LEVEL_HIGH>;
>>                         #mbox-cells = <3>;
>>                 };
>>

Yes I did indeed point Jim Quinlan to look at the above DTS as I roughly
knew that it used DRAM for mailbox communication.

>> and this is quite an interesting approach, although it appears to be
>> quite fragile and simplistic to me for a number of reasons:
>>
>> - if the mailbox at f7510000 was ever to be relocated within a parent "bus"
>> node that had a "ranges" property mapping the parent space from
>> 0xf000_0000 to produce offset relative nodes, the translation for its
>> "reg" properties would apply relative to both cells, which means that
>> now, the second cell may no longer point to DRAM anymore. This can be
>> worked around by keeping that particular node outside of the "bus" node,
>> but it is essentially means we mixed address spaces within the "reg"
>> property
>>

I am not suggesting that. The memory remains where ever it belong in the
bus and just marked as reserved for use for mailbox. The SCMI binding
just refers by phandle into that. In that respect, the above example is
wrong as it has mixed the shared memory with the mailbox controller
address range.

>> - punching holes in DRAM by omitting this mailbox's shared buffer from
>> the top-level memory "reg" property is extremely error prone, and seems
>> to be a shortcut/hack to allow the ARM/ARM64 implementations to
>> successfully call ioremap() against these regions. I am not commenting
>> about the other regions mentioned in the DTS, but pstore is possibly
>> another example where using ioremap() is convenient, but still not quite
>> correct.
>>

OK new to me and sorry for lack of my knowledge.

>> Using a reserved-memory entry would be far less error prone, would
>> clearly show the intents of the specific regions being declared, and
>> could, through the use of additional properties (e.g: "map", as opposed
>> to "no-map") indicate a possible need to map that DRAM region into
>> kernel virtual memory and using e.g: memremap() APIs.
>>

Sorry for missing this. For some reasons, I had assumed memremap was
specifically for DRAM and when I wrote the driver, I was testing on
our internal Juno dev platform which has dedicated SRAM for the
communication with remote control processor. However having a quick look
at the API, I see no reasons to not use that as it takes care of using
ioremap if required. But I am wondering why the generic SRAM driver in
drivers/misc/sram.c uses ioremap.

>> Also the semantics of ioremap() are not particularly portable from one
>> architecture to another whereas memremap() is IMHO.
>>

Again, wasn't aware of that.

Arnd,

Do you see any issues moving from ioremap to memremap for SCMI ?
I am asking you as you would have seen various platforms having multiple
options for shared IPC memory and wanted to get you feedback.

-- 
Regards,
Sudeep

ioremap() use with specific mailbox drivers and SCMI

[Arnd Bergmann]

On Thu, Apr 19, 2018 at 7:09 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:
> On 19/04/18 17:35, Florian Fainelli wrote:
>
>>> Using a reserved-memory entry would be far less error prone, would
>>> clearly show the intents of the specific regions being declared, and
>>> could, through the use of additional properties (e.g: "map", as opposed
>>> to "no-map") indicate a possible need to map that DRAM region into
>>> kernel virtual memory and using e.g: memremap() APIs.
>>>
>
> Sorry for missing this. For some reasons, I had assumed memremap was
> specifically for DRAM and when I wrote the driver, I was testing on
> our internal Juno dev platform which has dedicated SRAM for the
> communication with remote control processor. However having a quick look
> at the API, I see no reasons to not use that as it takes care of using
> ioremap if required. But I am wondering why the generic SRAM driver in
> drivers/misc/sram.c uses ioremap.
>
>>> Also the semantics of ioremap() are not particularly portable from one
>>> architecture to another whereas memremap() is IMHO.
>>>
>
> Again, wasn't aware of that.
>
> Arnd,
>
> Do you see any issues moving from ioremap to memremap for SCMI ?
> I am asking you as you would have seen various platforms having multiple
> options for shared IPC memory and wanted to get you feedback.

Good question. The important point here is that whatever way the
kernel maps the buffer must match whatever the other end does:

- If you have regular RAM that is accessed using the CPU cache from
  firmware running on the same CPU, or using a cache-coherent method
  from a remote processor, you need to use memremap().

- If you have device registers somewhere, you must use ioremap()
  or possibly ioremap_wc().

- A dedicated SRAM is a bit tricky, usually that kind of buffer is accessed
  without going through the cache, so memremap() would be wrong.
  ioremap() is probably safe here.

- The last case would get you into real trouble: a memory buffer at
  a fixed location that is physicall in system RAM and excluded from
  the memory visible to the OS, but accessed from a remote processor
  that is not cache-coherent with your CPU caches. Here, you can't
  use memremap() since you want an uncached mapping, but ioremap()
  is probably also wrong. You could construct something using
  dma_map_single() that might work, but that's not what this API is
  meant for either.

          Arnd

ioremap() use with specific mailbox drivers and SCMI

[Florian Fainelli]

On 04/19/2018 02:05 PM, Arnd Bergmann wrote:
> On Thu, Apr 19, 2018 at 7:09 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:
>> On 19/04/18 17:35, Florian Fainelli wrote:
>>
>>>> Using a reserved-memory entry would be far less error prone, would
>>>> clearly show the intents of the specific regions being declared, and
>>>> could, through the use of additional properties (e.g: "map", as opposed
>>>> to "no-map") indicate a possible need to map that DRAM region into
>>>> kernel virtual memory and using e.g: memremap() APIs.
>>>>
>>
>> Sorry for missing this. For some reasons, I had assumed memremap was
>> specifically for DRAM and when I wrote the driver, I was testing on
>> our internal Juno dev platform which has dedicated SRAM for the
>> communication with remote control processor. However having a quick look
>> at the API, I see no reasons to not use that as it takes care of using
>> ioremap if required. But I am wondering why the generic SRAM driver in
>> drivers/misc/sram.c uses ioremap.
>>
>>>> Also the semantics of ioremap() are not particularly portable from one
>>>> architecture to another whereas memremap() is IMHO.
>>>>
>>
>> Again, wasn't aware of that.
>>
>> Arnd,
>>
>> Do you see any issues moving from ioremap to memremap for SCMI ?
>> I am asking you as you would have seen various platforms having multiple
>> options for shared IPC memory and wanted to get you feedback.
> 
> Good question. The important point here is that whatever way the
> kernel maps the buffer must match whatever the other end does:
> 
> - If you have regular RAM that is accessed using the CPU cache from
>   firmware running on the same CPU, or using a cache-coherent method
>   from a remote processor, you need to use memremap().
> 
> - If you have device registers somewhere, you must use ioremap()
>   or possibly ioremap_wc().
> 
> - A dedicated SRAM is a bit tricky, usually that kind of buffer is accessed
>   without going through the cache, so memremap() would be wrong.
>   ioremap() is probably safe here.
> 
> - The last case would get you into real trouble: a memory buffer at
>   a fixed location that is physicall in system RAM and excluded from
>   the memory visible to the OS, but accessed from a remote processor
>   that is not cache-coherent with your CPU caches. Here, you can't
>   use memremap() since you want an uncached mapping, but ioremap()
>   is probably also wrong. You could construct something using
>   dma_map_single() that might work, but that's not what this API is
>   meant for either.

I suppose as long as you have a struct page created for that memory area
you should be fine with the DMA-API, right? Would have to that to
happen, otherwise the reserved memory region is flagged with "nomap" and
then you won't have any, and it's truly invisible to the Linux.

Part of the problem is that, when using a "shmem" phandle, we have no
clue about what address space we might be come from, we might be able to
figure it out somehow, like, are within a node, within a bus, which
appears to map to register space, or are we within a reserved-memory
node, in which case, we likely point to RAM.

ioremap() is actually quite conservative but what the HiSilicon DTS file
I referred to is doing seems like a hack because it carves out the
mailbox area from it's memory node's "reg" property, and besides
silencing the warning in arch/arm*/mmm/ioremap.c about remapping RAM, I
am not sure how this is any safer than ignoring the warning and still
doing ioremap() against a region of RAM?
-- 
Florian