| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <e63e2331-c30c-7151-9348-7f01850ff82e@opensynergy.com>
Date: Wed, 16 Feb 2022 11:15:40 +0100
From: Peter Hilber <peter.hilber@...nsynergy.com>
To: Cristian Marussi <cristian.marussi@....com>,
linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org
Cc: sudeep.holla@....com, james.quinlan@...adcom.com,
Jonathan.Cameron@...wei.com, f.fainelli@...il.com,
etienne.carriere@...aro.org, vincent.guittot@...aro.org,
souvik.chakravarty@....com, igor.skalkin@...nsynergy.com,
"Michael S. Tsirkin" <mst@...hat.com>,
virtualization@...ts.linux-foundation.org
Subject: Re: [PATCH v4 1/8] firmware: arm_scmi: Add a virtio channel refcount
On 13.02.22 20:58, Cristian Marussi wrote:
> Currently SCMI VirtIO channels are marked with a ready flag and related
> lock to track channel lifetime and support proper synchronization at
> shutdown when virtqueues have to be stopped.
>
> This leads to some extended spinlocked sections with IRQs off on the RX
> path to keep hold of the ready flag and does not scale well especially when
> SCMI VirtIO polling mode will be introduced.
>
> Add an SCMI VirtIO channel dedicated refcount to track active users on both
> the TX and the RX path and properly enforce synchronization and cleanup at
> shutdown, inhibiting further usage of the channel once freed.
>
> Cc: "Michael S. Tsirkin" <mst@...hat.com>
> Cc: Igor Skalkin <igor.skalkin@...nsynergy.com>
> Cc: Peter Hilber <peter.hilber@...nsynergy.com>
> Cc: virtualization@...ts.linux-foundation.org
> Signed-off-by: Cristian Marussi <cristian.marussi@....com>
> ---
> v2 --> v3
> - Break virtio device at shutdown while cleaning up SCMI channel
> ---
> drivers/firmware/arm_scmi/virtio.c | 140 +++++++++++++++++++----------
> 1 file changed, 92 insertions(+), 48 deletions(-)
>
> diff --git a/drivers/firmware/arm_scmi/virtio.c b/drivers/firmware/arm_scmi/virtio.c
> index fd0f6f91fc0b..112d6bd4be2e 100644
> --- a/drivers/firmware/arm_scmi/virtio.c
> +++ b/drivers/firmware/arm_scmi/virtio.c
> @@ -17,7 +17,9 @@
> * virtqueue. Access to each virtqueue is protected by spinlocks.
> */
>
> +#include <linux/completion.h>
> #include <linux/errno.h>
> +#include <linux/refcount.h>
> #include <linux/slab.h>
> #include <linux/virtio.h>
> #include <linux/virtio_config.h>
> @@ -27,6 +29,7 @@
>
> #include "common.h"
>
> +#define VIRTIO_MAX_RX_TIMEOUT_MS 60000
> #define VIRTIO_SCMI_MAX_MSG_SIZE 128 /* Value may be increased. */
> #define VIRTIO_SCMI_MAX_PDU_SIZE \
> (VIRTIO_SCMI_MAX_MSG_SIZE + SCMI_MSG_MAX_PROT_OVERHEAD)
> @@ -39,23 +42,21 @@
> * @cinfo: SCMI Tx or Rx channel
> * @free_list: List of unused scmi_vio_msg, maintained for Tx channels only
> * @is_rx: Whether channel is an Rx channel
> - * @ready: Whether transport user is ready to hear about channel
> * @max_msg: Maximum number of pending messages for this channel.
> - * @lock: Protects access to all members except ready.
> - * @ready_lock: Protects access to ready. If required, it must be taken before
> - * lock.
> + * @lock: Protects access to all members except users.
> + * @shutdown_done: A reference to a completion used when freeing this channel.
> + * @users: A reference count to currently active users of this channel.
> */
> struct scmi_vio_channel {
> struct virtqueue *vqueue;
> struct scmi_chan_info *cinfo;
> struct list_head free_list;
> bool is_rx;
> - bool ready;
> unsigned int max_msg;
> - /* lock to protect access to all members except ready. */
> + /* lock to protect access to all members except users. */
> spinlock_t lock;
> - /* lock to rotects access to ready flag. */
> - spinlock_t ready_lock;
> + struct completion *shutdown_done;
> + refcount_t users;
> };
>
> /**
> @@ -76,6 +77,63 @@ struct scmi_vio_msg {
> /* Only one SCMI VirtIO device can possibly exist */
> static struct virtio_device *scmi_vdev;
>
> +static void scmi_vio_channel_ready(struct scmi_vio_channel *vioch,
> + struct scmi_chan_info *cinfo)
> +{
> + unsigned long flags;
> +
> + spin_lock_irqsave(&vioch->lock, flags);
> + cinfo->transport_info = vioch;
> + /* Indirectly setting channel not available any more */
> + vioch->cinfo = cinfo;
> + spin_unlock_irqrestore(&vioch->lock, flags);
> +
> + refcount_set(&vioch->users, 1);
> +}
> +
> +static inline bool scmi_vio_channel_acquire(struct scmi_vio_channel *vioch)
> +{
> + return refcount_inc_not_zero(&vioch->users);
> +}
> +
> +static inline void scmi_vio_channel_release(struct scmi_vio_channel *vioch)
> +{
> + if (refcount_dec_and_test(&vioch->users)) {
> + unsigned long flags;
> +
> + spin_lock_irqsave(&vioch->lock, flags);
> + if (vioch->shutdown_done) {
> + vioch->cinfo = NULL;
> + complete(vioch->shutdown_done);
> + }
> + spin_unlock_irqrestore(&vioch->lock, flags);
> + }
> +}
> +
> +static void scmi_vio_channel_cleanup_sync(struct scmi_vio_channel *vioch)
> +{
> + unsigned long flags;
> + DECLARE_COMPLETION_ONSTACK(vioch_shutdown_done);
> +
> + /*
> + * Prepare to wait for the last release if not already released
> + * or in progress.
> + */
> + spin_lock_irqsave(&vioch->lock, flags);
> + if (!vioch->cinfo || vioch->shutdown_done) {
> + spin_unlock_irqrestore(&vioch->lock, flags);
> + return;
> + }
> + vioch->shutdown_done = &vioch_shutdown_done;
> + virtio_break_device(vioch->vqueue->vdev);
> + spin_unlock_irqrestore(&vioch->lock, flags);
> +
> + scmi_vio_channel_release(vioch);
> +
> + /* Let any possibly concurrent RX path release the channel */
> + wait_for_completion(vioch->shutdown_done);
> +}
> +
> static bool scmi_vio_have_vq_rx(struct virtio_device *vdev)
> {
> return virtio_has_feature(vdev, VIRTIO_SCMI_F_P2A_CHANNELS);
> @@ -119,7 +177,7 @@ static void scmi_finalize_message(struct scmi_vio_channel *vioch,
>
> static void scmi_vio_complete_cb(struct virtqueue *vqueue)
> {
> - unsigned long ready_flags;
> + unsigned long flags;
> unsigned int length;
> struct scmi_vio_channel *vioch;
> struct scmi_vio_msg *msg;
> @@ -130,27 +188,27 @@ static void scmi_vio_complete_cb(struct virtqueue *vqueue)
> vioch = &((struct scmi_vio_channel *)vqueue->vdev->priv)[vqueue->index];
>
> for (;;) {
> - spin_lock_irqsave(&vioch->ready_lock, ready_flags);
> -
> - if (!vioch->ready) {
> + if (!scmi_vio_channel_acquire(vioch)) {
> if (!cb_enabled)
> (void)virtqueue_enable_cb(vqueue);
This seems unneeded ATM (in particular since the virtqueue is now broken when
freeing the channel).
> - goto unlock_ready_out;
> + return;
> }
>
> - /* IRQs already disabled here no need to irqsave */
> - spin_lock(&vioch->lock);
> + spin_lock_irqsave(&vioch->lock, flags);
> if (cb_enabled) {
> virtqueue_disable_cb(vqueue);
> cb_enabled = false;
> }
> msg = virtqueue_get_buf(vqueue, &length);
> if (!msg) {
> - if (virtqueue_enable_cb(vqueue))
> - goto unlock_out;
> + if (virtqueue_enable_cb(vqueue)) {
> + spin_unlock_irqrestore(&vioch->lock, flags);
> + scmi_vio_channel_release(vioch);
> + return;
> + }
> cb_enabled = true;
> }
> - spin_unlock(&vioch->lock);
> + spin_unlock_irqrestore(&vioch->lock, flags);
>
> if (msg) {
> msg->rx_len = length;
> @@ -161,19 +219,14 @@ static void scmi_vio_complete_cb(struct virtqueue *vqueue)
> }
>
> /*
> - * Release ready_lock and re-enable IRQs between loop iterations
> - * to allow virtio_chan_free() to possibly kick in and set the
> - * flag vioch->ready to false even in between processing of
> - * messages, so as to force outstanding messages to be ignored
> - * when system is shutting down.
> + * Release vio channel between loop iterations to allow
> + * virtio_chan_free() to eventually fully release it when
> + * shutting down; in such a case, any outstanding message will
> + * be ignored since this loop will bail out at the next
> + * iteration.
> */
> - spin_unlock_irqrestore(&vioch->ready_lock, ready_flags);
> + scmi_vio_channel_release(vioch);
> }
> -
> -unlock_out:
> - spin_unlock(&vioch->lock);
> -unlock_ready_out:
> - spin_unlock_irqrestore(&vioch->ready_lock, ready_flags);
> }
>
> static const char *const scmi_vio_vqueue_names[] = { "tx", "rx" };
> @@ -273,35 +326,20 @@ static int virtio_chan_setup(struct scmi_chan_info *cinfo, struct device *dev,
> }
> }
>
> - spin_lock_irqsave(&vioch->lock, flags);
> - cinfo->transport_info = vioch;
> - /* Indirectly setting channel not available any more */
> - vioch->cinfo = cinfo;
> - spin_unlock_irqrestore(&vioch->lock, flags);
> -
> - spin_lock_irqsave(&vioch->ready_lock, flags);
> - vioch->ready = true;
> - spin_unlock_irqrestore(&vioch->ready_lock, flags);
> + scmi_vio_channel_ready(vioch, cinfo);
>
> return 0;
> }
>
> static int virtio_chan_free(int id, void *p, void *data)
> {
> - unsigned long flags;
> struct scmi_chan_info *cinfo = p;
> struct scmi_vio_channel *vioch = cinfo->transport_info;
>
> - spin_lock_irqsave(&vioch->ready_lock, flags);
> - vioch->ready = false;
> - spin_unlock_irqrestore(&vioch->ready_lock, flags);
> + scmi_vio_channel_cleanup_sync(vioch);
>
> scmi_free_channel(cinfo, data, id);
>
> - spin_lock_irqsave(&vioch->lock, flags);
> - vioch->cinfo = NULL;
> - spin_unlock_irqrestore(&vioch->lock, flags);
> -
> return 0;
> }
>
> @@ -316,10 +354,14 @@ static int virtio_send_message(struct scmi_chan_info *cinfo,
> int rc;
> struct scmi_vio_msg *msg;
>
> + if (!scmi_vio_channel_acquire(vioch))
> + return -EINVAL;
> +
> spin_lock_irqsave(&vioch->lock, flags);
>
> if (list_empty(&vioch->free_list)) {
> spin_unlock_irqrestore(&vioch->lock, flags);
> + scmi_vio_channel_release(vioch);
> return -EBUSY;
> }
>
> @@ -342,6 +384,8 @@ static int virtio_send_message(struct scmi_chan_info *cinfo,
>
> spin_unlock_irqrestore(&vioch->lock, flags);
>
> + scmi_vio_channel_release(vioch);
> +
> return rc;
> }
>
> @@ -416,7 +460,6 @@ static int scmi_vio_probe(struct virtio_device *vdev)
> unsigned int sz;
>
> spin_lock_init(&channels[i].lock);
> - spin_lock_init(&channels[i].ready_lock);
> INIT_LIST_HEAD(&channels[i].free_list);
> channels[i].vqueue = vqs[i];
>
> @@ -503,7 +546,8 @@ const struct scmi_desc scmi_virtio_desc = {
> .transport_init = virtio_scmi_init,
> .transport_exit = virtio_scmi_exit,
> .ops = &scmi_virtio_ops,
> - .max_rx_timeout_ms = 60000, /* for non-realtime virtio devices */
> + /* for non-realtime virtio devices */
> + .max_rx_timeout_ms = VIRTIO_MAX_RX_TIMEOUT_MS,
> .max_msg = 0, /* overridden by virtio_get_max_msg() */
> .max_msg_size = VIRTIO_SCMI_MAX_MSG_SIZE,
> };
Powered by blists - more mailing lists