[RFC,v2,5/8] xen/arm: introduce SCMI-SMC mediator driver

Commit Message

Oleksii Moisieiev Feb. 8, 2022, 6 p.m. UTC

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.

Originally, cpg should be passed to the domain so it can work with
power-domains/clocks/resets etc. Considering that cpg can't be split between
the Domains, we get the limitation that the devices, which are using
power-domains/clocks/resets etc, couldn't be split between the domains.
The solution is to move the power-domain/clock/resets etc to the
Firmware (such as SCP firmware or ATF) and provide interface for the
Domains. XEN should have an entity, caled SCI-Mediator, which is
responsible for messages redirection between Domains and Firmware and
for permission handling.

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 <[e-mail]>
---
 xen/arch/arm/Kconfig        |   2 +
 xen/arch/arm/sci/Kconfig    |  10 +
 xen/arch/arm/sci/scmi_smc.c | 959 ++++++++++++++++++++++++++++++++++++
 3 files changed, 971 insertions(+)
 create mode 100644 xen/arch/arm/sci/Kconfig
 create mode 100644 xen/arch/arm/sci/scmi_smc.c

Comments

Oleksandr Andrushchenko Feb. 9, 2022, 3:02 p.m. UTC | #1

Hi, Oleksii!

On 08.02.22 20:00, 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.
>
> Originally, cpg should be passed to the domain so it can work with
> power-domains/clocks/resets etc. Considering that cpg can't be split between
> the Domains, we get the limitation that the devices, which are using
> power-domains/clocks/resets etc, couldn't be split between the domains.
> The solution is to move the power-domain/clock/resets etc to the
> Firmware (such as SCP firmware or ATF) and provide interface for the
> Domains. XEN should have an entity, caled SCI-Mediator, which is
> responsible for messages redirection between Domains and Firmware and
> for permission handling.
>
> 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 <[e-mail]>
> ---
>   xen/arch/arm/Kconfig        |   2 +
>   xen/arch/arm/sci/Kconfig    |  10 +
>   xen/arch/arm/sci/scmi_smc.c | 959 ++++++++++++++++++++++++++++++++++++
>   3 files changed, 971 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 ab07833582..3b0dfc57b6 100644
> --- a/xen/arch/arm/Kconfig
> +++ b/xen/arch/arm/Kconfig
> @@ -123,6 +123,8 @@ config ARM_SCI
>   	  support. It allows guests to control system resourcess via one of
>   	  ARM_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..10b634d2ed
> --- /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 ARM_SCI && HOST_DTB_EXPORT
> +	---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/scmi_smc.c b/xen/arch/arm/sci/scmi_smc.c
> new file mode 100644
> index 0000000000..103529dfab
> --- /dev/null
> +++ b/xen/arch/arm/sci/scmi_smc.c
> @@ -0,0 +1,959 @@
> +/*
> + * xen/arch/arm/sci/scmi_smc.c
> + *
> + * SCMI mediator driver, using SCP as transport.
> + *
> + * Oleksii Moisieiev <[e-mail]>
> + * 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
Can the above be sorted?
> +
> +/* 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                 "arm,scmi-shmem"
> +#define SCMI_SHMEM                         "shmem"
> +#define SCMI_SHMEM_MAPPED_SIZE             PAGE_SIZE
> +
> +#define HYP_CHANNEL                          0x0
Alignment
> +
> +#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;
> +
> +#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 dt_channel_addr {
> +    u64 addr;
> +    u64 size;
Here and elsewhere: do not use uXX in new code
and use uintXX_t instead
> +    struct list_head list;
> +};
> +
> +struct scmi_channel {
> +    int chan_id;
> +    int agent_id;
Can these be unsigned int?
Below in get_channel_by_id you have it as uint8_t chan_id...
> +    uint32_t func_id;
> +    domid_t domain_id;
> +    uint64_t paddr;
> +    uint64_t len;
> +    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;
> +};
> +
> +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)
Use of inline is doubtful, please see [1]
> +{
> +    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;
> +}
> +
> +/*
> + * Copy data from IO memory space to "real" memory space.
> + */
> +void __memcpy_fromio(void *to, const volatile void __iomem *from, size_t count)
This seems to be a copy of [2].
We should think about moving this into a dedicated file like in Linux,
preserving the authorship+
> +{
> +    while (count && !IS_ALIGNED((unsigned long)from, 4)) {
> +        *(u8 *)to = __raw_readb(from);
> +        from++;
> +        to++;
> +        count--;
> +    }
> +
> +    while (count >= 4) {
> +        *(u32 *)to = __raw_readl(from);
> +        from += 4;
> +        to += 4;
> +        count -= 4;
> +    }
> +
> +    while (count) {
> +        *(u8 *)to = __raw_readb(from);
> +        from++;
> +        to++;
> +        count--;
> +    }
> +}
> +
> +/*
> + * Copy data from "real" memory space to IO memory space.
> + */
> +void __memcpy_toio(volatile void __iomem *to, const void *from, size_t count)
> +{
> +    while (count && !IS_ALIGNED((unsigned long)to, 4)) {
> +        __raw_writeb(*(u8 *)from, to);
> +        from++;
> +        to++;
> +        count--;
> +    }
> +
> +    while (count >= 4) {
> +        __raw_writel(*(u32 *)from, to);
> +        from += 4;
> +        to += 4;
> +        count -= 4;
> +    }
> +
> +    while (count) {
> +        __raw_writeb(*(u8 *)from, to);
> +        from++;
> +        to++;
> +        count--;
> +    }
> +}
> +
> +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;
> +
> +    if ( (len + sizeof(chan_info->shmem->msg_header)) >
> +                         SCMI_SHMEM_MAPPED_SIZE )
> +    {
> +        printk(XENLOG_ERR
> +               "scmi: Wrong size of smc message. Data is invalid\n");
> +        return -EINVAL;
> +    }
> +
> +    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;
Could it be just 0?
> +    /* 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 )
Here and elsewhere: please consider using parentheses
> +        __memcpy_toio((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;
> +    default:
> +        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);
> +
> +    if ( len >= SCMI_SHMEM_MAPPED_SIZE - sizeof(chan_info->shmem) )
Parentheses, as mentioned above, may improve code readability
> +    {
> +        printk(XENLOG_ERR
> +               "scmi: Wrong size of input smc message. Data may be invalid\n");
> +        return -EINVAL;
> +    }
> +
> +    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_fromio(data, chan_info->shmem->msg_payload, recv_len);
> +    }
No need for parentheses for a single statement
> +
> +    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;
> +
> +    ASSERT( channel && channel->shmem);
> +
> +    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);
Blank line
> +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;
> +        }
> +    }
> +
Extra line and in the code below
> +    spin_unlock(&scmi_data.channel_list_lock);
> +    if ( found )
> +        return curr;
> +
> +    return NULL;
> +}
> +
> +static struct scmi_channel *aquire_scmi_channel(domid_t 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->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)
> +{
> +    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_cache(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 after vmap %p\n", channel->shmem);
> +    return 0;
> +}
> +
> +static void unmap_channel_memory(struct scmi_channel *channel)
> +{
> +    ASSERT( channel && channel->shmem );
> +    iounmap(channel->shmem);
> +    channel->shmem = NULL;
> +}
> +
> +static struct scmi_channel *smc_create_channel(uint8_t chan_id,
> +                                               uint32_t func_id, uint64_t addr)
> +{
> +    struct scmi_channel *channel;
> +
> +    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;
> +    channel->shmem = NULL;
> +    channel->paddr = addr;
> +    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 mem_permit_access(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 mem_deny_access(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(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, SCMI_SHARED_MEMORY);
> +    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)
> +    {
> +        list_del(&curr->list);
> +        xfree(curr);
> +    }
> +
This looks like a pattern you use with a blank line after list_for_each
> +    spin_unlock(&scmi_data.channel_list_lock);
> +}
> +
> +static struct dt_device_node *get_dt_node_from_property(
> +                struct dt_device_node *node, const char * p_name)
> +{
> +    const __be32 *prop;
> +
> +    ASSERT( node );
> +
> +    prop = dt_get_property(node, p_name, NULL);
> +    if ( !prop )
> +        return ERR_PTR(-EINVAL);
> +
> +    return dt_find_node_by_phandle(be32_to_cpup(prop));
> +}
> +
> +static int get_shmem_regions(struct list_head *head, u64 hyp_addr)
> +{
> +    struct dt_device_node *node;
> +    int ret;
> +    struct dt_channel_addr *lchan;
> +    u64 laddr, lsize;
> +
> +    node = dt_find_compatible_node(NULL, NULL, SCMI_SHARED_MEMORY);
> +    if ( !node )
> +        return -ENOENT;
> +
> +    while ( node )
> +    {
> +        ret = dt_device_get_address(node, 0, &laddr, &lsize);
> +        if ( ret )
> +            return ret;
> +
> +        if ( laddr != hyp_addr )
> +        {
> +            lchan = xmalloc(struct dt_channel_addr);
> +            if ( !lchan )
> +                return -ENOMEM;
> +            lchan->addr = laddr;
> +            lchan->size = lsize;
> +
> +            list_add_tail(&lchan->list, head);
> +        }
> +
> +        node = dt_find_compatible_node(node, NULL, SCMI_SHARED_MEMORY);
> +    }
> +
> +    return 0;
> +}
> +
> +static int read_hyp_channel_addr(struct dt_device_node *scmi_node,
> +                                 u64 *addr, u64 *size)
> +{
> +    struct dt_device_node *shmem_node;
> +    shmem_node = get_dt_node_from_property(scmi_node, "shmem");
> +    if ( IS_ERR_OR_NULL(shmem_node) )
> +    {
> +        printk(XENLOG_ERR
> +               "scmi: Device tree error, can't parse reserved memory %ld\n",
> +               PTR_ERR(shmem_node));
> +        return PTR_ERR(shmem_node);
> +    }
> +
> +    return dt_device_get_address(shmem_node, 0, addr, size);
> +}
> +
> +static void free_shmem_regions(struct list_head *addr_list)
> +{
> +    struct dt_channel_addr *curr, *_curr;
> +
> +    list_for_each_entry_safe (curr, _curr, addr_list, list)
> +    {
> +        list_del(&curr->list);
> +        xfree(curr);
> +    }
> +}
> +
> +static __init bool scmi_probe(struct dt_device_node *scmi_node)
> +{
> +    u64 addr, size;
> +    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;
> +    struct dt_channel_addr *entry;
> +    struct list_head addr_list;
> +
> +    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;
> +    }
> +
> +    ret = read_hyp_channel_addr(scmi_node, &addr, &size);
> +    if ( IS_ERR_VALUE(ret) )
> +        return false;
> +
> +    if ( !IS_ALIGNED(size, SCMI_SHMEM_MAPPED_SIZE) )
> +    {
> +        printk(XENLOG_ERR "scmi: Reserved memory is not aligned\n");
> +        return false;
> +    }
> +
> +    INIT_LIST_HEAD(&addr_list);
> +
> +    ret = get_shmem_regions(&addr_list, addr);
> +    if ( IS_ERR_VALUE(ret) )
> +        goto out;
> +
> +    channel = smc_create_channel(HYP_CHANNEL, func_id, addr);
> +    if ( IS_ERR(channel) )
> +        goto out;
> +
> +    ret = map_channel_memory(channel);
> +    if ( ret )
> +        goto out;
> +
> +    spin_lock(&scmi_data.channel_list_lock);
> +    channel->domain_id = DOMID_XEN;
> +    spin_unlock(&scmi_data.channel_list_lock);
> +
> +    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;
> +
> +    ret = check_scmi_status(rx.status);
> +    if ( ret )
You should consider either using IS_ERR_VALUE in everywhere or
don't use it at all
> +        goto error;
> +
> +    n_agents = FIELD_GET(MSG_N_AGENTS_MASK, rx.attributes);
> +    printk(XENLOG_DEBUG "scmi: Got agent count %d\n", n_agents);
> +
> +    i = 1;
> +    list_for_each_entry(entry, &addr_list, list)
> +    {
> +        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,
> +                                           entry->addr);
This should fit in a single line
> +        if ( IS_ERR(agent_channel) )
> +        {
> +            ret = PTR_ERR(agent_channel);
> +            goto error;
> +        }
> +
> +        ret = map_channel_memory(agent_channel);
> +        if ( ret )
> +            goto error;
> +
> +        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 )
> +        {
> +            unmap_channel_memory(agent_channel);
> +            goto error;
> +        }
> +
> +        unmap_channel_memory(agent_channel);
This could be moved before checking ret value
> +
> +        ret = check_scmi_status(da_rx.status);
> +        if ( ret )
> +            goto error;
> +
> +        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;
> +
> +        if ( i == n_agents )
> +            break;
> +
> +        i++;
> +    }
> +
> +    scmi_data.initialized = true;
> +    goto out;
> +
> +error:
This label sounds strange while returning without error,
could it be like "unmap_channel" or something?
> +    unmap_channel_memory(channel);
> +    free_channel_list();
> +out:
> +    free_shmem_regions(&addr_list);
> +    return ret == 0;
> +}
> +
> +static int scmi_domain_init(struct domain *d,
> +                           struct xen_arch_domainconfig *config)
> +{
> +    struct scmi_channel *channel;
> +    int ret;
> +
> +    if ( !scmi_data.initialized )
> +        return 0;
> +
> +    printk(XENLOG_INFO "scmi: domain_id = %d\n", d->domain_id);
> +
> +    channel = aquire_scmi_channel(d->domain_id);
> +    if ( IS_ERR_OR_NULL(channel) )
> +        return -ENOENT;
> +
> +#ifdef CONFIG_ARM_32
> +    printk(XENLOG_INFO
> +           "scmi: Aquire SCMI channel id = 0x%x , domain_id = %d paddr = 0x%llx\n",
> +           channel->chan_id, channel->domain_id, channel->paddr);
> +#else
> +    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);
> +#endif
> +
> +    if ( is_hardware_domain(d) )
> +    {
> +        ret = mem_permit_access(d, channel->paddr, PAGE_SIZE);
You already have SCMI_SHMEM_MAPPED_SIZE
We should also assert if SCMI_SHMEM_MAPPED_SIZE !- PAGE_SIZE
I guess as currently all the code is built with this assumption
> +        if ( IS_ERR_VALUE(ret) )
> +            goto error;
> +
> +        ret = dt_update_domain_range(channel->paddr, PAGE_SIZE);
> +        if ( IS_ERR_VALUE(ret) )
> +        {
> +            int rc = mem_deny_access(d, channel->paddr, PAGE_SIZE);
> +            if ( rc )
> +                printk(XENLOG_ERR "Unable to mem_deny_access\n");
> +
> +            goto error;
> +        }
> +    }
> +
> +    d->arch.sci = channel;
> +    if ( config )
> +        config->arm_sci_agent_paddr = channel->paddr;
> +
> +    return 0;
Blank line
> +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;
> +    struct scmi_perms_tx {
> +        uint32_t agent_id;
> +        uint32_t device_id;
> +        uint32_t flags;
> +    } 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 = d->arch.sci;
> +    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));
This could be DEBUG print
> +
> +    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;
> +
> +    mem_deny_access(d, channel->paddr, PAGE_SIZE);
> +    spin_unlock(&channel->lock);
> +}
> +
> +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->r0 )
> +    {
> +        res = false;
> +        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 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,
> +};
> +
> +REGISTER_SCI_MEDIATOR(scmi_smc, "SCMI-SMC", XEN_DOMCTL_CONFIG_ARM_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:
> + */

[1] https://www.kernel.org/doc/local/inline.html
[2] https://elixir.bootlin.com/linux/v5.17-rc3/source/arch/arm64/kernel/io.c

Julien Grall Feb. 9, 2022, 3:23 p.m. UTC | #2

On 09/02/2022 15:02, Oleksandr Andrushchenko wrote:
>> +{
>> +    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;
>> +}
>> +
>> +/*
>> + * Copy data from IO memory space to "real" memory space.
>> + */
>> +void __memcpy_fromio(void *to, const volatile void __iomem *from, size_t count)
> This seems to be a copy of [2].
> We should think about moving this into a dedicated file like in Linux,
> preserving the authorship+

+1. Also this should be in a separate patch.

Cheers,

Bertrand Marquis Feb. 11, 2022, 8:46 a.m. UTC | #3

Hi Oleksii,


