[virtio-dev] Re: [PATCH v6 00/17] Introduce SCMI transport based on VirtIO

[virtio-dev] Re: [PATCH v6 00/17] Introduce SCMI transport based on VirtIO

[Peter Hilber]

On 12.07.21 16:18, Cristian Marussi wrote:
> Hi all,
> 

Hi Cristian,

thanks for your update. Please find some additional comments in this 
reply and the following.

Best regards,

Peter

> While reworking this series starting from the work done up to V3 by
> OpenSynergy, I am keeping the original autorship and list distribution
> unchanged.
> 
> The main aim of this rework, as said, is to simplify where possible the
> SCMI VirtIO support added in V3 by adding at first some new general
> mechanisms in the SCMI Transport layer.
> 
> Indeed, after some initial small fixes, patches 05/06/07/08 add such new
> additional mechanisms to the SCMI core to ease implementation of more
> complex transports like virtio, while also addressing a few general issues
> already potentially affecting existing transports.
> 
> In terms of rework I dropped original V3 patches 05/06/07/08/12 as no more
> needed, and modified where needed the remaining original patches to take
> advantage of the above mentioned new SCMI transport features.
> 
> DT bindings patch has been ported on top of freshly YAML converted arm,scmi
> bindings.
> 
> Moreover, since V5 I dropped support for polling mode from the virtio-scmi
> transport, since it is an optional general mechanism provided by the core
> to allow transports lacking a completion IRQ to work and it seemed a
> needless addition/complication in the context of virtio transport.
> 

Just for correctness, in my understanding polling is not completely 
optional ATM. Polling would be required by scmi_cpufreq_fast_switch(). 
But that requirement might be irrelevant for now.

> Additionally, in V5 I could also simplify a bit the virtio transport
> probing sequence starting from the observation that, by the VirtIO spec,
> in fact, only one single SCMI VirtIO device can possibly exist on a system.
> 

I wouldn't say that the virtio spec restricts the # of virtio-scmi 
devices to one. But I do think the one device limitation in the kernel 
is acceptable.

> The series has been tested using an emulated fake SCMI device and also a
> proper SCP-fw stack running through QEMU vhost-users, with the SCMI stack
> compiled, in both cases, as builtin and as a loadable module, running tests
> against mocked SCMI Sensors using HWMON and IIO interfaces to check the
> functionality of notifications and sync/async commands.
> 
> Virtio-scmi support has been exercised in the following testing scenario
> on a JUNO board:
> 
>   - normal sync/async command transfers
>   - notifications
>   - concurrent delivery of correlated response and delayed responses
>   - out-of-order delivery of delayed responses before related responses
>   - unexpected delayed response delivery for sync commands
>   - late delivery of timed-out responses and delayed responses
> 
> Some basic regression testing against mailbox transport has been performed
> for commands and notifications too.
> 
> No sensible overhead in total handling time of commands and notifications
> has been observed, even though this series do indeed add a considerable
> amount of code to execute on TX path.
> More test and measurements could be needed in these regards.
> 
> This series is based on top of v5.14-rc1.
> 
> Any feedback/testing is welcome :D
> 
> Thanks,
> Cristian
> ---
> V5 --> V6:
>   - removed delegated xfers and its usage
>   - add and use *priv optional parameter in scmi_rx_callback()
>   - made .poll_done and .clear_channel ops optional
> 
> V4 --> V5:
>   - removed msg raw_payload helpers
>   - reworked msg helpers to use xfer->priv reference
>   - simplified SCMI device probe sequence (one static device)
>   - added new SCMI Kconfig layout
>   - removed SCMI virtio polling support
> 
> V3 --> V4:
>   - using new delegated xfers support and monotonically increasing tokens
>     in virtio transport
>   - ported SCMI virtio transport DT bindings to YAML format
>   - added virtio-scmi polling support
>   - added delegated xfers support
> 
> Cristian Marussi (11):
>    firmware: arm_scmi: Avoid padding in sensor message structure
>    firmware: arm_scmi: Fix max pending messages boundary check
>    firmware: arm_scmi: Add support for type handling in common functions
>    firmware: arm_scmi: Remove scmi_dump_header_dbg() helper
>    firmware: arm_scmi: Add transport optional init/exit support
>    firmware: arm_scmi: Introduce monotonically increasing tokens
>    firmware: arm_scmi: Handle concurrent and out-of-order messages
>    firmware: arm_scmi: Add priv parameter to scmi_rx_callback
>    firmware: arm_scmi: Make .clear_channel optional
>    firmware: arm_scmi: Make polling mode optional
>    firmware: arm_scmi: Make SCMI transports configurable
> 
> Igor Skalkin (4):
>    firmware: arm_scmi: Make shmem support optional for transports
>    firmware: arm_scmi: Add method to override max message number
>    dt-bindings: arm: Add virtio transport for SCMI
>    firmware: arm_scmi: Add virtio transport
> 
> Peter Hilber (2):
>    firmware: arm_scmi: Add message passing abstractions for transports
>    firmware: arm_scmi: Add optional link_supplier() transport op
> 
>   .../bindings/firmware/arm,scmi.yaml           |   8 +-
>   MAINTAINERS                                   |   1 +
>   drivers/firmware/Kconfig                      |  34 +-
>   drivers/firmware/arm_scmi/Kconfig             |  97 +++
>   drivers/firmware/arm_scmi/Makefile            |   8 +-
>   drivers/firmware/arm_scmi/common.h            |  94 ++-
>   drivers/firmware/arm_scmi/driver.c            | 651 +++++++++++++++---
>   drivers/firmware/arm_scmi/mailbox.c           |   2 +-
>   drivers/firmware/arm_scmi/msg.c               | 113 +++
>   drivers/firmware/arm_scmi/sensors.c           |   6 +-
>   drivers/firmware/arm_scmi/smc.c               |   3 +-
>   drivers/firmware/arm_scmi/virtio.c            | 491 +++++++++++++
>   include/uapi/linux/virtio_ids.h               |   1 +
>   include/uapi/linux/virtio_scmi.h              |  24 +
>   14 files changed, 1389 insertions(+), 144 deletions(-)
>   create mode 100644 drivers/firmware/arm_scmi/Kconfig
>   create mode 100644 drivers/firmware/arm_scmi/msg.c
>   create mode 100644 drivers/firmware/arm_scmi/virtio.c
>   create mode 100644 include/uapi/linux/virtio_scmi.h
> 


---------------------------------------------------------------------
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 v6 00/17] Introduce SCMI transport based on VirtIO

[Peter Hilber]

On 19.07.21 13:36, Cristian Marussi wrote:
> On Thu, Jul 15, 2021 at 06:35:38PM +0200, Peter Hilber wrote:
>> On 12.07.21 16:18, Cristian Marussi wrote:
>>> Hi all,
>>>
>>
>> Hi Cristian,
>>
>> thanks for your update. Please find some additional comments in this reply
>> and the following.
>>
>> Best regards,
>>
>> Peter
> 
> Hi Peter,
> 
> thanks for the feedback.
> 
>>
>>> While reworking this series starting from the work done up to V3 by
>>> OpenSynergy, I am keeping the original autorship and list distribution
>>> unchanged.
>>>
>>> The main aim of this rework, as said, is to simplify where possible the
>>> SCMI VirtIO support added in V3 by adding at first some new general
>>> mechanisms in the SCMI Transport layer.
>>>
>>> Indeed, after some initial small fixes, patches 05/06/07/08 add such new
>>> additional mechanisms to the SCMI core to ease implementation of more
>>> complex transports like virtio, while also addressing a few general issues
>>> already potentially affecting existing transports.
>>>
>>> In terms of rework I dropped original V3 patches 05/06/07/08/12 as no more
>>> needed, and modified where needed the remaining original patches to take
>>> advantage of the above mentioned new SCMI transport features.
>>>
>>> DT bindings patch has been ported on top of freshly YAML converted arm,scmi
>>> bindings.
>>>
>>> Moreover, since V5 I dropped support for polling mode from the virtio-scmi
>>> transport, since it is an optional general mechanism provided by the core
>>> to allow transports lacking a completion IRQ to work and it seemed a
>>> needless addition/complication in the context of virtio transport.
>>>
>>
>> Just for correctness, in my understanding polling is not completely optional
>> ATM. Polling would be required by scmi_cpufreq_fast_switch(). But that
>> requirement might be irrelevant for now.
>>
> 
> Cpufreq core can use .fast_switch (scmi_cpufreq_fast_switch) op only if
> policy->fast_switch_enabled is true which in turn reported as true by
> the SCMI cpufreq driver iff SCMI FastChannels are supported by Perf
> implementation server side, but the SCMI Device VirtIO spec (5.17)
> explicitly does NOT support SCMI FastChannels as of now.
> 
> Anyway, even though we should support in the future SCMI FastChannels on
> VirtIO SCMI transport, fastchannels are by defintion per-protocol/per-command/
> per-domain-id specific, based on sharedMem or MMIO, unidirectional and do not
> even allow for a response from the platform (SCMIV3.0 4.1.1 5.3) so polling
> won't be a thing anyway unless I'm missing something.
> 
> BUT you made a good point in fact anyway, because the generic perf->freq_set/get
> API CAN be indeed invoked in polling mode, and, even though we do not use them
> in polling as of now (if not in the FastChannel scenario above) this could be a
> potential problem in general if when the underlying transport do not support poll
> the core just drop any poll_completion=true messages.
> 
> So, while I still think it is not sensible to enable poll mode in SCMI Virtio,
> because would be a sort of faked polling and increases complexity, I'm now
> considering the fact that maybe the right behaviour of the SCMI core in such a
> scenario would be to warn the user as it does now AND then fallback to use
> non-polling, probably better if such a behavior is made condtional on some
> transport config desc flag that allow such fallback behavior.
> 
> Any thought ?
> 

Maybe the SCMI protocols should request "atomic" instead of "polling"? 
That semantics are the actual intent in my understanding. So the 
"Introduce atomic support for SCMI transports" patch series [1] could 
potentially address this?

Best regards,

Peter


[1] https://lkml.org/lkml/2021/7/12/3089

---------------------------------------------------------------------
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 v6 07/17] firmware: arm_scmi: Handle concurrent and out-of-order messages

[Peter Hilber]

On 12.07.21 16:18, Cristian Marussi wrote:
> Even though in case of asynchronous commands an SCMI platform server is
> constrained to emit the delayed response message only after the related
> message response has been sent, the configured underlying transport could
> still deliver such messages together or in inverted order, causing races
> due to the concurrent or out-of-order access to the underlying xfer.
> 
> Introduce a mechanism to grant exclusive access to an xfer in order to
> properly serialize concurrent accesses to the same xfer originating from
> multiple correlated messages.
> 
> Add additional state information to xfer descriptors so as to be able to
> identify out-of-order message deliveries and act accordingly:
> 
>   - when a delayed response is expected but delivered before the related
>     response, the synchronous response is considered as successfully
>     received and the delayed response processing is carried on as usual.
> 
>   - when/if the missing synchronous response is subsequently received, it
>     is discarded as not congruent with the current state of the xfer, or
>     simply, because the xfer has been already released and so, now, the
>     monotonically increasing sequence number carried by the late response
>     is stale.
> 
> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> ---
> v5 --> v6
> - added spinlock comment
> ---
>   drivers/firmware/arm_scmi/common.h |  18 ++-
>   drivers/firmware/arm_scmi/driver.c | 229 ++++++++++++++++++++++++-----
>   2 files changed, 212 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h
> index 2233d0a188fc..9efebe1406d2 100644
> --- a/drivers/firmware/arm_scmi/common.h
> +++ b/drivers/firmware/arm_scmi/common.h
> @@ -19,6 +19,7 @@
>   #include <linux/module.h>
>   #include <linux/refcount.h>
>   #include <linux/scmi_protocol.h>
> +#include <linux/spinlock.h>
>   #include <linux/types.h>
>   
>   #include <asm/unaligned.h>
> @@ -145,6 +146,13 @@ struct scmi_msg {
>    * @pending: True for xfers added to @pending_xfers hashtable
>    * @node: An hlist_node reference used to store this xfer, alternatively, on
>    *	  the free list @free_xfers or in the @pending_xfers hashtable
> + * @busy: An atomic flag to ensure exclusive write access to this xfer
> + * @state: The current state of this transfer, with states transitions deemed
> + *	   valid being:
> + *	    - SCMI_XFER_SENT_OK -> SCMI_XFER_RESP_OK [ -> SCMI_XFER_DRESP_OK ]
> + *	    - SCMI_XFER_SENT_OK -> SCMI_XFER_DRESP_OK
> + *	      (Missing synchronous response is assumed OK and ignored)
> + * @lock: A spinlock to protect state and busy fields.
>    */
>   struct scmi_xfer {
>   	int transfer_id;
> @@ -156,6 +164,15 @@ struct scmi_xfer {
>   	refcount_t users;
>   	bool pending;
>   	struct hlist_node node;
> +#define SCMI_XFER_FREE		0
> +#define SCMI_XFER_BUSY		1
> +	atomic_t busy;
> +#define SCMI_XFER_SENT_OK	0
> +#define SCMI_XFER_RESP_OK	1
> +#define SCMI_XFER_DRESP_OK	2
> +	int state;
> +	/* A lock to protect state and busy fields */
> +	spinlock_t lock;
>   };
>   
>   /*
> @@ -392,5 +409,4 @@ bool shmem_poll_done(struct scmi_shared_mem __iomem *shmem,
>   void scmi_notification_instance_data_set(const struct scmi_handle *handle,
>   					 void *priv);
>   void *scmi_notification_instance_data_get(const struct scmi_handle *handle);
> -
>   #endif /* _SCMI_COMMON_H */
> diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> index 245ede223302..5ef33d692670 100644
> --- a/drivers/firmware/arm_scmi/driver.c
> +++ b/drivers/firmware/arm_scmi/driver.c
> @@ -369,6 +369,7 @@ static struct scmi_xfer *scmi_xfer_get(const struct scmi_handle *handle,
>   
>   	if (!IS_ERR(xfer)) {
>   		refcount_set(&xfer->users, 1);
> +		atomic_set(&xfer->busy, SCMI_XFER_FREE);
>   		xfer->transfer_id = atomic_inc_return(&transfer_last_id);
>   	}
>   	spin_unlock_irqrestore(&minfo->xfer_lock, flags);
> @@ -430,6 +431,168 @@ scmi_xfer_lookup_unlocked(struct scmi_xfers_info *minfo, u16 xfer_id)
>   	return xfer ?: ERR_PTR(-EINVAL);
>   }
>   
> +/**
> + * scmi_msg_response_validate  - Validate message type against state of related
> + * xfer
> + *
> + * @cinfo: A reference to the channel descriptor.
> + * @msg_type: Message type to check
> + * @xfer: A reference to the xfer to validate against @msg_type
> + *
> + * This function checks if @msg_type is congruent with the current state of
> + * a pending @xfer; if an asynchronous delayed response is received before the
> + * related synchronous response (Out-of-Order Delayed Response) the missing
> + * synchronous response is assumed to be OK and completed, carrying on with the
> + * Delayed Response: this is done to address the case in which the underlying
> + * SCMI transport can deliver such out-of-order responses.
> + *
> + * Context: Assumes to be called with xfer->lock already acquired.
> + *
> + * Return: 0 on Success, error otherwise
> + */
> +static inline int scmi_msg_response_validate(struct scmi_chan_info *cinfo,
> +					     u8 msg_type,
> +					     struct scmi_xfer *xfer)
> +{
> +	/*
> +	 * Even if a response was indeed expected on this slot at this point,
> +	 * a buggy platform could wrongly reply feeding us an unexpected
> +	 * delayed response we're not prepared to handle: bail-out safely
> +	 * blaming firmware.
> +	 */
> +	if (msg_type == MSG_TYPE_DELAYED_RESP && !xfer->async_done) {
> +		dev_err(cinfo->dev,
> +			"Delayed Response for %d not expected! Buggy F/W ?\n",
> +			xfer->hdr.seq);
> +		return -EINVAL;
> +	}
> +
> +	switch (xfer->state) {
> +	case SCMI_XFER_SENT_OK:
> +		if (msg_type == MSG_TYPE_DELAYED_RESP) {
> +			/*
> +			 * Delayed Response expected but delivered earlier.
> +			 * Assume message RESPONSE was OK and skip state.
> +			 */
> +			xfer->hdr.status = SCMI_SUCCESS;
> +			xfer->state = SCMI_XFER_RESP_OK;
> +			complete(&xfer->done);
> +			dev_warn(cinfo->dev,
> +				 "Received valid OoO Delayed Response for %d\n",
> +				 xfer->hdr.seq);
> +		}
> +		break;
> +	case SCMI_XFER_RESP_OK:
> +		if (msg_type != MSG_TYPE_DELAYED_RESP)
> +			return -EINVAL;
> +		break;
> +	case SCMI_XFER_DRESP_OK:
> +		/* No further message expected once in SCMI_XFER_DRESP_OK */
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static bool scmi_xfer_is_free(struct scmi_xfer *xfer)
> +{
> +	int ret;
> +
> +	ret = atomic_cmpxchg(&xfer->busy, SCMI_XFER_FREE, SCMI_XFER_BUSY);

Naming: Rather unusual to change state in an _is_free() function, 
looking at other _is_free() functions in the kernel.

> +
> +	return ret == SCMI_XFER_FREE;
> +}
> +
> +/**
> + * scmi_xfer_command_acquire  -  Helper to lookup and acquire a command xfer
> + *
> + * @cinfo: A reference to the channel descriptor.
> + * @msg_hdr: A message header to use as lookup key
> + *
> + * When a valid xfer is found for the sequence number embedded in the provided
> + * msg_hdr, reference counting is properly updated and exclusive access to this
> + * xfer is granted till released with @scmi_xfer_command_release.
> + *
> + * Return: A valid @xfer on Success or error otherwise.
> + */
> +static inline struct scmi_xfer *
> +scmi_xfer_command_acquire(struct scmi_chan_info *cinfo, u32 msg_hdr)
> +{
> +	int ret;
> +	unsigned long flags;
> +	struct scmi_xfer *xfer;
> +	struct scmi_info *info = handle_to_scmi_info(cinfo->handle);
> +	struct scmi_xfers_info *minfo = &info->tx_minfo;
> +	u8 msg_type = MSG_XTRACT_TYPE(msg_hdr);
> +	u16 xfer_id = MSG_XTRACT_TOKEN(msg_hdr);
> +
> +	/* Are we even expecting this? */
> +	spin_lock_irqsave(&minfo->xfer_lock, flags);
> +	xfer = scmi_xfer_lookup_unlocked(minfo, xfer_id);
> +	if (IS_ERR(xfer)) {
> +		dev_err(cinfo->dev,
> +			"Message for %d type %d is not expected!\n",
> +			xfer_id, msg_type);
> +		spin_unlock_irqrestore(&minfo->xfer_lock, flags);
> +		return xfer;
> +	}
> +	refcount_inc(&xfer->users);
> +	spin_unlock_irqrestore(&minfo->xfer_lock, flags);
> +
> +	spin_lock_irqsave(&xfer->lock, flags);
> +	ret = scmi_msg_response_validate(cinfo, msg_type, xfer);
> +	/*
> +	 * If a pending xfer was found which was also in a congruent state with
> +	 * the received message, acquire exclusive access to it setting the busy
> +	 * flag.
> +	 * Spins only on the rare limit condition of concurrent reception of
> +	 * RESP and DRESP for the same xfer.
> +	 */
> +	if (!ret) {
> +		spin_until_cond(scmi_xfer_is_free(xfer));

Maybe there should be an additional comment to indicate that the 
xfer->lock cannot be reaquired later during response processing, to 
avoid a deadlock in conjunction with the xfer->busy flag.

> +		xfer->hdr.type = msg_type;
> +	}
> +	spin_unlock_irqrestore(&xfer->lock, flags);
> +
> +	if (ret) {
> +		dev_err(cinfo->dev,
> +			"Invalid message type:%d for %d - HDR:0x%X  state:%d\n",
> +			msg_type, xfer_id, msg_hdr, xfer->state);
> +		/* On error the refcount incremented above has to be dropped */
> +		__scmi_xfer_put(minfo, xfer);
> +		xfer = ERR_PTR(-EINVAL);
> +	}
> +
> +	return xfer;
> +}
> +
> +static inline void scmi_xfer_command_release(struct scmi_info *info,
> +					     struct scmi_xfer *xfer)
> +{
> +	atomic_set(&xfer->busy, SCMI_XFER_FREE);
> +	__scmi_xfer_put(&info->tx_minfo, xfer);
> +}
> +
> +/**
> + * scmi_xfer_state_update  - Update xfer state
> + *
> + * @xfer: A reference to the xfer to update
> + *
> + * Context: Assumes to be called on an xfer exclusively acquired using the
> + *	    busy flag.
> + */
> +static inline void scmi_xfer_state_update(struct scmi_xfer *xfer)
> +{
> +	switch (xfer->hdr.type) {
> +	case MSG_TYPE_COMMAND:
> +		xfer->state = SCMI_XFER_RESP_OK;
> +		break;
> +	case MSG_TYPE_DELAYED_RESP:
> +		xfer->state = SCMI_XFER_DRESP_OK;
> +		break;
> +	}
> +}
> +
>   static void scmi_handle_notification(struct scmi_chan_info *cinfo, u32 msg_hdr)
>   {
>   	struct scmi_xfer *xfer;
> @@ -462,57 +625,37 @@ static void scmi_handle_notification(struct scmi_chan_info *cinfo, u32 msg_hdr)
>   	info->desc->ops->clear_channel(cinfo);
>   }
>   
> -static void scmi_handle_response(struct scmi_chan_info *cinfo,
> -				 u16 xfer_id, u8 msg_type)
> +static void scmi_handle_response(struct scmi_chan_info *cinfo, u32 msg_hdr)
>   {
> -	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? */
> -	spin_lock_irqsave(&minfo->xfer_lock, flags);
> -	xfer = scmi_xfer_lookup_unlocked(minfo, xfer_id);
> -	spin_unlock_irqrestore(&minfo->xfer_lock, flags);
> +	xfer = scmi_xfer_command_acquire(cinfo, msg_hdr);
>   	if (IS_ERR(xfer)) {
> -		dev_err(dev, "message for %d is not expected!\n", xfer_id);
>   		info->desc->ops->clear_channel(cinfo);
>   		return;
>   	}
>   
> -	/*
> -	 * Even if a response was indeed expected on this slot at this point,
> -	 * a buggy platform could wrongly reply feeding us an unexpected
> -	 * delayed response we're not prepared to handle: bail-out safely
> -	 * blaming firmware.
> -	 */
> -	if (unlikely(msg_type == MSG_TYPE_DELAYED_RESP && !xfer->async_done)) {
> -		dev_err(dev,
> -			"Delayed Response for %d not expected! Buggy F/W ?\n",
> -			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);
> -		return;
> -	}
> +	scmi_xfer_state_update(xfer);

Since this update is not protected by the xfer->lock any more, it may 
not become visible in time to a concurrent response which is checking 
and possibly updating state in scmi_msg_response_validate(). I think 
this should be avoided, even if it might not cause practical problems ATM.

>   
>   	/* rx.len could be shrunk in the sync do_xfer, so reset to maxsz */
> -	if (msg_type == MSG_TYPE_DELAYED_RESP)
> +	if (xfer->hdr.type == MSG_TYPE_DELAYED_RESP)
>   		xfer->rx.len = info->desc->max_msg_size;
>   
>   	info->desc->ops->fetch_response(cinfo, xfer);
>   
>   	trace_scmi_rx_done(xfer->transfer_id, xfer->hdr.id,
>   			   xfer->hdr.protocol_id, xfer->hdr.seq,
> -			   msg_type);
> +			   xfer->hdr.type);
>   
> -	if (msg_type == MSG_TYPE_DELAYED_RESP) {
> +	if (xfer->hdr.type == MSG_TYPE_DELAYED_RESP) {
>   		info->desc->ops->clear_channel(cinfo);
>   		complete(xfer->async_done);
>   	} else {
>   		complete(&xfer->done);
>   	}
> +
> +	scmi_xfer_command_release(info, xfer);
>   }
>   
>   /**
> @@ -529,7 +672,6 @@ static void scmi_handle_response(struct scmi_chan_info *cinfo,
>    */
>   void scmi_rx_callback(struct scmi_chan_info *cinfo, u32 msg_hdr)
>   {
> -	u16 xfer_id = MSG_XTRACT_TOKEN(msg_hdr);
>   	u8 msg_type = MSG_XTRACT_TYPE(msg_hdr);
>   
>   	switch (msg_type) {
> @@ -538,7 +680,7 @@ void scmi_rx_callback(struct scmi_chan_info *cinfo, u32 msg_hdr)
>   		break;
>   	case MSG_TYPE_COMMAND:
>   	case MSG_TYPE_DELAYED_RESP:
> -		scmi_handle_response(cinfo, xfer_id, msg_type);
> +		scmi_handle_response(cinfo, msg_hdr);
>   		break;
>   	default:
>   		WARN_ONCE(1, "received unknown msg_type:%d\n", msg_type);
> @@ -550,7 +692,7 @@ void scmi_rx_callback(struct scmi_chan_info *cinfo, u32 msg_hdr)
>    * xfer_put() - Release a transmit message
>    *
>    * @ph: Pointer to SCMI protocol handle
> - * @xfer: message that was reserved by scmi_xfer_get
> + * @xfer: message that was reserved by xfer_get_init
>    */
>   static void xfer_put(const struct scmi_protocol_handle *ph,
>   		     struct scmi_xfer *xfer)
> @@ -568,7 +710,12 @@ static bool scmi_xfer_done_no_timeout(struct scmi_chan_info *cinfo,
>   {
>   	struct scmi_info *info = handle_to_scmi_info(cinfo->handle);
>   
> +	/*
> +	 * Poll also on xfer->done so that polling can be forcibly terminated
> +	 * in case of out-of-order receptions of delayed responses
> +	 */
>   	return info->desc->ops->poll_done(cinfo, xfer) ||
> +	       try_wait_for_completion(&xfer->done) ||
>   	       ktime_after(ktime_get(), stop);
>   }
>   
> @@ -608,6 +755,7 @@ static int do_xfer(const struct scmi_protocol_handle *ph,
>   			      xfer->hdr.protocol_id, xfer->hdr.seq,
>   			      xfer->hdr.poll_completion);
>   
> +	xfer->state = SCMI_XFER_SENT_OK;

To be completely safe, this assignment could also be protected by the 
xfer->lock.

>   	ret = info->desc->ops->send_message(cinfo, xfer);
>   	if (ret < 0) {
>   		dev_dbg(dev, "Failed to send message %d\n", ret);
> @@ -619,10 +767,22 @@ static int do_xfer(const struct scmi_protocol_handle *ph,
>   
>   		spin_until_cond(scmi_xfer_done_no_timeout(cinfo, xfer, stop));
>   
> -		if (ktime_before(ktime_get(), stop))
> -			info->desc->ops->fetch_response(cinfo, xfer);
> -		else
> +		if (ktime_before(ktime_get(), stop)) {
> +			unsigned long flags;
> +
> +			/*
> +			 * Do not fetch_response if an out-of-order delayed
> +			 * response is being processed.
> +			 */
> +			spin_lock_irqsave(&xfer->lock, flags);
> +			if (xfer->state == SCMI_XFER_SENT_OK) {
> +				info->desc->ops->fetch_response(cinfo, xfer);
> +				xfer->state = SCMI_XFER_RESP_OK;
> +			}
> +			spin_unlock_irqrestore(&xfer->lock, flags);
> +		} else {
>   			ret = -ETIMEDOUT;
> +		}
>   	} else {
>   		/* And we wait for the response. */
>   		timeout = msecs_to_jiffies(info->desc->max_rx_timeout_ms);
> @@ -1220,6 +1380,7 @@ static int __scmi_xfer_info_init(struct scmi_info *sinfo,
>   
>   		xfer->tx.buf = xfer->rx.buf;
>   		init_completion(&xfer->done);
> +		spin_lock_init(&xfer->lock);
>   
>   		/* Add initialized xfer to the free list */
>   		hlist_add_head(&xfer->node, &info->free_xfers);
> 


---------------------------------------------------------------------
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 v6 07/17] firmware: arm_scmi: Handle concurrent and out-of-order messages

[Peter Hilber]

On 19.07.21 11:14, Cristian Marussi wrote:
> On Thu, Jul 15, 2021 at 06:36:03PM +0200, Peter Hilber wrote:
>> On 12.07.21 16:18, Cristian Marussi wrote:

[snip]

>>> @@ -608,6 +755,7 @@ static int do_xfer(const struct scmi_protocol_handle *ph,
>>>    			      xfer->hdr.protocol_id, xfer->hdr.seq,
>>>    			      xfer->hdr.poll_completion);
>>> +	xfer->state = SCMI_XFER_SENT_OK;
>>
>> To be completely safe, this assignment could also be protected by the
>> xfer->lock.
>>
> 
> In fact this would be true being xfer->lock meant to protect the state but it
> seemed to me unnecessary here given that this is a brand new xfer with a
> brand new (monotonic) seq number so that any possibly late-received msg will
> carry an old stale seq number certainly different from this such that cannot be
> possibly mapped to this same xfer. (but just discarded on xfer lookup in
> xfer_command_acquire)
> 
> The issue indeed could still exist only for do_xfer loops (as you pointed out
> already early on) where the seq_num is used, but in that case on a timeout we
> would have already bailed out of the loop and reported an error so any timed-out
> late received response would have been anyway discarded; so at the end I thought
> I could avoid spinlocking here.
> 
> Thanks,
> Cristian
> 

I mostly meant to refer to the possibility of a very fast response not 
seeing this assignment, since the next line is

>  	ret = info->desc->ops->send_message(cinfo, xfer);

and during that a regular scmi_rx_callback(), reading xfer->state, can 
already arrive. But maybe this is too theoretical.

Best regards,

Peter

---------------------------------------------------------------------
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 v6 07/17] firmware: arm_scmi: Handle concurrent and out-of-order messages

[Cristian Marussi]

On Mon, Aug 02, 2021 at 11:10:32AM +0100, Sudeep Holla wrote:
> On Mon, Jul 12, 2021 at 03:18:23PM +0100, Cristian Marussi wrote:
> > Even though in case of asynchronous commands an SCMI platform server is
> 
> Drop the term "server"
> 

Sure.

> > constrained to emit the delayed response message only after the related
> > message response has been sent, the configured underlying transport could
> > still deliver such messages together or in inverted order, causing races
> > due to the concurrent or out-of-order access to the underlying xfer.
> > 
> > Introduce a mechanism to grant exclusive access to an xfer in order to
> > properly serialize concurrent accesses to the same xfer originating from
> > multiple correlated messages.
> > 
> > Add additional state information to xfer descriptors so as to be able to
> > identify out-of-order message deliveries and act accordingly:
> > 
> >  - when a delayed response is expected but delivered before the related
> >    response, the synchronous response is considered as successfully
> >    received and the delayed response processing is carried on as usual.
> > 
> >  - when/if the missing synchronous response is subsequently received, it
> >    is discarded as not congruent with the current state of the xfer, or
> >    simply, because the xfer has been already released and so, now, the
> >    monotonically increasing sequence number carried by the late response
> >    is stale.
> > 
> > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> > ---
> > v5 --> v6
> > - added spinlock comment
> > ---
> >  drivers/firmware/arm_scmi/common.h |  18 ++-
> >  drivers/firmware/arm_scmi/driver.c | 229 ++++++++++++++++++++++++-----
> >  2 files changed, 212 insertions(+), 35 deletions(-)
> > 
> > diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h
> > index 2233d0a188fc..9efebe1406d2 100644
> > --- a/drivers/firmware/arm_scmi/common.h
> > +++ b/drivers/firmware/arm_scmi/common.h
> > @@ -19,6 +19,7 @@
> >  #include <linux/module.h>
> >  #include <linux/refcount.h>
> >  #include <linux/scmi_protocol.h>
> > +#include <linux/spinlock.h>
> >  #include <linux/types.h>
> >  
> >  #include <asm/unaligned.h>
> > @@ -145,6 +146,13 @@ struct scmi_msg {
> >   * @pending: True for xfers added to @pending_xfers hashtable
> >   * @node: An hlist_node reference used to store this xfer, alternatively, on
> >   *	  the free list @free_xfers or in the @pending_xfers hashtable
> > + * @busy: An atomic flag to ensure exclusive write access to this xfer
> > + * @state: The current state of this transfer, with states transitions deemed
> > + *	   valid being:
> > + *	    - SCMI_XFER_SENT_OK -> SCMI_XFER_RESP_OK [ -> SCMI_XFER_DRESP_OK ]
> > + *	    - SCMI_XFER_SENT_OK -> SCMI_XFER_DRESP_OK
> > + *	      (Missing synchronous response is assumed OK and ignored)
> > + * @lock: A spinlock to protect state and busy fields.
> >   */
> >  struct scmi_xfer {
> >  	int transfer_id;
> > @@ -156,6 +164,15 @@ struct scmi_xfer {
> >  	refcount_t users;
> >  	bool pending;
> >  	struct hlist_node node;
> > +#define SCMI_XFER_FREE		0
> > +#define SCMI_XFER_BUSY		1
> > +	atomic_t busy;
> > +#define SCMI_XFER_SENT_OK	0
> > +#define SCMI_XFER_RESP_OK	1
> > +#define SCMI_XFER_DRESP_OK	2
> > +	int state;
> > +	/* A lock to protect state and busy fields */
> > +	spinlock_t lock;
> >  };
> >  
> >  /*
> > @@ -392,5 +409,4 @@ bool shmem_poll_done(struct scmi_shared_mem __iomem *shmem,
> >  void scmi_notification_instance_data_set(const struct scmi_handle *handle,
> >  					 void *priv);
> >  void *scmi_notification_instance_data_get(const struct scmi_handle *handle);
> > -
> >  #endif /* _SCMI_COMMON_H */
> > diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> > index 245ede223302..5ef33d692670 100644
> > --- a/drivers/firmware/arm_scmi/driver.c
> > +++ b/drivers/firmware/arm_scmi/driver.c
> > @@ -369,6 +369,7 @@ static struct scmi_xfer *scmi_xfer_get(const struct scmi_handle *handle,
> >  
> >  	if (!IS_ERR(xfer)) {
> >  		refcount_set(&xfer->users, 1);
> > +		atomic_set(&xfer->busy, SCMI_XFER_FREE);
> >  		xfer->transfer_id = atomic_inc_return(&transfer_last_id);
> >  	}
> >  	spin_unlock_irqrestore(&minfo->xfer_lock, flags);
> > @@ -430,6 +431,168 @@ scmi_xfer_lookup_unlocked(struct scmi_xfers_info *minfo, u16 xfer_id)
> >  	return xfer ?: ERR_PTR(-EINVAL);
> >  }
> >  
> > +/**
> > + * scmi_msg_response_validate  - Validate message type against state of related
> > + * xfer
> > + *
> > + * @cinfo: A reference to the channel descriptor.
> > + * @msg_type: Message type to check
> > + * @xfer: A reference to the xfer to validate against @msg_type
> > + *
> > + * This function checks if @msg_type is congruent with the current state of
> > + * a pending @xfer; if an asynchronous delayed response is received before the
> > + * related synchronous response (Out-of-Order Delayed Response) the missing
> > + * synchronous response is assumed to be OK and completed, carrying on with the
> > + * Delayed Response: this is done to address the case in which the underlying
> > + * SCMI transport can deliver such out-of-order responses.
> > + *
> > + * Context: Assumes to be called with xfer->lock already acquired.
> > + *
> > + * Return: 0 on Success, error otherwise
> > + */
> > +static inline int scmi_msg_response_validate(struct scmi_chan_info *cinfo,
> > +					     u8 msg_type,
> > +					     struct scmi_xfer *xfer)
> > +{
> > +	/*
> > +	 * Even if a response was indeed expected on this slot at this point,
> > +	 * a buggy platform could wrongly reply feeding us an unexpected
> > +	 * delayed response we're not prepared to handle: bail-out safely
> > +	 * blaming firmware.
> > +	 */
> > +	if (msg_type == MSG_TYPE_DELAYED_RESP && !xfer->async_done) {
> > +		dev_err(cinfo->dev,
> > +			"Delayed Response for %d not expected! Buggy F/W ?\n",
> > +			xfer->hdr.seq);
> > +		return -EINVAL;
> > +	}
> > +
> > +	switch (xfer->state) {
> > +	case SCMI_XFER_SENT_OK:
> > +		if (msg_type == MSG_TYPE_DELAYED_RESP) {
> > +			/*
> > +			 * Delayed Response expected but delivered earlier.
> > +			 * Assume message RESPONSE was OK and skip state.
> > +			 */
> > +			xfer->hdr.status = SCMI_SUCCESS;
> > +			xfer->state = SCMI_XFER_RESP_OK;
> > +			complete(&xfer->done);
> > +			dev_warn(cinfo->dev,
> > +				 "Received valid OoO Delayed Response for %d\n",
> > +				 xfer->hdr.seq);
> > +		}
> > +		break;
> > +	case SCMI_XFER_RESP_OK:
> > +		if (msg_type != MSG_TYPE_DELAYED_RESP)
> > +			return -EINVAL;
> > +		break;
> > +	case SCMI_XFER_DRESP_OK:
> > +		/* No further message expected once in SCMI_XFER_DRESP_OK */
> 
> Do we really need this case ? If so, how can this happen.
> 

Given that I am checking for state validity I thought to account also
for the case of possible (even though rare) duplicated delayed response.

> > +		return -EINVAL;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static bool scmi_xfer_is_free(struct scmi_xfer *xfer)
> > +{
> > +	int ret;
> > +
> > +	ret = atomic_cmpxchg(&xfer->busy, SCMI_XFER_FREE, SCMI_XFER_BUSY);
> > +
> > +	return ret == SCMI_XFER_FREE;
> > +}
> > +
> > +/**
> > + * scmi_xfer_command_acquire  -  Helper to lookup and acquire a command xfer
> > + *
> > + * @cinfo: A reference to the channel descriptor.
> > + * @msg_hdr: A message header to use as lookup key
> > + *
> > + * When a valid xfer is found for the sequence number embedded in the provided
> > + * msg_hdr, reference counting is properly updated and exclusive access to this
> > + * xfer is granted till released with @scmi_xfer_command_release.
> > + *
> > + * Return: A valid @xfer on Success or error otherwise.
> > + */
> > +static inline struct scmi_xfer *
> > +scmi_xfer_command_acquire(struct scmi_chan_info *cinfo, u32 msg_hdr)
> > +{
> > +	int ret;
> > +	unsigned long flags;
> > +	struct scmi_xfer *xfer;
> > +	struct scmi_info *info = handle_to_scmi_info(cinfo->handle);
> > +	struct scmi_xfers_info *minfo = &info->tx_minfo;
> > +	u8 msg_type = MSG_XTRACT_TYPE(msg_hdr);
> > +	u16 xfer_id = MSG_XTRACT_TOKEN(msg_hdr);
> > +
> > +	/* Are we even expecting this? */
> > +	spin_lock_irqsave(&minfo->xfer_lock, flags);
> > +	xfer = scmi_xfer_lookup_unlocked(minfo, xfer_id);
> > +	if (IS_ERR(xfer)) {
> > +		dev_err(cinfo->dev,
> > +			"Message for %d type %d is not expected!\n",
> > +			xfer_id, msg_type);
> > +		spin_unlock_irqrestore(&minfo->xfer_lock, flags);
> > +		return xfer;
> > +	}
> > +	refcount_inc(&xfer->users);
> > +	spin_unlock_irqrestore(&minfo->xfer_lock, flags);
> > +
> > +	spin_lock_irqsave(&xfer->lock, flags);
> > +	ret = scmi_msg_response_validate(cinfo, msg_type, xfer);
> > +	/*
> > +	 * If a pending xfer was found which was also in a congruent state with
> > +	 * the received message, acquire exclusive access to it setting the busy
> > +	 * flag.
> > +	 * Spins only on the rare limit condition of concurrent reception of
> > +	 * RESP and DRESP for the same xfer.
> > +	 */
> > +	if (!ret) {
> > +		spin_until_cond(scmi_xfer_is_free(xfer));
> 
> I agree with the discussion between you and Peter around this, so I assume
> it will be renamed or handled accordingly.
> 

Ok I'll rename it.

> > +		xfer->hdr.type = msg_type;
> > +	}
> > +	spin_unlock_irqrestore(&xfer->lock, flags);
> > +
> > +	if (ret) {
> > +		dev_err(cinfo->dev,
> > +			"Invalid message type:%d for %d - HDR:0x%X  state:%d\n",
> > +			msg_type, xfer_id, msg_hdr, xfer->state);
> > +		/* On error the refcount incremented above has to be dropped */
> > +		__scmi_xfer_put(minfo, xfer);
> > +		xfer = ERR_PTR(-EINVAL);
> > +	}
> > +
> > +	return xfer;
> > +}
> > +
> > +static inline void scmi_xfer_command_release(struct scmi_info *info,
> > +					     struct scmi_xfer *xfer)
> > +{
> > +	atomic_set(&xfer->busy, SCMI_XFER_FREE);
> > +	__scmi_xfer_put(&info->tx_minfo, xfer);
> > +}
> > +
> > +/**
> > + * scmi_xfer_state_update  - Update xfer state
> > + *
> > + * @xfer: A reference to the xfer to update
> > + *
> > + * Context: Assumes to be called on an xfer exclusively acquired using the
> > + *	    busy flag.
> > + */
> > +static inline void scmi_xfer_state_update(struct scmi_xfer *xfer)
> > +{
> > +	switch (xfer->hdr.type) {
> > +	case MSG_TYPE_COMMAND:
> > +		xfer->state = SCMI_XFER_RESP_OK;
> > +		break;
> > +	case MSG_TYPE_DELAYED_RESP:
> > +		xfer->state = SCMI_XFER_DRESP_OK;
> > +		break;
> > +	}
> > +}
> 
> Can't this be if () ..  else if(), switch sounds unnecessary for 2 conditions.
> 

Yes indeed I'll rework in V7.

> Other than the things already discussed with you and Peter, don't have much to
> add ATM. I may look at this with fresh eyes once again in the next version.
> 

Thanks for the review.

Cristian


---------------------------------------------------------------------
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 v6 10/17] firmware: arm_scmi: Make polling mode optional

[Peter Hilber]

On 12.07.21 16:18, Cristian Marussi wrote:
> Add a check for the presence of .poll_done transport operation so that
> transports that do not need to support polling mode have no need to provide
> a dummy .poll_done callback either and polling mode can be disabled in the
> SCMI core for that tranport.
> 
> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> ---
>   drivers/firmware/arm_scmi/driver.c | 43 ++++++++++++++++++------------
>   1 file changed, 26 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> index a952b6527b8a..4183d25c9289 100644
> --- a/drivers/firmware/arm_scmi/driver.c
> +++ b/drivers/firmware/arm_scmi/driver.c
> @@ -777,25 +777,34 @@ static int do_xfer(const struct scmi_protocol_handle *ph,
>   	}
>   
>   	if (xfer->hdr.poll_completion) {
> -		ktime_t stop = ktime_add_ns(ktime_get(), SCMI_MAX_POLL_TO_NS);
> -
> -		spin_until_cond(scmi_xfer_done_no_timeout(cinfo, xfer, stop));
> -
> -		if (ktime_before(ktime_get(), stop)) {
> -			unsigned long flags;
> -
> -			/*
> -			 * Do not fetch_response if an out-of-order delayed
> -			 * response is being processed.
> -			 */
> -			spin_lock_irqsave(&xfer->lock, flags);
> -			if (xfer->state == SCMI_XFER_SENT_OK) {
> -				info->desc->ops->fetch_response(cinfo, xfer);
> -				xfer->state = SCMI_XFER_RESP_OK;
> +		if (info->desc->ops->poll_done) {
> +			ktime_t stop = ktime_add_ns(ktime_get(),
> +						    SCMI_MAX_POLL_TO_NS);
> +
> +			spin_until_cond(scmi_xfer_done_no_timeout(cinfo, xfer,
> +								  stop));
> +
> +			if (ktime_before(ktime_get(), stop)) {
> +				unsigned long flags;
> +
> +				/*
> +				 * Do not fetch_response if an out-of-order delayed
> +				 * response is being processed.
> +				 */
> +				spin_lock_irqsave(&xfer->lock, flags);
> +				if (xfer->state == SCMI_XFER_SENT_OK) {
> +					info->desc->ops->fetch_response(cinfo,
> +									xfer);
> +					xfer->state = SCMI_XFER_RESP_OK;
> +				}
> +				spin_unlock_irqrestore(&xfer->lock, flags);
> +			} else {
> +				ret = -ETIMEDOUT;
>   			}
> -			spin_unlock_irqrestore(&xfer->lock, flags);
>   		} else {
> -			ret = -ETIMEDOUT;
> +			dev_warn_once(dev,
> +				      "Polling mode is not supported by transport.\n");
> +			ret = EINVAL;

s/EINVAL/-EINVAL

>   		}
>   	} else {
>   		/* And we wait for the response. */
> 


---------------------------------------------------------------------
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 v6 10/17] firmware: arm_scmi: Make polling mode optional

[Cristian Marussi]

On Wed, Jul 28, 2021 at 03:34:18PM +0100, Sudeep Holla wrote:
> On Mon, Jul 12, 2021 at 03:18:26PM +0100, Cristian Marussi wrote:
> > Add a check for the presence of .poll_done transport operation so that
> > transports that do not need to support polling mode have no need to provide
> > a dummy .poll_done callback either and polling mode can be disabled in the
> > SCMI core for that tranport.
> > 
> > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> > ---

Hi,

> >  drivers/firmware/arm_scmi/driver.c | 43 ++++++++++++++++++------------
> >  1 file changed, 26 insertions(+), 17 deletions(-)
> > 
> > diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> > index a952b6527b8a..4183d25c9289 100644
> > --- a/drivers/firmware/arm_scmi/driver.c
> > +++ b/drivers/firmware/arm_scmi/driver.c
> > @@ -777,25 +777,34 @@ static int do_xfer(const struct scmi_protocol_handle *ph,
> >  	}
> >  
> >  	if (xfer->hdr.poll_completion) {
> > -		ktime_t stop = ktime_add_ns(ktime_get(), SCMI_MAX_POLL_TO_NS);
> > -
> > -		spin_until_cond(scmi_xfer_done_no_timeout(cinfo, xfer, stop));
> > -
> > -		if (ktime_before(ktime_get(), stop)) {
> > -			unsigned long flags;
> > -
> > -			/*
> > -			 * Do not fetch_response if an out-of-order delayed
> > -			 * response is being processed.
> > -			 */
> > -			spin_lock_irqsave(&xfer->lock, flags);
> > -			if (xfer->state == SCMI_XFER_SENT_OK) {
> > -				info->desc->ops->fetch_response(cinfo, xfer);
> > -				xfer->state = SCMI_XFER_RESP_OK;
> > +		if (info->desc->ops->poll_done) {
> > +			ktime_t stop = ktime_add_ns(ktime_get(),
> > +						    SCMI_MAX_POLL_TO_NS);
> > +
> > +			spin_until_cond(scmi_xfer_done_no_timeout(cinfo, xfer,
> > +								  stop));
> > +
> > +			if (ktime_before(ktime_get(), stop)) {
> > +				unsigned long flags;
> > +
> > +				/*
> > +				 * Do not fetch_response if an out-of-order delayed
> > +				 * response is being processed.
> > +				 */
> > +				spin_lock_irqsave(&xfer->lock, flags);
> > +				if (xfer->state == SCMI_XFER_SENT_OK) {
> > +					info->desc->ops->fetch_response(cinfo,
> > +									xfer);
> > +					xfer->state = SCMI_XFER_RESP_OK;
> > +				}
> > +				spin_unlock_irqrestore(&xfer->lock, flags);
> > +			} else {
> > +				ret = -ETIMEDOUT;
> >  			}
> > -			spin_unlock_irqrestore(&xfer->lock, flags);
> >  		} else {
> > -			ret = -ETIMEDOUT;
> > +			dev_warn_once(dev,
> > +				      "Polling mode is not supported by transport.\n");
> > +			ret = EINVAL;
> 
> Can't we just return this error as early as possible if the user isn't
> expected to use polling with this transport ? That would simplify the patch
> (as most of it is due to indentation which can go away) as you need not
> check it later ?
> 

Yes, indeed at first it was something like

if (xfer->hdr.poll_completion && !info->desc->ops->poll_done)
	return -EINVAL;

at the very beginning of do_xfer(), even before attempting to send
anything...and I liked much more but then I thought I would have run such
if-test for each and every command do_xfer() attempted...but maybe it's
just irrelevant and instead much more tidy if done as above.

I'll fix it as above.

Thanks,
Cristian

---------------------------------------------------------------------
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 v6 14/17] firmware: arm_scmi: Add message passing abstractions for transports

[Peter Hilber]

On 12.07.21 16:18, Cristian Marussi wrote:
> From: Peter Hilber <peter.hilber@opensynergy.com>
> 
> Add abstractions for future transports using message passing, such as
> virtio. Derive the abstractions from the shared memory abstractions.
> 
> Abstract the transport SDU through the opaque struct scmi_msg_payld.
> Also enable the transport to determine all other required information
> about the transport SDU.
> 
> Signed-off-by: Peter Hilber <peter.hilber@opensynergy.com>
> [ Cristian: Adapted to new SCMI Kconfig layout, updated Copyrigths ]
> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> ---
> v4 --> v5
> - adapted to new SCMI Kconfig
> - removed raw_payload msg helpers
> v3 --> v4
> - added raw_payload msg helpers
> ---
>   drivers/firmware/arm_scmi/Kconfig  |   6 ++
>   drivers/firmware/arm_scmi/Makefile |   1 +
>   drivers/firmware/arm_scmi/common.h |  15 ++++
>   drivers/firmware/arm_scmi/msg.c    | 113 +++++++++++++++++++++++++++++
>   4 files changed, 135 insertions(+)
>   create mode 100644 drivers/firmware/arm_scmi/msg.c
> 
> diff --git a/drivers/firmware/arm_scmi/Kconfig b/drivers/firmware/arm_scmi/Kconfig
> index ee6517b24080..1fdaa8ad7d3f 100644
> --- a/drivers/firmware/arm_scmi/Kconfig
> +++ b/drivers/firmware/arm_scmi/Kconfig
> @@ -34,6 +34,12 @@ config ARM_SCMI_HAVE_SHMEM
>   	  This declares whether a shared memory based transport for SCMI is
>   	  available.
>   
> +config ARM_SCMI_HAVE_MSG
> +	bool
> +	help
> +	  This declares whether a message passing based transport for SCMI is
> +	  available.
> +
>   config ARM_SCMI_TRANSPORT_MAILBOX
>   	bool "SCMI transport based on Mailbox"
>   	depends on ARM_SCMI_PROTOCOL && MAILBOX
> diff --git a/drivers/firmware/arm_scmi/Makefile b/drivers/firmware/arm_scmi/Makefile
> index e0e6bd3dba9e..aaad9f6589aa 100644
> --- a/drivers/firmware/arm_scmi/Makefile
> +++ b/drivers/firmware/arm_scmi/Makefile
> @@ -4,6 +4,7 @@ scmi-driver-y = driver.o notify.o
>   scmi-transport-$(CONFIG_ARM_SCMI_HAVE_SHMEM) = shmem.o
>   scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_MAILBOX) += mailbox.o
>   scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_SMC) += smc.o
> +scmi-transport-$(CONFIG_ARM_SCMI_HAVE_MSG) += msg.o
>   scmi-protocols-y = base.o clock.o perf.o power.o reset.o sensors.o system.o voltage.o
>   scmi-module-objs := $(scmi-bus-y) $(scmi-driver-y) $(scmi-protocols-y) \
>   		    $(scmi-transport-y)
> diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h
> index 14457f0d5dea..7a1e84dc191b 100644
> --- a/drivers/firmware/arm_scmi/common.h
> +++ b/drivers/firmware/arm_scmi/common.h
> @@ -415,6 +415,21 @@ void shmem_clear_channel(struct scmi_shared_mem __iomem *shmem);
>   bool shmem_poll_done(struct scmi_shared_mem __iomem *shmem,
>   		     struct scmi_xfer *xfer);
>   
> +/* declarations for message passing transports */
> +struct scmi_msg_payld;
> +
> +/* Maximum overhead of message w.r.t. struct scmi_desc.max_msg_size */
> +#define SCMI_MSG_MAX_PROT_OVERHEAD (2 * sizeof(__le32))
> +
> +size_t msg_response_size(struct scmi_xfer *xfer);
> +size_t msg_command_size(struct scmi_xfer *xfer);
> +void msg_tx_prepare(struct scmi_msg_payld *msg, struct scmi_xfer *xfer);
> +u32 msg_read_header(struct scmi_msg_payld *msg);
> +void msg_fetch_response(struct scmi_msg_payld *msg, size_t len,
> +			struct scmi_xfer *xfer);
> +void msg_fetch_notification(struct scmi_msg_payld *msg, size_t len,
> +			    size_t max_len, 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
> new file mode 100644
> index 000000000000..639969b7dc10
> --- /dev/null
> +++ b/drivers/firmware/arm_scmi/msg.c
> @@ -0,0 +1,113 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * For transports using message passing.
> + *
> + * Derived from shm.c.
> + *
> + * Copyright (C) 2019-2021 ARM Ltd.
> + * Copyright (C) 2020-2021 OpenSynergy GmbH
> + */
> +
> +#include <linux/io.h>
> +#include <linux/processor.h>

The above two includes appear to be unnecessary.

> +#include <linux/types.h>
> +
> +#include "common.h"
> +
> +/*
> + * struct scmi_msg_payld - Transport SDU layout
> + *
> + * The SCMI specification requires all parameters, message headers, return
> + * arguments or any protocol data to be expressed in little endian format only.
> + */
> +struct scmi_msg_payld {
> +	__le32 msg_header;
> +	__le32 msg_payload[];
> +};
> +
> +/**
> + * msg_command_size() - Actual size of transport SDU for command.
> + *
> + * @xfer: message which core has prepared for sending
> + *
> + * Return: transport SDU size.
> + */
> +size_t msg_command_size(struct scmi_xfer *xfer)
> +{
> +	return sizeof(struct scmi_msg_payld) + xfer->tx.len;
> +}
> +
> +/**
> + * msg_response_size() - Maximum size of transport SDU for response.
> + *
> + * @xfer: message which core has prepared for sending
> + *
> + * Return: transport SDU size.
> + */
> +size_t msg_response_size(struct scmi_xfer *xfer)
> +{
> +	return sizeof(struct scmi_msg_payld) + sizeof(__le32) + xfer->rx.len;
> +}
> +
> +/**
> + * msg_tx_prepare() - Set up transport SDU for command.
> + *
> + * @msg: transport SDU for command
> + * @xfer: message which is being sent
> + */
> +void msg_tx_prepare(struct scmi_msg_payld *msg, struct scmi_xfer *xfer)
> +{
> +	msg->msg_header = cpu_to_le32(pack_scmi_header(&xfer->hdr));
> +	if (xfer->tx.buf)
> +		memcpy(msg->msg_payload, xfer->tx.buf, xfer->tx.len);
> +}
> +
> +/**
> + * msg_read_header() - Read SCMI header from transport SDU.
> + *
> + * @msg: transport SDU
> + *
> + * Return: SCMI header
> + */
> +u32 msg_read_header(struct scmi_msg_payld *msg)
> +{
> +	return le32_to_cpu(msg->msg_header);
> +}
> +
> +/**
> + * msg_fetch_response() - Fetch response SCMI payload from transport SDU.
> + *
> + * @msg: transport SDU with response
> + * @len: transport SDU size
> + * @xfer: message being responded to
> + */
> +void msg_fetch_response(struct scmi_msg_payld *msg, size_t len,
> +			struct scmi_xfer *xfer)
> +{
> +	size_t prefix_len = sizeof(*msg) + sizeof(msg->msg_payload[0]);
> +
> +	xfer->hdr.status = le32_to_cpu(msg->msg_payload[0]);
> +	xfer->rx.len = min_t(size_t, xfer->rx.len,
> +			     len >= prefix_len ? len - prefix_len : 0);
> +
> +	/* Take a copy to the rx buffer.. */
> +	memcpy(xfer->rx.buf, &msg->msg_payload[1], xfer->rx.len);
> +}
> +
> +/**
> + * msg_fetch_notification() - Fetch notification payload from transport SDU.
> + *
> + * @msg: transport SDU with notification
> + * @len: transport SDU size
> + * @max_len: maximum SCMI payload size to fetch
> + * @xfer: notification message
> + */
> +void msg_fetch_notification(struct scmi_msg_payld *msg, size_t len,
> +			    size_t max_len, struct scmi_xfer *xfer)
> +{
> +	xfer->rx.len = min_t(size_t, max_len,
> +			     len >= sizeof(*msg) ? len - sizeof(*msg) : 0);
> +
> +	/* Take a copy to the rx buffer.. */
> +	memcpy(xfer->rx.buf, msg->msg_payload, xfer->rx.len);
> +}
> 


---------------------------------------------------------------------
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 v6 06/17] firmware: arm_scmi: Introduce monotonically increasing tokens

[Cristian Marussi]

On Wed, Jul 28, 2021 at 03:17:46PM +0100, Sudeep Holla wrote:
> On Mon, Jul 12, 2021 at 03:18:22PM +0100, 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.
> >

Hi,

> 
> The term ghost is used here without any reference to what it means. That
> could make it difficult to follow if someone unaware of it is trying to
> understand.
> 

Right, I'll reword this.

> > This commit introduces a new policy of selection of tokens for the OSPM
> > agent: each new command transfer now gets the next available, 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.
> > 
> > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> > ---
> > v4 --> V5
> > - removed empirical profiling info from commit msg
> > - do NOT use monotonic tokens and pending HT for notifications (not needed)
> > - release xfer_lock later in scmi_xfer_get
> > ---
> >  drivers/firmware/arm_scmi/common.h |  25 +++
> >  drivers/firmware/arm_scmi/driver.c | 260 +++++++++++++++++++++++++----
> >  2 files changed, 249 insertions(+), 36 deletions(-)
> > 
> > diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h
> > index 6bb734e0e3ac..2233d0a188fc 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>
> >  
> > @@ -138,6 +141,10 @@ 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
> > + * @pending: True for xfers added to @pending_xfers hashtable
> > + * @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,8 +153,26 @@ struct scmi_xfer {
> >  	struct scmi_msg rx;
> >  	struct completion done;
> >  	struct completion *async_done;
> > +	refcount_t users;
> > +	bool pending;
> > +	struct hlist_node node;
> >  };
> >  
> > +/*
> > + * 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_;							\
> > +})
> > +
> >  struct scmi_xfer_ops;
> >  
> >  /**
> > diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> > index 4c77ee13b1ad..245ede223302 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
> > +
> 
> Is there any particular reason to choose half the token size as hash bucket
> size ? IOW why not 1/3 or 1/4th of it ? I would appreciate a comment here.
> I see it is mentioned in the commit log. Also is it not better to associate
> or keep it close to MSG_TOKEN_ID_MASK and associated macros.
> 

I'll move this in the proper place where associated macros are defined.

The reason for the size choice is tricky (and not sure about its value
still...so I have not commented yet :D); the ideal size of this hashtable would
be desc->max_msg so equal to the maximum number of inflight messages allowed on
the system in order to minimize (probably to zero) collisions on the hashtable:
unfortunately max_msg is only finally available at runtime time and the
kernel hashtable is statically sized by design....

I tried to play some tricks to define dynamically the size but everything falls
apart since a lot of stuff in linux/hashtable.h is based on ARRAY_SIZE() and
friends (to speedup all I suppose). Another non-fit (in my opinion)
alternative would be using relativistic hashtable (linux/rhashtable.h) but
those are definitely overkill in our case since they are hashtables that
can be resized completely at runtime while populated O_o. (with even
more overhead)

At the end the size that fits all possible in-flight messages minimizing
collisions in any possible case that I can set at compile time would be 10,
which means really 2^10 1024 HT entries (equal to MAX_MSG_TOKEN) each of which
is a struct list_head (*prev,*next 16bytes) i.e. 16KB HT: Peter pointed out
that it would be a lot of wasted space on normal systems in which max in-flight
messages are far-less than 1024 AND would not even fit in one 4Kb page, so I
reduced it to 512 entries but the best would be 256 (8) if we want to
fit in one regular 4kb page. The drawback will be a bit of HT collisions on
system with more than 256 possible and effective in-flight messages.

So, best of all would be runtime sizing of the HT, which is not possible
with current statically sized Kernel hashtable.h (even though I started looking
into that I have to admit :D), or sizing to 8 (256 entries 4k) to fit in a page,
or 10 (1024 entries 16kb) to minimize any possible collision if we do
not care about wasted memory and/or page boundary.

> >  /**
> >   * 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;
> 
> Can we merge this and transfer_last_id ? Let this be free running like
> transfer_last_id and just use [0-9] from this ? I don't see any point
> having 2 different monotonically increasing tokens/id.
> 

Mmm I was tempted about that, but the reason I did not was that in some
rare limit condition as you can see in the ASCII art (:O) I can find a hole in
the next available token ids so I have to skip and update last_token itself,
not sure if this could cause confusion seeing transfer_ids with holes during
tracing if I unify them.

> > +	struct hlist_head free_xfers;
> > +	DECLARE_HASHTABLE(pending_xfers, SCMI_PENDING_XFERS_HT_ORDER_SZ);
> >  };
> >  
> >  /**
> > @@ -190,45 +201,177 @@ 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
> >   *
> >   * @handle: Pointer to SCMI entity handle
> >   * @minfo: Pointer to Tx/Rx Message management info based on channel type
> > + * @set_pending: If true a monotonic token is picked and the xfer is added to
> > + *		 the pending hash table.
> >   *
> >   * 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) and, if
> > + * required, 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)
> > +				       struct scmi_xfers_info *minfo,
> > +				       bool set_pending)
> >  {
> > -	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);
> > +
> > +	if (set_pending) {
> > +		/* Pick and set monotonic token */
> > +		ret = scmi_xfer_token_set(minfo, xfer);
> > +		if (!ret) {
> > +			hash_add(minfo->pending_xfers, &xfer->node,
> > +				 xfer->hdr.seq);
> > +			xfer->pending = true;
> > +		} 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);
> > +		}
> > +	}
> >  
> > -	xfer = &minfo->xfer_block[xfer_id];
> > -	xfer->hdr.seq = xfer_id;
> > -	xfer->transfer_id = atomic_inc_return(&transfer_last_id);
> > +	if (!IS_ERR(xfer)) {
> > +		refcount_set(&xfer->users, 1);
> > +		xfer->transfer_id = atomic_inc_return(&transfer_last_id);
> > +	}
> > +	spin_unlock_irqrestore(&minfo->xfer_lock, flags);
> >  
> >  	return xfer;
> >  }
> > @@ -239,6 +382,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
> > @@ -246,16 +392,44 @@ __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)) {
> 
> The introduction of users in this patch seems useless. I am assuming there are
> multiple users and this is to prevent some race. I was about to ask if we can
> manage without it, but seeing some additional use of it in later patches, I
> will comment later. It may still make sense to move this to later patch as
> it doesn't add anything here ?
> 

Indeed I was not sure about introducing it here, the problem that addresses is
as follows: a normal cmd flow is xfer_get/send/rx/xfer_put ... but if a command
times out on the TX path BEFORE is finally xfer_put() by the invoking protocol,
the MSG is still valid (as state) and pending for a while and if something is
received in the RX path concurrently, the transport calls scmi_rx_callback and
the risk would be that the xfer would be acquired just fine and processed by RX
(still has to be xfer_put as said) and then be suddendly vanished by the final
xfer_put on the TX path while RX is working on it. (or worst released while RX
is working on it and then picked from the free_list and reused for send while
RX is still processing it...ok bit of unlucky case I have to admit..)

So xfer->users is meant to avoid the xfer being vanished by the xfer_put() in
the TX path while rx_callback is still working on it....in such a case the
rx_callback will be allowed to process safely the xfer (even though it timed
out in TX) till the end of its RX-processing and the result will be then finally
discarded at the end with the final __scmi_xfer_put inside handle_response
that at the end releases the xfer once for all.

Using a refcount allows me to avoid bigger spinlocked critical sections (or at least
was the idea :D)

> > +		if (xfer->pending) {
> > +			scmi_xfer_token_clear(minfo, xfer);
> > +			hash_del(&xfer->node);
> > +			xfer->pending = false;
> > +		}
> > +		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);
> > +
> 
> Is this really needed ? I guess we always use MSG_XTRACT_TOKEN(hdr) to
> fetch the xfer_id, no ?
> 

Right, indeed comes from a MSG_XTRACT_TOKEN, I'll drop it.

Thanks,
Cristian


---------------------------------------------------------------------
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 v6 06/17] firmware: arm_scmi: Introduce monotonically increasing tokens

[Cristian Marussi]

On Mon, Aug 02, 2021 at 11:24:25AM +0100, Sudeep Holla wrote:
> On Wed, Jul 28, 2021 at 05:54:30PM +0100, Cristian Marussi wrote:
> > On Wed, Jul 28, 2021 at 03:17:46PM +0100, Sudeep Holla wrote:
> > > On Mon, Jul 12, 2021 at 03:18:22PM +0100, Cristian Marussi wrote:
> 
> [...]
> 

Hi Sudeep,

I'm goignt to post V7 with all the remarks on the series from you and Peter
addressed.  A few notes beow on this patch.

> > > >  
> > > > +#define SCMI_PENDING_XFERS_HT_ORDER_SZ	9
> > > > +
> > > 
> > > Is there any particular reason to choose half the token size as hash bucket
> > > size ? IOW why not 1/3 or 1/4th of it ? I would appreciate a comment here.
> > > I see it is mentioned in the commit log. Also is it not better to associate
> > > or keep it close to MSG_TOKEN_ID_MASK and associated macros.
> > > 
> > 
> > I'll move this in the proper place where associated macros are defined.
> > 
> > The reason for the size choice is tricky (and not sure about its value
> > still...so I have not commented yet :D); the ideal size of this hashtable would
> > be desc->max_msg so equal to the maximum number of inflight messages allowed on
> > the system in order to minimize (probably to zero) collisions on the hashtable:
> > unfortunately max_msg is only finally available at runtime time and the
> > kernel hashtable is statically sized by design....
> > 
> > I tried to play some tricks to define dynamically the size but everything falls
> > apart since a lot of stuff in linux/hashtable.h is based on ARRAY_SIZE() and
> > friends (to speedup all I suppose). Another non-fit (in my opinion)
> > alternative would be using relativistic hashtable (linux/rhashtable.h) but
> > those are definitely overkill in our case since they are hashtables that
> > can be resized completely at runtime while populated O_o. (with even
> > more overhead)
> > 
> > At the end the size that fits all possible in-flight messages minimizing
> > collisions in any possible case that I can set at compile time would be 10,
> > which means really 2^10 1024 HT entries (equal to MAX_MSG_TOKEN) each of which
> > is a struct list_head (*prev,*next 16bytes) i.e. 16KB HT: Peter pointed out
> > that it would be a lot of wasted space on normal systems in which max in-flight
> > messages are far-less than 1024 AND would not even fit in one 4Kb page, so I
> > reduced it to 512 entries but the best would be 256 (8) if we want to
> > fit in one regular 4kb page. The drawback will be a bit of HT collisions on
> > system with more than 256 possible and effective in-flight messages.
> >
> 
> I agree, 256 should be fine for now. Just add a note that it is chosen to
> fit a page and can be updated if required.
> 
> 

Yes I added a comment explaining the situation, but, my bad, the 'good'
size that fits the HT into a 4k page is indeed 512 (9) because the HT array
is made of hlist_head (so only one pointer, good I double checked on myself :P)

> > > >  /**
> > > >   * 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;
> > > 
> > > Can we merge this and transfer_last_id ? Let this be free running like
> > > transfer_last_id and just use [0-9] from this ? I don't see any point
> > > having 2 different monotonically increasing tokens/id.
> > > 
> > 
> > Mmm I was tempted about that, but the reason I did not was that in some
> > rare limit condition as you can see in the ASCII art (:O) I can find a hole in
> > the next available token ids so I have to skip and update last_token itself,
> > not sure if this could cause confusion seeing transfer_ids with holes during
> > tracing if I unify them.
> >
> 
> That should be fine as it won't be used at all.
> 

In V7 I removed mx_info last_token and I'm using last_transfer_id; my
only concern (mostly theoretical I think) is that last_transfer_id is
picked from a global atomic and so it is shared by any platform or
channel instance. (and being global is good for tracing correlation
in fact...)

In particular this means that it is also shared with notifications
(which do not use monotonic seqnums), so that, if a flood of notifs
suddenly kicks in, a lot of transfer_idS would be burnt (for tracing
notifs.. not for seqnums) and any following command will end up picking
a good valid monotonic seqnum BUT far away from the previous one
(i.e. there will be holes in the set of monotonic seqnums): no issue
with that but it could lead, under some very awkward limit conditions,
to an early reuse of seqnums for commands (i.e. a less effective
mitigation of seqnums reusing).

Anyway in my tests on this new version I have see no issues and the
above mentioned case is clearly borderline so I made the change (and
noted the above scenario in a comment)

Thanks,
Cristian


---------------------------------------------------------------------
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 v6 08/17] firmware: arm_scmi: Add priv parameter to scmi_rx_callback

[Cristian Marussi]

On Wed, Jul 28, 2021 at 03:26:25PM +0100, Sudeep Holla wrote:
> On Mon, Jul 12, 2021 at 03:18:24PM +0100, Cristian Marussi wrote:
> > Add a new opaque void *priv parameter to scmi_rx_callback which can be
> > optionally provided by the transport layer when invoking scmi_rx_callback
> > and that will be passed back to the transport layer in xfer->priv.
> > 
> > This can be used by transports that needs to keep track of their specific
> > data structures together with the valid xfers.
> >

Hi,

> 
> This change looks simple but doesn't make sense on its own. I assume you don't
> want to add all these in the patch making use of priv which makes sense. Not
> sure if the next patch uses it, but I prefer to keep them as close as possible
> if it is not too much of a hassle.
> 

Right, yes I'll move it close to 17/17 which is the actual user of this
at the end.

Thanks,
Cristian


---------------------------------------------------------------------
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 v6 11/17] firmware: arm_scmi: Make SCMI transports configurable

[Cristian Marussi]

On Wed, Jul 28, 2021 at 03:50:33PM +0100, Sudeep Holla wrote:
> On Mon, Jul 12, 2021 at 03:18:27PM +0100, Cristian Marussi wrote:
> > Add configuration options to be able to select which SCMI transports have
> > to be compiled into the SCMI stack.
> > 
> > Mailbox and SMC are by default enabled if their related dependencies are
> > satisfied.
> > 
> > While doing that move all SCMI related config options in their own
> > dedicated submenu.
> > 
> > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> > ---
> > Used a BUILD_BUG_ON() to avoid the scenario where SCMI is configured
> > without any transport. Coul dnot do in any other way in Kconfig due to
> > circular dependencies.
> > 
> > This will be neeed later on to add new Virtio based transport and
> > optionally exclude other transports.
> > ---

Hi Sudeep,

> >  drivers/firmware/Kconfig           | 34 +--------------
> >  drivers/firmware/arm_scmi/Kconfig  | 70 ++++++++++++++++++++++++++++++
> >  drivers/firmware/arm_scmi/Makefile |  4 +-
> >  drivers/firmware/arm_scmi/common.h |  4 +-
> >  drivers/firmware/arm_scmi/driver.c |  6 ++-
> >  5 files changed, 80 insertions(+), 38 deletions(-)
> >  create mode 100644 drivers/firmware/arm_scmi/Kconfig
> > 
> > diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
> > index 1db738d5b301..8d41f73f5395 100644
> > --- a/drivers/firmware/Kconfig
> > +++ b/drivers/firmware/Kconfig
> > @@ -6,39 +6,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
> > -	help
> > -	  ARM System Control and Management Interface (SCMI) protocol is a
> > -	  set of operating system-independent software interfaces that are
> > -	  used in system management. SCMI is extensible and currently provides
> > -	  interfaces for: Discovery and self-description of the interfaces
> > -	  it supports, Power domain management which is the ability to place
> > -	  a given device or domain into the various power-saving states that
> > -	  it supports, Performance management which is the ability to control
> > -	  the performance of a domain that is composed of compute engines
> > -	  such as application processors and other accelerators, Clock
> > -	  management which is the ability to set and inquire rates on platform
> > -	  managed clocks and Sensor management which is the ability to read
> > -	  sensor data, and be notified of sensor value.
> > -
> > -	  This protocol library provides interface for all the client drivers
> > -	  making use of the features offered by the SCMI.
> > -
> > -config ARM_SCMI_POWER_DOMAIN
> > -	tristate "SCMI power domain driver"
> > -	depends on ARM_SCMI_PROTOCOL || (COMPILE_TEST && OF)
> > -	default y
> > -	select PM_GENERIC_DOMAINS if PM
> > -	help
> > -	  This enables support for the SCMI power domains which can be
> > -	  enabled or disabled via the SCP firmware
> > -
> > -	  This driver can also be built as a module.  If so, the module
> > -	  will be called scmi_pm_domain. Note this may needed early in boot
> > -	  before rootfs may be available.
> > +source "drivers/firmware/arm_scmi/Kconfig"
> >  
> >  config ARM_SCPI_PROTOCOL
> >  	tristate "ARM System Control and Power Interface (SCPI) Message Protocol"
> > diff --git a/drivers/firmware/arm_scmi/Kconfig b/drivers/firmware/arm_scmi/Kconfig
> > new file mode 100644
> > index 000000000000..479fc8a3533e
> > --- /dev/null
> > +++ b/drivers/firmware/arm_scmi/Kconfig
> > @@ -0,0 +1,70 @@
> > +# SPDX-License-Identifier: GPL-2.0-only
> > +menu "ARM System Control and Management Interface Protocol"
> > +
> > +config ARM_SCMI_PROTOCOL
> > +	tristate "ARM System Control and Management Interface (SCMI) Message Protocol"
> > +	depends on ARM || ARM64 || COMPILE_TEST
> > +	help
> > +	  ARM System Control and Management Interface (SCMI) protocol is a
> > +	  set of operating system-independent software interfaces that are
> > +	  used in system management. SCMI is extensible and currently provides
> > +	  interfaces for: Discovery and self-description of the interfaces
> > +	  it supports, Power domain management which is the ability to place
> > +	  a given device or domain into the various power-saving states that
> > +	  it supports, Performance management which is the ability to control
> > +	  the performance of a domain that is composed of compute engines
> > +	  such as application processors and other accelerators, Clock
> > +	  management which is the ability to set and inquire rates on platform
> > +	  managed clocks and Sensor management which is the ability to read
> > +	  sensor data, and be notified of sensor value.
> > +
> > +	  This protocol library provides interface for all the client drivers
> > +	  making use of the features offered by the SCMI.
> > +
> 
> May be you can add if condition here to remove the depends on ARM_SCMI_PROTOCOL
> 
> if ARM_SCMI_PROTOCOL
> 

I'll do.

> > +config ARM_SCMI_HAVE_TRANSPORT
> > +	bool
> > +	help
> > +	  This declares whether at least one SCMI transport has been configured.
> > +	  Used to trigger a build bug when trying to build SCMI without any
> > +	  configured transport.
> > +
> > +config ARM_SCMI_TRANSPORT_MAILBOX
> > +	bool "SCMI transport based on Mailbox"
> > +	depends on ARM_SCMI_PROTOCOL && MAILBOX
> 
> And drop ARM_SCMI_PROTOCOL above
> 

Ditto

> > +	select ARM_SCMI_HAVE_TRANSPORT
> > +	default y
> 
> Do we need a user visible choice if it is always default on ?
> 

I'll leave for now as said offline to be able to not build unsupported
transport.

> > +	help
> > +	  Enable mailbox based transport for SCMI.
> > +
> > +	  If you want the ARM SCMI PROTOCOL stack to include support for a
> > +	  transport based on mailboxes, answer Y.
> > +	  A matching DT entry will also be needed to indicate the effective
> > +	  presence of this kind of transport.
> > +
> 
> I would drop the above comment on matching DT.
> 

Yes right, I was in doubt in fact if it was sensible place for that
comment.

> > +config ARM_SCMI_TRANSPORT_SMC
> > +	bool "SCMI transport based on SMC"
> > +	depends on ARM_SCMI_PROTOCOL && HAVE_ARM_SMCCC_DISCOVERY
> 
> Ditto
> 

DittoAck :D

> > +	select ARM_SCMI_HAVE_TRANSPORT
> > +	default y
> > +	help
> > +	  Enable SMC based transport for SCMI.
> > +
> > +	  If you want the ARM SCMI PROTOCOL stack to include support for a
> > +	  transport based on SMC, answer Y.
> > +	  A matching DT entry will also be needed to indicate the effective
> > +	  presence of this kind of transport.
> > +
> 
> endif #ARM_SCMI_PROTOCOL
> 

Thanks,
Cristian

---------------------------------------------------------------------
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 v6 15/17] firmware: arm_scmi: Add optional link_supplier() transport op

[Cristian Marussi]

On Wed, Jul 28, 2021 at 04:36:47PM +0100, Sudeep Holla wrote:
> On Mon, Jul 12, 2021 at 03:18:31PM +0100, Cristian Marussi wrote:
> > From: Peter Hilber <peter.hilber@opensynergy.com>
> > 
> > Some transports are also effectively registered with other kernel subsystem
> > in order to be properly probed and initialized; as a consequence such kind
> > of transports, and their related devices, might still not have been probed
> > and initialized at the time the main SCMI core driver is probed.
> > 
> > Add an optional .link_supplier() transport operation which can be used by
> > the core SCMI stack to dynamically check if the transport is ready and
> > dynamically link its device to the platform instance device.
> >
> 
> To be precise,
> 
> s/platform instance device/SCMI platform instance device

Yes I'll fix.

Thanks,
Cristian


---------------------------------------------------------------------
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 v6 00/17] Introduce SCMI transport based on VirtIO

[Cristian Marussi]

On Wed, Aug 11, 2021 at 09:31:21AM +0000, Floris Westermann wrote:
> Hi Cristian,
> 

Hi Floris,

> I am currently working on an interface for VMs to communicate their
> performance requirements to the hosts by passing through cpu frequency
> adjustments.
> 

So something like looking up SCMI requests from VMs in the hypervisor
and act on VM underlying hw accordingly ? Where the SCMI server is meant
to live ?

> Your patch looks very interesting but I have some questions:
> 

Happy to hear that, a new V7 (with minor cleanups) which is (hopefully)
being pulled these days is at:

https://lore.kernel.org/linux-arm-kernel/20210803131024.40280-1-cristian.marussi@arm.com/
> 
> On Mon, Jul 12, 2021 at 03:18:16PM +0100, Cristian Marussi wrote:
> >
> > The series has been tested using an emulated fake SCMI device and also a
> > proper SCP-fw stack running through QEMU vhost-users, with the SCMI stack
> > compiled, in both cases, as builtin and as a loadable module, running tests
> > against mocked SCMI Sensors using HWMON and IIO interfaces to check the
> > functionality of notifications and sync/async commands.
> >
> > Virtio-scmi support has been exercised in the following testing scenario
> > on a JUNO board:
> >
> >  - normal sync/async command transfers
> >  - notifications
> >  - concurrent delivery of correlated response and delayed responses
> >  - out-of-order delivery of delayed responses before related responses
> >  - unexpected delayed response delivery for sync commands
> >  - late delivery of timed-out responses and delayed responses
> >
> > Some basic regression testing against mailbox transport has been performed
> > for commands and notifications too.
> >
> > No sensible overhead in total handling time of commands and notifications
> > has been observed, even though this series do indeed add a considerable
> > amount of code to execute on TX path.
> > More test and measurements could be needed in these regards.
> >
> 
> Can you share any data and benchmarks using you fake SCMI device.
> Also, could you provide the emulated device code so that the results can
> be reproduced.
> 

Not really, because the testing based on the fake SCMI VirtIO device was
purely functional, just to exercise some rare limit conditions not
easily reproducible with a regular SCMI stack, I've made no benchmark
using the fake emulated SCMI virtio device because it mimics VirtIO
transfers but there's not even an host/guest in my fake emulation.
Moreover is a hacked driver+userspace blob not really in a state to be
shared :P

While developing this series I needed somehow to be able to let the Kernel
SCMI-agent in the guest "speak" some basic SCMI commands and notifs to
some SCMI server platform sitting somewhere across the new VirtIO SCMI
transport, which basically means that it's not enough to create an SCMI
VirtIO device reachable from the VirtIO layer, the SCMI stack itself
(or part of it) must live behind such device somehow/somewhere to be
able to receive meaningful replies.

One proper way to do that is to use some QEMU/SCP-fw vhost-users support
cooked by Linaro (not in the upstream for now) so as to basically run a
full proper SCP SCMI-platform fw stack in host userspace and let it speak
to a guest through the new scmi virtio transport and the vhost-users magic:
the drawback of this kind of approach is that it made hard to test limit
conditions like stale or out-of-order SCMI replies because to do so you
have to patch the official SCP/SCMI stack to behave badly and out-of specs,
which is not something is designed to do. (and also the fact that the
vhost-users QEMU/SCP-fw solution was only available later during devel).
Hence the emulation hack for testing rare limit conditions.

Now, the emulation hack, beside really ugly, is clearly a totally fake
VirtIO environment (there's not even a guest really...) and, as said, it
just served the need to exercise the SCMI virtio transport code enough to
test anomalous and bad-behaving SCMI commands flows and notifications and
as such made really no sense to be used as a performance testbed.

In fact, what I was really meaning (poorly) while saying:

> > No sensible overhead in total handling time of commands and notifications
> > has been observed, even though this series do indeed add a considerable
> > amount of code to execute on TX path.

is that I have NOT seen any sensible overhead/slowdown in the context of
OTHER real SCMI transports (like mailboxes), because the virtio series
contains a number of preliminary SCMI common core changes unrelated to
virtio (monotonic tokens/handle concurrent and out-of-order replies) that,
even though easing the devel of SCMI virtio, are really needed and used
by any other existent SCMI transport, so my fear was to introduce some
common slowdown in the core: in those regards only, I said that I have
not seen (looking at cmd traces) any slowdown with such additional core
changes even though more code is clearly now run in the TX path.
(contention can indeed only happen under very rare limit conditions)

Having said that (sorry for the flood of words) what I can give you are
a few traces (non statistically significant probably) showing the typical
round-trip time for some plain SCMI command sensor requests on an idle
system (JUNO)

In both cases the sensors being read are mocked, so the time is purely
related to SCMI core stack and virtio exchanges (i.e. there's no delay
introduced by reading real hw sensors)


-> Using a proper SCP-fw/QEMU vhost-users stack:

root@deb-guest:~# cat /sys/class/hwmon/hwmon0/temp1_input 
             cat-195     [000] ....  7044.614295: scmi_xfer_begin: transfer_id=27 msg_id=6 protocol_id=21 seq=27 poll=0
25000
          <idle>-0       [000] d.h3  7044.615342: scmi_rx_done: transfer_id=27 msg_id=6 protocol_id=21 seq=27 msg_type=0
             cat-195     [000] ....  7044.615420: scmi_xfer_end: transfer_id=27 msg_id=6 protocol_id=21 seq=27 status=0

root@deb-guest:~# cat /sys/class/hwmon/hwmon0/temp1_input 
             cat-196     [000] ....  7049.200349: scmi_xfer_begin: transfer_id=28 msg_id=6 protocol_id=21 seq=28 poll=0
          <idle>-0       [000] d.h3  7049.202053: scmi_rx_done: transfer_id=28 msg_id=6 protocol_id=21 seq=28 msg_type=0
             cat-196     [000] ....  7049.202152: scmi_xfer_end: transfer_id=28 msg_id=6 protocol_id=21 seq=28 status=0
25000

root@deb-guest:~# cat /sys/class/hwmon/hwmon0/temp1_input 
             cat-197     [000] ....  7053.699713: scmi_xfer_begin: transfer_id=29 msg_id=6 protocol_id=21 seq=29 poll=0
25000
          <idle>-0       [000] d.H3  7053.700366: scmi_rx_done: transfer_id=29 msg_id=6 protocol_id=21 seq=29 msg_type=0
             cat-197     [000] ....  7053.700468: scmi_xfer_end: transfer_id=29 msg_id=6 protocol_id=21 seq=29 status=0

root@deb-guest:~# cat /sys/class/hwmon/hwmon0/temp1_input 
             cat-198     [001] ....  7058.944442: scmi_xfer_begin: transfer_id=30 msg_id=6 protocol_id=21 seq=30 poll=0
             cat-173     [000] d.h2  7058.944959: scmi_rx_done: transfer_id=30 msg_id=6 protocol_id=21 seq=30 msg_type=0
             cat-198     [001] ....  7058.945500: scmi_xfer_end: transfer_id=30 msg_id=6 protocol_id=21 seq=30 status=0
25000

root@deb-guest:~# cat /sys/class/hwmon/hwmon0/temp1_input 
             cat-199     [000] ....  7064.598797: scmi_xfer_begin: transfer_id=31 msg_id=6 protocol_id=21 seq=31 poll=0
25000
          <idle>-0       [000] d.h3  7064.599710: scmi_rx_done: transfer_id=31 msg_id=6 protocol_id=21 seq=31 msg_type=0
             cat-199     [000] ....  7064.599787: scmi_xfer_end: transfer_id=31 msg_id=6 protocol_id=21 seq=31 status=0



-> Using the fake hack SCMI device that relays packets to userspace:

             cat-1306    [000] ....  7614.373161: scmi_xfer_begin: transfer_id=78 msg_id=6 protocol_id=21 seq=78 poll=0
 scmi_sniffer_ng-342     [000] d.h2  7614.373699: scmi_rx_done: transfer_id=78 msg_id=6 protocol_id=21 seq=78 msg_type=0
             cat-1306    [000] ....  7614.377653: scmi_xfer_end: transfer_id=78 msg_id=6 protocol_id=21 seq=78 status=0


             cat-1308    [004] ....  7626.677176: scmi_xfer_begin: transfer_id=79 msg_id=6 protocol_id=21 seq=79 poll=0
 scmi_sniffer_ng-342     [000] d.h2  7626.677653: scmi_rx_done: transfer_id=79 msg_id=6 protocol_id=21 seq=79 msg_type=0
             cat-1308    [004] ....  7626.677705: scmi_xfer_end: transfer_id=79 msg_id=6 protocol_id=21 seq=79 status=0


             cat-1309    [004] ....  7631.249412: scmi_xfer_begin: transfer_id=80 msg_id=6 protocol_id=21 seq=80 poll=0
 scmi_sniffer_ng-342     [000] d.h2  7631.250182: scmi_rx_done: transfer_id=80 msg_id=6 protocol_id=21 seq=80 msg_type=0
             cat-1309    [004] ....  7631.250237: scmi_xfer_end: transfer_id=80 msg_id=6 protocol_id=21 seq=80 status=0

             cat-1312    [004] ....  7642.210034: scmi_xfer_begin: transfer_id=81 msg_id=6 protocol_id=21 seq=81 poll=0
 scmi_sniffer_ng-342     [000] d.h2  7642.210514: scmi_rx_done: transfer_id=81 msg_id=6 protocol_id=21 seq=81 msg_type=0
             cat-1312    [004] ....  7642.210567: scmi_xfer_end: transfer_id=81 msg_id=6 protocol_id=21 seq=81 status=0

             cat-1314    [003] ....  7645.810775: scmi_xfer_begin: transfer_id=82 msg_id=6 protocol_id=21 seq=82 poll=0
 scmi_sniffer_ng-342     [000] d.h2  7645.811255: scmi_rx_done: transfer_id=82 msg_id=6 protocol_id=21 seq=82 msg_type=0
             cat-1314    [003] ....  7645.811307: scmi_xfer_end: transfer_id=82 msg_id=6 protocol_id=21 seq=82 status=0


In both cases SCMI requests are effectively relayed to userspace so
that's probably the reason timings are similar. (despite the hackish
internals of latter solution)

Not sure if all the above madness helped you at all :D

Thanks,
Cristian


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