|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [RFC v1 3/5] xen/arm: introduce SCMI-SMC mediator driver
Hi Oleksandr,
On Fri, Dec 17, 2021 at 01:35:35PM +0200, Oleksandr wrote:
>
> On 14.12.21 11:34, Oleksii Moisieiev wrote:
>
>
> Hi Oleksii
>
> > 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"
> > +#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;
> > +}
> > +
> > +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);
> > +
> > + 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;
> > +
> > + 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;
>
> Probably we should add a default: here?
>
Thanks, I will add the default, so fucntion will look like this:
static int check_scmi_status(int scmi_status)
{
...
switch ( scmi_status )
{
...
default:
return -EINVAL;
}
}
>
> > + }
> > +
> > + 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;
> > +
> > + 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)
> > + 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)
> > +{
> > + 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) )
> > + {
> > + 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);
> > + 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;
> > + mfn = maddr_to_mfn(addr);
> > + channel->shmem = vmap(&mfn, 1);
> > + 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)));
> > +}
>
> I wonder, why we need an extra level of indirection here. And if this is
> really needed, I wonder why map(unmap)_memory* name was chosen, as there is
> no memory mapping/unmapping really happens here.
>
I've added extra indirection to hide math like
paddr_to_pfn(PAGE_ALIGN(addr + len -1)
so you don't have to math in each call. unmap_memory_from_domain called
from 2 places, so I moved both calls to separate function.
Although, I agree that map/unmap is not perfect name. I consider
renaming it to mem_permit_acces and mam_deny_access.
>
> > +
> > +static int dt_update_domain_range(struct domain *d, uint64_t addr,
> > + uint64_t size)
>
> Looks like *d is not used in this function.
>
Yep I will remove it. Thanks.
>
> > +{
> > + 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");
> > +
> > + 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)
>
>
> Generic question to consider ...
> If probe fails for some reason (so we cannot use mediator in Xen) what
> should we do with SCMI nodes in domain's device-tree (leave as is, drop,
> etc)?
>
If probe fails - then the devices, which were configured to use scmi
clocks\resets\power-domains will not work properly. If Domain
configuration do not use SCMI - then it will not have the scmi node.
So from my standpoint, no additional check is needed before copying scmi
nodes to Domain device-tree.
>
> > +{
> > + 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;
> > +
> > + 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);
> > + 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++ )
> > + {
> > + 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);
>
>
> It seems this breaks build on Arm32:
>
> scmi_smc.c: In function ‘scmi_domain_init’:
> /home/otyshchenko/xen/xen/include/xen/config.h:53:24: error: format ‘%lx’
> expects argument of type ‘long unsigned int’, but argument 4 has type
> ‘uint64_t {aka long long unsigned int}’ [-Werror=format=]
> #define XENLOG_INFO "<2>"
> ^
> scmi_smc.c:569:12: note: in expansion of macro ‘XENLOG_INFO’
> printk(XENLOG_INFO "scmi: Aquire SCMI channel id = 0x%x , domain_id =
> %d"
> ^~~~~~~~~~~
> scmi_smc.c:570:25: note: format string is defined here
> "paddr = 0x%lx\n", channel->chan_id, channel->domain_id,
> ~~^
> %llx
>
Thanks a lot. I will work through all buids in v2.
>
>
> > +
> > + 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;
> > + }
> > + }
> > +
> > + 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;
> > +
> > + 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);
> I didn't manage to find where corresponding map_memory_from_domain() is
> called for a non-hardware domain (it seems that scmi_domain_init() only
> directly handles hardware domain case).
> Or perhaps, it is called indirectly at do_domctl(): case
> XEN_DOMCTL_iomem_permission:?
>
You were right. We need to permit access only for the Hardware domain.
>
> I wonder, do we really need to call this here? Taking into the account that
> unmap_memory_from_domain() doesn't actually unmap anything, but only removes
> a range from the iomem_caps rangeset
> for the domain to be destroyed and all involved rangesets (including
> iomem_caps) will be removed soon at rangeset_domain_destroy() anyway. Or I
> missed something?
>
unmap_memory_from_domain is also used in scmi_domain_init if error
occured after successfull map_memory_to_domain call.
But this is a good point. I think it will be a good idea to make
unmap_memory_from_domain in scmi_domain_destroy only for hardware
domain. And make sure that for Software domains range is unmapped using
XEN_DOMCTL_iomem_permission call.
I'll make those changes in v2.
>
> > + spin_unlock(&channel->lock);
> > + return;
>
> empty return could be dropped, I think.
>
Agree. Will be fixed.
>
> > +}
> > +
> > +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 )
>
> This also breaks build on Arm32:
>
> scmi_smc.c: In function ‘scmi_handle_call’:
> scmi_smc.c:736:42: error: ‘struct cpu_user_regs’ has no member named ‘x0’;
> did you mean ‘r0’?
> if ( agent_channel->func_id != regs->x0 )
> ^~
> r0
> cc1: all warnings being treated as errors
>
Thanks, I will fix it in v2.
> ***
>
> BTW, I noticed that xen/arch/arm/traps.c contains the following construct,
> probably we might want something similar here?
>
> #ifdef CONFIG_ARM_64
> #define HYPERCALL_RESULT_REG(r) (r)->x0
> [snip]
> #else
> #define HYPERCALL_RESULT_REG(r) (r)->r0
> [snip]
> #endif
>
>
> This RFC patch series, so I think, there is no serious issues at the moment,
> this is rather to let you know for the future (when you drop RFC tag).
> I have to admit, I often forget to build-test on Arm32 also))
>
Thank you for understanding. But will try to cover all architectures.
>
> > + {
> > + 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;
> > +}
> > +
> > +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 */
>
> --
> Regards,
>
> Oleksandr Tyshchenko
>
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |