Re: [PATCH v13 4/6] xen/arm: scmi: introduce SCI SCMI SMC multi-agent driver

[Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>]

Hi,

Oleksii Moisieiev <Oleksii_Moisieiev@xxxxxxxx> writes:

> On 24/09/2026 21:42, Volodymyr Babchuk wrote:
>> Hi Oleksii,
>>
>> Oleksii Moisieiev <Oleksii_Moisieiev@xxxxxxxx> writes:
>>
>>> On 24/09/2026 04:15, Volodymyr Babchuk wrote:
>>>> Hi Oleksii,
>>>>
>>>> Oleksii Moisieiev <Oleksii_Moisieiev@xxxxxxxx> writes:
>>>>
>>>>> On 23/09/2026 04:02, Volodymyr Babchuk wrote:
>>>>>> Hi Oleksii,
>>>>>>
>>>>>> Oleksii Moisieiev <Oleksii_Moisieiev@xxxxxxxx> writes:
>>>>>>
>>>>>> This is a great piece of documentation. It is larger than the text you
>>>>>> added to the 'doc' directory. So I believe it is better to put into a
>>>>>> design document, so it is will not be lost in the git commit messages.
>>>>> Hi Volodymyr,
>>>>>
>>>>> Thank you for a quick response.
>>>>>
>>>>> I've just recheked document in patch 6:
>>>>> docs/hypervisor-guide/arm/firmware/arm-scmi.rst and
>>>>>
>>>>> compared it with the information provided in the commit description.
>>>>>
>>>>> As I can see all information provided in the description (add schemes
>>>>> and dts examples at least)
>>>>>
>>>>> are present in the documentation. Also doc itself is more detailed then
>>>>> the commit description.
>>>>>
>>>>> Maybe I missed something?
>>>> Ah, so you put this information in the last patch in the series. Okay,
>>>> so I missed it.
>>>>
>>>>>>> This patch introduces SCI driver to support for ARM EL3 Trusted 
>>>>>>> Firmware-A
>>>>>>> (TF-A) which provides SCMI interface with multi-agent support, as shown
>>>>>>> below.
>>>>>>>
>>>>>>>      +-----------------------------------------+
>>>>>>>      |                                         |
>>>>>>>      | EL3 TF-A SCMI                           |
>>>>>>>      +-------+--+-------+--+-------+--+-------++
>>>>>>>      |shmem1 |  |shmem0 |  |shmem2 |  |shmemX |
>>>>>>>      +-----+-+  +---+---+  +--+----+  +---+---+
>>>>>>> smc-id1 |        |         |           |
>>>>>>> agent1  |        |         |           |
>>>>>>>      +-----v--------+---------+-----------+----+
>>>>>>>      |              |         |           |    |
>>>>>>>      |              |         |           |    |
>>>>>>>      +--------------+---------+-----------+----+
>>>>>>>             smc-id0 |  smc-id2|    smc-idX|
>>>>>>>             agent0  |  agent2 |    agentX |
>>>>>>>                     |         |           |
>>>>>>>                +----v---+  +--v-----+  +--v-----+
>>>>>>>                |        |  |        |  |        |
>>>>>>>                | Dom0   |  | Dom1   |  | DomX   |
>>>>>>>                |        |  |        |  |        |
>>>>>>>                |        |  |        |  |        |
>>>>>>>                +--------+  +--------+  +--------+
>>>>>>>
>>>>>>> The EL3 SCMI multi-agent firmware is expected to provide SCMI SMC shared
>>>>>>> memory transport for every Agent in the system.
>>>>>>>
>>>>>>> The SCMI Agent transport channel defined by pair:
>>>>>>>     - smc-id: SMC id used for Doorbell
>>>>>>>     - shmem: shared memory for messages transfer, Xen page
>>>>>>>     aligned. Shared memory is mapped with the following flags:
>>>>>>>     MT_DEVICE_nGnRE.
>>>>>>>
>>>>>>> The follwoing SCMI Agents are expected to be defined by SCMI FW to 
>>>>>>> enable SCMI
>>>>>>> multi-agent functionality under Xen:
>>>>>>> - Xen management agent: trusted agents that accesses to the Base 
>>>>>>> Protocol
>>>>>>> commands to configure agent specific permissions
>>>>>>> - OSPM VM agents: non-trusted agent, one for each Guest domain which is
>>>>>>>      allowed direct HW access. At least one OSPM VM agent has to be 
>>>>>>> provided
>>>>>>>      by FW if HW is handled only by Dom0 or Driver Domain.
>>>>>>>
>>>>>>> The EL3 SCMI FW is expected to implement following Base protocol 
>>>>>>> messages:
>>>>>>> - BASE_DISCOVER_AGENT (optional if agent_id was provided)
>>>>>>> - BASE_RESET_AGENT_CONFIGURATION (optional)
>>>>>>> - BASE_SET_DEVICE_PERMISSIONS (optional)
>>>>>>>
>>>>>>> The SCI SCMI SMC multi-agent driver implements following
>>>>>>> functionality:
>>>>>>> - The driver is initialized from the Xen SCMI container 
>>>>>>> ``xen_scmi_config``
>>>>>>>      (compatible ``xen,sci``) placed under ``/chosen/xen``. Only the
>>>>>>>      ``arm,scmi-smc`` node that is a child of this container will bind 
>>>>>>> to Xen;
>>>>>>>      other SCMI nodes (for example under ``/firmware``) are ignored to 
>>>>>>> avoid
>>>>>>>      stealing the host OSPM instance.
>>>>>>>
>>>>>>> scmi_shm_1: sram@47ff1000 {
>>>>>>>              compatible = "arm,scmi-shmem";
>>>>>>>              reg = <0x0 0x47ff1000 0x0 0x1000>;
>>>>>>> };
>>>>>>> scmi_xen: scmi {
>>>>>>>            compatible = "arm,scmi-smc";
>>>>>>>            arm,smc-id = <0x82000003>; <--- Xen management agent smc-id
>>>>>>>            #address-cells = < 1>;
>>>>>>>            #size-cells = < 0>;
>>>>>>>            #access-controller-cells = < 1>;
>>>>>>>            shmem = <&scmi_shm_1>; <--- Xen management agent shmem
>>>>>>> };
>>>>>>>
>>>>>>> - The driver obtains Xen specific SCMI Agent's configuration from the
>>>>>>>      Host DT, probes Agents and builds SCMI Agents list. The Agents
>>>>>>>      configuration is taken from "scmi-secondary-agents" property where
>>>>>>>      first item is "arm,smc-id", second - "arm,scmi-shmem" phandle and
>>>>>>>      third is optional "agent_id":
>>>>>>>
>>>>>>> / {
>>>>>>>      chosen {
>>>>>>>        xen {
>>>>>>>          ranges;
>>>>>>>          xen_scmi_config {
>>>>>>>            compatible = "xen,sci";
>>>>>>>            #address-cells = <2>;
>>>>>>>            #size-cells = <2>;
>>>>>>>            ranges;
>>>>>>>
>>>>>>>         scmi-secondary-agents = <
>>>>>>>              0x82000002 &scmi_shm_0 0
>>>>>>>              0x82000004 &scmi_shm_2 2
>>>>>>>              0x82000005 &scmi_shm_3 3>; <--- func_id, shmem, agent_id
>>>>>>>            #scmi-secondary-agents-cells = <3>;
>>>>>> I don't think that this is the correct way of using -cells property.
>>>>>>
>>>>>> These properties are used either to provide information for **child** 
>>>>>> nodes
>>>>>> (like #address-cells or #size-cells) or to provide a number of cells to
>>>>>> encode a specifier for a domain (like #interrupt-cells).
>>>>>>
>>>>>> Here you are doing neither of these. I think the proper way is to define
>>>>>> secondary agents as children:
>>>>>>
>>>>>> agents {
>>>>>>            #address-cells = 1;
>>>>>>            #size-cells = 0;
>>>>>>             scmi_agent_2: scmi_agent@2 {
>>>>>>               compatible = "arm,scmi-agent";  // Actually, I am not sure 
>>>>>> if
>>>>>>                                               // we allowed to use 'arm' 
>>>>>> namespace
>>>>>>
>>>>>>               reg = <2>;                      // Agent id goes here
>>>>>>               shmmem = &scmi_shm_2;
>>>>>>               arm,smc-id = 0x82000004;
>>>>>>             };
>>>>>> }
>>>>>>
>>>>>> With this approach you don't need #scmi-secondary-agents-cells at all
>>>>>> and the whole structure becomes more device-tree-ish.
>>>>> Thank you for looking at this. I agree that the two cases you list are
>>>>> the most common ones, but they are not the only sanctioned ones: there is 
>>>>> a
>>>>> third, long-standing pattern where a "#<name>-cells" property in a node
>>>>> declares the cell stride of a *non-phandle list property in that very same
>>>>> node*. That is exactly what "#scmi-secondary-agents-cells" does for
>>>>> "scmi-secondary-agents". Some evidence from the Devicetree Specification 
>>>>> and
>>>>> from Linux:
>>>>>
>>>>> 1) "ranges" and "interrupt-map" are parsed with the cell counts of the 
>>>>> node
>>>>>       that carries them, not of a child or of a phandle target.
>>>>>
>>>>>       dtc enforces this itself:
>>>>>
>>>>>       - scripts/dtc/checks.c:785 check_ranges_format() computes the entry
>>>>> length
>>>>>         as (parent #address-cells + this node's #address-cells + this 
>>>>> node's
>>>>>         #size-cells) and validates the length of "ranges" in this node
>>>>> against it.
>>>>>
>>>>>       - scripts/dtc/checks.c:1601 check_interrupt_map():
>>>>>
>>>>>             cellsize = node_addr_cells(node);
>>>>>             cellsize += propval_cell(get_property(node, 
>>>>> "#interrupt-cells"));
>>>>>
>>>>>         i.e. the stride of the "interrupt-map" entries in a node is taken 
>>>>> from
>>>>>         "#address-cells" and "#interrupt-cells" *of that same node*. Linux
>>>>> does
>>>>>         the same in drivers/of/irq.c.
>>>>>
>>>>>       So "a #*-cells property in node X describing the layout of a 
>>>>> property in
>>>>>       node X" is not an abuse of the convention, it is how two of the core
>>>>>       Devicetree Specification properties are defined.
>>>>>
>>>>> 2) There is a precedent whose shape is identical to ours - a vendor 
>>>>> property
>>>>>       holding a list, plus cells properties in the same node that describe
>>>>> how to
>>>>>       decode it:
>>>>>
>>>>>       Documentation/devicetree/bindings/tpm/ibm,vtpm.yaml:36
>>>>>
>>>>>           ibm,#dma-address-cells:
>>>>>             description:
>>>>>               number of cells that are used to encode the physical address
>>>>> field
>>>>>               of dma-window properties
>>>>>           ibm,#dma-size-cells:
>>>>>             description:
>>>>>               number of cells that are used to encode the size field of
>>>>>               dma-window properties
>>>>>           ibm,my-dma-window:
>>>>>             description: DMA window associated with this virtual I/O 
>>>>> Adapter
>>>>>
>>>>>       and the example (same file, lines 96-98):
>>>>>
>>>>>           ibm,#dma-address-cells = <0x2>;
>>>>>           ibm,#dma-size-cells = <0x2>;
>>>>>           ibm,my-dma-window = <0x10000003 0x0 0x0 0x0 0x10000000>;
>>>>>
>>>>>       Both cells properties are "required". The parser is
>>>>>       arch/powerpc/kernel/prom_parse.c:11 of_parse_dma_window(), which 
>>>>> reads
>>>>>       "ibm,#dma-address-cells" and "ibm,#dma-size-cells" from the same
>>>>> node "dn"
>>>>>       to walk "ibm,my-dma-window". There is no phandle and no child node
>>>>>       involved; this is exactly the "self-describing list property" 
>>>>> pattern.
>>>>>
>>>>> 3) A "#*-cells" property does not have to describe a phandle specifier
>>>>> at all:
>>>>>
>>>>>       - "#pinctrl-cells" (Documentation/devicetree/bindings/pinctrl/
>>>>>         pinctrl-single.yaml:55) gives the number of cells per entry of the
>>>>> plain
>>>>>         "pinctrl-single,pins" property; drivers/pinctrl/devicetree.c:297
>>>>>         pinctrl_find_cells_size() looks it up in the parent/grandparent 
>>>>> node.
>>>>>         No phandle specifier is involved.
>>>>>
>>>>>       - "#index-cells"
>>>>> (Documentation/devicetree/bindings/usb/fsl,usbmisc.yaml:56)
>>>>>         is not a specifier for any domain either.
>>>>>
>>>>> So the convention in practice is "a #<foo>-cells property states how
>>>>> many cells
>>>>> one <foo> entry occupies", and the entries may live in a child node's
>>>>> property
>>>>> (#address-cells), in a consumer's phandle specifier (#interrupt-cells,
>>>>> #clock-cells), or in a property of the declaring node itself (ranges,
>>>>> interrupt-map, ibm,my-dma-window). Our usage falls into the third group.
>>>>>
>>>>> This bring us to the following conclusion that
>>>>> #scmi-secondary-agents-cells usage
>>>>> doesn't technically break the device-tree convention with 2 exceptions:
>>>>>
>>>>> a) Vendor prefix. 
>>>>> Documentation/devicetree/bindings/writing-bindings.rst:68
>>>>>       says "DO use a vendor prefix on device-specific property names", 
>>>>> and all
>>>>>       the same-node precedents above are vendor-prefixed
>>>>> ("ibm,#dma-address-cells").
>>>>>       Both of our properties are Xen-specific and live under /chosen/xen,
>>>>> so if we
>>>>>       keep the flat form they should become "xen,scmi-secondary-agents" 
>>>>> and
>>>>>       "xen,#scmi-secondary-agents-cells".
>>>>>
>>>>> b) Entry layout. In our list the phandle is the second cell
>>>>>       (<smc-id &shmem agent-id>), which prevents the use of the standard
>>>>>       phandle-array helpers - they all expect the phandle first. If we
>>>>> keep the
>>>>>       flat form, reordering to <&shmem smc-id agent-id> would let the
>>>>> property be
>>>>>       parsed as an ordinary phandle-array.
>>>>>
>>>>> As for your proposal: the child-node form you suggest is certainly
>>>>> more idiomatic, it makes the agent id an addressable "reg" and it
>>>>> removes the
>>>>> optional 2-vs-3-cells variant altogether. My only reservations are that
>>>>> it grows
>>>>> the Xen DT parser (node iteration instead of a single property walk) and
>>>>> that the
>>>>> compatible string would need a namespace we own ("xen,scmi-agent" rather
>>>>> than
>>>>> "arm,scmi-agent", as you correctly suspected).
>>>>>
>>>>> So I do not think the current form is invalid, but I have no objection to
>>>>> moving to child nodes if you prefer it. Please let me know which way you
>>>>> would
>>>>> like me to go for v14 and I will respin accordingly.
>>>> Yes, I think more idiomatic format is better. Also, you need to write a
>>>> parser once, but users will write device tree more often.
>>>>
>>>> The second thing that bothers me is the shared memory nodes inside the
>>>> xen_scmi_config node. I believe they should belong to reserved_memory, no?
>>> During discussions on the previous versions, it was decided that the
>>> main goal is to leave the original device-tree untouched and put all
>>> Xen-related content into the xen node. That's why we preserve the
>>> original DTS structure, such as the scmi_shm_0 and /firmware/scmi nodes.
>>>
>>> Also, if you take a look at the arm,scmi.yaml file in the Linux kernel,
>>> there is no explicit requirement regarding the exact placement of the
>>> scmi-shmem node. As can be seen, arm,scmi-shmem is present in the
>>> patternProperties of sram.yaml, but some DTS files place it into
>>> reserved-memory instead.
>>>
>>> My main point is that we don't touch the original device-tree — we
>>> simply add the xen node and put all the required properties inside it.
>> Okay, but Xen should know about reserved memory, right? So, it is only
>> fair to have shared memory nodes in /reserved-memory/ as device tree
>> spec suggests.
>>> If we were to place shmem into sram or reserved-memory, it would give us
>>> the following structure:
>>>
>>> / {
>>>
>>>        xen {
>>>
>>>            <sram|reserved-memory>: {
>>>
>>>                scmi_shm0 ...
>>>
>>>               ....
>>>
>>>               scmi_shmX ...
>>>
>>>            }
>>>
>>>        }
>>>
>>> }
>>>
>>> This wouldn't provide any benefit but would overcomplicate the
>>> device-tree structure. What do you think?
>> Well, there is another argument why I want to move shared memory nodes
>> out of xen_scmi_config. In that case we can have more flat
>> structure. So, instead of
>>
>> xen_scmi_config {
>>          #address-cells = 2;
>>          #size-cells = 2;
>>
>>          shm1 {};
>>          shm2 {};
>>          shmN {};
>>
>>          agents {
>>                  #address-cells = 1;
>>                  #size-cells = 0;
>>                  agent1 {};
>>                  agent2 {};
>>          }
>> }
>>
>>
>> we could have
>>
>> xen_scmi_config {
>>          #address-cells = 1;
>>          #size-cells = 0;
>>
>>          agent1 {};
>>          agent2 {};
>> }
>>
>> which is more idiomatic, IMO.
>
>
> Ah, now I understand your point. Xen definitely parses the 
> /reserved-memory region, but that doesn't mean scmi_shm should be part 
> of it(as it is not required by the DT bindings), and this is not common 
> practice for vendor DTs.
>
> Let's say we have a board that delivers firmware supporting 8 agents, 
> starting from 0x440000 up until 0x448000, along with the device-tree for it.
>
> The device-tree it provides doesn't require all agents to be defined — 
> only the first one — but the range 0x440000 -> 0x449000 should be 
> reserved. That's why the DT should have the following format:
>
>
> / {
>
>      reserved-memory {
>
>           memory@440000 {
>
>              reg = < 0x440000 0x9000 >;
>
>          }
>
>     }
>
>     shm0: scmi_shm@0x440000 {
>
>     }
>
>     /firmware/scmi {
>
>        shm=  <&shm0>;
>
>    }
>
> }
>
> As can be seen, the memory is already reserved.
>
> Now let's add Xen to this DT:
>
> / {
>
>      reserved-memory {
>
>           memory@440000 {
>
>              reg = < 0x440000 0x9000 >;
>
>          }
>
>     }
>
>     shm0: scmi_shm@0x440000 {
>
>     }
>
>     /firmware/scmi {
>
>        shm=  <&shm0>;
>
>    }
>
>    /xen/xen_scmi_config {
>          #address-cells = 2;
>          #size-cells = 2;
>
>          shm1 {};
>          shm2 {};
>          shmN {};
>
>          agents {
>                  #address-cells = 1;
>                  #size-cells = 0;
>                  agent1 {};
>                  agent2 {};
>          }
>
> }
>
>
> So, as can be seen:
>
> - We don't make any modifications or break the vendor DT format, except 
> for adding the scmi node.
> - The reserved-memory node is parsed by Xen, so Xen is aware of the 
> reservations.
>
> - All shm nodes are placed in xen_scmi_config.

Okay, this makes sense, I guess... Anyway, I can't suggest a better alternative.

-- 
WBR, Volodymyr