Re: [PATCH v14 4/6] xen/arm: scmi: introduce SCI SCMI SMC multi-agent driver

[Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>]

Hi Oleksii,

Oleksii Moisieiev <Oleksii_Moisieiev@xxxxxxxx> writes:

> Hi Volodymyr,
>
> Thank you for the review. I agree with the most of your comments, just
> few more questions left:
>
> On 23/10/2026 04:56, Volodymyr Babchuk wrote:
>> Hi Oleksii,
>>
>> Oleksii Moisieiev <Oleksii_Moisieiev@xxxxxxxx> writes:
>>
>>> This patch introduces SCI driver to support for ARM EL3 Trusted Firmware-A
>>> (TF-A) which provides SCMI interface with multi-agent support, as shown
> [snip]
>>> diff --git a/xen/arch/arm/firmware/scmi-shmem.c 
>>> b/xen/arch/arm/firmware/scmi-shmem.c
>>> new file mode 100644
>>> index 0000000000..e36745a85e
>>> --- /dev/null
>>> +++ b/xen/arch/arm/firmware/scmi-shmem.c
>>> @@ -0,0 +1,118 @@
>>> +/* SPDX-License-Identifier: GPL-2.0-only */
>>> +/*
>>> + * SMC/HVC shmem transport implementation used by
>>> + * SCI SCMI multi-agent driver.
>>> + *
>>> + * Oleksii Moisieiev <oleksii_moisieiev@xxxxxxxx>
>>> + * Copyright (c) 2025 EPAM Systems
>>> + */
>>> +
>>> +#include <xen/err.h>
>>> +#include <xen/io.h>
>>> +#include <asm/io.h>
>>> +
>>> +#include "scmi-proto.h"
>>> +#include "scmi-shmem.h"
>>> +
>>> +static inline int
>>> +shmem_channel_status(const volatile struct scmi_shared_mem __iomem *shmem)
>>> +{
>>> +    return (readl(&shmem->channel_status) &
>>> +            SCMI_SHMEM_CHAN_STAT_CHANNEL_FREE) ? 0 : -EBUSY;
>>> +}
>>> +
>>> +int shmem_put_message(volatile struct scmi_shared_mem __iomem *shmem,
>>> +                      scmi_msg_header_t *hdr, void *data, unsigned int len)
>>> +{
>>> +    int ret;
>>> +
>>> +    if ( (len + offsetof(struct scmi_shared_mem, msg_payload)) >
>>> +         SCMI_SHMEM_MAPPED_SIZE )
>>> +    {
>>> +        printk(XENLOG_ERR "scmi: Wrong size of smc message. Data is 
>>> invalid\n");
>> "of the SMC message". Also, looks like it is over 80 characters.
> agreed with both. thank you.
>>> +        return -EINVAL;
>>> +    }
>>> +
>>> +    ret = shmem_channel_status(shmem);
>>> +    if ( ret )
>>> +        return ret;
>>> +
>>> +    writel_relaxed(0x0, &shmem->channel_status);
>>> +    /* Writing 0x0 right now, but "shmem"_FLAG_INTR_ENABLED can be set */
>>> +    writel_relaxed(0x0, &shmem->flags);
>>> +    writel_relaxed(sizeof(shmem->msg_header) + len, &shmem->length);
>>> +    writel(pack_scmi_header(hdr), &shmem->msg_header);
>> Why do you mix writel() and writel_relaxed() here?
>
> The main idea that I'm filling up SCMI header information following
> Device memory ordering and execute __iowmb before store in the last
> writel call.

Yes, but difference between writel_relaxed() and writel() is that
writel() has memory barrier for Normal Memory accesses. Writes to Device
Memory are guaranteed to arrive in the proper order.

>
>
>>> +
>>> +    if ( len > 0 && data )
>>> +        memcpy_toio(shmem->msg_payload, data, len);
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +int shmem_get_response(const volatile struct scmi_shared_mem __iomem 
>>> *shmem,
>>> +                       scmi_msg_header_t *hdr, void *data, unsigned int 
>>> len)
>>> +{
>>> +    int recv_len;
>>> +    int ret;
>>> +    /*
>>> +     * First word of msg_payload carries the returned status; exclude it 
>>> from
>>> +     * recv_len so only the protocol payload is copied back to the caller.
>>> +     */
>>> +    int pad = sizeof(hdr->status);
>>> +
>>> +    if ( len >= SCMI_SHMEM_MAPPED_SIZE -
>>> +         offsetof(struct scmi_shared_mem, msg_payload) )
>>> +    {
>>> +        printk(XENLOG_ERR
>>> +               "scmi: Wrong size of input smc message. Data may be 
>>> invalid\n");
>> "of the input SMC message"
> +1
>>> +        return -EINVAL;
>>> +    }
>>> +
>>> +    ret = shmem_channel_status(shmem);
>>> +    if ( ret )
>>> +        return ret;
>>> +
>>> +    recv_len = readl(&shmem->length) - sizeof(shmem->msg_header);
>>> +
>>> +    if ( recv_len < 0 )
>>> +    {
>>> +        printk(XENLOG_ERR
>>> +               "scmi: Wrong size of smc message. Data may be invalid\n");
>> "of the SMC message"
>>
>> Also, I am wondering if you really need to print this error messages in
>> the first place.
>>
> These generic messages were very helpful in determining the exact error.
> For example, this function returns -EINVAL both if
>
>   len >= SCMI_SHMEM_MAPPED_SIZE -  offsetof(struct scmi_shared_mem, 
> msg_payload)  and if recv_len < 0
>
> However, if you think they are excessive, I can remove the prints and return 
> different error codes instead.

Well, I have no strong opinion here. So you can leave them if you
wish. Just make sure that they have proper spelling.

>
>>> +        return -EINVAL;
>>> +    }
>>> +
>>> +    unpack_scmi_header(readl(&shmem->msg_header), hdr);
>>> +
>>> +    hdr->status = readl(&shmem->msg_payload);
>>> +    recv_len = recv_len > pad ? recv_len - pad : 0;
>>> +
>>> +    ret = scmi_to_xen_errno(hdr->status);
>>> +    if ( ret )
>>> +    {
>>> +        printk(XENLOG_DEBUG "scmi: Error received: %d\n", ret);
>>> +        return ret;
>>> +    }
>>> +
>>> +    if ( recv_len > len )
>>> +    {
>>> +        printk(XENLOG_ERR
>>> +               "scmi: Not enough buffer for message %d, expecting %d\n",
>>> +               recv_len, len);
>>> +        return -EINVAL;
>>> +    }
>>> +
>>> +    if ( recv_len > 0 )
>>> +        memcpy_fromio(data, shmem->msg_payload + pad, recv_len);
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +/*
>>> + * Local variables:
>>> + * mode: C
>>> + * c-file-style: "BSD"
>>> + * c-basic-offset: 4
>>> + * tab-width: 4
>>> + * indent-tabs-mode: nil
>>> + * End:
>>> + */
>>> diff --git a/xen/arch/arm/firmware/scmi-shmem.h 
>>> b/xen/arch/arm/firmware/scmi-shmem.h
>>> new file mode 100644
>>> index 0000000000..722263aa77
>>> --- /dev/null
>>> +++ b/xen/arch/arm/firmware/scmi-shmem.h
>>> @@ -0,0 +1,45 @@
>>> +/* SPDX-License-Identifier: GPL-2.0-only */
>>> +/*
>>> + * Arm System Control and Management Interface definitions
>>> + * Version 3.0 (DEN0056C)
>>> + * Shared Memory based Transport
>>> + *
>>> + * Copyright (c) 2024 EPAM Systems
>>> + */
>>> +
>>> +#ifndef ARM_FIRMWARE_SCMI_SHMEM_H_
>>> +#define ARM_FIRMWARE_SCMI_SHMEM_H_
>>> +
>>> +#include <xen/stdint.h>
>>> +
>>> +#define SCMI_SHMEM_CHAN_STAT_CHANNEL_FREE  BIT(0, UL)
>>> +#define SCMI_SHMEM_CHAN_STAT_CHANNEL_ERROR BIT(1, UL)
>>> +
>>> +struct scmi_shared_mem {
>>> +    uint32_t reserved;
>>> +    uint32_t channel_status;
>>> +    uint32_t reserved1[2];
>>> +    uint32_t flags;
>>> +    uint32_t length;
>>> +    uint32_t msg_header;
>>> +    uint8_t msg_payload[];
>>> +};
>> Should it be __packed?
> The layout already matches the shared-memory format without padding:
> Every fixed member is a uint32_t or an array of uint8_t, and the
> mapping is page aligned.
>
> Therefore __packed would not change these offsets, but would reduce the
> structure's alignment guarantee. I would prefer to keep the natural
> alignment and add compile-time offset checks to document the transport
> layout. Does that address your concern?

Yes.

[...]


>>> diff --git a/xen/arch/arm/firmware/scmi-smc-multiagent.c 
>>> b/xen/arch/arm/firmware/scmi-smc-multiagent.c
>>> new file mode 100644
>>> index 0000000000..6c46f6cd36
>>> --- /dev/null
>>> +++ b/xen/arch/arm/firmware/scmi-smc-multiagent.c
>>> @@ -0,0 +1,792 @@
>>> +/* SPDX-License-Identifier: GPL-2.0-only */
>>> +/*
>>> + * SCI SCMI multi-agent driver, using SMC/HVC shmem as transport.
>>> + *
>>> + * Oleksii Moisieiev <oleksii_moisieiev@xxxxxxxx>
>>> + * Copyright (c) 2025 EPAM Systems
>>> + */
>>> +
>>> +#include <xen/acpi.h>
>>> +
>>> +#include <xen/device_tree.h>
>>> +#include <xen/init.h>
>>> +#include <xen/iocap.h>
>>> +#include <xen/err.h>
>>> +#include <xen/libfdt/libfdt.h>
>>> +#include <xen/string.h>
>>> +#include <xen/param.h>
>>> +#include <xen/sched.h>
>>> +#include <xen/vmap.h>
>>> +
>>> +#include <asm/firmware/sci.h>
>>> +#include <asm/smccc.h>
>>> +
>>> +#include "scmi-proto.h"
>>> +#include "scmi-shmem.h"
>>> +
>>> +struct scmi_channel {
>>> +    uint32_t agent_id;
>> If all agent_ids large than UINT8_MAX are invalid, why do you need
>> uint32_t here?
>
> According to the DEN0056F document:
>
> https://support.arm.com/documentation/den0056/f/?lang=en
>
> agent_id is uint32_t  in SCMI messages.
>

Okay, so agent_id is 32-bit value. I can imagine a platform that have
agent IDs 0x100, 0x200, 0x300 and so on. I believe, in this case your
code will not work as intended, because you limit agent id to 8 bits in
domain structure.


[...]

>>> +static int collect_agent_id(struct scmi_channel *agent_channel)
>>> +{
>>> +    int ret;
>>> +    scmi_msg_header_t hdr;
>>> +    struct scmi_msg_base_discover_agent_p2a da_rx;
>>> +    struct scmi_msg_base_discover_agent_a2p da_tx;
>>> +
>>> +    ret = map_channel_memory(agent_channel);
>>> +    if ( ret )
>>> +        return ret;
>>> +
>>> +    hdr.id = SCMI_BASE_DISCOVER_AGENT;
>>> +    hdr.type = 0;
>>> +    hdr.protocol = SCMI_BASE_PROTOCOL;
>>> +
>>> +    da_tx.agent_id = agent_channel->agent_id;
>>> +
>>> +    ret = do_smc_xfer(agent_channel, &hdr, &da_tx, sizeof(da_tx), &da_rx,
>>> +                        sizeof(da_rx));
>>> +    if ( agent_channel->domain_id != DOMID_XEN )
>>> +        unmap_channel_memory(agent_channel);
>>> +    if ( ret )
>> On error, Xen's channel will be mapped still. Is this intended behaviour?
> It remains mapped when collect_agent_id() returns, but the existing
> caller cleans it up. scmi_probe() marks the management channel DOMID_XEN
> before discovery. If discovery fails, it jumps to error, calls
> unmap_channel_memory(channel), then free_channel_list(). Thus the current
> probe failure path does not leak the mapping.
>
>
> On successful discovery the management mapping is deliberately retained
> for subsequent permission and reset requests. Secondary discovery
> mappings are always removed by collect_agent_id(), whether the transfer
> succeeds or fails.

Ah, okay. Thank you for clarification.

[...]

--
WBR, Volodymyr