|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v9 4/5] xen/arm: scmi: introduce SCI SCMI SMC multi-agent driver
Hi Stefano,
Thank you for a deep review.
I'm on the finishing line preparing the next version.
Just a small clarifications before I will send the new patch version.
Please see below.
On 30/01/2026 02:12, Stefano Stabellini wrote:
> On Thu, 29 Jan 2026, Oleksii Moisieiev wrote:
>> 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 memort 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_shm_0: sram@47ff0000 {
>> compatible = "arm,scmi-shmem";
>> reg = <0x0 0x47ff0000 0x0 0x1000>;
>> };
>>
>> /* Xen SCMI management channel */
>> scmi_shm_1: sram@47ff1000 {
>> compatible = "arm,scmi-shmem";
>> reg = <0x0 0x47ff1000 0x0 0x1000>;
>> };
>>
>> scmi_shm_2: sram@47ff2000 {
>> compatible = "arm,scmi-shmem";
>> reg = <0x0 0x47ff2000 0x0 0x1000>;
>> };
>>
>> scmi_shm_3: sram@47ff3000 {
>> compatible = "arm,scmi-shmem";
>> reg = <0x0 0x47ff3000 0x0 0x1000>;
>> };
>>
>> 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>;
>> xen,dom0-sci-agent-id = <0>;
>>
>> scmi_xen: scmi {
>> compatible = "arm,scmi-smc";
>> arm,smc-id = <0x82000003>; <--- Xen management agent func_id
>> #address-cells = <1>;
>> #size-cells = <0>;
>> #access-controller-cells = <1>;
>> shmem = <&scmi_shm_1>; <--- Xen management agent shmem
>> };
>> };
>> };
>> };
>> };
>>
>> / {
>> /*
>> * Host SCMI OSPM channel - provided to the Dom0 as is if SCMI
>> * enabled for it, ignored by Xen multi-agent mediator
>> */
>> scmi_shm: sram@47ff0000 {
>> compatible = "arm,scmi-shmem";
>> reg = <0x0 0x47ff0000 0x0 0x1000>;
>> };
>>
>> firmware {
>> scmi: scmi {
>> compatible = "arm,scmi-smc";
>> arm,smc-id = <0x82000002>; <--- Host OSPM agent smc-id
>> #address-cells = < 1>;
>> #size-cells = < 0>;
>> shmem = <&scmi_shm>; <--- Host OSPM agent shmem
>>
>> protocol@X{
>> };
>> };
>> };
>> };
>>
[snip]
>> --- a/xen/arch/arm/firmware/Makefile
>> +++ b/xen/arch/arm/firmware/Makefile
>> @@ -1,2 +1,3 @@
>> obj-$(CONFIG_ARM_SCI) += sci.o
>> obj-$(CONFIG_SCMI_SMC) += scmi-smc.o
>> +obj-$(CONFIG_SCMI_SMC_MA) += scmi-shmem.o scmi-smc-multiagent.o
>> diff --git a/xen/arch/arm/firmware/scmi-proto.h
>> b/xen/arch/arm/firmware/scmi-proto.h
>> new file mode 100644
>> index 0000000000..49f63cfc0a
>> --- /dev/null
>> +++ b/xen/arch/arm/firmware/scmi-proto.h
>> @@ -0,0 +1,164 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * Arm System Control and Management Interface definitions
>> + * Version 3.0 (DEN0056C)
>> + *
>> + * Copyright (c) 2025 EPAM Systems
>> + */
>> +
>> +#ifndef ARM_FIRMWARE_SCMI_PROTO_H_
>> +#define ARM_FIRMWARE_SCMI_PROTO_H_
>> +
>> +#include <xen/stdint.h>
>> +
>> +#define SCMI_SHORT_NAME_MAX_SIZE 16
>> +
>> +/* SCMI status codes. See section 4.1.4 */
>> +#define SCMI_SUCCESS 0
>> +#define SCMI_NOT_SUPPORTED (-1)
>> +#define SCMI_INVALID_PARAMETERS (-2)
>> +#define SCMI_DENIED (-3)
>> +#define SCMI_NOT_FOUND (-4)
>> +#define SCMI_OUT_OF_RANGE (-5)
>> +#define SCMI_BUSY (-6)
>> +#define SCMI_COMMS_ERROR (-7)
>> +#define SCMI_GENERIC_ERROR (-8)
>> +#define SCMI_HARDWARE_ERROR (-9)
>> +#define SCMI_PROTOCOL_ERROR (-10)
>> +
>> +/* Protocol IDs */
>> +#define SCMI_BASE_PROTOCOL 0x10
>> +
>> +/* Base protocol message IDs */
>> +#define SCMI_BASE_PROTOCOL_VERSION 0x0
>> +#define SCMI_BASE_PROTOCOL_ATTIBUTES 0x1
>> +#define SCMI_BASE_PROTOCOL_MESSAGE_ATTRIBUTES 0x2
>> +#define SCMI_BASE_DISCOVER_AGENT 0x7
>> +#define SCMI_BASE_SET_DEVICE_PERMISSIONS 0x9
>> +#define SCMI_BASE_RESET_AGENT_CONFIGURATION 0xB
>> +
>> +typedef struct scmi_msg_header {
>> + uint8_t id;
>> + uint8_t type;
>> + uint8_t protocol;
>> + uint32_t status;
>> +} scmi_msg_header_t;
>>
> Should this be __packed or is there padding in here?
No because it'll be processed by pack_scmi_header() function.
>> +/* Table 2 Message header format */
>> +#define SCMI_HDR_ID GENMASK(7, 0)
>> +#define SCMI_HDR_TYPE GENMASK(9, 8)
>> +#define SCMI_HDR_PROTO GENMASK(17, 10)
>> +
>> +#define SCMI_FIELD_GET(_mask, _reg)
>> \
>> + ((typeof(_mask))(((_reg) & (_mask)) >> (ffs64(_mask) - 1)))
>> +#define SCMI_FIELD_PREP(_mask, _val)
>> \
>> + (((typeof(_mask))(_val) << (ffs64(_mask) - 1)) & (_mask))
>> +
>> +static inline uint32_t pack_scmi_header(scmi_msg_header_t *hdr)
>> +{
>> + return SCMI_FIELD_PREP(SCMI_HDR_ID, hdr->id) |
>> + SCMI_FIELD_PREP(SCMI_HDR_TYPE, hdr->type) |
>> + SCMI_FIELD_PREP(SCMI_HDR_PROTO, hdr->protocol);
>> +}
>> +
[snip]
>>
>>
>> +#include <xen/err.h>
>> +#include <xen/io.h>
>> +#include <asm/io.h>
>> +
>> +#include "scmi-proto.h"
>> +#include "scmi-shmem.h"
>> +
>> +static inline int
>> +shmem_channel_is_free(const volatile struct scmi_shared_mem __iomem *shmem)
>> +{
>> + return (readl(&shmem->channel_status) &
>> + SCMI_SHMEM_CHAN_STAT_CHANNEL_FREE) ? 0 : -EBUSY;
> This return zero when it is free but the function is called
> "shmem_channel_is_free", which typically you would expect returns true
> on success and false on failure
>
Fair enough. will rename to shmem_channel_status
>> +}
>> +
>> +int shmem_put_message(volatile struct scmi_shared_mem __iomem *shmem,
>> + scmi_msg_header_t *hdr, void *data, int len)
>> +{
>> + int ret;
>> +
>> + if ( (len + offsetof(struct scmi_shared_mem, msg_payload)) >
> should len be unsigned?
>
>
>> + SCMI_SHMEM_MAPPED_SIZE )
>> + {
>> + printk(XENLOG_ERR "scmi: Wrong size of smc message. Data is
>> invalid\n");
>> + return -EINVAL;
>> + }
>> +
>> + ret = shmem_channel_is_free(shmem);
>> + if ( ret )
>> + return ret;
>> +
>> + writel_relaxed(0x0, &shmem->channel_status);
>> + /* Writing 0x0 right now, but "shmem"_FLAG_INTR_ENABLED can be set */
>> + writel_relaxed(0x0, &shmem->flags);
>> + writel_relaxed(sizeof(shmem->msg_header) + len, &shmem->length);
>> + writel(pack_scmi_header(hdr), &shmem->msg_header);
>> +
>> + if ( len > 0 && data )
>> + memcpy_toio(shmem->msg_payload, data, len);
>> +
>> + return 0;
>> +}
>> +
>> +int shmem_get_response(const volatile struct scmi_shared_mem __iomem *shmem,
>> + scmi_msg_header_t *hdr, void *data, int len)
>> +{
>> + int recv_len;
>> + int ret;
>> + int pad = sizeof(hdr->status);
> this deserves a comment
Thanks. Added a comment. We skip status (common field) and return only
protocol specific info.
Status is processed separately in this func.
>
>> + if ( len >= SCMI_SHMEM_MAPPED_SIZE -
>> + offsetof(struct scmi_shared_mem, msg_payload) )
>> + {
>> + printk(XENLOG_ERR
>> + "scmi: Wrong size of input smc message. Data may be
>> invalid\n");
>> + return -EINVAL;
>> + }
>> +
>> + ret = shmem_channel_is_free(shmem);
>> + if ( ret )
>> + return ret;
>> +
>> + recv_len = readl(&shmem->length) - sizeof(shmem->msg_header);
>> +
>> + if ( recv_len < 0 )
>> + {
>> + printk(XENLOG_ERR
>> + "scmi: Wrong size of smc message. Data may be invalid\n");
>> + return -EINVAL;
>> + }
>> +
>> + unpack_scmi_header(readl(&shmem->msg_header), hdr);
>> +
>> + hdr->status = readl(&shmem->msg_payload);
>> + recv_len = recv_len > pad ? recv_len - pad : 0;
>> +
>> + ret = scmi_to_xen_errno(hdr->status);
>> + if ( ret )
>> + {
>> + printk(XENLOG_DEBUG "scmi: Error received: %d\n", ret);
>> + return ret;
>> + }
>> +
>> + if ( recv_len > len )
>> + {
>> + printk(XENLOG_ERR
>> + "scmi: Not enough buffer for message %d, expecting %d\n",
>> + recv_len, len);
>> + return -EINVAL;
>> + }
>> +
>> + if ( recv_len > 0 )
>> + memcpy_fromio(data, shmem->msg_payload + pad, recv_len);
>> +
>> + return 0;
>> +}
>> +
>> +/*
>> + * Local variables:
>> + * mode: C
>> + * c-file-style: "BSD"
>> + * c-basic-offset: 4
>> + * tab-width: 4
>> + * indent-tabs-mode: nil
>> + * End:
>> + */
>> diff --git a/xen/arch/arm/firmware/scmi-shmem.h
>> b/xen/arch/arm/firmware/scmi-shmem.h
>> new file mode 100644
>> index 0000000000..7313cb6b26
>> --- /dev/null
>> +++ b/xen/arch/arm/firmware/scmi-shmem.h
>> @@ -0,0 +1,45 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * Arm System Control and Management Interface definitions
>> + * Version 3.0 (DEN0056C)
>> + * Shared Memory based Transport
>> + *
>> + * Copyright (c) 2024 EPAM Systems
>> + */
>> +
>> +#ifndef ARM_FIRMWARE_SCMI_SHMEM_H_
>> +#define ARM_FIRMWARE_SCMI_SHMEM_H_
>> +
>> +#include <xen/stdint.h>
>> +
>> +#define SCMI_SHMEM_CHAN_STAT_CHANNEL_FREE BIT(0, UL)
>> +#define SCMI_SHMEM_CHAN_STAT_CHANNEL_ERROR BIT(1, UL)
>> +
>> +struct scmi_shared_mem {
>> + uint32_t reserved;
>> + uint32_t channel_status;
>> + uint32_t reserved1[2];
>> + uint32_t flags;
>> + uint32_t length;
>> + uint32_t msg_header;
>> + uint8_t msg_payload[];
>> +};
>> +
>> +#define SCMI_SHMEM_MAPPED_SIZE PAGE_SIZE
>> +
>> +int shmem_put_message(volatile struct scmi_shared_mem __iomem *shmem,
>> + scmi_msg_header_t *hdr, void *data, int len);
>> +
>> +int shmem_get_response(const volatile struct scmi_shared_mem __iomem *shmem,
>> + scmi_msg_header_t *hdr, void *data, int len);
>> +#endif /* ARM_FIRMWARE_SCMI_SHMEM_H_ */
>> +
>> +/*
>> + * Local variables:
>> + * mode: C
>> + * c-file-style: "BSD"
>> + * c-basic-offset: 4
>> + * tab-width: 4
>> + * indent-tabs-mode: nil
>> + * End:
>> + */
>> diff --git a/xen/arch/arm/firmware/scmi-smc-multiagent.c
>> b/xen/arch/arm/firmware/scmi-smc-multiagent.c
>> new file mode 100644
>> index 0000000000..339c45f285
>> --- /dev/null
>> +++ b/xen/arch/arm/firmware/scmi-smc-multiagent.c
>> @@ -0,0 +1,818 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * SCI SCMI multi-agent driver, using SMC/HVC shmem as transport.
>> + *
>> + * Oleksii Moisieiev <oleksii_moisieiev@xxxxxxxx>
>> + * Copyright (c) 2025 EPAM Systems
>> + */
>> +
>> +#include <xen/acpi.h>
>> +
>> +#include <xen/device_tree.h>
>> +#include <xen/init.h>
>> +#include <xen/iocap.h>
>> +#include <xen/err.h>
>> +#include <xen/libfdt/libfdt.h>
>> +#include <xen/string.h>
>> +#include <xen/param.h>
>> +#include <xen/sched.h>
>> +#include <xen/vmap.h>
>> +
>> +#include <asm/firmware/sci.h>
>> +#include <asm/smccc.h>
>> +
>> +#include "scmi-proto.h"
>> +#include "scmi-shmem.h"
>> +
>> +#define SCMI_SECONDARY_AGENTS "scmi-secondary-agents"
>> +
>> +struct scmi_channel {
>> + uint32_t agent_id;
>> + uint32_t func_id;
>> + domid_t domain_id;
>> + uint64_t paddr;
>> + struct scmi_shared_mem __iomem *shmem;
>> + spinlock_t lock;
>> + struct list_head list;
>> +};
>> +
>> +struct scmi_data {
>> + struct list_head channel_list;
>> + spinlock_t channel_list_lock;
>> + uint32_t func_id;
>> + bool initialized;
>> + uint32_t shmem_phandle;
>> + uint32_t hyp_channel_agent_id;
>> + struct dt_device_node *dt_dev;
>> +};
>> +
>> +static struct scmi_data scmi_data;
>> +
>> +static bool scmi_is_under_xen_sci(const struct dt_device_node *node)
>> +{
>> + const struct dt_device_node *p;
>> +
>> + for ( p = node->parent; p; p = p->parent )
>> + if ( dt_device_is_compatible(p, "xen,sci") )
>> + return true;
>> +
>> + return false;
>> +}
>> +
>> +static int send_smc_message(struct scmi_channel *chan_info,
>> + scmi_msg_header_t *hdr, void *data, int len)
>> +{
>> + struct arm_smccc_res resp;
>> + int ret;
>> +
>> + ret = shmem_put_message(chan_info->shmem, hdr, data, len);
>> + if ( ret )
>> + return ret;
>> +
>> + arm_smccc_1_1_smc(chan_info->func_id, 0, 0, 0, 0, 0, 0, 0, &resp);
>> +
>> + if ( resp.a0 == ARM_SMCCC_INVALID_PARAMETER )
> just noting that a0 is unsigned long and ARM_SMCCC_INVALID_PARAMETER is
> (-3). Maybe at least an explicit cast?
>
I'm reusing default interface. added explicit cast.
>> + return -EINVAL;
>> +
>> + if ( resp.a0 )
>> + return -EOPNOTSUPP;
>> +
>> + return 0;
>> +}
>> +
>> +static int do_smc_xfer(struct scmi_channel *chan_info, scmi_msg_header_t
>> *hdr,
>> + void *tx_data, int tx_size, void *rx_data, int
>> rx_size)
>> +{
>> + int ret = 0;
>> +
>> + ASSERT(chan_info && chan_info->shmem);
>> +
>> + if ( !hdr )
>> + return -EINVAL;
>> +
>> + spin_lock(&chan_info->lock);
>> +
>> + printk(XENLOG_DEBUG
>> + "scmi: agent_id = %d msg_id = %x type = %d, proto = %x\n",
>> + chan_info->agent_id, hdr->id, hdr->type, hdr->protocol);
>> +
>> + ret = send_smc_message(chan_info, hdr, tx_data, tx_size);
>> + if ( ret )
>> + goto clean;
>> +
>> + ret = shmem_get_response(chan_info->shmem, hdr, rx_data, rx_size);
>> +
>> +clean:
>> + printk(XENLOG_DEBUG
>> + "scmi: get smc response agent_id = %d msg_id = %x proto = %x
>> res=%d\n",
>> + chan_info->agent_id, hdr->id, hdr->protocol, ret);
>> +
>> + spin_unlock(&chan_info->lock);
>> +
>> + return ret;
>> +}
>> +
>> +static struct scmi_channel *get_channel_by_id(uint32_t agent_id)
>> +{
>> + struct scmi_channel *curr;
>> + bool found = false;
>> +
>> + spin_lock(&scmi_data.channel_list_lock);
>> + list_for_each_entry(curr, &scmi_data.channel_list, list)
>> + {
>> + if ( curr->agent_id == agent_id )
>> + {
>> + found = true;
>> + break;
>> + }
>> + }
>> +
>> + spin_unlock(&scmi_data.channel_list_lock);
>> + if ( found )
>> + return curr;
>> +
>> + return NULL;
>> +}
>> +
[snip]
>> +
>> +static int collect_agent_id(struct scmi_channel *agent_channel)
>> +{
>> + int ret;
>> + scmi_msg_header_t hdr;
>> + struct scmi_msg_base_discover_agent_p2a da_rx;
>> + struct scmi_msg_base_discover_agent_a2p da_tx;
>> +
>> + ret = map_channel_memory(agent_channel);
>> + if ( ret )
>> + return ret;
>> +
>> + hdr.id = SCMI_BASE_DISCOVER_AGENT;
>> + hdr.type = 0;
>> + hdr.protocol = SCMI_BASE_PROTOCOL;
>> +
>> + da_tx.agent_id = agent_channel->agent_id;
>> +
>> + ret = do_smc_xfer(agent_channel, &hdr, &da_tx, sizeof(da_tx), &da_rx,
>> + sizeof(da_rx));
>> + if ( agent_channel->domain_id != DOMID_XEN )
>> + unmap_channel_memory(agent_channel);
>> + if ( ret )
>> + return ret;
>>
> On error (ret != 0) should we call unmap_channel_memory ?
>
for DOMID_XEN it should be called later.
>> + printk(XENLOG_DEBUG "id=0x%x name=%s\n", da_rx.agent_id, da_rx.name);
>> + agent_channel->agent_id = da_rx.agent_id;
>> + return 0;
>> +}
>> +
>> +static __init int collect_agents(struct dt_device_node *scmi_node)
>> +{
>> + const struct dt_device_node *config_node;
>> + const __be32 *prop;
>>
[snip]
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |