Add vhost-user-sound crate

[epilys]

Summary of the PR

This PR adds a new crate, vhost-user-sound, that provides a vhost-user backend daemon for the VIRTIO Sound device as specified in the VIRTIO Spec v.1.2 ¹

Supported features are:

• One input stream, one output stream (hard-coded).
• Playback with ALSA backend (on host machine).
  Buffer/period settings need some tuning, the first period seems to come up short; I'm not familiar with ALSA programming at all.
• Playback with a "null" backend that just consumes audio without actual playback.

What remains to be done:

• There are various places where micro-refactors make sense
• Audio capture
• Add more audio backends.
• Add logic for setting up the default sound device stream information on startup from the audio backend, instead of hardcoding one input stream and one output stream.

Usage / Testing functionality

This PR was tested with QEMU, with a proposed patchset that enables generic vhost-user devices in virtualized guests ², contributed by @stsquad.

The following command line invocations were used:

Run the vhost-device daemon

cargo run -- --socket /tmp/snd.sock --backend alsa

Run a guest in QEMU

1. Apply the patchset ².

2. Apply these patches to create a vhost-user-snd and vhost-user-snd-pci device ³.

   • [PATCH] Add virtio-sound and virtio-sound-pci devices
   • [PATCH] Add vhost-user-snd and vhost-user-snd-pci devices

3. Build for desired target architecture, e.g. x86_64.

4. Get/build kernel configured with CONFIG_SND_VIRTIO.

5. Launch QEMU with

   $ export VHOST_USER_SOCK=/tmp/snd.sock
   $ /path/to/qemu-system-x86_64  \
       [... snip ...] \
     -chardev socket,id=vsnd,path="$VHOST_USER_SOCK" \
     -device vhost-user-snd-pci,chardev=vsnd,id=snd \
     [... snip ...]

6. Use speaker-test -l 1 to play some pink noise for one loop (-l N) or speaker-test -t wav -w /path/to/file.wav -l 1.

Requirements

• All commits in this PR are signed (with git commit -s), and the commit
  message has max 60 characters for the summary and max 75 characters for each
  description line.
• All added/changed functionality has a corresponding unit/integration
  test.
• All added/changed public-facing functionality has entries in the "Upcoming
  Release" section of CHANGELOG.md (if no such section exists, please create one).
• Any newly added unsafe code is properly documented.

Footnotes

1. Virtual I/O Device (VIRTIO) Version 1.2 Committee Specification Draft 01 09 May 2022 ↩

2. https://patchew.org/QEMU/20230418162140.373219-1-alex.bennee@linaro.org/ ↩ ↩²

3. https://github.com/epilys/qemu-virtio-snd/commits/54ae1cdd15fef2d88e9e387a175f099a38c636f4 ↩

[epilys]

@stefano-garzarella This is the PR as it was rebased against current upstream so it shows unrelated commits (I have to re-rebase it against this repo)

Should I make a new PR with only the alsa backend? I see there is code and open PRs handling TX and streams already so the alsa backend is the biggest difference.

[stefano-garzarella]

@stefano-garzarella This is the PR as it was rebased against current upstream so it shows unrelated commits (I have to re-rebase it against this repo)

Yep, would be great if you can rebase on top of virtio-sound branch.
Anyway, I also did a quick review and left some comments.

Should I make a new PR with only the alsa backend? I see there is code and open PRs handling TX and streams already so the alsa backend is the biggest difference.

@MatiasVara @dorindabassey WDYT?

[stefano-garzarella]

Are we sure that we always have this layout?

I mean: descriptor[1] header and descriptor[2] payload.

Or is it an implementation detail of the linux driver?

I ask because in virtio-vsock we had a similar problem: the specification doesn't define these details and the driver initially used 2 separate descriptors for header and payload, then it started to use only one descriptor and vhost-user-vsock didn't work.

That's fine for now, but I would put a comment if that's the case.

[epilys]

I think so, at least from my understanding of the spec wording (I haven't worked with virtqueues directly before, caveat emptor)

A generic control message consists of a request part and a response part.

A request part has, or consists of, a common header containing the following device-readable field:

code
    specifies a device request type (VIRTIO_SND_R_*).

A response part has, or consists of, a common header containing the following device-writable field:

code
    indicates a device request status (VIRTIO_SND_S_*).

The status field can take one of the following values:

    VIRTIO_SND_S_OK - success.
    VIRTIO_SND_S_BAD_MSG - a control message is malformed or contains invalid parameters.
    VIRTIO_SND_S_NOT_SUPP - requested operation or parameters are not supported.
    VIRTIO_SND_S_IO_ERR - an I/O error occurred.

The request part may be followed by an additional device-readable payload, and the response part may be followed by an additional device-writable payload. 

So in this case the generic ctrl header is followed by the additional device-readable payload, and then the response comes after.

[stefano-garzarella]

Okay, so even in this case it's an implementation detail of the driver. Nothing prevents it from using a single writable descriptor to contain both the header and the response payload, right?

[Ablu]

Almost all rust-vmm daemons get this wrong in one way or another... We would really need something like rust-vmm/vm-virtio#33 to prevent daemons from needing to worry about this stuff...

[stefano-garzarella]

Yep, I came to the same conclusion.

So that's okay in my opinion for now, but we should put comments where we make this assumption to say that's how the driver works in Linux 6.4.

WDYT?

[stefano-garzarella]

@epilys header and payload could be on a single buffer, though, couldn't they?

The important thing is not to mix writable and readable buffers, but then for virtqueue it can contain anything as long as it's contiguous memory. So nothing should prevent the driver from using a single contiguous memory area and expecting the device to write both the header and the response payload there.

[epilys]

are they considered the same element? (response header + payload)

[Ablu]

Yes, I think what you quoted above talks about an "element" in the virtqueue. So the element there is a descriptor chain. The chain then can have any form (as long as r/w constraints are satisfied) and consist of multiple descriptors.

[stefano-garzarella]

an element for the virtqueue are just contiguous bytes, then as each device uses them, they are a higher level of abstraction

[epilys]

I see, thanks!

[epilys]

Didn't rebase yet against virtio-sound because it has an error: the AudioBackend trait uses a struct defined in the pipewire backend which is behind a cfg compile-time flag.

[dorindabassey]

Didn't rebase yet against virtio-sound because it has an error: the AudioBackend trait uses a struct defined in the pipewire backend which is behind a cfg compile-time flag.

is this the struct you are referring to?

vhost-device/crates/sound/src/audio_backends/pw_backend.rs

Line 14 in 3a0f49d

pub struct PCMParams {

I think the next PR fixes that, so once we merge it, I think you should be able to rebase on top of it.

[dorindabassey]

I wonder if it make sense to you to split the PR into maybe 3 or 4, it depends - (documentation, "changes for ctrl queue, tx and rx queue that you think is nice to have", and the alsa-Backend.) IMHO that would simplify the review and merge process.

[epilys]

@dorindabassey you can view each commit separately, there are standalone commits for CTRL queue, TX queue and the alsa backend. Click on the files changed tab and select "view changes for X/Y/Z commit"

[dorindabassey]

Didn't rebase yet against virtio-sound because it has an error: the AudioBackend trait uses a struct defined in the pipewire backend which is behind a cfg compile-time flag.

Hi Manos, Everything is okay now, I think you can rebase your PR on top of our current work. Just a small note - please take care with the rebasing so that the already existing changes do not get lost. thank you :)