|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v14 4/6] xen/arm: scmi: introduce SCI SCMI SMC multi-agent driver
Hi Volodymyr,
Thank you for the review. I agree with the most of your comments, just
few more questions left:
On 23/10/2026 04:56, Volodymyr Babchuk wrote:
> Hi Oleksii,
>
> Oleksii Moisieiev <Oleksii_Moisieiev@xxxxxxxx> writes:
>
>> 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
[snip]
>> diff --git a/xen/arch/arm/firmware/scmi-shmem.c
>> b/xen/arch/arm/firmware/scmi-shmem.c
>> new file mode 100644
>> index 0000000000..e36745a85e
>> --- /dev/null
>> +++ b/xen/arch/arm/firmware/scmi-shmem.c
>> @@ -0,0 +1,118 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * SMC/HVC shmem transport implementation used by
>> + * SCI SCMI multi-agent driver.
>> + *
>> + * Oleksii Moisieiev <oleksii_moisieiev@xxxxxxxx>
>> + * Copyright (c) 2025 EPAM Systems
>> + */
>> +
>> +#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_status(const volatile struct scmi_shared_mem __iomem *shmem)
>> +{
>> + return (readl(&shmem->channel_status) &
>> + SCMI_SHMEM_CHAN_STAT_CHANNEL_FREE) ? 0 : -EBUSY;
>> +}
>> +
>> +int shmem_put_message(volatile struct scmi_shared_mem __iomem *shmem,
>> + scmi_msg_header_t *hdr, void *data, unsigned int len)
>> +{
>> + int ret;
>> +
>> + if ( (len + offsetof(struct scmi_shared_mem, msg_payload)) >
>> + SCMI_SHMEM_MAPPED_SIZE )
>> + {
>> + printk(XENLOG_ERR "scmi: Wrong size of smc message. Data is
>> invalid\n");
> "of the SMC message". Also, looks like it is over 80 characters.
agreed with both. thank you.
>> + return -EINVAL;
>> + }
>> +
>> + ret = shmem_channel_status(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);
> Why do you mix writel() and writel_relaxed() here?
The main idea that I'm filling up SCMI header information following
Device memory ordering and execute __iowmb before store in the last
writel call.
>> +
>> + 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, unsigned int len)
>> +{
>> + int recv_len;
>> + int ret;
>> + /*
>> + * First word of msg_payload carries the returned status; exclude it
>> from
>> + * recv_len so only the protocol payload is copied back to the caller.
>> + */
>> + int pad = sizeof(hdr->status);
>> +
>> + 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");
> "of the input SMC message"
+1
>> + return -EINVAL;
>> + }
>> +
>> + ret = shmem_channel_status(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");
> "of the SMC message"
>
> Also, I am wondering if you really need to print this error messages in
> the first place.
>
These generic messages were very helpful in determining the exact error.
For example, this function returns -EINVAL both if
len >= SCMI_SHMEM_MAPPED_SIZE - offsetof(struct scmi_shared_mem,
msg_payload) and if recv_len < 0
However, if you think they are excessive, I can remove the prints and return
different error codes instead.
>> + 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..722263aa77
>> --- /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[];
>> +};
> Should it be __packed?
The layout already matches the shared-memory format without padding:
Every fixed member is a uint32_t or an array of uint8_t, and the
mapping is page aligned.
Therefore __packed would not change these offsets, but would reduce the
structure's alignment guarantee. I would prefer to keep the natural
alignment and add compile-time offset checks to document the transport
layout. Does that address your concern?
>> +
>> +#define SCMI_SHMEM_MAPPED_SIZE PAGE_SIZE
>> +
>> +int shmem_put_message(volatile struct scmi_shared_mem __iomem *shmem,
> I am not sure if volatile makes sense here, taking into account that
> this is __iomem anyway.
>
> Also, shmem_put_message sounds to generic... Maybe scmi_put_message?
Agreed. volatile looks unneeded here. I will remove volatile and use
scmi_{get/put}_message function names.
>> + scmi_msg_header_t *hdr, void *data, unsigned int len);
>> +
>> +int shmem_get_response(const volatile struct scmi_shared_mem __iomem *shmem,
>> + scmi_msg_header_t *hdr, void *data, unsigned int
>> len);
> Basically the same comments as for previous one.
+
>> +#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..6c46f6cd36
>> --- /dev/null
>> +++ b/xen/arch/arm/firmware/scmi-smc-multiagent.c
>> @@ -0,0 +1,792 @@
>> +/* 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"
>> +
>> +struct scmi_channel {
>> + uint32_t agent_id;
> If all agent_ids large than UINT8_MAX are invalid, why do you need
> uint32_t here?
According to the DEN0056F document:
https://support.arm.com/documentation/den0056/f/?lang=en
agent_id is uint32_t in SCMI messages.
>> + uint32_t func_id;
>> + domid_t domain_id;
>> + uint64_t paddr;
> paddr_t
+1. thanks.
>> + 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;
>> +
>> + resp = arm_smccc_1_1_smc(chan_info->func_id, 0, 0, 0, 0, 0, 0, 0);
>> +
>> + if ( resp.a0 == (unsigned long)ARM_SMCCC_INVALID_PARAMETER )
>> + 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);
> You will print this even if send_smc_message() fails. I think it should
> go before "clean:" label.
>
> Also, maybe "cleanup:" ?
+1. It's better to place debug print before "clean" lavel.
>> +
>> + 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;
>> +}
>> +
>> +static struct scmi_channel *acquire_scmi_channel(struct domain *d,
>> + uint32_t agent_id)
>> +{
>> + struct scmi_channel *curr;
>> + struct scmi_channel *ret = ERR_PTR(-ENOENT);
>> +
>> + spin_lock(&scmi_data.channel_list_lock);
>> + list_for_each_entry(curr, &scmi_data.channel_list, list)
>> + {
>> + if ( curr->agent_id == agent_id )
>> + {
>> + if ( curr->domain_id != DOMID_INVALID )
>> + {
>> + ret = ERR_PTR(-EEXIST);
>> + break;
>> + }
>> +
>> + curr->domain_id = d->domain_id;
>> + ret = curr;
>> + break;
>> + }
>> + }
>> +
>> + spin_unlock(&scmi_data.channel_list_lock);
>> +
>> + return ret;
>> +}
>> +
>> +static void relinquish_scmi_channel(struct scmi_channel *channel)
>> +{
>> + ASSERT(channel != NULL);
>> +
>> + spin_lock(&scmi_data.channel_list_lock);
>> + channel->domain_id = DOMID_INVALID;
>> + spin_unlock(&scmi_data.channel_list_lock);
>> +}
>> +
>> +static int map_channel_memory(struct scmi_channel *channel)
>> +{
>> + ASSERT(channel && channel->paddr);
>> + channel->shmem = ioremap_nocache(channel->paddr,
>> SCMI_SHMEM_MAPPED_SIZE);
>> + if ( !channel->shmem )
>> + return -ENOMEM;
>> +
>> + channel->shmem->channel_status = SCMI_SHMEM_CHAN_STAT_CHANNEL_FREE;
>> + printk(XENLOG_DEBUG "scmi: Got shmem %lx after vmap %p\n",
>> channel->paddr,
> You will need proper format, not "%lx" for channel->paddr
Agree. with paddr_t I will use PRIpaddr format.
>> + channel->shmem);
>> +
>> + return 0;
>> +}
>> +
>> +static void unmap_channel_memory(struct scmi_channel *channel)
>> +{
>> + ASSERT(channel);
>> +
>> + if ( !channel->shmem )
>> + return;
>> +
>> + iounmap(channel->shmem);
>> + channel->shmem = NULL;
>> +}
>> +
>> +static struct scmi_channel *smc_create_channel(uint32_t agent_id,
>> + uint32_t func_id, uint64_t
>> addr)
>> +{
>> + struct scmi_channel *channel, *curr;
>> +
>> + spin_lock(&scmi_data.channel_list_lock);
>> +
>> + /* Check if channel already exists while holding the lock */
>> + list_for_each_entry(curr, &scmi_data.channel_list, list)
>> + {
>> + if ( curr->agent_id == agent_id )
>> + {
>> + spin_unlock(&scmi_data.channel_list_lock);
>> + return ERR_PTR(-EEXIST);
>> + }
>> + }
>> +
>> + channel = xmalloc(struct scmi_channel);
>> + if ( !channel )
>> + {
>> + spin_unlock(&scmi_data.channel_list_lock);
>> + return ERR_PTR(-ENOMEM);
>> + }
>> +
>> + spin_lock_init(&channel->lock);
>> + channel->agent_id = agent_id;
>> + channel->func_id = func_id;
>> + channel->domain_id = DOMID_INVALID;
>> + channel->shmem = NULL;
>> + channel->paddr = addr;
>> + list_add_tail(&channel->list, &scmi_data.channel_list);
>> +
>> + spin_unlock(&scmi_data.channel_list_lock);
>> + return channel;
>> +}
>> +
>> +static void free_channel_list(void)
>> +{
>> + struct scmi_channel *curr, *_curr;
>> + /*
>> + * Called only on the __init error path, before any runtime users exist,
> Then you can put this function into __init as well
+1
>> + * so no other thread is iterating channel_list. Keep the lock for the
>> + * whole drain for clarity and to avoid misleading readers about
>> possible
>> + * concurrent access.
>> + */
>> + spin_lock(&scmi_data.channel_list_lock);
>> + list_for_each_entry_safe(curr, _curr, &scmi_data.channel_list, list)
>> + {
>> + list_del(&curr->list);
>> + xfree(curr);
>> + }
>> + spin_unlock(&scmi_data.channel_list_lock);
>> +}
>> +
>> +static int __init
>> +scmi_dt_read_channel_addr(struct dt_device_node *scmi_node, u64 *addr,
>> + u64 *size)
>> +{
>> + struct dt_device_node *shmem_node;
>> + uint32_t phandle;
>> + int ret;
>> +
>> + ret = dt_property_read_variable_u32_array(scmi_node, "shmem", &phandle,
>> + 1, 1);
>> + if ( ret < 0 )
>> + return ret;
>> +
>> + shmem_node = dt_find_node_by_phandle(phandle);
>> + if ( !shmem_node ||
>> + !dt_device_is_compatible(shmem_node, "arm,scmi-shmem") )
>> + return -EINVAL;
>> +
>> + ret = dt_device_get_address(shmem_node, 0, addr, size);
>> + if ( ret )
>> + return ret;
>> +
>> + if ( !*size || !IS_ALIGNED(*size, SCMI_SHMEM_MAPPED_SIZE) ||
>> + !IS_ALIGNED(*addr, SCMI_SHMEM_MAPPED_SIZE) )
>> + {
>> + printk(XENLOG_ERR "scmi: invalid shmem range for %s\n",
>> + scmi_node->full_name);
>> + return -EINVAL;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +/*
>> + * Handle Dom0 SCMI specific DT nodes
>> + *
>> + * Make a decision on copying SCMI specific nodes into Dom0 device tree.
>> + * For SCMI multi-agent case:
>> + * - shmem nodes will not be copied and generated instead if SCMI
>> + * is enabled for Dom0
>> + * - scmi node will be copied if SCMI is enabled for Dom0
>> + */
>> +static bool scmi_dt_handle_node(struct domain *d, struct dt_device_node
>> *node)
> Also __init?
+
>> +{
>> + static const struct dt_device_match shmem_matches[] __initconst = {
>> + DT_MATCH_COMPATIBLE("arm,scmi-shmem"),
>> + { /* sentinel */ },
>> + };
>> + static const struct dt_device_match scmi_matches[] __initconst = {
>> + DT_MATCH_PATH("/firmware/scmi"),
>> + { /* sentinel */ },
>> + };
>> +
>> + if ( !scmi_data.initialized )
>> + return false;
>> +
>> + /* skip scmi shmem node for dom0 if scmi not enabled */
>> + if ( dt_match_node(shmem_matches, node) && !sci_domain_is_enabled(d) )
>> + {
>> + dt_dprintk(" Skip scmi shmem node\n");
>> + return true;
>> + }
>> +
>> + /* drop scmi if not enabled */
>> + if ( dt_match_node(scmi_matches, node) && !sci_domain_is_enabled(d) )
>> + {
>> + dt_dprintk(" Skip scmi node\n");
>> + return true;
>> + }
>> +
>> + return false;
>> +}
>> +
>> +static int scmi_assign_device(uint32_t agent_id, uint32_t device_id,
>> + uint32_t flags)
>> +{
>> + struct scmi_msg_base_set_device_permissions_a2p tx;
>> + struct scmi_channel *channel;
>> + scmi_msg_header_t hdr;
>> +
>> + channel = get_channel_by_id(scmi_data.hyp_channel_agent_id);
>> + if ( !channel )
>> + return -EINVAL;
>> +
>> + hdr.id = SCMI_BASE_SET_DEVICE_PERMISSIONS;
>> + hdr.type = 0;
>> + hdr.protocol = SCMI_BASE_PROTOCOL;
>> +
>> + tx.agent_id = agent_id;
>> + tx.device_id = device_id;
>> + tx.flags = flags;
>> +
>> + return do_smc_xfer(channel, &hdr, &tx, sizeof(tx), NULL, 0);
>> +}
>> +
>> +static int scmi_dt_assign_device(struct domain *d,
>> + struct dt_phandle_args *ac_spec)
>> +{
>> + struct scmi_channel *agent_channel;
>> + uint32_t scmi_device_id = ac_spec->args[0];
>> + int ret;
>> +
>> + if ( !d->arch.sci_data )
>> + return 0;
>> +
>> + /* The access-controllers is specified for DT dev, but it's not a SCMI
>> */
>> + if ( !scmi_data.dt_dev ||
>> + !dt_node_path_is_equal(ac_spec->np, scmi_data.dt_dev->full_name) )
>> + return 0;
>> +
>> + agent_channel = d->arch.sci_data;
>> +
>> + spin_lock(&agent_channel->lock);
>> +
>> + ret = scmi_assign_device(agent_channel->agent_id, scmi_device_id,
>> + SCMI_BASE_DEVICE_ACCESS_ALLOW);
>> + if ( ret )
>> + {
>> + printk(XENLOG_ERR
>> + "scmi: could not assign dev for %pd agent:%d dev_id:%u (%d)",
>> + d, agent_channel->agent_id, scmi_device_id, ret);
>> + }
>> +
>> + spin_unlock(&agent_channel->lock);
>> + return ret;
>> +}
>> +
>> +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 )
> On error, Xen's channel will be mapped still. Is this intended behaviour?
It remains mapped when collect_agent_id() returns, but the existing
caller cleans it up. scmi_probe() marks the management channel DOMID_XEN
before discovery. If discovery fails, it jumps to error, calls
unmap_channel_memory(channel), then free_channel_list(). Thus the current
probe failure path does not leak the mapping.
On successful discovery the management mapping is deliberately retained
for subsequent permission and reset requests. Secondary discovery
mappings are always removed by collect_agent_id(), whether the transfer
succeeds or fails.
>> + return ret;
>> +
>> + 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 int __init collect_agents(struct dt_device_node *scmi_node)
>> +{
>> + struct dt_device_node *agents, *node;
>> + unsigned int count = 0;
>> +
>> + /* Use the configuration containing this management channel. */
>> + for ( agents = scmi_node->parent; agents; agents = agents->parent )
>> + if ( dt_device_is_compatible(agents, "xen,sci") )
>> + break;
>> +
>> + ASSERT(agents);
>> +
>> + dt_for_each_child_node(agents, node)
>> + if ( dt_node_name_is_equal(node, "agents") )
>> + break;
>> +
>> + if ( !node )
>> + {
>> + printk(XENLOG_ERR "scmi: missing agents node\n");
>> + return -EINVAL;
>> + }
>> + agents = node;
>> +
>> + if ( dt_child_n_addr_cells(agents) != 1 ||
>> + dt_child_n_size_cells(agents) != 0 )
>> + return -EINVAL;
>> +
>> + dt_for_each_child_node(agents, node)
>> + {
>> + struct scmi_channel agent = {
>> + .agent_id = SCMI_BASE_AGENT_ID_OWN,
>> + .domain_id = DOMID_INVALID,
>> + };
>> + struct scmi_channel *channel;
>> + bool discover;
>> + uint32_t len;
>> + u64 size;
> uint64_t
>
>
+1
>> + int ret;
>> +
>> + if ( !dt_device_is_compatible(node, "xen,scmi-agent") ||
>> + !dt_device_is_available(node) )
>> + continue;
>> +
>> + discover = !dt_find_property(node, "reg", &len);
>> + if ( !discover )
>> + {
>> + if ( len != sizeof(uint32_t) ||
>> + !dt_property_read_u32(node, "reg", &agent.agent_id) )
>> + return -EINVAL;
>> + }
>> +
>> + if ( !dt_property_read_u32(node, "arm,smc-id", &agent.func_id) )
>> + return -EINVAL;
>> +
>> + ret = scmi_dt_read_channel_addr(node, &agent.paddr, &size);
>> + if ( ret )
>> + return ret;
>> +
>> + if ( discover )
>> + {
>> + /* Resolve the ID before inserting it into the channel list. */
>> + spin_lock_init(&agent.lock);
>> + ret = collect_agent_id(&agent);
>> + if ( ret )
>> + return ret;
>> + }
>> +
>> + /* Guest configurations encode agent IDs in an eight-bit field. */
>> + if ( agent.agent_id >= UINT8_MAX )
>> + return -EINVAL;
>> +
>> + channel = smc_create_channel(agent.agent_id, agent.func_id,
>> + agent.paddr);
>> + if ( IS_ERR(channel) )
>> + {
>> + printk(XENLOG_ERR "scmi: Could not create channel for %s:
>> %ld\n",
>> + node->full_name, PTR_ERR(channel));
>> + return PTR_ERR(channel);
>> + }
>> +
>> + count++;
>> + printk(XENLOG_DEBUG "scmi: Agent %u SMC %X addr %lx\n",
>> + agent.agent_id, agent.func_id, (unsigned long)agent.paddr);
> Why cast to unsigned long?
Was used to match %lx. Using PRIpaddr will drop the need of casing.
>> + }
>> +
>> + return count ? 0 : -EINVAL;
>> +}
>> +
>> +static int scmi_domain_init(struct domain *d,
>> + struct xen_domctl_createdomain *config)
>> +{
>> + struct scmi_channel *channel;
>> + int ret;
>> +
>> + if ( !scmi_data.initialized )
>> + return 0;
>> +
>> + /*
>> + * SCMI support is configured via:
>> + * - For dom0: xen,dom0-sci-agent-id property under the xen,sci
>> container
>> + * - For dom0less: xen,sci-agent-id in the domain node
>> + * The config->arch.arm_sci_type and config->arch.arm_sci_agent_id
>> + * are already set by domain_build.c or dom0less-build.c
>> + */
>> +
>> + if ( config->arch.arm_sci_type == XEN_DOMCTL_CONFIG_ARM_SCI_NONE )
>> + return 0;
>> +
>> + channel = acquire_scmi_channel(d, config->arch.arm_sci_agent_id);
>> + if ( IS_ERR(channel) )
>> + {
>> + printk(XENLOG_ERR
>> + "scmi: Failed to acquire SCMI channel for agent_id %u:
>> %ld\n",
>> + config->arch.arm_sci_agent_id, PTR_ERR(channel));
>> + return PTR_ERR(channel);
>> + }
>> +
>> + printk(XENLOG_INFO
>> + "scmi: Acquire channel id = 0x%x, domain_id = %d paddr =
>> 0x%lx\n",
>> + channel->agent_id, channel->domain_id, channel->paddr);
>> +
>> + /*
>> + * Dom0 (if present) needs to have an access to the guest memory range
>> + * to satisfy iomem_access_permitted() check in
>> XEN_DOMCTL_iomem_permission
>> + * domctl.
>> + */
>> + if ( hardware_domain && !is_hardware_domain(d) )
>> + {
>> + ret = iomem_permit_access(hardware_domain,
>> paddr_to_pfn(channel->paddr),
>> + paddr_to_pfn(channel->paddr +
>> + SCMI_SHMEM_MAPPED_SIZE - 1));
>> + if ( ret )
>> + goto error;
>> + }
>> +
>> + d->arch.sci_data = channel;
>> + d->arch.sci_enabled = true;
>> +
>> + return 0;
>> +
>> +error:
>> + relinquish_scmi_channel(channel);
>> + return ret;
>> +}
>> +
>> +int scmi_domain_sanitise_config(const struct xen_domctl_createdomain
>> *config)
>> +{
>> + if ( config->arch.arm_sci_type != XEN_DOMCTL_CONFIG_ARM_SCI_NONE &&
>> + config->arch.arm_sci_type != XEN_DOMCTL_CONFIG_ARM_SCI_SCMI_SMC_MA
>> )
>> + {
>> + dprintk(XENLOG_INFO, "scmi: Unsupported ARM_SCI type\n");
>> + return -EINVAL;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int scmi_relinquish_resources(struct domain *d)
>> +{
>> + int ret;
>> + struct scmi_channel *channel, *agent_channel;
>> + scmi_msg_header_t hdr;
>> + struct scmi_msg_base_reset_agent_cfg_a2p tx;
>> +
>> + if ( !d->arch.sci_data )
>> + return 0;
>> +
>> + agent_channel = d->arch.sci_data;
>> +
>> + spin_lock(&agent_channel->lock);
>> + tx.agent_id = agent_channel->agent_id;
>> + spin_unlock(&agent_channel->lock);
>> +
>> + channel = get_channel_by_id(scmi_data.hyp_channel_agent_id);
>> + if ( !channel )
>> + {
>> + printk(XENLOG_ERR
>> + "scmi: Unable to get Hypervisor scmi channel for domain
>> %d\n",
>> + d->domain_id);
>> + return -EINVAL;
>> + }
>> +
>> + hdr.id = SCMI_BASE_RESET_AGENT_CONFIGURATION;
>> + hdr.type = 0;
>> + hdr.protocol = SCMI_BASE_PROTOCOL;
>> +
>> + tx.flags = SCMI_BASE_AGENT_PERMISSIONS_RESET;
>> +
>> + ret = do_smc_xfer(channel, &hdr, &tx, sizeof(tx), NULL, 0);
>> + if ( ret == -EOPNOTSUPP )
>> + return 0;
>> +
>> + return ret;
>> +}
>> +
>> +static void scmi_domain_destroy(struct domain *d)
>> +{
>> + struct scmi_channel *channel;
>> +
>> + if ( !d->arch.sci_data )
>> + return;
>> +
>> + channel = d->arch.sci_data;
>> + spin_lock(&channel->lock);
>> +
>> + relinquish_scmi_channel(channel);
>> + printk(XENLOG_DEBUG "scmi: Free domain %d\n", d->domain_id);
> You can use "%pd"
+
>> +
>> + d->arch.sci_data = NULL;
>> + d->arch.sci_enabled = false;
>> +
>> + spin_unlock(&channel->lock);
>> +}
>> +
>> +static bool scmi_handle_call(struct cpu_user_regs *regs)
>> +{
>> + uint32_t fid = (uint32_t)get_user_reg(regs, 0);
>> + struct scmi_channel *agent_channel;
>> + struct domain *d = current->domain;
>> + bool res = false;
>> +
>> + if ( (!sci_domain_is_enabled(d)) || (!d->arch.sci_data) )
>> + return false;
>> +
>> + agent_channel = d->arch.sci_data;
>> + spin_lock(&agent_channel->lock);
>> +
>> + if ( agent_channel->func_id != fid )
>> + {
>> + res = false;
>> + goto unlock;
>> + }
>> +
>> + arm_smccc_guest_smc(regs);
>> + res = true;
>> +unlock:
>> + spin_unlock(&agent_channel->lock);
>> +
>> + return res;
>> +}
>> +
>> +static const struct sci_mediator_ops scmi_ops = {
>> + .domain_init = scmi_domain_init,
>> + .domain_destroy = scmi_domain_destroy,
>> + .relinquish_resources = scmi_relinquish_resources,
>> + .handle_call = scmi_handle_call,
>> + .dom0_dt_handle_node = scmi_dt_handle_node,
>> + .domain_sanitise_config = scmi_domain_sanitise_config,
>> + .assign_dt_device = scmi_dt_assign_device,
>> +};
>> +
>> +static int __init scmi_check_smccc_ver(void)
>> +{
>> + if ( smccc_ver < ARM_SMCCC_VERSION_1_1 )
>> + {
>> + printk(XENLOG_WARNING
>> + "scmi: No SMCCC 1.1 support, SCMI calls forwarding
>> disabled\n");
>> + return -ENOSYS;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int __init scmi_dt_hyp_channel_read(struct dt_device_node *scmi_node,
>> + struct scmi_data *scmi_data,
>> + u64 *addr)
>> +{
>> + u64 size;
>> +
>> + if ( !dt_property_read_u32(scmi_node, "arm,smc-id",
>> &scmi_data->func_id) )
>> + {
>> + printk(XENLOG_ERR "scmi: unable to read smc-id from DT\n");
>> + return -ENOENT;
>> + }
>> +
>> + return scmi_dt_read_channel_addr(scmi_node, addr, &size);
>> +}
>> +
>> +static __init int scmi_probe(struct dt_device_node *scmi_node, const void
>> *data)
>> +{
>> + u64 addr;
>> + int ret;
>> + struct scmi_channel *channel;
>> + unsigned int n_agents;
>> + scmi_msg_header_t hdr;
>> + struct scmi_msg_base_attributes_p2a rx;
>> +
>> + ASSERT(scmi_node != NULL);
>> +
>> + /*
>> + * Only bind to the SCMI node provided by Xen under the xen,sci
>> container
>> + * (e.g. /chosen/xen/xen_scmi_config/scmi). This avoids binding to
>> firmware
>> + * SCMI nodes that belong to the host OSPM and keeps the mediator
>> scoped to
>> + * Xen-provided configuration only.
>> + */
>> + if ( !scmi_is_under_xen_sci(scmi_node) )
>> + return -ENODEV;
>> +
>> + if ( !acpi_disabled )
>> + {
>> + printk(XENLOG_WARNING "scmi: is not supported when using ACPI\n");
>> + return -EINVAL;
>> + }
>> +
>> + ret = scmi_check_smccc_ver();
>> + if ( ret )
>> + return ret;
>> +
>> + ret = scmi_dt_hyp_channel_read(scmi_node, &scmi_data, &addr);
>> + if ( ret )
>> + return ret;
>> +
>> + INIT_LIST_HEAD(&scmi_data.channel_list);
>> + spin_lock_init(&scmi_data.channel_list_lock);
>> +
>> + scmi_data.dt_dev = scmi_node;
>> +
>> + channel = smc_create_channel(SCMI_BASE_AGENT_ID_OWN, scmi_data.func_id,
>> addr);
>> + if ( IS_ERR(channel) )
>> + {
>> + ret = PTR_ERR(channel);
>> + goto out;
>> + }
>> +
>> + /* Mark as Xen management channel before collecting agent ID */
>> + channel->domain_id = DOMID_XEN;
>> +
>> + /* Request agent id for Xen management channel */
>> + ret = collect_agent_id(channel);
>> + if ( ret )
>> + goto error;
>> +
>> + /* Save the agent id for Xen management channel */
>> + scmi_data.hyp_channel_agent_id = channel->agent_id;
>> +
>> + hdr.id = SCMI_BASE_PROTOCOL_ATTIBUTES;
>> + hdr.type = 0;
>> + hdr.protocol = SCMI_BASE_PROTOCOL;
>> +
>> + ret = do_smc_xfer(channel, &hdr, NULL, 0, &rx, sizeof(rx));
>> + if ( ret )
>> + goto error;
>> +
>> + n_agents = SCMI_FIELD_GET(SCMI_BASE_ATTR_NUM_AGENT, rx.attributes);
>> + printk(XENLOG_DEBUG "scmi: Got agent count %d\n", n_agents);
>> + ret = collect_agents(scmi_node);
>> + if ( ret )
>> + goto error;
>> +
>> + ret = sci_register(&scmi_ops);
>> + if ( ret )
>> + {
>> + printk(XENLOG_ERR "SCMI: mediator already registered (ret = %d)\n",
>> + ret);
>> + goto error;
>> + }
>> +
>> + scmi_data.initialized = true;
>> + goto out;
>> +
>> +error:
>> + unmap_channel_memory(channel);
>> + free_channel_list();
>> +out:
>> + return ret;
>> +}
>> +
>> +static const struct dt_device_match scmi_smc_match[] __initconst = {
>> + DT_MATCH_COMPATIBLE("arm,scmi-smc"),
>> + { /* sentinel */ },
>> +};
>> +
>> +DT_DEVICE_START(scmi_smc_ma, "SCMI SMC MEDIATOR", DEVICE_FIRMWARE)
>> + .dt_match = scmi_smc_match,
>> + .init = scmi_probe,
>> +DT_DEVICE_END
>> +
>> +/*
>> + * Local variables:
>> + * mode: C
>> + * c-file-style: "BSD"
>> + * c-basic-offset: 4
>> + * tab-width: 4
>> + * indent-tabs-mode: nil
>> + * End:
>> + */
>> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
>> index 14d51e0746..722c2d2e3a 100644
>> --- a/xen/include/public/arch-arm.h
>> +++ b/xen/include/public/arch-arm.h
>> @@ -329,6 +329,7 @@ DEFINE_XEN_GUEST_HANDLE(vcpu_guest_context_t);
>>
>> #define XEN_DOMCTL_CONFIG_ARM_SCI_NONE 0
>> #define XEN_DOMCTL_CONFIG_ARM_SCI_SCMI_SMC 1
>> +#define XEN_DOMCTL_CONFIG_ARM_SCI_SCMI_SMC_MA 2
>>
>> #define XEN_DOMCTL_CONFIG_ARM_V8R_EL1_MSA_NONE 0
>> #define XEN_DOMCTL_CONFIG_ARM_V8R_EL1_MSA_PMSA 1
>> @@ -347,7 +348,9 @@ struct xen_arch_domainconfig {
>> uint8_t arm_sci_type;
>> /* IN */
>> uint8_t v8r_el1_msa;
>> - uint16_t pad;
>> + /* IN */
>> + uint8_t arm_sci_agent_id;
>> + uint8_t pad;
>> };
>> #endif /* __XEN__ || __XEN_TOOLS__ */
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |