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

[Oleksii Moisieiev <Oleksii_Moisieiev@xxxxxxxx>]

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. 
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?
>
>>>>           xen,dom0-sci-agent-id = <0>;
>>>>
>>>>           scmi_shm_0: sram@47ff0000 {
>>>>             compatible = "arm,scmi-shmem";
>>>>             reg = <0x0 0x47ff0000 0x0 0x1000>;
>>>>           };
>>>>
> [...]
>