Re: [PATCH v9 00/12] Add VIRTIO sound card

2023-09-25 Thread Matias Ezequiel Vara Larsen 
On Thu, Sep 14, 2023 at 12:24:13PM +0200, Stefano Garzarella wrote:
> On Thu, Sep 14, 2023 at 01:02:05PM +0300, Manos Pitsidianakis wrote:
> > On Thu, 14 Sep 2023 12:54, Stefano Garzarella  wrote:
> > > We are seeing something strange with the virtio-sound Linux driver.
> > > It seems that the driver modifies the buffers after exposing them to
> > > the device via the avail ring.
> > 
> > I need more information about this bug. What is the unexpected behavior
> > that made you find that the buffer was modified in the first place?
> 
> CCing Matias for more details, but initially can you just run the test
> Matias suggested to check if you experience the same behaviour or not?
> 
Hello, 
we are discussing this issue in the virtio-comment mailing list:
https://lists.oasis-open.org/archives/virtio-comment/202309/msg00175.html

Thanks, Matias.

Re: [PATCH v9 00/12] Add VIRTIO sound card

2023-09-14 Thread Stefano Garzarella 

On Thu, Sep 14, 2023 at 01:02:05PM +0300, Manos Pitsidianakis wrote:

On Thu, 14 Sep 2023 12:54, Stefano Garzarella  wrote:

We are seeing something strange with the virtio-sound Linux driver.
It seems that the driver modifies the buffers after exposing them to
the device via the avail ring.


I need more information about this bug. What is the unexpected 
behavior that made you find that the buffer was modified in the first 
place?


CCing Matias for more details, but initially can you just run the test
Matias suggested to check if you experience the same behaviour or not?

Stefano

Re: [PATCH v9 00/12] Add VIRTIO sound card

2023-09-14 Thread Manos Pitsidianakis 

On Thu, 14 Sep 2023 12:54, Stefano Garzarella  wrote:

We are seeing something strange with the virtio-sound Linux driver.
It seems that the driver modifies the buffers after exposing them to
the device via the avail ring.


I need more information about this bug. What is the unexpected behavior 
that made you find that the buffer was modified in the first place?


Manos

Re: [PATCH v9 00/12] Add VIRTIO sound card

2023-09-14 Thread Stefano Garzarella 

On Wed, Sep 13, 2023 at 10:33:07AM +0300, Emmanouil Pitsidianakis wrote:

This patch series adds an audio device implementing the recent virtio
sound spec (1.2) and a corresponding PCI wrapper device.

v9 can be found online at:

https://gitlab.com/epilys/qemu/-/tree/virtio-snd-v9

Ref 06e6b17186

Main differences with v8 patch series [^v8]
:

- Addressed [^v8] review comments.
- Add cpu_to_le32(_) and le32_to_cpu(_) conversions for messages from/to
 the guest according to the virtio spec.
- Inlined some functions and types to reduce review complexity.
- Corrected the replies to IO messages; now both Playback and Capture
 work correctly for me. (If you hear cracks in pulseaudio+guest, try
 pipewire+guest).


We are seeing something strange with the virtio-sound Linux driver.
It seems that the driver modifies the buffers after exposing them to
the device via the avail ring.

It seems we have this strange behaviour with this built-in QEMU device,
but also with the vhost-device-sound, so it could be some spec
violation in the Linux driver.

Matias also reported on the v8 of this series:
https://lore.kernel.org/qemu-devel/ZPg60lzXWxHPQJEa@fedora/

Can you check if you have the same behaviour?

Nothing that blocks this series of course, but just to confirm that
there may be something to fix in the Linux driver.

Thanks,
Stefano

[PATCH v9 00/12] Add VIRTIO sound card

2023-09-13 Thread Emmanouil Pitsidianakis 
This patch series adds an audio device implementing the recent virtio 
sound spec (1.2) and a corresponding PCI wrapper device.

v9 can be found online at:

https://gitlab.com/epilys/qemu/-/tree/virtio-snd-v9

Ref 06e6b17186

Main differences with v8 patch series [^v8]
:

- Addressed [^v8] review comments.
- Add cpu_to_le32(_) and le32_to_cpu(_) conversions for messages from/to 
  the guest according to the virtio spec.
- Inlined some functions and types to reduce review complexity.
- Corrected the replies to IO messages; now both Playback and Capture 
  work correctly for me. (If you hear cracks in pulseaudio+guest, try 
  pipewire+guest).

Previously:

[^v8]: 
https://lore.kernel.org/qemu-devel/cover.1693252037.git.manos.pitsidiana...@linaro.org/
[^v7]: 
https://lore.kernel.org/qemu-devel/cover.1692731646.git.manos.pitsidiana...@linaro.org/
[^v6]: 
https://lore.kernel.org/qemu-devel/cover.1692089917.git.manos.pitsidiana...@linaro.org/
[^v5]: 
https://lore.kernel.org/qemu-devel/cover.1690626150.git.manos.pitsidiana...@linaro.org/
[^v4]: 
https://lore.kernel.org/qemu-devel/cover.1689857559.git.manos.pitsidiana...@linaro.org/
[^v3]: 
https://lore.kernel.org/qemu-devel/cover.1689692765.git.manos.pitsidiana...@linaro.org/

