Re: [PATCH v4 00/16] Introduce SCMI VirtIO transport

Re: [PATCH v4 00/16] Introduce SCMI VirtIO transport

[Christoph Hellwig]

On Fri, Jun 11, 2021 at 05:59:21PM +0100, Cristian Marussi wrote:
> Hi all,
> 
> I'm posting this V4 series starting from the work done up to V3 by
> OpenSynergy.

Who is 'OpenSynergy'?

> The main aim of this rework is to simplify where possible the SCMI VirtIO
> support added in V3 by adding upfront and then using some new mechanisms in
> the SCMI Core Transport layer.

And what is 'SCMI', and why would anyone want a new virtio transport?
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v4 13/16] dt-bindings: arm: Add virtio transport for SCMI

[Rob Herring]

On Fri, 11 Jun 2021 17:59:34 +0100, Cristian Marussi wrote:
> From: Igor Skalkin <igor.skalkin@opensynergy.com>
> 
> Document the properties for arm,scmi-virtio compatible nodes.
> The backing virtio SCMI device is described in patch [1].
> 
> While doing that, make shmem property required only for pre-existing
> mailbox and smc transports, since virtio-scmi does not need it.
> 
> [1] https://lists.oasis-open.org/archives/virtio-comment/202102/msg00018.html
> 
> CC: Rob Herring <robh+dt@kernel.org>
> CC: devicetree@vger.kernel.org
> Signed-off-by: Igor Skalkin <igor.skalkin@opensynergy.com>
> [ Peter: Adapted patch for submission to upstream. ]
> Co-developed-by: Peter Hilber <peter.hilber@opensynergy.com>
> Signed-off-by: Peter Hilber <peter.hilber@opensynergy.com>
> [ Cristian: converted to yaml format, moved shmen required property. ]
> Co-developed-by: Cristian Marussi <cristian.marussi@arm.com>
> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> ---
> v3 --> V4
> - convertd to YAML
> - make shmem required only for pre-existing mailbox and smc transport
> - updated VirtIO specification patch message reference
> - dropped virtio-mmio SCMI device example since really not pertinent to
>   virtio-scmi dt bindings transport: it is not even referenced in SCMI
>   virtio DT node since they are enumerated by VirtIO subsystem and there
>   could be PCI based SCMI devices anyway.
> ---
>  Documentation/devicetree/bindings/firmware/arm,scmi.yaml | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 

Reviewed-by: Rob Herring <robh@kernel.org>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[virtio-dev] Re: [PATCH v4 13/16] dt-bindings: arm: Add virtio transport for SCMI

[Peter Hilber]

On 11.06.21 18:59, Cristian Marussi wrote:
> From: Igor Skalkin <igor.skalkin@opensynergy.com>
> 
> Document the properties for arm,scmi-virtio compatible nodes.
> The backing virtio SCMI device is described in patch [1].
> 
> While doing that, make shmem property required only for pre-existing
> mailbox and smc transports, since virtio-scmi does not need it.
> 
> [1] https://lists.oasis-open.org/archives/virtio-comment/202102/msg00018.html
> 
> CC: Rob Herring <robh+dt@kernel.org>
> CC: devicetree@vger.kernel.org
> Signed-off-by: Igor Skalkin <igor.skalkin@opensynergy.com>
> [ Peter: Adapted patch for submission to upstream. ]
> Co-developed-by: Peter Hilber <peter.hilber@opensynergy.com>
> Signed-off-by: Peter Hilber <peter.hilber@opensynergy.com>
> [ Cristian: converted to yaml format, moved shmen required property. ]
> Co-developed-by: Cristian Marussi <cristian.marussi@arm.com>
> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> ---
> v3 --> V4
> - convertd to YAML
> - make shmem required only for pre-existing mailbox and smc transport
> - updated VirtIO specification patch message reference
> - dropped virtio-mmio SCMI device example since really not pertinent to
>    virtio-scmi dt bindings transport: it is not even referenced in SCMI
>    virtio DT node since they are enumerated by VirtIO subsystem and there
>    could be PCI based SCMI devices anyway.
> ---
>   Documentation/devicetree/bindings/firmware/arm,scmi.yaml | 8 +++++++-
>   1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> index cebf6ffe70d5..5c4c6782e052 100644
> --- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> +++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> @@ -34,6 +34,10 @@ properties:
>         - description: SCMI compliant firmware with ARM SMC/HVC transport
>           items:
>             - const: arm,scmi-smc
> +      - description: SCMI compliant firmware with SCMI Virtio transport.
> +                     The virtio transport only supports a single device.
> +        items:
> +          - const: arm,scmi-virtio
>   
>     interrupts:
>       description:
> @@ -172,6 +176,7 @@ patternProperties:
>         Each sub-node represents a protocol supported. If the platform
>         supports a dedicated communication channel for a particular protocol,
>         then the corresponding transport properties must be present.
> +      The virtio transport does not support a dedicated communication channel.
>   
>       properties:
>         reg:
> @@ -195,7 +200,6 @@ patternProperties:
>   
>   required:
>     - compatible
> -  - shmem
>   
>   if:
>     properties:
> @@ -209,6 +213,7 @@ then:
>   
>     required:
>       - mboxes
> +    - shmem
>   
>   else:
>     if:
> @@ -219,6 +224,7 @@ else:
>     then:
>       required:
>         - arm,smc-id
> +      - shmem
>   
>   examples:
>     - |
> 

Maybe a minimal example for arm,scmi-virtio could be added, such as below:

diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml 
b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
index 5c4c6782e052..576faf970c1b 100644
--- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
+++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
@@ -344,4 +344,19 @@ examples:
          };
      };

+  - |
+    firmware {
+        scmi {
+            compatible = "arm,scmi-virtio";
+
+            #address-cells = <1>;
+            #size-cells = <0>;
+
+            scmi_devpd2: protocol@11 {
+                reg = <0x11>;
+                #power-domain-cells = <1>;
+            };
+        };
+    };
+
  ...

---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

[virtio-dev] Re: [PATCH v4 01/16] firmware: arm_scmi: Fix max pending messages boundary check

[Peter Hilber]

Hi Cristian,

please find some remarks to the patch series in this email and the 
following.

