| 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: <20220216144719.GD6072@e120937-lin>
Date: Wed, 16 Feb 2022 14:47:19 +0000
From: Cristian Marussi <cristian.marussi@....com>
To: Peter Hilber <peter.hilber@...nsynergy.com>
Cc: linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
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 Wed, Feb 16, 2022 at 11:15:40AM +0100, Peter Hilber wrote:
> 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>
> > ---
Hi,
> > 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).
>
Yes I was unsure indeed. I'll drop it.
Thanks,
Cristian
Powered by blists - more mailing lists