linux-kernel - Re: [PATCH v6 14/16] firmware: arm_scmi: Add atomic mode support to virtio transport

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20211123121136.GD56473@e120937-lin>
Date:   Tue, 23 Nov 2021 12:11:36 +0000
From:   Cristian Marussi <cristian.marussi@....com>
To:     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,
        vincent.guittot@...aro.org, souvik.chakravarty@....com,
        "Michael S. Tsirkin" <mst@...hat.com>,
        Igor Skalkin <igor.skalkin@...nsynergy.com>,
        Peter Hilber <peter.hilber@...nsynergy.com>,
        virtualization@...ts.linux-foundation.org
Subject: Re: [PATCH v6 14/16] firmware: arm_scmi: Add atomic mode support to
 virtio transport

On Mon, Nov 22, 2021 at 11:06:38PM +0000, Cristian Marussi wrote:
> Add support for .mark_txdone and .poll_done transport operations to SCMI
> VirtIO transport as pre-requisites to enable atomic operations.
> 
> Add a Kernel configuration option to enable SCMI VirtIO transport polling
> and atomic mode for selected SCMI transactions while leaving it default
> disabled.
> 
> 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,

> ---
>  drivers/firmware/arm_scmi/Kconfig  |  15 ++
>  drivers/firmware/arm_scmi/virtio.c | 212 +++++++++++++++++++++++++++--
>  2 files changed, 214 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/firmware/arm_scmi/Kconfig b/drivers/firmware/arm_scmi/Kconfig
> index d429326433d1..7794bd41eaa0 100644
> --- a/drivers/firmware/arm_scmi/Kconfig
> +++ b/drivers/firmware/arm_scmi/Kconfig
> @@ -118,6 +118,21 @@ config ARM_SCMI_TRANSPORT_VIRTIO_VERSION1_COMPLIANCE
>  	  the ones implemented by kvmtool) and let the core Kernel VirtIO layer
>  	  take care of the needed conversions, say N.
>  
> +config ARM_SCMI_TRANSPORT_VIRTIO_ATOMIC_ENABLE
> +	bool "Enable atomic mode for SCMI VirtIO transport"
> +	depends on ARM_SCMI_TRANSPORT_VIRTIO
> +	help
> +	  Enable support of atomic operation for SCMI VirtIO based transport.
> +
> +	  If you want the SCMI VirtIO based transport to operate in atomic
> +	  mode, avoiding any kind of sleeping behaviour for selected
> +	  transactions on the TX path, answer Y.
> +
> +	  Enabling atomic mode operations allows any SCMI driver using this
> +	  transport to optionally ask for atomic SCMI transactions and operate
> +	  in atomic context too, at the price of using a number of busy-waiting
> +	  primitives all over instead. If unsure say N.
> +
>  endif #ARM_SCMI_PROTOCOL
>  

[snip]

>  static void scmi_vio_complete_cb(struct virtqueue *vqueue)
> @@ -138,12 +157,29 @@ static void scmi_vio_complete_cb(struct virtqueue *vqueue)
>  			goto unlock_ready_out;
>  		}
>  
> +#ifdef CONFIG_ARM_SCMI_TRANSPORT_VIRTIO_ATOMIC_ENABLE
> +		/* At first loop on pending_cmd_list of buffered msg if any */
> +		if (!vioch->is_rx) {
> +			struct scmi_vio_msg *tmp;
> +
> +			list_for_each_entry_safe(msg, tmp,
> +						 &vioch->pending_cmds_list,
> +						 list) {
> +				scmi_rx_callback(vioch->cinfo,
> +						 msg_read_header(msg->input),
> +						 msg);
> +				scmi_vio_feed_vq_tx(vioch, msg);
> +			}
> +		}
> +#endif
> +

doing further testing I spotted a trivial bug on the limit condition
where polling and non-polling requests are interleaved on the cmd
vqueue. (hard to reproduce ... but it's there and it's broken)

This one-liner fixes the issue with the above snippet:

diff --git a/drivers/firmware/arm_scmi/virtio.c b/drivers/firmware/arm_scmi/virtio.c
index 3101024751b0..cf247093ef48 100644
--- a/drivers/firmware/arm_scmi/virtio.c
+++ b/drivers/firmware/arm_scmi/virtio.c
@@ -168,6 +168,7 @@ static void scmi_vio_complete_cb(struct virtqueue *vqueue)
                                scmi_rx_callback(vioch->cinfo,
                                                 msg_read_header(msg->input),
                                                 msg);
+                               list_del(&msg->list);
                                scmi_vio_feed_vq_tx(vioch, msg);
                        }
                }

It seems there will be a V7 ...

Thanks,
Cristian