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

[Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>]

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.

-- 
WBR, Volodymyr