> On 8 Feb 2022, at 18:00, Oleksii Moisieiev <[e-mail]> 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.
> 
> Originally, cpg should be passed to the domain so it can work with
> power-domains/clocks/resets etc. Considering that cpg can't be split between
> the Domains, we get the limitation that the devices, which are using
> power-domains/clocks/resets etc, couldn't be split between the domains.
> The solution is to move the power-domain/clock/resets etc to the
> Firmware (such as SCP firmware or ATF) and provide interface for the
> Domains. XEN should have an entity, caled SCI-Mediator, which is
> responsible for messages redirection between Domains and Firmware and
> for permission handling.
> 
> 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.

Before going more deeply in the code I would like to discuss the general
design here and ask some questions to prevent to rework the code before
we all agree that this is the right solution and that we want this in Xen.

First I want to point out that clock/reset/power virtualization is a problem
on most applications using device pass-through and I am very glad that
someone is looking into it.
Also SCMI is the current standard existing for this so relying on it is a very
good idea.

Latest version SCMI standard (DEN0056D v3.1) is defining some means
to use SCMI on a virtualised system. In chapter 4.2.1, the standard
recommends to set permissions per agent in the hypervisor so that a VM
could later use the discovery protocol to detect the resources and use them.
Using this kind of scenario the mediator in Xen would just configure the
Permissions in the SCMI and would then rely on it to limit what is possible
by who just by just assigning a channel to a VM.

In your current design (please correct me if I am wrong) you seem to fully
rely on Xen and the FDT for discovery and permission.
Wouldn’t it be a better idea to use the protocol fully ?
Could we get rid of some of the FDT dependencies by using the discovery
system of SCMI ?
How is Linux doing this currently ? Is it relying on device tree to discover
 the SCMI resources ?

Also I understand that you rely on some entries to be declared in the device
tree and also some support to be implemented in ATF or SCP. I checked in
The boards I have access to and the device trees but none of this seem to
be supported there. Could you tell which board/configuration/ATF you are
using so that the implementation could be tested/validated ?


Regards
Bertrand


> 
> Signed-off-by: Oleksii Moisieiev <[e-mail]>
> ---
> xen/arch/arm/Kconfig        |   2 +
> xen/arch/arm/sci/Kconfig    |  10 +
> xen/arch/arm/sci/scmi_smc.c | 959 ++++++++++++++++++++++++++++++++++++
> 3 files changed, 971 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 ab07833582..3b0dfc57b6 100644
> --- a/xen/arch/arm/Kconfig
> +++ b/xen/arch/arm/Kconfig
> @@ -123,6 +123,8 @@ config ARM_SCI
> 	  support. It allows guests to control system resourcess via one of
> 	  ARM_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..10b634d2ed
> --- /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 ARM_SCI && HOST_DTB_EXPORT
> +	---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/scmi_smc.c b/xen/arch/arm/sci/scmi_smc.c
> new file mode 100644
> index 0000000000..103529dfab
> --- /dev/null
> +++ b/xen/arch/arm/sci/scmi_smc.c
> @@ -0,0 +1,959 @@
> +/*
> + * xen/arch/arm/sci/scmi_smc.c
> + *
> + * SCMI mediator driver, using SCP as transport.
> + *
> + * Oleksii Moisieiev <[e-mail]>
> + * 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                 "arm,scmi-shmem"
> +#define SCMI_SHMEM                         "shmem"
> +#define SCMI_SHMEM_MAPPED_SIZE             PAGE_SIZE
> +
> +#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;
> +
> +#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 dt_channel_addr {
> +    u64 addr;
> +    u64 size;
> +    struct list_head list;
> +};
> +
> +struct scmi_channel {
> +    int chan_id;
> +    int agent_id;
> +    uint32_t func_id;
> +    domid_t domain_id;
> +    uint64_t paddr;
> +    uint64_t len;
> +    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;
> +};
> +
> +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;
> +}
> +
> +/*
> + * Copy data from IO memory space to "real" memory space.
> + */
> +void __memcpy_fromio(void *to, const volatile void __iomem *from, size_t count)
> +{
> +    while (count && !IS_ALIGNED((unsigned long)from, 4)) {
> +        *(u8 *)to = __raw_readb(from);
> +        from++;
> +        to++;
> +        count--;
> +    }
> +
> +    while (count >= 4) {
> +        *(u32 *)to = __raw_readl(from);
> +        from += 4;
> +        to += 4;
> +        count -= 4;
> +    }
> +
> +    while (count) {
> +        *(u8 *)to = __raw_readb(from);
> +        from++;
> +        to++;
> +        count--;
> +    }
> +}
> +
> +/*
> + * Copy data from "real" memory space to IO memory space.
> + */
> +void __memcpy_toio(volatile void __iomem *to, const void *from, size_t count)
> +{
> +    while (count && !IS_ALIGNED((unsigned long)to, 4)) {
> +        __raw_writeb(*(u8 *)from, to);
> +        from++;
> +        to++;
> +        count--;
> +    }
> +
> +    while (count >= 4) {
> +        __raw_writel(*(u32 *)from, to);
> +        from += 4;
> +        to += 4;
> +        count -= 4;
> +    }
> +
> +    while (count) {
> +        __raw_writeb(*(u8 *)from, to);
> +        from++;
> +        to++;
> +        count--;
> +    }
> +}
> +
> +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;
> +
> +    if ( (len + sizeof(chan_info->shmem->msg_header)) >
> +                         SCMI_SHMEM_MAPPED_SIZE )
> +    {
> +        printk(XENLOG_ERR
> +               "scmi: Wrong size of smc message. Data is invalid\n");
> +        return -EINVAL;
> +    }
> +
> +    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_toio((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;
> +    default:
> +        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);
> +
> +    if ( len >= SCMI_SHMEM_MAPPED_SIZE - sizeof(chan_info->shmem) )
> +    {
> +        printk(XENLOG_ERR
> +               "scmi: Wrong size of input smc message. Data may be invalid\n");
> +        return -EINVAL;
> +    }
> +
> +    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_fromio(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;
> +
> +    ASSERT( channel && channel->shmem);
> +
> +    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 *aquire_scmi_channel(domid_t 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->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)
> +{
> +    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_cache(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 after vmap %p\n", channel->shmem);
> +    return 0;
> +}
> +
> +static void unmap_channel_memory(struct scmi_channel *channel)
> +{
> +    ASSERT( channel && channel->shmem );
> +    iounmap(channel->shmem);
> +    channel->shmem = NULL;
> +}
> +
> +static struct scmi_channel *smc_create_channel(uint8_t chan_id,
> +                                               uint32_t func_id, uint64_t addr)
> +{
> +    struct scmi_channel *channel;
> +
> +    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;
> +    channel->shmem = NULL;
> +    channel->paddr = addr;
> +    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 mem_permit_access(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 mem_deny_access(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(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, SCMI_SHARED_MEMORY);
> +    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)
> +    {
> +        list_del(&curr->list);
> +        xfree(curr);
> +    }
> +
> +    spin_unlock(&scmi_data.channel_list_lock);
> +}
> +
> +static struct dt_device_node *get_dt_node_from_property(
> +                struct dt_device_node *node, const char * p_name)
> +{
> +    const __be32 *prop;
> +
> +    ASSERT( node );
> +
> +    prop = dt_get_property(node, p_name, NULL);
> +    if ( !prop )
> +        return ERR_PTR(-EINVAL);
> +
> +    return dt_find_node_by_phandle(be32_to_cpup(prop));
> +}
> +
> +static int get_shmem_regions(struct list_head *head, u64 hyp_addr)
> +{
> +    struct dt_device_node *node;
> +    int ret;
> +    struct dt_channel_addr *lchan;
> +    u64 laddr, lsize;
> +
> +    node = dt_find_compatible_node(NULL, NULL, SCMI_SHARED_MEMORY);
> +    if ( !node )
> +        return -ENOENT;
> +
> +    while ( node )
> +    {
> +        ret = dt_device_get_address(node, 0, &laddr, &lsize);
> +        if ( ret )
> +            return ret;
> +
> +        if ( laddr != hyp_addr )
> +        {
> +            lchan = xmalloc(struct dt_channel_addr);
> +            if ( !lchan )
> +                return -ENOMEM;
> +            lchan->addr = laddr;
> +            lchan->size = lsize;
> +
> +            list_add_tail(&lchan->list, head);
> +        }
> +
> +        node = dt_find_compatible_node(node, NULL, SCMI_SHARED_MEMORY);
> +    }
> +
> +    return 0;
> +}
> +
> +static int read_hyp_channel_addr(struct dt_device_node *scmi_node,
> +                                 u64 *addr, u64 *size)
> +{
> +    struct dt_device_node *shmem_node;
> +    shmem_node = get_dt_node_from_property(scmi_node, "shmem");
> +    if ( IS_ERR_OR_NULL(shmem_node) )
> +    {
> +        printk(XENLOG_ERR
> +               "scmi: Device tree error, can't parse reserved memory %ld\n",
> +               PTR_ERR(shmem_node));
> +        return PTR_ERR(shmem_node);
> +    }
> +
> +    return dt_device_get_address(shmem_node, 0, addr, size);
> +}
> +
> +static void free_shmem_regions(struct list_head *addr_list)
> +{
> +    struct dt_channel_addr *curr, *_curr;
> +
> +    list_for_each_entry_safe (curr, _curr, addr_list, list)
> +    {
> +        list_del(&curr->list);
> +        xfree(curr);
> +    }
> +}
> +
> +static __init bool scmi_probe(struct dt_device_node *scmi_node)
> +{
> +    u64 addr, size;
> +    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;
> +    struct dt_channel_addr *entry;
> +    struct list_head addr_list;
> +
> +    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;
> +    }
> +
> +    ret = read_hyp_channel_addr(scmi_node, &addr, &size);
> +    if ( IS_ERR_VALUE(ret) )
> +        return false;
> +
> +    if ( !IS_ALIGNED(size, SCMI_SHMEM_MAPPED_SIZE) )
> +    {
> +        printk(XENLOG_ERR "scmi: Reserved memory is not aligned\n");
> +        return false;
> +    }
> +
> +    INIT_LIST_HEAD(&addr_list);
> +
> +    ret = get_shmem_regions(&addr_list, addr);
> +    if ( IS_ERR_VALUE(ret) )
> +        goto out;
> +
> +    channel = smc_create_channel(HYP_CHANNEL, func_id, addr);
> +    if ( IS_ERR(channel) )
> +        goto out;
> +
> +    ret = map_channel_memory(channel);
> +    if ( ret )
> +        goto out;
> +
> +    spin_lock(&scmi_data.channel_list_lock);
> +    channel->domain_id = DOMID_XEN;
> +    spin_unlock(&scmi_data.channel_list_lock);
> +
> +    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;
> +
> +    ret = check_scmi_status(rx.status);
> +    if ( ret )
> +        goto error;
> +
> +    n_agents = FIELD_GET(MSG_N_AGENTS_MASK, rx.attributes);
> +    printk(XENLOG_DEBUG "scmi: Got agent count %d\n", n_agents);
> +
> +    i = 1;
> +    list_for_each_entry(entry, &addr_list, list)
> +    {
> +        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,
> +                                           entry->addr);
> +        if ( IS_ERR(agent_channel) )
> +        {
> +            ret = PTR_ERR(agent_channel);
> +            goto error;
> +        }
> +
> +        ret = map_channel_memory(agent_channel);
> +        if ( ret )
> +            goto error;
> +
> +        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 )
> +        {
> +            unmap_channel_memory(agent_channel);
> +            goto error;
> +        }
> +
> +        unmap_channel_memory(agent_channel);
> +
> +        ret = check_scmi_status(da_rx.status);
> +        if ( ret )
> +            goto error;
> +
> +        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;
> +
> +        if ( i == n_agents )
> +            break;
> +
> +        i++;
> +    }
> +
> +    scmi_data.initialized = true;
> +    goto out;
> +
> +error:
> +    unmap_channel_memory(channel);
> +    free_channel_list();
> +out:
> +    free_shmem_regions(&addr_list);
> +    return ret == 0;
> +}
> +
> +static int scmi_domain_init(struct domain *d,
> +                           struct xen_arch_domainconfig *config)
> +{
> +    struct scmi_channel *channel;
> +    int ret;
> +
> +    if ( !scmi_data.initialized )
> +        return 0;
> +
> +    printk(XENLOG_INFO "scmi: domain_id = %d\n", d->domain_id);
> +
> +    channel = aquire_scmi_channel(d->domain_id);
> +    if ( IS_ERR_OR_NULL(channel) )
> +        return -ENOENT;
> +
> +#ifdef CONFIG_ARM_32
> +    printk(XENLOG_INFO
> +           "scmi: Aquire SCMI channel id = 0x%x , domain_id = %d paddr = 0x%llx\n",
> +           channel->chan_id, channel->domain_id, channel->paddr);
> +#else
> +    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);
> +#endif
> +
> +    if ( is_hardware_domain(d) )
> +    {
> +        ret = mem_permit_access(d, channel->paddr, PAGE_SIZE);
> +        if ( IS_ERR_VALUE(ret) )
> +            goto error;
> +
> +        ret = dt_update_domain_range(channel->paddr, PAGE_SIZE);
> +        if ( IS_ERR_VALUE(ret) )
> +        {
> +            int rc = mem_deny_access(d, channel->paddr, PAGE_SIZE);
> +            if ( rc )
> +                printk(XENLOG_ERR "Unable to mem_deny_access\n");
> +
> +            goto error;
> +        }
> +    }
> +
> +    d->arch.sci = channel;
> +    if ( config )
> +        config->arm_sci_agent_paddr = channel->paddr;
> +
> +    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;
> +    struct scmi_perms_tx {
> +        uint32_t agent_id;
> +        uint32_t device_id;
> +        uint32_t flags;
> +    } 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 = d->arch.sci;
> +    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;
> +
> +    mem_deny_access(d, channel->paddr, PAGE_SIZE);
> +    spin_unlock(&channel->lock);
> +}
> +
> +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->r0 )
> +    {
> +        res = false;
> +        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 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,
> +};
> +
> +REGISTER_SCI_MEDIATOR(scmi_smc, "SCMI-SMC", XEN_DOMCTL_CONFIG_ARM_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:
> + */
> -- 
> 2.27.0

Oleksii Moisieiev Feb. 11, 2022, 10:44 a.m. UTC | #4

Hi Bertrand,

On Fri, Feb 11, 2022 at 08:46:05AM +0000, Bertrand Marquis wrote:
> Hi Oleksii,
> 
> 
> > On 8 Feb 2022, at 18:00, Oleksii Moisieiev <[e-mail]> 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.
> > 
> > Originally, cpg should be passed to the domain so it can work with
> > power-domains/clocks/resets etc. Considering that cpg can't be split between
> > the Domains, we get the limitation that the devices, which are using
> > power-domains/clocks/resets etc, couldn't be split between the domains.
> > The solution is to move the power-domain/clock/resets etc to the
> > Firmware (such as SCP firmware or ATF) and provide interface for the
> > Domains. XEN should have an entity, caled SCI-Mediator, which is
> > responsible for messages redirection between Domains and Firmware and
> > for permission handling.
> > 
> > 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.
> 
> Before going more deeply in the code I would like to discuss the general
> design here and ask some questions to prevent to rework the code before
> we all agree that this is the right solution and that we want this in Xen.
> 
> First I want to point out that clock/reset/power virtualization is a problem
> on most applications using device pass-through and I am very glad that
> someone is looking into it.
> Also SCMI is the current standard existing for this so relying on it is a very
> good idea.
> 
> Latest version SCMI standard (DEN0056D v3.1) is defining some means
> to use SCMI on a virtualised system. In chapter 4.2.1, the standard
> recommends to set permissions per agent in the hypervisor so that a VM
> could later use the discovery protocol to detect the resources and use them.
> Using this kind of scenario the mediator in Xen would just configure the
> Permissions in the SCMI and would then rely on it to limit what is possible
> by who just by just assigning a channel to a VM.

> 
> In your current design (please correct me if I am wrong) you seem to fully
> rely on Xen and the FDT for discovery and permission.

In current implementation Xen is the trusted agent. And it's responsible
for permissions setting. During initialization it discovers agent and
set permissions by using BASE_SET_DEVICE_PERMISSIONS to the Dom0. When
new domain is created, Xen assigns agent id for this domain and request
resources, that are passed-through to this Domain.

I'm getting the follwing information from FDT:
1) Shared memory addressed, which should be used for agents. During
initialization I send BASE_DISCOVER_AGENT to each of this addresses and
receive agent_id. Xen is responsible for assigning agent_id for the
Domain. Then Xen intercept smc calls from the domain, set agent_id and
redirects it to the Firmware.

2) Devices, that are using SCMI. Those devices has clock/power/resets
etc related to scmi protocol (as it is done in Linux kernel)
and scmi_devid should be set. I'm currently preparing to send patch,
updating kernel bindings with this parameter to Linux kernel.
scmi_devid value should match device id, set in the Firmware.
dt example:
&usb0 {
    scmi_devid = <1>; // usb0 device id
    clocks = <&scmi_clock 1> // relays on clock with id 1
}

Xen requests permission for the device when device is attached to the
Domain during creation.

> Wouldn’t it be a better idea to use the protocol fully ?

Hm, I was thinking I am using the protocol fully. Did I miss something?

> Could we get rid of some of the FDT dependencies by using the discovery
> system of SCMI ?

I'm using FDT to get shmem regions for the channels. Then I send
BASE_DISCOVER_AGENT to each region and getting agent data. Did I use the
discovery system wrong?

> How is Linux doing this currently ? Is it relying on device tree to discover
>  the SCMI resources ?

Yes. Linux kernel has 2 nodes in the device-tree: arm,scmi-shmem, which
includes memory region for the communication and arm,scmi-smc node,
which describes all data related to scmi ( func_id, protocols etc)
Then the device nodes refer to the protocols by setting
clock/resets/power-domains etc. Please see the example above.
BASE_DISCOVER_AGENT is not used in Linux kernel.
The main idea was that scmi related changes to the device-tree are
common for virtualized and non virtualized systems. So the same FDT
configuration should work with of without Xen.

> 
> Also I understand that you rely on some entries to be declared in the device
> tree and also some support to be implemented in ATF or SCP. I checked in
> The boards I have access to and the device trees but none of this seem to
> be supported there. Could you tell which board/configuration/ATF you are
> using so that the implementation could be tested/validated ?
> 

We're currently have POC made for r8a77951-ulcb-kf and
r8a77961-salvator-xs boards. It's based on:
Linux-bsp kernel: 
[e-mail]:renesas-rcar/linux-bsp.git
based on tag <rcar-5.0.0.rc4>

ATF: 
[e-mail]:renesas-rcar/arm-trusted-firmware.git
based on branch <rcar_gen3_v2.5>

I can push those changes to Github, so you can review them.

Best regards,
Oleksii.

> 
> Regards
> Bertrand
> 
> 
> > 
> > Signed-off-by: Oleksii Moisieiev <[e-mail]>
> > ---
> > xen/arch/arm/Kconfig        |   2 +
> > xen/arch/arm/sci/Kconfig    |  10 +
> > xen/arch/arm/sci/scmi_smc.c | 959 ++++++++++++++++++++++++++++++++++++
> > 3 files changed, 971 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 ab07833582..3b0dfc57b6 100644
> > --- a/xen/arch/arm/Kconfig
> > +++ b/xen/arch/arm/Kconfig
> > @@ -123,6 +123,8 @@ config ARM_SCI
> > 	  support. It allows guests to control system resourcess via one of
> > 	  ARM_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..10b634d2ed
> > --- /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 ARM_SCI && HOST_DTB_EXPORT
> > +	---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/scmi_smc.c b/xen/arch/arm/sci/scmi_smc.c
> > new file mode 100644
> > index 0000000000..103529dfab
> > --- /dev/null
> > +++ b/xen/arch/arm/sci/scmi_smc.c
> > @@ -0,0 +1,959 @@
> > +/*
> > + * xen/arch/arm/sci/scmi_smc.c
> > + *
> > + * SCMI mediator driver, using SCP as transport.
> > + *
> > + * Oleksii Moisieiev <[e-mail]>
> > + * 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                 "arm,scmi-shmem"
> > +#define SCMI_SHMEM                         "shmem"
> > +#define SCMI_SHMEM_MAPPED_SIZE             PAGE_SIZE
> > +
> > +#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;
> > +
> > +#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 dt_channel_addr {
> > +    u64 addr;
> > +    u64 size;
> > +    struct list_head list;
> > +};
> > +
> > +struct scmi_channel {
> > +    int chan_id;
> > +    int agent_id;
> > +    uint32_t func_id;
> > +    domid_t domain_id;
> > +    uint64_t paddr;
> > +    uint64_t len;
> > +    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;
> > +};
> > +
> > +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;
> > +}
> > +
> > +/*
> > + * Copy data from IO memory space to "real" memory space.
> > + */
> > +void __memcpy_fromio(void *to, const volatile void __iomem *from, size_t count)
> > +{
> > +    while (count && !IS_ALIGNED((unsigned long)from, 4)) {
> > +        *(u8 *)to = __raw_readb(from);
> > +        from++;
> > +        to++;
> > +        count--;
> > +    }
> > +
> > +    while (count >= 4) {
> > +        *(u32 *)to = __raw_readl(from);
> > +        from += 4;
> > +        to += 4;
> > +        count -= 4;
> > +    }
> > +
> > +    while (count) {
> > +        *(u8 *)to = __raw_readb(from);
> > +        from++;
> > +        to++;
> > +        count--;
> > +    }
> > +}
> > +
> > +/*
> > + * Copy data from "real" memory space to IO memory space.
> > + */
> > +void __memcpy_toio(volatile void __iomem *to, const void *from, size_t count)
> > +{
> > +    while (count && !IS_ALIGNED((unsigned long)to, 4)) {
> > +        __raw_writeb(*(u8 *)from, to);
> > +        from++;
> > +        to++;
> > +        count--;
> > +    }
> > +
> > +    while (count >= 4) {
> > +        __raw_writel(*(u32 *)from, to);
> > +        from += 4;
> > +        to += 4;
> > +        count -= 4;
> > +    }
> > +
> > +    while (count) {
> > +        __raw_writeb(*(u8 *)from, to);
> > +        from++;
> > +        to++;
> > +        count--;
> > +    }
> > +}
> > +
> > +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;
> > +
> > +    if ( (len + sizeof(chan_info->shmem->msg_header)) >
> > +                         SCMI_SHMEM_MAPPED_SIZE )
> > +    {
> > +        printk(XENLOG_ERR
> > +               "scmi: Wrong size of smc message. Data is invalid\n");
> > +        return -EINVAL;
> > +    }
> > +
> > +    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_toio((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;
> > +    default:
> > +        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);
> > +
> > +    if ( len >= SCMI_SHMEM_MAPPED_SIZE - sizeof(chan_info->shmem) )
> > +    {
> > +        printk(XENLOG_ERR
> > +               "scmi: Wrong size of input smc message. Data may be invalid\n");
> > +        return -EINVAL;
> > +    }
> > +
> > +    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_fromio(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;
> > +
> > +    ASSERT( channel && channel->shmem);
> > +
> > +    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 *aquire_scmi_channel(domid_t 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->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)
> > +{
> > +    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_cache(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 after vmap %p\n", channel->shmem);
> > +    return 0;
> > +}
> > +
> > +static void unmap_channel_memory(struct scmi_channel *channel)
> > +{
> > +    ASSERT( channel && channel->shmem );
> > +    iounmap(channel->shmem);
> > +    channel->shmem = NULL;
> > +}
> > +
> > +static struct scmi_channel *smc_create_channel(uint8_t chan_id,
> > +                                               uint32_t func_id, uint64_t addr)
> > +{
> > +    struct scmi_channel *channel;
> > +
> > +    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;
> > +    channel->shmem = NULL;
> > +    channel->paddr = addr;
> > +    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 mem_permit_access(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 mem_deny_access(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(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, SCMI_SHARED_MEMORY);
> > +    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)
> > +    {
> > +        list_del(&curr->list);
> > +        xfree(curr);
> > +    }
> > +
> > +    spin_unlock(&scmi_data.channel_list_lock);
> > +}
> > +
> > +static struct dt_device_node *get_dt_node_from_property(
> > +                struct dt_device_node *node, const char * p_name)
> > +{
> > +    const __be32 *prop;
> > +
> > +    ASSERT( node );
> > +
> > +    prop = dt_get_property(node, p_name, NULL);
> > +    if ( !prop )
> > +        return ERR_PTR(-EINVAL);
> > +
> > +    return dt_find_node_by_phandle(be32_to_cpup(prop));
> > +}
> > +
> > +static int get_shmem_regions(struct list_head *head, u64 hyp_addr)
> > +{
> > +    struct dt_device_node *node;
> > +    int ret;
> > +    struct dt_channel_addr *lchan;
> > +    u64 laddr, lsize;
> > +
> > +    node = dt_find_compatible_node(NULL, NULL, SCMI_SHARED_MEMORY);
> > +    if ( !node )
> > +        return -ENOENT;
> > +
> > +    while ( node )
> > +    {
> > +        ret = dt_device_get_address(node, 0, &laddr, &lsize);
> > +        if ( ret )
> > +            return ret;
> > +
> > +        if ( laddr != hyp_addr )
> > +        {
> > +            lchan = xmalloc(struct dt_channel_addr);
> > +            if ( !lchan )
> > +                return -ENOMEM;
> > +            lchan->addr = laddr;
> > +            lchan->size = lsize;
> > +
> > +            list_add_tail(&lchan->list, head);
> > +        }
> > +
> > +        node = dt_find_compatible_node(node, NULL, SCMI_SHARED_MEMORY);
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> > +static int read_hyp_channel_addr(struct dt_device_node *scmi_node,
> > +                                 u64 *addr, u64 *size)
> > +{
> > +    struct dt_device_node *shmem_node;
> > +    shmem_node = get_dt_node_from_property(scmi_node, "shmem");
> > +    if ( IS_ERR_OR_NULL(shmem_node) )
> > +    {
> > +        printk(XENLOG_ERR
> > +               "scmi: Device tree error, can't parse reserved memory %ld\n",
> > +               PTR_ERR(shmem_node));
> > +        return PTR_ERR(shmem_node);
> > +    }
> > +
> > +    return dt_device_get_address(shmem_node, 0, addr, size);
> > +}
> > +
> > +static void free_shmem_regions(struct list_head *addr_list)
> > +{
> > +    struct dt_channel_addr *curr, *_curr;
> > +
> > +    list_for_each_entry_safe (curr, _curr, addr_list, list)
> > +    {
> > +        list_del(&curr->list);
> > +        xfree(curr);
> > +    }
> > +}
> > +
> > +static __init bool scmi_probe(struct dt_device_node *scmi_node)
> > +{
> > +    u64 addr, size;
> > +    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;
> > +    struct dt_channel_addr *entry;
> > +    struct list_head addr_list;
> > +
> > +    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;
> > +    }
> > +
> > +    ret = read_hyp_channel_addr(scmi_node, &addr, &size);
> > +    if ( IS_ERR_VALUE(ret) )
> > +        return false;
> > +
> > +    if ( !IS_ALIGNED(size, SCMI_SHMEM_MAPPED_SIZE) )
> > +    {
> > +        printk(XENLOG_ERR "scmi: Reserved memory is not aligned\n");
> > +        return false;
> > +    }
> > +
> > +    INIT_LIST_HEAD(&addr_list);
> > +
> > +    ret = get_shmem_regions(&addr_list, addr);
> > +    if ( IS_ERR_VALUE(ret) )
> > +        goto out;
> > +
> > +    channel = smc_create_channel(HYP_CHANNEL, func_id, addr);
> > +    if ( IS_ERR(channel) )
> > +        goto out;
> > +
> > +    ret = map_channel_memory(channel);
> > +    if ( ret )
> > +        goto out;
> > +
> > +    spin_lock(&scmi_data.channel_list_lock);
> > +    channel->domain_id = DOMID_XEN;
> > +    spin_unlock(&scmi_data.channel_list_lock);
> > +
> > +    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;
> > +
> > +    ret = check_scmi_status(rx.status);
> > +    if ( ret )
> > +        goto error;
> > +
> > +    n_agents = FIELD_GET(MSG_N_AGENTS_MASK, rx.attributes);
> > +    printk(XENLOG_DEBUG "scmi: Got agent count %d\n", n_agents);
> > +
> > +    i = 1;
> > +    list_for_each_entry(entry, &addr_list, list)
> > +    {
> > +        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,
> > +                                           entry->addr);
> > +        if ( IS_ERR(agent_channel) )
> > +        {
> > +            ret = PTR_ERR(agent_channel);
> > +            goto error;
> > +        }
> > +
> > +        ret = map_channel_memory(agent_channel);
> > +        if ( ret )
> > +            goto error;
> > +
> > +        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 )
> > +        {
> > +            unmap_channel_memory(agent_channel);
> > +            goto error;
> > +        }
> > +
> > +        unmap_channel_memory(agent_channel);
> > +
> > +        ret = check_scmi_status(da_rx.status);
> > +        if ( ret )
> > +            goto error;
> > +
> > +        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;
> > +
> > +        if ( i == n_agents )
> > +            break;
> > +
> > +        i++;
> > +    }
> > +
> > +    scmi_data.initialized = true;
> > +    goto out;
> > +
> > +error:
> > +    unmap_channel_memory(channel);
> > +    free_channel_list();
> > +out:
> > +    free_shmem_regions(&addr_list);
> > +    return ret == 0;
> > +}
> > +
> > +static int scmi_domain_init(struct domain *d,
> > +                           struct xen_arch_domainconfig *config)
> > +{
> > +    struct scmi_channel *channel;
> > +    int ret;
> > +
> > +    if ( !scmi_data.initialized )
> > +        return 0;
> > +
> > +    printk(XENLOG_INFO "scmi: domain_id = %d\n", d->domain_id);
> > +
> > +    channel = aquire_scmi_channel(d->domain_id);
> > +    if ( IS_ERR_OR_NULL(channel) )
> > +        return -ENOENT;
> > +
> > +#ifdef CONFIG_ARM_32
> > +    printk(XENLOG_INFO
> > +           "scmi: Aquire SCMI channel id = 0x%x , domain_id = %d paddr = 0x%llx\n",
> > +           channel->chan_id, channel->domain_id, channel->paddr);
> > +#else
> > +    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);
> > +#endif
> > +
> > +    if ( is_hardware_domain(d) )
> > +    {
> > +        ret = mem_permit_access(d, channel->paddr, PAGE_SIZE);
> > +        if ( IS_ERR_VALUE(ret) )
> > +            goto error;
> > +
> > +        ret = dt_update_domain_range(channel->paddr, PAGE_SIZE);
> > +        if ( IS_ERR_VALUE(ret) )
> > +        {
> > +            int rc = mem_deny_access(d, channel->paddr, PAGE_SIZE);
> > +            if ( rc )
> > +                printk(XENLOG_ERR "Unable to mem_deny_access\n");
> > +
> > +            goto error;
> > +        }
> > +    }
> > +
> > +    d->arch.sci = channel;
> > +    if ( config )
> > +        config->arm_sci_agent_paddr = channel->paddr;
> > +
> > +    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;
> > +    struct scmi_perms_tx {
> > +        uint32_t agent_id;
> > +        uint32_t device_id;
> > +        uint32_t flags;
> > +    } 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 = d->arch.sci;
> > +    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;
> > +
> > +    mem_deny_access(d, channel->paddr, PAGE_SIZE);
> > +    spin_unlock(&channel->lock);
> > +}
> > +
> > +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->r0 )
> > +    {
> > +        res = false;
> > +        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 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,
> > +};
> > +
> > +REGISTER_SCI_MEDIATOR(scmi_smc, "SCMI-SMC", XEN_DOMCTL_CONFIG_ARM_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:
> > + */
> > -- 
> > 2.27.0
>

Bertrand Marquis Feb. 11, 2022, 11:18 a.m. UTC | #5

Hi Oleksii,


> On 11 Feb 2022, at 10:44, Oleksii Moisieiev <[e-mail]> wrote:
> 
> Hi Bertrand,
> 
> On Fri, Feb 11, 2022 at 08:46:05AM +0000, Bertrand Marquis wrote:
>> Hi Oleksii,
>> 
>> 
>>> On 8 Feb 2022, at 18:00, Oleksii Moisieiev <[e-mail]> 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.
>>> 
>>> Originally, cpg should be passed to the domain so it can work with
>>> power-domains/clocks/resets etc. Considering that cpg can't be split between
>>> the Domains, we get the limitation that the devices, which are using
>>> power-domains/clocks/resets etc, couldn't be split between the domains.
>>> The solution is to move the power-domain/clock/resets etc to the
>>> Firmware (such as SCP firmware or ATF) and provide interface for the
>>> Domains. XEN should have an entity, caled SCI-Mediator, which is
>>> responsible for messages redirection between Domains and Firmware and
>>> for permission handling.
>>> 
>>> 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.
>> 
>> Before going more deeply in the code I would like to discuss the general
>> design here and ask some questions to prevent to rework the code before
>> we all agree that this is the right solution and that we want this in Xen.
>> 
>> First I want to point out that clock/reset/power virtualization is a problem
>> on most applications using device pass-through and I am very glad that
>> someone is looking into it.
>> Also SCMI is the current standard existing for this so relying on it is a very
>> good idea.
>> 
>> Latest version SCMI standard (DEN0056D v3.1) is defining some means
>> to use SCMI on a virtualised system. In chapter 4.2.1, the standard
>> recommends to set permissions per agent in the hypervisor so that a VM
>> could later use the discovery protocol to detect the resources and use them.
>> Using this kind of scenario the mediator in Xen would just configure the
>> Permissions in the SCMI and would then rely on it to limit what is possible
>> by who just by just assigning a channel to a VM.
> 
>> 
>> In your current design (please correct me if I am wrong) you seem to fully
>> rely on Xen and the FDT for discovery and permission.
> 
> In current implementation Xen is the trusted agent. And it's responsible
> for permissions setting. During initialization it discovers agent and
> set permissions by using BASE_SET_DEVICE_PERMISSIONS to the Dom0. When
> new domain is created, Xen assigns agent id for this domain and request
> resources, that are passed-through to this Domain.

Ok

> 
> I'm getting the follwing information from FDT:
> 1) Shared memory addressed, which should be used for agents. During
> initialization I send BASE_DISCOVER_AGENT to each of this addresses and
> receive agent_id. Xen is responsible for assigning agent_id for the
> Domain. Then Xen intercept smc calls from the domain, set agent_id and
> redirects it to the Firmware.

So Xen is setting the agent ID, no way for a guest to get access to something it
should with more check, am I right ?

> 
> 2) Devices, that are using SCMI. Those devices has clock/power/resets
> etc related to scmi protocol (as it is done in Linux kernel)
> and scmi_devid should be set. I'm currently preparing to send patch,
> updating kernel bindings with this parameter to Linux kernel.
> scmi_devid value should match device id, set in the Firmware.
> dt example:
> &usb0 {
>    scmi_devid = <1>; // usb0 device id
>    clocks = <&scmi_clock 1> // relays on clock with id 1
> }
> 
> Xen requests permission for the device when device is attached to the
> Domain during creation.

