|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [RFC v1 3/5] xen/arm: introduce SCMI-SMC mediator driver
On Tue, 14 Dec 2021, Oleksii Moisieiev wrote:
> This is the implementation of SCI interface, called SCMI-SMC driver,
> which works as the mediator between XEN Domains and Firmware (SCP, ATF etc).
> This allows devices from the Domains to work with clocks, resets and
> power-domains without access to CPG.
>
> The following features are implemented:
> - request SCMI channels from ATF and pass channels to Domains;
> - set device permissions for Domains based on the Domain partial
> device-tree. Devices with permissions are able to work with clocks,
> resets and power-domains via SCMI;
> - redirect scmi messages from Domains to ATF.
>
> Signed-off-by: Oleksii Moisieiev <oleksii_moisieiev@xxxxxxxx>
> ---
> xen/arch/arm/Kconfig | 2 +
> xen/arch/arm/sci/Kconfig | 10 +
> xen/arch/arm/sci/Makefile | 1 +
> xen/arch/arm/sci/scmi_smc.c | 795 ++++++++++++++++++++++++++++++++++
> xen/include/public/arch-arm.h | 1 +
> 5 files changed, 809 insertions(+)
> create mode 100644 xen/arch/arm/sci/Kconfig
> create mode 100644 xen/arch/arm/sci/scmi_smc.c
>
> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
> index 186e1db389..02d96c6cfc 100644
> --- a/xen/arch/arm/Kconfig
> +++ b/xen/arch/arm/Kconfig
> @@ -114,6 +114,8 @@ config SCI
> support. It allows guests to control system resourcess via one of
> SCI mediators implemented in XEN.
>
> +source "arch/arm/sci/Kconfig"
> +
> endmenu
>
> menu "ARM errata workaround via the alternative framework"
> diff --git a/xen/arch/arm/sci/Kconfig b/xen/arch/arm/sci/Kconfig
> new file mode 100644
> index 0000000000..9563067ddc
> --- /dev/null
> +++ b/xen/arch/arm/sci/Kconfig
> @@ -0,0 +1,10 @@
> +config SCMI_SMC
> + bool "Enable SCMI-SMC mediator driver"
> + default n
> + depends on SCI
> + ---help---
> +
> + Enables mediator in XEN to pass SCMI requests from Domains to ATF.
> + This feature allows drivers from Domains to work with System
> + Controllers (such as power,resets,clock etc.). SCP is used as transport
> + for communication.
> diff --git a/xen/arch/arm/sci/Makefile b/xen/arch/arm/sci/Makefile
> index 837dc7492b..67f2611872 100644
> --- a/xen/arch/arm/sci/Makefile
> +++ b/xen/arch/arm/sci/Makefile
> @@ -1 +1,2 @@
> obj-y += sci.o
> +obj-$(CONFIG_SCMI_SMC) += scmi_smc.o
> diff --git a/xen/arch/arm/sci/scmi_smc.c b/xen/arch/arm/sci/scmi_smc.c
> new file mode 100644
> index 0000000000..2eb01ea82d
> --- /dev/null
> +++ b/xen/arch/arm/sci/scmi_smc.c
> @@ -0,0 +1,795 @@
> +/*
> + * xen/arch/arm/sci/scmi_smc.c
> + *
> + * SCMI mediator driver, using SCP as transport.
> + *
> + * Oleksii Moisieiev <oleksii_moisieiev@xxxxxxxx>
> + * Copyright (C) 2021, EPAM Systems.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <asm/sci/sci.h>
> +#include <asm/smccc.h>
> +#include <asm/io.h>
> +#include <xen/bitops.h>
> +#include <xen/config.h>
> +#include <xen/sched.h>
> +#include <xen/device_tree.h>
> +#include <xen/iocap.h>
> +#include <xen/init.h>
> +#include <xen/err.h>
> +#include <xen/lib.h>
> +#include <xen/list.h>
> +#include <xen/mm.h>
> +#include <xen/string.h>
> +#include <xen/time.h>
> +#include <xen/vmap.h>
> +
> +#define SCMI_BASE_PROTOCOL 0x10
> +#define SCMI_BASE_PROTOCOL_ATTIBUTES 0x1
> +#define SCMI_BASE_SET_DEVICE_PERMISSIONS 0x9
> +#define SCMI_BASE_RESET_AGENT_CONFIGURATION 0xB
> +#define SCMI_BASE_DISCOVER_AGENT 0x7
> +
> +/* SCMI return codes. See section 4.1.4 of SCMI spec (DEN0056C) */
> +#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)
> +
> +#define DT_MATCH_SCMI_SMC DT_MATCH_COMPATIBLE("arm,scmi-smc")
> +
> +#define SCMI_SMC_ID "arm,smc-id"
> +#define SCMI_SHARED_MEMORY "linux,scmi_mem"
I could find the following SCMI binding in Linux, which describes
the arm,scmi-smc compatible and the arm,smc-id property:
Documentation/devicetree/bindings/firmware/arm,scmi.yaml
However, linux,scmi_mem is not described. Aren't you supposed to read
the "shmem" property instead? And the compatible string used for this
seems to be "arm,scmi-shmem".
> +#define SCMI_SHMEM "shmem"
> +
> +#define HYP_CHANNEL 0x0
> +
> +#define HDR_ID GENMASK(7,0)
> +#define HDR_TYPE GENMASK(9, 8)
> +#define HDR_PROTO GENMASK(17, 10)
> +
> +/* SCMI protocol, refer to section 4.2.2.2 (DEN0056C) */
> +#define MSG_N_AGENTS_MASK GENMASK(15, 8)
> +
> +#define FIELD_GET(_mask, _reg)\
> + ((typeof(_mask))(((_reg) & (_mask)) >> (ffs64(_mask) - 1)))
> +#define FIELD_PREP(_mask, _val)\
> + (((typeof(_mask))(_val) << (ffs64(_mask) - 1)) & (_mask))
> +
> +typedef struct scmi_msg_header {
> + uint8_t id;
> + uint8_t type;
> + uint8_t protocol;
> +} scmi_msg_header_t;
> +
> +typedef struct scmi_perms_tx {
> + uint32_t agent_id;
> + uint32_t device_id;
> + uint32_t flags;
> +} scmi_perms_tx_t;
> +
> +#define SCMI_SHMEM_CHAN_STAT_CHANNEL_FREE BIT(0, UL)
> +#define SCMI_SHMEM_CHAN_STAT_CHANNEL_ERROR BIT(1, UL)
> +
> +#define SCMI_ALLOW_ACCESS BIT(0, 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[];
> +};
> +
> +struct scmi_channel {
> + int chan_id;
> + int agent_id;
> + uint32_t func_id;
> + int domain_id;
> + uint64_t paddr;
> + struct scmi_shared_mem *shmem;
> + spinlock_t lock;
> + struct list_head list;
> +};
> +
> +struct scmi_data {
> + struct list_head channel_list;
> + spinlock_t channel_list_lock;
> + bool initialized;
> + u64 shmem_addr, shmem_size;
> +};
> +
> +static struct scmi_data scmi_data;
> +
> +/*
> + * pack_scmi_header() - packs and returns 32-bit header
> + *
> + * @hdr: pointer to header containing all the information on message id,
> + * protocol id and type id.
> + *
> + * Return: 32-bit packed message header to be sent to the platform.
> + */
> +static inline uint32_t pack_scmi_header(scmi_msg_header_t *hdr)
> +{
> + return FIELD_PREP(HDR_ID, hdr->id) |
> + FIELD_PREP(HDR_TYPE, hdr->type) |
> + FIELD_PREP(HDR_PROTO, hdr->protocol);
> +}
> +
> +/*
> + * unpack_scmi_header() - unpacks and records message and protocol id
> + *
> + * @msg_hdr: 32-bit packed message header sent from the platform
> + * @hdr: pointer to header to fetch message and protocol id.
> + */
> +static inline void unpack_scmi_header(uint32_t msg_hdr, scmi_msg_header_t
> *hdr)
> +{
> + hdr->id = FIELD_GET(HDR_ID, msg_hdr);
> + hdr->type = FIELD_GET(HDR_TYPE, msg_hdr);
> + hdr->protocol = FIELD_GET(HDR_PROTO, msg_hdr);
> +}
> +
> +static inline int channel_is_free(struct scmi_channel *chan_info)
> +{
> + return ( chan_info->shmem->channel_status
> + & SCMI_SHMEM_CHAN_STAT_CHANNEL_FREE ) ? 0 : -EBUSY;
Does this need a memory barrier? Or not, because the other end always
runs on the same CPU at a different execution level so the
channel_status would be always guaranteed to be read as updated?
> +}
> +
> +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;
> +
> + printk(XENLOG_DEBUG "scmi: status =%d len=%d\n",
> + chan_info->shmem->channel_status, len);
> + printk(XENLOG_DEBUG "scmi: header id = %d type = %d, proto = %d\n",
> + hdr->id, hdr->type, hdr->protocol);
> +
> + ret = channel_is_free(chan_info);
> + if ( IS_ERR_VALUE(ret) )
> + return ret;
> +
> + chan_info->shmem->channel_status = 0x0;
> + /* Writing 0x0 right now, but SCMI_SHMEM_FLAG_INTR_ENABLED can be set */
> + chan_info->shmem->flags = 0x0;
> + chan_info->shmem->length = sizeof(chan_info->shmem->msg_header) + len;
> + chan_info->shmem->msg_header = pack_scmi_header(hdr);
> +
> + printk(XENLOG_DEBUG "scmi: Writing to shmem address %p\n",
> + chan_info->shmem);
> + if ( len > 0 && data )
> + memcpy((void *)(chan_info->shmem->msg_payload), data, len);
Again, here we don't need a barrier because it is implicit in the SMC?
Don't we need to check that "len" fits in the shared memory?
> + arm_smccc_smc(chan_info->func_id, 0, 0, 0, 0, 0, 0, chan_info->chan_id,
> + &resp);
> +
> + printk(XENLOG_DEBUG "scmi: scmccc_smc response %d\n", (int)(resp.a0));
> +
> + if ( resp.a0 )
> + return -EOPNOTSUPP;
Why is that?
> + return 0;
> +}
> +
> +static int check_scmi_status(int scmi_status)
> +{
> + if ( scmi_status == SCMI_SUCCESS )
> + return 0;
> +
> + printk(XENLOG_DEBUG "scmi: Error received: %d\n", scmi_status);
> +
> + switch ( scmi_status )
> + {
> + case SCMI_NOT_SUPPORTED:
> + return -EOPNOTSUPP;
> + case SCMI_INVALID_PARAMETERS:
> + return -EINVAL;
> + case SCMI_DENIED:
> + return -EACCES;
> + case SCMI_NOT_FOUND:
> + return -ENOENT;
> + case SCMI_OUT_OF_RANGE:
> + return -ERANGE;
> + case SCMI_BUSY:
> + return -EBUSY;
> + case SCMI_COMMS_ERROR:
> + return -ENOTCONN;
> + case SCMI_GENERIC_ERROR:
> + return -EIO;
> + case SCMI_HARDWARE_ERROR:
> + return -ENXIO;
> + case SCMI_PROTOCOL_ERROR:
> + return -EBADMSG;
> + }
> +
> + return -EINVAL;
> +}
> +
> +static int get_smc_response(struct scmi_channel *chan_info,
> + scmi_msg_header_t *hdr, void *data, int len)
> +{
> + int recv_len;
> + int ret;
> +
> + printk(XENLOG_DEBUG "scmi: get smc responce msgid %d\n", hdr->id);
> +
> + ret = channel_is_free(chan_info);
> + if ( IS_ERR_VALUE(ret) )
> + return ret;
I am not familiar with the spec (do you have a link?) but is it expected
that the channel is "free" when actually we want to read a message on
the channel?
> + recv_len = chan_info->shmem->length -
> sizeof(chan_info->shmem->msg_header);
> +
> + if ( recv_len < 0 )
> + {
> + printk(XENLOG_ERR
> + "scmi: Wrong size of smc message. Data may be invalid\n");
> + return -EINVAL;
> + }
> +
> + if ( recv_len > len )
> + {
> + printk(XENLOG_ERR
> + "scmi: Not enough buffer for message %d, expecting %d\n",
> + recv_len, len);
> + return -EINVAL;
> + }
> +
> + unpack_scmi_header(chan_info->shmem->msg_header, hdr);
> +
> + if ( recv_len > 0 )
> + {
> + memcpy(data, chan_info->shmem->msg_payload, recv_len);
> + }
> +
> + return 0;
> +}
> +
> +static int do_smc_xfer(struct scmi_channel *channel, scmi_msg_header_t *hdr,
> void *tx_data, int tx_size,
> + void *rx_data, int rx_size)
> +{
> + int ret = 0;
> +
> + if ( !hdr )
> + return -EINVAL;
> +
> + spin_lock(&channel->lock);
> +
> + ret = send_smc_message(channel, hdr, tx_data, tx_size);
> + if ( ret )
> + goto clean;
> +
> + ret = get_smc_response(channel, hdr, rx_data, rx_size);
> +clean:
> + spin_unlock(&channel->lock);
> +
> + return ret;
> +}
> +
> +static struct scmi_channel *get_channel_by_id(uint8_t chan_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)
please use parenthesis around the inner if (also in other places)
> + if ( curr->chan_id == chan_id )
> + {
> + found = true;
> + break;
> + }
> +
> + spin_unlock(&scmi_data.channel_list_lock);
> + if ( found )
> + return curr;
> +
> + return NULL;
> +}
> +
> +static struct scmi_channel *get_channel_by_domain(uint8_t domain_id)
Use domid_t for domain ids.
Also, wouldn't it be better to implement it as:
static inline struct scmi_channel *get_channel_by_domain(struct domain *d) {
return d->arch.sci
}
> +{
> + 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->domain_id == domain_id )
> + {
> + found = true;
> + break;
> + }
> +
> + spin_unlock(&scmi_data.channel_list_lock);
> + if ( found )
> + return curr;
> +
> + return NULL;
> +}
> +
> +static struct scmi_channel *aquire_scmi_channel(int domain_id)
> +{
> + struct scmi_channel *curr;
> + bool found = false;
> +
> + ASSERT(domain_id != DOMID_INVALID && domain_id >= 0);
> +
> + spin_lock(&scmi_data.channel_list_lock);
> + list_for_each_entry(curr, &scmi_data.channel_list, list)
> + if ( (curr->domain_id == DOMID_INVALID)
> + && (curr->chan_id != HYP_CHANNEL) )
If you use DOMID_XEN for HYP_CHANNEL, then this check becomes more
intuitive
> + {
> + curr->domain_id = domain_id;
> + found = true;
> + break;
> + }
> +
> + spin_unlock(&scmi_data.channel_list_lock);
> + if ( found )
> + return curr;
> +
> + return NULL;
> +}
> +
> +static void relinquish_scmi_channel(struct scmi_channel *channel)
> +{
> + spin_lock(&scmi_data.channel_list_lock);
> + ASSERT(channel != NULL);
the ASSERT could be before the spin_lock
> + channel->domain_id = DOMID_INVALID;
> + spin_unlock(&scmi_data.channel_list_lock);
> +}
> +
> +static struct scmi_channel *smc_create_channel(uint8_t chan_id,
> + uint32_t func_id, uint64_t
> addr)
> +{
> + struct scmi_channel *channel;
> + mfn_t mfn;
> +
> + channel = get_channel_by_id(chan_id);
> + if ( channel )
> + return ERR_PTR(EEXIST);
> +
> + channel = xmalloc(struct scmi_channel);
> + if ( !channel )
> + return ERR_PTR(ENOMEM);
> +
> + channel->chan_id = chan_id;
> + channel->func_id = func_id;
> + channel->domain_id = DOMID_INVALID;
I take you are using DOMID_INVALID to mark a channel used by Xen itself?
If so, then DOMID_XEN would be more appropriate.
> + mfn = maddr_to_mfn(addr);
> + channel->shmem = vmap(&mfn, 1);
One thing to be careful is the mapping attributes, for a couple of
reasons. As you might be aware, the ARM architecture forbids mismatching
attributes for mapping memory in different places in the system. So the
attributes that we use here must be the same used by the firmware
(and/or the guest.)
The second reason to be careful is that in the bindings example
Documentation/devicetree/bindings/firmware/arm,scmi.yaml the shared
memory is "mmio-sram", which is special. It is not supposed to be normal
memory, but it is OK to map it cacheable. Still, it might be more
appropriate to use ioremap_cache.
> + if ( !channel->shmem )
> + {
> + xfree(channel);
> + return ERR_PTR(ENOMEM);
> + }
> +
> + printk(XENLOG_DEBUG "scmi: Got shmem after vmap %p\n", channel->shmem);
> + channel->paddr = addr;
> + channel->shmem->channel_status = SCMI_SHMEM_CHAN_STAT_CHANNEL_FREE;
> + spin_lock_init(&channel->lock);
> + spin_lock(&scmi_data.channel_list_lock);
> + list_add(&channel->list, &scmi_data.channel_list);
> + spin_unlock(&scmi_data.channel_list_lock);
> + return channel;
> +}
> +
> +static int map_memory_to_domain(struct domain *d, uint64_t addr, uint64_t
> len)
> +{
> + return iomem_permit_access(d, paddr_to_pfn(addr),
> + paddr_to_pfn(PAGE_ALIGN(addr + len -1)));
> +}
> +
> +static int unmap_memory_from_domain(struct domain *d, uint64_t addr,
> + uint64_t len)
> +{
> + return iomem_deny_access(d, paddr_to_pfn(addr),
> + paddr_to_pfn(PAGE_ALIGN(addr + len -1)));
> +}
> +
> +static int dt_update_domain_range(struct domain *d, uint64_t addr,
> + uint64_t size)
> +{
> + struct dt_device_node *shmem_node;
> + __be32 *hw_reg;
> + const struct dt_property *pp;
> + uint32_t len;
> +
> + shmem_node = dt_find_compatible_node(NULL, NULL, "arm,scmi-shmem");
Here we are using "arm,scmi-shmem" while below we are checking for
"linux,scmi_mem". What's the difference?
Also, this function is looking for "arm,scmi-shmem" in dt_host and
replaces its value. For dom0less domUs we'll probably need a
make_scmi_node function to create the node from scratch like for
instance xen/arch/arm/domain_build.c:make_gic_domU_node.
I wonder if we had such a function whether it wouldn't be better to also
use it for dom0 (and blacklist the physical "arm,scmi-shmem" in
handle_node so that dom0 doesn't get the real shared memory information
by accident).
> +
> + if ( !shmem_node )
> + {
> + printk(XENLOG_ERR "scmi: Unable to find %s node in DT\n",
> SCMI_SHMEM);
> + return -EINVAL;
> + }
> +
> + pp = dt_find_property(shmem_node, "reg", &len);
> + if ( !pp )
> + {
> + printk(XENLOG_ERR "scmi: Unable to find regs entry in shmem node\n");
> + return -ENOENT;
> + }
> +
> + hw_reg = pp->value;
> + dt_set_range(&hw_reg, shmem_node, addr, size);
> +
> + return 0;
> +}
> +
> +static void free_channel_list(void)
> +{
> + struct scmi_channel *curr, *_curr;
> +
> + spin_lock(&scmi_data.channel_list_lock);
> + list_for_each_entry_safe (curr, _curr, &scmi_data.channel_list, list)
> + {
> + vunmap(curr->shmem);
> + list_del(&curr->list);
> + xfree(curr);
> + }
> +
> + spin_unlock(&scmi_data.channel_list_lock);
> +}
> +
> +static __init bool scmi_probe(struct dt_device_node *scmi_node)
> +{
> + struct dt_device_node *shmem_node;
> + int ret, i;
> + struct scmi_channel *channel, *agent_channel;
> + int n_agents;
> + scmi_msg_header_t hdr;
> + struct rx_t {
> + int32_t status;
> + uint32_t attributes;
> + } rx;
Should rx be defined at the top together with scmi_perms_tx_t and
others?
> + uint32_t func_id;
> +
> + ASSERT(scmi_node != NULL);
> +
> + INIT_LIST_HEAD(&scmi_data.channel_list);
> + spin_lock_init(&scmi_data.channel_list_lock);
> +
> + if ( !dt_property_read_u32(scmi_node, SCMI_SMC_ID, &func_id) )
> + {
> + printk(XENLOG_ERR "scmi: Unable to read smc-id from DT\n");
> + return false;
> + }
> +
> + shmem_node = dt_find_node_by_name(NULL, SCMI_SHARED_MEMORY);
>From the spec, it looks like you should be getting the shared memory
area from the phandle list "shmem".
> + if ( IS_ERR_OR_NULL(shmem_node) )
> + {
> + printk(XENLOG_ERR
> + "scmi: Device tree error, can't parse shmem phandle %ld\n",
> + PTR_ERR(shmem_node));
> + return false;
> + }
> +
> + ret = dt_device_get_address(shmem_node, 0, &scmi_data.shmem_addr,
> + &scmi_data.shmem_size);
> + if ( IS_ERR_VALUE(ret) )
> + return false;
> +
> + channel = smc_create_channel(HYP_CHANNEL, func_id, scmi_data.shmem_addr);
> + if ( IS_ERR(channel) )
> + return false;
> +
> + 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 clean;
> +
> + ret = check_scmi_status(rx.status);
> + if ( ret )
> + goto clean;
> +
> + n_agents = FIELD_GET(MSG_N_AGENTS_MASK, rx.attributes);
> + printk(XENLOG_DEBUG "scmi: Got agent count %d\n", n_agents);
> +
> + n_agents = (n_agents > scmi_data.shmem_size / PAGE_SIZE) ?
> + scmi_data.shmem_size / PAGE_SIZE : n_agents;
> +
> + for ( i = 1; i < n_agents; i++ )
> + {
Given that HYP_CHANNEL is actually zero, it looks like we could do
everything here in this loop but starting from i=0?
> + uint32_t tx_agent_id = 0xFFFFFFFF;
> + struct {
> + int32_t status;
> + uint32_t agent_id;
> + char name[16];
> + } da_rx;
> +
> + agent_channel = smc_create_channel(i, func_id, scmi_data.shmem_addr +
> + i * PAGE_SIZE);
> + if ( IS_ERR(agent_channel) )
> + {
> + ret = PTR_ERR(agent_channel);
> + goto clean;
> + }
> +
> + hdr.id = SCMI_BASE_DISCOVER_AGENT;
> + hdr.type = 0;
> + hdr.protocol = SCMI_BASE_PROTOCOL;
> +
> + ret = do_smc_xfer(agent_channel, &hdr, &tx_agent_id,
> + sizeof(tx_agent_id), &da_rx, sizeof(da_rx));
> + if ( ret )
> + goto clean;
> +
> + ret = check_scmi_status(da_rx.status);
> + if ( ret )
> + goto clean;
> +
> + printk(XENLOG_DEBUG "scmi: status=0x%x id=0x%x name=%s\n",
> + da_rx.status, da_rx.agent_id, da_rx.name);
> +
> + agent_channel->agent_id = da_rx.agent_id;
> + }
> +
> + scmi_data.initialized = true;
> + return true;
> +
> +clean:
> + free_channel_list();
> + return ret == 0;
> +}
> +
> +static int scmi_domain_init(struct domain *d)
> +{
> + struct scmi_channel *channel;
> + int ret;
> +
> + if ( !scmi_data.initialized )
> + return 0;
> +
> + channel = aquire_scmi_channel(d->domain_id);
> + if ( IS_ERR_OR_NULL(channel) )
> + return -ENOENT;
> +
> + printk(XENLOG_INFO "scmi: Aquire SCMI channel id = 0x%x , domain_id = %d"
> + "paddr = 0x%lx\n", channel->chan_id, channel->domain_id,
> + channel->paddr);
> +
> + if ( is_hardware_domain(d) )
> + {
> + ret = map_memory_to_domain(d, scmi_data.shmem_addr,
> + scmi_data.shmem_size);
> + if ( IS_ERR_VALUE(ret) )
> + goto error;
> +
> + ret = dt_update_domain_range(d, channel->paddr, PAGE_SIZE);
> + if ( IS_ERR_VALUE(ret) )
> + {
> + int rc = unmap_memory_from_domain(d, scmi_data.shmem_addr,
> + scmi_data.shmem_size);
> + if ( rc )
> + printk(XENLOG_ERR "Unable to unmap_memory_from_domain\n");
> +
> + goto error;
> + }
> + }
Is dom0 the only domain to get direct access to the shared memory
region? If so, I don't think it is a good idea to make Dom0 "special" in
this case.
Let me make an example: if we assign a device to a domU since boot, and
dom0 wants to change the frequency of a clock that affects the assigned
device (likely because it doesn't know it is assigned), then dom0
shouldn't be able to. We might have to perform checks in Xen to make
sure dom0 cannot stop the clock for the assigned device.
So I think it would be better if all domains are treated the same way in
the mediator unless really necessary.
On the other hand, if all domains get access to the shared memory
region, then I don't think this is likely the right place to create the
dom0 mapping. We probably want to do it in domain_build.c in a way that
can be reused for dom0less domUs.
In regards to shared memory: it looks like the only two functions to
access the real shared memory are send_smc_message and get_smc_response.
If that is the case, then we actually don't need to expose the real
shared memory to any of the domains.
We could simply:
- expose a regular normal memory region as dom0/domU channel memory
- on SMC trap, read from the "fake" shared memory and set the
corresponding real shared memory on the appropriate channel
- issue the SMC call
- on return from SMC, copy over data from the real shared memory to the
"fake" channel reagion
This is useful if we need to "filter" any of the SCMI commands and
options from the domains to the firmware, and also it is useful if the
channel memory is not page aligned. But if the permissions are
fine-grained enough and also the channel memory is page aligned (and
multiple of 4K in size) then we could map the memory.
> +
> + d->arch.sci = channel;
> +
> + return 0;
> +error:
> + relinquish_scmi_channel(channel);
> +
> + return ret;
> +}
> +
> +static int scmi_add_device_by_devid(struct domain *d, uint32_t scmi_devid)
> +{
> + struct scmi_channel *channel, *agent_channel;
> + scmi_msg_header_t hdr;
> + scmi_perms_tx_t tx;
> + struct rx_t {
> + int32_t status;
> + uint32_t attributes;
> + } rx;
> + int ret;
> +
> + if ( !scmi_data.initialized )
> + return 0;
> +
> + printk(XENLOG_DEBUG "scmi: scmi_devid = %d\n", scmi_devid);
> +
> + agent_channel = get_channel_by_domain(d->domain_id);
> + if ( IS_ERR_OR_NULL(agent_channel) )
> + return PTR_ERR(agent_channel);
> +
> + channel = get_channel_by_id(HYP_CHANNEL);
> + if ( IS_ERR_OR_NULL(channel) )
> + return PTR_ERR(channel);
> +
> + hdr.id = SCMI_BASE_SET_DEVICE_PERMISSIONS;
> + hdr.type = 0;
> + hdr.protocol = SCMI_BASE_PROTOCOL;
> +
> + tx.agent_id = agent_channel->agent_id;
> + tx.device_id = scmi_devid;
> + tx.flags = SCMI_ALLOW_ACCESS;
> +
> + ret = do_smc_xfer(channel, &hdr, &tx, sizeof(tx), &rx, sizeof(&rx));
> + if ( IS_ERR_VALUE(ret) )
> + return ret;
> +
> + ret = check_scmi_status(rx.status);
> + if ( IS_ERR_VALUE(ret) )
> + return ret;
> +
> + return 0;
> +}
> +
> +static int scmi_add_dt_device(struct domain *d, struct dt_device_node *dev)
> +{
> + uint32_t scmi_devid;
> +
> + if ( (!scmi_data.initialized) || (!d->arch.sci) )
> + return 0;
> +
> + if ( !dt_property_read_u32(dev, "scmi_devid", &scmi_devid) )
> + return 0;
scmi_devid is another property that is not documented in the binding.
> + printk(XENLOG_INFO "scmi: dt_node = %s\n", dt_node_full_name(dev));
> +
> + return scmi_add_device_by_devid(d, scmi_devid);
> +}
> +
> +static int scmi_relinquish_resources(struct domain *d)
> +{
> + int ret;
> + struct scmi_channel *channel, *agent_channel;
> + scmi_msg_header_t hdr;
> + struct reset_agent_tx {
> + uint32_t agent_id;
> + uint32_t flags;
> + } tx;
> + uint32_t rx;
> +
> + if ( !d->arch.sci )
> + return 0;
> +
> + agent_channel = d->arch.sci;
> +
> + spin_lock(&agent_channel->lock);
> + tx.agent_id = agent_channel->agent_id;
> + spin_unlock(&agent_channel->lock);
> +
> + channel = get_channel_by_id(HYP_CHANNEL);
> + 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 = 0;
> +
> + ret = do_smc_xfer(channel, &hdr, &tx, sizeof(tx), &rx, sizeof(rx));
> + if ( ret )
> + return ret;
> +
> + ret = check_scmi_status(rx);
> +
> + return ret;
> +}
> +
> +static void scmi_domain_destroy(struct domain *d)
> +{
> + struct scmi_channel *channel;
> +
> + if ( !d->arch.sci )
> + return;
> +
> + channel = d->arch.sci;
> + spin_lock(&channel->lock);
> +
> + relinquish_scmi_channel(channel);
> + printk(XENLOG_DEBUG "scmi: Free domain %d\n", d->domain_id);
> +
> + d->arch.sci = NULL;
> +
> + unmap_memory_from_domain(d, channel->paddr, PAGE_SIZE);
> + spin_unlock(&channel->lock);
> + return;
> +}
> +
> +static bool scmi_handle_call(struct domain *d, void *args)
> +{
> + bool res = false;
> + struct scmi_channel *agent_channel;
> + struct arm_smccc_res resp;
> + struct cpu_user_regs *regs = args;
> +
> + if ( !d->arch.sci )
> + return false;
> +
> + agent_channel = d->arch.sci;
> + spin_lock(&agent_channel->lock);
> +
> + if ( agent_channel->func_id != regs->x0 )
> + {
> + printk(XENLOG_ERR "scmi: func_id mismatch, exiting\n");
> + goto unlock;
> + }
> +
> + arm_smccc_smc(agent_channel->func_id, 0, 0, 0, 0, 0, 0,
> + agent_channel->chan_id, &resp);
> +
> + set_user_reg(regs, 0, resp.a0);
> + set_user_reg(regs, 1, resp.a1);
> + set_user_reg(regs, 2, resp.a2);
> + set_user_reg(regs, 3, resp.a3);
> + res = true;
> +unlock:
> + spin_unlock(&agent_channel->lock);
> +
> + return res;
> +}
> +
> +static int scmi_get_channel_paddr(void *scmi_ops,
> + struct xen_arch_domainconfig *config)
> +{
> + struct scmi_channel *agent_channel = scmi_ops;
> +
> + if ( !agent_channel )
> + return -EINVAL;
> +
> + config->sci_agent_paddr = agent_channel->paddr;
> + return 0;
> +}
I am still not sure why it couldn't be done by scmi_domain_init.
> +static const struct dt_device_match scmi_smc_match[] __initconst =
> +{
> + DT_MATCH_SCMI_SMC,
> + { /* sentinel */ },
> +};
> +
> +static const struct sci_mediator_ops scmi_ops =
> +{
> + .probe = scmi_probe,
> + .domain_init = scmi_domain_init,
> + .domain_destroy = scmi_domain_destroy,
> + .add_dt_device = scmi_add_dt_device,
> + .relinquish_resources = scmi_relinquish_resources,
> + .handle_call = scmi_handle_call,
> + .get_channel_info = scmi_get_channel_paddr
> +};
> +
> +REGISTER_SCI_MEDIATOR(scmi_smc, "SCMI-SMC", XEN_DOMCTL_CONFIG_SCI_SCMI_SMC,
> + scmi_smc_match, &scmi_ops);
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
> index 9180be5e86..a67237942d 100644
> --- a/xen/include/public/arch-arm.h
> +++ b/xen/include/public/arch-arm.h
> @@ -315,6 +315,7 @@ DEFINE_XEN_GUEST_HANDLE(vcpu_guest_context_t);
> #define XEN_DOMCTL_CONFIG_TEE_OPTEE 1
>
> #define XEN_DOMCTL_CONFIG_SCI_NONE 0
> +#define XEN_DOMCTL_CONFIG_SCI_SCMI_SMC 1
>
> struct xen_arch_domainconfig {
> /* IN/OUT */
> --
> 2.27.0
>
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |