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

[Oleksii Moisieiev <Oleksii_Moisieiev@xxxxxxxx>]

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.


Best regards,
Oleksii