On 11.06.21 18:59, Cristian Marussi wrote:
> SCMI message headers carry a sequence number and such field is sized to
> allow for MSG_TOKEN_MAX distinct numbers; moreover zero is not really an
> acceptable maximum number of pending in-flight messages.
> 
> Fix accordignly the checks performed on the value exported by transports
> in scmi_desc.max_msg.
> 
> Reported-by: Vincent Guittot <vincent.guittot@linaro.org>
> Fixes: aa4f886f3893 ("firmware: arm_scmi: add basic driver infrastructure for SCMI")
> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> ---
>  drivers/firmware/arm_scmi/driver.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> index 66e5e694be7d..6713b259f1e6 100644
> --- a/drivers/firmware/arm_scmi/driver.c
> +++ b/drivers/firmware/arm_scmi/driver.c
> @@ -1025,8 +1025,9 @@ static int __scmi_xfer_info_init(struct scmi_info *sinfo,
>  	const struct scmi_desc *desc = sinfo->desc;
>  
>  	/* Pre-allocated messages, no more than what hdr.seq can support */
> -	if (WARN_ON(desc->max_msg >= MSG_TOKEN_MAX)) {
> -		dev_err(dev, "Maximum message of %d exceeds supported %ld\n",
> +	if (WARN_ON(!desc->max_msg || desc->max_msg > MSG_TOKEN_MAX)) {
> +		dev_err(dev,
> +			"Invalid max_msg %d. Maximum messages supported %ld.\n",

%ld -> %lu

>  			desc->max_msg, MSG_TOKEN_MAX);
>  		return -EINVAL;
>  	}
>

---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

[virtio-dev] Re: [PATCH v4 04/16] firmware: arm_scmi: Introduce monotonically increasing tokens

[Peter Hilber]

I find `monotonically increasing tokens' misleading, since the token may 
wrap around quite often during normal usage. How about `modulo 
incrementing'?

On 11.06.21 18:59, Cristian Marussi wrote:
> Tokens are sequence numbers embedded in the each SCMI message header: they
> are used to correlate commands with responses (and delayed responses), but
> their usage and policy of selection is entirely up to the caller (usually
> the OSPM agent), while they are completely opaque to the callee (SCMI
> server platform) which merely copies them back from the command into the
> response message header.
> This also means that the platform does not, can not and should not enforce
> any kind of policy on received messages depending on the contained sequence
> number: platform can perfectly handle concurrent requests carrying the same
> identifiying token if that should happen.
> 
> Moreover the platform is not required to produce in-order responses to
> agent requests, the only constraint in these regards is that in case of
> an asynchronous message the delayed response must be sent after the
> immediate response for the synchronous part of the command transaction.
> 
> Currenly the SCMI stack of the OSPM agent selects a token for the egressing
> commands picking the lowest possible number which is not already in use by
> an existing in-flight transaction, which means, in other words, that we
> immediately reuse any token after its transaction has completed or it has
> timed out: this policy indeed does simplify management and lookup of tokens
> and associated xfers.
> 
> Under the above assumptions and constraints, since there is really no state
> shared between the agent and the platform to let the platform know when a
> token and its associated message has timed out, the current policy of early
> reuse of tokens can easily lead to the situation in which a spurious or
> late received response (or delayed_response), related to an old stale and
> timed out transaction, can be wrongly associated to a newer valid in-flight
> xfer that just happens to have reused the same token.
> 
> This misbehaviour on such ghost responses is more easily exposed on those
> transports that naturally have an higher level of parallelism in processing
> multiple concurrent in-flight messages.
> 
> This commit introduces a new policy of selection of tokens for the OSPM
> agent: each new transfer now gets the next available and monotonically
> increasing token, until tokens are exhausted and the counter rolls over.
> 
> Such new policy mitigates the above issues with ghost responses since the
> tokens are now reused as late as possible (when they roll back ideally)
> and so it is much easier to identify such ghost responses to stale timed
> out transactions: this also helps in simplifying the specific transports
> implementation since stale transport messages can be easily identified
> and discarded early on in the rx path without the need to cross check
> their actual state with the core transport layer.
> This mitigation is even more effective when, as is usually the case, the
> maximum number of pending messages is capped by the platform to a much
> lower number than the whole possible range of tokens values (2^10).
> 
> This internal policy change in the core SCMI transport layer is fully
> transparent to the specific transports so it has not and should not have
> any impact on the transports implementation.
> 
> The empirically observed cost of such new procedure of token selection
> amounts in the best case to ~10us out of an observed full transaction cost
> of 3ms for the completion of a synchronous sensor reading command on a
> platform supporting commands completion interrupts.
> 
> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> ---
>  drivers/firmware/arm_scmi/common.h |  23 +++
>  drivers/firmware/arm_scmi/driver.c | 243 +++++++++++++++++++++++++----
>  2 files changed, 233 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h
> index 6bb734e0e3ac..e64c5ca9ee7c 100644
> --- a/drivers/firmware/arm_scmi/common.h
> +++ b/drivers/firmware/arm_scmi/common.h
> @@ -14,7 +14,10 @@
>  #include <linux/device.h>
>  #include <linux/errno.h>
>  #include <linux/kernel.h>
> +#include <linux/hashtable.h>
> +#include <linux/list.h>
>  #include <linux/module.h>
> +#include <linux/refcount.h>
>  #include <linux/scmi_protocol.h>
>  #include <linux/types.h>
>  
> @@ -127,6 +130,21 @@ struct scmi_msg {
>  	size_t len;
>  };
>  
> +/**
> + * An helper macro to lookup an xfer from the @pending_xfers hashtable
> + * using the message sequence number token as a key.
> + */
> +#define XFER_FIND(__ht, __k)					\
> +({								\
> +	typeof(__k) k_ = __k;					\
> +	struct scmi_xfer *xfer_ = NULL;				\
> +								\
> +	hash_for_each_possible((__ht), xfer_, node, k_)		\
> +		if (xfer_->hdr.seq == k_)			\
> +			break;					\
> +	 xfer_;							\

There is an extra space before the return value.

> +})
> +
>  /**
>   * struct scmi_xfer - Structure representing a message flow
>   *
> @@ -138,6 +156,9 @@ struct scmi_msg {
>   *	buffer for the rx path as we use for the tx path.
>   * @done: command message transmit completion event
>   * @async_done: pointer to delayed response message received event completion
> + * @users: A refcount to track the active users for this xfer
> + * @node: An hlist_node reference used to store this xfer, alternatively, on
> + *	  the free list @free_xfers or in the @pending_xfers hashtable
>   */
>  struct scmi_xfer {
>  	int transfer_id;
> @@ -146,6 +167,8 @@ struct scmi_xfer {
>  	struct scmi_msg rx;
>  	struct completion done;
>  	struct completion *async_done;
> +	refcount_t users;
> +	struct hlist_node node;
>  };
>  
>  struct scmi_xfer_ops;
> diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> index 20f8f0581f3a..f0b20ddb24f4 100644
> --- a/drivers/firmware/arm_scmi/driver.c
> +++ b/drivers/firmware/arm_scmi/driver.c
> @@ -21,6 +21,7 @@
>  #include <linux/io.h>
>  #include <linux/kernel.h>
>  #include <linux/ktime.h>
> +#include <linux/hashtable.h>
>  #include <linux/list.h>
>  #include <linux/module.h>
>  #include <linux/of_address.h>
> @@ -65,19 +66,29 @@ struct scmi_requested_dev {
>  	struct list_head node;
>  };
>  
> +#define SCMI_PENDING_XFERS_HT_ORDER_SZ	9

Micro-optimization note: This increases struct scmi_info size to > 8 kiB 
(on 64-bit architectures). A lower size of the hash table would be 
enough for some transports.

> +
>  /**
>   * struct scmi_xfers_info - Structure to manage transfer information
>   *
> - * @xfer_block: Preallocated Message array
>   * @xfer_alloc_table: Bitmap table for allocated messages.
>   *	Index of this bitmap table is also used for message
>   *	sequence identifier.
>   * @xfer_lock: Protection for message allocation
> + * @last_token: A counter to use as base to generate for monotonically
> + *		increasing tokens.
> + * @free_xfers: A free list for available to use xfers. It is initialized with
> + *		a number of xfers equal to the maximum allowed in-flight
> + *		messages.
> + * @pending_xfers: An hashtable, indexed by msg_hdr.seq, used to keep all the
> + *		   currently in-flight messages.
>   */
>  struct scmi_xfers_info {
> -	struct scmi_xfer *xfer_block;
>  	unsigned long *xfer_alloc_table;
>  	spinlock_t xfer_lock;
> +	atomic_t last_token;
> +	struct hlist_head free_xfers;
> +	DECLARE_HASHTABLE(pending_xfers, SCMI_PENDING_XFERS_HT_ORDER_SZ);
>  };
>  
>  /**
> @@ -203,6 +214,117 @@ void *scmi_notification_instance_data_get(const struct scmi_handle *handle)
>  	return info->notify_priv;
>  }
>  
> +/**
> + * scmi_xfer_token_set  - Reserve and set new token for the xfer at hand
> + *
> + * @minfo: Pointer to Tx/Rx Message management info based on channel type
> + * @xfer: The xfer to act upon
> + *
> + * Pick the next unused monotonically increasing token and set it into
> + * xfer->hdr.seq: picking a monotonically increasing value avoids immediate
> + * reuse of freshly completed or timed-out xfers, thus mitigating the risk
> + * of incorrect association of a late and expired xfer with a live in-flight
> + * transaction, both happening to re-use the same token identifier.
> + *
> + * Since platform is NOT required to answer our request in-order we should
> + * account for a few rare but possible scenarios:
> + *
> + *  - exactly 'next_token' may be NOT available so pick xfer_id >= next_token
> + *    using find_next_zero_bit() starting from candidate next_token bit
> + *
> + *  - all tokens ahead upto (MSG_TOKEN_ID_MASK - 1) are used in-flight but we
> + *    are plenty of free tokens at start, so try a second pass using
> + *    find_next_zero_bit() and starting from 0.
> + *
> + *  X = used in-flight
> + *
> + * Normal
> + * ------
> + *
> + *		|- xfer_id picked
> + *   -----------+----------------------------------------------------------
> + *   | | |X|X|X| | | | | | ... ... ... ... ... ... ... ... ... ... ...|X|X|
> + *   ----------------------------------------------------------------------
> + *		^
> + *		|- next_token
> + *
> + * Out-of-order pending at start
> + * -----------------------------
> + *
> + *	  |- xfer_id picked, last_token fixed
> + *   -----+----------------------------------------------------------------
> + *   |X|X| | | | |X|X| ... ... ... ... ... ... ... ... ... ... ... ...|X| |
> + *   ----------------------------------------------------------------------
> + *    ^
> + *    |- next_token
> + *
> + *
> + * Out-of-order pending at end
> + * ---------------------------
> + *
> + *	  |- xfer_id picked, last_token fixed
> + *   -----+----------------------------------------------------------------
> + *   |X|X| | | | |X|X| ... ... ... ... ... ... ... ... ... ... |X|X|X||X|X|
> + *   ----------------------------------------------------------------------
> + *								^
> + *								|- next_token
> + *
> + * Context: Assumes to be called with @xfer_lock already acquired.
> + *
> + * Return: 0 on Success or error
> + */
> +static int scmi_xfer_token_set(struct scmi_xfers_info *minfo,
> +			       struct scmi_xfer *xfer)
> +{
> +	unsigned long xfer_id, next_token;
> +
> +	/* Pick a candidate monotonic token in range [0, MSG_TOKEN_MAX - 1] */
> +	next_token = (atomic_inc_return(&minfo->last_token) &
> +		      (MSG_TOKEN_MAX - 1));
> +
> +	/* Pick the next available xfer_id >= next_token */
> +	xfer_id = find_next_zero_bit(minfo->xfer_alloc_table,
> +				     MSG_TOKEN_MAX, next_token);
> +	if (xfer_id == MSG_TOKEN_MAX) {
> +		/*
> +		 * After heavily out-of-order responses, there are no free
> +		 * tokens ahead, but only at start of xfer_alloc_table so
> +		 * try again from the beginning.
> +		 */
> +		xfer_id = find_next_zero_bit(minfo->xfer_alloc_table,
> +					     MSG_TOKEN_MAX, 0);
> +		/*
> +		 * Something is wrong if we got here since there can be a
> +		 * maximum number of (MSG_TOKEN_MAX - 1) in-flight messages
> +		 * but we have not found any free token [0, MSG_TOKEN_MAX - 1].
> +		 */
> +		if (WARN_ON_ONCE(xfer_id == MSG_TOKEN_MAX))
> +			return -ENOMEM;
> +	}
> +
> +	/* Update +/- last_token accordingly if we skipped some hole */
> +	if (xfer_id != next_token)
> +		atomic_add((int)(xfer_id - next_token), &minfo->last_token);
> +
> +	/* Set in-flight */
> +	set_bit(xfer_id, minfo->xfer_alloc_table);
> +	xfer->hdr.seq = (u16)xfer_id;
> +
> +	return 0;
> +}
> +
> +/**
> + * scmi_xfer_token_clear  - Release the token
> + *
> + * @minfo: Pointer to Tx/Rx Message management info based on channel type
> + * @xfer: The xfer to act upon
> + */
> +static inline void scmi_xfer_token_clear(struct scmi_xfers_info *minfo,
> +					 struct scmi_xfer *xfer)
> +{
> +	clear_bit(xfer->hdr.seq, minfo->xfer_alloc_table);
> +}
> +
>  /**
>   * scmi_xfer_get() - Allocate one message
>   *
> @@ -212,36 +334,49 @@ void *scmi_notification_instance_data_get(const struct scmi_handle *handle)
>   * Helper function which is used by various message functions that are
>   * exposed to clients of this driver for allocating a message traffic event.
>   *
> - * This function can sleep depending on pending requests already in the system
> - * for the SCMI entity. Further, this also holds a spinlock to maintain
> - * integrity of internal data structures.
> + * Picks an xfer from the free list @free_xfers (if any available), sets a
> + * monotonically increasing token and stores the inflight xfer into the
> + * @pending_xfers hashtable for later retrieval.
> + *
> + * The successfully initialized xfer is refcounted.
> + *
> + * Context: Holds @xfer_lock while manipulating @xfer_alloc_table and
> + *	    @free_xfers.
>   *
>   * Return: 0 if all went fine, else corresponding error.
>   */
>  static struct scmi_xfer *scmi_xfer_get(const struct scmi_handle *handle,
>  				       struct scmi_xfers_info *minfo)
>  {
> -	u16 xfer_id;
> +	int ret;
> +	unsigned long flags;
>  	struct scmi_xfer *xfer;
> -	unsigned long flags, bit_pos;
> -	struct scmi_info *info = handle_to_scmi_info(handle);
>  
> -	/* Keep the locked section as small as possible */
>  	spin_lock_irqsave(&minfo->xfer_lock, flags);
> -	bit_pos = find_first_zero_bit(minfo->xfer_alloc_table,
> -				      info->desc->max_msg);
> -	if (bit_pos == info->desc->max_msg) {
> +	if (hlist_empty(&minfo->free_xfers)) {
>  		spin_unlock_irqrestore(&minfo->xfer_lock, flags);
>  		return ERR_PTR(-ENOMEM);
>  	}
> -	set_bit(bit_pos, minfo->xfer_alloc_table);
> -	spin_unlock_irqrestore(&minfo->xfer_lock, flags);
>  
> -	xfer_id = bit_pos;
> +	/* grab an xfer from the free_list */
> +	xfer = hlist_entry(minfo->free_xfers.first, struct scmi_xfer, node);
> +	hlist_del_init(&xfer->node);
>  
> -	xfer = &minfo->xfer_block[xfer_id];
> -	xfer->hdr.seq = xfer_id;
> -	xfer->transfer_id = atomic_inc_return(&transfer_last_id);
> +	/* Pick and set monotonic token */
> +	ret = scmi_xfer_token_set(minfo, xfer);
> +	if (!ret) {
> +		hash_add(minfo->pending_xfers, &xfer->node, xfer->hdr.seq);
> +	} else {
> +		dev_err(handle->dev, "Failed to get monotonic token %d\n", ret);
> +		hlist_add_head(&xfer->node, &minfo->free_xfers);
> +		xfer = ERR_PTR(ret);
> +	}
> +	spin_unlock_irqrestore(&minfo->xfer_lock, flags);
> +
> +	if (!IS_ERR(xfer)) {
> +		refcount_set(&xfer->users, 1);

Maybe it would be better to do this inside the lock, so that there is no 
(unlikely) race with refcount_inc() in scmi_xfer_acquire().

> +		xfer->transfer_id = atomic_inc_return(&transfer_last_id);
> +	}
>  
>  	return xfer;
>  }
> @@ -252,6 +387,9 @@ static struct scmi_xfer *scmi_xfer_get(const struct scmi_handle *handle,
>   * @minfo: Pointer to Tx/Rx Message management info based on channel type
>   * @xfer: message that was reserved by scmi_xfer_get
>   *
> + * After refcount check, possibly release an xfer, clearing the token slot,
> + * removing xfer from @pending_xfers and putting it back into free_xfers.
> + *
>   * This holds a spinlock to maintain integrity of internal data structures.
>   */
>  static void
> @@ -259,16 +397,41 @@ __scmi_xfer_put(struct scmi_xfers_info *minfo, struct scmi_xfer *xfer)
>  {
>  	unsigned long flags;
>  
> -	/*
> -	 * Keep the locked section as small as possible
> -	 * NOTE: we might escape with smp_mb and no lock here..
> -	 * but just be conservative and symmetric.
> -	 */
>  	spin_lock_irqsave(&minfo->xfer_lock, flags);
> -	clear_bit(xfer->hdr.seq, minfo->xfer_alloc_table);
> +	if (refcount_dec_and_test(&xfer->users)) {
> +		scmi_xfer_token_clear(minfo, xfer);
> +		hash_del(&xfer->node);
> +		hlist_add_head(&xfer->node, &minfo->free_xfers);
> +	}
>  	spin_unlock_irqrestore(&minfo->xfer_lock, flags);
>  }
>  
> +/**
> + * scmi_xfer_lookup_unlocked  -  Helper to lookup an xfer_id
> + *
> + * @minfo: Pointer to Tx/Rx Message management info based on channel type
> + * @xfer_id: Token ID to lookup in @pending_xfers
> + *
> + * Refcounting is untouched.
> + *
> + * Context: Assumes to be called with @xfer_lock already acquired.
> + *
> + * Return: A valid xfer on Success or error otherwise
> + */
> +static struct scmi_xfer *
> +scmi_xfer_lookup_unlocked(struct scmi_xfers_info *minfo, u16 xfer_id)
> +{
> +	struct scmi_xfer *xfer = NULL;
> +
> +	if (xfer_id >= MSG_TOKEN_MAX)
> +		return ERR_PTR(-EINVAL);
> +
> +	if (test_bit(xfer_id, minfo->xfer_alloc_table))
> +		xfer = XFER_FIND(minfo->pending_xfers, xfer_id);
> +
> +	return xfer ?: ERR_PTR(-EINVAL);
> +}
> +
>  static void scmi_handle_notification(struct scmi_chan_info *cinfo, u32 msg_hdr)
>  {
>  	struct scmi_xfer *xfer;
> @@ -305,19 +468,22 @@ static void scmi_handle_notification(struct scmi_chan_info *cinfo, u32 msg_hdr)
>  static void scmi_handle_response(struct scmi_chan_info *cinfo,
>  				 u16 xfer_id, u8 msg_type)
>  {
> +	unsigned long flags;
>  	struct scmi_xfer *xfer;
>  	struct device *dev = cinfo->dev;
>  	struct scmi_info *info = handle_to_scmi_info(cinfo->handle);
>  	struct scmi_xfers_info *minfo = &info->tx_minfo;
>  
>  	/* Are we even expecting this? */
> -	if (!test_bit(xfer_id, minfo->xfer_alloc_table)) {
> +	spin_lock_irqsave(&minfo->xfer_lock, flags);
> +	xfer = scmi_xfer_lookup_unlocked(minfo, xfer_id);
> +	spin_unlock_irqrestore(&minfo->xfer_lock, flags);
> +	if (IS_ERR(xfer)) {
>  		dev_err(dev, "message for %d is not expected!\n", xfer_id);
>  		info->desc->ops->clear_channel(cinfo);
>  		return;
>  	}
>  
> -	xfer = &minfo->xfer_block[xfer_id];
>  	/*
>  	 * Even if a response was indeed expected on this slot at this point,
>  	 * a buggy platform could wrongly reply feeding us an unexpected
> @@ -1033,18 +1199,25 @@ static int __scmi_xfer_info_init(struct scmi_info *sinfo,
>  		return -EINVAL;
>  	}
>  
> -	info->xfer_block = devm_kcalloc(dev, desc->max_msg,
> -					sizeof(*info->xfer_block), GFP_KERNEL);
> -	if (!info->xfer_block)
> -		return -ENOMEM;
> +	hash_init(info->pending_xfers);
>  
> -	info->xfer_alloc_table = devm_kcalloc(dev, BITS_TO_LONGS(desc->max_msg),
> +	/* Allocate a bitmask sized to hold MSG_TOKEN_MAX tokens */
> +	info->xfer_alloc_table = devm_kcalloc(dev, BITS_TO_LONGS(MSG_TOKEN_MAX),
>  					      sizeof(long), GFP_KERNEL);
>  	if (!info->xfer_alloc_table)
>  		return -ENOMEM;
>  
> -	/* Pre-initialize the buffer pointer to pre-allocated buffers */
> -	for (i = 0, xfer = info->xfer_block; i < desc->max_msg; i++, xfer++) {
> +	/*
> +	 * Preallocate a number of xfers equal to max inflight messages,
> +	 * pre-initialize the buffer pointer to pre-allocated buffers and
> +	 * attach all of them to the free list
> +	 */
> +	INIT_HLIST_HEAD(&info->free_xfers);
> +	for (i = 0; i < desc->max_msg; i++) {
> +		xfer = devm_kzalloc(dev, sizeof(*xfer), GFP_KERNEL);
> +		if (!xfer)
> +			return -ENOMEM;
> +
>  		xfer->rx.buf = devm_kcalloc(dev, sizeof(u8), desc->max_msg_size,
>  					    GFP_KERNEL);
>  		if (!xfer->rx.buf)
> @@ -1052,8 +1225,12 @@ static int __scmi_xfer_info_init(struct scmi_info *sinfo,
>  
>  		xfer->tx.buf = xfer->rx.buf;
>  		init_completion(&xfer->done);
> +
> +		/* Add initialized xfer to the free list */
> +		hlist_add_head(&xfer->node, &info->free_xfers);
>  	}
>  
> +	atomic_set(&info->last_token, -1);
>  	spin_lock_init(&info->xfer_lock);
>  
>  	return 0;
> 




---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

[virtio-dev] Re: [PATCH v4 05/16] firmware: arm_scmi: Introduce delegated xfers support

[Peter Hilber]

On 11.06.21 18:59, Cristian Marussi wrote:
> Introduce optional support for delegated xfers allocation.
> 
> An SCMI transport can optionally declare to support delegated xfers and
> then use a few helper functions exposed by the core SCMI transport layer to
> query the core for existing in-flight transfers matching a provided message
> header or alternatively and transparently obtain a brand new xfer to handle
> a freshly received notification message.
> In both cases the obtained xfer is uniquely mapped into a specific xfer
> through the means of the message header acting as key.
> 
> In this way such a transport can properly store its own transport specific
> payload into the xfer uniquely associated to the message header before
> even calling into the core scmi_rx_callback() in the usual way, so that
> the transport specific message envelope structures can be freed early
> and there is no more need to keep track of their status till the core
> fully processes the xfer to completion or times out.
> 
> The scmi_rx_callbak() does not need to be modified to carry additional
> transport-specific ancillary data related to such message envelopes since
> an unique natural association is established between the xfer and the
> related message header.
> 
> Existing transports that do not need anything of the above will continue
> to work as before without any change.
> 
> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> ---
>  drivers/firmware/arm_scmi/common.h |  14 +++
>  drivers/firmware/arm_scmi/driver.c | 132 ++++++++++++++++++++++++++++-
>  2 files changed, 143 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h
> index e64c5ca9ee7c..0edc04bc434c 100644
> --- a/drivers/firmware/arm_scmi/common.h
> +++ b/drivers/firmware/arm_scmi/common.h
> @@ -80,6 +80,7 @@ struct scmi_msg_resp_prot_version {
>   * @status: Status of the transfer once it's complete
>   * @poll_completion: Indicate if the transfer needs to be polled for
>   *	completion or interrupt mode is used
> + * @saved_hdr: A copy of the original msg_hdr
>   */
>  struct scmi_msg_hdr {
>  	u8 id;
> @@ -88,6 +89,7 @@ struct scmi_msg_hdr {
>  	u16 seq;
>  	u32 status;
>  	bool poll_completion;
> +	u32 saved_hdr;
>  };
>  
>  /**
> @@ -154,6 +156,9 @@ struct scmi_msg {
>   * @rx: Receive message, the buffer should be pre-allocated to store
>   *	message. If request-ACK protocol is used, we can reuse the same
>   *	buffer for the rx path as we use for the tx path.
> + * @rx_raw_len: A field which can be optionally used by a specific transport
> + *		to save transport specific message length
> + *		It is not used by the SCMI transport core
>   * @done: command message transmit completion event
>   * @async_done: pointer to delayed response message received event completion
>   * @users: A refcount to track the active users for this xfer
> @@ -165,6 +170,7 @@ struct scmi_xfer {
>  	struct scmi_msg_hdr hdr;
>  	struct scmi_msg tx;
>  	struct scmi_msg rx;
> +	size_t rx_raw_len;
>  	struct completion done;
>  	struct completion *async_done;
>  	refcount_t users;
> @@ -355,6 +361,9 @@ struct scmi_device *scmi_child_dev_find(struct device *parent,
>   * @max_msg: Maximum number of messages that can be pending
>   *	simultaneously in the system
>   * @max_msg_size: Maximum size of data per message that can be handled.
> + * @support_xfers_delegation: A flag to indicate if the described transport
> + *			      will handle delegated xfers, so the core can
> + *			      derive proper related assumptions.
>   */
>  struct scmi_desc {
>  	int (*init)(void);
> @@ -363,6 +372,7 @@ struct scmi_desc {
>  	int max_rx_timeout_ms;
>  	int max_msg;
>  	int max_msg_size;
> +	bool support_xfers_delegation;
>  };
>  
>  extern const struct scmi_desc scmi_mailbox_desc;
> @@ -391,4 +401,8 @@ void scmi_notification_instance_data_set(const struct scmi_handle *handle,
>  					 void *priv);
>  void *scmi_notification_instance_data_get(const struct scmi_handle *handle);
>  
> +int scmi_transfer_acquire(struct scmi_chan_info *cinfo, u32 *msg_hdr,
> +			  struct scmi_xfer **xfer);
> +void scmi_transfer_release(struct scmi_chan_info *cinfo,
> +			   struct scmi_xfer *xfer);
>  #endif /* _SCMI_COMMON_H */
> diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> index f0b20ddb24f4..371d3804cd79 100644
> --- a/drivers/firmware/arm_scmi/driver.c
> +++ b/drivers/firmware/arm_scmi/driver.c
> @@ -432,6 +432,124 @@ scmi_xfer_lookup_unlocked(struct scmi_xfers_info *minfo, u16 xfer_id)
>  	return xfer ?: ERR_PTR(-EINVAL);
>  }
>  
> +/**
> + * scmi_xfer_acquire  -  Helper to lookup and acquire an xfer
> + *
> + * @minfo: Pointer to Tx/Rx Message management info based on channel type
> + * @xfer_id: Token ID to lookup in @pending_xfers
> + *
> + * When a valid xfer is found for the provided @xfer_id, reference counting is
> + * properly updated.
> + *
> + * Return: A valid @xfer on Success or error otherwise.
> + */
> +static struct scmi_xfer *
> +scmi_xfer_acquire(struct scmi_xfers_info *minfo, u16 xfer_id)
> +{
> +	unsigned long flags;
> +	struct scmi_xfer *xfer;
> +
> +	spin_lock_irqsave(&minfo->xfer_lock, flags);
> +	xfer = scmi_xfer_lookup_unlocked(minfo, xfer_id);
> +	if (!IS_ERR(xfer))
> +		refcount_inc(&xfer->users);
> +	spin_unlock_irqrestore(&minfo->xfer_lock, flags);
> +
> +	return xfer;
> +}
> +
> +/**
> + * scmi_transfer_acquire  -  Lookup for an existing xfer or freshly allocate a
> + * new one depending on the type of the message
> + *
> + * @cinfo: A reference to the channel descriptor.
> + * @msg_hdr: A pointer to the message header to lookup.
> + * @xfer: A reference to the pre-existent or freshly allocated xfer
> + *	  associated with the provided *msg_hdr.
> + *
> + * This function can be used by transports supporting delegated xfers to obtain
> + * a valid @xfer associated with the provided @msg_hdr param.
> + *
> + * The nature of the resulting @xfer depends on the type of message specified in
> + * @msg_hdr:
> + *  - for responses and delayed responses a pre-existent/pre-allocated in-flight
> + *    xfer descriptor will be returned (properly refcounted)
> + *  - for notifications a brand new xfer will be allocated; in this case the
> + *    provided message header sequence number will also be mangled to match
> + *    the token in the freshly allocated xfer: this is needed to establish a
> + *    link between the picked xfer and the msg_hdr that will be subsequently
> + *    passed back via usual scmi_rx_callback().
> + *
> + * Return: 0 if a valid xfer is returned in @xfer, error otherwise.
> + */
> +int scmi_transfer_acquire(struct scmi_chan_info *cinfo, u32 *msg_hdr,
> +			  struct scmi_xfer **xfer)
> +{
> +	u8 msg_type;
> +	struct scmi_info *info = handle_to_scmi_info(cinfo->handle);
> +
> +	if (!xfer || !msg_hdr || !info->desc->support_xfers_delegation)
> +		return -EINVAL;
> +
> +	msg_type = MSG_XTRACT_TYPE(*msg_hdr);
> +	switch (msg_type) {
> +	case MSG_TYPE_COMMAND:
> +	case MSG_TYPE_DELAYED_RESP:
> +		/* Grab an existing xfer for xfer_id */
> +		*xfer = scmi_xfer_acquire(&info->tx_minfo,
> +					  MSG_XTRACT_TOKEN(*msg_hdr));
> +		break;
> +	case MSG_TYPE_NOTIFICATION:
> +		/* Get a brand new RX xfer */
> +		*xfer = scmi_xfer_get(cinfo->handle, &info->rx_minfo);
> +		if (!IS_ERR(*xfer)) {
> +			/* Save original msg_hdr and fix sequence number */
> +			(*xfer)->hdr.saved_hdr = *msg_hdr;

The saved header isn't used anywhere.

> +			*msg_hdr &= ~MSG_TOKEN_ID_MASK;
> +			*msg_hdr |= FIELD_PREP(MSG_TOKEN_ID_MASK,
> +					       (*xfer)->hdr.seq);

This will invalidate the token set by the platform in 
scmi_dump_header_dbg(). Maybe it would have been more elegant to 
introduce a dedicated hash table key field?

> +		}
> +		break;
> +	default:
> +		*xfer = ERR_PTR(-EINVAL);
> +		break;
> +	}
> +
> +	if (IS_ERR(*xfer)) {
> +		dev_err(cinfo->dev,
> +			"Failed to acquire a valid xfer for hdr:0x%X\n",
> +			*msg_hdr);
> +		return PTR_ERR(*xfer);
> +	}
> +
> +	/* Fix xfer->hdr.type with actual msg_hdr carried type */
> +	unpack_scmi_header(*msg_hdr, &((*xfer)->hdr));
> +
> +	return 0;
> +}
> +
> +/**
> + * scmi_transfer_release  - Release an previously acquired xfer
> + *
> + * @cinfo: A reference to the channel descriptor.
> + * @xfer: A reference to the xfer to release.
> + */
> +void scmi_transfer_release(struct scmi_chan_info *cinfo, struct scmi_xfer *xfer)
> +{
> +	struct scmi_info *info = handle_to_scmi_info(cinfo->handle);
> +	struct scmi_xfers_info *minfo;
> +
> +	if (!xfer || !info->desc->support_xfers_delegation)
> +		return;
> +
> +	if (xfer->hdr.type == MSG_TYPE_NOTIFICATION)
> +		minfo = &info->rx_minfo;
> +	else
> +		minfo = &info->tx_minfo;
> +
> +	__scmi_xfer_put(minfo, xfer);
> +}
> +
>  static void scmi_handle_notification(struct scmi_chan_info *cinfo, u32 msg_hdr)
>  {
>  	struct scmi_xfer *xfer;
> @@ -441,7 +559,11 @@ static void scmi_handle_notification(struct scmi_chan_info *cinfo, u32 msg_hdr)
>  	ktime_t ts;
>  
>  	ts = ktime_get_boottime();
> -	xfer = scmi_xfer_get(cinfo->handle, minfo);
> +
> +	if (!info->desc->support_xfers_delegation)
> +		xfer = scmi_xfer_get(cinfo->handle, minfo);
> +	else
> +		xfer = scmi_xfer_acquire(minfo, MSG_XTRACT_TOKEN(msg_hdr));
>  	if (IS_ERR(xfer)) {
>  		dev_err(dev, "failed to get free message slot (%ld)\n",
>  			PTR_ERR(xfer));
> @@ -449,8 +571,11 @@ static void scmi_handle_notification(struct scmi_chan_info *cinfo, u32 msg_hdr)
>  		return;
>  	}
>  
> -	unpack_scmi_header(msg_hdr, &xfer->hdr);
>  	scmi_dump_header_dbg(dev, &xfer->hdr);
> +
> +	if (!info->desc->support_xfers_delegation)
> +		unpack_scmi_header(msg_hdr, &xfer->hdr);
> +

Why dump the header before unpacking?

>  	info->desc->ops->fetch_notification(cinfo, info->desc->max_msg_size,
>  					    xfer);
>  	scmi_notify(cinfo->handle, xfer->hdr.protocol_id,
> @@ -496,7 +621,8 @@ static void scmi_handle_response(struct scmi_chan_info *cinfo,
>  			xfer_id);
>  		info->desc->ops->clear_channel(cinfo);
>  		/* It was unexpected, so nobody will clear the xfer if not us */
> -		__scmi_xfer_put(minfo, xfer);
> +		if (!info->desc->support_xfers_delegation) //XXX ??? Really
> +			__scmi_xfer_put(minfo, xfer);
>  		return;
>  	}
>  
> 




---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

[virtio-dev] Re: [PATCH v4 06/16] firmware: arm_scmi, smccc, mailbox: Make shmem based transports optional

[Peter Hilber]

On 11.06.21 18:59, Cristian Marussi wrote:
> From: Igor Skalkin <igor.skalkin@opensynergy.com>
> 
> Upon adding the virtio transport in this patch series, SCMI will also
> work without shared memory based transports. Also, the mailbox transport
> may not be needed if the smc transport is used.
> 
> - Compile shmem.c only if a shmem based transport is available.
> 
> - Remove hard dependency of SCMI on mailbox.

The hard dependency has now already been removed with

   c05b07963e96 ("firmware: arm_scmi: Add SMCCC discovery dependency in")

> 
> [ Peter: Adapted patch for submission to upstream. ]
> 
> Co-developed-by: Peter Hilber <peter.hilber@opensynergy.com>
> Signed-off-by: Igor Skalkin <igor.skalkin@opensynergy.com>
> Signed-off-by: Peter Hilber <peter.hilber@opensynergy.com>
> ---
>  drivers/firmware/Kconfig           | 8 +++++++-
>  drivers/firmware/arm_scmi/Makefile | 2 +-
>  drivers/firmware/arm_scmi/common.h | 2 ++
>  drivers/firmware/smccc/Kconfig     | 1 +
>  drivers/mailbox/Kconfig            | 1 +
>  5 files changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
> index 1db738d5b301..358f895847b5 100644
> --- a/drivers/firmware/Kconfig
> +++ b/drivers/firmware/Kconfig
> @@ -9,7 +9,7 @@ menu "Firmware Drivers"
>  config ARM_SCMI_PROTOCOL
>  	tristate "ARM System Control and Management Interface (SCMI) Message Protocol"
>  	depends on ARM || ARM64 || COMPILE_TEST
> -	depends on MAILBOX || HAVE_ARM_SMCCC_DISCOVERY
> +	depends on ARM_SCMI_HAVE_SHMEM
>  	help
>  	  ARM System Control and Management Interface (SCMI) protocol is a
>  	  set of operating system-independent software interfaces that are
> @@ -27,6 +27,12 @@ config ARM_SCMI_PROTOCOL
>  	  This protocol library provides interface for all the client drivers
>  	  making use of the features offered by the SCMI.
>  
> +config ARM_SCMI_HAVE_SHMEM
> +	bool
> +	help
> +	  This declares whether a shared memory based transport for SCMI is
> +	  available.
> +
>  config ARM_SCMI_POWER_DOMAIN
>  	tristate "SCMI power domain driver"
>  	depends on ARM_SCMI_PROTOCOL || (COMPILE_TEST && OF)
> diff --git a/drivers/firmware/arm_scmi/Makefile b/drivers/firmware/arm_scmi/Makefile
> index 6a2ef63306d6..5a2d4c32e0ae 100644
> --- a/drivers/firmware/arm_scmi/Makefile
> +++ b/drivers/firmware/arm_scmi/Makefile
> @@ -1,7 +1,7 @@
>  # SPDX-License-Identifier: GPL-2.0-only
>  scmi-bus-y = bus.o
>  scmi-driver-y = driver.o notify.o
> -scmi-transport-y = shmem.o
> +scmi-transport-$(CONFIG_ARM_SCMI_HAVE_SHMEM) = shmem.o
>  scmi-transport-$(CONFIG_MAILBOX) += mailbox.o
>  scmi-transport-$(CONFIG_HAVE_ARM_SMCCC_DISCOVERY) += smc.o
>  scmi-protocols-y = base.o clock.o perf.o power.o reset.o sensors.o system.o voltage.o
> diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h
> index 0edc04bc434c..4666777019fa 100644
> --- a/drivers/firmware/arm_scmi/common.h
> +++ b/drivers/firmware/arm_scmi/common.h
> @@ -375,7 +375,9 @@ struct scmi_desc {
>  	bool support_xfers_delegation;
>  };
>  
> +#ifdef CONFIG_MAILBOX
>  extern const struct scmi_desc scmi_mailbox_desc;
> +#endif
>  #ifdef CONFIG_HAVE_ARM_SMCCC_DISCOVERY
>  extern const struct scmi_desc scmi_smc_desc;
>  #endif
> diff --git a/drivers/firmware/smccc/Kconfig b/drivers/firmware/smccc/Kconfig
> index 15e7466179a6..69c4d6cabf62 100644
> --- a/drivers/firmware/smccc/Kconfig
> +++ b/drivers/firmware/smccc/Kconfig
> @@ -9,6 +9,7 @@ config HAVE_ARM_SMCCC_DISCOVERY
>  	bool
>  	depends on ARM_PSCI_FW
>  	default y
> +	select ARM_SCMI_HAVE_SHMEM
>  	help
>  	 SMCCC v1.0 lacked discoverability and hence PSCI v1.0 was updated
>  	 to add SMCCC discovery mechanism though the PSCI firmware
> diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
> index 68de2c6af727..fc02c38c0739 100644
> --- a/drivers/mailbox/Kconfig
> +++ b/drivers/mailbox/Kconfig
> @@ -1,6 +1,7 @@
>  # SPDX-License-Identifier: GPL-2.0-only
>  menuconfig MAILBOX
>  	bool "Mailbox Hardware Support"
> +	select ARM_SCMI_HAVE_SHMEM
>  	help
>  	  Mailbox is a framework to control hardware communication between
>  	  on-chip processors through queued messages and interrupt driven
> 




---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

[virtio-dev] Re: [PATCH v4 14/16] firmware: arm_scmi: Add virtio transport

[Peter Hilber]

On 11.06.21 18:59, Cristian Marussi wrote:

<snip>

> +static struct virtio_driver virtio_scmi_driver = {
> +	.driver.name = "scmi-virtio",
> +	.driver.owner = THIS_MODULE,
> +	.feature_table = features,
> +	.feature_table_size = ARRAY_SIZE(features),
> +	.id_table = id_table,
> +	.probe = scmi_vio_probe,
> +	.remove = scmi_vio_remove,
> +};
> +

It might be good to also check for the VIRTIO_F_VERSION_1 feature bit in 
the optional .validate op (not yet implemented above), as some other 
devices do (quoting virtio-snd in the following):

> /**
>  * virtsnd_validate() - Validate if the device can be started.
>  * @vdev: VirtIO parent device.
>  *
>  * Context: Any context.
>  * Return: 0 on success, -EINVAL on failure.
>  */
> static int virtsnd_validate(struct virtio_device *vdev)
> {

<snip>

> 
> 	if (!virtio_has_feature(vdev, VIRTIO_F_VERSION_1)) {
> 		dev_err(&vdev->dev,
> 			"device does not comply with spec version 1.x\n");
> 		return -EINVAL;
> 	}
> 

<snip>

> 
> static struct virtio_driver virtsnd_driver = {
> 	.driver.name = KBUILD_MODNAME,
> 	.driver.owner = THIS_MODULE,
> 	.id_table = id_table,
> 	.validate = virtsnd_validate,

(end of virtio-snd quote)

---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

[virtio-dev] Re: [PATCH v4 15/16] [RFC][REWORK] firmware: arm_scmi: make virtio-scmi use delegated xfers

[Peter Hilber]

On 11.06.21 18:59, Cristian Marussi wrote:
> Draft changes to virtio-scmi to use new support for core delegated xfers
> in an attempt to simplify the interactions between virtio-scmi transport
> and the SCMI core transport layer.
> 

These changes seem to make xfers delegation mandatory for 
message-passing transports, so that might be documented.

> TODO:
>   - Polling is still not supported.
>   - Probe/remove sequence still to be reviewed.
>   - Concurrent or inverted reception of related responses and delayed
>     responses is still not addressed.
>     (it will be addressed in the SCMI core anyway most probably)
> 
> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> ---
>   drivers/firmware/arm_scmi/common.h |   5 +
>   drivers/firmware/arm_scmi/msg.c    |  35 +++++
>   drivers/firmware/arm_scmi/virtio.c | 212 +++++++++++++++--------------
>   3 files changed, 152 insertions(+), 100 deletions(-)
> 
> diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h
> index c143a449d278..22e5532fc698 100644
> --- a/drivers/firmware/arm_scmi/common.h
> +++ b/drivers/firmware/arm_scmi/common.h
> @@ -428,6 +428,11 @@ void msg_fetch_response(struct scmi_msg_payld *msg, size_t len,
>   void msg_fetch_notification(struct scmi_msg_payld *msg, size_t len,
>   			    size_t max_len, struct scmi_xfer *xfer);
>   
> +void msg_fetch_raw_payload(struct scmi_msg_payld *msg, size_t msg_len,
> +			   size_t max_len, struct scmi_xfer *xfer);
> +void msg_fetch_raw_response(struct scmi_xfer *xfer);
> +void msg_fetch_raw_notification(struct scmi_xfer *xfer);
> +
>   void scmi_notification_instance_data_set(const struct scmi_handle *handle,
>   					 void *priv);
>   void *scmi_notification_instance_data_get(const struct scmi_handle *handle);
> diff --git a/drivers/firmware/arm_scmi/msg.c b/drivers/firmware/arm_scmi/msg.c
> index 8a2d3303d281..3ed3ad0961ae 100644
> --- a/drivers/firmware/arm_scmi/msg.c
> +++ b/drivers/firmware/arm_scmi/msg.c
> @@ -74,6 +74,17 @@ u32 msg_read_header(struct scmi_msg_payld *msg)
>   	return le32_to_cpu(msg->msg_header);
>   }
>   
> +void msg_fetch_raw_payload(struct scmi_msg_payld *msg, size_t msg_len,
> +			   size_t max_len, struct scmi_xfer *xfer)
> +{
> +	xfer->rx_raw_len = min_t(size_t, max_len,
> +				 msg_len >= sizeof(*msg) ?
> +				 msg_len - sizeof(*msg) : 0);
> +
> +	/* Take a copy to the rx buffer.. */
> +	memcpy(xfer->rx.buf, msg->msg_payload, xfer->rx_raw_len);

In the usage throughout arm-scmi so far, scmi_desc.max_msg_size seems to 
consistently refer to the payload excluding the return status. 
scmi_desc.max_msg_size is the actual max_len parameter to this function.

So the logic here would reduce the maximum payload length for responses 
to (scmi_desc.max_msg_size - sizeof(return status)).

> +}
> +
>   /**
>    * msg_fetch_response() - Fetch response SCMI payload from transport SDU.
>    *
> @@ -94,6 +105,25 @@ void msg_fetch_response(struct scmi_msg_payld *msg, size_t len,
>   	memcpy(xfer->rx.buf, &msg->msg_payload[1], xfer->rx.len);
>   }
>   
> +void msg_fetch_raw_response(struct scmi_xfer *xfer)
> +{
> +	__le32 *msg_payload = xfer->rx.buf;
> +
> +	if (xfer->rx_raw_len < sizeof(xfer->hdr.status)) {
> +		xfer->rx.len = 0;
> +		return;
> +	}
> +
> +	xfer->hdr.status = le32_to_cpu(msg_payload[0]);
> +	/*
> +	 * rx.len has been already pre-calculated by fetch_raw
> +	 * for the whole payload including status, so shrink it
> +	 */
> +	xfer->rx.len = xfer->rx_raw_len - sizeof(xfer->hdr.status);
> +	/* Carveout status 4-byte field */
> +	memmove(xfer->rx.buf, &msg_payload[1], xfer->rx.len);

Wouldn't it be feasible to align properly in msg_fetch_raw_payload() 
already? That could also get rid of .rx_raw_len.

> +}
> +
>   /**
>    * msg_fetch_notification() - Fetch notification payload from transport SDU.
>    *
> @@ -111,3 +141,8 @@ void msg_fetch_notification(struct scmi_msg_payld *msg, size_t len,
>   	/* Take a copy to the rx buffer.. */
>   	memcpy(xfer->rx.buf, msg->msg_payload, xfer->rx.len);
>   }
> +
> +void msg_fetch_raw_notification(struct scmi_xfer *xfer)
> +{
> +	xfer->rx.len = xfer->rx_raw_len;
> +}
> diff --git a/drivers/firmware/arm_scmi/virtio.c b/drivers/firmware/arm_scmi/virtio.c
> index 20972adf6dc7..4412bc590ca7 100644
> --- a/drivers/firmware/arm_scmi/virtio.c
> +++ b/drivers/firmware/arm_scmi/virtio.c
> @@ -37,23 +37,24 @@
>   /**
>    * struct scmi_vio_channel - Transport channel information
>    *
> - * @lock: Protects access to all members except ready.
> - * @ready_lock: Protects access to ready. If required, it must be taken before
> - *              lock.
>    * @vqueue: Associated virtqueue
>    * @cinfo: SCMI Tx or Rx channel
>    * @free_list: List of unused scmi_vio_msg, maintained for Tx channels only
>    * @is_rx: Whether channel is an Rx channel
>    * @ready: Whether transport user is ready to hear about channel
> + * @lock: Protects access to all members except ready.
> + * @ready_lock: Protects access to ready. If required, it must be taken before
> + *              lock.
>    */
>   struct scmi_vio_channel {
> -	spinlock_t lock;
> -	spinlock_t ready_lock;
>   	struct virtqueue *vqueue;
>   	struct scmi_chan_info *cinfo;
>   	struct list_head free_list;
> -	u8 is_rx;
> -	u8 ready;
> +	bool is_rx;
> +	bool ready;
> +	unsigned int max_msg;
> +	spinlock_t lock;
> +	spinlock_t ready_lock;
>   };
>   
>   /**
> @@ -100,6 +101,73 @@ static int scmi_vio_feed_vq_rx(struct scmi_vio_channel *vioch,
>   	return rc;
>   }
>   
> +static void scmi_finalize_message(struct scmi_vio_channel *vioch,
> +				  struct scmi_vio_msg *msg)
> +{
> +	unsigned long flags;
> +
> +	if (vioch->is_rx) {
> +		scmi_vio_feed_vq_rx(vioch, msg);
> +	} else {
> +		spin_lock_irqsave(&vioch->lock, flags);
> +		list_add(&msg->list, &vioch->free_list);
> +		spin_unlock_irqrestore(&vioch->lock, flags);
> +	}
> +}
> +
> +static void scmi_process_vqueue_input(struct scmi_vio_channel *vioch,
> +				      struct scmi_vio_msg *msg)
> +{
> +	u32 msg_hdr;
> +	int ret;
> +	struct scmi_xfer *xfer = NULL;
> +
> +	msg_hdr = msg_read_header(msg->input);
> +	/*
> +	 * Acquire from the core transport layer a currently valid xfer
> +	 * descriptor associated to the received msg_hdr: this could be a
> +	 * previously allocated xfer for responses and delayed responses to
> +	 * in-flight commands, or a freshly allocated new xfer for a just
> +	 * received notification.
> +	 *
> +	 * In case of responses and delayed_responses the acquired xfer, at
> +	 * the time scmi_transfer_acquire() succcessfully returns is guaranteed
> +	 * to be still associated with a valid (not timed-out nor stale)
> +	 * descriptor and proper refcounting is kept in the core along this xfer
> +	 * so that should the core time out the xfer concurrently to this receive
> +	 * path the xfer will be properly deallocated only once the last user is
> +	 * done with it. (and this code path will terminate normally even though
> +	 * all the processing related to the timed out xfer will be discarded).
> +	 */

This comment would better fit to scmi_transfer_acquire().

> +	ret = scmi_transfer_acquire(vioch->cinfo, &msg_hdr, &xfer);
> +	if (ret) {
> +		dev_err(vioch->cinfo->dev,
> +			"Cannot find matching xfer for hdr:0x%X\n", msg_hdr);
> +		scmi_finalize_message(vioch, msg);
> +		return;
> +	}
> +
> +	dev_dbg(vioch->cinfo->dev,
> +		"VQUEUE[%d] - INPUT MSG_RX_LEN:%d - HDR:0x%X  TYPE:%d  XFER_ID:%d  XFER:%px\n",
> +		vioch->vqueue->index, msg->rx_len, msg_hdr, xfer->hdr.type,
> +		xfer->hdr.seq, xfer);
> +
> +	msg_fetch_raw_payload(msg->input, msg->rx_len,
> +			      scmi_virtio_desc.max_msg_size, xfer); > +
> +	/* Drop processed virtio message anyway */
> +	scmi_finalize_message(vioch, msg);
> +
> +	if (vioch->is_rx || !xfer->hdr.poll_completion)
> +		scmi_rx_callback(vioch->cinfo, msg_hdr);
> +	else
> +		dev_warn(vioch->cinfo->dev,
> +			 "Polling mode NOT supported. Dropped hdr:0X%X\n",
> +			 msg_hdr);
> +
> +	scmi_transfer_release(vioch->cinfo, xfer);
> +}
> +
>   static void scmi_vio_complete_cb(struct virtqueue *vqueue)
>   {
>   	unsigned long ready_flags;
> @@ -138,15 +206,9 @@ static void scmi_vio_complete_cb(struct virtqueue *vqueue)
>   
>   		if (msg) {
>   			msg->rx_len = length;
> -
> -			/*
> -			 * Hold the ready_lock during the callback to avoid
> -			 * races when the arm-scmi driver is unbinding while
> -			 * the virtio device is not quiesced yet.
> -			 */
> -			scmi_rx_callback(vioch->cinfo,
> -					 msg_read_header(msg->input), msg);
> +			scmi_process_vqueue_input(vioch, msg);
>   		}
> +
>   		spin_unlock_irqrestore(&vioch->ready_lock, ready_flags);
>   	}
>   
> @@ -163,27 +225,11 @@ static vq_callback_t *scmi_vio_complete_callbacks[] = {
>   	scmi_vio_complete_cb
>   };
>   
> -static unsigned int virtio_get_max_msg(bool tx,
> -				       struct scmi_chan_info *base_cinfo)
> +static unsigned int virtio_get_max_msg(struct scmi_chan_info *base_cinfo)
>   {
>   	struct scmi_vio_channel *vioch = base_cinfo->transport_info;
> -	unsigned int ret;
>   
> -	ret = virtqueue_get_vring_size(vioch->vqueue);
> -
> -	/* Tx messages need multiple descriptors. */
> -	if (tx)
> -		ret /= DESCRIPTORS_PER_TX_MSG;
> -
> -	if (ret > MSG_TOKEN_MAX) {
> -		dev_info_once(
> -			base_cinfo->dev,
> -			"Only %ld messages can be pending simultaneously, while the %s virtqueue could hold %d\n",
> -			MSG_TOKEN_MAX, tx ? "tx" : "rx", ret);
> -		ret = MSG_TOKEN_MAX;
> -	}
> -
> -	return ret;
> +	return vioch->max_msg;
>   }
>   
>   static int scmi_vio_match_any_dev(struct device *dev, const void *data)
> @@ -195,13 +241,14 @@ static struct virtio_driver virtio_scmi_driver; /* Forward declaration */
>   
>   static int virtio_link_supplier(struct device *dev)
>   {
> -	struct device *vdev = driver_find_device(
> -		&virtio_scmi_driver.driver, NULL, NULL, scmi_vio_match_any_dev);
> +	struct device *vdev;
> +
> +	vdev = driver_find_device(&virtio_scmi_driver.driver,
> +				  NULL, NULL, scmi_vio_match_any_dev);
>   
>   	if (!vdev) {
> -		dev_notice_once(
> -			dev,
> -			"Deferring probe after not finding a bound scmi-virtio device\n");
> +		dev_notice_once(dev,
> +				"Deferring probe after not finding a bound scmi-virtio device\n");
>   		return -EPROBE_DEFER;
>   	}
>   
> @@ -245,12 +292,8 @@ static int virtio_chan_setup(struct scmi_chan_info *cinfo, struct device *dev,
>   	struct virtio_device *vdev;
>   	struct scmi_vio_channel *vioch;
>   	int index = tx ? VIRTIO_SCMI_VQ_TX : VIRTIO_SCMI_VQ_RX;
> -	int max_msg;
>   	int i;
>   
> -	if (!virtio_chan_available(dev, index))
> -		return -ENODEV;
> -
>   	vdev = scmi_get_transport_info(dev);
>   	vioch = &((struct scmi_vio_channel *)vdev->priv)[index];
>   
> @@ -259,9 +302,7 @@ static int virtio_chan_setup(struct scmi_chan_info *cinfo, struct device *dev,
>   	vioch->cinfo = cinfo;
>   	spin_unlock_irqrestore(&vioch->lock, flags);
>   
> -	max_msg = virtio_get_max_msg(tx, cinfo);
> -
> -	for (i = 0; i < max_msg; i++) {
> +	for (i = 0; i < vioch->max_msg; i++) {
>   		struct scmi_vio_msg *msg;
>   
>   		msg = devm_kzalloc(cinfo->dev, sizeof(*msg), GFP_KERNEL);
> @@ -322,13 +363,6 @@ static int virtio_send_message(struct scmi_chan_info *cinfo,
>   	int rc;
>   	struct scmi_vio_msg *msg;
>   
> -	/*
> -	 * TODO: For now, we don't support polling. But it should not be
> -	 * difficult to add support.
> -	 */
> -	if (xfer->hdr.poll_completion)
> -		return -EINVAL;
> -
>   	spin_lock_irqsave(&vioch->lock, flags);
>   
>   	if (list_empty(&vioch->free_list)) {
> @@ -351,6 +385,11 @@ static int virtio_send_message(struct scmi_chan_info *cinfo,
>   			     "%s() failed to add to virtqueue (%d)\n", __func__,
>   			     rc);
>   	} else {
> +		dev_dbg(vioch->cinfo->dev,
> +			"VQUEUE[%d] - REQUEST - PROTO:0x%X  ID:0x%X  XFER_ID:%d  XFER:%px  RX_LEN:%zd\n",
> +		 vioch->vqueue->index, xfer->hdr.protocol_id,
> +		 xfer->hdr.id, xfer->hdr.seq, xfer, xfer->rx.len);

Indentation appears to be inconsistent.

> +
>   		virtqueue_kick(vioch->vqueue);
>   	}
>   
> @@ -360,36 +399,15 @@ static int virtio_send_message(struct scmi_chan_info *cinfo,
>   }
>   
>   static void virtio_fetch_response(struct scmi_chan_info *cinfo,
> -				  struct scmi_xfer *xfer, void *msg_handle)
> +				  struct scmi_xfer *xfer)
>   {
> -	struct scmi_vio_msg *msg = msg_handle;
> -	struct scmi_vio_channel *vioch = cinfo->transport_info;
> -
> -	if (!msg) {
> -		dev_dbg_once(&vioch->vqueue->vdev->dev,
> -			     "Ignoring %s() call with NULL msg_handle\n",
> -			     __func__);
> -		return;
> -	}
> -
> -	msg_fetch_response(msg->input, msg->rx_len, xfer);
> +	msg_fetch_raw_response(xfer);
>   }
>   
>   static void virtio_fetch_notification(struct scmi_chan_info *cinfo,
> -				      size_t max_len, struct scmi_xfer *xfer,
> -				      void *msg_handle)
> +				      size_t max_len, struct scmi_xfer *xfer)
>   {
> -	struct scmi_vio_msg *msg = msg_handle;
> -	struct scmi_vio_channel *vioch = cinfo->transport_info;
> -
> -	if (!msg) {
> -		dev_dbg_once(&vioch->vqueue->vdev->dev,
> -			     "Ignoring %s() call with NULL msg_handle\n",
> -			     __func__);
> -		return;
> -	}
> -
> -	msg_fetch_notification(msg->input, msg->rx_len, max_len, xfer);
> +	msg_fetch_raw_notification(xfer);
>   }
>   
>   static void dummy_clear_channel(struct scmi_chan_info *cinfo)
> @@ -402,28 +420,6 @@ static bool dummy_poll_done(struct scmi_chan_info *cinfo,
>   	return false;
>   }
>   
> -static void virtio_drop_message(struct scmi_chan_info *cinfo, void *msg_handle)
> -{
> -	unsigned long flags;
> -	struct scmi_vio_channel *vioch = cinfo->transport_info;
> -	struct scmi_vio_msg *msg = msg_handle;
> -
> -	if (!msg) {
> -		dev_dbg_once(&vioch->vqueue->vdev->dev,
> -			     "Ignoring %s() call with NULL msg_handle\n",
> -			     __func__);
> -		return;
> -	}
> -
> -	if (vioch->is_rx) {
> -		scmi_vio_feed_vq_rx(vioch, msg);
> -	} else {
> -		spin_lock_irqsave(&vioch->lock, flags);
> -		list_add(&msg->list, &vioch->free_list);
> -		spin_unlock_irqrestore(&vioch->lock, flags);
> -	}
> -}
> -
>   static const struct scmi_transport_ops scmi_virtio_ops = {
>   	.link_supplier = virtio_link_supplier,
>   	.chan_available = virtio_chan_available,
> @@ -435,7 +431,6 @@ static const struct scmi_transport_ops scmi_virtio_ops = {
>   	.fetch_notification = virtio_fetch_notification,
>   	.clear_channel = dummy_clear_channel,
>   	.poll_done = dummy_poll_done,
> -	.drop_message = virtio_drop_message,
>   };
>   
>   static int scmi_vio_probe(struct virtio_device *vdev)
> @@ -467,10 +462,26 @@ static int scmi_vio_probe(struct virtio_device *vdev)
>   	dev_info(dev, "Found %d virtqueue(s)\n", vq_cnt);
>   
>   	for (i = 0; i < vq_cnt; i++) {
> +		unsigned int sz;
> +
>   		spin_lock_init(&channels[i].lock);
>   		spin_lock_init(&channels[i].ready_lock);
>   		INIT_LIST_HEAD(&channels[i].free_list);
>   		channels[i].vqueue = vqs[i];
> +
> +		sz = virtqueue_get_vring_size(channels[i].vqueue);
> +		/* Tx messages need multiple descriptors. */
> +		if (!channels[i].is_rx)
> +			sz /= DESCRIPTORS_PER_TX_MSG;
> +
> +		if (sz > MSG_TOKEN_MAX) {
> +			dev_info_once(dev,
> +				      "%s virtqueue could hold %d messages. Only %ld allowed to be pending.\n",
> +				      channels[i].is_rx ? "rx" : "tx",
> +				      sz, MSG_TOKEN_MAX);
> +			sz = MSG_TOKEN_MAX;
> +		}
> +		channels[i].max_msg = sz;
>   	}
>   
>   	vdev->priv = channels;
> @@ -520,4 +531,5 @@ const struct scmi_desc scmi_virtio_desc = {
>   	.max_rx_timeout_ms = 60000, /* for non-realtime virtio devices */
>   	.max_msg = 0, /* overridden by virtio_get_max_msg() */
>   	.max_msg_size = VIRTIO_SCMI_MAX_MSG_SIZE,
> +	.support_xfers_delegation = true,
>   };
> 




---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org