Re: [PATCH v7 07/16] ALSA: virtio: stage VZ capture and restart paused streams

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] ALSA: virtio: DMA cacheline sharing in virtsnd_pcm_capture_send()
- [Low] ALSA: virtio: 64-bit division and modulo on u64 capture counters break 32-bit builds
--

commit b45632282ebc50eaee6059eb5173344a95c32ede
Author: Joonho Lee <mynameisjune111@gmail.com>

ALSA: virtio: stage VZ capture and restart paused streams

This patch adds opt-in capture staging using request-owned buffers with
response validation and ring accounting. It also introduces a pause
release and reprepare state machine for staged capture streams.

> diff --git a/sound/virtio/virtio_pcm.c b/sound/virtio/virtio_pcm.c
> index be3893de40a55..a3478fc8848db 100644
> --- a/sound/virtio/virtio_pcm.c
> +++ b/sound/virtio/virtio_pcm.c
> @@ -310,8 +310,94 @@ static void virtsnd_pcm_period_elapsed(struct work_struct *work)
>  {

[ ... ]

> +		period_bytes = snd_pcm_lib_period_bytes(substream);
> +		if (vss->xfer_xrun) {
> +			notify = true;
> +		} else if (!period_bytes || period_bytes >= vss->buffer_bytes ||
> +			   !virtsnd_pcm_capture_counters_valid(vss) ||
> +			   vss->capture_notified_periods >
> +				vss->capture_copied_total / period_bytes) {
> +			vss->xfer_xrun = true;
> +			notify = true;
> +		} else {
> +			period_floor = vss->capture_copied_total / period_bytes;
> +			if (period_floor > vss->capture_notified_periods) {
> +				/* Reserve crossed boundaries before the callback. */
> +				vss->capture_notified_periods = period_floor;
> +				notify = true;
> +			}
> +		}

[ ... ]

> diff --git a/sound/virtio/virtio_pcm.h b/sound/virtio/virtio_pcm.h
> index 61faf7b13cbc1..e143a15592f05 100644
> --- a/sound/virtio/virtio_pcm.h
> +++ b/sound/virtio/virtio_pcm.h

[ ... ]

> +static inline bool
> +virtsnd_pcm_capture_counters_valid(const struct virtio_pcm_substream *vss)
> +{
> +	return vss->buffer_bytes &&
> +	       vss->capture_copied_total >= vss->capture_acked_total &&
> +	       vss->capture_copied_total - vss->capture_acked_total ==
> +			(u64)vss->capture_ready &&
> +	       vss->capture_ready < vss->buffer_bytes &&
> +	       vss->capture_pointer_seen_total <= vss->capture_copied_total &&
> +	       vss->hw_ptr == vss->capture_copied_total % vss->buffer_bytes &&
> +	       (!vss->capture_callback_snapshot_active ||
> +		(vss->capture_callback_snapshot >=
> +			vss->capture_pointer_seen_total &&
> +		 vss->capture_callback_snapshot <=
> +			vss->capture_copied_total));
> +}

[Severity: Low]
Will these raw 64-bit divisions and modulo operations break 32-bit builds
with undefined references to __udivdi3 and __umoddi3?

Here in virtsnd_pcm_capture_counters_valid() (sound/virtio/virtio_pcm.h)
and virtsnd_pcm_period_elapsed() (sound/virtio/virtio_pcm.c), as well as in
virtsnd_pcm_capture_complete() (sound/virtio/virtio_pcm_msg.c):

	else if (vss->hw_ptr !=
		 vss->capture_copied_total % vss->buffer_bytes)
	...
	size_t ring_ptr = vss->capture_copied_total %
			  vss->buffer_bytes;
	...
	vss->hw_ptr = next_copied_total % vss->buffer_bytes;

and in virtsnd_pcm_cp_pointer() (sound/virtio/virtio_pcm_ops.c):

	hw_ptr = total % vss->buffer_bytes;

the u64 counters (capture_copied_total, next_copied_total, and total) are
divided by or taken modulo a size_t value (vss->buffer_bytes or
period_bytes).

On 32-bit architectures, usual arithmetic conversions promote the size_t
operand to u64, causing the compiler to emit __udivdi3 and __umoddi3 calls.
Should these use helpers from linux/math64.h such as div64_ul() or
div_u64_rem() instead?

> diff --git a/sound/virtio/virtio_pcm_msg.c b/sound/virtio/virtio_pcm_msg.c
> index b54642a9521db..795e52e09564b 100644
> --- a/sound/virtio/virtio_pcm_msg.c
> +++ b/sound/virtio/virtio_pcm_msg.c
> @@ -16,6 +17,7 @@
>   * @xfer: Request header payload.
>   * @status: Response header payload.
>   * @length: Data length in bytes.
> + * @data: Request-owned payload for staged capture.
>   * @data_offset: Offset in the driver-owned PCM buffer on NOMMU systems.
>   * @sgs: Payload scatter-gather table.
>   */
> @@ -24,6 +26,7 @@ struct virtio_pcm_msg {
>  	struct virtio_snd_pcm_xfer xfer;
>  	struct virtio_snd_pcm_status status;
>  	size_t length;
> +	u8 *data;
>  #ifndef CONFIG_MMU
>  	size_t data_offset;
>  #endif

[ ... ]

> +	for (i = 0; i < vss->nmsgs && vss->msg_count < request_limit; ++i) {
> +		struct virtio_pcm_msg *msg = vss->msgs[i];
> +		struct scatterlist *sgs[] = {
> +			&msg->sgs[PCM_MSG_SG_XFER],
> +			&msg->sgs[PCM_MSG_SG_DATA],
> +			&msg->sgs[PCM_MSG_SG_STATUS]
> +		};
> +		int rc;
> +
> +		if (msg->length)
> +			continue;
> +
> +		msg->xfer.stream_id = cpu_to_le32(vss->sid);
> +		/* Do not let an absent packed trailer reuse old PCM bytes. */
> +		memset(msg->data, 0, period_bytes);
> +		memset(&msg->status, 0, sizeof(msg->status));
> +		msg->status.status = cpu_to_le32(VIRTIO_SND_S_BAD_MSG);
> +		rc = virtqueue_add_sgs(vqueue, sgs, 1, 2, msg, GFP_ATOMIC);
> +		if (rc) {
> +			if (added && !(vss->features &
> +				       (1U << VIRTIO_SND_PCM_F_MSG_POLLING)) &&
> +			    virtqueue_kick_prepare(vqueue))
> +				virtqueue_notify(vqueue);
> +			return rc;
> +		}
> +
> +		msg->length = period_bytes;
> +		vss->msg_count++;
> +		added = true;
> +	}

[Severity: Medium]
Can touching msg->length after virtqueue_add_sgs() in
virtsnd_pcm_capture_send() corrupt DMA transfers on non-coherent platforms?

In struct virtio_pcm_msg, status, length, and data are adjacent fields in
the same slab allocation and share a CPU cacheline.

When virtsnd_pcm_capture_send() calls virtqueue_add_sgs(), msg->status is
mapped for device-to-driver DMA. Immediately after virtqueue_add_sgs()
returns, virtsnd_pcm_capture_send() writes msg->length = period_bytes, and
subsequent calls to virtsnd_pcm_capture_send() read msg->length while the
descriptor is still queued.

On non-coherent DMA systems where virtio uses the DMA API, dirtying the
cacheline containing msg->length after msg->status is mapped can cause a
later cacheline writeback to overwrite the device's DMA write to
msg->status, or cache invalidation during DMA unmap in virtqueue_get_buf()
can discard the write to msg->length so virtsnd_pcm_capture_complete() sees
msg->length == 0 and triggers a false xfer_xrun.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261028094132.20364-1-mynameisjune111@gmail.com?part=7