linux-kernel - Re: [PATCH v7 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]

Date:   Tue, 18 Jan 2022 15:20:42 +0100
From:   Peter Hilber <peter.hilber@...nsynergy.com>
To:     Cristian Marussi <cristian.marussi@....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, "Michael S. Tsirkin" <mst@...hat.com>,
        Igor Skalkin <igor.skalkin@...nsynergy.com>,
        virtualization@...ts.linux-foundation.org
Subject: Re: [PATCH v7 14/16] firmware: arm_scmi: Add atomic mode support to
 virtio transport

On 20.12.21 22:30, Cristian Marussi wrote:
> On Fri, Dec 10, 2021 at 01:12:18PM +0100, Peter Hilber wrote:
>> On 29.11.21 20:11, Cristian Marussi wrote:
<snip>
>>> @@ -65,12 +69,22 @@ struct scmi_vio_channel {
>>>   * @input: SDU used for (delayed) responses and notifications
>>>   * @list: List which scmi_vio_msg may be part of
>>>   * @rx_len: Input SDU size in bytes, once input has been received
>>> + * @poll_idx: Last used index registered for polling purposes if this message
>>> + *	      transaction reply was configured for polling.
>>> + *	      Note that virtqueue used index is an unsigned 16-bit.
>>> + * @poll_lock: Protect access to @poll_idx.
>>>   */
>>>  struct scmi_vio_msg {
>>>  	struct scmi_msg_payld *request;
>>>  	struct scmi_msg_payld *input;
>>>  	struct list_head list;
>>>  	unsigned int rx_len;
>>> +#ifdef CONFIG_ARM_SCMI_TRANSPORT_VIRTIO_ATOMIC_ENABLE
>>> +#define VIO_MSG_POLL_DONE	0xffffffffUL
>>
>> virtqueue_enable_cb_prepare() returns an "opaque unsigned value", so
>> this special value should not be used for .poll_idx.
>>
> 
> Yes you are right, but looking at comments in virtqueue_enable_cb_prepare()
> and virtqueue_poll()
> 
> /**
>  * virtqueue_enable_cb_prepare - restart callbacks after disable_cb
>  *
>  * This re-enables callbacks; it returns current queue state
>  * in an opaque unsigned value. This value should be later tested by
>  * virtqueue_poll, to detect a possible race between the driver
>  * checking for more work, and enabling callbacks.
> 
> 
> /**
>  * virtqueue_poll - query pending used buffers
>  * @_vq: the struct virtqueue we're talking about.
>  * @last_used_idx: virtqueue state (from call to virtqueue_enable_cb_prepare).
> 
> ... it seems to me that is exactly how I'm using it, and moreover I
> don't see any other way via the VirtIO API to grab that last_used_idx
> that I need for virtqueu_poll.
> 

I meant to say that the VIO_MSG_POLL_DONE special value should best not be put
into the .poll_idx (since the special value might in theory overlap with an
opaque value). Another variable could hold the special states VIO_MSG_POLL_DONE
and VIO_MSG_NOT_POLLED.