| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <1a486ead-3a65-4ef9-a562-c23ea7026b9e@broadcom.com>
Date: Tue, 20 Aug 2024 09:34:59 -0700
From: Florian Fainelli <florian.fainelli@...adcom.com>
To: Peng Fan <peng.fan@....com>,
"linux-arm-kernel@...ts.infradead.org" <linux-arm-kernel@...ts.infradead.org>
Cc: Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>, Sudeep Holla <sudeep.holla@....com>,
Cristian Marussi <cristian.marussi@....com>,
"open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS"
<devicetree@...r.kernel.org>, open list <linux-kernel@...r.kernel.org>,
"open list:SYSTEM CONTROL & POWER/MANAGEMENT INTERFACE"
<arm-scmi@...r.kernel.org>,
"justin.chen@...adcom.com" <justin.chen@...adcom.com>,
"opendmb@...il.com" <opendmb@...il.com>,
"kapil.hali@...adcom.com" <kapil.hali@...adcom.com>,
"bcm-kernel-feedback-list@...adcom.com"
<bcm-kernel-feedback-list@...adcom.com>
Subject: Re: [PATCH v3 2/2] firmware: arm_scmi: Support 'reg-io-width'
property for shared memory
On 8/19/24 19:49, Peng Fan wrote:
>> Subject: [PATCH v3 2/2] firmware: arm_scmi: Support 'reg-io-width'
>> property for shared memory
>>
>> Some shared memory areas might only support a certain access width,
>> such as 32-bit, which memcpy_{from,to}_io() does not adhere to at
>> least on ARM64 by making both 8-bit and 64-bit accesses to such
>> memory.
>>
>> Update the shmem layer to support reading from and writing to such
>> shared memory area using the specified I/O width in the Device Tree.
>> The various transport layers making use of the shmem.c code are
>> updated accordingly to pass the I/O accessors that they store.
>>
>> Signed-off-by: Florian Fainelli <florian.fainelli@...adcom.com>
>> ---
>> drivers/firmware/arm_scmi/common.h | 32 ++++++-
>> .../arm_scmi/scmi_transport_mailbox.c | 13 ++-
>> .../firmware/arm_scmi/scmi_transport_optee.c | 11 ++-
>> .../firmware/arm_scmi/scmi_transport_smc.c | 11 ++-
>> drivers/firmware/arm_scmi/shmem.c | 86 +++++++++++++++++-
>> -
>> 5 files changed, 132 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/firmware/arm_scmi/common.h
>> b/drivers/firmware/arm_scmi/common.h
>> index 69928bbd01c2..73bb496fac01 100644
>> --- a/drivers/firmware/arm_scmi/common.h
>> +++ b/drivers/firmware/arm_scmi/common.h
>> @@ -316,6 +316,26 @@ enum scmi_bad_msg {
>> MSG_MBOX_SPURIOUS = -5,
>> };
>>
>> +/* Used for compactness and signature validation of the function
>> +pointers being
>> + * passed.
>> + */
>> +typedef void (*shmem_copy_toio_t)(volatile void __iomem *to, const
>> void *from,
>> + size_t count);
>> +typedef void (*shmem_copy_fromio_t)(void *to, const volatile void
>> __iomem *from,
>> + size_t count);
>> +
>> +/**
>> + * struct scmi_shmem_io_ops - I/O operations to read from/write to
>> + * Shared Memory
>> + *
>> + * @toio: Copy data to the shared memory area
>> + * @fromio: Copy data from the shared memory area */ struct
>> +scmi_shmem_io_ops {
>> + shmem_copy_fromio_t fromio;
>> + shmem_copy_toio_t toio;
>> +};
>> +
>> /* shmem related declarations */
>> struct scmi_shared_mem;
>>
>> @@ -336,13 +356,16 @@ struct scmi_shared_mem; struct
>> scmi_shared_mem_operations {
>> void (*tx_prepare)(struct scmi_shared_mem __iomem
>> *shmem,
>> struct scmi_xfer *xfer,
>> - struct scmi_chan_info *cinfo);
>> + struct scmi_chan_info *cinfo,
>> + shmem_copy_toio_t toio);
>> u32 (*read_header)(struct scmi_shared_mem __iomem
>> *shmem);
>>
>> void (*fetch_response)(struct scmi_shared_mem __iomem
>> *shmem,
>> - struct scmi_xfer *xfer);
>> + struct scmi_xfer *xfer,
>> + shmem_copy_fromio_t fromio);
>> void (*fetch_notification)(struct scmi_shared_mem __iomem
>> *shmem,
>> - size_t max_len, struct scmi_xfer
>> *xfer);
>> + size_t max_len, struct scmi_xfer
>> *xfer,
>> + shmem_copy_fromio_t fromio);
>> void (*clear_channel)(struct scmi_shared_mem __iomem
>> *shmem);
>> bool (*poll_done)(struct scmi_shared_mem __iomem *shmem,
>> struct scmi_xfer *xfer);
>> @@ -350,7 +373,8 @@ struct scmi_shared_mem_operations {
>> bool (*channel_intr_enabled)(struct scmi_shared_mem
>> __iomem *shmem);
>> void __iomem *(*setup_iomap)(struct scmi_chan_info *cinfo,
>> struct device *dev,
>> - bool tx, struct resource *res);
>> + bool tx, struct resource *res,
>> + struct scmi_shmem_io_ops **ops);
>> };
>>
>> const struct scmi_shared_mem_operations
>> *scmi_shared_mem_operations_get(void);
>> diff --git a/drivers/firmware/arm_scmi/scmi_transport_mailbox.c
>> b/drivers/firmware/arm_scmi/scmi_transport_mailbox.c
>> index dc5ca894d5eb..1a2e90e5c765 100644
>> --- a/drivers/firmware/arm_scmi/scmi_transport_mailbox.c
>> +++ b/drivers/firmware/arm_scmi/scmi_transport_mailbox.c
>> @@ -25,6 +25,7 @@
>> * @chan_platform_receiver: Optional Platform Receiver mailbox
>> unidirectional channel
>> * @cinfo: SCMI channel info
>> * @shmem: Transmit/Receive shared memory area
>> + * @io_ops: Transport specific I/O operations
>> */
>> struct scmi_mailbox {
>> struct mbox_client cl;
>> @@ -33,6 +34,7 @@ struct scmi_mailbox {
>> struct mbox_chan *chan_platform_receiver;
>> struct scmi_chan_info *cinfo;
>> struct scmi_shared_mem __iomem *shmem;
>> + struct scmi_shmem_io_ops *io_ops;
>> };
>>
>> #define client_to_scmi_mailbox(c) container_of(c, struct scmi_mailbox,
>> cl) @@ -43,7 +45,8 @@ static void tx_prepare(struct mbox_client *cl,
>> void *m) {
>> struct scmi_mailbox *smbox = client_to_scmi_mailbox(cl);
>>
>> - core->shmem->tx_prepare(smbox->shmem, m, smbox->cinfo);
>> + core->shmem->tx_prepare(smbox->shmem, m, smbox->cinfo,
>> + smbox->io_ops->toio);
>> }
>>
>> static void rx_callback(struct mbox_client *cl, void *m) @@ -197,7
>> +200,8 @@ static int mailbox_chan_setup(struct scmi_chan_info
>> *cinfo, struct device *dev,
>> if (!smbox)
>> return -ENOMEM;
>>
>> - smbox->shmem = core->shmem->setup_iomap(cinfo, dev, tx,
>> NULL);
>> + smbox->shmem = core->shmem->setup_iomap(cinfo, dev, tx,
>> NULL,
>> + &smbox->io_ops);
>> if (IS_ERR(smbox->shmem))
>> return PTR_ERR(smbox->shmem);
>>
>> @@ -298,7 +302,7 @@ static void mailbox_fetch_response(struct
>> scmi_chan_info *cinfo, {
>> struct scmi_mailbox *smbox = cinfo->transport_info;
>>
>> - core->shmem->fetch_response(smbox->shmem, xfer);
>> + core->shmem->fetch_response(smbox->shmem, xfer,
>> +smbox->io_ops->fromio);
>> }
>>
>> static void mailbox_fetch_notification(struct scmi_chan_info *cinfo,
>> @@ -306,7 +310,8 @@ static void mailbox_fetch_notification(struct
>> scmi_chan_info *cinfo, {
>> struct scmi_mailbox *smbox = cinfo->transport_info;
>>
>> - core->shmem->fetch_notification(smbox->shmem, max_len,
>> xfer);
>> + core->shmem->fetch_notification(smbox->shmem, max_len,
>> xfer,
>> + smbox->io_ops->fromio);
>> }
>>
>> static void mailbox_clear_channel(struct scmi_chan_info *cinfo) diff --
>> git a/drivers/firmware/arm_scmi/scmi_transport_optee.c
>> b/drivers/firmware/arm_scmi/scmi_transport_optee.c
>> index 08911f40d1ff..2be4124c6826 100644
>> --- a/drivers/firmware/arm_scmi/scmi_transport_optee.c
>> +++ b/drivers/firmware/arm_scmi/scmi_transport_optee.c
>> @@ -114,6 +114,7 @@ enum scmi_optee_pta_cmd {
>> * @req.shmem: Virtual base address of the shared memory
>> * @req.msg: Shared memory protocol handle for SCMI request and
>> * synchronous response
>> + * @io_ops: Transport specific I/O operations
>> * @tee_shm: TEE shared memory handle @req or NULL if using
>> IOMEM shmem
>> * @link: Reference in agent's channel list
>> */
>> @@ -128,6 +129,7 @@ struct scmi_optee_channel {
>> struct scmi_shared_mem __iomem *shmem;
>> struct scmi_msg_payld *msg;
>> } req;
>> + struct scmi_shmem_io_ops *io_ops;
>> struct tee_shm *tee_shm;
>> struct list_head link;
>> };
>> @@ -350,7 +352,8 @@ static int setup_dynamic_shmem(struct device
>> *dev, struct scmi_optee_channel *ch static int
>> setup_static_shmem(struct device *dev, struct scmi_chan_info *cinfo,
>> struct scmi_optee_channel *channel) {
>> - channel->req.shmem = core->shmem->setup_iomap(cinfo, dev,
>> true, NULL);
>> + channel->req.shmem = core->shmem->setup_iomap(cinfo, dev,
>> true, NULL,
>> + &channel-
>>> io_ops);
>> if (IS_ERR(channel->req.shmem))
>> return PTR_ERR(channel->req.shmem);
>>
>> @@ -465,7 +468,8 @@ static int scmi_optee_send_message(struct
>> scmi_chan_info *cinfo,
>> ret = invoke_process_msg_channel(channel,
>> core->msg-
>>> command_size(xfer));
>> } else {
>> - core->shmem->tx_prepare(channel->req.shmem, xfer,
>> cinfo);
>> + core->shmem->tx_prepare(channel->req.shmem, xfer,
>> cinfo,
>> + channel->io_ops->toio);
>> ret = invoke_process_smt_channel(channel);
>> }
>>
>> @@ -484,7 +488,8 @@ static void scmi_optee_fetch_response(struct
>> scmi_chan_info *cinfo,
>> core->msg->fetch_response(channel->req.msg,
>> channel->rx_len, xfer);
>> else
>> - core->shmem->fetch_response(channel->req.shmem,
>> xfer);
>> + core->shmem->fetch_response(channel->req.shmem,
>> xfer,
>> + channel->io_ops->fromio);
>> }
>>
>> static void scmi_optee_mark_txdone(struct scmi_chan_info *cinfo, int
>> ret, diff --git a/drivers/firmware/arm_scmi/scmi_transport_smc.c
>> b/drivers/firmware/arm_scmi/scmi_transport_smc.c
>> index c6c69a17a9cc..04e715ec1570 100644
>> --- a/drivers/firmware/arm_scmi/scmi_transport_smc.c
>> +++ b/drivers/firmware/arm_scmi/scmi_transport_smc.c
>> @@ -45,6 +45,7 @@
>> * @irq: An optional IRQ for completion
>> * @cinfo: SCMI channel info
>> * @shmem: Transmit/Receive shared memory area
>> + * @io_ops: Transport specific I/O operations
>> * @shmem_lock: Lock to protect access to Tx/Rx shared memory area.
>> * Used when NOT operating in atomic mode.
>> * @inflight: Atomic flag to protect access to Tx/Rx shared memory
>> area.
>> @@ -60,6 +61,7 @@ struct scmi_smc {
>> int irq;
>> struct scmi_chan_info *cinfo;
>> struct scmi_shared_mem __iomem *shmem;
>> + struct scmi_shmem_io_ops *io_ops;
>> /* Protect access to shmem area */
>> struct mutex shmem_lock;
>> #define INFLIGHT_NONE MSG_TOKEN_MAX
>> @@ -144,7 +146,8 @@ static int smc_chan_setup(struct
>> scmi_chan_info *cinfo, struct device *dev,
>> if (!scmi_info)
>> return -ENOMEM;
>>
>> - scmi_info->shmem = core->shmem->setup_iomap(cinfo, dev,
>> tx, &res);
>> + scmi_info->shmem = core->shmem->setup_iomap(cinfo, dev,
>> tx, &res,
>> + &scmi_info-
>>> io_ops);
>> if (IS_ERR(scmi_info->shmem))
>> return PTR_ERR(scmi_info->shmem);
>>
>> @@ -229,7 +232,8 @@ static int smc_send_message(struct
>> scmi_chan_info *cinfo,
>> */
>> smc_channel_lock_acquire(scmi_info, xfer);
>>
>> - core->shmem->tx_prepare(scmi_info->shmem, xfer, cinfo);
>> + core->shmem->tx_prepare(scmi_info->shmem, xfer, cinfo,
>> + scmi_info->io_ops->toio);
>>
>> if (scmi_info->cap_id != ULONG_MAX)
>> arm_smccc_1_1_invoke(scmi_info->func_id,
>> scmi_info->cap_id, 0, @@ -253,7 +257,8 @@ static void
>> smc_fetch_response(struct scmi_chan_info *cinfo, {
>> struct scmi_smc *scmi_info = cinfo->transport_info;
>>
>> - core->shmem->fetch_response(scmi_info->shmem, xfer);
>> + core->shmem->fetch_response(scmi_info->shmem, xfer,
>> + scmi_info->io_ops->fromio);
>> }
>>
>> static void smc_mark_txdone(struct scmi_chan_info *cinfo, int ret, diff
>> --git a/drivers/firmware/arm_scmi/shmem.c
>> b/drivers/firmware/arm_scmi/shmem.c
>> index 01d8a9398fe8..aded5f1cd49f 100644
>> --- a/drivers/firmware/arm_scmi/shmem.c
>> +++ b/drivers/firmware/arm_scmi/shmem.c
>> @@ -34,9 +34,67 @@ struct scmi_shared_mem {
>> u8 msg_payload[];
>> };
>>
>> +static inline void shmem_memcpy_fromio32(void *to,
>> + const volatile void __iomem
>> *from,
>> + size_t count)
>> +{
>> + while (count) {
>> + *(u32 *)to = __raw_readl(from);
>> + from += 4;
>> + to += 4;
>> + count -= 4;
>
> This may not have issue, considering the 'count % 4' will be 0
> and 'from' is 4 bytes aligned.
Correct, this cannot possibly happen today by virtue of msg->payload
being naturally aligned on a 4 bytes boundary.
>
> But I think it maybe better to add check if 'count' and 'from'
> are not 4 bytes aligned.
Humm, what would be the fallback, or should we just WARN()?
>
>> + }
>> +
>> + /* Ensure all reads from I/O have completed */
>> + rmb();
>
> Is there a need to use rmb here? I am not sure, just wonder.
I personally do not think there is because there is an implicit data
dependency eventually we will be consuming data that was copied into
msg->payload and that will ensure that the data is there.
>
>> +}
>> +
>> +static inline void shmem_memcpy_toio32(volatile void __iomem *to,
>> + const void *from,
>> + size_t count)
>> +{
>> + while (count) {
>> + __raw_writel(*(u32 *)from, to);
>> + from += 4;
>> + to += 4;
>> + count -= 4;
>> + }
>
> Ditto.
>
>> +
>> + /* Ensure all writes to I/O have completed */
>> + wmb();
>
> This maybe not needed.
> The mailbox will use "writel", the SMC will use "smc",
> the virtio will have "hvc", both will have barrier I think.
Yes, those are all implicit barriers, I was attempting to address
Christian's concerns raised in v2:
https://lore.kernel.org/all/Zr-GJts3Gu6GEkhC@pluto/
--
Florian
Powered by blists - more mailing lists