Without this, how is (if it is) the linux kernel using SCMI for power management ?

> 
>> Wouldn’t it be a better idea to use the protocol fully ?
> 
> Hm, I was thinking I am using the protocol fully. Did I miss something?

Sorry you seem to be, my understanding of your design was not right.

> 
>> Could we get rid of some of the FDT dependencies by using the discovery
>> system of SCMI ?
> 
> I'm using FDT to get shmem regions for the channels. Then I send
> BASE_DISCOVER_AGENT to each region and getting agent data. Did I use the
> discovery system wrong?

After more digging it seems you are. The link between scmi resource and device
is not possible to get automatically.

> 
>> How is Linux doing this currently ? Is it relying on device tree to discover
>> the SCMI resources ?
> 
> Yes. Linux kernel has 2 nodes in the device-tree: arm,scmi-shmem, which
> includes memory region for the communication and arm,scmi-smc node,
> which describes all data related to scmi ( func_id, protocols etc)
> Then the device nodes refer to the protocols by setting
> clock/resets/power-domains etc. Please see the example above.
> BASE_DISCOVER_AGENT is not used in Linux kernel.
> The main idea was that scmi related changes to the device-tree are
> common for virtualized and non virtualized systems. So the same FDT
> configuration should work with of without Xen.

So at this stage this is not supported in Linux and you plan to add support for it to.

> 
>> 
>> Also I understand that you rely on some entries to be declared in the device
>> tree and also some support to be implemented in ATF or SCP. I checked in
>> The boards I have access to and the device trees but none of this seem to
>> be supported there. Could you tell which board/configuration/ATF you are
>> using so that the implementation could be tested/validated ?
>> 
> 
> We're currently have POC made for r8a77951-ulcb-kf and
> r8a77961-salvator-xs boards. It's based on:
> Linux-bsp kernel: 
> [e-mail]:renesas-rcar/linux-bsp.git
> based on tag <rcar-5.0.0.rc4>
> 
> ATF: 
> [e-mail]:renesas-rcar/arm-trusted-firmware.git
> based on branch <rcar_gen3_v2.5>
> 
> I can push those changes to Github, so you can review them.

Do you plan to add support for other boards ?

Did you discuss more in general with the linux kernel guys to see if this
approach was agreed and will be adopted by other manufacturers ?

All in all I think this is a good idea but I fear that all this will actually only
be used by one board or one manufacturer and other might use a different
strategy, I would like to unrisk this before merging this in Xen.

@julien and Stefano: what is your view here ?

Cheers
Bertrand

> 
> Best regards,
> Oleksii.
> 
>> 
>> Regards
>> Bertrand
>> 
>> 
>>> 
>>> Signed-off-by: Oleksii Moisieiev <[e-mail]>
>>> ---
>>> xen/arch/arm/Kconfig        |   2 +
>>> xen/arch/arm/sci/Kconfig    |  10 +
>>> xen/arch/arm/sci/scmi_smc.c | 959 ++++++++++++++++++++++++++++++++++++
>>> 3 files changed, 971 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 ab07833582..3b0dfc57b6 100644
>>> --- a/xen/arch/arm/Kconfig
>>> +++ b/xen/arch/arm/Kconfig
>>> @@ -123,6 +123,8 @@ config ARM_SCI
>>> 	  support. It allows guests to control system resourcess via one of
>>> 	  ARM_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..10b634d2ed
>>> --- /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 ARM_SCI && HOST_DTB_EXPORT
>>> +	---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/scmi_smc.c b/xen/arch/arm/sci/scmi_smc.c
>>> new file mode 100644
>>> index 0000000000..103529dfab
>>> --- /dev/null
>>> +++ b/xen/arch/arm/sci/scmi_smc.c
>>> @@ -0,0 +1,959 @@
>>> +/*
>>> + * xen/arch/arm/sci/scmi_smc.c
>>> + *
>>> + * SCMI mediator driver, using SCP as transport.
>>> + *
>>> + * Oleksii Moisieiev <[e-mail]>
>>> + * 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                 "arm,scmi-shmem"
>>> +#define SCMI_SHMEM                         "shmem"
>>> +#define SCMI_SHMEM_MAPPED_SIZE             PAGE_SIZE
>>> +
>>> +#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;
>>> +
>>> +#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 dt_channel_addr {
>>> +    u64 addr;
>>> +    u64 size;
>>> +    struct list_head list;
>>> +};
>>> +
>>> +struct scmi_channel {
>>> +    int chan_id;
>>> +    int agent_id;
>>> +    uint32_t func_id;
>>> +    domid_t domain_id;
>>> +    uint64_t paddr;
>>> +    uint64_t len;
>>> +    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;
>>> +};
>>> +
>>> +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;
>>> +}
>>> +
>>> +/*
>>> + * Copy data from IO memory space to "real" memory space.
>>> + */
>>> +void __memcpy_fromio(void *to, const volatile void __iomem *from, size_t count)
>>> +{
>>> +    while (count && !IS_ALIGNED((unsigned long)from, 4)) {
>>> +        *(u8 *)to = __raw_readb(from);
>>> +        from++;
>>> +        to++;
>>> +        count--;
>>> +    }
>>> +
>>> +    while (count >= 4) {
>>> +        *(u32 *)to = __raw_readl(from);
>>> +        from += 4;
>>> +        to += 4;
>>> +        count -= 4;
>>> +    }
>>> +
>>> +    while (count) {
>>> +        *(u8 *)to = __raw_readb(from);
>>> +        from++;
>>> +        to++;
>>> +        count--;
>>> +    }
>>> +}
>>> +
>>> +/*
>>> + * Copy data from "real" memory space to IO memory space.
>>> + */
>>> +void __memcpy_toio(volatile void __iomem *to, const void *from, size_t count)
>>> +{
>>> +    while (count && !IS_ALIGNED((unsigned long)to, 4)) {
>>> +        __raw_writeb(*(u8 *)from, to);
>>> +        from++;
>>> +        to++;
>>> +        count--;
>>> +    }
>>> +
>>> +    while (count >= 4) {
>>> +        __raw_writel(*(u32 *)from, to);
>>> +        from += 4;
>>> +        to += 4;
>>> +        count -= 4;
>>> +    }
>>> +
>>> +    while (count) {
>>> +        __raw_writeb(*(u8 *)from, to);
>>> +        from++;
>>> +        to++;
>>> +        count--;
>>> +    }
>>> +}
>>> +
>>> +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;
>>> +
>>> +    if ( (len + sizeof(chan_info->shmem->msg_header)) >
>>> +                         SCMI_SHMEM_MAPPED_SIZE )
>>> +    {
>>> +        printk(XENLOG_ERR
>>> +               "scmi: Wrong size of smc message. Data is invalid\n");
>>> +        return -EINVAL;
>>> +    }
>>> +
>>> +    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_toio((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;
>>> +    default:
>>> +        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);
>>> +
>>> +    if ( len >= SCMI_SHMEM_MAPPED_SIZE - sizeof(chan_info->shmem) )
>>> +    {
>>> +        printk(XENLOG_ERR
>>> +               "scmi: Wrong size of input smc message. Data may be invalid\n");
>>> +        return -EINVAL;
>>> +    }
>>> +
>>> +    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_fromio(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;
>>> +
>>> +    ASSERT( channel && channel->shmem);
>>> +
>>> +    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 *aquire_scmi_channel(domid_t 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->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)
>>> +{
>>> +    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_cache(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 after vmap %p\n", channel->shmem);
>>> +    return 0;
>>> +}
>>> +
>>> +static void unmap_channel_memory(struct scmi_channel *channel)
>>> +{
>>> +    ASSERT( channel && channel->shmem );
>>> +    iounmap(channel->shmem);
>>> +    channel->shmem = NULL;
>>> +}
>>> +
>>> +static struct scmi_channel *smc_create_channel(uint8_t chan_id,
>>> +                                               uint32_t func_id, uint64_t addr)
>>> +{
>>> +    struct scmi_channel *channel;
>>> +
>>> +    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;
>>> +    channel->shmem = NULL;
>>> +    channel->paddr = addr;
>>> +    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 mem_permit_access(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 mem_deny_access(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(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, SCMI_SHARED_MEMORY);
>>> +    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)
>>> +    {
>>> +        list_del(&curr->list);
>>> +        xfree(curr);
>>> +    }
>>> +
>>> +    spin_unlock(&scmi_data.channel_list_lock);
>>> +}
>>> +
>>> +static struct dt_device_node *get_dt_node_from_property(
>>> +                struct dt_device_node *node, const char * p_name)
>>> +{
>>> +    const __be32 *prop;
>>> +
>>> +    ASSERT( node );
>>> +
>>> +    prop = dt_get_property(node, p_name, NULL);
>>> +    if ( !prop )
>>> +        return ERR_PTR(-EINVAL);
>>> +
>>> +    return dt_find_node_by_phandle(be32_to_cpup(prop));
>>> +}
>>> +
>>> +static int get_shmem_regions(struct list_head *head, u64 hyp_addr)
>>> +{
>>> +    struct dt_device_node *node;
>>> +    int ret;
>>> +    struct dt_channel_addr *lchan;
>>> +    u64 laddr, lsize;
>>> +
>>> +    node = dt_find_compatible_node(NULL, NULL, SCMI_SHARED_MEMORY);
>>> +    if ( !node )
>>> +        return -ENOENT;
>>> +
>>> +    while ( node )
>>> +    {
>>> +        ret = dt_device_get_address(node, 0, &laddr, &lsize);
>>> +        if ( ret )
>>> +            return ret;
>>> +
>>> +        if ( laddr != hyp_addr )
>>> +        {
>>> +            lchan = xmalloc(struct dt_channel_addr);
>>> +            if ( !lchan )
>>> +                return -ENOMEM;
>>> +            lchan->addr = laddr;
>>> +            lchan->size = lsize;
>>> +
>>> +            list_add_tail(&lchan->list, head);
>>> +        }
>>> +
>>> +        node = dt_find_compatible_node(node, NULL, SCMI_SHARED_MEMORY);
>>> +    }
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static int read_hyp_channel_addr(struct dt_device_node *scmi_node,
>>> +                                 u64 *addr, u64 *size)
>>> +{
>>> +    struct dt_device_node *shmem_node;
>>> +    shmem_node = get_dt_node_from_property(scmi_node, "shmem");
>>> +    if ( IS_ERR_OR_NULL(shmem_node) )
>>> +    {
>>> +        printk(XENLOG_ERR
>>> +               "scmi: Device tree error, can't parse reserved memory %ld\n",
>>> +               PTR_ERR(shmem_node));
>>> +        return PTR_ERR(shmem_node);
>>> +    }
>>> +
>>> +    return dt_device_get_address(shmem_node, 0, addr, size);
>>> +}
>>> +
>>> +static void free_shmem_regions(struct list_head *addr_list)
>>> +{
>>> +    struct dt_channel_addr *curr, *_curr;
>>> +
>>> +    list_for_each_entry_safe (curr, _curr, addr_list, list)
>>> +    {
>>> +        list_del(&curr->list);
>>> +        xfree(curr);
>>> +    }
>>> +}
>>> +
>>> +static __init bool scmi_probe(struct dt_device_node *scmi_node)
>>> +{
>>> +    u64 addr, size;
>>> +    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;
>>> +    struct dt_channel_addr *entry;
>>> +    struct list_head addr_list;
>>> +
>>> +    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;
>>> +    }
>>> +
>>> +    ret = read_hyp_channel_addr(scmi_node, &addr, &size);
>>> +    if ( IS_ERR_VALUE(ret) )
>>> +        return false;
>>> +
>>> +    if ( !IS_ALIGNED(size, SCMI_SHMEM_MAPPED_SIZE) )
>>> +    {
>>> +        printk(XENLOG_ERR "scmi: Reserved memory is not aligned\n");
>>> +        return false;
>>> +    }
>>> +
>>> +    INIT_LIST_HEAD(&addr_list);
>>> +
>>> +    ret = get_shmem_regions(&addr_list, addr);
>>> +    if ( IS_ERR_VALUE(ret) )
>>> +        goto out;
>>> +
>>> +    channel = smc_create_channel(HYP_CHANNEL, func_id, addr);
>>> +    if ( IS_ERR(channel) )
>>> +        goto out;
>>> +
>>> +    ret = map_channel_memory(channel);
>>> +    if ( ret )
>>> +        goto out;
>>> +
>>> +    spin_lock(&scmi_data.channel_list_lock);
>>> +    channel->domain_id = DOMID_XEN;
>>> +    spin_unlock(&scmi_data.channel_list_lock);
>>> +
>>> +    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;
>>> +
>>> +    ret = check_scmi_status(rx.status);
>>> +    if ( ret )
>>> +        goto error;
>>> +
>>> +    n_agents = FIELD_GET(MSG_N_AGENTS_MASK, rx.attributes);
>>> +    printk(XENLOG_DEBUG "scmi: Got agent count %d\n", n_agents);
>>> +
>>> +    i = 1;
>>> +    list_for_each_entry(entry, &addr_list, list)
>>> +    {
>>> +        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,
>>> +                                           entry->addr);
>>> +        if ( IS_ERR(agent_channel) )
>>> +        {
>>> +            ret = PTR_ERR(agent_channel);
>>> +            goto error;
>>> +        }
>>> +
>>> +        ret = map_channel_memory(agent_channel);
>>> +        if ( ret )
>>> +            goto error;
>>> +
>>> +        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 )
>>> +        {
>>> +            unmap_channel_memory(agent_channel);
>>> +            goto error;
>>> +        }
>>> +
>>> +        unmap_channel_memory(agent_channel);
>>> +
>>> +        ret = check_scmi_status(da_rx.status);
>>> +        if ( ret )
>>> +            goto error;
>>> +
>>> +        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;
>>> +
>>> +        if ( i == n_agents )
>>> +            break;
>>> +
>>> +        i++;
>>> +    }
>>> +
>>> +    scmi_data.initialized = true;
>>> +    goto out;
>>> +
>>> +error:
>>> +    unmap_channel_memory(channel);
>>> +    free_channel_list();
>>> +out:
>>> +    free_shmem_regions(&addr_list);
>>> +    return ret == 0;
>>> +}
>>> +
>>> +static int scmi_domain_init(struct domain *d,
>>> +                           struct xen_arch_domainconfig *config)
>>> +{
>>> +    struct scmi_channel *channel;
>>> +    int ret;
>>> +
>>> +    if ( !scmi_data.initialized )
>>> +        return 0;
>>> +
>>> +    printk(XENLOG_INFO "scmi: domain_id = %d\n", d->domain_id);
>>> +
>>> +    channel = aquire_scmi_channel(d->domain_id);
>>> +    if ( IS_ERR_OR_NULL(channel) )
>>> +        return -ENOENT;
>>> +
>>> +#ifdef CONFIG_ARM_32
>>> +    printk(XENLOG_INFO
>>> +           "scmi: Aquire SCMI channel id = 0x%x , domain_id = %d paddr = 0x%llx\n",
>>> +           channel->chan_id, channel->domain_id, channel->paddr);
>>> +#else
>>> +    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);
>>> +#endif
>>> +
>>> +    if ( is_hardware_domain(d) )
>>> +    {
>>> +        ret = mem_permit_access(d, channel->paddr, PAGE_SIZE);
>>> +        if ( IS_ERR_VALUE(ret) )
>>> +            goto error;
>>> +
>>> +        ret = dt_update_domain_range(channel->paddr, PAGE_SIZE);
>>> +        if ( IS_ERR_VALUE(ret) )
>>> +        {
>>> +            int rc = mem_deny_access(d, channel->paddr, PAGE_SIZE);
>>> +            if ( rc )
>>> +                printk(XENLOG_ERR "Unable to mem_deny_access\n");
>>> +
>>> +            goto error;
>>> +        }
>>> +    }
>>> +
>>> +    d->arch.sci = channel;
>>> +    if ( config )
>>> +        config->arm_sci_agent_paddr = channel->paddr;
>>> +
>>> +    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;
>>> +    struct scmi_perms_tx {
>>> +        uint32_t agent_id;
>>> +        uint32_t device_id;
>>> +        uint32_t flags;
>>> +    } 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 = d->arch.sci;
>>> +    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;
>>> +
>>> +    mem_deny_access(d, channel->paddr, PAGE_SIZE);
>>> +    spin_unlock(&channel->lock);
>>> +}
>>> +
>>> +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->r0 )
>>> +    {
>>> +        res = false;
>>> +        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 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,
>>> +};
>>> +
>>> +REGISTER_SCI_MEDIATOR(scmi_smc, "SCMI-SMC", XEN_DOMCTL_CONFIG_ARM_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:
>>> + */
>>> -- 
>>> 2.27.0

Oleksii Moisieiev Feb. 11, 2022, 11:55 a.m. UTC | #6

On Fri, Feb 11, 2022 at 11:18:47AM +0000, Bertrand Marquis wrote:
> Hi Oleksii,
> 
> 
> > On 11 Feb 2022, at 10:44, Oleksii Moisieiev <[e-mail]> wrote:
> > 
> > Hi Bertrand,
> > 
> > On Fri, Feb 11, 2022 at 08:46:05AM +0000, Bertrand Marquis wrote:
> >> Hi Oleksii,
> >> 
> >> 
> >>> On 8 Feb 2022, at 18:00, Oleksii Moisieiev <[e-mail]> 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.
> >>> 
> >>> Originally, cpg should be passed to the domain so it can work with
> >>> power-domains/clocks/resets etc. Considering that cpg can't be split between
> >>> the Domains, we get the limitation that the devices, which are using
> >>> power-domains/clocks/resets etc, couldn't be split between the domains.
> >>> The solution is to move the power-domain/clock/resets etc to the
> >>> Firmware (such as SCP firmware or ATF) and provide interface for the
> >>> Domains. XEN should have an entity, caled SCI-Mediator, which is
> >>> responsible for messages redirection between Domains and Firmware and
> >>> for permission handling.
> >>> 
> >>> 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.
> >> 
> >> Before going more deeply in the code I would like to discuss the general
> >> design here and ask some questions to prevent to rework the code before
> >> we all agree that this is the right solution and that we want this in Xen.
> >> 
> >> First I want to point out that clock/reset/power virtualization is a problem
> >> on most applications using device pass-through and I am very glad that
> >> someone is looking into it.
> >> Also SCMI is the current standard existing for this so relying on it is a very
> >> good idea.
> >> 
> >> Latest version SCMI standard (DEN0056D v3.1) is defining some means
> >> to use SCMI on a virtualised system. In chapter 4.2.1, the standard
> >> recommends to set permissions per agent in the hypervisor so that a VM
> >> could later use the discovery protocol to detect the resources and use them.
> >> Using this kind of scenario the mediator in Xen would just configure the
> >> Permissions in the SCMI and would then rely on it to limit what is possible
> >> by who just by just assigning a channel to a VM.
> > 
> >> 
> >> In your current design (please correct me if I am wrong) you seem to fully
> >> rely on Xen and the FDT for discovery and permission.
> > 
> > In current implementation Xen is the trusted agent. And it's responsible
> > for permissions setting. During initialization it discovers agent and
> > set permissions by using BASE_SET_DEVICE_PERMISSIONS to the Dom0. When
> > new domain is created, Xen assigns agent id for this domain and request
> > resources, that are passed-through to this Domain.
> 
> Ok
> 
> > 
> > I'm getting the follwing information from FDT:
> > 1) Shared memory addressed, which should be used for agents. During
> > initialization I send BASE_DISCOVER_AGENT to each of this addresses and
> > receive agent_id. Xen is responsible for assigning agent_id for the
> > Domain. Then Xen intercept smc calls from the domain, set agent_id and
> > redirects it to the Firmware.
> 
> So Xen is setting the agent ID, no way for a guest to get access to something it
> should with more check, am I right ?
> 

