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
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID:
 <PAXPR04MB8459322CED779885532BD852888D2@PAXPR04MB8459.eurprd04.prod.outlook.com>
Date: Tue, 20 Aug 2024 02:49:02 +0000
From: Peng Fan <peng.fan@....com>
To: Florian Fainelli <florian.fainelli@...adcom.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

> 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.

But I think it maybe better to add check if 'count' and 'from'
are not 4 bytes aligned. 

> +	}
> +
> +	/* Ensure all reads from I/O have completed */
> +	rmb();

Is there a need to use rmb here? I am not sure, just wonder.

> +}
> +
> +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.

Regards,
Peng.


> +}
> +
> +static struct scmi_shmem_io_ops shmem_io_ops32 = {
> +	.fromio	= shmem_memcpy_fromio32,
> +	.toio	= shmem_memcpy_toio32,
> +};
> +
> +/* Wrappers are needed for proper memcpy_{from,to}_io expansion
> by the
> + * pre-processor.
> + */
> +static inline void shmem_memcpy_fromio(void *to,
> +				       const volatile void __iomem
> *from,
> +				       size_t count)
> +{
> +	memcpy_fromio(to, from, count);
> +}
> +
> +static inline void shmem_memcpy_toio(volatile void __iomem *to,
> +				     const void *from,
> +				     size_t count)
> +{
> +	memcpy_toio(to, from, count);
> +}
> +
> +static struct scmi_shmem_io_ops shmem_io_ops_default = {
> +	.fromio = shmem_memcpy_fromio,
> +	.toio	= shmem_memcpy_toio,
> +};
> +
>  static void shmem_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 copy_toio)
>  {
>  	ktime_t stop;
> 
> @@ -73,7 +131,7 @@ static void shmem_tx_prepare(struct
> scmi_shared_mem __iomem *shmem,
>  	iowrite32(sizeof(shmem->msg_header) + xfer->tx.len,
> &shmem->length);
>  	iowrite32(pack_scmi_header(&xfer->hdr), &shmem-
> >msg_header);
>  	if (xfer->tx.buf)
> -		memcpy_toio(shmem->msg_payload, xfer->tx.buf,
> xfer->tx.len);
> +		copy_toio(shmem->msg_payload, xfer->tx.buf, xfer-
> >tx.len);
>  }
> 
>  static u32 shmem_read_header(struct scmi_shared_mem __iomem
> *shmem) @@ -82,7 +140,8 @@ static u32 shmem_read_header(struct
> scmi_shared_mem __iomem *shmem)  }
> 
>  static void shmem_fetch_response(struct scmi_shared_mem __iomem
> *shmem,
> -				 struct scmi_xfer *xfer)
> +				 struct scmi_xfer *xfer,
> +				 shmem_copy_fromio_t copy_fromio)
>  {
>  	size_t len = ioread32(&shmem->length);
> 
> @@ -91,11 +150,12 @@ static void shmem_fetch_response(struct
> scmi_shared_mem __iomem *shmem,
>  	xfer->rx.len = min_t(size_t, xfer->rx.len, len > 8 ? len - 8 : 0);
> 
>  	/* Take a copy to the rx buffer.. */
> -	memcpy_fromio(xfer->rx.buf, shmem->msg_payload + 4, xfer-
> >rx.len);
> +	copy_fromio(xfer->rx.buf, shmem->msg_payload + 4, xfer-
> >rx.len);
>  }
> 
>  static void shmem_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
> copy_fromio)
>  {
>  	size_t len = ioread32(&shmem->length);
> 
> @@ -103,7 +163,7 @@ static void shmem_fetch_notification(struct
> scmi_shared_mem __iomem *shmem,
>  	xfer->rx.len = min_t(size_t, max_len, len > 4 ? len - 4 : 0);
> 
>  	/* Take a copy to the rx buffer.. */
> -	memcpy_fromio(xfer->rx.buf, shmem->msg_payload, xfer-
> >rx.len);
> +	copy_fromio(xfer->rx.buf, shmem->msg_payload, xfer->rx.len);
>  }
> 
>  static void shmem_clear_channel(struct scmi_shared_mem __iomem
> *shmem) @@ -139,7 +199,8 @@ static bool
> shmem_channel_intr_enabled(struct scmi_shared_mem __iomem
> *shmem)
> 
>  static void __iomem *shmem_setup_iomap(struct scmi_chan_info
> *cinfo,
>  				       struct device *dev, bool tx,
> -				       struct resource *res)
> +				       struct resource *res,
> +				       struct scmi_shmem_io_ops
> **ops)
>  {
>  	struct device_node *shmem __free(device_node);
>  	const char *desc = tx ? "Tx" : "Rx";
> @@ -148,6 +209,7 @@ static void __iomem
> *shmem_setup_iomap(struct scmi_chan_info *cinfo,
>  	struct resource lres = {};
>  	resource_size_t size;
>  	void __iomem *addr;
> +	u32 reg_io_width;
> 
>  	shmem = of_parse_phandle(cdev->of_node, "shmem", idx);
>  	if (!shmem)
> @@ -173,6 +235,16 @@ static void __iomem
> *shmem_setup_iomap(struct scmi_chan_info *cinfo,
>  		return IOMEM_ERR_PTR(-EADDRNOTAVAIL);
>  	}
> 
> +	of_property_read_u32(shmem, "reg-io-width", &reg_io_width);
> +	switch (reg_io_width) {
> +	case 4:
> +		*ops = &shmem_io_ops32;
> +		break;
> +	default:
> +		*ops = &shmem_io_ops_default;
> +		break;
> +	}
> +
>  	return addr;
>  }
> 
> --
> 2.34.1
> 


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