Emmanouil Pitsidianakis (12):
  Add virtio-sound device stub
  Add virtio-sound-pci device
  virtio-sound: handle control messages and streams
  virtio-sound: set PCM stream parameters
  virtio-sound: handle VIRTIO_SND_R_PCM_INFO request
  virtio-sound: handle VIRTIO_SND_R_PCM_{START,STOP}
  virtio-sound: handle VIRTIO_SND_R_PCM_SET_PARAMS
  virtio-sound: handle VIRTIO_SND_R_PCM_PREPARE
  virtio-sound: handle VIRTIO_SND_R_PCM_RELEASE
  virtio-sound: implement audio output (TX)
  virtio-sound: implement audio capture (RX)
  docs/system: add basic virtio-snd documentation

 MAINTAINERS|6 +
 docs/system/device-emulation.rst   |1 +
 docs/system/devices/virtio-snd.rst |   49 +
 hw/virtio/Kconfig  |5 +
 hw/virtio/meson.build  |2 +
 hw/virtio/trace-events |   20 +
 hw/virtio/virtio-snd-pci.c |   93 ++
 hw/virtio/virtio-snd.c | 1339 
 include/hw/virtio/virtio-snd.h |  193 
 softmmu/qdev-monitor.c |1 +
 10 files changed, 1709 insertions(+)
 create mode 100644 docs/system/devices/virtio-snd.rst
 create mode 100644 hw/virtio/virtio-snd-pci.c
 create mode 100644 hw/virtio/virtio-snd.c
 create mode 100644 include/hw/virtio/virtio-snd.h

Range-diff against v8:
 1:  238de1757e !  1:  5173e2c243 Add virtio-sound device stub
@@ hw/virtio/virtio-snd.c (new)
 +#include "trace.h"
 +#include "qapi/error.h"
 +#include "hw/virtio/virtio-snd.h"
++#include "hw/core/cpu.h"
 +
 +#define VIRTIO_SOUND_VM_VERSION 1
 +#define VIRTIO_SOUND_JACK_DEFAULT 0
@@ hw/virtio/virtio-snd.c (new)
 +};
 +
 +static Property virtio_snd_properties[] = {
++DEFINE_AUDIO_PROPERTIES(VirtIOSound, card),
 +DEFINE_PROP_UINT32("jacks", VirtIOSound, snd_conf.jacks,
 +   VIRTIO_SOUND_JACK_DEFAULT),
 +DEFINE_PROP_UINT32("streams", VirtIOSound, snd_conf.streams,
@@ hw/virtio/virtio-snd.c (new)
 +VirtIOSound *vsnd = VIRTIO_SND(dev);
 +
 +qemu_del_vm_change_state_handler(vsnd->vmstate);
-+virtio_del_queue(vdev, 0);
-+
 +trace_virtio_snd_unrealize(vsnd);
 +
 +AUD_remove_card(>card);
++virtio_delete_queue(vsnd->queues[VIRTIO_SND_VQ_CONTROL]);
++virtio_delete_queue(vsnd->queues[VIRTIO_SND_VQ_EVENT]);
++virtio_delete_queue(vsnd->queues[VIRTIO_SND_VQ_TX]);
++virtio_delete_queue(vsnd->queues[VIRTIO_SND_VQ_RX]);
 +virtio_cleanup(vdev);
 +}
 +
@@ include/hw/virtio/virtio-snd.h (new)
 +
 +typedef struct VirtIOSound {
 +VirtIODevice parent_obj;
++
 +VirtQueue *queues[VIRTIO_SND_VQ_MAX];
 +uint64_t features;
 +QEMUSoundCard card;
 2:  8de966a86b !  2:  d2fdd5852d Add virtio-sound-pci device
@@ hw/virtio/virtio-snd-pci.c (new)
 + */
 +
 +#include "qemu/osdep.h"
++#include "qom/object.h"
 +#include "qapi/error.h"
 +#include "hw/audio/soundhw.h"
 +#include "hw/virtio/virtio-pci.h"
 +#include "hw/virtio/virtio-snd.h"
 +
-+typedef struct VirtIOSoundPCI VirtIOSoundPCI;
-+
 +/*
 + * virtio-snd-pci: This extends VirtioPCIProxy.
 + */
 +#define TYPE_VIRTIO_SND_PCI "virtio-sound-pci"
-+DECLARE_INSTANCE_CHECKER(VirtIOSoundPCI, VIRTIO_SND_PCI,
-+ TYPE_VIRTIO_SND_PCI)
++OBJECT_DECLARE_SIMPLE_TYPE(VirtIOSoundPCI, VIRTIO_SND_PCI)
 +
 +struct VirtIOSoundPCI {
-+VirtIOPCIProxy parent;
++VirtIOPCIProxy parent_obj;
++
 +VirtIOSound vdev;
 +};
 +
 +static Property virtio_snd_pci_properties[] = {
-+