Yes. Xen is the only entity, which is trusted. So it's responsible for
setting permissions and assigning agent_id. Guest get's an access only
for the devices it's allowed to.

> > 
> > 2) Devices, that are using SCMI. Those devices has clock/power/resets
> > etc related to scmi protocol (as it is done in Linux kernel)
> > and scmi_devid should be set. I'm currently preparing to send patch,
> > updating kernel bindings with this parameter to Linux kernel.
> > scmi_devid value should match device id, set in the Firmware.
> > dt example:
> > &usb0 {
> >    scmi_devid = <1>; // usb0 device id
> >    clocks = <&scmi_clock 1> // relays on clock with id 1
> > }
> > 
> > Xen requests permission for the device when device is attached to the
> > Domain during creation.
> 
> Without this, how is (if it is) the linux kernel using SCMI for power management ?

Here is how it should be desribed in FDT: 
/
{
    firmware {
        scmi {
            arm,smc-id = <0x82000002>;
            scmi_power: protocol@11 {
                reg = <0x11>;
                #power-domain-cells = <1>;
            };
            ...
            scmi_clock: protocol@14 {
            ...
            scmi_reset: protocol@16 {
            ...
        };
    };
};

&avb {
    scmi_devid = <0>; // Matches Etherned device_id in Firmware
    clocks = <&scmi_clock 0>;
    power-domains = <&scmi_power 0>;
    resets = <&scmi_reset 0>;
};

In the provided case devid equals to reset, clock and power-domain id,
but this is conicidence. Each clock/power-domain/reset parameter can
have more than one entity.
Also - no changes was done to linux kernel scmi drivers.

> 
> > 
> >> Wouldn’t it be a better idea to use the protocol fully ?
> > 
> > Hm, I was thinking I am using the protocol fully. Did I miss something?
> 
> Sorry you seem to be, my understanding of your design was not right.
> 
> > 
> >> Could we get rid of some of the FDT dependencies by using the discovery
> >> system of SCMI ?
> > 
> > I'm using FDT to get shmem regions for the channels. Then I send
> > BASE_DISCOVER_AGENT to each region and getting agent data. Did I use the
> > discovery system wrong?
> 
> After more digging it seems you are. The link between scmi resource and device
> is not possible to get automatically.
> 
> > 
> >> How is Linux doing this currently ? Is it relying on device tree to discover
> >> the SCMI resources ?
> > 
> > Yes. Linux kernel has 2 nodes in the device-tree: arm,scmi-shmem, which
> > includes memory region for the communication and arm,scmi-smc node,
> > which describes all data related to scmi ( func_id, protocols etc)
> > Then the device nodes refer to the protocols by setting
> > clock/resets/power-domains etc. Please see the example above.
> > BASE_DISCOVER_AGENT is not used in Linux kernel.
> > The main idea was that scmi related changes to the device-tree are
> > common for virtualized and non virtualized systems. So the same FDT
> > configuration should work with of without Xen.
> 
> So at this stage this is not supported in Linux and you plan to add support for it to.
> 

Yes. That's correct. I've already prepared patch which should update
linux kernel device-tree bindings.

> > 
> >> 
> >> Also I understand that you rely on some entries to be declared in the device
> >> tree and also some support to be implemented in ATF or SCP. I checked in
> >> The boards I have access to and the device trees but none of this seem to
> >> be supported there. Could you tell which board/configuration/ATF you are
> >> using so that the implementation could be tested/validated ?
> >> 
> > 
> > We're currently have POC made for r8a77951-ulcb-kf and
> > r8a77961-salvator-xs boards. It's based on:
> > Linux-bsp kernel: 
> > [e-mail]:renesas-rcar/linux-bsp.git
> > based on tag <rcar-5.0.0.rc4>
> > 
> > ATF: 
> > [e-mail]:renesas-rcar/arm-trusted-firmware.git
> > based on branch <rcar_gen3_v2.5>
> > 
> > I can push those changes to Github, so you can review them
> 
> Do you plan to add support for other boards ?
> 

Right now we're working only with r8a77951 and r8a77961 boards.

> Did you discuss more in general with the linux kernel guys to see if this
> approach was agreed and will be adopted by other manufacturers ?

I didn't. I've contacted Sudeep Holla <[e-mail]>, who is the
maintainer of the SCMI protocol drivers. Waiting for the response.

Also we proposed to add Pinctl support to SCMI specification. It was
agreed and should be added to SCMI protocol in SCMIv3.2 (due end-2022/early 2023).

> 
> All in all I think this is a good idea but I fear that all this will actually only
> be used by one board or one manufacturer and other might use a different
> strategy, I would like to unrisk this before merging this in Xen.

The main idea was to make Xen SCMI mediator completely transparent from
the Domain point of view. So there is no Xen specific changes should be
done to OS pinctrl drivers to work through SCMI.

This means that all platforms, that already using SCMI can work with it
in virtualized system.

Also the advantage is that the devices, passed-through the
Domains, which doesn't have an access to CPG, can access to
clocks/resets and power-domains. We already have Pinctl protocol POC, so
the devices from different Domains can access pins either.

--
Oleksii.
> 
> @julien and Stefano: what is your view here ?
> 
> Cheers
> Bertrand
> 
> > 
> > Best regards,
> > Oleksii.
> > 
> >> 
> >> Regards
> >> Bertrand
> >> 
> >> 
> >>> 
> >>> Signed-off-by: Oleksii Moisieiev <[e-mail]>
> >>> ---
> >>> xen/arch/arm/Kconfig        |   2 +
> >>> xen/arch/arm/sci/Kconfig    |  10 +
> >>> xen/arch/arm/sci/scmi_smc.c | 959 ++++++++++++++++++++++++++++++++++++
> >>> 3 files changed, 971 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 ab07833582..3b0dfc57b6 100644
> >>> --- a/xen/arch/arm/Kconfig
> >>> +++ b/xen/arch/arm/Kconfig
> >>> @@ -123,6 +123,8 @@ config ARM_SCI
> >>> 	  support. It allows guests to control system resourcess via one of
> >>> 	  ARM_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..10b634d2ed
> >>> --- /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 ARM_SCI && HOST_DTB_EXPORT
> >>> +	---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/scmi_smc.c b/xen/arch/arm/sci/scmi_smc.c
> >>> new file mode 100644
> >>> index 0000000000..103529dfab
> >>> --- /dev/null
> >>> +++ b/xen/arch/arm/sci/scmi_smc.c
> >>> @@ -0,0 +1,959 @@
> >>> +/*
> >>> + * xen/arch/arm/sci/scmi_smc.c
> >>> + *
> >>> + * SCMI mediator driver, using SCP as transport.
> >>> + *
> >>> + * Oleksii Moisieiev <[e-mail]>
> >>> + * 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                 "arm,scmi-shmem"
> >>> +#define SCMI_SHMEM                         "shmem"
> >>> +#define SCMI_SHMEM_MAPPED_SIZE             PAGE_SIZE
> >>> +
> >>> +#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;
> >>> +
> >>> +#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 dt_channel_addr {
> >>> +    u64 addr;
> >>> +    u64 size;
> >>> +    struct list_head list;
> >>> +};
> >>> +
> >>> +struct scmi_channel {
> >>> +    int chan_id;
> >>> +    int agent_id;
> >>> +    uint32_t func_id;
> >>> +    domid_t domain_id;
> >>> +    uint64_t paddr;
> >>> +    uint64_t len;
> >>> +    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;
> >>> +};
> >>> +
> >>> +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;
> >>> +}
> >>> +
> >>> +/*
> >>> + * Copy data from IO memory space to "real" memory space.
> >>> + */
> >>> +void __memcpy_fromio(void *to, const volatile void __iomem *from, size_t count)
> >>> +{
> >>> +    while (count && !IS_ALIGNED((unsigned long)from, 4)) {
> >>> +        *(u8 *)to = __raw_readb(from);
> >>> +        from++;
> >>> +        to++;
> >>> +        count--;
> >>> +    }
> >>> +
> >>> +    while (count >= 4) {
> >>> +        *(u32 *)to = __raw_readl(from);
> >>> +        from += 4;
> >>> +        to += 4;
> >>> +        count -= 4;
> >>> +    }
> >>> +
> >>> +    while (count) {
> >>> +        *(u8 *)to = __raw_readb(from);
> >>> +        from++;
> >>> +        to++;
> >>> +        count--;
> >>> +    }
> >>> +}
> >>> +
> >>> +/*
> >>> + * Copy data from "real" memory space to IO memory space.
> >>> + */
> >>> +void __memcpy_toio(volatile void __iomem *to, const void *from, size_t count)
> >>> +{
> >>> +    while (count && !IS_ALIGNED((unsigned long)to, 4)) {
> >>> +        __raw_writeb(*(u8 *)from, to);
> >>> +        from++;
> >>> +        to++;
> >>> +        count--;
> >>> +    }
> >>> +
> >>> +    while (count >= 4) {
> >>> +        __raw_writel(*(u32 *)from, to);
> >>> +        from += 4;
> >>> +        to += 4;
> >>> +        count -= 4;
> >>> +    }
> >>> +
> >>> +    while (count) {
> >>> +        __raw_writeb(*(u8 *)from, to);
> >>> +        from++;
> >>> +        to++;
> >>> +        count--;
> >>> +    }
> >>> +}
> >>> +
> >>> +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;
> >>> +
> >>> +    if ( (len + sizeof(chan_info->shmem->msg_header)) >
> >>> +                         SCMI_SHMEM_MAPPED_SIZE )
> >>> +    {
> >>> +        printk(XENLOG_ERR
> >>> +               "scmi: Wrong size of smc message. Data is invalid\n");
> >>> +        return -EINVAL;
> >>> +    }
> >>> +
> >>> +    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_toio((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;
> >>> +    default:
> >>> +        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);
> >>> +
> >>> +    if ( len >= SCMI_SHMEM_MAPPED_SIZE - sizeof(chan_info->shmem) )
> >>> +    {
> >>> +        printk(XENLOG_ERR
> >>> +               "scmi: Wrong size of input smc message. Data may be invalid\n");
> >>> +        return -EINVAL;
> >>> +    }
> >>> +
> >>> +    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_fromio(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;
> >>> +
> >>> +    ASSERT( channel && channel->shmem);
> >>> +
> >>> +    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 *aquire_scmi_channel(domid_t 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->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)
> >>> +{
> >>> +    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_cache(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 after vmap %p\n", channel->shmem);
> >>> +    return 0;
> >>> +}
> >>> +
> >>> +static void unmap_channel_memory(struct scmi_channel *channel)
> >>> +{
> >>> +    ASSERT( channel && channel->shmem );
> >>> +    iounmap(channel->shmem);
> >>> +    channel->shmem = NULL;
> >>> +}
> >>> +
> >>> +static struct scmi_channel *smc_create_channel(uint8_t chan_id,
> >>> +                                               uint32_t func_id, uint64_t addr)
> >>> +{
> >>> +    struct scmi_channel *channel;
> >>> +
> >>> +    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;
> >>> +    channel->shmem = NULL;
> >>> +    channel->paddr = addr;
> >>> +    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 mem_permit_access(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 mem_deny_access(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(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, SCMI_SHARED_MEMORY);
> >>> +    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)
> >>> +    {
> >>> +        list_del(&curr->list);
> >>> +        xfree(curr);
> >>> +    }
> >>> +
> >>> +    spin_unlock(&scmi_data.channel_list_lock);
> >>> +}
> >>> +
> >>> +static struct dt_device_node *get_dt_node_from_property(
> >>> +                struct dt_device_node *node, const char * p_name)
> >>> +{
> >>> +    const __be32 *prop;
> >>> +
> >>> +    ASSERT( node );
> >>> +
> >>> +    prop = dt_get_property(node, p_name, NULL);
> >>> +    if ( !prop )
> >>> +        return ERR_PTR(-EINVAL);
> >>> +
> >>> +    return dt_find_node_by_phandle(be32_to_cpup(prop));
> >>> +}
> >>> +
> >>> +static int get_shmem_regions(struct list_head *head, u64 hyp_addr)
> >>> +{
> >>> +    struct dt_device_node *node;
> >>> +    int ret;
> >>> +    struct dt_channel_addr *lchan;
> >>> +    u64 laddr, lsize;
> >>> +
> >>> +    node = dt_find_compatible_node(NULL, NULL, SCMI_SHARED_MEMORY);
> >>> +    if ( !node )
> >>> +        return -ENOENT;
> >>> +
> >>> +    while ( node )
> >>> +    {
> >>> +        ret = dt_device_get_address(node, 0, &laddr, &lsize);
> >>> +        if ( ret )
> >>> +            return ret;
> >>> +
> >>> +        if ( laddr != hyp_addr )
> >>> +        {
> >>> +            lchan = xmalloc(struct dt_channel_addr);
> >>> +            if ( !lchan )
> >>> +                return -ENOMEM;
> >>> +            lchan->addr = laddr;
> >>> +            lchan->size = lsize;
> >>> +
> >>> +            list_add_tail(&lchan->list, head);
> >>> +        }
> >>> +
> >>> +        node = dt_find_compatible_node(node, NULL, SCMI_SHARED_MEMORY);
> >>> +    }
> >>> +
> >>> +    return 0;
> >>> +}
> >>> +
> >>> +static int read_hyp_channel_addr(struct dt_device_node *scmi_node,
> >>> +                                 u64 *addr, u64 *size)
> >>> +{
> >>> +    struct dt_device_node *shmem_node;
> >>> +    shmem_node = get_dt_node_from_property(scmi_node, "shmem");
> >>> +    if ( IS_ERR_OR_NULL(shmem_node) )
> >>> +    {
> >>> +        printk(XENLOG_ERR
> >>> +               "scmi: Device tree error, can't parse reserved memory %ld\n",
> >>> +               PTR_ERR(shmem_node));
> >>> +        return PTR_ERR(shmem_node);
> >>> +    }
> >>> +
> >>> +    return dt_device_get_address(shmem_node, 0, addr, size);
> >>> +}
> >>> +
> >>> +static void free_shmem_regions(struct list_head *addr_list)
> >>> +{
> >>> +    struct dt_channel_addr *curr, *_curr;
> >>> +
> >>> +    list_for_each_entry_safe (curr, _curr, addr_list, list)
> >>> +    {
> >>> +        list_del(&curr->list);
> >>> +        xfree(curr);
> >>> +    }
> >>> +}
> >>> +
> >>> +static __init bool scmi_probe(struct dt_device_node *scmi_node)
> >>> +{
> >>> +    u64 addr, size;
> >>> +    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;
> >>> +    struct dt_channel_addr *entry;
> >>> +    struct list_head addr_list;
> >>> +
> >>> +    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;
> >>> +    }
> >>> +
> >>> +    ret = read_hyp_channel_addr(scmi_node, &addr, &size);
> >>> +    if ( IS_ERR_VALUE(ret) )
> >>> +        return false;
> >>> +
> >>> +    if ( !IS_ALIGNED(size, SCMI_SHMEM_MAPPED_SIZE) )
> >>> +    {
> >>> +        printk(XENLOG_ERR "scmi: Reserved memory is not aligned\n");
> >>> +        return false;
> >>> +    }
> >>> +
> >>> +    INIT_LIST_HEAD(&addr_list);
> >>> +
> >>> +    ret = get_shmem_regions(&addr_list, addr);
> >>> +    if ( IS_ERR_VALUE(ret) )
> >>> +        goto out;
> >>> +
> >>> +    channel = smc_create_channel(HYP_CHANNEL, func_id, addr);
> >>> +    if ( IS_ERR(channel) )
> >>> +        goto out;
> >>> +
> >>> +    ret = map_channel_memory(channel);
> >>> +    if ( ret )
> >>> +        goto out;
> >>> +
> >>> +    spin_lock(&scmi_data.channel_list_lock);
> >>> +    channel->domain_id = DOMID_XEN;
> >>> +    spin_unlock(&scmi_data.channel_list_lock);
> >>> +
> >>> +    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;
> >>> +
> >>> +    ret = check_scmi_status(rx.status);
> >>> +    if ( ret )
> >>> +        goto error;
> >>> +
> >>> +    n_agents = FIELD_GET(MSG_N_AGENTS_MASK, rx.attributes);
> >>> +    printk(XENLOG_DEBUG "scmi: Got agent count %d\n", n_agents);
> >>> +
> >>> +    i = 1;
> >>> +    list_for_each_entry(entry, &addr_list, list)
> >>> +    {
> >>> +        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,
> >>> +                                           entry->addr);
> >>> +        if ( IS_ERR(agent_channel) )
> >>> +        {
> >>> +            ret = PTR_ERR(agent_channel);
> >>> +            goto error;
> >>> +        }
> >>> +
> >>> +        ret = map_channel_memory(agent_channel);
> >>> +        if ( ret )
> >>> +            goto error;
> >>> +
> >>> +        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 )
> >>> +        {
> >>> +            unmap_channel_memory(agent_channel);
> >>> +            goto error;
> >>> +        }
> >>> +
> >>> +        unmap_channel_memory(agent_channel);
> >>> +
> >>> +        ret = check_scmi_status(da_rx.status);
> >>> +        if ( ret )
> >>> +            goto error;
> >>> +
> >>> +        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;
> >>> +
> >>> +        if ( i == n_agents )
> >>> +            break;
> >>> +
> >>> +        i++;
> >>> +    }
> >>> +
> >>> +    scmi_data.initialized = true;
> >>> +    goto out;
> >>> +
> >>> +error:
> >>> +    unmap_channel_memory(channel);
> >>> +    free_channel_list();
> >>> +out:
> >>> +    free_shmem_regions(&addr_list);
> >>> +    return ret == 0;
> >>> +}
> >>> +
> >>> +static int scmi_domain_init(struct domain *d,
> >>> +                           struct xen_arch_domainconfig *config)
> >>> +{
> >>> +    struct scmi_channel *channel;
> >>> +    int ret;
> >>> +
> >>> +    if ( !scmi_data.initialized )
> >>> +        return 0;
> >>> +
> >>> +    printk(XENLOG_INFO "scmi: domain_id = %d\n", d->domain_id);
> >>> +
> >>> +    channel = aquire_scmi_channel(d->domain_id);
> >>> +    if ( IS_ERR_OR_NULL(channel) )
> >>> +        return -ENOENT;
> >>> +
> >>> +#ifdef CONFIG_ARM_32
> >>> +    printk(XENLOG_INFO
> >>> +           "scmi: Aquire SCMI channel id = 0x%x , domain_id = %d paddr = 0x%llx\n",
> >>> +           channel->chan_id, channel->domain_id, channel->paddr);
> >>> +#else
> >>> +    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);
> >>> +#endif
> >>> +
> >>> +    if ( is_hardware_domain(d) )
> >>> +    {
> >>> +        ret = mem_permit_access(d, channel->paddr, PAGE_SIZE);
> >>> +        if ( IS_ERR_VALUE(ret) )
> >>> +            goto error;
> >>> +
> >>> +        ret = dt_update_domain_range(channel->paddr, PAGE_SIZE);
> >>> +        if ( IS_ERR_VALUE(ret) )
> >>> +        {
> >>> +            int rc = mem_deny_access(d, channel->paddr, PAGE_SIZE);
> >>> +            if ( rc )
> >>> +                printk(XENLOG_ERR "Unable to mem_deny_access\n");
> >>> +
> >>> +            goto error;
> >>> +        }
> >>> +    }
> >>> +
> >>> +    d->arch.sci = channel;
> >>> +    if ( config )
> >>> +        config->arm_sci_agent_paddr = channel->paddr;
> >>> +
> >>> +    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;
> >>> +    struct scmi_perms_tx {
> >>> +        uint32_t agent_id;
> >>> +        uint32_t device_id;
> >>> +        uint32_t flags;
> >>> +    } 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 = d->arch.sci;
> >>> +    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;
> >>> +
> >>> +    mem_deny_access(d, channel->paddr, PAGE_SIZE);
> >>> +    spin_unlock(&channel->lock);
> >>> +}
> >>> +
> >>> +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->r0 )
> >>> +    {
> >>> +        res = false;
> >>> +        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 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,
> >>> +};
> >>> +
> >>> +REGISTER_SCI_MEDIATOR(scmi_smc, "SCMI-SMC", XEN_DOMCTL_CONFIG_ARM_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:
> >>> + */
> >>> -- 
> >>> 2.27.0
>

Stefano Stabellini Feb. 11, 2022, 11:35 p.m. UTC | #7

On Fri, 11 Feb 2022, Oleksii Moisieiev wrote:
> On Fri, Feb 11, 2022 at 11:18:47AM +0000, Bertrand Marquis wrote:
> > Hi Oleksii,
> > 
> > 
> > > On 11 Feb 2022, at 10:44, Oleksii Moisieiev <[e-mail]> wrote:
> > > 
> > > Hi Bertrand,
> > > 
> > > On Fri, Feb 11, 2022 at 08:46:05AM +0000, Bertrand Marquis wrote:
> > >> Hi Oleksii,
> > >> 
> > >> 
> > >>> On 8 Feb 2022, at 18:00, Oleksii Moisieiev <[e-mail]> 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.
> > >>> 
> > >>> Originally, cpg should be passed to the domain so it can work with
> > >>> power-domains/clocks/resets etc. Considering that cpg can't be split between
> > >>> the Domains, we get the limitation that the devices, which are using
> > >>> power-domains/clocks/resets etc, couldn't be split between the domains.
> > >>> The solution is to move the power-domain/clock/resets etc to the
> > >>> Firmware (such as SCP firmware or ATF) and provide interface for the
> > >>> Domains. XEN should have an entity, caled SCI-Mediator, which is
> > >>> responsible for messages redirection between Domains and Firmware and
> > >>> for permission handling.
> > >>> 
> > >>> 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.
> > >> 
> > >> Before going more deeply in the code I would like to discuss the general
> > >> design here and ask some questions to prevent to rework the code before
> > >> we all agree that this is the right solution and that we want this in Xen.
> > >> 
> > >> First I want to point out that clock/reset/power virtualization is a problem
> > >> on most applications using device pass-through and I am very glad that
> > >> someone is looking into it.
> > >> Also SCMI is the current standard existing for this so relying on it is a very
> > >> good idea.
> > >> 
> > >> Latest version SCMI standard (DEN0056D v3.1) is defining some means
> > >> to use SCMI on a virtualised system. In chapter 4.2.1, the standard
> > >> recommends to set permissions per agent in the hypervisor so that a VM
> > >> could later use the discovery protocol to detect the resources and use them.
> > >> Using this kind of scenario the mediator in Xen would just configure the
> > >> Permissions in the SCMI and would then rely on it to limit what is possible
> > >> by who just by just assigning a channel to a VM.
> > > 
> > >> 
> > >> In your current design (please correct me if I am wrong) you seem to fully
> > >> rely on Xen and the FDT for discovery and permission.
> > > 
> > > In current implementation Xen is the trusted agent. And it's responsible
> > > for permissions setting. During initialization it discovers agent and
> > > set permissions by using BASE_SET_DEVICE_PERMISSIONS to the Dom0. When
> > > new domain is created, Xen assigns agent id for this domain and request
> > > resources, that are passed-through to this Domain.
> > 
> > Ok
> > 
> > > 
> > > I'm getting the follwing information from FDT:
> > > 1) Shared memory addressed, which should be used for agents. During
> > > initialization I send BASE_DISCOVER_AGENT to each of this addresses and
> > > receive agent_id. Xen is responsible for assigning agent_id for the
> > > Domain. Then Xen intercept smc calls from the domain, set agent_id and
> > > redirects it to the Firmware.
> > 
> > So Xen is setting the agent ID, no way for a guest to get access to something it
> > should with more check, am I right ?
> > 
> 
> Yes. Xen is the only entity, which is trusted. So it's responsible for
> setting permissions and assigning agent_id. Guest get's an access only
> for the devices it's allowed to.
> 
> > > 
> > > 2) Devices, that are using SCMI. Those devices has clock/power/resets
> > > etc related to scmi protocol (as it is done in Linux kernel)
> > > and scmi_devid should be set. I'm currently preparing to send patch,
> > > updating kernel bindings with this parameter to Linux kernel.
> > > scmi_devid value should match device id, set in the Firmware.
> > > dt example:
> > > &usb0 {
> > >    scmi_devid = <1>; // usb0 device id
> > >    clocks = <&scmi_clock 1> // relays on clock with id 1
> > > }
> > > 
> > > Xen requests permission for the device when device is attached to the
> > > Domain during creation.
> > 
> > Without this, how is (if it is) the linux kernel using SCMI for power management ?
> 
> Here is how it should be desribed in FDT: 
> /
> {
>     firmware {
>         scmi {
>             arm,smc-id = <0x82000002>;
>             scmi_power: protocol@11 {
>                 reg = <0x11>;
>                 #power-domain-cells = <1>;
>             };
>             ...
>             scmi_clock: protocol@14 {
>             ...
>             scmi_reset: protocol@16 {
>             ...
>         };
>     };
> };
> 
> &avb {
>     scmi_devid = <0>; // Matches Etherned device_id in Firmware
>     clocks = <&scmi_clock 0>;
>     power-domains = <&scmi_power 0>;
>     resets = <&scmi_reset 0>;
> };
> 
> In the provided case devid equals to reset, clock and power-domain id,
> but this is conicidence. Each clock/power-domain/reset parameter can
> have more than one entity.
> Also - no changes was done to linux kernel scmi drivers.
> 
> > 
> > > 
> > >> Wouldn’t it be a better idea to use the protocol fully ?
> > > 
> > > Hm, I was thinking I am using the protocol fully. Did I miss something?
> > 
> > Sorry you seem to be, my understanding of your design was not right.
> > 
> > > 
> > >> Could we get rid of some of the FDT dependencies by using the discovery
> > >> system of SCMI ?
> > > 
> > > I'm using FDT to get shmem regions for the channels. Then I send
> > > BASE_DISCOVER_AGENT to each region and getting agent data. Did I use the
> > > discovery system wrong?
> > 
> > After more digging it seems you are. The link between scmi resource and device
> > is not possible to get automatically.
> > 
> > > 
> > >> How is Linux doing this currently ? Is it relying on device tree to discover
> > >> the SCMI resources ?
> > > 
> > > Yes. Linux kernel has 2 nodes in the device-tree: arm,scmi-shmem, which
> > > includes memory region for the communication and arm,scmi-smc node,
> > > which describes all data related to scmi ( func_id, protocols etc)
> > > Then the device nodes refer to the protocols by setting
> > > clock/resets/power-domains etc. Please see the example above.
> > > BASE_DISCOVER_AGENT is not used in Linux kernel.
> > > The main idea was that scmi related changes to the device-tree are
> > > common for virtualized and non virtualized systems. So the same FDT
> > > configuration should work with of without Xen.
> > 
> > So at this stage this is not supported in Linux and you plan to add support for it to.
> > 
> 
> Yes. That's correct. I've already prepared patch which should update
> linux kernel device-tree bindings.
> 
> > > 
> > >> 
> > >> Also I understand that you rely on some entries to be declared in the device
> > >> tree and also some support to be implemented in ATF or SCP. I checked in
> > >> The boards I have access to and the device trees but none of this seem to
> > >> be supported there. Could you tell which board/configuration/ATF you are
> > >> using so that the implementation could be tested/validated ?
> > >> 
> > > 
> > > We're currently have POC made for r8a77951-ulcb-kf and
> > > r8a77961-salvator-xs boards. It's based on:
> > > Linux-bsp kernel: 
> > > [e-mail]:renesas-rcar/linux-bsp.git
> > > based on tag <rcar-5.0.0.rc4>
> > > 
> > > ATF: 
> > > [e-mail]:renesas-rcar/arm-trusted-firmware.git
> > > based on branch <rcar_gen3_v2.5>
> > > 
> > > I can push those changes to Github, so you can review them
> > 
> > Do you plan to add support for other boards ?
> > 
> 
> Right now we're working only with r8a77951 and r8a77961 boards.
> 
> > Did you discuss more in general with the linux kernel guys to see if this
> > approach was agreed and will be adopted by other manufacturers ?
> 
> I didn't. I've contacted Sudeep Holla <[e-mail]>, who is the
> maintainer of the SCMI protocol drivers. Waiting for the response.
> 
> Also we proposed to add Pinctl support to SCMI specification. It was
> agreed and should be added to SCMI protocol in SCMIv3.2 (due end-2022/early 2023).
> 
> > 
> > All in all I think this is a good idea but I fear that all this will actually only
> > be used by one board or one manufacturer and other might use a different
> > strategy, I would like to unrisk this before merging this in Xen.
> 
> The main idea was to make Xen SCMI mediator completely transparent from
> the Domain point of view. So there is no Xen specific changes should be
> done to OS pinctrl drivers to work through SCMI.
> 
> This means that all platforms, that already using SCMI can work with it
> in virtualized system.

I like this statement. The aim of this series should not be just one
board, but it should be able to easily support any board with an SCMI
interface. For it to work, any changes to device tree interfaces should
be done in upstream device tree
(linux.git/Documentation/devicetree/bindings and/or devicetree.org).

Xilinx doesn't make use of SCMI yet. We are currently using an older
firmware interface called EEMI. EEMI and SCMI are not the same but they
are somewhat similar.

From my experience with this kind of interfaces, I think Oleksii's
design is the right way to go. There are some important details to
review, like the device tree interfaces at the host level and domU
level, and the memory mapping of the channels; we need to be very
careful about those details. But overall I think it is the right
design.

Julien Grall Feb. 12, 2022, 12:43 p.m. UTC | #8

Hi,

On 11/02/2022 11:18, Bertrand Marquis wrote:
> Do you plan to add support for other boards ?
> 
> Did you discuss more in general with the linux kernel guys to see if this
> approach was agreed and will be adopted by other manufacturers ?
> 
> All in all I think this is a good idea but I fear that all this will actually only
> be used by one board or one manufacturer and other might use a different
> strategy, I would like to unrisk this before merging this in Xen.

In the past we merged code that would only benefits one vendor (i.e. 
EEMI). That said, this was a vendor specific protocol. I believe the 
situation is different here because the spec is meant to be generic.

> @julien and Stefano: what is your view here ?

I share the same concerns as you. I think we need to make sure all the 
pieces we rely on (e.g. firmware, DT bindings) have been agreed before 
we can merge such code in Xen.

The first step is to have all the pieces available in public so they can 
be reviewed and tested together.

Oleksii, on a separate e-mail, you said you made change for ATF. How 
much of those changes was related to support for Xen? If they are some, 
then I think they should be upstreamed first.

Cheers,

Oleksii Moisieiev Feb. 14, 2022, 11:13 a.m. UTC | #9

Hi Julien,

On Sat, Feb 12, 2022 at 12:43:56PM +0000, Julien Grall wrote:
> Hi,
> 
> On 11/02/2022 11:18, Bertrand Marquis wrote:
> > Do you plan to add support for other boards ?
> > 
> > Did you discuss more in general with the linux kernel guys to see if this
> > approach was agreed and will be adopted by other manufacturers ?
> > 
> > All in all I think this is a good idea but I fear that all this will actually only
> > be used by one board or one manufacturer and other might use a different
> > strategy, I would like to unrisk this before merging this in Xen.
> 
> In the past we merged code that would only benefits one vendor (i.e. EEMI).
> That said, this was a vendor specific protocol. I believe the situation is
> different here because the spec is meant to be generic.
> 
> > @julien and Stefano: what is your view here ?
> 
> I share the same concerns as you. I think we need to make sure all the
> pieces we rely on (e.g. firmware, DT bindings) have been agreed before we
> can merge such code in Xen.
> 
> The first step is to have all the pieces available in public so they can be
> reviewed and tested together.
> 
> Oleksii, on a separate e-mail, you said you made change for ATF. How much of
> those changes was related to support for Xen? If they are some, then I think
> they should be upstreamed first.
> 

Let me share changes, that were done to AT-F and Linux kernel
device-tree in terms of the SCMI mediator POC.
Changes to the Linux kernel:
https://github.com/oleksiimoisieiev/arm-trusted-firmware/pull/4
Based on renesas-rcar linux-bsp, branch v5.10/rcar-5.0.0.rc5

Changes to AT-F:
https://github.com/oleksiimoisieiev/linux-bsp/pull/3
Based on renesas-rcar/arm-trusted-firmware branch rcar_gen3_v2.5.

All changes that were done are not Xen specific. Given AT-F changes are
the implementation of the SCMI feature using SMC as transport. All
changes were done accoding to the DEN0056C [0] and DEN0028D [1].

We pass channel_id via SMC to the Firmware on r7 bits [15:0] (See Section
4.1 of [1]). Then Firmware converts channel_id to agent_id. Channel_id can't be
equal to agent_id in our case, because, according to the 4.2.2.8 of [0]
- agent_id 0 is reserved to identify platform itself.


[0] https://developer.arm.com/documentation/den0056/latest
[1] https://developer.arm.com/documentation/den0028/latest

Best regards,
Oleksii.

Bertrand Marquis Feb. 14, 2022, 11:27 a.m. UTC | #10

Hi Oleksii,

> On 14 Feb 2022, at 11:13, Oleksii Moisieiev <[e-mail]> wrote:
> 
> Hi Julien,
> 
> On Sat, Feb 12, 2022 at 12:43:56PM +0000, Julien Grall wrote:
>> Hi,
>> 
>> On 11/02/2022 11:18, Bertrand Marquis wrote:
>>> Do you plan to add support for other boards ?
>>> 
>>> Did you discuss more in general with the linux kernel guys to see if this
>>> approach was agreed and will be adopted by other manufacturers ?
>>> 
>>> All in all I think this is a good idea but I fear that all this will actually only
>>> be used by one board or one manufacturer and other might use a different
>>> strategy, I would like to unrisk this before merging this in Xen.
>> 
>> In the past we merged code that would only benefits one vendor (i.e. EEMI).
>> That said, this was a vendor specific protocol. I believe the situation is
>> different here because the spec is meant to be generic.
>> 
>>> @julien and Stefano: what is your view here ?
>> 
>> I share the same concerns as you. I think we need to make sure all the
>> pieces we rely on (e.g. firmware, DT bindings) have been agreed before we
>> can merge such code in Xen.
>> 
>> The first step is to have all the pieces available in public so they can be
>> reviewed and tested together.
>> 
>> Oleksii, on a separate e-mail, you said you made change for ATF. How much of
>> those changes was related to support for Xen? If they are some, then I think
>> they should be upstreamed first.
>> 
> 
> Let me share changes, that were done to AT-F and Linux kernel
> device-tree in terms of the SCMI mediator POC.
> Changes to the Linux kernel:
> https://github.com/oleksiimoisieiev/arm-trusted-firmware/pull/4
> Based on renesas-rcar linux-bsp, branch v5.10/rcar-5.0.0.rc5
> 
> Changes to AT-F:
> https://github.com/oleksiimoisieiev/linux-bsp/pull/3
> Based on renesas-rcar/arm-trusted-firmware branch rcar_gen3_v2.5.

You inverted the links but thanks this is really useful.

Did you push the ATF changes to mainstream ATF or discuss those with
the maintainers ?

The strategy overall is nice but we need to make sure this is accepted and
 merged by all parties (ATF and Linux) to make sure the support for this will
not only be available in Xen and for one board.

I will try to get in touch with the SCMI linux driver maintainer at arm to get his view.

Regards
Bertrand

> 
> All changes that were done are not Xen specific. Given AT-F changes are
> the implementation of the SCMI feature using SMC as transport. All
> changes were done accoding to the DEN0056C [0] and DEN0028D [1].
> 
> We pass channel_id via SMC to the Firmware on r7 bits [15:0] (See Section
> 4.1 of [1]). Then Firmware converts channel_id to agent_id. Channel_id can't be
> equal to agent_id in our case, because, according to the 4.2.2.8 of [0]
> - agent_id 0 is reserved to identify platform itself.
> 
> 
> [0] https://developer.arm.com/documentation/den0056/latest
> [1] https://developer.arm.com/documentation/den0028/latest
> 
> Best regards,
> Oleksii.

Oleksii Moisieiev Feb. 14, 2022, 11:51 a.m. UTC | #11

Hi Bertrand,

On Mon, Feb 14, 2022 at 11:27:21AM +0000, Bertrand Marquis wrote:
> Hi Oleksii,
> 
> > On 14 Feb 2022, at 11:13, Oleksii Moisieiev <[e-mail]> wrote:
> > 
> > Hi Julien,
> > 
> > On Sat, Feb 12, 2022 at 12:43:56PM +0000, Julien Grall wrote:
> >> Hi,
> >> 
> >> On 11/02/2022 11:18, Bertrand Marquis wrote:
> >>> Do you plan to add support for other boards ?
> >>> 
> >>> Did you discuss more in general with the linux kernel guys to see if this
> >>> approach was agreed and will be adopted by other manufacturers ?
> >>> 
> >>> All in all I think this is a good idea but I fear that all this will actually only
> >>> be used by one board or one manufacturer and other might use a different
> >>> strategy, I would like to unrisk this before merging this in Xen.
> >> 
> >> In the past we merged code that would only benefits one vendor (i.e. EEMI).
> >> That said, this was a vendor specific protocol. I believe the situation is
> >> different here because the spec is meant to be generic.
> >> 
> >>> @julien and Stefano: what is your view here ?
> >> 
> >> I share the same concerns as you. I think we need to make sure all the
> >> pieces we rely on (e.g. firmware, DT bindings) have been agreed before we
> >> can merge such code in Xen.
> >> 
> >> The first step is to have all the pieces available in public so they can be
> >> reviewed and tested together.
> >> 
> >> Oleksii, on a separate e-mail, you said you made change for ATF. How much of
> >> those changes was related to support for Xen? If they are some, then I think
> >> they should be upstreamed first.
> >> 
> > 
> > Let me share changes, that were done to AT-F and Linux kernel
> > device-tree in terms of the SCMI mediator POC.
> > Changes to the Linux kernel:
> > https://urldefense.com/v3/__https://github.com/oleksiimoisieiev/arm-trusted-firmware/pull/4__;!!GF_29dbcQIUBPA!je9Cu0n0498Yn76OLWjxxVaB7jWJtyWycHX0YARezTnc7aYHpGRJ8tSxHqIC0fTMUUSV$ [github[.]com]
> > Based on renesas-rcar linux-bsp, branch v5.10/rcar-5.0.0.rc5
> > 
> > Changes to AT-F:
> > https://urldefense.com/v3/__https://github.com/oleksiimoisieiev/linux-bsp/pull/3__;!!GF_29dbcQIUBPA!je9Cu0n0498Yn76OLWjxxVaB7jWJtyWycHX0YARezTnc7aYHpGRJ8tSxHqIC0eDKS3ge$ [github[.]com]
> > Based on renesas-rcar/arm-trusted-firmware branch rcar_gen3_v2.5.
> 
> You inverted the links but thanks this is really useful.
> 

That's strange. Links looks good from xen.markmail.org interface.

> Did you push the ATF changes to mainstream ATF or discuss those with
> the maintainers ?

No. We did changes in ATF as a proof of concept.

> 
> The strategy overall is nice but we need to make sure this is accepted and
>  merged by all parties (ATF and Linux) to make sure the support for this will
> not only be available in Xen and for one board.

I've prepared patch to Linux kernel, which is introducing scmi_devid
binding, needed to set device permissions via SCMI. I've contacted
Sudeep Holla <[e-mail]>, who is the maintainer of the SCMI protocol
drivers. Waiting for the response.

Changes to ATF are not Xen specific and were done in terms of POC. We do
not have plans to upstream those changes right now.

> 
> I will try to get in touch with the SCMI linux driver maintainer at arm to get his view.
> 

Thanks.

Best regards,
Oleksii.

Stefano Stabellini Feb. 14, 2022, 10:05 p.m. UTC | #12

On Mon, 14 Feb 2022, Oleksii Moisieiev wrote:
> Hi Bertrand,
> 
> On Mon, Feb 14, 2022 at 11:27:21AM +0000, Bertrand Marquis wrote:
> > Hi Oleksii,
> > 
> > > On 14 Feb 2022, at 11:13, Oleksii Moisieiev <[e-mail]> wrote:
> > > 
> > > Hi Julien,
> > > 
> > > On Sat, Feb 12, 2022 at 12:43:56PM +0000, Julien Grall wrote:
> > >> Hi,
> > >> 
> > >> On 11/02/2022 11:18, Bertrand Marquis wrote:
> > >>> Do you plan to add support for other boards ?
> > >>> 
> > >>> Did you discuss more in general with the linux kernel guys to see if this
> > >>> approach was agreed and will be adopted by other manufacturers ?
> > >>> 
> > >>> All in all I think this is a good idea but I fear that all this will actually only
> > >>> be used by one board or one manufacturer and other might use a different
> > >>> strategy, I would like to unrisk this before merging this in Xen.
> > >> 
> > >> In the past we merged code that would only benefits one vendor (i.e. EEMI).
> > >> That said, this was a vendor specific protocol. I believe the situation is
> > >> different here because the spec is meant to be generic.
> > >> 
> > >>> @julien and Stefano: what is your view here ?
> > >> 
> > >> I share the same concerns as you. I think we need to make sure all the
> > >> pieces we rely on (e.g. firmware, DT bindings) have been agreed before we
> > >> can merge such code in Xen.
> > >> 
> > >> The first step is to have all the pieces available in public so they can be
> > >> reviewed and tested together.
> > >>
> > >> Oleksii, on a separate e-mail, you said you made change for ATF. How much of
> > >> those changes was related to support for Xen? If they are some, then I think
> > >> they should be upstreamed first.
> > >> 
> > > 
> > > Let me share changes, that were done to AT-F and Linux kernel
> > > device-tree in terms of the SCMI mediator POC.
> > > Changes to the Linux kernel:
> > > https://urldefense.com/v3/__https://github.com/oleksiimoisieiev/arm-trusted-firmware/pull/4__;!!GF_29dbcQIUBPA!je9Cu0n0498Yn76OLWjxxVaB7jWJtyWycHX0YARezTnc7aYHpGRJ8tSxHqIC0fTMUUSV$ [github[.]com]
> > > Based on renesas-rcar linux-bsp, branch v5.10/rcar-5.0.0.rc5
> > > 
> > > Changes to AT-F:
> > > https://urldefense.com/v3/__https://github.com/oleksiimoisieiev/linux-bsp/pull/3__;!!GF_29dbcQIUBPA!je9Cu0n0498Yn76OLWjxxVaB7jWJtyWycHX0YARezTnc7aYHpGRJ8tSxHqIC0eDKS3ge$ [github[.]com]
> > > Based on renesas-rcar/arm-trusted-firmware branch rcar_gen3_v2.5.
> > 
> > You inverted the links but thanks this is really useful.
> > 
> 
> That's strange. Links looks good from xen.markmail.org interface.
> 
> > Did you push the ATF changes to mainstream ATF or discuss those with
> > the maintainers ?
> 
> No. We did changes in ATF as a proof of concept.
> 
> > 
> > The strategy overall is nice but we need to make sure this is accepted and
> >  merged by all parties (ATF and Linux) to make sure the support for this will
> > not only be available in Xen and for one board.

+1


> I've prepared patch to Linux kernel, which is introducing scmi_devid
> binding, needed to set device permissions via SCMI. I've contacted
> Sudeep Holla <[e-mail]>, who is the maintainer of the SCMI protocol
> drivers. Waiting for the response.
> 
> Changes to ATF are not Xen specific and were done in terms of POC. We do
> not have plans to upstream those changes right now.

If this work relies on a new interface in ATF, and the interface is not
vendor-specific, then at least the interface (if not the code) should be
reviewed and accepted by ATF.

Otherwise we risk ending up with an upstream SCMI implementation in Xen
that cannot be used anywhere, except the PoC. To make things worse, this
could happen:

- we upstream the SCMI mediator to Xen
- we upstream any required changes to Linux
- ATF rejects the SCMI-related interface changes
- ATF comes up with a difference interface

At this point we would have to deprecate the implementation in Xen. It
might also be difficult to do so due to versioning issues. We would
need to be able to detect which version of ATF we are running on, to
distinguish the ATF PoC version that works with the old interface from
the new ATF version that supports a different interface.

To avoid this kind of issues we typically expect that all relevant
communities agree on the public interfaces before upstreaming the code.

Oleksii Moisieiev Feb. 16, 2022, 5:41 p.m. UTC | #13

Hi Stefano,

On Mon, Feb 14, 2022 at 02:05:18PM -0800, Stefano Stabellini wrote:
> On Mon, 14 Feb 2022, Oleksii Moisieiev wrote:
> > Hi Bertrand,
> > 
> > On Mon, Feb 14, 2022 at 11:27:21AM +0000, Bertrand Marquis wrote:
> > > Hi Oleksii,
> > > 
> > > > On 14 Feb 2022, at 11:13, Oleksii Moisieiev <[e-mail]> wrote:
> > > > 
> > > > Hi Julien,
> > > > 
> > > > On Sat, Feb 12, 2022 at 12:43:56PM +0000, Julien Grall wrote:
> > > >> Hi,
> > > >> 
> > > >> On 11/02/2022 11:18, Bertrand Marquis wrote:
> > > >>> Do you plan to add support for other boards ?
> > > >>> 
> > > >>> Did you discuss more in general with the linux kernel guys to see if this
> > > >>> approach was agreed and will be adopted by other manufacturers ?
> > > >>> 
> > > >>> All in all I think this is a good idea but I fear that all this will actually only
> > > >>> be used by one board or one manufacturer and other might use a different
> > > >>> strategy, I would like to unrisk this before merging this in Xen.
> > > >> 
> > > >> In the past we merged code that would only benefits one vendor (i.e. EEMI).
> > > >> That said, this was a vendor specific protocol. I believe the situation is
> > > >> different here because the spec is meant to be generic.
> > > >> 
> > > >>> @julien and Stefano: what is your view here ?
> > > >> 
> > > >> I share the same concerns as you. I think we need to make sure all the
> > > >> pieces we rely on (e.g. firmware, DT bindings) have been agreed before we
> > > >> can merge such code in Xen.
> > > >> 
> > > >> The first step is to have all the pieces available in public so they can be
> > > >> reviewed and tested together.
> > > >>
> > > >> Oleksii, on a separate e-mail, you said you made change for ATF. How much of
> > > >> those changes was related to support for Xen? If they are some, then I think
> > > >> they should be upstreamed first.
> > > >> 
> > > > 
> > > > Let me share changes, that were done to AT-F and Linux kernel
> > > > device-tree in terms of the SCMI mediator POC.
> > > > Changes to the Linux kernel:
> > > > https://urldefense.com/v3/__https://github.com/oleksiimoisieiev/arm-trusted-firmware/pull/4__;!!GF_29dbcQIUBPA!je9Cu0n0498Yn76OLWjxxVaB7jWJtyWycHX0YARezTnc7aYHpGRJ8tSxHqIC0fTMUUSV$ [github[.]com]
> > > > Based on renesas-rcar linux-bsp, branch v5.10/rcar-5.0.0.rc5
> > > > 
> > > > Changes to AT-F:
> > > > https://urldefense.com/v3/__https://github.com/oleksiimoisieiev/linux-bsp/pull/3__;!!GF_29dbcQIUBPA!je9Cu0n0498Yn76OLWjxxVaB7jWJtyWycHX0YARezTnc7aYHpGRJ8tSxHqIC0eDKS3ge$ [github[.]com]
> > > > Based on renesas-rcar/arm-trusted-firmware branch rcar_gen3_v2.5.
> > > 
> > > You inverted the links but thanks this is really useful.
> > > 
> > 
> > That's strange. Links looks good from xen.markmail.org interface.
> > 
> > > Did you push the ATF changes to mainstream ATF or discuss those with
> > > the maintainers ?
> > 
> > No. We did changes in ATF as a proof of concept.
> > 
> > > 
> > > The strategy overall is nice but we need to make sure this is accepted and
> > >  merged by all parties (ATF and Linux) to make sure the support for this will
> > > not only be available in Xen and for one board.
> 
> +1
> 
> 
> > I've prepared patch to Linux kernel, which is introducing scmi_devid
> > binding, needed to set device permissions via SCMI. I've contacted
> > Sudeep Holla <[e-mail]>, who is the maintainer of the SCMI protocol
> > drivers. Waiting for the response.
> > 
> > Changes to ATF are not Xen specific and were done in terms of POC. We do
> > not have plans to upstream those changes right now.
> 
> If this work relies on a new interface in ATF, and the interface is not
> vendor-specific, then at least the interface (if not the code) should be
> reviewed and accepted by ATF.
> 
> Otherwise we risk ending up with an upstream SCMI implementation in Xen
> that cannot be used anywhere, except the PoC. To make things worse, this
> could happen:
> 
> - we upstream the SCMI mediator to Xen
> - we upstream any required changes to Linux
> - ATF rejects the SCMI-related interface changes
> - ATF comes up with a difference interface
> 
> At this point we would have to deprecate the implementation in Xen. It
> might also be difficult to do so due to versioning issues. We would
> need to be able to detect which version of ATF we are running on, to
> distinguish the ATF PoC version that works with the old interface from
> the new ATF version that supports a different interface.
> 
> To avoid this kind of issues we typically expect that all relevant
> communities agree on the public interfaces before upstreaming the code.

That's sound reasonable.
I'll contact with AT-F maintainers. Maybe I'll be able to upstream SCMI
to AT-F.
Also I hope I'll be able to contact Linux kernel SCMI maintainers and
discuss device-tree structure.

Best regards,
Oleksii

Patch

diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
index ab07833582..3b0dfc57b6 100644
--- a/xen/arch/arm/Kconfig
+++ b/xen/arch/arm/Kconfig
@@ -123,6 +123,8 @@  config ARM_SCI
 	  support. It allows guests to control system resourcess via one of
 	  ARM_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..10b634d2ed
--- /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 ARM_SCI && HOST_DTB_EXPORT
+	---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/scmi_smc.c b/xen/arch/arm/sci/scmi_smc.c
new file mode 100644
index 0000000000..103529dfab
--- /dev/null
+++ b/xen/arch/arm/sci/scmi_smc.c
@@ -0,0 +1,959 @@ 
+/*
+ * xen/arch/arm/sci/scmi_smc.c
+ *
+ * SCMI mediator driver, using SCP as transport.
+ *
+ * Oleksii Moisieiev <[e-mail]>
+ * 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                 "arm,scmi-shmem"
+#define SCMI_SHMEM                         "shmem"
+#define SCMI_SHMEM_MAPPED_SIZE             PAGE_SIZE
+
+#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;
+
+#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 dt_channel_addr {
+    u64 addr;
+    u64 size;
+    struct list_head list;
+};
+
+struct scmi_channel {
+    int chan_id;
+    int agent_id;
+    uint32_t func_id;
+    domid_t domain_id;
+    uint64_t paddr;
+    uint64_t len;
+    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;
+};
+
+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;
+}
+
+/*
+ * Copy data from IO memory space to "real" memory space.
+ */
+void __memcpy_fromio(void *to, const volatile void __iomem *from, size_t count)
+{
+    while (count && !IS_ALIGNED((unsigned long)from, 4)) {
+        *(u8 *)to = __raw_readb(from);
+        from++;
+        to++;
+        count--;
+    }
+
+    while (count >= 4) {
+        *(u32 *)to = __raw_readl(from);
+        from += 4;
+        to += 4;
+        count -= 4;
+    }
+
+    while (count) {
+        *(u8 *)to = __raw_readb(from);
+        from++;
+        to++;
+        count--;
+    }
+}
+
+/*
+ * Copy data from "real" memory space to IO memory space.
+ */
+void __memcpy_toio(volatile void __iomem *to, const void *from, size_t count)
+{
+    while (count && !IS_ALIGNED((unsigned long)to, 4)) {
+        __raw_writeb(*(u8 *)from, to);
+        from++;
+        to++;
+        count--;
+    }
+
+    while (count >= 4) {
+        __raw_writel(*(u32 *)from, to);
+        from += 4;
+        to += 4;
+        count -= 4;
+    }
+
+    while (count) {
+        __raw_writeb(*(u8 *)from, to);
+        from++;
+        to++;
+        count--;
+    }
+}
+
+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;
+
+    if ( (len + sizeof(chan_info->shmem->msg_header)) >
+                         SCMI_SHMEM_MAPPED_SIZE )
+    {
+        printk(XENLOG_ERR
+               "scmi: Wrong size of smc message. Data is invalid\n");
+        return -EINVAL;
+    }
+
+    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_toio((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;
+    default:
+        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);
+
+    if ( len >= SCMI_SHMEM_MAPPED_SIZE - sizeof(chan_info->shmem) )
+    {
+        printk(XENLOG_ERR
+               "scmi: Wrong size of input smc message. Data may be invalid\n");
+        return -EINVAL;
+    }
+
+    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_fromio(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;
+
+    ASSERT( channel && channel->shmem);
+
+    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 *aquire_scmi_channel(domid_t 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->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)
+{
+    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_cache(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 after vmap %p\n", channel->shmem);
+    return 0;
+}
+
+static void unmap_channel_memory(struct scmi_channel *channel)
+{
+    ASSERT( channel && channel->shmem );
+    iounmap(channel->shmem);
+    channel->shmem = NULL;
+}
+
+static struct scmi_channel *smc_create_channel(uint8_t chan_id,
+                                               uint32_t func_id, uint64_t addr)
+{
+    struct scmi_channel *channel;
+
+    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;
+    channel->shmem = NULL;
+    channel->paddr = addr;
+    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 mem_permit_access(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 mem_deny_access(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(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, SCMI_SHARED_MEMORY);
+    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)
+    {
+        list_del(&curr->list);
+        xfree(curr);
+    }
+
+    spin_unlock(&scmi_data.channel_list_lock);
+}
+
+static struct dt_device_node *get_dt_node_from_property(
+                struct dt_device_node *node, const char * p_name)
+{
+    const __be32 *prop;
+
+    ASSERT( node );
+
+    prop = dt_get_property(node, p_name, NULL);
+    if ( !prop )
+        return ERR_PTR(-EINVAL);
+
+    return dt_find_node_by_phandle(be32_to_cpup(prop));
+}
+
+static int get_shmem_regions(struct list_head *head, u64 hyp_addr)
+{
+    struct dt_device_node *node;
+    int ret;
+    struct dt_channel_addr *lchan;
+    u64 laddr, lsize;
+
+    node = dt_find_compatible_node(NULL, NULL, SCMI_SHARED_MEMORY);
+    if ( !node )
+        return -ENOENT;
+
+    while ( node )
+    {
+        ret = dt_device_get_address(node, 0, &laddr, &lsize);
+        if ( ret )
+            return ret;
+
+        if ( laddr != hyp_addr )
+        {
+            lchan = xmalloc(struct dt_channel_addr);
+            if ( !lchan )
+                return -ENOMEM;
+            lchan->addr = laddr;
+            lchan->size = lsize;
+
+            list_add_tail(&lchan->list, head);
+        }
+
+        node = dt_find_compatible_node(node, NULL, SCMI_SHARED_MEMORY);
+    }
+
+    return 0;
+}
+
+static int read_hyp_channel_addr(struct dt_device_node *scmi_node,
+                                 u64 *addr, u64 *size)
+{
+    struct dt_device_node *shmem_node;
+    shmem_node = get_dt_node_from_property(scmi_node, "shmem");
+    if ( IS_ERR_OR_NULL(shmem_node) )
+    {
+        printk(XENLOG_ERR
+               "scmi: Device tree error, can't parse reserved memory %ld\n",
+               PTR_ERR(shmem_node));
+        return PTR_ERR(shmem_node);
+    }
+
+    return dt_device_get_address(shmem_node, 0, addr, size);
+}
+
+static void free_shmem_regions(struct list_head *addr_list)
+{
+    struct dt_channel_addr *curr, *_curr;
+
+    list_for_each_entry_safe (curr, _curr, addr_list, list)
+    {
+        list_del(&curr->list);
+        xfree(curr);
+    }
+}
+
+static __init bool scmi_probe(struct dt_device_node *scmi_node)
+{
+    u64 addr, size;
+    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;
+    struct dt_channel_addr *entry;
+    struct list_head addr_list;
+
+    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;
+    }
+
+    ret = read_hyp_channel_addr(scmi_node, &addr, &size);
+    if ( IS_ERR_VALUE(ret) )
+        return false;
+
+    if ( !IS_ALIGNED(size, SCMI_SHMEM_MAPPED_SIZE) )
+    {
+        printk(XENLOG_ERR "scmi: Reserved memory is not aligned\n");
+        return false;
+    }
+
+    INIT_LIST_HEAD(&addr_list);
+
+    ret = get_shmem_regions(&addr_list, addr);
+    if ( IS_ERR_VALUE(ret) )
+        goto out;
+
+    channel = smc_create_channel(HYP_CHANNEL, func_id, addr);
+    if ( IS_ERR(channel) )
+        goto out;
+
+    ret = map_channel_memory(channel);
+    if ( ret )
+        goto out;
+
+    spin_lock(&scmi_data.channel_list_lock);
+    channel->domain_id = DOMID_XEN;
+    spin_unlock(&scmi_data.channel_list_lock);
+
+    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;
+
+    ret = check_scmi_status(rx.status);
+    if ( ret )
+        goto error;
+
+    n_agents = FIELD_GET(MSG_N_AGENTS_MASK, rx.attributes);
+    printk(XENLOG_DEBUG "scmi: Got agent count %d\n", n_agents);
+
+    i = 1;
+    list_for_each_entry(entry, &addr_list, list)
+    {
+        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,
+                                           entry->addr);
+        if ( IS_ERR(agent_channel) )
+        {
+            ret = PTR_ERR(agent_channel);
+            goto error;
+        }
+
+        ret = map_channel_memory(agent_channel);
+        if ( ret )
+            goto error;
+
+        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 )
+        {
+            unmap_channel_memory(agent_channel);
+            goto error;
+        }
+
+        unmap_channel_memory(agent_channel);
+
+        ret = check_scmi_status(da_rx.status);
+        if ( ret )
+            goto error;
+
+        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;
+
+        if ( i == n_agents )
+            break;
+
+        i++;
+    }
+
+    scmi_data.initialized = true;
+    goto out;
+
+error:
+    unmap_channel_memory(channel);
+    free_channel_list();
+out:
+    free_shmem_regions(&addr_list);
+    return ret == 0;
+}
+
+static int scmi_domain_init(struct domain *d,
+                           struct xen_arch_domainconfig *config)
+{
+    struct scmi_channel *channel;
+    int ret;
+
+    if ( !scmi_data.initialized )
+        return 0;
+
+    printk(XENLOG_INFO "scmi: domain_id = %d\n", d->domain_id);
+
+    channel = aquire_scmi_channel(d->domain_id);
+    if ( IS_ERR_OR_NULL(channel) )
+        return -ENOENT;
+
+#ifdef CONFIG_ARM_32
+    printk(XENLOG_INFO
+           "scmi: Aquire SCMI channel id = 0x%x , domain_id = %d paddr = 0x%llx\n",
+           channel->chan_id, channel->domain_id, channel->paddr);
+#else
+    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);
+#endif
+
+    if ( is_hardware_domain(d) )
+    {
+        ret = mem_permit_access(d, channel->paddr, PAGE_SIZE);
+        if ( IS_ERR_VALUE(ret) )
+            goto error;
+
+        ret = dt_update_domain_range(channel->paddr, PAGE_SIZE);
+        if ( IS_ERR_VALUE(ret) )
+        {
+            int rc = mem_deny_access(d, channel->paddr, PAGE_SIZE);
+            if ( rc )
+                printk(XENLOG_ERR "Unable to mem_deny_access\n");
+
+            goto error;
+        }
+    }
+
+    d->arch.sci = channel;
+    if ( config )
+        config->arm_sci_agent_paddr = channel->paddr;
+
+    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;
+    struct scmi_perms_tx {
+        uint32_t agent_id;
+        uint32_t device_id;
+        uint32_t flags;
+    } 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 = d->arch.sci;
+    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;
+
+    mem_deny_access(d, channel->paddr, PAGE_SIZE);
+    spin_unlock(&channel->lock);
+}
+
+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->r0 )
+    {
+        res = false;
+        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 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,
+};
+
+REGISTER_SCI_MEDIATOR(scmi_smc, "SCMI-SMC", XEN_DOMCTL_CONFIG_ARM_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:
+ */