Re: [PATCH v5 5/6] hw/virtio: add vhost-user-snd and virtio-snd-pci devices

[Manos Pitsidianakis]

On Fri, 20 Oct 2023 12:02, "Michael S. Tsirkin"  wrote:

On Fri, Oct 20, 2023 at 09:16:03AM +0100, Alex Bennée wrote:


Viresh Kumar  writes:

> On 19-10-23, 10:56, Alex Bennée wrote:
>> From: Manos Pitsidianakis 
>> 
>> Tested with rust-vmm vhost-user-sound daemon:
>> 
>> RUST_LOG=trace cargo run --bin vhost-user-sound -- --socket /tmp/snd.sock --backend null
>> 
>> Invocation:
>> 
>> qemu-system-x86_64  \

>> -qmp unix:./qmp-sock,server,wait=off  \
>> -m 4096 \
>> -numa node,memdev=mem \
>> -object 
memory-backend-file,id=mem,size=4G,mem-path=/dev/shm,share=on \
>> -D qemu.log \
>> -d guest_errors,trace:\*snd\*,trace:\*sound\*,trace:\*vhost\* \
>> -chardev socket,id=vsnd,path=/tmp/snd.sock \
>> -device vhost-user-snd-pci,chardev=vsnd,id=snd \
>> /path/to/disk
>> 
>> [AJB: imported from https://github.com/epilys/qemu-virtio-snd/commit/54ae1cdd15fef2d88e9e387a175f099a38c636f4.patch]
>> 
>> Signed-off-by: Alex Bennée 

>
> Missing SOB from Manos ?

oops, guess I need a respin then ;-)


Just ask Manos to send his S.O.B in a reply.


Signed-off-by: Manos Pitsidianakis 

Re: [PATCH v11 00/11] Add VIRTIO sound card

[Manos Pitsidianakis]

On Wed, 18 Oct 2023 17:50, "Michael S. Tsirkin"  wrote:
> > For context, the device was in hw/audio initially but in early 
> > list

> > discussions we decided to move it with the rest of virtio devices.
> 
> Link to these discussions? virtio-gpu is under ./hw/display

> seems inconsistent.

I agree that it is inconsistent, but I do not know if there is a general
consensus on this. I am fine with placing it anywhere that is deemed
appropriate.

<87a5xkde0c@linaro.org>
https://lore.kernel.org/qemu-devel/87a5xkde0c@linaro.org/

> And you are asking Gerd to merge but
> your MAINTAINERS patch wouldn't even let people know
> he needs to be Cc'd on changes.

No, I didn't ask to merge this, I asked if Gerd would be the one merging it
because I am not sure myself and wanted more information. What do you think
should be the correct process?


I'd put it under audio and then you get an unambigous maintainer
to go through.


Thank you! I will do just that.

Re: [PATCH v11 00/11] Add VIRTIO sound card

[Manos Pitsidianakis]

Hello Michael,

Thank you for your reply;

On Wed, 18 Oct 2023 12:08, "Michael S. Tsirkin"  wrote:

On Wed, Oct 18, 2023 at 11:49:00AM +0300, Manos Pitsidianakis wrote:

On Wed, 11 Oct 2023 17:34, Manos Pitsidianakis  
wrote:
> This patch series adds an audio device implementing the recent virtio
> sound spec (1.2) and a corresponding PCI wrapper device.
> 
> v11 can be found online at:
> 
> https://gitlab.com/epilys/qemu/-/tree/virtio-snd-v11
> 
> Ref 885b01fe272541fdab5583780d4c3a59bfd8e734
> 
Hello Gerd, MAINTAINERS says you own the hw/audio subsystem, would you pull

this in your tree if/when it is to be merged?

For context, the device was in hw/audio initially but in early list
discussions we decided to move it with the rest of virtio devices.


Link to these discussions? virtio-gpu is under ./hw/display
seems inconsistent.


I agree that it is inconsistent, but I do not know if there is a general 
consensus on this. I am fine with placing it anywhere that is deemed 
appropriate.


<87a5xkde0c@linaro.org>
https://lore.kernel.org/qemu-devel/87a5xkde0c@linaro.org/


And you are asking Gerd to merge but
your MAINTAINERS patch wouldn't even let people know
he needs to be Cc'd on changes.


No, I didn't ask to merge this, I asked if Gerd would be the one merging 
it because I am not sure myself and wanted more information. What do you 
think should be the correct process?


--
Manos

Re: [PATCH v11 00/11] Add VIRTIO sound card

[Manos Pitsidianakis]

On Wed, 11 Oct 2023 17:34, Manos Pitsidianakis  
wrote:
This patch series adds an audio device implementing the recent virtio 
sound spec (1.2) and a corresponding PCI wrapper device.


v11 can be found online at:

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

Ref 885b01fe272541fdab5583780d4c3a59bfd8e734


Hello Gerd, MAINTAINERS says you own the hw/audio subsystem, would you 
pull this in your tree if/when it is to be merged?


For context, the device was in hw/audio initially but in early list 
discussions we decided to move it with the rest of virtio devices.


At its current state it LGTM, if there's agreement that there are no 
issues we can merge this series.


All the best,

--
Manos

Re: [PATCH v8 3/5] vhost-user-scsi: support reconnect to backend

[Manos Pitsidianakis]

If the backend crashes and restarts, the device is broken.
This patch adds reconnect for vhost-user-scsi.

This patch also improves the error messages, and reports some silent 
errors.


Tested with spdk backend.

Signed-off-by: Li Feng 
---


Reviewed-by: Manos Pitsidianakis 

Re: [RFC PATCH v2 02/78] block: add fallthrough pseudo-keyword

[Manos Pitsidianakis]

On Wed, 18 Oct 2023 01:11, Eric Blake  wrote:
 static bool cluster_needs_new_alloc(BlockDriverState *bs, uint64_t 
 l2_entry)

 {
 switch (qcow2_get_cluster_type(bs, l2_entry)) {
 case QCOW2_CLUSTER_NORMAL:
+fallthrough;
 case QCOW2_CLUSTER_ZERO_ALLOC:


Why is this one needed?  It looks two case labels for the same code is
okay; the fallthrough attribute is only needed once a case label is no
lonter empty.
...
These two also look spurious.

...
but these three seem spurious.


Thanks for pointing it out, Eric. Indeed these are mistakes.

By the way, there's a newer version posted [0] because as you noticed I 
accidentally left --function-context on git-format-patch and didn't 
notice that some of them blew up in size.


Also, the consensus in v1 was to reject this patch series in general 
[1].


[0]:  
https://lore.kernel.org/qemu-devel/cover.1697186560.git.manos.pitsidiana...@linaro.org/


[1]: 


https://lore.kernel.org/qemu-devel/cafeaca_flbe9cuwfypeuejj8dcerhftpnx+ivavfvh4sxx1...@mail.gmail.com/


--
Manos

Re: [PATCH 1/7] hw/virtio/virtio-pmem: Replace impossible check by assertion

[Manos Pitsidianakis]

On Tue, 17 Oct 2023 17:01, Philippe Mathieu-Daudé  wrote:

The get_memory_region() handler is used when (un)plugging the
device, which can only occur *after* it is realized.

virtio_pmem_realize() ensure the instance can not be realized
without 'memdev'. Remove the superfluous check, replacing it
by an assertion.

Signed-off-by: Philippe Mathieu-Daudé 
---
hw/virtio/virtio-pmem.c | 5 +
1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/hw/virtio/virtio-pmem.c b/hw/virtio/virtio-pmem.c
index c3512c2dae..cc24812d2e 100644
--- a/hw/virtio/virtio-pmem.c
+++ b/hw/virtio/virtio-pmem.c
@@ -147,10 +147,7 @@ static void virtio_pmem_fill_device_info(const VirtIOPMEM 
*pmem,
static MemoryRegion *virtio_pmem_get_memory_region(VirtIOPMEM *pmem,
   Error **errp)
{
-if (!pmem->memdev) {
-error_setg(errp, "'%s' property must be set", VIRTIO_PMEM_MEMDEV_PROP);
-return NULL;
-}
+assert(pmem->memdev);

return &pmem->memdev->mr;
}
--
2.41.0



Reviewed-by: Manos Pitsidianakis 

Re: [PATCH 4/7] hw/scsi/virtio-scsi: Use VIRTIO_SCSI_COMMON() macro

[Manos Pitsidianakis]

On Tue, 17 Oct 2023 17:01, Philippe Mathieu-Daudé  wrote:

Access QOM parent with the proper QOM VIRTIO_SCSI_COMMON() macro.

Signed-off-by: Philippe Mathieu-Daudé 
---
hw/scsi/virtio-scsi.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index 45b95ea070..fa53f0902c 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -761,7 +761,7 @@ static void virtio_scsi_fail_cmd_req(VirtIOSCSIReq *req)

static int virtio_scsi_handle_cmd_req_prepare(VirtIOSCSI *s, VirtIOSCSIReq *req)
{
-VirtIOSCSICommon *vs = &s->parent_obj;
+VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(s);
SCSIDevice *d;
int rc;

--
2.41.0




Reviewed-by: Manos Pitsidianakis 

Re: [PATCH 3/7] hw/display/virtio-gpu: Use VIRTIO_DEVICE() macro

[Manos Pitsidianakis]

On Tue, 17 Oct 2023 17:01, Philippe Mathieu-Daudé  wrote:

Access QOM parent with the proper QOM VIRTIO_DEVICE() macro.

Signed-off-by: Philippe Mathieu-Daudé 
---
hw/display/virtio-gpu.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index 93857ad523..51cb517999 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -1132,7 +1132,7 @@ static void virtio_gpu_ctrl_bh(void *opaque)
VirtIOGPU *g = opaque;
VirtIOGPUClass *vgc = VIRTIO_GPU_GET_CLASS(g);

-vgc->handle_ctrl(&g->parent_obj.parent_obj, g->ctrl_vq);
+vgc->handle_ctrl(VIRTIO_DEVICE(g), g->ctrl_vq);
}

static void virtio_gpu_handle_cursor(VirtIODevice *vdev, VirtQueue *vq)
--
2.41.0




Reviewed-by: Manos Pitsidianakis 

Re: [PATCH 2/7] hw/block/vhost-user-blk: Use DEVICE() / VIRTIO_DEVICE() macros

[Manos Pitsidianakis]

On Tue, 17 Oct 2023 17:01, Philippe Mathieu-Daudé  wrote:

Access QOM parent with the proper QOM [VIRTIO_]DEVICE() macros.

Signed-off-by: Philippe Mathieu-Daudé 
---
hw/block/vhost-user-blk.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
index eecf3f7a81..4b37e26120 100644
--- a/hw/block/vhost-user-blk.c
+++ b/hw/block/vhost-user-blk.c
@@ -405,7 +405,7 @@ static void vhost_user_blk_event(void *opaque, QEMUChrEvent 
event)

static int vhost_user_blk_realize_connect(VHostUserBlk *s, Error **errp)
{
-DeviceState *dev = &s->parent_obj.parent_obj;
+DeviceState *dev = DEVICE(s);
int ret;

s->connected = false;
@@ -423,7 +423,7 @@ static int vhost_user_blk_realize_connect(VHostUserBlk *s, 
Error **errp)
assert(s->connected);

ret = vhost_dev_get_config(&s->dev, (uint8_t *)&s->blkcfg,
-   s->parent_obj.config_len, errp);
+   VIRTIO_DEVICE(s)->config_len, errp);
if (ret < 0) {
qemu_chr_fe_disconnect(&s->chardev);
vhost_dev_cleanup(&s->dev);
--
2.41.0




Reviewed-by: Manos Pitsidianakis 

Re: [RFC PATCH 00/78] Strict disable implicit fallthrough

[Manos Pitsidianakis]

On Mon, 16 Oct 2023, 18:04 Peter Maydell,  wrote:

> On Mon, 16 Oct 2023 at 15:58, Manos Pitsidianakis
>  wrote:
> >
> > Hello Peter,
> >
> > On Mon, 16 Oct 2023, 17:13 Peter Maydell, 
> wrote:
> >>
> >> On Fri, 13 Oct 2023 at 13:42, Markus Armbruster 
> wrote:
> >> >
> >> > Emmanouil Pitsidianakis  writes:
> >> >
> >> > > Hello,
> >> > >
> >> > > This RFC is inspired by the kernel's move to
> -Wimplicit-fallthrough=3
> >> > > back in 2019.[0]
> >> > > We take one step (or two) further by increasing it to 5 which
> rejects
> >> > > fall through comments and requires an attribute statement.
> >> > >
> >> > > [0]:
> >> > >
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=a035d552a93b
> >> > >
> >> > > The line differences are not many, but they spread all over
> different
> >> > > subsystems, architectures and devices. An attempt has been made to
> split
> >> > > them in cohesive patches to aid post-RFC review. Part of the RFC is
> to
> >> > > determine whether these patch divisions needs improvement.
> >> > >
> >> > > Main questions this RFC poses
> >> > > =
> >> > >
> >> > > - Is this change desirable and net-positive.
> >> >
> >> > Unwanted fallthrough is an easy mistake to make, and
> >> > -Wimplicit-fallthrough=N helps avoid it.  The question is how far up
> we
> >> > need to push N.  Right now we're at N=2.  Has unwanted fallthrough
> been
> >> > a problem?
> >>
> >> Mmm, this is my opinion I think. We have a mechanism for
> >> catching "forgot the 'break'" already (our =2 setting) and
> >> a way to say "intentional" in a fairly natural way (add the
> >> comment). Does pushing N up any further gain us anything
> >> except a load of churn?
> >>
> >> Also, the compiler is not the only thing that processes our
> >> code: Coverity also looks for "unexpected fallthrough" issues,
> >> so if we wanted to switch away from our current practice we
> >> should check whether what we're switching to is an idiom
> >> that Coverity recognises.
> >
> >
> > It is a code style change as the cover letter mentions, it's not related
> to the static analysis itself.
>
> Yes, exactly. As a code style change it needs a fairly high level
> of justification for the code churn, and the cover letter
> doesn't really provide one...
>


As I state in the cover letter, I personally find that using one macro
instead of a comment regex feels more consistent. But your view is valid as
well!

Let's consider the RFC retracted then.

--
Manos

>

Re: [RFC PATCH 00/78] Strict disable implicit fallthrough

[Manos Pitsidianakis]

Hello Peter,

On Mon, 16 Oct 2023, 17:13 Peter Maydell,  wrote:

> On Fri, 13 Oct 2023 at 13:42, Markus Armbruster  wrote:
> >
> > Emmanouil Pitsidianakis  writes:
> >
> > > Hello,
> > >
> > > This RFC is inspired by the kernel's move to -Wimplicit-fallthrough=3
> > > back in 2019.[0]
> > > We take one step (or two) further by increasing it to 5 which rejects
> > > fall through comments and requires an attribute statement.
> > >
> > > [0]:
> > >
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=a035d552a93b
> > >
> > > The line differences are not many, but they spread all over different
> > > subsystems, architectures and devices. An attempt has been made to
> split
> > > them in cohesive patches to aid post-RFC review. Part of the RFC is to
> > > determine whether these patch divisions needs improvement.
> > >
> > > Main questions this RFC poses
> > > =
> > >
> > > - Is this change desirable and net-positive.
> >
> > Unwanted fallthrough is an easy mistake to make, and
> > -Wimplicit-fallthrough=N helps avoid it.  The question is how far up we
> > need to push N.  Right now we're at N=2.  Has unwanted fallthrough been
> > a problem?
>
> Mmm, this is my opinion I think. We have a mechanism for
> catching "forgot the 'break'" already (our =2 setting) and
> a way to say "intentional" in a fairly natural way (add the
> comment). Does pushing N up any further gain us anything
> except a load of churn?
>
> Also, the compiler is not the only thing that processes our
> code: Coverity also looks for "unexpected fallthrough" issues,
> so if we wanted to switch away from our current practice we
> should check whether what we're switching to is an idiom
> that Coverity recognises.
>

It is a code style change as the cover letter mentions, it's not related to
the static analysis itself.

--
Manos

>

Re: [PATCH] vhost-user: Fix protocol feature bit conflict

[Manos Pitsidianakis]

On Mon, 16 Oct 2023 13:32, Viresh Kumar  wrote:

Perhaps because we will never use it from Qemu code ?


It'd be nice if downstream users of the vhost-user protocol can fetch 
and use the header verbatim. For example, for automatically generating 
bindings.

Re: [PATCH 08/17] configure, tests/tcg: simplify GDB conditionals

[Manos Pitsidianakis]

On Mon, 16 Oct 2023 09:31, Paolo Bonzini  wrote:

Unify HAVE_GDB_BIN (currently in config-host.mak) and
HOST_GDB_SUPPORTS_ARCH into a single GDB variable in
config-target.mak.

Signed-off-by: Paolo Bonzini 


Reviewed-by: Emmanouil Pitsidianakis 

Re: [PATCH 03/17] meson, cutils: allow non-relocatable installs

[Manos Pitsidianakis]

On Mon, 16 Oct 2023 09:31, Paolo Bonzini  wrote:

diff --git a/meson.build b/meson.build
index 010d2c649c2..251838f2609 100644
--- a/meson.build
+++ b/meson.build
@@ -2111,6 +2111,7 @@ config_host_data.set('CONFIG_OPENGL', opengl.found())
config_host_data.set('CONFIG_PLUGIN', get_option('plugins'))
config_host_data.set('CONFIG_RBD', rbd.found())
config_host_data.set('CONFIG_RDMA', rdma.found())
+config_host_data.set('CONFIG_RELOCATABLE', get_option('relocatable'))
config_host_data.set('CONFIG_SAFESTACK', get_option('safe_stack'))
config_host_data.set('CONFIG_SDL', sdl.found())
config_host_data.set('CONFIG_SDL_IMAGE', sdl_image.found())


Is relocatable a good choice here? The term is used in linking and might 
be confusing (when I read the subject that's what I thought it'd be 
about). How about 'movable`?


Otherwise:

Reviewed-by: Emmanouil Pitsidianakis 

Re: [PATCH 01/17] meson: do not build shaders by default

[Manos Pitsidianakis]

On Mon, 16 Oct 2023 09:31, Paolo Bonzini  wrote:

They are not needed when building user-mode emulators.

Signed-off-by: Paolo Bonzini 


Reviewed-by: Emmanouil Pitsidianakis 

Re: [PATCH] tests/docker: avoid invalid escape in Python string

[Manos Pitsidianakis]

On Mon, 16 Oct 2023 09:23, Paolo Bonzini  wrote:

This is an error in Python 3.12; fix it by using a raw string literal.

Cc: qemu-sta...@nongnu.org
Signed-off-by: Paolo Bonzini 
---
tests/docker/docker.py | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/docker/docker.py b/tests/docker/docker.py
index 688ef62989c..3b8a26704df 100755
--- a/tests/docker/docker.py
+++ b/tests/docker/docker.py
@@ -186,7 +186,7 @@ def _check_binfmt_misc(executable):
  (binary))
return None, True

-m = re.search("interpreter (\S+)\n", entry)
+m = re.search(r"interpreter (\S+)\n", entry)
interp = m.group(1)
if interp and interp != executable:
print("binfmt_misc for %s does not point to %s, using %s" %
--
2.41.0




Reviewed-by: Emmanouil Pitsidianakis 

Re: [PATCH] vhost-user: Fix protocol feature bit conflict

[Manos Pitsidianakis]

On Mon, 16 Oct 2023 11:32, Hanna Czenczek  wrote:
diff --git a/include/hw/virtio/vhost-user.h 
b/include/hw/virtio/vhost-user.h

index 9f9ddf878d..1d4121431b 100644
--- a/include/hw/virtio/vhost-user.h
+++ b/include/hw/virtio/vhost-user.h
@@ -29,7 +29,8 @@ enum VhostUserProtocolFeature {
VHOST_USER_PROTOCOL_F_INBAND_NOTIFICATIONS = 14,
VHOST_USER_PROTOCOL_F_CONFIGURE_MEM_SLOTS = 15,
VHOST_USER_PROTOCOL_F_STATUS = 16,
-VHOST_USER_PROTOCOL_F_SHARED_OBJECT = 17,
+/* Feature 17 reserved for VHOST_USER_PROTOCOL_F_XEN_MMAP. */
+VHOST_USER_PROTOCOL_F_SHARED_OBJECT = 18,
VHOST_USER_PROTOCOL_F_MAX
};


May I ask, why not define VHOST_USER_PROTOCOL_F_XEN_MMAP as well instead 
of a comment mention?


Otherwise:

Reviewed-by: Emmanouil Pitsidianakis 

Re: [PATCH] qemu-ui-gtk clipboard: fix for freeze-crashes v2

[Manos Pitsidianakis]

Hello Edmund,

The reproduction in the bug description sounds convoluted so I will 
focus on the code and patch instead:


First of all, the patch title should not include a `v2`. The versioning 
(reroll count) must go in the `[PATCH]` prefix, e.g. `[PATCH v2]`
Secondly, the commit message should describe what the problem is and how 
the fixes in the patch are a solution for it.


See the "meaningfull commit message" in the QEMU docs.

https://www.qemu.org/docs/master/devel/submitting-a-patch.html

 If your patch fixes a bug in the gitlab bug tracker, please add a line 
 with “Resolves: ” to the commit message, too. Gitlab 
 can close bugs automatically once commits with the “Resolved:” keyword 
 get merged into the master branch of the project. And if your patch 
 addresses a bug in another public bug tracker, you can also use a line 
 with “Buglink: ” for reference here, too.


 Example:

 Fixes: 14055ce53c2d ("s390x/tcg: avoid overflows in 
 time2tod/tod2time")

 Resolves: https://gitlab.com/qemu-project/qemu/-/issues/42
 Buglink: https://bugs.launchpad.net/qemu/+bug/1804323``

This information should go before your Signed-off-by: trailer line.

You can still include all that information that should not go into the 
commit message by putting them below the -- line and above the patch, 
see 
https://www.qemu.org/docs/master/devel/submitting-a-patch.html#include-version-history-in-patchset-revisions



On Sat, 14 Oct 2023 11:48, Edmund Raile  wrote:
To not risk breaking anything in the mailing list, I'm starting this 
new mail thread instead of replying to my first one.


That's the right thing to do, replying to old versions will make it less 
visible to people.


Some code comments follow:



---
include/ui/clipboard.h |  2 ++
ui/gtk-clipboard.c | 34 --
2 files changed, 22 insertions(+), 14 deletions(-)

diff --git a/include/ui/clipboard.h b/include/ui/clipboard.h
index ab6acdbd8a..123c04fc07 100644
--- a/include/ui/clipboard.h
+++ b/include/ui/clipboard.h
@@ -106,6 +106,7 @@ struct QemuClipboardNotify {
 * @types: clipboard data array (one entry per type).
 * @has_serial: whether @serial is available.
 * @serial: the grab serial counter.
+ * @serial_last: used by GTK UI to discard outdated transaction results.
 *
 * Clipboard content data and metadata.
 */
@@ -115,6 +116,7 @@ struct QemuClipboardInfo {
QemuClipboardSelection selection;
bool has_serial;
uint32_t serial;
+uint32_t serial_last;
struct {
bool available;
bool requested;
diff --git a/ui/gtk-clipboard.c b/ui/gtk-clipboard.c
index 8d8a636fd1..9e96cc2fb5 100644
--- a/ui/gtk-clipboard.c
+++ b/ui/gtk-clipboard.c
@@ -133,26 +133,38 @@ static void gd_clipboard_notify(Notifier *notifier, void 
*data)
}
}

+/* asynchronous clipboard text transfer (host -> guest): callback */
+static void gd_clipboard_transfer_text_to_guest_callback(GtkClipboard 
*clipboard, const gchar *text, gpointer data)
+{
+QemuClipboardInfo *info = (QemuClipboardInfo *)data;





+
+// serial_last is intentionally not stored as a static in this function as 
callbacks implementing other data types (e.g. images) need access as well


This line and several others are too long. If you run 
scripts/checkpatch.pl on your patch you will see them reported:


 ERROR: line over 90 characters
 #81: FILE: ui/gtk-clipboard.c:141:
 +// serial_last is intentionally not stored as a static in this 
 function as callbacks implementing other data types (e.g. images) need 
 access as well


Also, no C99 // comments:

 ERROR: do not use C99 // comments
 #81: FILE: ui/gtk-clipboard.c:141:
 +// serial_last is intentionally not stored as a static in this 
 function as callbacks implementing other data types (e.g. images) need 
 access as well


In any case I think this comment is not needed.


+
+if (text && (info->serial > info->serial_last)) {
+info->types[QEMU_CLIPBOARD_TYPE_TEXT].available = true;
+qemu_clipboard_update(info);
+info->serial_last = info->serial;
+}
+
+qemu_clipboard_info_unref(info);


Does this free `info`? If yes why update its fields in the previous 
line?


--
Manos

Re: [RFC PATCH v3 00/78] Strict disable implicit fallthrough

[Manos Pitsidianakis]

Hello Richard,

On Fri, 13 Oct 2023 16:52, Richard Henderson  
wrote:
Did this catch any new problems?  If not, I really don't see the 
benefit.

The compiler comment matching appears to be sufficient.


As the cover letter states, this is a code style change. See "Background 
- Motivation" section.


--
Manos

Re: [RFC PATCH 00/78] Strict disable implicit fallthrough

[Manos Pitsidianakis]

On Fri, 13 Oct 2023 11:14, "Daniel P. Berrangé"  wrote:

On Fri, Oct 13, 2023 at 10:47:04AM +0300, Emmanouil Pitsidianakis wrote:


Main questions this RFC poses
=

- Is this change desirable and net-positive.


Yes, IMHO it is worth standardizing on use of the attribute. The allowed
use of comments was a nice thing by the compiler for coping with pre-existing
code, but using the attribute is best long term for a consistent style.


- Should the `fallthrough;` pseudo-keyword be defined like in the Linux
  kernel, or use glib's G_GNUC_FALLTHROUGH, or keep the already existing
  QEMU_FALLTHROUGH macro.


As a general rule, if glib provides functionality we aim o use that
and not reinvent the wheel. IOW, we should just use G_GNUC_FALLTHROUGH.


I agree. My reasoning was:

- The reinvented wheel is only an attribute and not a big bunch of NIH 
 code

- The macro def in glib depends on the glib version you use
- G_GNUC_FALLTHROUGH looks kind of abrasive to my eye, while 
 `fallthrough` blends in with other switch keywords like break.
- C23 standardises fallthrough. We might not ever support C23 but it's 
 good to be consistent with standards and other, larger projects (linux 
 kernel).


I think these (except for myself finding G_GNUC_FALLTHROUGH ugly) make a 
strong case for not using the glib macro, personally. I'd be interested 
to know if there is a counterpoint to it: because I don't want this 
change to cause problems in the future.



Manos

Re: [RFC PATCH 01/78] include/qemu/compiler.h: replace QEMU_FALLTHROUGH with fallthrough

[Manos Pitsidianakis]

Hello Markus,

On Fri, 13 Oct 2023 15:28, Markus Armbruster  wrote:

The commit message needs to explain why.


Certainly.


This is wrong.  docs/devel/style.rst:

   Include directives
   --

   Order include directives as follows:

   .. code-block:: c

   #include "qemu/osdep.h"  /* Always first... */
   #include <...>   /* then system headers... */
   #include "..."   /* and finally QEMU headers. */

Separate patch, please.


I know. spa headers use the `fallthrough` attribute and qemu/compiler.h 
defines it as a macro, so it breaks compilation. If the spa headers go 
after, we'd need to undef fallthrough before including them and 
re-define or re-include qemu/compiler.h after. What do you think would 
be best?





diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h
index 1109482a00..959982805d 100644
--- a/include/qemu/compiler.h
+++ b/include/qemu/compiler.h
@@ -1,215 +1,231 @@


[...]


 #define QEMU_ALWAYS_INLINE
 #endif
 
-/**

- * In most cases, normal "fallthrough" comments are good enough for
- * switch-case statements, but sometimes the compiler has problems
- * with those. In that case you can use QEMU_FALLTHROUGH instead.
+/*
+ * Add the pseudo keyword 'fallthrough' so case statement blocks


Pseudo-keyword?  It's a macro.


C calls reserved words that you cannot redefine 'keywords'. Like 
'break', 'continue', 'return'. Hence it's a pseudo-keyword. It's also a 
macro. They are not mutually exclusive.


I did not write this, it was taken verbatim from the linux kernel 
source, see: /include/linux/compiler_attributes.h





+ * must end with any of these keywords:
+ *   break;
+ *   fallthrough;
+ *   continue;
+ *   goto ;
+ *   return [expression];


These are statements, not keywords.


I'm pretty sure it refers to {break, fallthrough, continue, goto, 
return} by themselves.





+ *
+ *  gcc: 
https://gcc.gnu.org/onlinedocs/gcc/Statement-Attributes.html#Statement-Attributes


Not sure we need to point to the docs here.  We have hundreds of
__attribute__ uses in the tree.


Again, copied from /include/linux/compiler_attributes.h




  */
-#if __has_attribute(fallthrough)
-# define QEMU_FALLTHROUGH __attribute__((fallthrough))
+
+/*
+ * glib_macros.h contains its own definition of fallthrough, so if we define
+ * the pseudokeyword here it will expand when the glib header checks for the
+ * attribute. glib headers must be #included after this header.
+ */
+#ifdef fallthrough
+#undef fallthrough
+#endif


Why do we need to roll our own macro then?


Glib uses a different name. The problem is when it checks for the 
compiler attribute, which expands into our macro.



--
Manos 

Re: [RFC PATCH v3 57/78] hw/net: add fallthrough pseudo-keyword

[Manos Pitsidianakis]

On Fri, 13 Oct 2023 at 12:11, Akihiko Odaki  wrote:
> > diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c
> > index 4525fda383..42f19618b1 100644
> > --- a/hw/net/rtl8139.c
> > +++ b/hw/net/rtl8139.c
> > @@ -2447,8 +2447,10 @@ static uint32_t 
> > rtl8139_TxStatus_TxAddr_read(RTL8139State *s, uint32_t regs[],
> >   }
> >
> >   switch (size) {
> > -case 1: /* fall through */
> > -case 2: /* fall through */
> > +case 1:
> > +fallthrough;
> > +case 2:
> > +fallthrough;
> >   case 4:
>
> I don't think you need comments or pseudo-keywords here.

That's correct, it was a stylistic change. I can remove them in the
next version. Thank you!

Manos

Re: [RFC PATCH 01/78] include/qemu/compiler.h: replace QEMU_FALLTHROUGH with fallthrough

[Manos Pitsidianakis]

On Fri, 13 Oct 2023 at 11:16, Daniel P. Berrangé  wrote:
> This patch (and all the others in the series) have a ridiculously
> large context either side of the change. It makes this horrible
> to review as it requires wading through pages of pre-existing code
> trying to spot the change.
>
> Please send patches with the default git context lines setting.

Many thanks Daniel, I had not noticed at all. Somehow that option
slipped through...

Will reroll the patch series.

Re: [PATCH 03/10] tests/virtio-scsi: Clean up global variable shadowing

[Manos Pitsidianakis]

On Mon, 09 Oct 2023 13:02, Philippe Mathieu-Daudé  wrote:

Rename the (unused) 'allow' argument, following the pattern


s/allow/alloc

Otherwise,

Reviewed-By: Emmanouil Pitsidianakis 


used by the other tests in this file. This fixes:

 tests/qtest/virtio-scsi-test.c:159:61: error: declaration shadows a variable 
in the global scope [-Werror,-Wshadow]
 static void hotplug(void *obj, void *data, QGuestAllocator *alloc)
 ^
 tests/qtest/virtio-scsi-test.c:37:25: note: previous declaration is here
 static QGuestAllocator *alloc;
 ^

Signed-off-by: Philippe Mathieu-Daudé 
---
tests/qtest/virtio-scsi-test.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qtest/virtio-scsi-test.c b/tests/qtest/virtio-scsi-test.c
index ceaa7f2415..db10d572d0 100644
--- a/tests/qtest/virtio-scsi-test.c
+++ b/tests/qtest/virtio-scsi-test.c
@@ -156,7 +156,7 @@ static QVirtioSCSIQueues *qvirtio_scsi_init(QVirtioDevice 
*dev)
return vs;
}

-static void hotplug(void *obj, void *data, QGuestAllocator *alloc)
+static void hotplug(void *obj, void *data, QGuestAllocator *t_alloc)
{
QTestState *qts = global_qtest;

--
2.41.0

[PATCH v11 07/11] virtio-sound: handle VIRTIO_SND_R_PCM_PREPARE

[Manos Pitsidianakis]

From: Emmanouil Pitsidianakis 

Handles the PCM prepare control request. It initializes a PCM stream
when the guests asks for it.

Based-on: 
https://github.com/OpenSynergy/qemu/commit/5a2f350eec5d157b90d9c7b40a8e603f4da92471
Signed-off-by: Igor Skalkin 
Signed-off-by: Anton Yakovlev 
Signed-off-by: Emmanouil Pitsidianakis 
Reviewed-by: Alex Bennée 
---
 hw/virtio/virtio-snd.c | 24 
 1 file changed, 24 insertions(+)

diff --git a/hw/virtio/virtio-snd.c b/hw/virtio/virtio-snd.c
index 6a7545536b..49cd9f3ca4 100644
--- a/hw/virtio/virtio-snd.c
+++ b/hw/virtio/virtio-snd.c
@@ -458,30 +458,52 @@ static uint32_t virtio_snd_pcm_prepare(VirtIOSound *s, 
uint32_t stream_id)
 static const char *print_code(uint32_t code)
 {
 #define CASE(CODE)\
 case VIRTIO_SND_R_##CODE: \
 return "VIRTIO_SND_R_"#CODE
 
 switch (code) {
 CASE(JACK_INFO);
 CASE(JACK_REMAP);
 CASE(PCM_INFO);
 CASE(PCM_SET_PARAMS);
 CASE(PCM_PREPARE);
 CASE(PCM_RELEASE);
 CASE(PCM_START);
 CASE(PCM_STOP);
 CASE(CHMAP_INFO);
 default:
 return "invalid code";
 }
 
 #undef CASE
 };
 
+/*
+ * Handles VIRTIO_SND_R_PCM_PREPARE.
+ *
+ * @s: VirtIOSound device
+ * @cmd: The request command queue element from VirtIOSound cmdq field
+ */
+static void virtio_snd_handle_pcm_prepare(VirtIOSound *s,
+  virtio_snd_ctrl_command *cmd)
+{
+uint32_t stream_id;
+size_t msg_sz = iov_to_buf(cmd->elem->out_sg,
+   cmd->elem->out_num,
+   sizeof(virtio_snd_hdr),
+   &stream_id,
+   sizeof(stream_id));
+
+stream_id = le32_to_cpu(stream_id);
+cmd->resp.code = msg_sz == sizeof(stream_id)
+   ? virtio_snd_pcm_prepare(s, stream_id)
+   : cpu_to_le32(VIRTIO_SND_S_BAD_MSG);
+}
+
 /*
  * Handles VIRTIO_SND_R_PCM_START.
  *
  * @s: VirtIOSound device
  * @cmd: The request command queue element from VirtIOSound cmdq field
  * @start: whether to start or stop the device
  */
@@ -529,72 +551,74 @@ static inline void
 process_cmd(VirtIOSound *s, virtio_snd_ctrl_command *cmd)
 {
 uint32_t code;
 size_t msg_sz = iov_to_buf(cmd->elem->out_sg,
cmd->elem->out_num,
0,
&cmd->ctrl,
sizeof(virtio_snd_hdr));
 
 if (msg_sz != sizeof(virtio_snd_hdr)) {
 /*
  * TODO: do we need to set DEVICE_NEEDS_RESET?
  */
 qemu_log_mask(LOG_GUEST_ERROR,
 "%s: virtio-snd command size incorrect %zu vs \
 %zu\n", __func__, msg_sz, sizeof(virtio_snd_hdr));
 return;
 }
 
 code = le32_to_cpu(cmd->ctrl.code);
 
 trace_virtio_snd_handle_code(code, print_code(code));
 
 switch (code) {
 case VIRTIO_SND_R_JACK_INFO:
 case VIRTIO_SND_R_JACK_REMAP:
 qemu_log_mask(LOG_UNIMP,
  "virtio_snd: jack functionality is unimplemented.\n");
 cmd->resp.code = cpu_to_le32(VIRTIO_SND_S_NOT_SUPP);
 break;
 case VIRTIO_SND_R_PCM_INFO:
 virtio_snd_handle_pcm_info(s, cmd);
 break;
 case VIRTIO_SND_R_PCM_START:
 virtio_snd_handle_pcm_start_stop(s, cmd, true);
 break;
 case VIRTIO_SND_R_PCM_STOP:
 virtio_snd_handle_pcm_start_stop(s, cmd, false);
 break;
 case VIRTIO_SND_R_PCM_SET_PARAMS:
 virtio_snd_handle_pcm_set_params(s, cmd);
 break;
 case VIRTIO_SND_R_PCM_PREPARE:
+virtio_snd_handle_pcm_prepare(s, cmd);
+break;
 case VIRTIO_SND_R_PCM_RELEASE:
 cmd->resp.code = cpu_to_le32(VIRTIO_SND_S_NOT_SUPP);
 break;
 case VIRTIO_SND_R_CHMAP_INFO:
 qemu_log_mask(LOG_UNIMP,
  "virtio_snd: chmap info functionality is 
unimplemented.\n");
 trace_virtio_snd_handle_chmap_info();
 cmd->resp.code = cpu_to_le32(VIRTIO_SND_S_NOT_SUPP);
 break;
 default:
 /* error */
 error_report("virtio snd header not recognized: %"PRIu32, code);
 cmd->resp.code = cpu_to_le32(VIRTIO_SND_S_BAD_MSG);
 }
 
 iov_from_buf(cmd->elem->in_sg,
  cmd->elem->in_num,
  0,
  &cmd->resp,
  sizeof(virtio_snd_hdr));
 virtqueue_push(cmd->vq, cmd->elem, sizeof(virtio_snd_hdr));
 virtio_notify(VIRTIO_DEVICE(s), cmd->vq);
 }
 
 /*
  * Consume all elements in command queue.
  *
  * @s: VirtIOSound device
  */
-- 
2.39.2

[PATCH v11 06/11] virtio-sound: handle VIRTIO_SND_R_PCM_SET_PARAMS

[Manos Pitsidianakis]

From: Emmanouil Pitsidianakis 

Handle the set parameters control request. It reconfigures a stream
based on a guest's preference if the values are valid and supported.

Based-on: 
https://github.com/OpenSynergy/qemu/commit/5a2f350eec5d157b90d9c7b40a8e603f4da92471
Signed-off-by: Igor Skalkin 
Signed-off-by: Anton Yakovlev 
Signed-off-by: Emmanouil Pitsidianakis 
Reviewed-by: Alex Bennée 
---
 hw/virtio/trace-events |  1 +
 hw/virtio/virtio-snd.c | 34 ++
 2 files changed, 35 insertions(+)

diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
index 7044b110b7..7907b610c1 100644
--- a/hw/virtio/trace-events
+++ b/hw/virtio/trace-events
@@ -163,6 +163,7 @@ virtio_snd_vm_state_running(void) "vm state running"
 virtio_snd_vm_state_stopped(void) "vm state stopped"
 virtio_snd_realize(void *snd) "snd %p: realize"
 virtio_snd_unrealize(void *snd) "snd %p: unrealize"
+virtio_snd_handle_pcm_set_params(uint32_t stream) "VIRTIO_SND_PCM_SET_PARAMS 
called for stream %"PRIu32
 virtio_snd_handle_ctrl(void *vdev, void *vq) "snd %p: handle ctrl event for 
queue %p"
 virtio_snd_handle_pcm_info(uint32_t stream) "VIRTIO_SND_R_PCM_INFO called for 
stream %"PRIu32
 virtio_snd_handle_pcm_start_stop(const char *code, uint32_t stream) "%s called 
for stream %"PRIu32
diff --git a/hw/virtio/virtio-snd.c b/hw/virtio/virtio-snd.c
index 50b5a9d5df..6a7545536b 100644
--- a/hw/virtio/virtio-snd.c
+++ b/hw/virtio/virtio-snd.c
@@ -250,43 +250,75 @@ static
 uint32_t virtio_snd_set_pcm_params(VirtIOSound *s,
uint32_t stream_id,
virtio_snd_pcm_set_params *params)
 {
 virtio_snd_pcm_set_params *st_params;
 
 if (stream_id >= s->snd_conf.streams || s->pcm->pcm_params == NULL) {
 /*
  * TODO: do we need to set DEVICE_NEEDS_RESET?
  */
 virtio_error(VIRTIO_DEVICE(s), "Streams have not been initialized.\n");
 return cpu_to_le32(VIRTIO_SND_S_BAD_MSG);
 }
 
 st_params = virtio_snd_pcm_get_params(s, stream_id);
 
 if (params->channels < 1 || params->channels > AUDIO_MAX_CHANNELS) {
 error_report("Number of channels is not supported.");
 return cpu_to_le32(VIRTIO_SND_S_NOT_SUPP);
 }
 if (!(supported_formats & BIT(params->format))) {
 error_report("Stream format is not supported.");
 return cpu_to_le32(VIRTIO_SND_S_NOT_SUPP);
 }
 if (!(supported_rates & BIT(params->rate))) {
 error_report("Stream rate is not supported.");
 return cpu_to_le32(VIRTIO_SND_S_NOT_SUPP);
 }
 
 st_params->buffer_bytes = le32_to_cpu(params->buffer_bytes);
 st_params->period_bytes = le32_to_cpu(params->period_bytes);
 st_params->features = le32_to_cpu(params->features);
 /* the following are uint8_t, so there's no need to bswap the values. */
 st_params->channels = params->channels;
 st_params->format = params->format;
 st_params->rate = params->rate;
 
 return cpu_to_le32(VIRTIO_SND_S_OK);
 }
 
+/*
+ * Handles the VIRTIO_SND_R_PCM_SET_PARAMS request.
+ *
+ * @s: VirtIOSound device
+ * @cmd: The request command queue element from VirtIOSound cmdq field
+ */
+static void virtio_snd_handle_pcm_set_params(VirtIOSound *s,
+ virtio_snd_ctrl_command *cmd)
+{
+virtio_snd_pcm_set_params req = { 0 };
+uint32_t stream_id;
+size_t msg_sz = iov_to_buf(cmd->elem->out_sg,
+   cmd->elem->out_num,
+   0,
+   &req,
+   sizeof(virtio_snd_pcm_set_params));
+
+if (msg_sz != sizeof(virtio_snd_pcm_set_params)) {
+/*
+ * TODO: do we need to set DEVICE_NEEDS_RESET?
+ */
+qemu_log_mask(LOG_GUEST_ERROR,
+"%s: virtio-snd command size incorrect %zu vs \
+%zu\n", __func__, msg_sz, sizeof(virtio_snd_pcm_set_params));
+cmd->resp.code = cpu_to_le32(VIRTIO_SND_S_BAD_MSG);
+return;
+}
+stream_id = le32_to_cpu(req.hdr.stream_id);
+trace_virtio_snd_handle_pcm_set_params(stream_id);
+cmd->resp.code = virtio_snd_set_pcm_params(s, stream_id, &req);
+}
+
 /*
  * Get a QEMU Audiosystem compatible format value from a VIRTIO_SND_PCM_FMT_*
  */
@@ -497,70 +529,72 @@ static inline void
 process_cmd(VirtIOSound *s, virtio_snd_ctrl_command *cmd)
 {
 uint32_t code;
 size_t msg_sz = iov_to_buf(cmd->elem->out_sg,
cmd->elem->out_num,
0,
&cmd->ctrl,
sizeof(virtio_snd_hdr));
 
 if (msg_sz != sizeof(virtio_snd_hdr)) {
 /*
  * TODO: do we need to set DEVICE_NEEDS_RESET?
  */
 qemu_log_mask(LOG_GUEST_ERROR,
 "%s: virtio-snd command size incorrect %zu vs \
 %zu\n", __func__, msg_sz, sizeof(virtio_snd_hdr)

[PATCH v11 09/11] virtio-sound: implement audio output (TX)

[Manos Pitsidianakis]

From: Emmanouil Pitsidianakis 

Handle output IO messages in the transmit (TX) virtqueue.

It allocates a VirtIOSoundPCMBuffer for each IO message and copies the
data buffer to it. When the IO buffer is written to the host's sound
card, the guest will be notified that it has been consumed.

The lifetime of an IO message is:

1. Guest sends IO message to TX virtqueue.
2. QEMU adds it to the appropriate stream's IO buffer queue.
3. Sometime later, the host audio backend calls the output callback,
   virtio_snd_pcm_out_cb(), which is defined with an AUD_open_out()
   call. The callback gets an available number of bytes the backend can
   receive. Then it writes data from the IO buffer queue to the backend.
   If at any time a buffer is exhausted, it is returned to the guest as
   completed.
4. If the guest releases the stream, its buffer queue is flushed by
   attempting to write any leftover data to the audio backend and
   releasing all IO messages back to the guest. This is how according to
   the spec the guest knows the release was successful.

Based-on: 
https://github.com/OpenSynergy/qemu/commit/5a2f350eec5d157b90d9c7b40a8e603f4da92471
Signed-off-by: Igor Skalkin 
Signed-off-by: Anton Yakovlev 
Signed-off-by: Emmanouil Pitsidianakis 
Reviewed-by: Alex Bennée 
---
 hw/virtio/trace-events |   2 +
 hw/virtio/virtio-snd.c | 288 -
 include/hw/virtio/virtio-snd.h |  47 ++
 3 files changed, 332 insertions(+), 5 deletions(-)

diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
index b0789a6e7e..a31531970d 100644
--- a/hw/virtio/trace-events
+++ b/hw/virtio/trace-events
@@ -171,3 +171,5 @@ virtio_snd_handle_pcm_release(uint32_t stream) 
"VIRTIO_SND_PCM_RELEASE called fo
 virtio_snd_handle_code(uint32_t val, const char *code) "ctrl code msg val = 
%"PRIu32" == %s"
 virtio_snd_handle_chmap_info(void) "VIRTIO_SND_CHMAP_INFO called"
 virtio_snd_handle_event(void) "event queue callback called"
+virtio_snd_pcm_stream_flush(uint32_t stream) "flushing stream %"PRIu32
+virtio_snd_handle_xfer(void) "tx/rx queue callback called"
diff --git a/hw/virtio/virtio-snd.c b/hw/virtio/virtio-snd.c
index ca873fd6d4..5e9513fb26 100644
--- a/hw/virtio/virtio-snd.c
+++ b/hw/virtio/virtio-snd.c
@@ -32,6 +32,10 @@
 #define VIRTIO_SOUND_CHMAP_DEFAULT 0
 #define VIRTIO_SOUND_HDA_FN_NID 0
 
+static void virtio_snd_pcm_out_cb(void *data, int available);
+static void virtio_snd_process_cmdq(VirtIOSound *s);
+static void virtio_snd_pcm_flush(VirtIOSoundPCMStream *stream);
+
 static uint32_t supported_formats = BIT(VIRTIO_SND_PCM_FMT_S8)
   | BIT(VIRTIO_SND_PCM_FMT_U8)
   | BIT(VIRTIO_SND_PCM_FMT_S16)
@@ -123,6 +127,13 @@ virtio_snd_set_config(VirtIODevice *vdev, const uint8_t 
*config)
 
 }
 
+static void
+virtio_snd_pcm_buffer_free(VirtIOSoundPCMBuffer *buffer)
+{
+g_free(buffer->elem);
+g_free(buffer);
+}
+
 static void
 virtio_snd_ctrl_cmd_free(virtio_snd_ctrl_command *cmd)
 {
@@ -392,66 +403,88 @@ static void virtio_snd_get_qemu_audsettings(audsettings 
*as,
 /*
  * Close a stream and free all its resources.
  *
  * @stream: VirtIOSoundPCMStream *stream
  */
 static void virtio_snd_pcm_close(VirtIOSoundPCMStream *stream)
 {
+if (stream) {
+if (stream->info.direction == VIRTIO_SND_D_OUTPUT) {
+virtio_snd_pcm_flush(stream);
+AUD_close_out(&stream->pcm->snd->card, stream->voice.out);
+stream->voice.out = NULL;
+}
+}
 }
 
 /*
  * Prepares a VirtIOSound card stream.
  * Returns the response status code. (VIRTIO_SND_S_*).
  *
  * @s: VirtIOSound device
  * @stream_id: stream id
  */
 static uint32_t virtio_snd_pcm_prepare(VirtIOSound *s, uint32_t stream_id)
 {
 audsettings as;
 virtio_snd_pcm_set_params *params;
 VirtIOSoundPCMStream *stream;
 
 if (s->pcm->streams == NULL ||
 s->pcm->pcm_params == NULL ||
 stream_id >= s->snd_conf.streams) {
 return cpu_to_le32(VIRTIO_SND_S_BAD_MSG);
 }
 
 params = virtio_snd_pcm_get_params(s, stream_id);
 if (params == NULL) {
 return cpu_to_le32(VIRTIO_SND_S_BAD_MSG);
 }
 
 stream = virtio_snd_pcm_get_stream(s, stream_id);
 if (stream == NULL) {
 stream = g_new0(VirtIOSoundPCMStream, 1);
 stream->active = false;
 stream->id = stream_id;
 stream->pcm = s->pcm;
 stream->s = s;
+qemu_mutex_init(&stream->queue_mutex);
+QSIMPLEQ_INIT(&stream->queue);
+QSIMPLEQ_INIT(&stream->invalid);
 
 /*
  * stream_id >= s->snd_conf.streams was checked before so this is
  * in-bounds
  */
 s->pcm->streams[stream_id] = stream;
 }
 
 virtio_snd_get_qemu_audsettings(&as, params);
 stream->info.direction = stream_id < s->snd_conf.streams / 2 +
 (s->snd_conf.streams & 1) ? VIRTIO_SND_D_OUTPUT : VIRTIO_SND_D_INPUT;
 stream->info.hdr.hda_fn_nid = VI

[PATCH v11 05/11] virtio-sound: handle VIRTIO_SND_R_PCM_{START,STOP}

[Manos Pitsidianakis]

From: Emmanouil Pitsidianakis 

Handle the start and stop control messages for a stream_id. This request
does nothing at the moment except for replying to it. Audio playback
or capture will be started/stopped here in follow-up commits.

Based-on: 
https://github.com/OpenSynergy/qemu/commit/5a2f350eec5d157b90d9c7b40a8e603f4da92471
Signed-off-by: Igor Skalkin 
Signed-off-by: Anton Yakovlev 
Signed-off-by: Emmanouil Pitsidianakis 
Reviewed-by: Alex Bennée 
---
 hw/virtio/trace-events |  1 +
 hw/virtio/virtio-snd.c | 49 --
 2 files changed, 48 insertions(+), 2 deletions(-)

diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
index 88de2021c8..7044b110b7 100644
--- a/hw/virtio/trace-events
+++ b/hw/virtio/trace-events
@@ -165,6 +165,7 @@ virtio_snd_realize(void *snd) "snd %p: realize"
 virtio_snd_unrealize(void *snd) "snd %p: unrealize"
 virtio_snd_handle_ctrl(void *vdev, void *vq) "snd %p: handle ctrl event for 
queue %p"
 virtio_snd_handle_pcm_info(uint32_t stream) "VIRTIO_SND_R_PCM_INFO called for 
stream %"PRIu32
+virtio_snd_handle_pcm_start_stop(const char *code, uint32_t stream) "%s called 
for stream %"PRIu32
 virtio_snd_handle_code(uint32_t val, const char *code) "ctrl code msg val = 
%"PRIu32" == %s"
 virtio_snd_handle_chmap_info(void) "VIRTIO_SND_CHMAP_INFO called"
 virtio_snd_handle_event(void) "event queue callback called"
diff --git a/hw/virtio/virtio-snd.c b/hw/virtio/virtio-snd.c
index ec945d55a7..50b5a9d5df 100644
--- a/hw/virtio/virtio-snd.c
+++ b/hw/virtio/virtio-snd.c
@@ -426,29 +426,70 @@ static uint32_t virtio_snd_pcm_prepare(VirtIOSound *s, 
uint32_t stream_id)
 static const char *print_code(uint32_t code)
 {
 #define CASE(CODE)\
 case VIRTIO_SND_R_##CODE: \
 return "VIRTIO_SND_R_"#CODE
 
 switch (code) {
 CASE(JACK_INFO);
 CASE(JACK_REMAP);
 CASE(PCM_INFO);
 CASE(PCM_SET_PARAMS);
 CASE(PCM_PREPARE);
 CASE(PCM_RELEASE);
 CASE(PCM_START);
 CASE(PCM_STOP);
 CASE(CHMAP_INFO);
 default:
 return "invalid code";
 }
 
 #undef CASE
 };
 
+/*
+ * Handles VIRTIO_SND_R_PCM_START.
+ *
+ * @s: VirtIOSound device
+ * @cmd: The request command queue element from VirtIOSound cmdq field
+ * @start: whether to start or stop the device
+ */
+static void virtio_snd_handle_pcm_start_stop(VirtIOSound *s,
+ virtio_snd_ctrl_command *cmd,
+ bool start)
+{
+VirtIOSoundPCMStream *stream;
+virtio_snd_pcm_hdr req;
+uint32_t stream_id;
+size_t msg_sz = iov_to_buf(cmd->elem->out_sg,
+   cmd->elem->out_num,
+   0,
+   &req,
+   sizeof(virtio_snd_pcm_hdr));
+
+if (msg_sz != sizeof(virtio_snd_pcm_hdr)) {
+qemu_log_mask(LOG_GUEST_ERROR,
+"%s: virtio-snd command size incorrect %zu vs \
+%zu\n", __func__, msg_sz, sizeof(virtio_snd_pcm_hdr));
+cmd->resp.code = cpu_to_le32(VIRTIO_SND_S_BAD_MSG);
+return;
+}
+
+stream_id = le32_to_cpu(req.stream_id);
+cmd->resp.code = cpu_to_le32(VIRTIO_SND_S_OK);
+trace_virtio_snd_handle_pcm_start_stop(start ? "VIRTIO_SND_R_PCM_START" :
+"VIRTIO_SND_R_PCM_STOP", stream_id);
+stream = virtio_snd_pcm_get_stream(s, stream_id);
+if (stream == NULL) {
+error_report("Invalid stream id: %"PRIu32, req.stream_id);
+cmd->resp.code = cpu_to_le32(VIRTIO_SND_S_BAD_MSG);
+return;
+}
+stream->active = start;
+}
+
 /*
  * The actual processing done in virtio_snd_process_cmdq().
  *
  * @s: VirtIOSound device
  * @cmd: control command request
  */
@@ -456,66 +497,70 @@ static inline void
 process_cmd(VirtIOSound *s, virtio_snd_ctrl_command *cmd)
 {
 uint32_t code;
 size_t msg_sz = iov_to_buf(cmd->elem->out_sg,
cmd->elem->out_num,
0,
&cmd->ctrl,
sizeof(virtio_snd_hdr));
 
 if (msg_sz != sizeof(virtio_snd_hdr)) {
 /*
  * TODO: do we need to set DEVICE_NEEDS_RESET?
  */
 qemu_log_mask(LOG_GUEST_ERROR,
 "%s: virtio-snd command size incorrect %zu vs \
 %zu\n", __func__, msg_sz, sizeof(virtio_snd_hdr));
 return;
 }
 
 code = le32_to_cpu(cmd->ctrl.code);
 
 trace_virtio_snd_handle_code(code, print_code(code));
 
 switch (code) {
 case VIRTIO_SND_R_JACK_INFO:
 case VIRTIO_SND_R_JACK_REMAP:
 qemu_log_mask(LOG_UNIMP,
  "virtio_snd: jack functionality is unimplemented.\n");
 cmd->resp.code = cpu_to_le32(VIRTIO_SND_S_NOT_SUPP);
 break;
 case VIRTIO_SND_R_PCM_INFO:
 virtio_snd_handle_pcm_info(s, cmd);
 break;
-case VIRTIO_SND_R_PCM_SET_PARAMS:
-c

[PATCH v11 04/11] virtio-sound: handle VIRTIO_SND_R_PCM_INFO request

[Manos Pitsidianakis]

From: Emmanouil Pitsidianakis 

Respond to the VIRTIO_SND_R_PCM_INFO control request with the parameters
of each requested PCM stream.

Based-on: 
https://github.com/OpenSynergy/qemu/commit/5a2f350eec5d157b90d9c7b40a8e603f4da92471
Signed-off-by: Igor Skalkin 
Signed-off-by: Anton Yakovlev 
Signed-off-by: Emmanouil Pitsidianakis 
Reviewed-by: Alex Bennée 
---
 hw/virtio/trace-events |  1 +
 hw/virtio/virtio-snd.c | 82 ++
 2 files changed, 83 insertions(+)

diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
index 48043fed3e..88de2021c8 100644
--- a/hw/virtio/trace-events
+++ b/hw/virtio/trace-events
@@ -164,6 +164,7 @@ virtio_snd_vm_state_stopped(void) "vm state stopped"
 virtio_snd_realize(void *snd) "snd %p: realize"
 virtio_snd_unrealize(void *snd) "snd %p: unrealize"
 virtio_snd_handle_ctrl(void *vdev, void *vq) "snd %p: handle ctrl event for 
queue %p"
+virtio_snd_handle_pcm_info(uint32_t stream) "VIRTIO_SND_R_PCM_INFO called for 
stream %"PRIu32
 virtio_snd_handle_code(uint32_t val, const char *code) "ctrl code msg val = 
%"PRIu32" == %s"
 virtio_snd_handle_chmap_info(void) "VIRTIO_SND_CHMAP_INFO called"
 virtio_snd_handle_event(void) "event queue callback called"
diff --git a/hw/virtio/virtio-snd.c b/hw/virtio/virtio-snd.c
index 705dc07212..ec945d55a7 100644
--- a/hw/virtio/virtio-snd.c
+++ b/hw/virtio/virtio-snd.c
@@ -147,22 +147,102 @@ static VirtIOSoundPCMStream 
*virtio_snd_pcm_get_stream(VirtIOSound *s,
 /*
  * Get params for a specific stream.
  *
  * @s: VirtIOSound device
  * @stream_id: stream id
  */
 static virtio_snd_pcm_set_params *virtio_snd_pcm_get_params(VirtIOSound *s,
 uint32_t stream_id)
 {
 return stream_id >= s->snd_conf.streams ? NULL
 : &s->pcm->pcm_params[stream_id];
 }
 
+/*
+ * Handle the VIRTIO_SND_R_PCM_INFO request.
+ * The function writes the info structs to the request element.
+ *
+ * @s: VirtIOSound device
+ * @cmd: The request command queue element from VirtIOSound cmdq field
+ */
+static void virtio_snd_handle_pcm_info(VirtIOSound *s,
+   virtio_snd_ctrl_command *cmd)
+{
+uint32_t stream_id, start_id, count, size;
+virtio_snd_pcm_info val;
+virtio_snd_query_info req;
+VirtIOSoundPCMStream *stream = NULL;
+g_autofree virtio_snd_pcm_info *pcm_info = NULL;
+size_t msg_sz = iov_to_buf(cmd->elem->out_sg,
+   cmd->elem->out_num,
+   0,
+   &req,
+   sizeof(virtio_snd_query_info));
+
+if (msg_sz != sizeof(virtio_snd_query_info)) {
+/*
+ * TODO: do we need to set DEVICE_NEEDS_RESET?
+ */
+qemu_log_mask(LOG_GUEST_ERROR,
+"%s: virtio-snd command size incorrect %zu vs \
+%zu\n", __func__, msg_sz, sizeof(virtio_snd_query_info));
+cmd->resp.code = cpu_to_le32(VIRTIO_SND_S_BAD_MSG);
+return;
+}
+
+start_id = le32_to_cpu(req.start_id);
+count = le32_to_cpu(req.count);
+size = le32_to_cpu(req.size);
+
+if (iov_size(cmd->elem->in_sg, cmd->elem->in_num) <
+sizeof(virtio_snd_hdr) + size * count) {
+/*
+ * TODO: do we need to set DEVICE_NEEDS_RESET?
+ */
+error_report("pcm info: buffer too small, got: %zu, needed: %zu",
+iov_size(cmd->elem->in_sg, cmd->elem->in_num),
+sizeof(virtio_snd_pcm_info));
+cmd->resp.code = cpu_to_le32(VIRTIO_SND_S_BAD_MSG);
+return;
+}
+
+pcm_info = g_new0(virtio_snd_pcm_info, count);
+for (uint32_t i = 0; i < count; i++) {
+stream_id = i + start_id;
+trace_virtio_snd_handle_pcm_info(stream_id);
+stream = virtio_snd_pcm_get_stream(s, stream_id);
+if (!stream) {
+error_report("Invalid stream id: %"PRIu32, stream_id);
+cmd->resp.code = cpu_to_le32(VIRTIO_SND_S_BAD_MSG);
+return;
+}
+val = stream->info;
+val.hdr.hda_fn_nid = cpu_to_le32(val.hdr.hda_fn_nid);
+val.features = cpu_to_le32(val.features);
+val.formats = cpu_to_le64(val.formats);
+val.rates = cpu_to_le64(val.rates);
+/*
+ * 5.14.6.6.2.1 Device Requirements: Stream Information The device MUST
+ * NOT set undefined feature, format, rate and direction values. The
+ * device MUST initialize the padding bytes to 0.
+ */
+pcm_info[i] = val;
+memset(&pcm_info[i].padding, 0, 5);
+}
+
+cmd->resp.code = cpu_to_le32(VIRTIO_SND_S_OK);
+iov_from_buf(cmd->elem->in_sg,
+ cmd->elem->in_num,
+ sizeof(virtio_snd_hdr),
+ pcm_info,
+ sizeof(virtio_snd_pcm_info) * count);
+}
+
 /*
  * Set the given stream params.
  * Called by both virtio_snd_handle_pcm_set_params and during device
  * initial

[PATCH v11 03/11] virtio-sound: handle control messages and streams

[Manos Pitsidianakis]

From: Emmanouil Pitsidianakis 

Receive guest requests in the control (CTRL) queue of the virtio sound
device and reply with a NOT SUPPORTED error to all control commands.

The receiving handler is virtio_snd_handle_ctrl(). It stores all control
messages in the queue in the device's command queue. Then it calls
virtio_snd_process_cmdq() to handle each message.

The handler is process_cmd() which replies with VIRTIO_SND_S_NOT_SUPP.

Based-on: 
https://github.com/OpenSynergy/qemu/commit/5a2f350eec5d157b90d9c7b40a8e603f4da92471
Reviewed-by: Alex Bennée 
Signed-off-by: Igor Skalkin 
Signed-off-by: Anton Yakovlev 
Signed-off-by: Emmanouil Pitsidianakis 
---
 hw/virtio/trace-events |   4 +
 hw/virtio/virtio-snd.c | 487 -
 include/hw/virtio/virtio-snd.h | 113 +++-
 3 files changed, 595 insertions(+), 9 deletions(-)

diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
index 96b997f427..48043fed3e 100644
--- a/hw/virtio/trace-events
+++ b/hw/virtio/trace-events
@@ -163,3 +163,7 @@ virtio_snd_vm_state_running(void) "vm state running"
 virtio_snd_vm_state_stopped(void) "vm state stopped"
 virtio_snd_realize(void *snd) "snd %p: realize"
 virtio_snd_unrealize(void *snd) "snd %p: unrealize"
+virtio_snd_handle_ctrl(void *vdev, void *vq) "snd %p: handle ctrl event for 
queue %p"
+virtio_snd_handle_code(uint32_t val, const char *code) "ctrl code msg val = 
%"PRIu32" == %s"
+virtio_snd_handle_chmap_info(void) "VIRTIO_SND_CHMAP_INFO called"
+virtio_snd_handle_event(void) "event queue callback called"
diff --git a/hw/virtio/virtio-snd.c b/hw/virtio/virtio-snd.c
index 14bc32f476..705dc07212 100644
--- a/hw/virtio/virtio-snd.c
+++ b/hw/virtio/virtio-snd.c
@@ -32,6 +32,29 @@
 #define VIRTIO_SOUND_CHMAP_DEFAULT 0
 #define VIRTIO_SOUND_HDA_FN_NID 0
 
+static uint32_t supported_formats = BIT(VIRTIO_SND_PCM_FMT_S8)
+  | BIT(VIRTIO_SND_PCM_FMT_U8)
+  | BIT(VIRTIO_SND_PCM_FMT_S16)
+  | BIT(VIRTIO_SND_PCM_FMT_U16)
+  | BIT(VIRTIO_SND_PCM_FMT_S32)
+  | BIT(VIRTIO_SND_PCM_FMT_U32)
+  | BIT(VIRTIO_SND_PCM_FMT_FLOAT);
+
+static uint32_t supported_rates = BIT(VIRTIO_SND_PCM_RATE_5512)
+| BIT(VIRTIO_SND_PCM_RATE_8000)
+| BIT(VIRTIO_SND_PCM_RATE_11025)
+| BIT(VIRTIO_SND_PCM_RATE_16000)
+| BIT(VIRTIO_SND_PCM_RATE_22050)
+| BIT(VIRTIO_SND_PCM_RATE_32000)
+| BIT(VIRTIO_SND_PCM_RATE_44100)
+| BIT(VIRTIO_SND_PCM_RATE_48000)
+| BIT(VIRTIO_SND_PCM_RATE_64000)
+| BIT(VIRTIO_SND_PCM_RATE_88200)
+| BIT(VIRTIO_SND_PCM_RATE_96000)
+| BIT(VIRTIO_SND_PCM_RATE_176400)
+| BIT(VIRTIO_SND_PCM_RATE_192000)
+| BIT(VIRTIO_SND_PCM_RATE_384000);
+
 static const VMStateDescription vmstate_virtio_snd_device = {
 .name = TYPE_VIRTIO_SND,
 .version_id = VIRTIO_SOUND_VM_VERSION,
@@ -81,32 +104,416 @@ static void
 virtio_snd_set_config(VirtIODevice *vdev, const uint8_t *config)
 {
 VirtIOSound *s = VIRTIO_SND(vdev);
 const virtio_snd_config *sndconfig =
 (const virtio_snd_config *)config;
 
 
trace_virtio_snd_set_config(vdev,
s->snd_conf.jacks,
sndconfig->jacks,
s->snd_conf.streams,
sndconfig->streams,
s->snd_conf.chmaps,
sndconfig->chmaps);
 
 memcpy(&s->snd_conf, sndconfig, sizeof(virtio_snd_config));
 le32_to_cpus(&s->snd_conf.jacks);
 le32_to_cpus(&s->snd_conf.streams);
 le32_to_cpus(&s->snd_conf.chmaps);
 
 }
 
+static void
+virtio_snd_ctrl_cmd_free(virtio_snd_ctrl_command *cmd)
+{
+g_free(cmd->elem);
+g_free(cmd);
+}
+
+/*
+ * Get a specific stream from the virtio sound card device.
+ * Returns NULL if @stream_id is invalid or not allocated.
+ *
+ * @s: VirtIOSound device
+ * @stream_id: stream id
+ */
+static VirtIOSoundPCMStream *virtio_snd_pcm_get_stream(VirtIOSound *s,
+   uint32_t stream_id)
+{
+return stream_id >= s->snd_conf.streams ? NULL :
+s->pcm->streams[stream_id];
+}
+
+/*
+ * Get params for a specific stream.
+ *
+ * @s: VirtIOSound device
+ * @stream_id: stream id
+ */
+static virtio_snd_pcm_set_params *virtio_snd_pcm_get_params(VirtIOSound *s,
+uint32_t stream_id)
+{
+return stream_id >= s->snd_conf.streams ? NULL
+: &s-

[PATCH v11 08/11] virtio-sound: handle VIRTIO_SND_R_PCM_RELEASE

[Manos Pitsidianakis]

From: Emmanouil Pitsidianakis 

Handle the PCM release control request, which is necessary for flushing
pending sound IO. No IO is handled yet so currently it only replies to
the request.

Based-on: 
https://github.com/OpenSynergy/qemu/commit/5a2f350eec5d157b90d9c7b40a8e603f4da92471
Signed-off-by: Igor Skalkin 
Signed-off-by: Anton Yakovlev 
Signed-off-by: Emmanouil Pitsidianakis 
Reviewed-by: Alex Bennée 
---
 hw/virtio/trace-events |  1 +
 hw/virtio/virtio-snd.c | 48 +-
 2 files changed, 48 insertions(+), 1 deletion(-)

diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
index 7907b610c1..b0789a6e7e 100644
--- a/hw/virtio/trace-events
+++ b/hw/virtio/trace-events
@@ -167,6 +167,7 @@ virtio_snd_handle_pcm_set_params(uint32_t stream) 
"VIRTIO_SND_PCM_SET_PARAMS cal
 virtio_snd_handle_ctrl(void *vdev, void *vq) "snd %p: handle ctrl event for 
queue %p"
 virtio_snd_handle_pcm_info(uint32_t stream) "VIRTIO_SND_R_PCM_INFO called for 
stream %"PRIu32
 virtio_snd_handle_pcm_start_stop(const char *code, uint32_t stream) "%s called 
for stream %"PRIu32
+virtio_snd_handle_pcm_release(uint32_t stream) "VIRTIO_SND_PCM_RELEASE called 
for stream %"PRIu32
 virtio_snd_handle_code(uint32_t val, const char *code) "ctrl code msg val = 
%"PRIu32" == %s"
 virtio_snd_handle_chmap_info(void) "VIRTIO_SND_CHMAP_INFO called"
 virtio_snd_handle_event(void) "event queue callback called"
diff --git a/hw/virtio/virtio-snd.c b/hw/virtio/virtio-snd.c
index 49cd9f3ca4..ca873fd6d4 100644
--- a/hw/virtio/virtio-snd.c
+++ b/hw/virtio/virtio-snd.c
@@ -503,47 +503,93 @@ static void virtio_snd_handle_pcm_prepare(VirtIOSound *s,
 /*
  * Handles VIRTIO_SND_R_PCM_START.
  *
  * @s: VirtIOSound device
  * @cmd: The request command queue element from VirtIOSound cmdq field
  * @start: whether to start or stop the device
  */
 static void virtio_snd_handle_pcm_start_stop(VirtIOSound *s,
  virtio_snd_ctrl_command *cmd,
  bool start)
 {
 VirtIOSoundPCMStream *stream;
 virtio_snd_pcm_hdr req;
 uint32_t stream_id;
 size_t msg_sz = iov_to_buf(cmd->elem->out_sg,
cmd->elem->out_num,
0,
&req,
sizeof(virtio_snd_pcm_hdr));
 
 if (msg_sz != sizeof(virtio_snd_pcm_hdr)) {
 qemu_log_mask(LOG_GUEST_ERROR,
 "%s: virtio-snd command size incorrect %zu vs \
 %zu\n", __func__, msg_sz, sizeof(virtio_snd_pcm_hdr));
 cmd->resp.code = cpu_to_le32(VIRTIO_SND_S_BAD_MSG);
 return;
 }
 
 stream_id = le32_to_cpu(req.stream_id);
 cmd->resp.code = cpu_to_le32(VIRTIO_SND_S_OK);
 trace_virtio_snd_handle_pcm_start_stop(start ? "VIRTIO_SND_R_PCM_START" :
 "VIRTIO_SND_R_PCM_STOP", stream_id);
 stream = virtio_snd_pcm_get_stream(s, stream_id);
 if (stream == NULL) {
 error_report("Invalid stream id: %"PRIu32, req.stream_id);
 cmd->resp.code = cpu_to_le32(VIRTIO_SND_S_BAD_MSG);
 return;
 }
 stream->active = start;
 }
 
+/*
+ * Handles VIRTIO_SND_R_PCM_RELEASE. Releases the buffer resources allocated to
+ * a stream.
+ *
+ * @s: VirtIOSound device
+ * @cmd: The request command queue element from VirtIOSound cmdq field
+ */
+static void virtio_snd_handle_pcm_release(VirtIOSound *s,
+  virtio_snd_ctrl_command *cmd)
+{
+uint32_t stream_id;
+VirtIOSoundPCMStream *stream;
+size_t msg_sz = iov_to_buf(cmd->elem->out_sg,
+   cmd->elem->out_num,
+   sizeof(virtio_snd_hdr),
+   &stream_id,
+   sizeof(stream_id));
+
+if (msg_sz != sizeof(stream_id)) {
+/*
+ * TODO: do we need to set DEVICE_NEEDS_RESET?
+ */
+qemu_log_mask(LOG_GUEST_ERROR,
+"%s: virtio-snd command size incorrect %zu vs \
+%zu\n", __func__, msg_sz, sizeof(stream_id));
+cmd->resp.code = cpu_to_le32(VIRTIO_SND_S_BAD_MSG);
+return;
+}
+
+stream_id = le32_to_cpu(stream_id);
+trace_virtio_snd_handle_pcm_release(stream_id);
+stream = virtio_snd_pcm_get_stream(s, stream_id);
+if (stream == NULL) {
+/*
+ * TODO: do we need to set DEVICE_NEEDS_RESET?
+ */
+error_report("already released stream %"PRIu32, stream_id);
+virtio_error(VIRTIO_DEVICE(s),
+ "already released stream %"PRIu32,
+ stream_id);
+cmd->resp.code = cpu_to_le32(VIRTIO_SND_S_BAD_MSG);
+return;
+}
+cmd->resp.code = cpu_to_le32(VIRTIO_SND_S_OK);
+}
+
 /*
  * The actual processing done in virtio_snd_process_cmdq().
  *
  * @s: VirtIOSound device
  * @cmd: control command request
  */
@@ -551,74 +597,74 @@ static in

[PATCH v11 11/11] docs/system: add basic virtio-snd documentation

[Manos Pitsidianakis]

From: Emmanouil Pitsidianakis 

This commit adds basic documentation for using virtio-snd.

Signed-off-by: Emmanouil Pitsidianakis 
Reviewed-by: Alex Bennée 
---
 MAINTAINERS  |  1 +
 docs/system/device-emulation.rst |  1 +
 docs/system/devices/virtio-snd.rst (new) | 49 
 3 files changed, 51 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 701f12026a..a32e512a61 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2259,6 +2259,7 @@ M: Manos Pitsidianakis 
 S: Supported
 F: hw/virtio/virtio-snd*.c
 F: include/hw/virtio/virtio-snd.h
+F: docs/system/devices/virtio-snd.rst
 
 nvme
 M: Keith Busch 
diff --git a/docs/system/device-emulation.rst b/docs/system/device-emulation.rst
index 4491c4cbf7..dae19446e5 100644
--- a/docs/system/device-emulation.rst
+++ b/docs/system/device-emulation.rst
@@ -79,20 +79,21 @@ in a PCI slot to the exclusive use of the guest.
 Emulated Devices
 
 
 .. toctree::
:maxdepth: 1
 
devices/can.rst
devices/ccid.rst
devices/cxl.rst
devices/ivshmem.rst
devices/keyboard.rst
devices/net.rst
devices/nvme.rst
devices/usb.rst
devices/vhost-user.rst
devices/virtio-pmem.rst
+   devices/virtio-snd.rst
devices/vhost-user-rng.rst
devices/canokey.rst
devices/usb-u2f.rst
devices/igb.rst
diff --git a/docs/system/devices/virtio-snd.rst 
b/docs/system/devices/virtio-snd.rst
new file mode 100644
index 00..2a9187fd70
--- /dev/null
+++ b/docs/system/devices/virtio-snd.rst
@@ -0,0 +1,49 @@
+virtio sound
+
+
+This document explains the setup and usage of the Virtio sound device.
+The Virtio sound device is a paravirtualized sound card device.
+
+Linux kernel support
+
+
+Virtio sound requires a guest Linux kernel built with the
+``CONFIG_SND_VIRTIO`` option.
+
+Description
+---
+
+Virtio sound implements capture and playback from inside a guest using the
+configured audio backend of the host machine.
+
+Device properties
+-
+
+The Virtio sound device can be configured with the following properties:
+
+ * ``jacks`` number of physical jacks (Unimplemented).
+ * ``streams`` number of PCM streams. At the moment, no stream configuration 
is supported: the first one will always be a playback stream, an optional 
second will always be a capture stream. Adding more will cycle stream 
directions from playback to capture.
+ * ``chmaps`` number of channel maps (Unimplemented).
+
+All streams are stereo and have the default channel positions ``Front left, 
right``.
+
+Examples
+
+
+Add an audio device and an audio backend at once with ``-audio`` and 
``model=virtio``:
+
+ * pulseaudio: ``-audio driver=pa,model=virtio``
+   or ``-audio driver=pa,model=virtio,server=/run/user/1000/pulse/native``
+ * sdl: ``-audio driver=sdl,model=virtio``
+ * coreaudio: ``-audio driver=coreaudio,model=virtio``
+
+etc.
+
+To specifically add virtualized sound devices, you have to specify a PCI device
+and an audio backend listed with ``-audio driver=help`` that works on your host
+machine, e.g.:
+
+::
+
+  -device virtio-sound-pci,audiodev=my_audiodev \
+  -audiodev alsa,id=my_audiodev
-- 
2.39.2

[PATCH v11 10/11] virtio-sound: implement audio capture (RX)

[Manos Pitsidianakis]

From: Emmanouil Pitsidianakis 

To perform audio capture we duplicate the TX logic of the previous
commit with the following difference: we receive data from the QEMU
audio backend and write it in the virt queue IO buffers the guest sends
to QEMU. When they are full (i.e. they have `period_bytes` amount of
data) or when recording stops in QEMU's audio backend, the buffer is
returned to the guest by notifying it.

Signed-off-by: Emmanouil Pitsidianakis 
Reviewed-by: Alex Bennée 
---
 hw/virtio/trace-events |   3 +-
 hw/virtio/virtio-snd.c | 262 +++--
 2 files changed, 230 insertions(+), 35 deletions(-)

diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
index a31531970d..56a2f44981 100644
--- a/hw/virtio/trace-events
+++ b/hw/virtio/trace-events
@@ -172,4 +172,5 @@ virtio_snd_handle_code(uint32_t val, const char *code) 
"ctrl code msg val = %"PR
 virtio_snd_handle_chmap_info(void) "VIRTIO_SND_CHMAP_INFO called"
 virtio_snd_handle_event(void) "event queue callback called"
 virtio_snd_pcm_stream_flush(uint32_t stream) "flushing stream %"PRIu32
-virtio_snd_handle_xfer(void) "tx/rx queue callback called"
+virtio_snd_handle_tx_xfer(void) "tx queue callback called"
+virtio_snd_handle_rx_xfer(void) "rx queue callback called"
diff --git a/hw/virtio/virtio-snd.c b/hw/virtio/virtio-snd.c
index 5e9513fb26..636ea08c1a 100644
--- a/hw/virtio/virtio-snd.c
+++ b/hw/virtio/virtio-snd.c
@@ -1,40 +1,41 @@
 /*
  * VIRTIO Sound Device conforming to
  *
  * "Virtual I/O Device (VIRTIO) Version 1.2
  * Committee Specification Draft 01
  * 09 May 2022"
  *
  * 

  *
  * Copyright (c) 2023 Emmanouil Pitsidianakis 
  * Copyright (C) 2019 OpenSynergy GmbH
  *
  * This work is licensed under the terms of the GNU GPL, version 2 or
  * (at your option) any later version.  See the COPYING file in the
  * top-level directory.
  */
 
 #include "qemu/osdep.h"
 #include "qemu/iov.h"
 #include "qemu/log.h"
 #include "qemu/error-report.h"
 #include "include/qemu/lockable.h"
 #include "sysemu/runstate.h"
 #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
-#define VIRTIO_SOUND_STREAM_DEFAULT 1
+#define VIRTIO_SOUND_STREAM_DEFAULT 2
 #define VIRTIO_SOUND_CHMAP_DEFAULT 0
 #define VIRTIO_SOUND_HDA_FN_NID 0
 
 static void virtio_snd_pcm_out_cb(void *data, int available);
 static void virtio_snd_process_cmdq(VirtIOSound *s);
 static void virtio_snd_pcm_flush(VirtIOSoundPCMStream *stream);
+static void virtio_snd_pcm_in_cb(void *data, int available);
 
 static uint32_t supported_formats = BIT(VIRTIO_SND_PCM_FMT_S8)
   | BIT(VIRTIO_SND_PCM_FMT_U8)
@@ -403,87 +404,96 @@ static void virtio_snd_get_qemu_audsettings(audsettings 
*as,
 /*
  * Close a stream and free all its resources.
  *
  * @stream: VirtIOSoundPCMStream *stream
  */
 static void virtio_snd_pcm_close(VirtIOSoundPCMStream *stream)
 {
 if (stream) {
+virtio_snd_pcm_flush(stream);
 if (stream->info.direction == VIRTIO_SND_D_OUTPUT) {
-virtio_snd_pcm_flush(stream);
 AUD_close_out(&stream->pcm->snd->card, stream->voice.out);
 stream->voice.out = NULL;
+} else if (stream->info.direction == VIRTIO_SND_D_INPUT) {
+AUD_close_in(&stream->pcm->snd->card, stream->voice.in);
+stream->voice.in = NULL;
 }
 }
 }
 
 /*
  * Prepares a VirtIOSound card stream.
  * Returns the response status code. (VIRTIO_SND_S_*).
  *
  * @s: VirtIOSound device
  * @stream_id: stream id
  */
 static uint32_t virtio_snd_pcm_prepare(VirtIOSound *s, uint32_t stream_id)
 {
 audsettings as;
 virtio_snd_pcm_set_params *params;
 VirtIOSoundPCMStream *stream;
 
 if (s->pcm->streams == NULL ||
 s->pcm->pcm_params == NULL ||
 stream_id >= s->snd_conf.streams) {
 return cpu_to_le32(VIRTIO_SND_S_BAD_MSG);
 }
 
 params = virtio_snd_pcm_get_params(s, stream_id);
 if (params == NULL) {
 return cpu_to_le32(VIRTIO_SND_S_BAD_MSG);
 }
 
 stream = virtio_snd_pcm_get_stream(s, stream_id);
 if (stream == NULL) {
 stream = g_new0(VirtIOSoundPCMStream, 1);
 stream->active = false;
 stream->id = stream_id;
 stream->pcm = s->pcm;
 stream->s = s;
 qemu_mutex_init(&stream->queue_mutex);
 QSIMPLEQ_INIT(&stream->queue);
 QSIMPLEQ_INIT(&stream->invalid);
 
 /*
  * stream_id >= s->snd_conf.streams was checked before so this is
  * in-bounds
  */
 s->pcm->streams[stream_id] = stream;
 }
 
 virtio_snd_get_qemu_audsettings(&as, params);
 stream->info.direction = stream_id < s->snd_conf.streams / 2 +
 (s->snd_conf.streams & 1) ? VIRTIO_SND_D_OUTPUT : VIRTIO_SND_D_INPUT;
 st

[PATCH v11 01/11] Add virtio-sound device stub

[Manos Pitsidianakis]

From: Emmanouil Pitsidianakis 

Add a new VIRTIO device for the virtio sound device id. Functionality
will be added in the following commits.

Based-on: 
https://github.com/OpenSynergy/qemu/commit/5a2f350eec5d157b90d9c7b40a8e603f4da92471
Reviewed-by: Alex Bennée 
Signed-off-by: Igor Skalkin 
Signed-off-by: Anton Yakovlev 
Signed-off-by: Emmanouil Pitsidianakis 
---
 MAINTAINERS  |   6 +
 hw/virtio/Kconfig|   5 +
 hw/virtio/meson.build|   1 +
 hw/virtio/trace-events   |   9 ++
 hw/virtio/virtio-snd.c (new) | 223 +++
 include/hw/virtio/virtio-snd.h (new) |  79 ++
 6 files changed, 323 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 9e7dec4a58..701f12026a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2254,6 +2254,12 @@ F: hw/virtio/virtio-mem-pci.h
 F: hw/virtio/virtio-mem-pci.c
 F: include/hw/virtio/virtio-mem.h
 
+virtio-snd
+M: Manos Pitsidianakis 
+S: Supported
+F: hw/virtio/virtio-snd*.c
+F: include/hw/virtio/virtio-snd.h
+
 nvme
 M: Keith Busch 
 M: Klaus Jensen 
diff --git a/hw/virtio/Kconfig b/hw/virtio/Kconfig
index 92c9cf6c96..d6f20657b3 100644
--- a/hw/virtio/Kconfig
+++ b/hw/virtio/Kconfig
@@ -17,6 +17,11 @@ config VIRTIO_PCI
 depends on PCI
 select VIRTIO
 
+config VIRTIO_SND
+bool
+default y
+depends on VIRTIO
+
 config VIRTIO_MMIO
 bool
 select VIRTIO
diff --git a/hw/virtio/meson.build b/hw/virtio/meson.build
index c0055a7832..d0572b298c 100644
--- a/hw/virtio/meson.build
+++ b/hw/virtio/meson.build
@@ -35,6 +35,7 @@ specific_virtio_ss.add(when: 'CONFIG_VHOST_VSOCK', if_true: 
files('vhost-vsock.c
 specific_virtio_ss.add(when: 'CONFIG_VHOST_USER_VSOCK', if_true: 
files('vhost-user-vsock.c'))
 specific_virtio_ss.add(when: 'CONFIG_VIRTIO_RNG', if_true: 
files('virtio-rng.c'))
 specific_virtio_ss.add(when: 'CONFIG_VIRTIO_MEM', if_true: 
files('virtio-mem.c'))
+specific_virtio_ss.add(when: 'CONFIG_VIRTIO_SND', if_true: 
files('virtio-snd.c'))
 specific_virtio_ss.add(when: 'CONFIG_VHOST_USER_I2C', if_true: 
files('vhost-user-i2c.c'))
 specific_virtio_ss.add(when: 'CONFIG_VHOST_USER_RNG', if_true: 
files('vhost-user-rng.c'))
 specific_virtio_ss.add(when: 'CONFIG_VHOST_USER_GPIO', if_true: 
files('vhost-user-gpio.c'))
diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
index 1cb9027d1e..96b997f427 100644
--- a/hw/virtio/trace-events
+++ b/hw/virtio/trace-events
@@ -154,3 +154,12 @@ virtio_pmem_flush_done(int type) "fsync return=%d"
 virtio_gpio_start(void) "start"
 virtio_gpio_stop(void) "stop"
 virtio_gpio_set_status(uint8_t status) "0x%x"
+
+#virtio-snd.c
+virtio_snd_get_config(void *vdev, uint32_t jacks, uint32_t streams, uint32_t 
chmaps) "snd %p: get_config jacks=%"PRIu32" streams=%"PRIu32" chmaps=%"PRIu32""
+virtio_snd_set_config(void *vdev, uint32_t jacks, uint32_t new_jacks, uint32_t 
streams, uint32_t new_streams, uint32_t chmaps, uint32_t new_chmaps) "snd %p: 
set_config jacks from %"PRIu32"->%"PRIu32", streams from %"PRIu32"->%"PRIu32", 
chmaps from %"PRIu32"->%"PRIu32
+virtio_snd_get_features(void *vdev, uint64_t features) "snd %p: get_features 
0x%"PRIx64
+virtio_snd_vm_state_running(void) "vm state running"
+virtio_snd_vm_state_stopped(void) "vm state stopped"
+virtio_snd_realize(void *snd) "snd %p: realize"
+virtio_snd_unrealize(void *snd) "snd %p: unrealize"
diff --git a/hw/virtio/virtio-snd.c b/hw/virtio/virtio-snd.c
new file mode 100644
index 00..adce9f371e
--- /dev/null
+++ b/hw/virtio/virtio-snd.c
@@ -0,0 +1,223 @@
+/*
+ * VIRTIO Sound Device conforming to
+ *
+ * "Virtual I/O Device (VIRTIO) Version 1.2
+ * Committee Specification Draft 01
+ * 09 May 2022"
+ *
+ * 
<https://docs.oasis-open.org/virtio/virtio/v1.2/csd01/virtio-v1.2-csd01.html#x1-52900014>
+ *
+ * Copyright (c) 2023 Emmanouil Pitsidianakis 
+ * Copyright (C) 2019 OpenSynergy GmbH
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or
+ * (at your option) any later version.  See the COPYING file in the
+ * top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/iov.h"
+#include "qemu/log.h"
+#include "qemu/error-report.h"
+#include "include/qemu/lockable.h"
+#include "sysemu/runstate.h"
+#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
+#define VIRTIO_SOUND_STREAM_DEFAULT 1
+#define VIRTIO_SOUND_CHMAP_DEFAULT 0
+#defin

[PATCH v11 02/11] Add virtio-sound-pci device

[Manos Pitsidianakis]

From: Emmanouil Pitsidianakis 

This patch adds a PCI wrapper device for the virtio-sound device.
It is necessary to instantiate a virtio-snd device in a guest.
All sound logic will be added to the virtio-snd device in the following
commits.

To add this device with a guest, you'll need a >=5.13 kernel compiled
with CONFIG_SND_VIRTIO=y, which at the time of writing most distros have
off by default.

Use with following flags in the invocation:

Pulseaudio:
  -audio driver=pa,model=virtio
  or
  -audio driver=pa,model=virtio,server=/run/user/1000/pulse/native
sdl:
  -audio driver=sdl,model=virtio
coreaudio (macos/darwin):
  -audio driver=coreaudio,model=virtio
etc.

Based-on: 
https://github.com/OpenSynergy/qemu/commit/5a2f350eec5d157b90d9c7b40a8e603f4da92471
Reviewed-by: Alex Bennée 
Signed-off-by: Igor Skalkin 
Signed-off-by: Anton Yakovlev 
Signed-off-by: Emmanouil Pitsidianakis 
---
 hw/virtio/meson.build|  1 +
 hw/virtio/virtio-snd-pci.c (new) | 93 
 hw/virtio/virtio-snd.c   | 14 -
 system/qdev-monitor.c|  1 +
 4 files changed, 107 insertions(+), 2 deletions(-)

diff --git a/hw/virtio/meson.build b/hw/virtio/meson.build
index d0572b298c..6233795018 100644
--- a/hw/virtio/meson.build
+++ b/hw/virtio/meson.build
@@ -67,6 +67,7 @@ virtio_pci_ss.add(when: 'CONFIG_VIRTIO_SERIAL', if_true: 
files('virtio-serial-pc
 virtio_pci_ss.add(when: 'CONFIG_VIRTIO_PMEM', if_true: 
files('virtio-pmem-pci.c'))
 virtio_pci_ss.add(when: 'CONFIG_VIRTIO_IOMMU', if_true: 
files('virtio-iommu-pci.c'))
 virtio_pci_ss.add(when: 'CONFIG_VIRTIO_MEM', if_true: 
files('virtio-mem-pci.c'))
+virtio_pci_ss.add(when: 'CONFIG_VIRTIO_SND', if_true: 
files('virtio-snd-pci.c'))
 virtio_pci_ss.add(when: 'CONFIG_VHOST_VDPA_DEV', if_true: 
files('vdpa-dev-pci.c'))
 virtio_pci_ss.add(when: 'CONFIG_VIRTIO_MD', if_true: files('virtio-md-pci.c'))
 
diff --git a/hw/virtio/virtio-snd-pci.c b/hw/virtio/virtio-snd-pci.c
new file mode 100644
index 00..afe50a5354
--- /dev/null
+++ b/hw/virtio/virtio-snd-pci.c
@@ -0,0 +1,93 @@
+/*
+ * VIRTIO Sound Device PCI Bindings
+ *
+ * Copyright (c) 2023 Emmanouil Pitsidianakis 
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or
+ * (at your option) any later version.  See the COPYING file in the
+ * top-level directory.
+ */
+
+#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"
+
+/*
+ * virtio-snd-pci: This extends VirtioPCIProxy.
+ */
+#define TYPE_VIRTIO_SND_PCI "virtio-sound-pci"
+OBJECT_DECLARE_SIMPLE_TYPE(VirtIOSoundPCI, VIRTIO_SND_PCI)
+
+struct VirtIOSoundPCI {
+VirtIOPCIProxy parent_obj;
+
+VirtIOSound vdev;
+};
+
+static Property virtio_snd_pci_properties[] = {
+DEFINE_PROP_BIT("ioeventfd", VirtIOPCIProxy, flags,
+VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT, true),
+DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 2),
+DEFINE_PROP_END_OF_LIST(),
+};
+
+static void virtio_snd_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
+{
+VirtIOSoundPCI *dev = VIRTIO_SND_PCI(vpci_dev);
+DeviceState *vdev = DEVICE(&dev->vdev);
+
+virtio_pci_force_virtio_1(vpci_dev);
+qdev_realize(vdev, BUS(&vpci_dev->bus), errp);
+}
+
+static void virtio_snd_pci_class_init(ObjectClass *klass, void *data)
+{
+DeviceClass *dc = DEVICE_CLASS(klass);
+VirtioPCIClass *vpciklass = VIRTIO_PCI_CLASS(klass);
+
+device_class_set_props(dc, virtio_snd_pci_properties);
+dc->desc = "Virtio Sound";
+set_bit(DEVICE_CATEGORY_SOUND, dc->categories);
+
+vpciklass->realize = virtio_snd_pci_realize;
+}
+
+static void virtio_snd_pci_instance_init(Object *obj)
+{
+VirtIOSoundPCI *dev = VIRTIO_SND_PCI(obj);
+
+virtio_instance_init_common(obj, &dev->vdev, sizeof(dev->vdev),
+TYPE_VIRTIO_SND);
+}
+
+static const VirtioPCIDeviceTypeInfo virtio_snd_pci_info = {
+.generic_name  = TYPE_VIRTIO_SND_PCI,
+.instance_size = sizeof(VirtIOSoundPCI),
+.instance_init = virtio_snd_pci_instance_init,
+.class_init= virtio_snd_pci_class_init,
+};
+
+/* Create a Virtio Sound PCI device, so '-audio driver,model=virtio' works. */
+static int virtio_snd_pci_init(PCIBus *bus, const char *audiodev)
+{
+DeviceState *vdev = NULL;
+VirtIOSoundPCI *dev = NULL;
+
+vdev = qdev_new(TYPE_VIRTIO_SND_PCI);
+assert(vdev);
+dev = VIRTIO_SND_PCI(vdev);
+qdev_prop_set_string(DEVICE(&dev->vdev), "audiodev", audiodev);
+qdev_realize_and_unref(vdev, BUS(bus), &error_fatal);
+return 0;
+}
+
+static void virtio_snd_pci_register(void)
+{
+virtio_pci_types_register(&virtio_snd_pci_info);
+pci_register_soundhw("virtio", "Virtio Sound", virtio_snd_pci_init);
+}
+
+type_init(virtio_snd_pci_register);
diff --git a/hw/virtio/virtio-snd.c b/hw/virtio/virtio-snd.c
index adce9f371e..14bc32f476 100644
--- a/hw/virti

[PATCH v11 00/11] Add VIRTIO sound card

[Manos Pitsidianakis]

From: Emmanouil Pitsidianakis 

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

v11 can be found online at:

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

Ref 885b01fe272541fdab5583780d4c3a59bfd8e734

Main differences with v10 patch series [^v10]
:

- Rebased against upstream, which has minor changes to the AUD_* API.
- Fixed noise in playback because of invalid bounds when accessing the 
  audio data in the VirtQueueElement.
- Refactor invalid I/O message queue flushing into separate function.
- Removed attempt to write unwritten bytes to QEMU sound backend when 
  flushing: it should only happen when the stream STARTs.
- Set latency_bytes to buffer size when returning TX I/O message because 
  it happens immediately after writing the last bytes to the QEMU 
  backend, therefore there might be up to  bytes to be 
  played before all the buffer data has finished playing.
- Addressed [^v10] review comments:
  - Refactored VirtIOSoundPCMBuffer return code into a function instead 
of using goto labels in output/input audio callbacks. (Suggested by 
)

Previously:

[^v10]: 
https://lore.kernel.org/qemu-devel/cover.1695996196.git.manos.pitsidiana...@linaro.org/
[^v9]: 
https://lore.kernel.org/qemu-devel/cover.1694588927.git.manos.pitsidiana...@linaro.org/
[^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 (11):
  Add virtio-sound device stub
  Add virtio-sound-pci device
  virtio-sound: handle control messages and streams
  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  |7 +
 docs/system/device-emulation.rst |1 +
 docs/system/devices/virtio-snd.rst (new) |   49 +
 hw/virtio/Kconfig|5 +
 hw/virtio/meson.build|2 +
 hw/virtio/trace-events   |   20 +
 hw/virtio/virtio-snd-pci.c (new) |   93 ++
 hw/virtio/virtio-snd.c (new) | 1409 ++
 include/hw/virtio/virtio-snd.h (new) |  235 
 system/qdev-monitor.c|1 +
 10 files changed, 1822 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 v10:
 1:  6e7bdf6dda !  1:  03ecf1f615 Add virtio-sound device stub
@@ hw/virtio/virtio-snd.c (new)
 +return;
 +}
 +
-+AUD_register_card("virtio-sound", &vsnd->card);
++AUD_register_card("virtio-sound", &vsnd->card, errp);
 +
 +vsnd->queues[VIRTIO_SND_VQ_CONTROL] =
 +virtio_add_queue(vdev, 64, virtio_snd_handle_queue);
 2:  82138b9c7d !  2:  ba49f45eb3 Add virtio-sound-pci device
@@ hw/virtio/virtio-snd.c: virtio_snd_set_config(VirtIODevice *vdev, const 
uint8_t
  
  /*
 
- ## softmmu/qdev-monitor.c ##
-@@ softmmu/qdev-monitor.c: static const QDevAlias qdev_alias_table[] = {
+ ## system/qdev-monitor.c ##
+@@ system/qdev-monitor.c: static const QDevAlias qdev_alias_table[] = {
  { "virtio-serial-device", "virtio-serial", QEMU_ARCH_VIRTIO_MMIO },
  { "virtio-serial-ccw", "virtio-serial", QEMU_ARCH_VIRTIO_CCW },
  { "virtio-serial-pci", "virtio-serial", QEMU_ARCH_VIRTIO_PCI},
 3:  c1a2cb0304 !  3:  5831b5cfa5 virtio-sound: handle control messages and 
streams
@@ hw/virtio/virtio-snd.c: static void virtio_snd_realize(DeviceState *dev, 
Error *
  
 @@ hw/virtio/virtio-snd.c: static void virtio_snd_realize(DeviceState 
*dev, Error **errp)
  
- AUD_register_card("virtio-sound", &vsnd->card);
+ AUD_register_card("virtio-sound", &vsnd->card, errp);
  
 +/* set default params for all streams */
 +default_params.features = 0;
 4:  28b2ecfa1f =  4:  425cbc2986 virtio-sound: handle VIRTIO_SND_R_PCM_INFO 
request
 5:  a52d20b2c3 =  5:  d1403721fa virtio-sound: handle 
VIRTIO_SND_R_PCM_{START,STOP}
 6:  25fbb2eb25 =  6:  68ac43df35 virtio-sou

Re: [PATCH v4 6/6] docs/system: add a basic enumeration of vhost-user devices

[Manos Pitsidianakis]

On Mon, 09 Oct 2023 12:59, Alex Bennée  wrote:
diff --git a/docs/system/devices/vhost-user.rst 
b/docs/system/devices/vhost-user.rst

index a80e95a48a..0f9eec3f00 100644
--- a/docs/system/devices/vhost-user.rst
+++ b/docs/system/devices/vhost-user.rst
@@ -15,6 +15,47 @@ to the guest. The code is mostly boilerplate although each 
device has
a ``chardev`` option which specifies the ID of the ``--chardev``
device that connects via a socket to the vhost-user *daemon*.

+Each device will have an virtio-mmio and virtio-pci variant. See your
+platform details for what sort of virtio bus to use.
+
+.. list-table:: vhost-user devices
+  :widths: 20 20 60
+  :header-rows: 1
+
+  * - Device
+- Type
+- Notes
+  * - vhost-user-device
+- Generic Development Device
+- You must manually specify ``virtio-id`` and the correct ``num_vqs``. 
Intended for expert use.


May be worth specifying they are `VHostUserBase` interface fields since 
it's not directly obvious if you come across this before reading the 
vhost-user-device code.



+  * - vhost-user-blk
+- Block storage
+-
+  * - vhost-user-fs
+- File based storage driver
+- See https://gitlab.com/virtio-fs/virtiofsd
+  * - vhost-user-scsi
+- SCSI based storage
+- See contrib/vhost-user/scsi
+  * - vhost-user-gpio
+- Proxy gpio pins to host
+- See https://github.com/rust-vmm/vhost-device
+  * - vhost-user-i2c
+- Proxy i2c devices to host
+- See https://github.com/rust-vmm/vhost-device
+  * - vhost-user-input
+- Generic input driver
+- See contrib/vhost-user-input
+  * - vhost-user-rng
+- Entropy driver
+- :ref:`vhost_user_rng`
+  * - vhost-user-gpu
+- GPU driver
+-
+  * - vhost-user-vsock
+- Socket based communication
+- See https://github.com/rust-vmm/vhost-device
+


There's also:

- hw/virtio/vhost-user-scmi.c
- hw/virtio/vhost-user-snd.c

Re: [PATCH v4 2/6] hw/virtio: derive vhost-user-rng from vhost-user-base

[Manos Pitsidianakis]

On Mon, 09 Oct 2023 12:59, Alex Bennée  wrote:

diff --git a/hw/virtio/meson.build b/hw/virtio/meson.build
index 51c3f97c2d..d0b963199c 100644
--- a/hw/virtio/meson.build
+++ b/hw/virtio/meson.build
@@ -18,8 +18,15 @@ if have_vhost
# fixme - this really should be generic
specific_virtio_ss.add(files('vhost-user.c'))
system_virtio_ss.add(files('vhost-user-base.c'))
+
+# MMIO Stubs
system_virtio_ss.add(files('vhost-user-device.c'))
+system_virtio_ss.add(when: 'CONFIG_VHOST_USER_RNG', if_true: 
files('vhost-user-rng.c'))
+
+# PCI Stubs
system_virtio_ss.add(when: 'CONFIG_VIRTIO_PCI', if_true: 
files('vhost-user-device-pci.c'))
+system_virtio_ss.add(when: ['CONFIG_VIRTIO_PCI', 'CONFIG_VHOST_USER_RNG'],
+ if_true: files('vhost-user-rng-pci.c'))


Is there a reason why the target was moved to system_virtio_ss from 
virtio_pci_ss?



  endif
  if have_vhost_vdpa
system_virtio_ss.add(files('vhost-vdpa.c'))
@@ -34,10 +41,8 @@ specific_virtio_ss.add(when: 'CONFIG_VHOST_USER_FS', 
if_true: files('vhost-user-
specific_virtio_ss.add(when: 'CONFIG_VIRTIO_PMEM', if_true: 
files('virtio-pmem.c'))
specific_virtio_ss.add(when: 'CONFIG_VHOST_VSOCK', if_true: 
files('vhost-vsock.c'))
specific_virtio_ss.add(when: 'CONFIG_VHOST_USER_VSOCK', if_true: 
files('vhost-user-vsock.c'))
-specific_virtio_ss.add(when: 'CONFIG_VIRTIO_RNG', if_true: 
files('virtio-rng.c'))


Was this accidental? It's not added anywhere else, only deleted.

@@ -57,7 +61,6 @@ virtio_pci_ss.add(when: 'CONFIG_VHOST_USER_FS', 
if_true: files('vhost-user-fs-pc

virtio_pci_ss.add(when: 'CONFIG_VIRTIO_CRYPTO', if_true: 
files('virtio-crypto-pci.c'))
virtio_pci_ss.add(when: 'CONFIG_VIRTIO_INPUT_HOST', if_true: 
files('virtio-input-host-pci.c'))
virtio_pci_ss.add(when: 'CONFIG_VIRTIO_INPUT', if_true: 
files('virtio-input-pci.c'))
-virtio_pci_ss.add(when: 'CONFIG_VIRTIO_RNG', if_true: 
files('virtio-rng-pci.c'))
virtio_pci_ss.add(when: 'CONFIG_VIRTIO_BALLOON', if_true: 
files('virtio-balloon-pci.c'))
virtio_pci_ss.add(when: 'CONFIG_VIRTIO_9P', if_true: files('virtio-9p-pci.c'))
virtio_pci_ss.add(when: 'CONFIG_VIRTIO_SCSI', if_true: 
files('virtio-scsi-pci.c'))


Same here

Manos

Re: [PATCH v7 3/5] vhost-user-scsi: support reconnect to backend

[Manos Pitsidianakis]

Hello Li, I have some trivial style comments you could possibly address 
in a next version:


On Sun, 08 Oct 2023 12:12, Li Feng  wrote:

diff --git a/hw/scsi/vhost-user-scsi.c b/hw/scsi/vhost-user-scsi.c
index df6b66cc1a..5df24faff4 100644
--- a/hw/scsi/vhost-user-scsi.c
+++ b/hw/scsi/vhost-user-scsi.c
@@ -39,26 +39,56 @@ static const int user_feature_bits[] = {
VHOST_INVALID_FEATURE_BIT
};

+static int vhost_user_scsi_start(VHostUserSCSI *s, Error **errp)
+{
+VHostSCSICommon *vsc = VHOST_SCSI_COMMON(s);
+int ret;
+
+ret = vhost_scsi_common_start(vsc, errp);
+s->started_vu = (ret < 0 ? false : true);


-+s->started_vu = (ret < 0 ? false : true);
++s->started_vu = !(ret < 0);

static void vhost_user_scsi_set_status(VirtIODevice *vdev, uint8_t 
status)

{
VHostUserSCSI *s = (VHostUserSCSI *)vdev;
+DeviceState *dev = &s->parent_obj.parent_obj.parent_obj.parent_obj;


-+DeviceState *dev = &s->parent_obj.parent_obj.parent_obj.parent_obj;
++DeviceState *dev = DEVICE(vdev);


+static int vhost_user_scsi_connect(DeviceState *dev, Error **errp)
+{
+VirtIODevice *vdev = VIRTIO_DEVICE(dev);
+VHostUserSCSI *s = VHOST_USER_SCSI(vdev);
+VHostSCSICommon *vsc = VHOST_SCSI_COMMON(s);
+VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(dev);
+int ret = 0;
+
+if (s->connected) {
+return 0;
+}
+s->connected = true;
+
+vsc->dev.num_queues = vs->conf.num_queues;
+vsc->dev.nvqs = VIRTIO_SCSI_VQ_NUM_FIXED + vs->conf.num_queues;
+vsc->dev.vqs = s->vhost_vqs;
+vsc->dev.vq_index = 0;
+vsc->dev.backend_features = 0;
+
+ret = vhost_dev_init(&vsc->dev, &s->vhost_user, VHOST_BACKEND_TYPE_USER, 0,
+ errp);
+if (ret < 0) {
+return ret;
+}
+
+/* restore vhost state */
+if (virtio_device_started(vdev, vdev->status)) {
+ret = vhost_user_scsi_start(s, errp);
+if (ret < 0) {
+return ret;
+}
+}
+
+return 0;
+}



-+if (virtio_device_started(vdev, vdev->status)) {
-+ret = vhost_user_scsi_start(s, errp);
-+if (ret < 0) {
-+return ret;
-+}
-+}
-+
-+return 0;
-+}
++if (virtio_device_started(vdev, vdev->status)) {
++ret = vhost_user_scsi_start(s, errp);
++}
++
++return ret;
++}

[skipping..]


+static int vhost_user_scsi_realize_connect(VHostUserSCSI *s, Error **errp)
+{
+DeviceState *dev = &s->parent_obj.parent_obj.parent_obj.parent_obj;



-+DeviceState *dev = &s->parent_obj.parent_obj.parent_obj.parent_obj;
++DeviceState *dev = DEVICE(s);


diff --git a/include/hw/virtio/vhost-user-scsi.h 
b/include/hw/virtio/vhost-user-scsi.h
index 521b08e559..b405ec952a 100644
--- a/include/hw/virtio/vhost-user-scsi.h
+++ b/include/hw/virtio/vhost-user-scsi.h
@@ -29,6 +29,10 @@ OBJECT_DECLARE_SIMPLE_TYPE(VHostUserSCSI, VHOST_USER_SCSI)
struct VHostUserSCSI {
VHostSCSICommon parent_obj;
VhostUserState vhost_user;
+bool connected;
+bool started_vu;
+
+struct vhost_virtqueue *vhost_vqs;


+bool connected;
+bool started_vu;
-+
+struct vhost_virtqueue *vhost_vqs;

See 
https://www.qemu.org/docs/master/devel/style.html#qemu-object-model-declarations

The definition should look like:

struct VHostUserSCSI {
   VHostSCSICommon parent_obj;

   /* Properties */
   bool connected;
   bool started_vu;

   VhostUserState vhost_user;
   struct vhost_virtqueue *vhost_vqs;
}

Re: [PATCH v10 00/11] Add VIRTIO sound card

[Manos Pitsidianakis]

On Tue, 03 Oct 2023 17:10, "Michael S. Tsirkin"  wrote:

I hope you are also fixing the linux driver to fix this in a robust
way, yes?


Hello Michael,

You can find pertinent discussion on the virtio-comment list here:

https://lists.oasis-open.org/archives/virtio-comment/202309/threads.html#00175

Fixing it is on our radar but it may prove non-trivial if it requires 
changing how the virtio-snd driver interacts with the kernel alsa API. 

Meanwhile AFAIK the qemu device conforms to the virtio spec in this case 
both with the previous version behavior and the current one. The change 
was made to compensate for the linux driver's behavior, not because it 
was previously incorrect.


Manos

Re: [PATCH v10 09/11] virtio-sound: implement audio output (TX)

[Manos Pitsidianakis]

On Fri, 29 Sep 2023 17:08, Emmanouil Pitsidianakis 
 wrote:

Handle output IO messages in the transmit (TX) virtqueue.

[..]

+if (!stream->active) {
+/* Stream has stopped, so do not perform AUD_write. */
+goto return_tx_buffer;
+}

[..]

+return_tx_buffer:
+virtio_snd_pcm_status resp = { 0 };
+resp.status = cpu_to_le32(VIRTIO_SND_S_OK);



It seems I was too hasty to submit this patch. It does not build with 
clang on macos because it does not allow labels before declarations.


It needs the following changes to compile:

--- a/hw/virtio/virtio-snd.c
+++ b/hw/virtio/virtio-snd.c
@@ -1187,7 +1187,7 @@ static void virtio_snd_pcm_out_cb(void *data, int 
available)
buffer->offset += size;
available -= size;
if (buffer->size < 1) {
-return_tx_buffer:
+return_tx_buffer:;
virtio_snd_pcm_status resp = { 0 };
resp.status = cpu_to_le32(VIRTIO_SND_S_OK);
resp.latency_bytes = 0;


--- a/hw/virtio/virtio-snd.c
+++ b/hw/virtio/virtio-snd.c
@@ -1251,7 +1251,7 @@ static void virtio_snd_pcm_in_cb(void *data, int 
available)
buffer->size += size;
available -= size;
if (buffer->size >= stream->params.period_bytes) {
-return_rx_buffer:
+return_rx_buffer:;
resp.status = cpu_to_le32(VIRTIO_SND_S_OK);
resp.latency_bytes = 0;
/* Copy data -if any- to guest */


- Manos

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

[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 v8 03/12] virtio-sound: handle control messages and streams

[Manos Pitsidianakis]

On Mon, 04 Sep 2023 14:30, Philippe Mathieu-Daudé  wrote:

On 4/9/23 13:00, Manos Pitsidianakis wrote:
On Mon, 04 Sep 2023 13:46, Philippe Mathieu-Daudé  
wrote:

+    size_t sz = iov_to_buf(cmd->elem->out_sg,
+   cmd->elem->out_num,
+   0,
+   &cmd->ctrl,
+   sizeof(cmd->ctrl));
+    if (sz != sizeof(cmd->ctrl)) {
+    qemu_log_mask(LOG_GUEST_ERROR,
+    "%s: virtio-snd command size incorrect %zu vs \
+    %zu\n", __func__, sz, sizeof(cmd->ctrl));
+    return;
+    }
+
+    trace_virtio_snd_handle_code(cmd->ctrl.code,


IIUC the spec, this structure is in little endian, is that right?
So shouldn't swap various fields in this series?


Not sure about the answer to this. Need input from someone more 
knowledgeable in virtio.


You can test running a big-endian guest (m68k, s390x, sparc64) on your
little-endian host.


The linux driver uses le{32,64}_to_cpu for reading fields, if there's 
any problem then it would be with the host?

Re: [PATCH v8 03/12] virtio-sound: handle control messages and streams

[Manos Pitsidianakis]

On Mon, 04 Sep 2023 13:46, Philippe Mathieu-Daudé  wrote:

+size_t sz = iov_to_buf(cmd->elem->out_sg,
+   cmd->elem->out_num,
+   0,
+   &cmd->ctrl,
+   sizeof(cmd->ctrl));
+if (sz != sizeof(cmd->ctrl)) {
+qemu_log_mask(LOG_GUEST_ERROR,
+"%s: virtio-snd command size incorrect %zu vs \
+%zu\n", __func__, sz, sizeof(cmd->ctrl));
+return;
+}
+
+trace_virtio_snd_handle_code(cmd->ctrl.code,


IIUC the spec, this structure is in little endian, is that right?
So shouldn't swap various fields in this series?


Not sure about the answer to this. Need input from someone more 
knowledgeable in virtio.

Re: [PATCH v8 10/12] virtio-sound: implement audio output (TX)

[Manos Pitsidianakis]

On Mon, 04 Sep 2023 13:26, Philippe Mathieu-Daudé  wrote:

  /*
- * Handles VIRTIO_SND_R_PCM_RELEASE. Releases the buffer resources allocated to
- * a stream.
+ * Returns the number of I/O messages that are being processed.
+ *
+ * @stream: VirtIOSoundPCMStream
+ */
+static size_t virtio_snd_pcm_get_pending_io_msgs(VirtIOSoundPCMStream *stream)
+{
+VirtIOSoundPCMBlock *block;
+VirtIOSoundPCMBlock *next;
+size_t size = 0;
+
+WITH_QEMU_LOCK_GUARD(&stream->queue_mutex) {
+QSIMPLEQ_FOREACH_SAFE(block, &stream->queue, entry, next) {
+size += 1;


Can you add a comment explaining this magic size?


It's not magic, it's simply how many messages there are as explained in 
the function doc comment. This was previously bytes hence `size`. I will 
change the variable name to `count`.



+static void virtio_snd_handle_tx(VirtIODevice *vdev, VirtQueue *vq)
+{
+VirtIOSound *s = VIRTIO_SND(vdev);
+VirtIOSoundPCMStream *stream = NULL;
+VirtQueueElement *elem;
+size_t sz;
+virtio_snd_pcm_xfer hdr;
+virtio_snd_pcm_status resp = { 0 };


virtio_snd_pcm_status has multiple fields, so better zero-initialize
all of them with '{ }'.


I don't understand why, virtio_snd_pcm_status has two int fields hence { 
0 } zero-initializes all of them.



+/*
+ * AUD_* output callback.
+ *
+ * @data: VirtIOSoundPCMStream stream
+ * @available: number of bytes that can be written with AUD_write()
+ */
+static void virtio_snd_pcm_out_cb(void *data, int available)
+{
+VirtIOSoundPCMStream *stream = data;
+VirtIOSoundPCMBlock *block;
+VirtIOSoundPCMBlock *next;
+size_t size;
+
+WITH_QEMU_LOCK_GUARD(&stream->queue_mutex) {
+QSIMPLEQ_FOREACH_SAFE(block, &stream->queue, entry, next) {
+for (;;) {
+size = MIN(block->size, available);
+size = AUD_write(stream->voice.out,
+block->data + block->offset,
+size);


If AUD_write() returns 0, is this an infinite loop?


Hm since we have available > 0 bytes this wouldn't theoretically happen, 
but I see there are code paths that return 0 on bugs/failures, I will 
add the check.



+block->size -= size;
+block->offset += size;
+if (!block->size) {
+virtqueue_push(block->vq,
+block->elem,
+sizeof(block->elem));
+virtio_notify(VIRTIO_DEVICE(stream->s),
+block->vq);
+QSIMPLEQ_REMOVE_HEAD(&stream->queue, entry);
+g_free(block);
+available -= size;
+break;
+}
+
+available -= size;
+if (!available) {
+break;
+}
+}
+if (!available) {
+break;
+}
+}
+}
+}
+
+/*
+ * Flush all buffer data from this stream's queue into the driver's virtual
+ * queue.
+ *
+ * @stream: VirtIOSoundPCMStream *stream
+ */
+static void virtio_snd_pcm_flush(VirtIOSoundPCMStream *stream)
+{
+VirtIOSoundPCMBlock *block;
+VirtIOSoundPCMBlock *next;
+
+WITH_QEMU_LOCK_GUARD(&stream->queue_mutex) {
+QSIMPLEQ_FOREACH_SAFE(block, &stream->queue, entry, next) {
+AUD_write(stream->voice.out, block->data + block->offset, 
block->size);


Is it OK to ignore AUD_write() returning < block->size?
If so, can you add a comment please?


This is a flush event with a timeout so it should complete asap. As 
mentioned in another reply it might be better to copy the data to a 
buffer in order not to lose any audio bytes.


Thank you for the feedback,
Manos

Re: [PATCH v8 02/12] Add virtio-sound-pci device

[Manos Pitsidianakis]

On Mon, 04 Sep 2023 09:32, Volker Rümelin  wrote:

+static Property virtio_snd_pci_properties[] = {
+DEFINE_AUDIO_PROPERTIES(VirtIOSoundPCI, vdev.card),


I think DEFINE_AUDIO_PROPERTIES should be moved back to virtio-snd.c. 
The audiodev property is a virtio-sound property and not a 
virtio-sound-pci property.


Hm, is it? Can you instantiate a virtio-sound device without the PCI 
wrapper? Under hw/audio, DEFINE_AUDIO_PROPERTIES is set in PCI devices 
as well (e.g. ac97)





+DEFINE_PROP_BIT("ioeventfd", VirtIOPCIProxy, flags,
+VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT, true),
+DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors,
+   DEV_NVECTORS_UNSPECIFIED),
+DEFINE_PROP_END_OF_LIST(),
+};
+
+static void virtio_snd_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
+{
+VirtIOSoundPCI *dev = VIRTIO_SND_PCI(vpci_dev);
+DeviceState *vdev = DEVICE(&dev->vdev);
+
+if (vpci_dev->nvectors == DEV_NVECTORS_UNSPECIFIED) {
+vpci_dev->nvectors = 2;
+}


Why do you need that intermediate step with DEV_NVECTORS_UNSPECIFIED? 
Unlike e.g. virtio-scsi-pci and virtio-net-pci devices, the default 
value of nvectors is already known at compile time and can be specified 
in the property definition.


I did not think this through properly, you are correct. Thank you!

Manos

Re: [PATCH v8 05/12] virtio-sound: handle VIRTIO_SND_R_PCM_INFO request

[Manos Pitsidianakis]

On Mon, 04 Sep 2023 13:13, Philippe Mathieu-Daudé  wrote:

+
+pcm_info = g_new0(virtio_snd_pcm_info, req.count);
+for (uint32_t i = req.start_id; i < req.start_id + req.count; i++) {


Starting from req.start_id seems to increase this code complexity.


I see your point, will change it!

Manos

Re: [PATCH v8 03/12] virtio-sound: handle control messages and streams

[Manos Pitsidianakis]

Good morning Philippe,

On Mon, 04 Sep 2023 13:08, Philippe Mathieu-Daudé  wrote:

+iov_from_buf(cmd->elem->in_sg,
+ cmd->elem->in_num,
+ 0,
+ &cmd->resp,
+ sizeof(cmd->resp));
+virtqueue_push(cmd->vq, cmd->elem, sizeof(cmd->elem));
+virtio_notify(VIRTIO_DEVICE(s), cmd->vq);


I have very few understanding of virtio, but I'm wondering here,
since this function is called under cmdq_mutex(), could it be
useful to batch the queue by calling virtio_notify() only once
in the caller once the whole cmdq is processed ...


In the linux driver (sound/virtio/virtio_ctl_msg.c), the guest has a 
timeout for receiving the message. I found that if I did not notify as 
fast as possible, I got timeout errors on the guest.

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

[Manos Pitsidianakis]

Hello Volker :)

On Mon, 04 Sep 2023 10:20, Volker Rümelin  wrote:

All qemu_log_mask() format strings need a trailing \n.


Thank you, will fix it!

I still hear a lot of playback dropouts. I had planned to look at the 
playback code, but I didn't have the time until now.


Compared to v6 audio recording has improved but there are bugs. When I 
start QEMU with -audiodev 
pipewire,out.frequency=48000,in.frequency=48000,id=audio0 there are two 
either uninitialized or stale samples every 25ms in the recorded audio 
stream.


To reproduce the issue start audacity on the host and generate a 2s 
square wave tone with 315Hz and an amplitude of 0.8. Use pavucontrol to 
select the monitor of your host playback device as QEMU recording 
device. In the guest start recording with audacity. Start playback of 
the generated square wave on the host. Stop recording in the guest and 
have a look at a 200ms sequence of the recorded square wave and notice 
the wrong samples every 25ms.


We've noticed this and decided to fix it in the future. I think the 
problem lies when PCM release is called from the guest. Quoting the 
spec:


 The device MUST complete all pending I/O messages for the specified 
 stream ID.
 The device MUST NOT complete the control request while there are 
 pending I/O messages for the specified stream ID.


When RELEASE is received, buffers are simply dropped. This is pure 
conjecture but I think creating an in-device buffer could solve this.
Unless the bug is found to be caused by something else, I settled on 
accepting it for this patch series because it is spec conformant.


When I start QEMU with -audiodev 
pipewire,out.mixing-engine=off,in.mixing-engine=off,id=audio0 audio 
recording starts but the recorded stream immediately stalls.


Can you elaborate? Do you mean you repeat the same process as before, 
but the stall happens immediately? I personally rarely get any drops I 
could notice, only one or two for many minutes of playback / capture. I 
also could not reproduce exactly the same behavior you had in the 
previous version. The bugs *were* there but it was not as severe. Maybe 
it's a hardware performance issue? Can someone else test this too? It'd 
be helpful.


Thank you very much for your help,
Manos

Re: [PATCH v7 10/12] virtio-sound: implement audio output (TX)

[Manos Pitsidianakis]

Hello Philippe,

On Wed, 23 Aug 2023 01:17, Philippe Mathieu-Daudé  wrote:

+trace_virtio_snd_handle_xfer();


Maybe ...

<- snip ->

... it is more useful displaying the callback stream index here?

   trace_virtio_snd_handle_xfer(hdr.stream_id);



Certainly, why not. This is an extremely noisy trace print by the way, I 
was not sure whether to keep it or not. Printing it inside the for-loop 
would potentially make it more noisy.


Would you say this warrants a new patch series version? Otherwise I will 
include it if other things come up as well.


Manos

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

[Manos Pitsidianakis]

Hello Volker,

On Sun, 20 Aug 2023 14:46, Volker Rümelin  wrote:
I tested the virtio-sound-pci device. It seems the device works 
unreliably. Audio playback has a lot of dropouts. I can actually hear 
my mouse moving around. Audio recording with audacity doesn't work. 
Either recording stops with an error or the recorded stream is silent.


I'll see if I can change the code so audio playback works reliably. I 
don't think it makes sense to review the current code as it is. I will 
of course report any issues I find.


have you been having this bad performance with pulseaudio/pipewire? Are 
you using alsa for playback/recording in the guest?


I am asking because this was my setup and I was wondering if it affected 
the code I ended up with. For me I had normal playback, except for a 
short delay at first (maybe something to do with alsa buffer lengths, I 
am not familiar with ALSA much).


If you can share your guest and host setup you used for this I can try 
replicating it.


Manos

Re: [PATCH v6 01/12] Add virtio-sound device stub

[Manos Pitsidianakis]

Hello Volker!

On Sun, 20 Aug 2023 12:33, Volker Rümelin  wrote:
I think the virtio-snd.c code, the trace events and the Kconfig 
VIRTIO_SND should be moved to hw/audio. The code for nearly all audio 
devices is in this directory. This would be similar to other virtio 
devices. E.g. the virtio-scsi code is in hw/scsi and the virtio-net 
code is in hw/net.


This was where it was initially but in previous patchset versions it was 
recommended to move them to hw/virtio. I don't mind either approach 
though.


Manos

Re: [PATCH v5 9/9] docs/system: add basic virtio-gpu documentation

[Manos Pitsidianakis]

Reviewed-by: Emmanouil Pitsidianakis 

On Tue, 15 Aug 2023 03:35, Gurchetan Singh  wrote:

This adds basic documentation for virtio-gpu.

Suggested-by: Akihiko Odaki 
Signed-off-by: Gurchetan Singh 
---
v2: - Incorporated suggestions by Akihiko Odaki
   - Listed the currently supported capset_names (Bernard)

v3: - Incorporated suggestions by Akihiko Odaki and Alyssa Ross

v4: - Incorporated suggestions by Akihiko Odaki

v5: - Removed pci suffix from examples
   - Verified that -device virtio-gpu-rutabaga works.  Strangely
 enough, I don't remember changing anything, and I remember
 it not working.  I did rebase to top of tree though.
   - Fixed meson examples in crosvm docs
docs/system/device-emulation.rst   |   1 +
docs/system/devices/virtio-gpu.rst | 113 +
2 files changed, 114 insertions(+)
create mode 100644 docs/system/devices/virtio-gpu.rst

diff --git a/docs/system/device-emulation.rst b/docs/system/device-emulation.rst
index 4491c4cbf7..1167f3a9f2 100644
--- a/docs/system/device-emulation.rst
+++ b/docs/system/device-emulation.rst
@@ -91,6 +91,7 @@ Emulated Devices
   devices/nvme.rst
   devices/usb.rst
   devices/vhost-user.rst
+   devices/virtio-gpu.rst
   devices/virtio-pmem.rst
   devices/vhost-user-rng.rst
   devices/canokey.rst
diff --git a/docs/system/devices/virtio-gpu.rst 
b/docs/system/devices/virtio-gpu.rst
new file mode 100644
index 00..8c5c708272
--- /dev/null
+++ b/docs/system/devices/virtio-gpu.rst
@@ -0,0 +1,113 @@
+..
+   SPDX-License-Identifier: GPL-2.0
+
+virtio-gpu
+==
+
+This document explains the setup and usage of the virtio-gpu device.
+The virtio-gpu device paravirtualizes the GPU and display controller.
+
+Linux kernel support
+
+
+virtio-gpu requires a guest Linux kernel built with the
+``CONFIG_DRM_VIRTIO_GPU`` option.
+
+QEMU virtio-gpu variants
+
+
+QEMU virtio-gpu device variants come in the following form:
+
+ * ``virtio-vga[-BACKEND]``
+ * ``virtio-gpu[-BACKEND][-INTERFACE]``
+ * ``vhost-user-vga``
+ * ``vhost-user-pci``
+
+**Backends:** QEMU provides a 2D virtio-gpu backend, and two accelerated
+backends: virglrenderer ('gl' device label) and rutabaga_gfx ('rutabaga'
+device label).  There is a vhost-user backend that runs the graphics stack
+in a separate process for improved isolation.
+
+**Interfaces:** QEMU further categorizes virtio-gpu device variants based
+on the interface exposed to the guest. The interfaces can be classified
+into VGA and non-VGA variants. The VGA ones are prefixed with virtio-vga
+or vhost-user-vga while the non-VGA ones are prefixed with virtio-gpu or
+vhost-user-gpu.
+
+The VGA ones always use the PCI interface, but for the non-VGA ones, the
+user can further pick between MMIO or PCI. For MMIO, the user can suffix
+the device name with -device, though vhost-user-gpu does not support MMIO.
+For PCI, the user can suffix it with -pci. Without these suffixes, the
+platform default will be chosen.
+
+virtio-gpu 2d
+-
+
+The default 2D backend only performs 2D operations. The guest needs to
+employ a software renderer for 3D graphics.
+
+Typically, the software renderer is provided by `Mesa`_ or `SwiftShader`_.
+Mesa's implementations (LLVMpipe, Lavapipe and virgl below) work out of box
+on typical modern Linux distributions.
+
+.. parsed-literal::
+-device virtio-gpu
+
+.. _Mesa: https://www.mesa3d.org/
+.. _SwiftShader: https://github.com/google/swiftshader
+
+virtio-gpu virglrenderer
+
+
+When using virgl accelerated graphics mode in the guest, OpenGL API calls
+are translated into an intermediate representation (see `Gallium3D`_). The
+intermediate representation is communicated to the host and the
+`virglrenderer`_ library on the host translates the intermediate
+representation back to OpenGL API calls.
+
+.. parsed-literal::
+-device virtio-gpu-gl
+
+.. _Gallium3D: https://www.freedesktop.org/wiki/Software/gallium/
+.. _virglrenderer: https://gitlab.freedesktop.org/virgl/virglrenderer/
+
+virtio-gpu rutabaga
+---
+
+virtio-gpu can also leverage `rutabaga_gfx`_ to provide `gfxstream`_
+rendering and `Wayland display passthrough`_.  With the gfxstream rendering
+mode, GLES and Vulkan calls are forwarded to the host with minimal
+modification.
+
+The crosvm book provides directions on how to build a `gfxstream-enabled
+rutabaga`_ and launch a `guest Wayland proxy`_.
+
+This device does require host blob support (``hostmem`` field below). The
+``hostmem`` field specifies the size of virtio-gpu host memory window.
+This is typically between 256M and 8G.
+
+At least one capset (see colon separated ``capset_names`` below) must be
+specified when starting the device.  The currently supported
+``capset_names`` are ``gfxstream-vulkan`` and ``cross-domain`` on Linux
+guests. For Android guests, ``gfxstream-gles`` is also supported.
+
+The device will try to auto-detect the wayland socket path

Re: [PATCH v5 8/9] gfxstream + rutabaga: enable rutabaga

[Manos Pitsidianakis]

Reviewed-by: Emmanouil Pitsidianakis 
Tested-by: Emmanouil Pitsidianakis 

On Tue, 15 Aug 2023 03:35, Gurchetan Singh  wrote:

This change enables rutabaga to receive virtio-gpu-3d hypercalls
when it is active.

Signed-off-by: Gurchetan Singh 
Tested-by: Alyssa Ross 
---
v3: Whitespace fix (Akihiko)

hw/display/virtio-gpu-base.c | 3 ++-
hw/display/virtio-gpu.c  | 5 +++--
softmmu/qdev-monitor.c   | 3 +++
softmmu/vl.c | 1 +
4 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/hw/display/virtio-gpu-base.c b/hw/display/virtio-gpu-base.c
index 4f2b0ba1f3..50c5373b65 100644
--- a/hw/display/virtio-gpu-base.c
+++ b/hw/display/virtio-gpu-base.c
@@ -223,7 +223,8 @@ virtio_gpu_base_get_features(VirtIODevice *vdev, uint64_t 
features,
{
VirtIOGPUBase *g = VIRTIO_GPU_BASE(vdev);

-if (virtio_gpu_virgl_enabled(g->conf)) {
+if (virtio_gpu_virgl_enabled(g->conf) ||
+virtio_gpu_rutabaga_enabled(g->conf)) {
features |= (1 << VIRTIO_GPU_F_VIRGL);
}
if (virtio_gpu_edid_enabled(g->conf)) {
diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index 3e658f1fef..08e170e029 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -1361,8 +1361,9 @@ void virtio_gpu_device_realize(DeviceState *qdev, Error 
**errp)
VirtIOGPU *g = VIRTIO_GPU(qdev);

if (virtio_gpu_blob_enabled(g->parent_obj.conf)) {
-if (!virtio_gpu_have_udmabuf()) {
-error_setg(errp, "cannot enable blob resources without udmabuf");
+if (!virtio_gpu_have_udmabuf() &&
+!virtio_gpu_rutabaga_enabled(g->parent_obj.conf)) {
+error_setg(errp, "need udmabuf or rutabaga for blob resources");
return;
}

diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
index 74f4e41338..1b8005ae55 100644
--- a/softmmu/qdev-monitor.c
+++ b/softmmu/qdev-monitor.c
@@ -86,6 +86,9 @@ static const QDevAlias qdev_alias_table[] = {
{ "virtio-gpu-pci", "virtio-gpu", QEMU_ARCH_VIRTIO_PCI },
{ "virtio-gpu-gl-device", "virtio-gpu-gl", QEMU_ARCH_VIRTIO_MMIO },
{ "virtio-gpu-gl-pci", "virtio-gpu-gl", QEMU_ARCH_VIRTIO_PCI },
+{ "virtio-gpu-rutabaga-device", "virtio-gpu-rutabaga",
+  QEMU_ARCH_VIRTIO_MMIO },
+{ "virtio-gpu-rutabaga-pci", "virtio-gpu-rutabaga", QEMU_ARCH_VIRTIO_PCI },
{ "virtio-input-host-device", "virtio-input-host", QEMU_ARCH_VIRTIO_MMIO },
{ "virtio-input-host-ccw", "virtio-input-host", QEMU_ARCH_VIRTIO_CCW },
{ "virtio-input-host-pci", "virtio-input-host", QEMU_ARCH_VIRTIO_PCI },
diff --git a/softmmu/vl.c b/softmmu/vl.c
index b0b96f67fa..2f98eefdf3 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -216,6 +216,7 @@ static struct {
{ .driver = "ati-vga",  .flag = &default_vga   },
{ .driver = "vhost-user-vga",   .flag = &default_vga   },
{ .driver = "virtio-vga-gl",.flag = &default_vga   },
+{ .driver = "virtio-vga-rutabaga",  .flag = &default_vga   },
};

static QemuOptsList qemu_rtc_opts = {
--
2.41.0.694.ge786442a9b-goog

Re: [PATCH v5 7/9] gfxstream + rutabaga: meson support

[Manos Pitsidianakis]

Reviewed-by: Emmanouil Pitsidianakis 
Tested-by: Emmanouil Pitsidianakis 

On Tue, 15 Aug 2023 03:35, Gurchetan Singh  wrote:

- Add meson detection of rutabaga_gfx
- Build virtio-gpu-rutabaga.c + associated vga/pci files when
 present

Signed-off-by: Gurchetan Singh 
Tested-by: Alyssa Ross 
---
v3: Fix alignment issues (Akihiko)

hw/display/meson.build| 22 ++
meson.build   |  7 +++
meson_options.txt |  2 ++
scripts/meson-buildoptions.sh |  3 +++
4 files changed, 34 insertions(+)

diff --git a/hw/display/meson.build b/hw/display/meson.build
index 413ba4ab24..e362d625dd 100644
--- a/hw/display/meson.build
+++ b/hw/display/meson.build
@@ -79,6 +79,13 @@ if config_all_devices.has_key('CONFIG_VIRTIO_GPU')
 if_true: [files('virtio-gpu-gl.c', 
'virtio-gpu-virgl.c'), pixman, virgl])
hw_display_modules += {'virtio-gpu-gl': virtio_gpu_gl_ss}
  endif
+
+  if rutabaga.found()
+virtio_gpu_rutabaga_ss = ss.source_set()
+virtio_gpu_rutabaga_ss.add(when: ['CONFIG_VIRTIO_GPU', rutabaga],
+   if_true: [files('virtio-gpu-rutabaga.c'), 
pixman])
+hw_display_modules += {'virtio-gpu-rutabaga': virtio_gpu_rutabaga_ss}
+  endif
endif

if config_all_devices.has_key('CONFIG_VIRTIO_PCI')
@@ -95,6 +102,12 @@ if config_all_devices.has_key('CONFIG_VIRTIO_PCI')
 if_true: [files('virtio-gpu-pci-gl.c'), pixman])
hw_display_modules += {'virtio-gpu-pci-gl': virtio_gpu_pci_gl_ss}
  endif
+  if rutabaga.found()
+virtio_gpu_pci_rutabaga_ss = ss.source_set()
+virtio_gpu_pci_rutabaga_ss.add(when: ['CONFIG_VIRTIO_GPU', 
'CONFIG_VIRTIO_PCI', rutabaga],
+   if_true: 
[files('virtio-gpu-pci-rutabaga.c'), pixman])
+hw_display_modules += {'virtio-gpu-pci-rutabaga': 
virtio_gpu_pci_rutabaga_ss}
+  endif
endif

if config_all_devices.has_key('CONFIG_VIRTIO_VGA')
@@ -113,6 +126,15 @@ if config_all_devices.has_key('CONFIG_VIRTIO_VGA')
  virtio_vga_gl_ss.add(when: 'CONFIG_ACPI', if_true: files('acpi-vga.c'),
if_false: files('acpi-vga-stub.c'))
  hw_display_modules += {'virtio-vga-gl': virtio_vga_gl_ss}
+
+  if rutabaga.found()
+virtio_vga_rutabaga_ss = ss.source_set()
+virtio_vga_rutabaga_ss.add(when: ['CONFIG_VIRTIO_VGA', rutabaga],
+   if_true: [files('virtio-vga-rutabaga.c'), 
pixman])
+virtio_vga_rutabaga_ss.add(when: 'CONFIG_ACPI', if_true: 
files('acpi-vga.c'),
+if_false: 
files('acpi-vga-stub.c'))
+hw_display_modules += {'virtio-vga-rutabaga': virtio_vga_rutabaga_ss}
+  endif
endif

system_ss.add(when: 'CONFIG_OMAP', if_true: files('omap_lcdc.c'))
diff --git a/meson.build b/meson.build
index 98e68ef0b1..293f388e53 100644
--- a/meson.build
+++ b/meson.build
@@ -1069,6 +1069,12 @@ if not get_option('virglrenderer').auto() or have_system 
or have_vhost_user_gpu
   dependencies: virgl))
  endif
endif
+rutabaga = not_found
+if not get_option('rutabaga_gfx').auto() or have_system or have_vhost_user_gpu
+  rutabaga = dependency('rutabaga_gfx_ffi',
+ method: 'pkg-config',
+ required: get_option('rutabaga_gfx'))
+endif
blkio = not_found
if not get_option('blkio').auto() or have_block
  blkio = dependency('blkio',
@@ -4272,6 +4278,7 @@ summary_info += {'libtasn1':  tasn1}
summary_info += {'PAM':   pam}
summary_info += {'iconv support': iconv}
summary_info += {'virgl support': virgl}
+summary_info += {'rutabaga support':  rutabaga}
summary_info += {'blkio support': blkio}
summary_info += {'curl support':  curl}
summary_info += {'Multipath support': mpathpersist}
diff --git a/meson_options.txt b/meson_options.txt
index aaea5ddd77..dea3bf7d9c 100644
--- a/meson_options.txt
+++ b/meson_options.txt
@@ -224,6 +224,8 @@ option('vmnet', type : 'feature', value : 'auto',
   description: 'vmnet.framework network backend support')
option('virglrenderer', type : 'feature', value : 'auto',
   description: 'virgl rendering support')
+option('rutabaga_gfx', type : 'feature', value : 'auto',
+   description: 'rutabaga_gfx support')
option('png', type : 'feature', value : 'auto',
   description: 'PNG support with libpng')
option('vnc', type : 'feature', value : 'auto',
diff --git a/scripts/meson-buildoptions.sh b/scripts/meson-buildoptions.sh
index 9da3fe299b..9a95b4f782 100644
--- a/scripts/meson-buildoptions.sh
+++ b/scripts/meson-buildoptions.sh
@@ -154,6 +154,7 @@ meson_options_help() {
  printf "%s\n" '  rbd Ceph block device driver'
  printf "%s\n" '  rdmaEnable RDMA-based migration'
  printf "%s\n" '  replication replication support'
+  printf "%s\n" '  rutabaga-gfxrutabaga_gfx support'
  printf "%s\n" '  sdl SDL user interface'
  printf "%s\n" '  sdl-im

Re: [PATCH v5 6/9] gfxstream + rutabaga: add initial support for gfxstream

[Manos Pitsidianakis]

Reviewed-by: Emmanouil Pitsidianakis 
Tested-by: Emmanouil Pitsidianakis 

On Tue, 15 Aug 2023 03:35, Gurchetan Singh  wrote:

This adds initial support for gfxstream and cross-domain.  Both
features rely on virtio-gpu blob resources and context types, which
are also implemented in this patch.

gfxstream has a long and illustrious history in Android graphics
paravirtualization.  It has been powering graphics in the Android
Studio Emulator for more than a decade, which is the main developer
platform.

Originally conceived by Jesse Hall, it was first known as "EmuGL" [a].
The key design characteristic was a 1:1 threading model and
auto-generation, which fit nicely with the OpenGLES spec.  It also
allowed easy layering with ANGLE on the host, which provides the GLES
implementations on Windows or MacOS enviroments.

gfxstream has traditionally been maintained by a single engineer, and
between 2015 to 2021, the goldfish throne passed to Frank Yang.
Historians often remark this glorious reign ("pax gfxstreama" is the
academic term) was comparable to that of Augustus and both Queen
Elizabeths.  Just to name a few accomplishments in a resplendent
panoply: higher versions of GLES, address space graphics, snapshot
support and CTS compliant Vulkan [b].

One major drawback was the use of out-of-tree goldfish drivers.
Android engineers didn't know much about DRM/KMS and especially TTM so
a simple guest to host pipe was conceived.

Luckily, virtio-gpu 3D started to emerge in 2016 due to the work of
the Mesa/virglrenderer communities.  In 2018, the initial virtio-gpu
port of gfxstream was done by Cuttlefish enthusiast Alistair Delva.
It was a symbol compatible replacement of virglrenderer [c] and named
"AVDVirglrenderer".  This implementation forms the basis of the
current gfxstream host implementation still in use today.

cross-domain support follows a similar arc.  Originally conceived by
Wayland aficionado David Reveman and crosvm enjoyer Zach Reizner in
2018, it initially relied on the downstream "virtio-wl" device.

In 2020 and 2021, virtio-gpu was extended to include blob resources
and multiple timelines by yours truly, features gfxstream/cross-domain
both require to function correctly.

Right now, we stand at the precipice of a truly fantastic possibility:
the Android Emulator powered by upstream QEMU and upstream Linux
kernel.  gfxstream will then be packaged properfully, and app
developers can even fix gfxstream bugs on their own if they encounter
them.

It's been quite the ride, my friends.  Where will gfxstream head next,
nobody really knows.  I wouldn't be surprised if it's around for
another decade, maintained by a new generation of Android graphics
enthusiasts.

Technical details:
 - Very simple initial display integration: just used Pixman
 - Largely, 1:1 mapping of virtio-gpu hypercalls to rutabaga function
   calls

Next steps for Android VMs:
 - The next step would be improving display integration and UI interfaces
   with the goal of the QEMU upstream graphics being in an emulator
   release [d].

Next steps for Linux VMs for display virtualization:
 - For widespread distribution, someone needs to package Sommelier or the
   wayland-proxy-virtwl [e] ideally into Debian main. In addition, newer
   versions of the Linux kernel come with DRM_VIRTIO_GPU_KMS option,
   which allows disabling KMS hypercalls.  If anyone cares enough, it'll
   probably be possible to build a custom VM variant that uses this display
   virtualization strategy.

[a] https://android-review.googlesource.com/c/platform/development/+/34470
[b] 
https://android-review.googlesource.com/q/topic:%22vulkan-hostconnection-start%22
[c] 
https://android-review.googlesource.com/c/device/generic/goldfish-opengl/+/761927
[d] https://developer.android.com/studio/releases/emulator
[e] https://github.com/talex5/wayland-proxy-virtwl

Signed-off-by: Gurchetan Singh 
Tested-by: Alyssa Ross 
---
v1: Incorported various suggestions by Akihiko Odaki and Bernard Berschow
   - Removed GET_VIRTIO_GPU_GL / GET_RUTABAGA macros
   - Used error_report(..)
   - Used g_autofree to fix leaks on error paths
   - Removed unnecessary casts
   - added virtio-gpu-pci-rutabaga.c + virtio-vga-rutabaga.c files

v2: Incorported various suggestions by Akihiko Odaki, Marc-André Lureau and
   Bernard Berschow:
   - Parenthesis in CHECK macro
   - CHECK_RESULT(result, ..) --> CHECK(!result, ..)
   - delay until g->parent_obj.enable = 1
   - Additional cast fixes
   - initialize directly in virtio_gpu_rutabaga_realize(..)
   - add debug callback to hook into QEMU error's APIs

v3: Incorporated feedback from Akihiko Odaki and Alyssa Ross:
   - Autodetect Wayland socket when not explicitly specified
   - Fix map_blob error paths
   - Add comment why we need both `res` and `resource` in create blob
   - Cast and whitespace fixes
   - Big endian check comes before virtio_gpu_rutabaga_init().
   - VirtIOVGARUTABAGA --> VirtIOVGARutabaga

v4: Incorporated feedback from Akihiko Oda

Re: [PATCH v5 5/9] gfxstream + rutabaga prep: added need defintions, fields, and options

[Manos Pitsidianakis]

On Tue, 15 Aug 2023 03:35, Gurchetan Singh  
wrote:

This modifies the common virtio-gpu.h file have the fields and
defintions needed by gfxstream/rutabaga, by VirtioGpuRutabaga.

The command to run these would be:

Signed-off-by: Gurchetan Singh 
Tested-by: Alyssa Ross 
---
-device virtio-vga-rutabaga,capset_names=gfxstream-vulkan:cross-domain, \
   wayland_socket_path=/run/user/1000/wayland-0,hostmem=8G  \


I think the commit message got mangled here, the commit trailers got 
between "The command to run these would be:" and the CLI flags.


By the way minor typo, splitting the device+commas argument with 
white-space will not get parsed correctly, "wayland_socket_path..." 
should be immediately after the previous comma.


Reviewed-by: Emmanouil Pitsidianakis 
Tested-by: Emmanouil Pitsidianakis 



v1: void *rutabaga --> struct rutabaga *rutabaga (Akihiko)
   have a separate rutabaga device instead of using GL device (Bernard)

v2: VirtioGpuRutabaga --> VirtIOGPURutabaga (Akihiko)
   move MemoryRegionInfo into VirtIOGPURutabaga (Akihiko)
   remove 'ctx' field (Akihiko)
   remove 'rutabaga_active'

include/hw/virtio/virtio-gpu.h | 28 
1 file changed, 28 insertions(+)

diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h
index 55973e112f..e2a07e68d9 100644
--- a/include/hw/virtio/virtio-gpu.h
+++ b/include/hw/virtio/virtio-gpu.h
@@ -38,6 +38,9 @@ OBJECT_DECLARE_SIMPLE_TYPE(VirtIOGPUGL, VIRTIO_GPU_GL)
#define TYPE_VHOST_USER_GPU "vhost-user-gpu"
OBJECT_DECLARE_SIMPLE_TYPE(VhostUserGPU, VHOST_USER_GPU)

+#define TYPE_VIRTIO_GPU_RUTABAGA "virtio-gpu-rutabaga-device"
+OBJECT_DECLARE_SIMPLE_TYPE(VirtIOGPURutabaga, VIRTIO_GPU_RUTABAGA)
+
struct virtio_gpu_simple_resource {
uint32_t resource_id;
uint32_t width;
@@ -94,6 +97,7 @@ enum virtio_gpu_base_conf_flags {
VIRTIO_GPU_FLAG_DMABUF_ENABLED,
VIRTIO_GPU_FLAG_BLOB_ENABLED,
VIRTIO_GPU_FLAG_CONTEXT_INIT_ENABLED,
+VIRTIO_GPU_FLAG_RUTABAGA_ENABLED,
};

#define virtio_gpu_virgl_enabled(_cfg) \
@@ -108,6 +112,8 @@ enum virtio_gpu_base_conf_flags {
(_cfg.flags & (1 << VIRTIO_GPU_FLAG_BLOB_ENABLED))
#define virtio_gpu_context_init_enabled(_cfg) \
(_cfg.flags & (1 << VIRTIO_GPU_FLAG_CONTEXT_INIT_ENABLED))
+#define virtio_gpu_rutabaga_enabled(_cfg) \
+(_cfg.flags & (1 << VIRTIO_GPU_FLAG_RUTABAGA_ENABLED))
#define virtio_gpu_hostmem_enabled(_cfg) \
(_cfg.hostmem > 0)

@@ -232,6 +238,28 @@ struct VhostUserGPU {
bool backend_blocked;
};

+#define MAX_SLOTS 4096
+
+struct MemoryRegionInfo {
+int used;
+MemoryRegion mr;
+uint32_t resource_id;
+};
+
+struct rutabaga;
+
+struct VirtIOGPURutabaga {
+struct VirtIOGPU parent_obj;
+
+struct MemoryRegionInfo memory_regions[MAX_SLOTS];
+char *capset_names;
+char *wayland_socket_path;
+char *wsi;
+bool headless;
+uint32_t num_capsets;
+struct rutabaga *rutabaga;
+};
+
#define VIRTIO_GPU_FILL_CMD(out) do {   \
size_t s;   \
s = iov_to_buf(cmd->elem.out_sg, cmd->elem.out_num, 0,  \
--
2.41.0.694.ge786442a9b-goog

Re: [PATCH v5 4/9] virtio-gpu: blob prep

[Manos Pitsidianakis]

Reviewed-by: Emmanouil Pitsidianakis 
Tested-by: Emmanouil Pitsidianakis 

On Tue, 15 Aug 2023 03:35, Gurchetan Singh  wrote:

From: Antonio Caggiano 

This adds preparatory functions needed to:

- decode blob cmds
- tracking iovecs

Signed-off-by: Antonio Caggiano 
Signed-off-by: Dmitry Osipenko 
Signed-off-by: Gurchetan Singh 
Tested-by: Alyssa Ross 
---
hw/display/virtio-gpu.c  | 10 +++---
include/hw/virtio/virtio-gpu-bswap.h | 18 ++
include/hw/virtio/virtio-gpu.h   |  5 +
3 files changed, 26 insertions(+), 7 deletions(-)

diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index 48ef0d9fad..3e658f1fef 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -33,15 +33,11 @@

#define VIRTIO_GPU_VM_VERSION 1

-static struct virtio_gpu_simple_resource*
-virtio_gpu_find_resource(VirtIOGPU *g, uint32_t resource_id);
static struct virtio_gpu_simple_resource *
virtio_gpu_find_check_resource(VirtIOGPU *g, uint32_t resource_id,
   bool require_backing,
   const char *caller, uint32_t *error);

-static void virtio_gpu_cleanup_mapping(VirtIOGPU *g,
-   struct virtio_gpu_simple_resource *res);
static void virtio_gpu_reset_bh(void *opaque);

void virtio_gpu_update_cursor_data(VirtIOGPU *g,
@@ -116,7 +112,7 @@ static void update_cursor(VirtIOGPU *g, struct 
virtio_gpu_update_cursor *cursor)
  cursor->resource_id ? 1 : 0);
}

-static struct virtio_gpu_simple_resource *
+struct virtio_gpu_simple_resource *
virtio_gpu_find_resource(VirtIOGPU *g, uint32_t resource_id)
{
struct virtio_gpu_simple_resource *res;
@@ -904,8 +900,8 @@ void virtio_gpu_cleanup_mapping_iov(VirtIOGPU *g,
g_free(iov);
}

-static void virtio_gpu_cleanup_mapping(VirtIOGPU *g,
-   struct virtio_gpu_simple_resource *res)
+void virtio_gpu_cleanup_mapping(VirtIOGPU *g,
+struct virtio_gpu_simple_resource *res)
{
virtio_gpu_cleanup_mapping_iov(g, res->iov, res->iov_cnt);
res->iov = NULL;
diff --git a/include/hw/virtio/virtio-gpu-bswap.h 
b/include/hw/virtio/virtio-gpu-bswap.h
index 9124108485..dd1975e2d4 100644
--- a/include/hw/virtio/virtio-gpu-bswap.h
+++ b/include/hw/virtio/virtio-gpu-bswap.h
@@ -63,10 +63,28 @@ virtio_gpu_create_blob_bswap(struct 
virtio_gpu_resource_create_blob *cblob)
{
virtio_gpu_ctrl_hdr_bswap(&cblob->hdr);
le32_to_cpus(&cblob->resource_id);
+le32_to_cpus(&cblob->blob_mem);
le32_to_cpus(&cblob->blob_flags);
+le32_to_cpus(&cblob->nr_entries);
+le64_to_cpus(&cblob->blob_id);
le64_to_cpus(&cblob->size);
}

+static inline void
+virtio_gpu_map_blob_bswap(struct virtio_gpu_resource_map_blob *mblob)
+{
+virtio_gpu_ctrl_hdr_bswap(&mblob->hdr);
+le32_to_cpus(&mblob->resource_id);
+le64_to_cpus(&mblob->offset);
+}
+
+static inline void
+virtio_gpu_unmap_blob_bswap(struct virtio_gpu_resource_unmap_blob *ublob)
+{
+virtio_gpu_ctrl_hdr_bswap(&ublob->hdr);
+le32_to_cpus(&ublob->resource_id);
+}
+
static inline void
virtio_gpu_scanout_blob_bswap(struct virtio_gpu_set_scanout_blob *ssb)
{
diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h
index de4f624e94..55973e112f 100644
--- a/include/hw/virtio/virtio-gpu.h
+++ b/include/hw/virtio/virtio-gpu.h
@@ -257,6 +257,9 @@ void virtio_gpu_base_fill_display_info(VirtIOGPUBase *g,
void virtio_gpu_base_generate_edid(VirtIOGPUBase *g, int scanout,
   struct virtio_gpu_resp_edid *edid);
/* virtio-gpu.c */
+struct virtio_gpu_simple_resource *
+virtio_gpu_find_resource(VirtIOGPU *g, uint32_t resource_id);
+
void virtio_gpu_ctrl_response(VirtIOGPU *g,
  struct virtio_gpu_ctrl_command *cmd,
  struct virtio_gpu_ctrl_hdr *resp,
@@ -275,6 +278,8 @@ int virtio_gpu_create_mapping_iov(VirtIOGPU *g,
  uint32_t *niov);
void virtio_gpu_cleanup_mapping_iov(VirtIOGPU *g,
struct iovec *iov, uint32_t count);
+void virtio_gpu_cleanup_mapping(VirtIOGPU *g,
+struct virtio_gpu_simple_resource *res);
void virtio_gpu_process_cmdq(VirtIOGPU *g);
void virtio_gpu_device_realize(DeviceState *qdev, Error **errp);
void virtio_gpu_reset(VirtIODevice *vdev);
--
2.41.0.694.ge786442a9b-goog

Re: [PATCH v5 3/9] virtio-gpu: hostmem

[Manos Pitsidianakis]

Reviewed-by: Emmanouil Pitsidianakis 
Tested-by: Emmanouil Pitsidianakis 

On Tue, 15 Aug 2023 03:35, Gurchetan Singh  wrote:

From: Gerd Hoffmann 

Use VIRTIO_GPU_SHM_ID_HOST_VISIBLE as id for virtio-gpu.

Signed-off-by: Antonio Caggiano 
Tested-by: Alyssa Ross 
Acked-by: Michael S. Tsirkin 
---
hw/display/virtio-gpu-pci.c| 14 ++
hw/display/virtio-gpu.c|  1 +
hw/display/virtio-vga.c| 33 -
include/hw/virtio/virtio-gpu.h |  5 +
4 files changed, 44 insertions(+), 9 deletions(-)

diff --git a/hw/display/virtio-gpu-pci.c b/hw/display/virtio-gpu-pci.c
index 93f214ff58..da6a99f038 100644
--- a/hw/display/virtio-gpu-pci.c
+++ b/hw/display/virtio-gpu-pci.c
@@ -33,6 +33,20 @@ static void virtio_gpu_pci_base_realize(VirtIOPCIProxy 
*vpci_dev, Error **errp)
DeviceState *vdev = DEVICE(g);
int i;

+if (virtio_gpu_hostmem_enabled(g->conf)) {
+vpci_dev->msix_bar_idx = 1;
+vpci_dev->modern_mem_bar_idx = 2;
+memory_region_init(&g->hostmem, OBJECT(g), "virtio-gpu-hostmem",
+   g->conf.hostmem);
+pci_register_bar(&vpci_dev->pci_dev, 4,
+ PCI_BASE_ADDRESS_SPACE_MEMORY |
+ PCI_BASE_ADDRESS_MEM_PREFETCH |
+ PCI_BASE_ADDRESS_MEM_TYPE_64,
+ &g->hostmem);
+virtio_pci_add_shm_cap(vpci_dev, 4, 0, g->conf.hostmem,
+   VIRTIO_GPU_SHM_ID_HOST_VISIBLE);
+}
+
virtio_pci_force_virtio_1(vpci_dev);
if (!qdev_realize(vdev, BUS(&vpci_dev->bus), errp)) {
return;
diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index bbd5c6561a..48ef0d9fad 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -1509,6 +1509,7 @@ static Property virtio_gpu_properties[] = {
 256 * MiB),
DEFINE_PROP_BIT("blob", VirtIOGPU, parent_obj.conf.flags,
VIRTIO_GPU_FLAG_BLOB_ENABLED, false),
+DEFINE_PROP_SIZE("hostmem", VirtIOGPU, parent_obj.conf.hostmem, 0),
DEFINE_PROP_END_OF_LIST(),
};

diff --git a/hw/display/virtio-vga.c b/hw/display/virtio-vga.c
index e6fb0aa876..c8552ff760 100644
--- a/hw/display/virtio-vga.c
+++ b/hw/display/virtio-vga.c
@@ -115,17 +115,32 @@ static void virtio_vga_base_realize(VirtIOPCIProxy 
*vpci_dev, Error **errp)
pci_register_bar(&vpci_dev->pci_dev, 0,
 PCI_BASE_ADDRESS_MEM_PREFETCH, &vga->vram);

-/*
- * Configure virtio bar and regions
- *
- * We use bar #2 for the mmio regions, to be compatible with stdvga.
- * virtio regions are moved to the end of bar #2, to make room for
- * the stdvga mmio registers at the start of bar #2.
- */
-vpci_dev->modern_mem_bar_idx = 2;
-vpci_dev->msix_bar_idx = 4;
vpci_dev->modern_io_bar_idx = 5;

+if (!virtio_gpu_hostmem_enabled(g->conf)) {
+/*
+ * Configure virtio bar and regions
+ *
+ * We use bar #2 for the mmio regions, to be compatible with stdvga.
+ * virtio regions are moved to the end of bar #2, to make room for
+ * the stdvga mmio registers at the start of bar #2.
+ */
+vpci_dev->modern_mem_bar_idx = 2;
+vpci_dev->msix_bar_idx = 4;
+} else {
+vpci_dev->msix_bar_idx = 1;
+vpci_dev->modern_mem_bar_idx = 2;
+memory_region_init(&g->hostmem, OBJECT(g), "virtio-gpu-hostmem",
+   g->conf.hostmem);
+pci_register_bar(&vpci_dev->pci_dev, 4,
+ PCI_BASE_ADDRESS_SPACE_MEMORY |
+ PCI_BASE_ADDRESS_MEM_PREFETCH |
+ PCI_BASE_ADDRESS_MEM_TYPE_64,
+ &g->hostmem);
+virtio_pci_add_shm_cap(vpci_dev, 4, 0, g->conf.hostmem,
+   VIRTIO_GPU_SHM_ID_HOST_VISIBLE);
+}
+
if (!(vpci_dev->flags & VIRTIO_PCI_FLAG_PAGE_PER_VQ)) {
/*
 * with page-per-vq=off there is no padding space we can use
diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h
index 8377c365ef..de4f624e94 100644
--- a/include/hw/virtio/virtio-gpu.h
+++ b/include/hw/virtio/virtio-gpu.h
@@ -108,12 +108,15 @@ enum virtio_gpu_base_conf_flags {
(_cfg.flags & (1 << VIRTIO_GPU_FLAG_BLOB_ENABLED))
#define virtio_gpu_context_init_enabled(_cfg) \
(_cfg.flags & (1 << VIRTIO_GPU_FLAG_CONTEXT_INIT_ENABLED))
+#define virtio_gpu_hostmem_enabled(_cfg) \
+(_cfg.hostmem > 0)

struct virtio_gpu_base_conf {
uint32_t max_outputs;
uint32_t flags;
uint32_t xres;
uint32_t yres;
+uint64_t hostmem;
};

struct virtio_gpu_ctrl_command {
@@ -137,6 +140,8 @@ struct VirtIOGPUBase {
int renderer_blocked;
int enable;

+MemoryRegion hostmem;
+
struct virtio_gpu_scanout scanout[VIRTIO_GPU_MAX_SCANOUTS];

int enabled_output_bitmask;
--
2.41.0.694.ge786442a9b-goog

Re: [RFC v4 11/11] docs/user: Add doc for native library calls

[Manos Pitsidianakis]

On Tue, 08 Aug 2023 16:17, Yeqi Fu  wrote:

+arm and aarch64
+---
+HLT is an invalid instruction for userspace and usefully has 16
+bits of spare immeadiate data which we can stuff data in.


s/immeadiate/immediate

With that fix, you can add

Reviewed-by: Emmanouil Pitsidianakis 

Re: [RFC v4 01/11] build: Implement logic for sharing cross-building config files

[Manos Pitsidianakis]

This patch needs a detailed commit message, since it's not obvious why 
these changes are made at all. It'd also be helpful for reviewing.


General style comment for shell scripts: Always put curly braces around 
variables even if they are unnecessary. a $source_path could become 
$source_pathPREFIX in the future and instead of ${source_path} it would 
expand to ${source_pathPREFIX}.


On Tue, 08 Aug 2023 16:17, Yeqi Fu  wrote:

+tcg_tests_targets=
+for target in $target_list; do
+  case $target in
+*-softmmu)
+  test -f "$source_path/tests/tcg/$arch/Makefile.softmmu-target" || 
continue
+  ;;
+  esac

+  if test -f cross-build/$target/config-target.mak; then


targets will never have spaces but I'd still double quote ${target} for 
consistency and style




+  mkdir -p "tests/tcg/$target"
+  ln -srf cross-build/$target/config-target.mak 
tests/tcg/$target/config-target.mak
+  ln -sf $source_path/tests/tcg/Makefile.target tests/tcg/$target/Makefile


This ln definitely needs double quoting.


  echo "run-tcg-tests-$target: $qemu\$(EXESUF)" >> Makefile.prereqs
  tcg_tests_targets="$tcg_tests_targets $target"
  fi
--
2.34.1

Re: [PATCH v3 1/3] qmp: remove virtio_list, search QOM tree instead

[Manos Pitsidianakis]

On Thu, 03 Aug 2023 17:54, Jonah Palmer  wrote:

-VirtioInfoList *qmp_x_query_virtio(Error **errp)
+static int query_dev_child(Object *child, void *opaque)
{
-VirtioInfoList *list = NULL;
-VirtioInfo *node;
-VirtIODevice *vdev;
+VirtioInfoList **vdevs = opaque;
+Object *dev = object_dynamic_cast(child, TYPE_VIRTIO_DEVICE);
+if (dev != NULL && DEVICE(dev)->realized) {
+VirtIODevice *vdev = VIRTIO_DEVICE(dev);
+
+VirtioInfo *info = g_new(VirtioInfo, 1);
+
+/* Get canonical path of device */
+gchar *path = object_get_canonical_path(dev);


(You can use g_autofree char * here)


+
+info->path = g_strdup(path);
+info->name = g_strdup(vdev->name);
+QAPI_LIST_PREPEND(*vdevs, info);

-QTAILQ_FOREACH(vdev, &virtio_list, next) {
-DeviceState *dev = DEVICE(vdev);
-Error *err = NULL;
-QObject *obj = qmp_qom_get(dev->canonical_path, "realized", &err);
-
-if (err == NULL) {
-GString *is_realized = qobject_to_json_pretty(obj, true);
-/* virtio device is NOT realized, remove it from list */
-if (!strncmp(is_realized->str, "false", 4)) {
-QTAILQ_REMOVE(&virtio_list, vdev, next);
-} else {
-node = g_new(VirtioInfo, 1);
-node->path = g_strdup(dev->canonical_path);
-node->name = g_strdup(vdev->name);
-QAPI_LIST_PREPEND(list, node);
-}
-   g_string_free(is_realized, true);
-}
-qobject_unref(obj);
+g_free(path);
+} else {
+object_unref(dev);
}


The object_unref should not happen only in the else branch, no? Though 
it's not clear to me where the ref count was previously incremented.


+object_child_foreach_recursive(object_get_root(), query_dev_child, 
&vdevs);

+if (vdevs == NULL) {
+error_setg(errp, "No virtio devices found");
+return NULL;


(No need for early return here)


}
-return NULL;
+return vdevs;
+}
+
+VirtIODevice *qmp_find_virtio_device(const char *path)
+{
+/* Verify the canonical path is a realized virtio device */
+Object *dev = object_dynamic_cast(object_resolve_path(path, NULL),
+  TYPE_VIRTIO_DEVICE);
+if (!dev || !DEVICE(dev)->realized) {
+object_unref(dev);


Same as before with object refs

Re: [PATCH v3 2/3] qmp: update virtio feature maps, vhost-user-gpio introspection

[Manos Pitsidianakis]

On Thu, 03 Aug 2023 17:54, Jonah Palmer  wrote:

Add new virtio transport feature to transport feature map:
- VIRTIO_F_RING_RESET

Add new vhost-user protocol feature to vhost-user protocol feature map
and enumeration:
- VHOST_USER_PROTOCOL_F_STATUS

Add new virtio device features for several virtio devices to their
respective feature mappings:

virtio-blk:
- VIRTIO_BLK_F_SECURE_ERASE

virtio-net:
- VIRTIO_NET_F_NOTF_COAL
- VIRTIO_NET_F_GUEST_USO4
- VIRTIO_NET_F_GUEST_USO6
- VIRTIO_NET_F_HOST_USO

virtio/vhost-user-gpio:
- VIRTIO_GPIO_F_IRQ
- VHOST_F_LOG_ALL
- VHOST_USER_F_PROTOCOL_FEATURES

Add support for introspection on vhost-user-gpio devices.

Signed-off-by: Jonah Palmer 


Reviewed-by: Emmanouil Pitsidianakis 

Re: [PATCH v3 3/3] vhost-user: move VhostUserProtocolFeature definition to header file

[Manos Pitsidianakis]

On Thu, 03 Aug 2023 17:55, Jonah Palmer  wrote:

Move the definition of VhostUserProtocolFeature to
include/hw/virtio/vhost-user.h.

Remove previous definitions in hw/scsi/vhost-user-scsi.c,
hw/virtio/vhost-user.c, and hw/virtio/virtio-qmp.c.

Previously there were 3 separate definitions of this over 3 different
files. Now only 1 definition of this will be present for these 3 files.

Signed-off-by: Jonah Palmer 


Reviewed-by: Emmanouil Pitsidianakis 

Re: [PATCH v5 05/12] virtio-sound: prepare PCM streams

[Manos Pitsidianakis]

On Sat, 29 Jul 2023 14:00, Marc-André Lureau  
wrote:

+
+s->pcm->streams[stream_id] = stream;


Same remark as v4:
Shouldn't it close & free the existing stream? Or return an error?



Hello, yes you are correct. I missed that comment on the v4 reviews :)

Will send a v6 with a fix, I will wait a bit in case any other patch 
gets a Reviewed-By this time.

[PATCH v3 1/3] Add virtio-sound device

[Manos Pitsidianakis]

This patch adds an audio device implementing the recent virtio sound
spec (1.2).

PCM functionality is implemented, and jack[0], chmaps[1] messages are
at the moment left unimplemented.

PS2: This patch was based on a draft patch posted by OpenSynergy in 2019. [2]

[0]: https://www.kernel.org/doc/html/latest/sound/designs/jack-controls.html
[1]: 
https://www.kernel.org/doc/html/latest/sound/designs/channel-mapping-api.html
[2]: 
https://github.com/OpenSynergy/qemu/commit/5a2f350eec5d157b90d9c7b40a8e603f4da92471#diff-fa5c10be8476fb5385a280885d527b0b40dfc11ddcc74369fce085d8b5b17bbd

Signed-off-by: Emmanouil Pitsidianakis 
Signed-off-by: Igor Skalkin 
Signed-off-by: Anton Yakovlev 
base-commit: 361d539735
---
 MAINTAINERS|6 +
 hw/virtio/Kconfig  |5 +
 hw/virtio/meson.build  |1 +
 hw/virtio/trace-events |   21 +
 hw/virtio/virtio-snd.c | 1121 
 include/hw/virtio/virtio-snd.h |  194 ++
 6 files changed, 1348 insertions(+)
 create mode 100644 hw/virtio/virtio-snd.c
 create mode 100644 include/hw/virtio/virtio-snd.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 12e59b6b27..2bed60f9c1 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2245,6 +2245,12 @@ F: hw/virtio/virtio-mem-pci.h
 F: hw/virtio/virtio-mem-pci.c
 F: include/hw/virtio/virtio-mem.h
 
+virtio-snd
+M: Manos Pitsidianakis 
+S: Supported
+F: hw/virtio/virtio-snd*.c
+F: include/hw/virtio/virtio-snd.h
+
 nvme
 M: Keith Busch 
 M: Klaus Jensen 
diff --git a/hw/virtio/Kconfig b/hw/virtio/Kconfig
index 92c9cf6c96..d6f20657b3 100644
--- a/hw/virtio/Kconfig
+++ b/hw/virtio/Kconfig
@@ -17,6 +17,11 @@ config VIRTIO_PCI
 depends on PCI
 select VIRTIO
 
+config VIRTIO_SND
+bool
+default y
+depends on VIRTIO
+
 config VIRTIO_MMIO
 bool
 select VIRTIO
diff --git a/hw/virtio/meson.build b/hw/virtio/meson.build
index 13e7c6c272..120d4bfa0a 100644
--- a/hw/virtio/meson.build
+++ b/hw/virtio/meson.build
@@ -31,6 +31,7 @@ specific_virtio_ss.add(when: 'CONFIG_VHOST_VSOCK', if_true: 
files('vhost-vsock.c
 specific_virtio_ss.add(when: 'CONFIG_VHOST_USER_VSOCK', if_true: 
files('vhost-user-vsock.c'))
 specific_virtio_ss.add(when: 'CONFIG_VIRTIO_RNG', if_true: 
files('virtio-rng.c'))
 specific_virtio_ss.add(when: 'CONFIG_VIRTIO_MEM', if_true: 
files('virtio-mem.c'))
+specific_virtio_ss.add(when: 'CONFIG_VIRTIO_SND', if_true: 
files('virtio-snd.c'))
 specific_virtio_ss.add(when: 'CONFIG_VHOST_USER_I2C', if_true: 
files('vhost-user-i2c.c'))
 specific_virtio_ss.add(when: 'CONFIG_VHOST_USER_RNG', if_true: 
files('vhost-user-rng.c'))
 specific_virtio_ss.add(when: 'CONFIG_VHOST_USER_GPIO', if_true: 
files('vhost-user-gpio.c'))
diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
index 7109cf1a3b..8f3953dc28 100644
--- a/hw/virtio/trace-events
+++ b/hw/virtio/trace-events
@@ -154,3 +154,24 @@ virtio_pmem_flush_done(int type) "fsync return=%d"
 virtio_gpio_start(void) "start"
 virtio_gpio_stop(void) "stop"
 virtio_gpio_set_status(uint8_t status) "0x%x"
+
+#virtio-snd.c
+virtio_snd_pcm_stream_flush(int stream) "flushing stream %d"
+virtio_snd_handle_ctrl(void *vdev, void *vq) "snd %p: handle ctrl event for 
queue %p"
+#virtio_snd_handle_jack_info(int jack) "VIRTIO_SND_JACK_INFO called for jack 
%d"
+#virtio_snd_handle_jack_remap(void) "VIRTIO_SND_PCM_JACK_REMAP called"
+virtio_snd_handle_pcm_info(int stream) "VIRTIO_SND_R_PCM_INFO called for 
stream %d"
+virtio_snd_handle_pcm_set_params(int stream) "VIRTIO_SND_PCM_SET_PARAMS called 
for stream %d"
+virtio_snd_handle_pcm_start_stop(const char *code, int stream) "%s called for 
stream %d"
+virtio_snd_handle_pcm_release(int stream) "VIRTIO_SND_PCM_RELEASE called for 
stream %d"
+virtio_snd_handle_chmap_info(void) "VIRTIO_SND_CHMAP_INFO called"
+virtio_snd_handle_xfer(void) "tx/rx queue callback called"
+virtio_snd_handle_event(void) "event queue callback called"
+virtio_snd_realize(void *snd) "snd %p: realize"
+virtio_snd_unrealize(void *snd) "snd %p: unrealize"
+virtio_snd_get_features(void *vdev, uint64_t features) "snd %p: get_features 
0x%"PRIx64
+virtio_snd_get_config(void *vdev, uint32_t jacks, uint32_t streams, uint32_t 
chmaps) "snd %p: get_config jacks=%d streams=%d chmaps=%d"
+virtio_snd_set_config(void *vdev, uint32_t jacks, uint32_t new_jacks, uint32_t 
streams, uint32_t new_streams, uint32_t chmaps, uint32_t new_chmaps) "snd %p: 
set_config jacks from %d->%d, streams from %d->%d, chmaps from %d->%d"
+virtio_snd_vm_state_running(void) "vm state running"
+virtio_snd_vm_state_stopped(void) "vm state stopped"

[PATCH v3 3/3] Implement audio capture in virtio-snd device

[Manos Pitsidianakis]

Signed-off-by: Emmanouil Pitsidianakis 
---
 hw/virtio/trace-events |   3 +-
 hw/virtio/virtio-snd.c | 238 +++--
 2 files changed, 205 insertions(+), 36 deletions(-)

diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
index 8f3953dc28..3e7b259aef 100644
--- a/hw/virtio/trace-events
+++ b/hw/virtio/trace-events
@@ -165,7 +165,8 @@ virtio_snd_handle_pcm_set_params(int stream) 
"VIRTIO_SND_PCM_SET_PARAMS called f
 virtio_snd_handle_pcm_start_stop(const char *code, int stream) "%s called for 
stream %d"
 virtio_snd_handle_pcm_release(int stream) "VIRTIO_SND_PCM_RELEASE called for 
stream %d"
 virtio_snd_handle_chmap_info(void) "VIRTIO_SND_CHMAP_INFO called"
-virtio_snd_handle_xfer(void) "tx/rx queue callback called"
+virtio_snd_handle_tx_xfer(void) "tx queue callback called"
+virtio_snd_handle_rx_xfer(void) "rx queue callback called"
 virtio_snd_handle_event(void) "event queue callback called"
 virtio_snd_realize(void *snd) "snd %p: realize"
 virtio_snd_unrealize(void *snd) "snd %p: unrealize"
diff --git a/hw/virtio/virtio-snd.c b/hw/virtio/virtio-snd.c
index dbf8d8162f..a6ac9d8ce0 100644
--- a/hw/virtio/virtio-snd.c
+++ b/hw/virtio/virtio-snd.c
@@ -27,7 +27,7 @@
 #define VIRTIO_SOUND_VM_VERSION 1
 
 #define VIRTIO_SOUND_JACK_DEFAULT 0
-#define VIRTIO_SOUND_STREAM_DEFAULT 1
+#define VIRTIO_SOUND_STREAM_DEFAULT 2
 #define VIRTIO_SOUND_CHMAP_DEFAULT 0
 
 #define VIRTIO_SOUND_HDA_FN_NID 0
@@ -114,12 +114,16 @@ virtio_snd_set_config(VirtIODevice *vdev, const uint8_t 
*config)
 }
 
 static void virtio_snd_process_cmdq(VirtIOSound *s);
-static void virtio_snd_pcm_flush(VirtIOSoundPCMStream *stream);
+static void virtio_snd_pcm_out_flush(VirtIOSoundPCMStream *stream);
+static void virtio_snd_pcm_in_flush(VirtIOSoundPCMStream *stream);
 static void virtio_snd_pcm_out_cb(void *data, int available);
-static uint32_t virtio_snd_pcm_read_write(VirtIOSoundPCMStream *stream,
+static void virtio_snd_pcm_in_cb(void *data, int available);
+static uint32_t virtio_snd_pcm_write(VirtIOSoundPCMStream *stream,
   VirtQueue *vq,
-  VirtQueueElement *element,
-  bool read);
+  VirtQueueElement *element);
+static uint32_t virtio_snd_pcm_read(VirtIOSoundPCMStream *stream,
+  VirtQueue *vq,
+  VirtQueueElement *element);
 
 /*
  * Get a specific stream from the virtio sound card device.
@@ -421,15 +425,12 @@ static uint32_t virtio_snd_pcm_prepare_impl(VirtIOSound 
*s, uint32_t stream_id)
  &as);
 
 } else {
-qemu_log_mask(LOG_UNIMP, "virtio_snd: input/capture is 
unimplemented.");
-/*
- * stream->voice.in = AUD_open_in(&s->card,
- *stream->voice.in,
- *"virtio_snd_card",
- *stream,
- *virtio_snd_input_cb,
- *&as);
- */
+stream->voice.in = AUD_open_in(&s->card,
+stream->voice.in,
+"virtio_snd_card",
+stream,
+virtio_snd_pcm_in_cb,
+&as);
 }
 
 stream->as = as;
@@ -502,6 +503,8 @@ static void virtio_snd_handle_pcm_start_stop(VirtIOSound *s,
 if (stream) {
 if (stream->direction == VIRTIO_SND_D_OUTPUT) {
 AUD_set_active_out(stream->voice.out, start);
+} else {
+AUD_set_active_in(stream->voice.in, start);
 }
 } else {
 error_report("Invalid stream id: %d", req.stream_id);
@@ -552,7 +555,11 @@ static uint32_t 
virtio_snd_pcm_release_impl(VirtIOSoundPCMStream *stream,
  */
 virtio_snd_process_cmdq(stream->s);
 trace_virtio_snd_pcm_stream_flush(stream_id);
-virtio_snd_pcm_flush(stream);
+if (stream->direction == VIRTIO_SND_D_OUTPUT) {
+virtio_snd_pcm_out_flush(stream);
+} else {
+virtio_snd_pcm_in_flush(stream);
+}
 }
 
 return VIRTIO_SND_S_OK;
@@ -738,7 +745,7 @@ static void virtio_snd_handle_event(VirtIODevice *vdev, 
VirtQueue *vq)
  * @vdev: VirtIOSound device
  * @vq: tx virtqueue
  */
-static void virtio_snd_handle_xfer(VirtIODevice *vdev, VirtQueue *vq)
+static void virtio_snd_handle_tx_xfer(VirtIODevice *vdev, VirtQueue *vq)
 {
 VirtIOSound *s = VIRTIO_SND(vdev);
 VirtIOSoundPCMStream *stream = NULL;
@@ -747,7 +754,7 @@ static void virtio_snd_handle_xfer(VirtIODevice *vdev, 
VirtQueue *vq)
 virtio_snd_pcm_xfer hdr;
 virtio_snd_pcm_status resp = { 0 };
 
-trace_virtio_snd_handle_xfer();
+trace_virti

[PATCH v3 2/3] Add virtio-sound-pci device

[Manos Pitsidianakis]

This patch adds a PCI wrapper device for the virtio-sound device.

To test this, you'll need a >=5.13 kernel compiled with
CONFIG_SND_VIRTIO=y, which at the time of writing most distros have off
by default.

Use with following flags in the invocation:

  -device virtio-sound-pci,disable-legacy=on

And an audio backend listed with `-audio driver=help` that works on
your host machine, e.g.:

Pulseaudio:
  -audio driver=pa,model=virtio-sound
sdl:
  -audio driver=sdl,model=virtio-sound
coreaudio (macos/darwin):
  -audio driver=coreaudio,model=virtio-sound
etc.

You can use speaker-test from alsa-tools to play noise, sines, or
WAV files.

Signed-off-by: Emmanouil Pitsidianakis 
---
 hw/virtio/meson.build  |   1 +
 hw/virtio/virtio-snd-pci.c | 101 +
 include/hw/pci/pci.h   |   1 +
 softmmu/qdev-monitor.c |   1 +
 4 files changed, 104 insertions(+)
 create mode 100644 hw/virtio/virtio-snd-pci.c

diff --git a/hw/virtio/meson.build b/hw/virtio/meson.build
index 120d4bfa0a..5e5a83a4ee 100644
--- a/hw/virtio/meson.build
+++ b/hw/virtio/meson.build
@@ -63,6 +63,7 @@ virtio_pci_ss.add(when: 'CONFIG_VIRTIO_SERIAL', if_true: 
files('virtio-serial-pc
 virtio_pci_ss.add(when: 'CONFIG_VIRTIO_PMEM', if_true: 
files('virtio-pmem-pci.c'))
 virtio_pci_ss.add(when: 'CONFIG_VIRTIO_IOMMU', if_true: 
files('virtio-iommu-pci.c'))
 virtio_pci_ss.add(when: 'CONFIG_VIRTIO_MEM', if_true: 
files('virtio-mem-pci.c'))
+virtio_pci_ss.add(when: 'CONFIG_VIRTIO_SND', if_true: 
files('virtio-snd-pci.c'))
 virtio_pci_ss.add(when: 'CONFIG_VHOST_VDPA_DEV', if_true: 
files('vdpa-dev-pci.c'))
 virtio_pci_ss.add(when: 'CONFIG_VIRTIO_MD', if_true: files('virtio-md-pci.c'))
 
diff --git a/hw/virtio/virtio-snd-pci.c b/hw/virtio/virtio-snd-pci.c
new file mode 100644
index 00..44b1edd718
--- /dev/null
+++ b/hw/virtio/virtio-snd-pci.c
@@ -0,0 +1,101 @@
+/*
+ * VIRTIO Sound Device PCI Bindings
+ *
+ * Copyright (c) 2023 Emmanouil Pitsidianakis 
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or
+ * (at your option) any later version.  See the COPYING file in the
+ * top-level directory.
+ */
+
+#include "qemu/osdep.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-base"
+DECLARE_INSTANCE_CHECKER(VirtIOSoundPCI, VIRTIO_SOUND_PCI,
+ TYPE_VIRTIO_SND_PCI)
+
+struct VirtIOSoundPCI {
+VirtIOPCIProxy parent;
+VirtIOSound vdev;
+};
+
+static Property virtio_snd_pci_properties[] = {
+DEFINE_PROP_END_OF_LIST(),
+};
+
+static const char *audiodev_id;
+
+static int virtio_snd_init_pci(PCIBus *init_bus, const char *audiodev)
+{
+audiodev_id = audiodev;
+return 0;
+}
+
+static void virtio_snd_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
+{
+VirtIOSoundPCI *dev = VIRTIO_SOUND_PCI(vpci_dev);
+DeviceState *vdev = DEVICE(&dev->vdev);
+VirtIOSound *vsnd = VIRTIO_SND(&dev->vdev);
+
+/*
+ * According to spec, non-legacy virtio PCI devices are always little
+ * endian
+ */
+vsnd->virtio_access_is_big_endian = false;
+
+
+qdev_set_parent_bus(vdev, BUS(&vpci_dev->bus), errp);
+
+qdev_prop_set_string(vdev, "audiodev", audiodev_id);
+
+object_property_set_bool(OBJECT(vdev), "realized", true, errp);
+}
+
+static void virtio_snd_pci_class_init(ObjectClass *klass, void *data)
+{
+DeviceClass *dc = DEVICE_CLASS(klass);
+VirtioPCIClass *vpciklass = VIRTIO_PCI_CLASS(klass);
+PCIDeviceClass *pcidev_k = PCI_DEVICE_CLASS(klass);
+
+vpciklass->realize = virtio_snd_pci_realize;
+set_bit(DEVICE_CATEGORY_SOUND, dc->categories);
+
+pcidev_k->vendor_id = PCI_VENDOR_ID_REDHAT_QUMRANET;
+pcidev_k->device_id = PCI_DEVICE_ID_VIRTIO_SND;
+pcidev_k->revision = VIRTIO_PCI_ABI_VERSION;
+pcidev_k->class_id = PCI_CLASS_MULTIMEDIA_AUDIO;
+device_class_set_props(dc, virtio_snd_pci_properties);
+}
+
+static void virtio_snd_pci_instance_init(Object *obj)
+{
+VirtIOSoundPCI *dev = VIRTIO_SOUND_PCI(obj);
+
+virtio_instance_init_common(obj, &dev->vdev, sizeof(dev->vdev),
+TYPE_VIRTIO_SND);
+}
+
+static const VirtioPCIDeviceTypeInfo virtio_snd_pci_info = {
+.base_name = TYPE_VIRTIO_SND_PCI,
+.generic_name  = "virtio-sound-pci",
+.instance_size = sizeof(VirtIOSoundPCI),
+.instance_init = virtio_snd_pci_instance_init,
+.class_init= virtio_snd_pci_class_init,
+};
+
+static void virtio_snd_pci_register(void)
+{
+virtio_pci_types_register(&virtio_snd_pci_info);
+pci_register_soundhw("virtio-sound", "Virtio Sound Device",
+ virtio_snd_init_pci);
+}
+
+type_init(virtio_snd_pci_register);
diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index abdc1ef103..3cd5712035 100644
--- a/include/hw/pci/pci.h

[PATCH v3 0/3] Add VIRTIO sound card

[Manos Pitsidianakis]

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

Main differences with v2 patch 
:
- Addressed review comments.

Manos Pitsidianakis (3):
  Add virtio-sound device
  Add virtio-sound-pci device
  Implement audio capture in virtio-snd device

 MAINTAINERS|6 +
 hw/virtio/Kconfig  |5 +
 hw/virtio/meson.build  |2 +
 hw/virtio/trace-events |   22 +
 hw/virtio/virtio-snd-pci.c |  101 +++
 hw/virtio/virtio-snd.c | 1289 
 include/hw/pci/pci.h   |1 +
 include/hw/virtio/virtio-snd.h |  194 +
 softmmu/qdev-monitor.c |1 +
 9 files changed, 1621 insertions(+)
 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

-- 
2.39.2

Re: [PATCH v2 0/3] Add VIRTIO sound card

[Manos Pitsidianakis]

Ping

Patch series on patchew: 
https://patchew.org/QEMU/cover.1686238728.git.manos.pitsidiana...@linaro.org/


Patch series on lore:
https://lore.kernel.org/qemu-devel/cover.1686238728.git.manos.pitsidiana...@linaro.org/


On Thu, 08 Jun 2023 18:56, Manos Pitsidianakis  
wrote:
This patch series adds an audio device implementing the recent virtio 
sound spec (1.2) and a corresponding PCI wrapper device.


Main differences with v1 patch 
<20230526204845.673031-1-manos.pitsidiana...@linaro.org>:


- Split virtio-snd and virtio-snd-pci devices to two commits
- Added audio capture support

Known problems (On pipewire at least):

- Stereo recording results in one channel with the recording + a ticking 
 kind of artifact and one channel that has no artifacts, just the 
 recording.


Manos Pitsidianakis (3):
 Add virtio-sound device
 Add virtio-sound-pci device
 Implement audio capture in virtio-snd device

MAINTAINERS|6 +
hw/virtio/Kconfig  |5 +
hw/virtio/meson.build  |2 +
hw/virtio/trace-events |   22 +
hw/virtio/virtio-snd-pci.c |  102 +++
hw/virtio/virtio-snd.c | 1290 
include/hw/pci/pci.h   |1 +
include/hw/virtio/virtio-snd.h |  194 +
softmmu/qdev-monitor.c |1 +
9 files changed, 1623 insertions(+)
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 v1:
1:  652c7d2d01 ! 1:  95d564fc1f Add virtio-sound and virtio-sound-pci devices
   @@ Metadata
Author: Manos Pitsidianakis 

 ## Commit message ##

   -Add virtio-sound and virtio-sound-pci devices
   +Add virtio-sound device

This patch adds an audio device implementing the recent virtio sound

   -spec (1.2) and a corresponding PCI wrapper device.
   +spec (1.2).

PCM functionality is implemented, and jack[0], chmaps[1] messages are

   -at the moment ignored.
   -
   -To test this, you'll need a >6.0 kernel compiled with the virtio-snd
   -flag enabled, which distros have off by default.
   -
   -Use with following flags in the invocation:
   -
   -  -device virtio-sound-pci,disable-legacy=on
   -
   -And an audio backend listed with `-audio driver=help` that works on
   -your host machine, e.g.:
   -
   -Pulseaudio:
   -  -audio driver=pa,model=virtio-sound,server=/run/user/1000/pulse/native
   -sdl:
   -  -audio driver=sdl,model=virtio-sound
   -coreaudio:
   -  -audio driver=coreaudio,model=virtio-sound
   -etc.
   -
   -You can use speaker-test from alsa-tools to play noise, sines, or
   -WAV files.
   +at the moment left unimplemented.

PS2: This patch was based on a draft patch posted by OpenSynergy in 2019. [2]

   @@ Commit message

Signed-off-by: Igor Skalkin 
Signed-off-by: Anton Yakovlev 

   + ## MAINTAINERS ##

   +@@ MAINTAINERS: F: hw/virtio/virtio-mem-pci.h
   + F: hw/virtio/virtio-mem-pci.c
   + F: include/hw/virtio/virtio-mem.h
   + 
   ++virtio-snd

   ++M: Manos Pitsidianakis 
   ++S: Supported
   ++F: hw/virtio/virtio-snd*.c
   ++F: include/hw/virtio/virtio-snd.h
   ++
   + nvme
   + M: Keith Busch 
   + M: Klaus Jensen 
   +
 ## hw/virtio/Kconfig ##
@@ hw/virtio/Kconfig: config VIRTIO_PCI
 depends on PCI
   @@ hw/virtio/Kconfig: config VIRTIO_PCI
 select VIRTIO

 ## hw/virtio/meson.build ##

   -@@ hw/virtio/meson.build: virtio_pci_ss.add(when: 'CONFIG_VIRTIO_SERIAL', 
if_true: files('virtio-serial-pc
   - virtio_pci_ss.add(when: 'CONFIG_VIRTIO_PMEM', if_true: 
files('virtio-pmem-pci.c'))
   - virtio_pci_ss.add(when: 'CONFIG_VIRTIO_IOMMU', if_true: 
files('virtio-iommu-pci.c'))
   - virtio_pci_ss.add(when: 'CONFIG_VIRTIO_MEM', if_true: 
files('virtio-mem-pci.c'))
   -+virtio_pci_ss.add(when: 'CONFIG_VIRTIO_SND', if_true: 
files('virtio-snd-pci.c'))
   - virtio_pci_ss.add(when: 'CONFIG_VHOST_VDPA_DEV', if_true: 
files('vdpa-dev-pci.c'))
   - 
   - specific_virtio_ss.add_all(when: 'CONFIG_VIRTIO_PCI', if_true: virtio_pci_ss)

@@ hw/virtio/meson.build: softmmu_ss.add(when: 'CONFIG_VIRTIO', if_false: 
files('virtio-stub.c'))
 softmmu_ss.add(when: 'CONFIG_ALL', if_true: files('vhost-stub.c'))
 softmmu_ss.add(when: 'CONFIG_ALL', if_true: files('virtio-stub.c'))
   @@ hw/virtio/trace-events: virtio_pmem_flush_done(int type) "fsync return=%d"
+virtio_snd_handle_pcm_info(int stream) "VIRTIO_SND_R_PCM_INFO called for stream 
%d"
+virtio_snd_handle_pcm_set_params(int stream) "VIRTIO_SND_PCM_SET_PARAMS called 
for stream %d"
+virtio_snd_handle_pcm_start_stop(const char *cod

[PATCH] vhost-user: fully use new backend/frontend naming

[Manos Pitsidianakis]

Slave/master nomenclature was replaced with backend/frontend in commit

1fc19b65279a246083fc4d918510aae68586f734
vhost-user: Adopt new backend naming

This patch replaces all remaining uses of master and slave in the
codebase.

Signed-off-by: Emmanouil Pitsidianakis 
---
 block/export/vhost-user-blk-server.c  |  2 +-
 contrib/vhost-user-blk/vhost-user-blk.c   |  2 +-
 hw/block/vhost-user-blk.c |  2 +-
 hw/display/vhost-user-gpu.c   |  2 +-
 hw/input/vhost-user-input.c   |  2 +-
 hw/net/virtio-net.c   |  4 +-
 hw/virtio/vdpa-dev.c  |  2 +-
 hw/virtio/vhost-user.c| 52 +++---
 hw/virtio/virtio-qmp.c|  2 +-
 include/hw/virtio/vhost-backend.h |  2 +-
 subprojects/libvhost-user/libvhost-user.c | 54 +++
 subprojects/libvhost-user/libvhost-user.h | 20 +
 12 files changed, 74 insertions(+), 72 deletions(-)

diff --git a/block/export/vhost-user-blk-server.c 
b/block/export/vhost-user-blk-server.c
index 81b59761e3..f7b5073605 100644
--- a/block/export/vhost-user-blk-server.c
+++ b/block/export/vhost-user-blk-server.c
@@ -167,7 +167,7 @@ vu_blk_set_config(VuDev *vu_dev, const uint8_t *data,
 uint8_t wce;
 
 /* don't support live migration */
-if (flags != VHOST_SET_CONFIG_TYPE_MASTER) {
+if (flags != VHOST_SET_CONFIG_TYPE_FRONTEND) {
 return -EINVAL;
 }
 
diff --git a/contrib/vhost-user-blk/vhost-user-blk.c 
b/contrib/vhost-user-blk/vhost-user-blk.c
index 7941694e53..89e5f11a64 100644
--- a/contrib/vhost-user-blk/vhost-user-blk.c
+++ b/contrib/vhost-user-blk/vhost-user-blk.c
@@ -421,7 +421,7 @@ vub_set_config(VuDev *vu_dev, const uint8_t *data,
 int fd;
 
 /* don't support live migration */
-if (flags != VHOST_SET_CONFIG_TYPE_MASTER) {
+if (flags != VHOST_SET_CONFIG_TYPE_FRONTEND) {
 return -1;
 }
 
diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
index aff4d2b8cb..eecf3f7a81 100644
--- a/hw/block/vhost-user-blk.c
+++ b/hw/block/vhost-user-blk.c
@@ -81,7 +81,7 @@ static void vhost_user_blk_set_config(VirtIODevice *vdev, 
const uint8_t *config)
 ret = vhost_dev_set_config(&s->dev, &blkcfg->wce,
offsetof(struct virtio_blk_config, wce),
sizeof(blkcfg->wce),
-   VHOST_SET_CONFIG_TYPE_MASTER);
+   VHOST_SET_CONFIG_TYPE_FRONTEND);
 if (ret) {
 error_report("set device config space failed");
 return;
diff --git a/hw/display/vhost-user-gpu.c b/hw/display/vhost-user-gpu.c
index 1386e869e5..15f9d99d09 100644
--- a/hw/display/vhost-user-gpu.c
+++ b/hw/display/vhost-user-gpu.c
@@ -452,7 +452,7 @@ vhost_user_gpu_set_config(VirtIODevice *vdev,
 
 ret = vhost_dev_set_config(&g->vhost->dev, config_data,
0, sizeof(struct virtio_gpu_config),
-   VHOST_SET_CONFIG_TYPE_MASTER);
+   VHOST_SET_CONFIG_TYPE_FRONTEND);
 if (ret) {
 error_report("vhost-user-gpu: set device config space failed");
 return;
diff --git a/hw/input/vhost-user-input.c b/hw/input/vhost-user-input.c
index 1352e372ff..4ee3542106 100644
--- a/hw/input/vhost-user-input.c
+++ b/hw/input/vhost-user-input.c
@@ -69,7 +69,7 @@ static void vhost_input_set_config(VirtIODevice *vdev,
 
 ret = vhost_dev_set_config(&vhi->vhost->dev, config_data,
0, sizeof(virtio_input_config),
-   VHOST_SET_CONFIG_TYPE_MASTER);
+   VHOST_SET_CONFIG_TYPE_FRONTEND);
 if (ret) {
 error_report("vhost-user-input: set device config space failed");
 return;
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 6df6b7329d..75d7b89cd2 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -211,7 +211,7 @@ static void virtio_net_set_config(VirtIODevice *vdev, const 
uint8_t *config)
 if (nc->peer && nc->peer->info->type == NET_CLIENT_DRIVER_VHOST_VDPA) {
 vhost_net_set_config(get_vhost_net(nc->peer),
  (uint8_t *)&netcfg, 0, n->config_size,
- VHOST_SET_CONFIG_TYPE_MASTER);
+ VHOST_SET_CONFIG_TYPE_FRONTEND);
   }
 }
 
@@ -3733,7 +3733,7 @@ static void virtio_net_device_realize(DeviceState *dev, 
Error **errp)
 struct virtio_net_config netcfg = {};
 memcpy(&netcfg.mac, &n->nic_conf.macaddr, ETH_ALEN);
 vhost_net_set_config(get_vhost_net(nc->peer),
-(uint8_t *)&netcfg, 0, ETH_ALEN, VHOST_SET_CONFIG_TYPE_MASTER);
+(uint8_t *)&netcfg, 0, ETH_ALEN, VHOST_SET_CONFIG_TYPE_FRONTEND);
 }
 QTAILQ_INIT(&n->rsc_chains);
 n->qdev = dev;
diff --git a/hw/virtio/vdpa-dev.c b/hw/virtio/vdpa-dev.c
index 01b41eb0f1..cd40fe2de1 100644
--- a/hw/virtio/v

Re: [RFC v2 6/6] linux-user: Add '-native-bypass' option

[Manos Pitsidianakis]

On Wed, 07 Jun 2023 19:47, Yeqi Fu  wrote:

--- a/linux-user/main.c
+++ b/linux-user/main.c
+/* Set the library for native bypass  */
+if (native_lib != NULL) {
+char *token = malloc(strlen(native_lib) + 12);


malloc() can fail (in rare circumstances). Check for the return value 
here. Or use g_malloc() which terminates on alloc failure.



+strcpy(token, "LD_PRELOAD=");
+strcat(token, native_lib);


(You could alternatively use snprintf() here)



diff --git a/util/envlist.c b/util/envlist.c
index db937c0427..713d52497e 100644
+int
+envlist_appendenv(envlist_t *envlist, const char *env, const char *separator)
+{
+struct envlist_entry *entry = NULL;
+const char *eq_sign;
+size_t envname_len;
+
+if ((envlist == NULL) || (env == NULL)) {


separator must not be NULL as well.


+return (EINVAL);


Unnecessary parentheses here and in later returns.


+}
+
+/* find out first equals sign in given env */
+eq_sign = strchr(env, '=');
+if (eq_sign == NULL) {


Perhaps you can return an error message to the user here also, and 
explain why it failed. You can do that by passing an error message 
pointer with the function arguments.


By the way, if strchr(strchr(env, '='), '=') != NULL, shouldn't this 
fail also?

Re: [RFC v2 1/6] build: Add configure options for native calls

[Manos Pitsidianakis]

On Wed, 07 Jun 2023 19:47, Yeqi Fu  wrote:

+have_user_native_call = get_option('user_native_call') \
+.require(have_user, error_message: 'user_native_call requires user') \
+.require(targetos == 'linux', error_message: 'user_native_call requires 
Linux') \
+.allowed()



Is there a check for accepting user_native_call only for supported CPU 
user targets?




By the way, a question, would native calls be possible on BSD? I assume 
if yes it'd be a task for another time.

[PATCH v2 3/3] Implement audio capture in virtio-snd device

[Manos Pitsidianakis]

Signed-off-by: Emmanouil Pitsidianakis 
---
 hw/virtio/trace-events |   3 +-
 hw/virtio/virtio-snd.c | 239 +++--
 2 files changed, 206 insertions(+), 36 deletions(-)

diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
index b77d78abdc..b58c007297 100644
--- a/hw/virtio/trace-events
+++ b/hw/virtio/trace-events
@@ -162,7 +162,8 @@ virtio_snd_handle_pcm_set_params(int stream) 
"VIRTIO_SND_PCM_SET_PARAMS called f
 virtio_snd_handle_pcm_start_stop(const char *code, int stream) "%s called for 
stream %d"
 virtio_snd_handle_pcm_release(int stream) "VIRTIO_SND_PCM_RELEASE called for 
stream %d"
 virtio_snd_handle_chmap_info(void) "VIRTIO_SND_CHMAP_INFO called"
-virtio_snd_handle_xfer(void) "tx/rx queue callback called"
+virtio_snd_handle_tx_xfer(void) "tx queue callback called"
+virtio_snd_handle_rx_xfer(void) "rx queue callback called"
 virtio_snd_handle_event(void) "event queue callback called"
 virtio_snd_realize(void *snd) "snd %p: realize"
 virtio_snd_unrealize(void *snd) "snd %p: unrealize"
diff --git a/hw/virtio/virtio-snd.c b/hw/virtio/virtio-snd.c
index e3b6c353ad..fe7d70ff06 100644
--- a/hw/virtio/virtio-snd.c
+++ b/hw/virtio/virtio-snd.c
@@ -25,7 +25,7 @@
 #define VIRTIO_SOUND_VM_VERSION 1
 
 #define VIRTIO_SOUND_JACK_DEFAULT 0
-#define VIRTIO_SOUND_STREAM_DEFAULT 1
+#define VIRTIO_SOUND_STREAM_DEFAULT 2
 #define VIRTIO_SOUND_CHMAP_DEFAULT 0
 
 #define VIRTIO_SOUND_HDA_FN_NID 0
@@ -89,12 +89,16 @@ virtio_snd_set_config(VirtIODevice *vdev, const uint8_t 
*config)
 }
 
 static void virtio_snd_process_cmdq(VirtIOSound *s);
-static void virtio_snd_pcm_flush(VirtIOSoundPCMStream *stream);
+static void virtio_snd_pcm_out_flush(VirtIOSoundPCMStream *stream);
+static void virtio_snd_pcm_in_flush(VirtIOSoundPCMStream *stream);
 static void virtio_snd_pcm_out_cb(void *data, int available);
-static uint32_t virtio_snd_pcm_read_write(VirtIOSoundPCMStream *stream,
+static void virtio_snd_pcm_in_cb(void *data, int available);
+static uint32_t virtio_snd_pcm_write(VirtIOSoundPCMStream *stream,
   VirtQueue *vq,
-  VirtQueueElement *element,
-  bool read);
+  VirtQueueElement *element);
+static uint32_t virtio_snd_pcm_read(VirtIOSoundPCMStream *stream,
+  VirtQueue *vq,
+  VirtQueueElement *element);
 
 /*
  * Get a specific stream from the virtio sound card device.
@@ -450,15 +454,12 @@ static uint32_t virtio_snd_pcm_prepare_impl(VirtIOSound 
*s, uint32_t stream_id)
  &as);
 
 } else {
-qemu_log_mask(LOG_UNIMP, "virtio_snd: input/capture is 
unimplemented.");
-/*
- * stream->voice.in = AUD_open_in(&s->card,
- *stream->voice.in,
- *"virtio_snd_card",
- *stream,
- *virtio_snd_input_cb,
- *&as);
- */
+stream->voice.in = AUD_open_in(&s->card,
+stream->voice.in,
+"virtio_snd_card",
+stream,
+virtio_snd_pcm_in_cb,
+&as);
 }
 
 stream->as = as;
@@ -522,6 +523,8 @@ static void virtio_snd_handle_pcm_start_stop(VirtIOSound *s,
 if (stream) {
 if (stream->direction == VIRTIO_SND_D_OUTPUT) {
 AUD_set_active_out(stream->voice.out, start);
+} else {
+AUD_set_active_in(stream->voice.in, start);
 }
 } else {
 cmd->resp.code = VIRTIO_SND_S_BAD_MSG;
@@ -573,7 +576,11 @@ static uint32_t 
virtio_snd_pcm_release_impl(VirtIOSoundPCMStream *stream,
  */
 virtio_snd_process_cmdq(stream->s);
 trace_virtio_snd_pcm_stream_flush(stream_id);
-virtio_snd_pcm_flush(stream);
+if (stream->direction == VIRTIO_SND_D_OUTPUT) {
+virtio_snd_pcm_out_flush(stream);
+} else {
+virtio_snd_pcm_in_flush(stream);
+}
 }
 
 return VIRTIO_SND_S_OK;
@@ -754,7 +761,7 @@ static void virtio_snd_handle_event(VirtIODevice *vdev, 
VirtQueue *vq)
  * @vdev: VirtIOSound device
  * @vq: tx virtqueue
  */
-static void virtio_snd_handle_xfer(VirtIODevice *vdev, VirtQueue *vq)
+static void virtio_snd_handle_tx_xfer(VirtIODevice *vdev, VirtQueue *vq)
 {
 VirtIOSound *s = VIRTIO_SND(vdev);
 VirtIOSoundPCMStream *stream = NULL;
@@ -763,7 +770,7 @@ static void virtio_snd_handle_xfer(VirtIODevice *vdev, 
VirtQueue *vq)
 virtio_snd_pcm_xfer hdr;
 virtio_snd_pcm_status resp = { 0 };
 
-trace_virtio_snd_handle_xfer();
+trace_virtio_snd_handle_tx_xfe

[PATCH v2 1/3] Add virtio-sound device

[Manos Pitsidianakis]

This patch adds an audio device implementing the recent virtio sound
spec (1.2).

PCM functionality is implemented, and jack[0], chmaps[1] messages are
at the moment left unimplemented.

PS2: This patch was based on a draft patch posted by OpenSynergy in 2019. [2]

[0]: https://www.kernel.org/doc/html/latest/sound/designs/jack-controls.html
[1]: 
https://www.kernel.org/doc/html/latest/sound/designs/channel-mapping-api.html
[2]: 
https://github.com/OpenSynergy/qemu/commit/5a2f350eec5d157b90d9c7b40a8e603f4da92471#diff-fa5c10be8476fb5385a280885d527b0b40dfc11ddcc74369fce085d8b5b17bbd

Signed-off-by: Emmanouil Pitsidianakis 
Signed-off-by: Igor Skalkin 
Signed-off-by: Anton Yakovlev 
---
 MAINTAINERS|6 +
 hw/virtio/Kconfig  |5 +
 hw/virtio/meson.build  |1 +
 hw/virtio/trace-events |   21 +
 hw/virtio/virtio-snd.c | 1121 
 include/hw/virtio/virtio-snd.h |  194 ++
 6 files changed, 1348 insertions(+)
 create mode 100644 hw/virtio/virtio-snd.c
 create mode 100644 include/hw/virtio/virtio-snd.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 1c93ab0ee5..5d5475b1c1 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2218,6 +2218,12 @@ F: hw/virtio/virtio-mem-pci.h
 F: hw/virtio/virtio-mem-pci.c
 F: include/hw/virtio/virtio-mem.h
 
+virtio-snd
+M: Manos Pitsidianakis 
+S: Supported
+F: hw/virtio/virtio-snd*.c
+F: include/hw/virtio/virtio-snd.h
+
 nvme
 M: Keith Busch 
 M: Klaus Jensen 
diff --git a/hw/virtio/Kconfig b/hw/virtio/Kconfig
index 89e9e426d8..fcc522ed21 100644
--- a/hw/virtio/Kconfig
+++ b/hw/virtio/Kconfig
@@ -17,6 +17,11 @@ config VIRTIO_PCI
 depends on PCI
 select VIRTIO
 
+config VIRTIO_SND
+bool
+default y
+depends on VIRTIO
+
 config VIRTIO_MMIO
 bool
 select VIRTIO
diff --git a/hw/virtio/meson.build b/hw/virtio/meson.build
index bdec78bfc6..8cf49af618 100644
--- a/hw/virtio/meson.build
+++ b/hw/virtio/meson.build
@@ -68,5 +68,6 @@ softmmu_ss.add(when: 'CONFIG_VIRTIO', if_false: 
files('virtio-stub.c'))
 softmmu_ss.add(when: 'CONFIG_ALL', if_true: files('vhost-stub.c'))
 softmmu_ss.add(when: 'CONFIG_ALL', if_true: files('virtio-stub.c'))
 softmmu_ss.add(files('virtio-hmp-cmds.c'))
+softmmu_ss.add(when: 'CONFIG_VIRTIO_SND', if_true: files('virtio-snd.c'))
 
 specific_ss.add_all(when: 'CONFIG_VIRTIO', if_true: specific_virtio_ss)
diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
index 8f8d05cf9b..b77d78abdc 100644
--- a/hw/virtio/trace-events
+++ b/hw/virtio/trace-events
@@ -151,3 +151,24 @@ virtio_pmem_flush_done(int type) "fsync return=%d"
 virtio_gpio_start(void) "start"
 virtio_gpio_stop(void) "stop"
 virtio_gpio_set_status(uint8_t status) "0x%x"
+
+#virtio-snd.c
+virtio_snd_pcm_stream_flush(int stream) "flushing stream %d"
+virtio_snd_handle_ctrl(void *vdev, void *vq) "snd %p: handle ctrl event for 
queue %p"
+#virtio_snd_handle_jack_info(int jack) "VIRTIO_SND_JACK_INFO called for jack 
%d"
+#virtio_snd_handle_jack_remap(void) "VIRTIO_SND_PCM_JACK_REMAP called"
+virtio_snd_handle_pcm_info(int stream) "VIRTIO_SND_R_PCM_INFO called for 
stream %d"
+virtio_snd_handle_pcm_set_params(int stream) "VIRTIO_SND_PCM_SET_PARAMS called 
for stream %d"
+virtio_snd_handle_pcm_start_stop(const char *code, int stream) "%s called for 
stream %d"
+virtio_snd_handle_pcm_release(int stream) "VIRTIO_SND_PCM_RELEASE called for 
stream %d"
+virtio_snd_handle_chmap_info(void) "VIRTIO_SND_CHMAP_INFO called"
+virtio_snd_handle_xfer(void) "tx/rx queue callback called"
+virtio_snd_handle_event(void) "event queue callback called"
+virtio_snd_realize(void *snd) "snd %p: realize"
+virtio_snd_unrealize(void *snd) "snd %p: unrealize"
+virtio_snd_get_features(void *vdev, uint64_t features) "snd %p: get_features 
0x%"PRIx64
+virtio_snd_get_config(void *vdev, uint32_t jacks, uint32_t streams, uint32_t 
chmaps) "snd %p: get_config jacks=%d streams=%d chmaps=%d"
+virtio_snd_set_config(void *vdev, uint32_t jacks, uint32_t new_jacks, uint32_t 
streams, uint32_t new_streams, uint32_t chmaps, uint32_t new_chmaps) "snd %p: 
set_config jacks from %d->%d, streams from %d->%d, chmaps from %d->%d"
+virtio_snd_vm_state_running(void) "vm state running"
+virtio_snd_vm_state_stopped(void) "vm state stopped"
+virtio_snd_handle_code(int val, const char *code) "ctrl code msg val = %d == 
%s"
diff --git a/hw/virtio/virtio-snd.c b/hw/virtio/virtio-snd.c
new file mode 100644
index 00..e3b6c353ad
--- /dev/null
+++ b/hw/virtio/virtio-snd.c
@@ -0,0 +1,1121 @@
+/*
+ * VIRTIO Sound Device conforming to
+ *
+ * "Virtual I/O Device (VIRTIO) Version 1.2
+ * Committee Specificat

[PATCH v2 0/3] Add VIRTIO sound card

[Manos Pitsidianakis]

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

Main differences with v1 patch 
<20230526204845.673031-1-manos.pitsidiana...@linaro.org>:

- Split virtio-snd and virtio-snd-pci devices to two commits
- Added audio capture support

Known problems (On pipewire at least):

- Stereo recording results in one channel with the recording + a ticking 
  kind of artifact and one channel that has no artifacts, just the 
  recording.

Manos Pitsidianakis (3):
  Add virtio-sound device
  Add virtio-sound-pci device
  Implement audio capture in virtio-snd device

 MAINTAINERS|6 +
 hw/virtio/Kconfig  |5 +
 hw/virtio/meson.build  |2 +
 hw/virtio/trace-events |   22 +
 hw/virtio/virtio-snd-pci.c |  102 +++
 hw/virtio/virtio-snd.c | 1290 
 include/hw/pci/pci.h   |1 +
 include/hw/virtio/virtio-snd.h |  194 +
 softmmu/qdev-monitor.c |1 +
 9 files changed, 1623 insertions(+)
 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 v1:
1:  652c7d2d01 ! 1:  95d564fc1f Add virtio-sound and virtio-sound-pci devices
@@ Metadata
 Author: Manos Pitsidianakis 
 
  ## Commit message ##
-Add virtio-sound and virtio-sound-pci devices
+Add virtio-sound device
 
 This patch adds an audio device implementing the recent virtio sound
-spec (1.2) and a corresponding PCI wrapper device.
+spec (1.2).
 
 PCM functionality is implemented, and jack[0], chmaps[1] messages are
-at the moment ignored.
-
-To test this, you'll need a >6.0 kernel compiled with the virtio-snd
-flag enabled, which distros have off by default.
-
-Use with following flags in the invocation:
-
-  -device virtio-sound-pci,disable-legacy=on
-
-And an audio backend listed with `-audio driver=help` that works on
-your host machine, e.g.:
-
-Pulseaudio:
-  -audio 
driver=pa,model=virtio-sound,server=/run/user/1000/pulse/native
-sdl:
-  -audio driver=sdl,model=virtio-sound
-coreaudio:
-  -audio driver=coreaudio,model=virtio-sound
-etc.
-
-You can use speaker-test from alsa-tools to play noise, sines, or
-WAV files.
+at the moment left unimplemented.
 
 PS2: This patch was based on a draft patch posted by OpenSynergy in 
2019. [2]
 
@@ Commit message
 Signed-off-by: Igor Skalkin 
 Signed-off-by: Anton Yakovlev 
 
+ ## MAINTAINERS ##
+@@ MAINTAINERS: F: hw/virtio/virtio-mem-pci.h
+ F: hw/virtio/virtio-mem-pci.c
+ F: include/hw/virtio/virtio-mem.h
+ 
++virtio-snd
    ++M: Manos Pitsidianakis 
++S: Supported
++F: hw/virtio/virtio-snd*.c
++F: include/hw/virtio/virtio-snd.h
++
+ nvme
+ M: Keith Busch 
+ M: Klaus Jensen 
+
  ## hw/virtio/Kconfig ##
 @@ hw/virtio/Kconfig: config VIRTIO_PCI
  depends on PCI
@@ hw/virtio/Kconfig: config VIRTIO_PCI
  select VIRTIO
 
  ## hw/virtio/meson.build ##
-@@ hw/virtio/meson.build: virtio_pci_ss.add(when: 'CONFIG_VIRTIO_SERIAL', 
if_true: files('virtio-serial-pc
- virtio_pci_ss.add(when: 'CONFIG_VIRTIO_PMEM', if_true: 
files('virtio-pmem-pci.c'))
- virtio_pci_ss.add(when: 'CONFIG_VIRTIO_IOMMU', if_true: 
files('virtio-iommu-pci.c'))
- virtio_pci_ss.add(when: 'CONFIG_VIRTIO_MEM', if_true: 
files('virtio-mem-pci.c'))
-+virtio_pci_ss.add(when: 'CONFIG_VIRTIO_SND', if_true: 
files('virtio-snd-pci.c'))
- virtio_pci_ss.add(when: 'CONFIG_VHOST_VDPA_DEV', if_true: 
files('vdpa-dev-pci.c'))
- 
- specific_virtio_ss.add_all(when: 'CONFIG_VIRTIO_PCI', if_true: 
virtio_pci_ss)
 @@ hw/virtio/meson.build: softmmu_ss.add(when: 'CONFIG_VIRTIO', if_false: 
files('virtio-stub.c'))
  softmmu_ss.add(when: 'CONFIG_ALL', if_true: files('vhost-stub.c'))
  softmmu_ss.add(when: 'CONFIG_ALL', if_true: files('virtio-stub.c'))
@@ hw/virtio/trace-events: virtio_pmem_flush_done(int type) "fsync 
return=%d"
 +virtio_snd_handle_pcm_info(int stream) "VIRTIO_SND_R_PCM_INFO called for 
stream %d"
 +virtio_snd_handle_pcm_set_params(int stream) "VIRTIO_SND_PCM_SET_PARAMS 
called for stream %d"
 +virtio_snd_handle_pcm_start_stop(const char *code, int stream) "%s called 
for stream %d"
-+virtio_snd_handle_pcm_release(int stream) "VIRTIO_SND_PCM_RELEASE called 
for stream %id"
++virtio_snd_handle_pcm_release(in

[PATCH v2 2/3] Add virtio-sound-pci device

[Manos Pitsidianakis]

This patch adds a PCI wrapper device for the virtio-sound device.

To test this, you'll need a >=5.13 kernel compiled with
CONFIG_SND_VIRTIO=y, which at the time of writing most distros have off
by default.

Use with following flags in the invocation:

  -device virtio-sound-pci,disable-legacy=on

And an audio backend listed with `-audio driver=help` that works on
your host machine, e.g.:

Pulseaudio:
  -audio driver=pa,model=virtio-sound
sdl:
  -audio driver=sdl,model=virtio-sound
coreaudio (macos/darwin):
  -audio driver=coreaudio,model=virtio-sound
etc.

You can use speaker-test from alsa-tools to play noise, sines, or
WAV files.

Signed-off-by: Emmanouil Pitsidianakis 
---
---
 hw/virtio/meson.build  |   1 +
 hw/virtio/virtio-snd-pci.c | 102 +
 include/hw/pci/pci.h   |   1 +
 softmmu/qdev-monitor.c |   1 +
 4 files changed, 105 insertions(+)
 create mode 100644 hw/virtio/virtio-snd-pci.c

diff --git a/hw/virtio/meson.build b/hw/virtio/meson.build
index 8cf49af618..55d9426415 100644
--- a/hw/virtio/meson.build
+++ b/hw/virtio/meson.build
@@ -58,6 +58,7 @@ virtio_pci_ss.add(when: 'CONFIG_VIRTIO_SERIAL', if_true: 
files('virtio-serial-pc
 virtio_pci_ss.add(when: 'CONFIG_VIRTIO_PMEM', if_true: 
files('virtio-pmem-pci.c'))
 virtio_pci_ss.add(when: 'CONFIG_VIRTIO_IOMMU', if_true: 
files('virtio-iommu-pci.c'))
 virtio_pci_ss.add(when: 'CONFIG_VIRTIO_MEM', if_true: 
files('virtio-mem-pci.c'))
+virtio_pci_ss.add(when: 'CONFIG_VIRTIO_SND', if_true: 
files('virtio-snd-pci.c'))
 virtio_pci_ss.add(when: 'CONFIG_VHOST_VDPA_DEV', if_true: 
files('vdpa-dev-pci.c'))
 
 specific_virtio_ss.add_all(when: 'CONFIG_VIRTIO_PCI', if_true: virtio_pci_ss)
diff --git a/hw/virtio/virtio-snd-pci.c b/hw/virtio/virtio-snd-pci.c
new file mode 100644
index 00..1a41b53e07
--- /dev/null
+++ b/hw/virtio/virtio-snd-pci.c
@@ -0,0 +1,102 @@
+/*
+ * VIRTIO Sound Device PCI Bindings
+ *
+ * Copyright (c) 2023 Emmanouil Pitsidianakis 
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or
+ * (at your option) any later version.  See the COPYING file in the
+ * top-level directory.
+ */
+
+#include "qemu/osdep.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-base"
+DECLARE_INSTANCE_CHECKER(VirtIOSoundPCI, VIRTIO_SOUND_PCI,
+ TYPE_VIRTIO_SND_PCI)
+
+struct VirtIOSoundPCI {
+VirtIOPCIProxy parent;
+VirtIOSound vdev;
+};
+
+static Property virtio_snd_pci_properties[] = {
+DEFINE_PROP_UINT32("class", VirtIOPCIProxy, class_code, 0),
+DEFINE_PROP_END_OF_LIST(),
+};
+
+static const char *audiodev_id;
+
+static int virtio_snd_init_pci(PCIBus *init_bus, const char *audiodev)
+{
+audiodev_id = audiodev;
+return 0;
+}
+
+static void virtio_snd_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
+{
+VirtIOSoundPCI *dev = VIRTIO_SOUND_PCI(vpci_dev);
+DeviceState *vdev = DEVICE(&dev->vdev);
+VirtIOSound *vsnd = VIRTIO_SND(&dev->vdev);
+
+/*
+ * According to spec, non-legacy virtio PCI devices are always little
+ * endian
+ */
+vsnd->virtio_access_is_big_endian = false;
+
+
+qdev_set_parent_bus(vdev, BUS(&vpci_dev->bus), errp);
+
+qdev_prop_set_string(vdev, "audiodev", audiodev_id);
+
+object_property_set_bool(OBJECT(vdev), "realized", true, errp);
+}
+
+static void virtio_snd_pci_class_init(ObjectClass *klass, void *data)
+{
+DeviceClass *dc = DEVICE_CLASS(klass);
+VirtioPCIClass *vpciklass = VIRTIO_PCI_CLASS(klass);
+PCIDeviceClass *pcidev_k = PCI_DEVICE_CLASS(klass);
+
+vpciklass->realize = virtio_snd_pci_realize;
+set_bit(DEVICE_CATEGORY_SOUND, dc->categories);
+
+pcidev_k->vendor_id = PCI_VENDOR_ID_REDHAT_QUMRANET;
+pcidev_k->device_id = PCI_DEVICE_ID_VIRTIO_SND;
+pcidev_k->revision = VIRTIO_PCI_ABI_VERSION;
+pcidev_k->class_id = PCI_CLASS_MULTIMEDIA_AUDIO;
+device_class_set_props(dc, virtio_snd_pci_properties);
+}
+
+static void virtio_snd_pci_instance_init(Object *obj)
+{
+VirtIOSoundPCI *dev = VIRTIO_SOUND_PCI(obj);
+
+virtio_instance_init_common(obj, &dev->vdev, sizeof(dev->vdev),
+TYPE_VIRTIO_SND);
+}
+
+static const VirtioPCIDeviceTypeInfo virtio_snd_pci_info = {
+.base_name = TYPE_VIRTIO_SND_PCI,
+.generic_name  = "virtio-sound-pci",
+.instance_size = sizeof(VirtIOSoundPCI),
+.instance_init = virtio_snd_pci_instance_init,
+.class_init= virtio_snd_pci_class_init,
+};
+
+static void virtio_snd_pci_register(void)
+{
+virtio_pci_types_register(&virtio_snd_pci_info);
+pci_register_soundhw("virtio-sound", "Virtio Sound Device",
+ virtio_snd_init_pci);
+}
+
+type_init(virtio_snd_pci_register);
diff --git a/include/hw/pci/pci.h b/include/hw/pci/pc

Re: [PATCH] Add virtio-sound and virtio-sound-pci devices

[Manos Pitsidianakis]

On Wed, 31 May 2023 12:36, Alex Bennée  wrote:

If it's based of shouldn't we keep the author attribution and their
original s-o-b?



I kept the Copyright in the headers but not the Signed-off-by lines, 
I'll add them in v2.




What about:

 
https://patchew.org/QEMU/20220211221319.193404-1-chouhan.shreyansh2...@gmail.com/

(which is also much more nicely split up).


Comments in that thread were:


IMHO, all your patches can be merged in only one


and


Possibly also patches adding
significant functionality in the future (i.e. one patch with all
basics and playback support, one patch adding recording
functionality, ...).


which is why capture was left unimplemented for a future patch:


+
+} else {
+/*
+ * Unimplemented.
+ * stream->voice.in = AUD_open_in(&s->card,
+ *stream->voice.in,
+ *"virtio_snd_card",
+ *stream,
+ *virtio_snd_input_cb,
+ *&as);
+ */


 qemu_log(LOG_UNIMP, ) - although why not implement it?


+}

[PATCH] Add virtio-sound and virtio-sound-pci devices

[Manos Pitsidianakis]

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

PCM functionality is implemented, and jack[0], chmaps[1] messages are
at the moment ignored.

To test this, you'll need a >6.0 kernel compiled with the virtio-snd
flag enabled, which distros have off by default.

Use with following flags in the invocation:

  -device virtio-sound-pci,disable-legacy=on

And an audio backend listed with `-audio driver=help` that works on
your host machine, e.g.:

Pulseaudio:
  -audio driver=pa,model=virtio-sound,server=/run/user/1000/pulse/native
sdl:
  -audio driver=sdl,model=virtio-sound
coreaudio:
  -audio driver=coreaudio,model=virtio-sound
etc.

You can use speaker-test from alsa-tools to play noise, sines, or
WAV files.

PS2: This patch was based on a draft patch posted by opensynergy a few
 years ago.

[0]: https://www.kernel.org/doc/html/latest/sound/designs/jack-controls.html
[1]: 
https://www.kernel.org/doc/html/latest/sound/designs/channel-mapping-api.html

Signed-off-by: Emmanouil Pitsidianakis 
---
 hw/audio/Kconfig   |5 +
 hw/audio/meson.build   |1 +
 hw/audio/trace-events  |   23 +
 hw/audio/virtio-snd.c  | 1139 
 hw/virtio/Kconfig  |5 +
 hw/virtio/meson.build  |1 +
 hw/virtio/virtio-snd-pci.c |  104 +++
 include/hw/pci/pci.h   |1 +
 include/hw/virtio/virtio-snd.h |  193 ++
 softmmu/qdev-monitor.c |1 +
 10 files changed, 1473 insertions(+)
 create mode 100644 hw/audio/virtio-snd.c
 create mode 100644 hw/virtio/virtio-snd-pci.c
 create mode 100644 include/hw/virtio/virtio-snd.h

diff --git a/hw/audio/Kconfig b/hw/audio/Kconfig
index e76c69ca7e..74afa21c50 100644
--- a/hw/audio/Kconfig
+++ b/hw/audio/Kconfig
@@ -47,3 +47,8 @@ config PL041
 
 config CS4231
 bool
+
+config VIRTIO_SND
+bool
+default y
+depends on VIRTIO
diff --git a/hw/audio/meson.build b/hw/audio/meson.build
index e48a9fc73d..455e6a1501 100644
--- a/hw/audio/meson.build
+++ b/hw/audio/meson.build
@@ -12,3 +12,4 @@ softmmu_ss.add(when: 'CONFIG_PL041', if_true: 
files('pl041.c', 'lm4549.c'))
 softmmu_ss.add(when: 'CONFIG_SB16', if_true: files('sb16.c'))
 softmmu_ss.add(when: 'CONFIG_VT82C686', if_true: files('via-ac97.c'))
 softmmu_ss.add(when: 'CONFIG_WM8750', if_true: files('wm8750.c'))
+softmmu_ss.add(when: 'CONFIG_VIRTIO_SND', if_true: files('virtio-snd.c'))
diff --git a/hw/audio/trace-events b/hw/audio/trace-events
index 4dec48a4fd..d8ade63f13 100644
--- a/hw/audio/trace-events
+++ b/hw/audio/trace-events
@@ -17,3 +17,26 @@ via_ac97_codec_write(uint8_t addr, uint16_t val) "0x%x <- 
0x%x"
 via_ac97_sgd_fetch(uint32_t curr, uint32_t addr, char stop, char eol, char 
flag, uint32_t len) "curr=0x%x addr=0x%x %c%c%c len=%d"
 via_ac97_sgd_read(uint64_t addr, unsigned size, uint64_t val) "0x%"PRIx64" %d 
-> 0x%"PRIx64
 via_ac97_sgd_write(uint64_t addr, unsigned size, uint64_t val) "0x%"PRIx64" %d 
<- 0x%"PRIx64
+
+#virtio-snd.c
+virtio_snd_pcm_stream_flush(int stream) "flushing stream %d"
+virtio_snd_handle_ctrl(void *vdev, void *vq) "snd %p: handle ctrl event for 
queue %p"
+#virtio_snd_handle_jack_info(int jack) "VIRTIO_SND_JACK_INFO called for jack 
%d"
+#virtio_snd_handle_jack_remap(void) "VIRTIO_SND_PCM_JACK_REMAP called"
+virtio_snd_handle_pcm_info(int stream) "VIRTIO_SND_R_PCM_INFO called for 
stream %d"
+virtio_snd_handle_pcm_set_params(int stream) "VIRTIO_SND_PCM_SET_PARAMS called 
for stream %d"
+virtio_snd_handle_pcm_start_stop(const char *code, int stream) "%s called for 
stream %d"
+virtio_snd_handle_pcm_release(int stream) "VIRTIO_SND_PCM_RELEASE called for 
stream %id"
+virtio_snd_handle_chmap_info(void) "VIRTIO_SND_CHMAP_INFO called"
+virtio_snd_handle_xfer(void) "tx/rx queue callback called"
+virtio_snd_handle_xfer_elem(const char * k) "xfer handled in virtio_snd_pcm_%s"
+virtio_snd_handle_event(void) "event queue callback called"
+virtio_snd_cpu_is_stopped(void *snd, int size) "snd %p: cpu is stopped, 
dropping %d bytes"
+virtio_snd_realize(void *snd) "snd %p: realize"
+virtio_snd_unrealize(void *snd) "snd %p: realize"
+virtio_snd_get_features(void *vdev, uint64_t features) "snd %p: get_features 
0x%"PRIx64
+virtio_snd_get_config(void *vdev, uint32_t jacks, uint32_t streams, uint32_t 
chmaps) "snd %p: get_config jacks=%d streams=%d chmaps=%d"
+virtio_snd_set_config(void *vdev, uint32_t jacks, uint32_t new_jacks, uint32_t 
streams, uint32_t new_streams, uint32_t chmaps, uint32_t new_chmaps) "snd %p: 
set_config jacks from %d->%d, streams from %d->%d, chmaps from %d->%d"
+virtio_snd_vm_state_running(void) "vm state running"
+virtio_snd_vm_state_stopped(void) "vm state stopped"
+virtio_snd_handle_code(int val, const char *code) "ctrl code msg val = %d == 
%s"
diff --git a/hw/audio/virtio-snd.c b/hw/audio/virtio-snd.c
new file mode 100644
index 00..3bf657f368
--- /dev/null
+++ b/hw/audio/virtio-snd.c
@@ -0,0 

Re: [Qemu-devel] Throttling filter node status?

[Manos Pitsidianakis]

On Wed, Feb 07, 2018 at 06:35:20PM +, Stefan Hajnoczi wrote:

Hi Manos,
I hope you're doing well and that you've had a good time at university
after Google Summer of Code.

Do you have time to work on getting the remainder of your GSoC work
merged over the coming weeks?

If not, don't feel bad but please let us know so someone can take over
the patches.

Stefan


Hi Stefan, thanks for the reminder. I will try to catch up in the 
following days.


signature.asc
Description: PGP signature

Re: [Qemu-devel] [Qemu-block] [PATCH v2] throttle: fix a qemu crash problem when calling blk_delete

[Manos Pitsidianakis]

On Fri, Nov 03, 2017 at 02:26:33PM +, Stefan Hajnoczi wrote:

On Wed, Oct 25, 2017 at 10:40:47AM +0200, Alberto Garcia wrote:

On Tue 24 Oct 2017 05:33:51 AM CEST, sochin jiang wrote:
> --- a/block/throttle-groups.c
> +++ b/block/throttle-groups.c
> @@ -576,7 +576,9 @@ void throttle_group_unregister_tgm(ThrottleGroupMember 
*tgm)
>
>  /* remove the current tgm from the list */
>  QLIST_REMOVE(tgm, round_robin);
> -throttle_timers_destroy(&tgm->throttle_timers);
> +if (throttle_timers_are_initialized(&tgm->throttle_timers)) {
> +throttle_timers_destroy(&tgm->throttle_timers);
> +}
>  qemu_mutex_unlock(&tg->lock);
>
>  throttle_group_unref(&tg->ts);

I don't know what the rest of the people think, but I'm not completely
convinced that it's a good idea to have an active ThrottleState inside a
ThrottleGroupMember without timers.

Perhaps in blk_remove_bs() after detaching the AioContext from the BDS
we can attach the default one (what you would get with
blk_get_aio_context()).

On the other hand I think that Manos's series to remove the legacy
throttling code gets rid of BlockBackend.ThrottleGroupMember completely.


What is the status of Manos' series?

It would be good to merge it and then consider currently open throttling
bugs again.



I have been busy with schoolwork the past weeks, but I'm close to 
finishing some changes Kevin recommended for this series. I hope they 
will be ready soon.


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH v13 2/6] qmp: Use ThrottleLimits structure

[Manos Pitsidianakis]

On Fri, Oct 13, 2017 at 09:26:17AM -0500, Eric Blake wrote:

[adding Markus, and block list]

On 10/13/2017 09:16 AM, Alberto Garcia wrote:

On Mon 02 Oct 2017 04:33:28 PM CEST, Pradeep Jagadeesh wrote:

This patch factors out code to use the ThrottleLimits
structure.



 { 'struct': 'BlockIOThrottle',
-  'data': { '*device': 'str', '*id': 'str', 'bps': 'int', 'bps_rd': 'int',
-'bps_wr': 'int', 'iops': 'int', 'iops_rd': 'int', 'iops_wr': 'int',
-'*bps_max': 'int', '*bps_rd_max': 'int',
-'*bps_wr_max': 'int', '*iops_max': 'int',
-'*iops_rd_max': 'int', '*iops_wr_max': 'int',
-'*bps_max_length': 'int', '*bps_rd_max_length': 'int',
-'*bps_wr_max_length': 'int', '*iops_max_length': 'int',
-'*iops_rd_max_length': 'int', '*iops_wr_max_length': 'int',
-'*iops_size': 'int', '*group': 'str' } }
+  'base': 'ThrottleLimits',
+  'data': { '*device': 'str', '*id': 'str', '*group': 'str' } }


So BlockIOThrottle used to have parameters named bps_rd and iops_wr, and
after this patch they become bps-read and iops-write. This breaks the
API completely, as you can see if you run e.g. iotest 129:

AssertionError: failed path traversal for "return" in "{u'error': {u'class': u'GenericError', 
u'desc': u"Parameter 'iops_rd' is unexpected"}}"

I just checked previous versions of the series and I see that Manos
already warned you of this in v11:

   https://lists.gnu.org/archive/html/qemu-devel/2017-09/msg04698.html


On the bright side, ThrottleLimits is marked 'since 2.11' (added in
commit 432d889e), meaning it has not yet been released, so we CAN fix
the naming in ThrottleLimits to be compatible with BlockIOThrottle if we
want to share the type, as long as we get it done before the 2.11
release.


We decided to keep BlockIOThrottle separate from ThrottleLimits because 
that would break the old I/O throttling API, just like is done in this 
patch series.  BlockIOThrottle is the one using old naming conventions 
so I think it should be the one to go, if that has to be done.


But this all boils down to whether the legacy throttling API has to 
break in 2.11 or not, which probably is the maintainer's decision.




It does mean tweaking Manos' code to use compatible names
everywhere, but that may be a wise course of action (we tend to favor
'-' in new API names unless there is a strong reason to use '_'; but
sharing code for maximum back-compat would be a reason to use '_').

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org







signature.asc
Description: PGP signature

Re: [Qemu-devel] [Qemu-block] [PATCH RFC] block: add block-insert-node QMP command

[Manos Pitsidianakis]

On Fri, Oct 06, 2017 at 02:59:55PM +0200, Max Reitz wrote:

On 2017-10-04 23:04, Manos Pitsidianakis wrote:

On Wed, Oct 04, 2017 at 08:09:24PM +0200, Max Reitz wrote:

On 2017-10-04 19:05, Manos Pitsidianakis wrote:

On Wed, Oct 04, 2017 at 02:49:27PM +0200, Max Reitz wrote:

On 2017-08-15 09:45, Manos Pitsidianakis wrote:

block-insert-node and its pair command block-remove-node provide
runtime
insertion and removal of filter nodes.

block-insert-node takes a (parent, child) and (node, child) pair of
edges and unrefs the (parent, child) BdrvChild relationship and
creates
a new (parent, node) child with the same BdrvChildRole.

This is a different approach than x-blockdev-change which uses the
driver
methods bdrv_add_child() and bdrv_del_child(),


Why? :-)

Can't we reuse x-blockdev-change? As far as I'm concerned, this was one
of its roles, and at least I don't want to have both x-blockdev-change
and these new commands.

I think it would be a good idea just to change bdrv_add_child() and
bdrv_del_child(): If the driver has a bdrv_{add,del}_child() callback,
invoke that.  If it doesn't, then just attach the child.

Of course, it may turn out that x-blockdev-change is lacking something
(e.g. a way to specify as what kind of child a new node is to be
attached).  Then we should either extend it so that it covers what it's
lacking, or replace it completely by these new commands.  In the latter
case, however, they would need to cover the existing use case which is
reconfiguring the quorum driver.  (And that would mean it would have to
invoke bdrv_add_child()/bdrv_del_child() when the driver has them.)

Max



I think the two use cases are this:

a) Adding/removing a replication child to an existing quorum node
b) Insert a filter between two existing nodes.


For me both are the same: Adding/removing nodes into/from the graph.

The difference is how those children are added (or removed, but it's the
same in reverse): In case of quorum, you can simply attach a new child
because it supports a variable number of children.  Another case where
this would work are all block drivers which support backing files (you
can freely attach/detach those).


Doesn't blockdev-snapshot-sync cover this? (I may be missing something).


Not really, because it doesn't add a new child.  But it's still a good
point, because your block-insert-node command would make it obsolete,
actually.

blockdev-snapshot literally is a special-cased block-insert-node.  And
blockdev-snapshot-sync then is qemu-img create + blockdev-add +
blockdev-snapshot.


Now that we're on this topic, quorum might be a good candidate for
*_reopen when and if it lands on QMP: Reconfiguring the children could
be done by reopening the BDS with new options.


Hmmm...  I guess that would work, too.  But intuitively, that seems a
bit heavy-weight to me


In this series, it's not about adding or removing new children, but
instead "injecting" them into an edge: An existing child is replaced,
but it now serves as some child of the new one.

(I guess writing too much trying to get my point across, sorry...)

The gist is that for me it's not so much about "quorum" or "filter
nodes".  It's about adding a new child to a node vs. replacing an
existing one.


These are not directly compatible semantically, but as you said
x-blockdev-change can perform b) if bdrv_add_child()/bdrv_del_child()
are not implemented. Wouldn't that be unnecessary overloading?


Yes, I think it would be. :-)

So say we have these two trees in our graph:

[ Parent BDS -> Child BDS ]
[ Filter BDS -> Child BDS ]

So here's what you can do with x-blockdev-change (in theory; in practice
it can only do that for quorum):
- Remove a child, so
   [ Parent BDS -> Child BDS ]
 is split into two trees
   [ Parent BDS ] and
   [ Child BDS ]
- Add a child, so we can merge
   [ Parent BDS ] and
   [ Filter BDS -> Child BDS ]
 into
   [ Parent BDS -> Filter BDS -> Child BDS ]



Yes, of course this would have to be done in one transaction.


Would it?  If you want to put Filter BDS into the chain, you can just
create [ Filter BDS ] without any child, and then use a single
x-blockdev-change invocation to inject it.

The only thing that's necessary is that the filter BDS would have to be
able to handle having no child.


Yeah that was what I had in mind.


[...]


So after I've written all of this, I feel like the new insert-node and
remove-node commands are exactly what x-blockdev-change should do when
asked to replace a node.  The only difference is that x-blockdev-change
would allow you to replace any node with anything, without the
constraints that block-insert-node and block-remove-node exact.

(And node replacement with x-blockdev-change would work by specifying
all three parameters.)

Not sure if that makes sense, I hope it does. :-)



Hm, I can't think of a way to fit that

Re: [Qemu-devel] [Qemu-block] [PATCH RFC] block: add block-insert-node QMP command

[Manos Pitsidianakis]

On Wed, Oct 04, 2017 at 08:09:24PM +0200, Max Reitz wrote:

On 2017-10-04 19:05, Manos Pitsidianakis wrote:

On Wed, Oct 04, 2017 at 02:49:27PM +0200, Max Reitz wrote:

On 2017-08-15 09:45, Manos Pitsidianakis wrote:

block-insert-node and its pair command block-remove-node provide runtime
insertion and removal of filter nodes.

block-insert-node takes a (parent, child) and (node, child) pair of
edges and unrefs the (parent, child) BdrvChild relationship and creates
a new (parent, node) child with the same BdrvChildRole.

This is a different approach than x-blockdev-change which uses the
driver
methods bdrv_add_child() and bdrv_del_child(),


Why? :-)

Can't we reuse x-blockdev-change? As far as I'm concerned, this was one
of its roles, and at least I don't want to have both x-blockdev-change
and these new commands.

I think it would be a good idea just to change bdrv_add_child() and
bdrv_del_child(): If the driver has a bdrv_{add,del}_child() callback,
invoke that.  If it doesn't, then just attach the child.

Of course, it may turn out that x-blockdev-change is lacking something
(e.g. a way to specify as what kind of child a new node is to be
attached).  Then we should either extend it so that it covers what it's
lacking, or replace it completely by these new commands.  In the latter
case, however, they would need to cover the existing use case which is
reconfiguring the quorum driver.  (And that would mean it would have to
invoke bdrv_add_child()/bdrv_del_child() when the driver has them.)

Max



I think the two use cases are this:

a) Adding/removing a replication child to an existing quorum node
b) Insert a filter between two existing nodes.


For me both are the same: Adding/removing nodes into/from the graph.

The difference is how those children are added (or removed, but it's the
same in reverse): In case of quorum, you can simply attach a new child
because it supports a variable number of children.  Another case where
this would work are all block drivers which support backing files (you
can freely attach/detach those).


Doesn't blockdev-snapshot-sync cover this? (I may be missing something).
Now that we're on this topic, quorum might be a good candidate for 
*_reopen when and if it lands on QMP: Reconfiguring the children could 
be done by reopening the BDS with new options.


In this series, it's not about adding or removing new children, but
instead "injecting" them into an edge: An existing child is replaced,
but it now serves as some child of the new one.

(I guess writing too much trying to get my point across, sorry...)

The gist is that for me it's not so much about "quorum" or "filter
nodes".  It's about adding a new child to a node vs. replacing an
existing one.


These are not directly compatible semantically, but as you said
x-blockdev-change can perform b) if bdrv_add_child()/bdrv_del_child()
are not implemented. Wouldn't that be unnecessary overloading?


Yes, I think it would be. :-)

So say we have these two trees in our graph:

[ Parent BDS -> Child BDS ]
[ Filter BDS -> Child BDS ]

So here's what you can do with x-blockdev-change (in theory; in practice
it can only do that for quorum):
- Remove a child, so
   [ Parent BDS -> Child BDS ]
 is split into two trees
   [ Parent BDS ] and
   [ Child BDS ]
- Add a child, so we can merge
   [ Parent BDS ] and
   [ Filter BDS -> Child BDS ]
 into
   [ Parent BDS -> Filter BDS -> Child BDS ]



Yes, of course this would have to be done in one transaction.


However, this is only possible with quorum because usually block drivers
don't support detaching children.

And here's what you can do with your commands (from what I can see):
- Replace a child (you call it insertion, but it really is just
 replacement of an existing child with the constraint that both nodes
 involved must have the same child):
   [ Parent BDS -> Child BDS ]
   [ Filter BDS -> Child BDS ]
 to
   [ Parent BDS -> Filter BDS -> Child BDS ]
- Replace a child (you call it removal, but it really is just
 replacement of a child with its child):
   [ Parent BDS -> Filter BDS -> Child BDS ]
 to
   [ Parent BDS -> Child BDS ]

This works on all BDSs because you don't change the number of children.


The interesting thing of course is that the "change" command can
actually add and remove children; where as the "insert" and "remove"
commands can only replace children.  So that's already a bit funny (one
command does two things; two commands do one thing).


That is true, but the replacing is more in terms of inserting and 
removing a node in a BDS chain.


And then of course you can simply modify x-blockdev-change so it can do
the same thing block-insert-node and block-remove-node can do: It just
needs another mode which can be used to replace a child (and its
description already states 

Re: [Qemu-devel] [Qemu-block] [PATCH RFC] block: add block-insert-node QMP command

[Manos Pitsidianakis]

On Wed, Oct 04, 2017 at 02:49:27PM +0200, Max Reitz wrote:

On 2017-08-15 09:45, Manos Pitsidianakis wrote:

block-insert-node and its pair command block-remove-node provide runtime
insertion and removal of filter nodes.

block-insert-node takes a (parent, child) and (node, child) pair of
edges and unrefs the (parent, child) BdrvChild relationship and 
creates

a new (parent, node) child with the same BdrvChildRole.

This is a different approach than x-blockdev-change which uses the driver
methods bdrv_add_child() and bdrv_del_child(),


Why? :-)

Can't we reuse x-blockdev-change? As far as I'm concerned, this was one
of its roles, and at least I don't want to have both x-blockdev-change
and these new commands.

I think it would be a good idea just to change bdrv_add_child() and
bdrv_del_child(): If the driver has a bdrv_{add,del}_child() callback,
invoke that.  If it doesn't, then just attach the child.

Of course, it may turn out that x-blockdev-change is lacking something
(e.g. a way to specify as what kind of child a new node is to be
attached).  Then we should either extend it so that it covers what it's
lacking, or replace it completely by these new commands.  In the latter
case, however, they would need to cover the existing use case which is
reconfiguring the quorum driver.  (And that would mean it would have to
invoke bdrv_add_child()/bdrv_del_child() when the driver has them.)

Max



I think the two use cases are this:

a) Adding/removing a replication child to an existing quorum node
b) Insert a filter between two existing nodes.

These are not directly compatible semantically, but as you said 
x-blockdev-change can perform b) if bdrv_add_child()/bdrv_del_child() 
are not implemented. Wouldn't that be unnecessary overloading?


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH RFC] block: add block-insert-node QMP command

[Manos Pitsidianakis]

On Fri, Sep 29, 2017 at 07:52:35PM +0200, Kevin Wolf wrote:

Am 15.08.2017 um 09:45 hat Manos Pitsidianakis geschrieben:

block-insert-node and its pair command block-remove-node provide runtime
insertion and removal of filter nodes.

block-insert-node takes a (parent, child) and (node, child) pair of
edges and unrefs the (parent, child) BdrvChild relationship and creates
a new (parent, node) child with the same BdrvChildRole.

This is a different approach than x-blockdev-change which uses the driver
methods bdrv_add_child() and bdrv_del_child(),

Signed-off-by: Manos Pitsidianakis 



diff --git a/qapi/block-core.json b/qapi/block-core.json
index 4d6ba1baef..16e19cb648 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3947,3 +3947,63 @@
   'data' : { 'parent': 'str',
  '*child': 'str',
  '*node': 'str' } }
+
+##
+# @block-insert-node:
+#
+# Insert a filter node between a specific edge in the block driver state graph.
+# @parent:  the name of the parent node or device
+# @node:the name of the node to insert under parent
+# @child:   the name of the child of both node and parent
+#
+# Example:
+# Insert and remove a throttle filter on top of a device chain, between the
+# device 'ide0-hd0' and node 'node-A'
+#
+# -> {'execute': 'object-add',
+# "arguments": {
+#   "qom-type": "throttle-group",
+#   "id": "group0",
+#   "props" : { "limits": { "iops-total": 300 } }
+# }
+#}
+# <- { 'return': {} }
+# -> {'execute': 'blockdev-add',
+#   'arguments': {
+#   'driver': 'throttle',
+#   'node-name': 'throttle0',
+#   'throttle-group': 'group0',
+#   'file': 'node-A'
+#   }
+#}
+# <- { 'return': {} }
+# -> { 'execute': 'block-insert-node',
+#   'arguments': { 'parent': 'ide0-hd0', 'child': 'node-A', 'node': 
'throttle0' }
+#}
+# <- { 'return': {} }
+# -> { 'execute': 'block-remove-node',
+#   'arguments': { 'parent': 'ide0-hd0', 'child': 'node-A', 'node': 
'throttle0' }
+#}
+# <- { 'return': {} }
+# -> { 'execute': 'blockdev-del',
+#   'arguments': { 'node-name': 'throttle0' }
+#}
+# <- { 'return': {} }
+#
+##
+{ 'command': 'block-insert-node',
+  'data': { 'parent': 'str',
+ 'child': 'str',
+ 'node': 'str'} }


I would suggest a change to the meaning of @child: Instead of using the
node-name of the child BDS, I would use the name of the BdrvChild that
represents the link.

The reason for this is that the node-name could be ambiguous, if you
have two edges between the same two nodes.

The only use of the node-name of the child that I can remember was for
checking that the graph still looks like what the user expects. But I
think we came to the conclusion that there are no race conditions to
check for if we have manual block job deletion instead of automatic
completion which can involve surprise changes to the graph. So probably
we don't need the node-name even for this.


+##
+# @block-remove-node:
+#
+# Remove a filter node between two other nodes in the block driver state graph.
+# @parent:  the name of the parent node or device
+# @node:the name of the node to remove from parent
+# @child:   the name of the child of node which will go under parent
+##
+{ 'command': 'block-remove-node',
+  'data': { 'parent': 'str',
+ 'child': 'str',
+ 'node': 'str'} }


Same thing here.


diff --git a/block.c b/block.c
index 81bd51b670..f874aabbfb 100644
--- a/block.c
+++ b/block.c
+/* insert 'node' as child bs of 'parent' node */
+if (check_node_edge(parent, child, errp)) {
+return;
+}
+parent_bs = bdrv_find_node(parent);
+c = bdrv_find_child(parent_bs, child);
+role = c->role;
+assert(role == &child_file || role == &child_backing);
+
+bdrv_ref(node_bs);
+
+bdrv_drained_begin(parent_bs);
+bdrv_unref_child(parent_bs, c);
+if (role == &child_file) {
+parent_bs->file = bdrv_attach_child(parent_bs, node_bs, "file",
+&child_file, errp);
+if (!parent_bs->file) {
+parent_bs->

Re: [Qemu-devel] [PATCH v3 0/3] add bdrv_co_drain_begin/end BlockDriver callbacks

[Manos Pitsidianakis]

On Tue, Sep 26, 2017 at 12:00:24PM +0100, Stefan Hajnoczi wrote:

On Sat, Sep 23, 2017 at 02:14:08PM +0300, Manos Pitsidianakis wrote:

This patch series renames bdrv_co_drain to bdrv_co_drain_begin and adds a new
bdrv_co_drain_end callback to match bdrv_drained_begin/end and
drained_begin/end of BdrvChild. This is needed because the throttle driver
(block/throttle.c) needs a way to mark the end of the drain in order to toggle
io_limits_disabled correctly.

Based-on: <20170918202529.28379-1-el13...@mail.ntua.gr>
"block/throttle-groups.c: allocate RestartData on the heap"
Which fixes a coroutine crash in block/throttle-groups.c

v3:
  fixed commit message typo in first patch [Fam]
  rephrased doc comment based on mailing discussion
v2:
  add doc for callbacks and change order of request polling for completion
  [Stefan]

Manos Pitsidianakis (3):
  block: add bdrv_co_drain_end callback
  block: rename bdrv_co_drain to bdrv_co_drain_begin
  block/throttle.c: add bdrv_co_drain_begin/end callbacks

 include/block/block_int.h | 13 ++---
 block/io.c| 48 +--
 block/qed.c   |  6 +++---
 block/throttle.c  | 18 ++
 4 files changed, 65 insertions(+), 20 deletions(-)


Oops, this seems to cause a qemu-iotests failure.  Please take a look:

$ ./check -qcow2 184
184 0s ... - output mismatch (see 184.out.bad)
--- /home/stefanha/qemu/tests/qemu-iotests/184.out  2017-09-19 
14:51:46.673854437 +0100
+++ 184.out.bad 2017-09-26 11:13:06.946610239 +0100
@@ -142,6 +142,9 @@
"guest": false
}
}
+./common.config: line 118:  9196 Segmentation fault  (core dumped) ( if [ -n 
"${QEMU_NEED_PID}" ]; then
+echo $BASHPID > "${QEMU_TEST_DIR}/qemu-${_QEMU_HANDLE}.pid";
+fi; exec "$QEMU_PROG" $QEMU_OPTIONS "$@" )



Hey Stefan,

This is fixed in 

Based-on: <20170918202529.28379-1-el13...@mail.ntua.gr>
"block/throttle-groups.c: allocate RestartData on the heap"
Which fixes a coroutine crash in block/throttle-groups.c


Which I sent before this series and is in Kevin's branch.


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH v2 1/4] qom: provide root container for internal objs

[Manos Pitsidianakis]

On Mon, Sep 25, 2017 at 04:14:26PM +0800, Peter Xu wrote:

On Mon, Sep 25, 2017 at 09:14:21AM +0200, Andreas Färber wrote:

Am 25.09.2017 um 08:37 schrieb Peter Xu:
> We have object_get_objects_root() to keep user created objects, however
> no place for objects that will be used internally.  Create such a
> container for internal objects.
>
> CC: Andreas Färber 
> CC: Markus Armbruster 
> CC: Paolo Bonzini 
> Suggested-by: Daniel P. Berrange 
> Signed-off-by: Peter Xu 
> ---
>  include/qom/object.h | 10 ++
>  qom/object.c |  5 +
>  2 files changed, 15 insertions(+)
>
> diff --git a/include/qom/object.h b/include/qom/object.h
> index f3e5cff..f567052 100644
> --- a/include/qom/object.h
> +++ b/include/qom/object.h
> @@ -1214,6 +1214,16 @@ Object *object_get_root(void);
>  Object *object_get_objects_root(void);
>
>  /**
> + * object_get_internal_root:
> + *
> + * Get the container object that holds internally used object
> + * instances. This is the object at path "/internal-objects"
> + *
> + * Returns: the internal object container
> + */
> +Object *object_get_internal_root(void);
> +
> +/**
>   * object_get_canonical_path_component:
>   *
>   * Returns: The final component in the object's canonical path.  The 
canonical
> diff --git a/qom/object.c b/qom/object.c
> index 3e18537..857cee7 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -1370,6 +1370,11 @@ Object *object_get_objects_root(void)
>  return container_get(object_get_root(), "/objects");
>  }
>
> +Object *object_get_internal_root(void)
> +{
> +return container_get(object_get_root(), "/internal-objects");

Whatever you expose in the QOM tree is no longer internal. Other name?


Hi, Andreas,

If you mean "info qom-tree" here, can we still treat it as internal?
Since after all it's not exposed in QMP (while IMHO QMP is the
official protocol for QEMU clients).  And IIUC some HMP commands do
dump some internal structs for debugging purpose (like: "info
ramblock").

Or, do you have any suggestion?

(I did think about something like "hidden-objects", but I believe they
are not hidden as well if we think they are not internal...)

Thanks,



Regards,
Andreas


If there's no need for internal IOThreads to be together with user 
created ones, a simple queue will be enough (include/qemu/queue.h).


signature.asc
Description: PGP signature

[Qemu-devel] [PATCH v3 2/3] block: rename bdrv_co_drain to bdrv_co_drain_begin

[Manos Pitsidianakis]

Reviewed-by: Stefan Hajnoczi 
Reviewed-by: Fam Zheng 
Signed-off-by: Manos Pitsidianakis 
---
 include/block/block_int.h | 4 ++--
 block/io.c| 4 ++--
 block/qed.c   | 6 +++---
 3 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/include/block/block_int.h b/include/block/block_int.h
index 9ebdeb6db0..51575d22b6 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -354,7 +354,7 @@ struct BlockDriver {
 int (*bdrv_probe_geometry)(BlockDriverState *bs, HDGeometry *geo);
 
 /**
- * bdrv_co_drain is called if implemented in the beginning of a
+ * bdrv_co_drain_begin is called if implemented in the beginning of a
  * drain operation to drain and stop any internal sources of requests in
  * the driver.
  * bdrv_co_drain_end is called if implemented at the end of the drain.
@@ -363,7 +363,7 @@ struct BlockDriver {
  * requests, or toggle an internal state. After the end of the drain new
  * requests will continue normally.
  */
-void coroutine_fn (*bdrv_co_drain)(BlockDriverState *bs);
+void coroutine_fn (*bdrv_co_drain_begin)(BlockDriverState *bs);
 void coroutine_fn (*bdrv_co_drain_end)(BlockDriverState *bs);
 
 void (*bdrv_add_child)(BlockDriverState *parent, BlockDriverState *child,
diff --git a/block/io.c b/block/io.c
index b0a10ad3ef..65e8094d7c 100644
--- a/block/io.c
+++ b/block/io.c
@@ -162,7 +162,7 @@ static void coroutine_fn bdrv_drain_invoke_entry(void 
*opaque)
 BlockDriverState *bs = data->bs;
 
 if (data->begin) {
-bs->drv->bdrv_co_drain(bs);
+bs->drv->bdrv_co_drain_begin(bs);
 } else {
 bs->drv->bdrv_co_drain_end(bs);
 }
@@ -176,7 +176,7 @@ static void bdrv_drain_invoke(BlockDriverState *bs, bool 
begin)
 {
 BdrvCoDrainData data = { .bs = bs, .done = false, .begin = begin};
 
-if (!bs->drv || (begin && !bs->drv->bdrv_co_drain) ||
+if (!bs->drv || (begin && !bs->drv->bdrv_co_drain_begin) ||
 (!begin && !bs->drv->bdrv_co_drain_end)) {
 return;
 }
diff --git a/block/qed.c b/block/qed.c
index 28e2ec89e8..821dcaa055 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -265,7 +265,7 @@ static bool qed_plug_allocating_write_reqs(BDRVQEDState *s)
 assert(!s->allocating_write_reqs_plugged);
 if (s->allocating_acb != NULL) {
 /* Another allocating write came concurrently.  This cannot happen
- * from bdrv_qed_co_drain, but it can happen when the timer runs.
+ * from bdrv_qed_co_drain_begin, but it can happen when the timer runs.
  */
 qemu_co_mutex_unlock(&s->table_lock);
 return false;
@@ -358,7 +358,7 @@ static void bdrv_qed_attach_aio_context(BlockDriverState 
*bs,
 }
 }
 
-static void coroutine_fn bdrv_qed_co_drain(BlockDriverState *bs)
+static void coroutine_fn bdrv_qed_co_drain_begin(BlockDriverState *bs)
 {
 BDRVQEDState *s = bs->opaque;
 
@@ -1608,7 +1608,7 @@ static BlockDriver bdrv_qed = {
 .bdrv_check   = bdrv_qed_check,
 .bdrv_detach_aio_context  = bdrv_qed_detach_aio_context,
 .bdrv_attach_aio_context  = bdrv_qed_attach_aio_context,
-.bdrv_co_drain= bdrv_qed_co_drain,
+.bdrv_co_drain_begin  = bdrv_qed_co_drain_begin,
 };
 
 static void bdrv_qed_init(void)
-- 
2.11.0

[Qemu-devel] [PATCH v3 3/3] block/throttle.c: add bdrv_co_drain_begin/end callbacks

[Manos Pitsidianakis]

Reviewed-by: Stefan Hajnoczi 
Reviewed-by: Fam Zheng 
Signed-off-by: Manos Pitsidianakis 
---
 block/throttle.c | 18 ++
 1 file changed, 18 insertions(+)

diff --git a/block/throttle.c b/block/throttle.c
index 5bca76300f..833175ac77 100644
--- a/block/throttle.c
+++ b/block/throttle.c
@@ -197,6 +197,21 @@ static bool 
throttle_recurse_is_first_non_filter(BlockDriverState *bs,
 return bdrv_recurse_is_first_non_filter(bs->file->bs, candidate);
 }
 
+static void coroutine_fn throttle_co_drain_begin(BlockDriverState *bs)
+{
+ThrottleGroupMember *tgm = bs->opaque;
+if (atomic_fetch_inc(&tgm->io_limits_disabled) == 0) {
+throttle_group_restart_tgm(tgm);
+}
+}
+
+static void coroutine_fn throttle_co_drain_end(BlockDriverState *bs)
+{
+ThrottleGroupMember *tgm = bs->opaque;
+assert(tgm->io_limits_disabled);
+atomic_dec(&tgm->io_limits_disabled);
+}
+
 static BlockDriver bdrv_throttle = {
 .format_name=   "throttle",
 .protocol_name  =   "throttle",
@@ -226,6 +241,9 @@ static BlockDriver bdrv_throttle = {
 .bdrv_reopen_abort  =   throttle_reopen_abort,
 .bdrv_co_get_block_status   =   bdrv_co_get_block_status_from_file,
 
+.bdrv_co_drain_begin=   throttle_co_drain_begin,
+.bdrv_co_drain_end  =   throttle_co_drain_end,
+
 .is_filter  =   true,
 };
 
-- 
2.11.0

[Qemu-devel] [PATCH v3 0/3] add bdrv_co_drain_begin/end BlockDriver callbacks

[Manos Pitsidianakis]

This patch series renames bdrv_co_drain to bdrv_co_drain_begin and adds a new 
bdrv_co_drain_end callback to match bdrv_drained_begin/end and 
drained_begin/end of BdrvChild. This is needed because the throttle driver 
(block/throttle.c) needs a way to mark the end of the drain in order to toggle 
io_limits_disabled correctly.

Based-on: <20170918202529.28379-1-el13...@mail.ntua.gr>
"block/throttle-groups.c: allocate RestartData on the heap"
Which fixes a coroutine crash in block/throttle-groups.c

v3:
  fixed commit message typo in first patch [Fam]
  rephrased doc comment based on mailing discussion
v2: 
  add doc for callbacks and change order of request polling for completion 
  [Stefan]

Manos Pitsidianakis (3):
  block: add bdrv_co_drain_end callback
  block: rename bdrv_co_drain to bdrv_co_drain_begin
  block/throttle.c: add bdrv_co_drain_begin/end callbacks

 include/block/block_int.h | 13 ++---
 block/io.c| 48 +--
 block/qed.c   |  6 +++---
 block/throttle.c  | 18 ++
 4 files changed, 65 insertions(+), 20 deletions(-)

-- 
2.11.0

[Qemu-devel] [PATCH v3 1/3] block: add bdrv_co_drain_end callback

[Manos Pitsidianakis]

BlockDriverState has a bdrv_co_drain() callback but no equivalent for
the end of the drain. The throttle driver (block/throttle.c) needs a way
to mark the end of the drain in order to toggle io_limits_disabled
correctly, thus bdrv_co_drain_end is needed.

Signed-off-by: Manos Pitsidianakis 
---
 include/block/block_int.h | 11 +--
 block/io.c| 48 +--
 2 files changed, 43 insertions(+), 16 deletions(-)

diff --git a/include/block/block_int.h b/include/block/block_int.h
index ba4c383393..9ebdeb6db0 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -354,10 +354,17 @@ struct BlockDriver {
 int (*bdrv_probe_geometry)(BlockDriverState *bs, HDGeometry *geo);
 
 /**
- * Drain and stop any internal sources of requests in the driver, and
- * remain so until next I/O callback (e.g. bdrv_co_writev) is called.
+ * bdrv_co_drain is called if implemented in the beginning of a
+ * drain operation to drain and stop any internal sources of requests in
+ * the driver.
+ * bdrv_co_drain_end is called if implemented at the end of the drain.
+ *
+ * They should be used by the driver to e.g. manage scheduled I/O
+ * requests, or toggle an internal state. After the end of the drain new
+ * requests will continue normally.
  */
 void coroutine_fn (*bdrv_co_drain)(BlockDriverState *bs);
+void coroutine_fn (*bdrv_co_drain_end)(BlockDriverState *bs);
 
 void (*bdrv_add_child)(BlockDriverState *parent, BlockDriverState *child,
Error **errp);
diff --git a/block/io.c b/block/io.c
index 4378ae4c7d..b0a10ad3ef 100644
--- a/block/io.c
+++ b/block/io.c
@@ -153,6 +153,7 @@ typedef struct {
 Coroutine *co;
 BlockDriverState *bs;
 bool done;
+bool begin;
 } BdrvCoDrainData;
 
 static void coroutine_fn bdrv_drain_invoke_entry(void *opaque)
@@ -160,18 +161,23 @@ static void coroutine_fn bdrv_drain_invoke_entry(void 
*opaque)
 BdrvCoDrainData *data = opaque;
 BlockDriverState *bs = data->bs;
 
-bs->drv->bdrv_co_drain(bs);
+if (data->begin) {
+bs->drv->bdrv_co_drain(bs);
+} else {
+bs->drv->bdrv_co_drain_end(bs);
+}
 
 /* Set data->done before reading bs->wakeup.  */
 atomic_mb_set(&data->done, true);
 bdrv_wakeup(bs);
 }
 
-static void bdrv_drain_invoke(BlockDriverState *bs)
+static void bdrv_drain_invoke(BlockDriverState *bs, bool begin)
 {
-BdrvCoDrainData data = { .bs = bs, .done = false };
+BdrvCoDrainData data = { .bs = bs, .done = false, .begin = begin};
 
-if (!bs->drv || !bs->drv->bdrv_co_drain) {
+if (!bs->drv || (begin && !bs->drv->bdrv_co_drain) ||
+(!begin && !bs->drv->bdrv_co_drain_end)) {
 return;
 }
 
@@ -180,15 +186,16 @@ static void bdrv_drain_invoke(BlockDriverState *bs)
 BDRV_POLL_WHILE(bs, !data.done);
 }
 
-static bool bdrv_drain_recurse(BlockDriverState *bs)
+static bool bdrv_drain_recurse(BlockDriverState *bs, bool begin)
 {
 BdrvChild *child, *tmp;
 bool waited;
 
-waited = BDRV_POLL_WHILE(bs, atomic_read(&bs->in_flight) > 0);
-
 /* Ensure any pending metadata writes are submitted to bs->file.  */
-bdrv_drain_invoke(bs);
+bdrv_drain_invoke(bs, begin);
+
+/* Wait for drained requests to finish */
+waited = BDRV_POLL_WHILE(bs, atomic_read(&bs->in_flight) > 0);
 
 QLIST_FOREACH_SAFE(child, &bs->children, next, tmp) {
 BlockDriverState *bs = child->bs;
@@ -205,7 +212,7 @@ static bool bdrv_drain_recurse(BlockDriverState *bs)
  */
 bdrv_ref(bs);
 }
-waited |= bdrv_drain_recurse(bs);
+waited |= bdrv_drain_recurse(bs, begin);
 if (in_main_loop) {
 bdrv_unref(bs);
 }
@@ -221,12 +228,18 @@ static void bdrv_co_drain_bh_cb(void *opaque)
 BlockDriverState *bs = data->bs;
 
 bdrv_dec_in_flight(bs);
-bdrv_drained_begin(bs);
+if (data->begin) {
+bdrv_drained_begin(bs);
+} else {
+bdrv_drained_end(bs);
+}
+
 data->done = true;
 aio_co_wake(co);
 }
 
-static void coroutine_fn bdrv_co_yield_to_drain(BlockDriverState *bs)
+static void coroutine_fn bdrv_co_yield_to_drain(BlockDriverState *bs,
+bool begin)
 {
 BdrvCoDrainData data;
 
@@ -239,6 +252,7 @@ static void coroutine_fn 
bdrv_co_yield_to_drain(BlockDriverState *bs)
 .co = qemu_coroutine_self(),
 .bs = bs,
 .done = false,
+.begin = begin,
 };
 bdrv_inc_in_flight(bs);
 aio_bh_schedule_oneshot(bdrv_get_aio_context(bs),
@@ -253,7 +267,7 @@ static void coroutine_fn 
bdrv_co_yield_to_drain(BlockDriverState *bs)
 void bdrv_drained_begin(BlockDriverState *bs)
 {
 if (qemu_in_coroutine()) {
-bdrv_co_yield_to_drain(

Re: [Qemu-devel] [PATCH v11 1/6] throttle: factor out duplicate code

[Manos Pitsidianakis]

On Fri, Sep 22, 2017 at 01:31:58PM +0200, Pradeep Jagadeesh wrote:

On 9/18/2017 6:20 PM, Manos Pitsidianakis wrote:

On Thu, Sep 14, 2017 at 06:40:05AM -0400, Pradeep Jagadeesh wrote:

This patch factors out the duplicate throttle code that was still
present in block and fsdev devices.

Signed-off-by: Pradeep Jagadeesh 
Reviewed-by: Alberto Garcia 
Reviewed-by: Greg Kurz 
Reviewed-by: Eric Blake 
---
blockdev.c  | 44 +-
fsdev/qemu-fsdev-throttle.c | 44 ++
include/qemu/throttle-options.h |  3 +++
include/qemu/throttle.h |  4 ++--
include/qemu/typedefs.h |  1 +
util/throttle.c | 52
+
6 files changed, 61 insertions(+), 87 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 56a6b24..9d33c25 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -387,49 +387,7 @@ static void
extract_common_blockdev_options(QemuOpts *opts, int *bdrv_flags,
   }

   if (throttle_cfg) {
-throttle_config_init(throttle_cfg);
-throttle_cfg->buckets[THROTTLE_BPS_TOTAL].avg =
-qemu_opt_get_number(opts, "throttling.bps-total", 0);
-throttle_cfg->buckets[THROTTLE_BPS_READ].avg  =
-qemu_opt_get_number(opts, "throttling.bps-read", 0);
-throttle_cfg->buckets[THROTTLE_BPS_WRITE].avg =
-qemu_opt_get_number(opts, "throttling.bps-write", 0);
-throttle_cfg->buckets[THROTTLE_OPS_TOTAL].avg =
-qemu_opt_get_number(opts, "throttling.iops-total", 0);
-throttle_cfg->buckets[THROTTLE_OPS_READ].avg =
-qemu_opt_get_number(opts, "throttling.iops-read", 0);
-throttle_cfg->buckets[THROTTLE_OPS_WRITE].avg =
-qemu_opt_get_number(opts, "throttling.iops-write", 0);
-
-throttle_cfg->buckets[THROTTLE_BPS_TOTAL].max =
-qemu_opt_get_number(opts, "throttling.bps-total-max", 0);
-throttle_cfg->buckets[THROTTLE_BPS_READ].max  =
-qemu_opt_get_number(opts, "throttling.bps-read-max", 0);
-throttle_cfg->buckets[THROTTLE_BPS_WRITE].max =
-qemu_opt_get_number(opts, "throttling.bps-write-max", 0);
-throttle_cfg->buckets[THROTTLE_OPS_TOTAL].max =
-qemu_opt_get_number(opts, "throttling.iops-total-max", 0);
-throttle_cfg->buckets[THROTTLE_OPS_READ].max =
-qemu_opt_get_number(opts, "throttling.iops-read-max", 0);
-throttle_cfg->buckets[THROTTLE_OPS_WRITE].max =
-qemu_opt_get_number(opts, "throttling.iops-write-max", 0);
-
-throttle_cfg->buckets[THROTTLE_BPS_TOTAL].burst_length =
-qemu_opt_get_number(opts,
"throttling.bps-total-max-length", 1);
-throttle_cfg->buckets[THROTTLE_BPS_READ].burst_length  =
-qemu_opt_get_number(opts,
"throttling.bps-read-max-length", 1);
-throttle_cfg->buckets[THROTTLE_BPS_WRITE].burst_length =
-qemu_opt_get_number(opts,
"throttling.bps-write-max-length", 1);
-throttle_cfg->buckets[THROTTLE_OPS_TOTAL].burst_length =
-qemu_opt_get_number(opts,
"throttling.iops-total-max-length", 1);
-throttle_cfg->buckets[THROTTLE_OPS_READ].burst_length =
-qemu_opt_get_number(opts,
"throttling.iops-read-max-length", 1);
-throttle_cfg->buckets[THROTTLE_OPS_WRITE].burst_length =
-qemu_opt_get_number(opts,
"throttling.iops-write-max-length", 1);
-
-throttle_cfg->op_size =
-qemu_opt_get_number(opts, "throttling.iops-size", 0);
-
+throttle_parse_options(throttle_cfg, opts);
   if (!throttle_is_valid(throttle_cfg, errp)) {
   return;
   }
diff --git a/fsdev/qemu-fsdev-throttle.c b/fsdev/qemu-fsdev-throttle.c
index 49eebb5..0e6fb86 100644
--- a/fsdev/qemu-fsdev-throttle.c
+++ b/fsdev/qemu-fsdev-throttle.c
@@ -16,6 +16,7 @@
#include "qemu/error-report.h"
#include "qemu-fsdev-throttle.h"
#include "qemu/iov.h"
+#include "qemu/throttle-options.h"

static void fsdev_throttle_read_timer_cb(void *opaque)
{
@@ -31,48 +32,7 @@ static void fsdev_throttle_write_timer_cb(void
*opaque)

void fsdev_throttle_parse_opts(QemuOpts *opts, FsThrottle *fst, Error
**errp)
{
-throttle_config_init(&fst->cfg);
-fst->cfg.buckets[THROTTLE_BPS_TOTAL].avg =
-qemu_opt_get_number(opts, "throttling.bps-total", 0);
-fst->cfg.buckets[THROTTLE_BPS_READ].avg  =
-qemu_opt_get_number(opts, "throttling.bps-read", 0);
-fst->cfg.buckets[THROTTLE_BPS_WRITE].avg =
-qemu_opt_get_number(opts, "throttling.bps-write", 0);
-fst->cfg.buckets[THROTTLE_OPS_TOTAL].avg

Re: [Qemu-devel] [PATCH v2 1/3] block: add bdrv_co_drain_end callback

[Manos Pitsidianakis]

On Thu, Sep 21, 2017 at 09:29:43PM +0800, Fam Zheng wrote:

On Thu, 09/21 16:17, Manos Pitsidianakis wrote:

BlockDriverState has a bdrv_do_drain() callback but no equivalent for the end


s/bdrv_do_drain/bdrv_co_drain/


of the drain. The throttle driver (block/throttle.c) needs a way to mark the
end of the drain in order to toggle io_limits_disabled correctly, thus
bdrv_co_drain_end is needed.

Signed-off-by: Manos Pitsidianakis 
---
 include/block/block_int.h |  6 ++
 block/io.c| 48 +--
 2 files changed, 40 insertions(+), 14 deletions(-)

diff --git a/include/block/block_int.h b/include/block/block_int.h
index ba4c383393..21950cfda3 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -356,8 +356,14 @@ struct BlockDriver {
 /**
  * Drain and stop any internal sources of requests in the driver, and
  * remain so until next I/O callback (e.g. bdrv_co_writev) is called.


This line needs update too, maybe:

  /**
   * bdrv_co_drain drains and stops any ... and remain so until
   * bdrv_co_drain_end is called.


+ *
+ * The callbacks are called at the beginning and ending of the drain
+ * respectively. They should be implemented if the driver needs to e.g.


As implied by above change, should we explicitly require "should both be
implemented"? It may not be technically required, but I think is cleaner and
easier to reason about.



It might imply to someone that there's an 
assert(drv->bdrv_co_drain_begin && drv->bdrv_co_drain_end) somewhere 
unless you state they don't have to be implemented at the same time. How 
about we be completely explicit:


 bdrv_co_drain_begin is called if implemented in the beggining of a
 drain operation to drain and stop any internal sources of requests in
 the driver.
 bdrv_co_drain_end is called if implemented at the end of the drain.

 They should be used by the driver to e.g. manage scheduled I/O
 requests, or toggle an internal state. After the end of the drain new
 requests will continue normally.

I hope this is easier for a reader to understand!

+ * manage scheduled I/O requests, or toggle an internal state. 
After an


s/After an/After a/


+ * bdrv_co_drain_end() invocation new requests will continue normally.
  */
 void coroutine_fn (*bdrv_co_drain)(BlockDriverState *bs);
+void coroutine_fn (*bdrv_co_drain_end)(BlockDriverState *bs);

 void (*bdrv_add_child)(BlockDriverState *parent, BlockDriverState *child,
Error **errp);
diff --git a/block/io.c b/block/io.c
index 4378ae4c7d..b0a10ad3ef 100644
--- a/block/io.c
+++ b/block/io.c
@@ -153,6 +153,7 @@ typedef struct {
 Coroutine *co;
 BlockDriverState *bs;
 bool done;
+bool begin;
 } BdrvCoDrainData;

 static void coroutine_fn bdrv_drain_invoke_entry(void *opaque)
@@ -160,18 +161,23 @@ static void coroutine_fn bdrv_drain_invoke_entry(void 
*opaque)
 BdrvCoDrainData *data = opaque;
 BlockDriverState *bs = data->bs;

-bs->drv->bdrv_co_drain(bs);
+if (data->begin) {
+bs->drv->bdrv_co_drain(bs);
+} else {
+bs->drv->bdrv_co_drain_end(bs);
+}

 /* Set data->done before reading bs->wakeup.  */
 atomic_mb_set(&data->done, true);
 bdrv_wakeup(bs);
 }

-static void bdrv_drain_invoke(BlockDriverState *bs)
+static void bdrv_drain_invoke(BlockDriverState *bs, bool begin)
 {
-BdrvCoDrainData data = { .bs = bs, .done = false };
+BdrvCoDrainData data = { .bs = bs, .done = false, .begin = begin};

-if (!bs->drv || !bs->drv->bdrv_co_drain) {
+if (!bs->drv || (begin && !bs->drv->bdrv_co_drain) ||
+(!begin && !bs->drv->bdrv_co_drain_end)) {
 return;
 }

@@ -180,15 +186,16 @@ static void bdrv_drain_invoke(BlockDriverState *bs)
 BDRV_POLL_WHILE(bs, !data.done);
 }

-static bool bdrv_drain_recurse(BlockDriverState *bs)
+static bool bdrv_drain_recurse(BlockDriverState *bs, bool begin)
 {
 BdrvChild *child, *tmp;
 bool waited;

-waited = BDRV_POLL_WHILE(bs, atomic_read(&bs->in_flight) > 0);
-
 /* Ensure any pending metadata writes are submitted to bs->file.  */
-bdrv_drain_invoke(bs);
+bdrv_drain_invoke(bs, begin);
+
+/* Wait for drained requests to finish */
+waited = BDRV_POLL_WHILE(bs, atomic_read(&bs->in_flight) > 0);

 QLIST_FOREACH_SAFE(child, &bs->children, next, tmp) {
 BlockDriverState *bs = child->bs;
@@ -205,7 +212,7 @@ static bool bdrv_drain_recurse(BlockDriverState *bs)
  */
 bdrv_ref(bs);
 }
-waited |= bdrv_drain_recurse(bs);
+waited |= bdrv_drain_recurse(bs, begin);
 if (in_main_loop) {
 bdrv_unref(bs);
 }
@@ -221,12 +228,18 @@ static void bdrv_co_drain_bh_cb(void *opaque)
 Bl

Re: [Qemu-devel] [PATCH v2 0/3] add bdrv_co_drain_begin/end BlockDriver callbacks

[Manos Pitsidianakis]

On Thu, Sep 21, 2017 at 09:35:35PM +0800, Fam Zheng wrote:

On Thu, 09/21 16:17, Manos Pitsidianakis wrote:

This patch series renames bdrv_co_drain to bdrv_co_drain_begin and adds a new
bdrv_co_drain_end callback to match bdrv_drained_begin/end and
drained_begin/end of BdrvChild. This is needed because the throttle driver
(block/throttle.c) needs a way to mark the end of the drain in order to toggle
io_limits_disabled correctly.


Is this a bug fix? I.e. do we need to Cc qemu-stable@?

Fam


No that's not needed, throttle is not in 2.10. The bug in this case is 
that the drain would wait for the throttled requests to be completed as 
they were scheduled by the I/O throttling instead of restarting the 
queue and scheduling them right away.


signature.asc
Description: PGP signature

[Qemu-devel] [PATCH v2 3/3] block/throttle.c: add bdrv_co_drain_begin/end callbacks

[Manos Pitsidianakis]

Reviewed-by: Stefan Hajnoczi 
Signed-off-by: Manos Pitsidianakis 
---
 block/throttle.c | 18 ++
 1 file changed, 18 insertions(+)

diff --git a/block/throttle.c b/block/throttle.c
index 5bca76300f..833175ac77 100644
--- a/block/throttle.c
+++ b/block/throttle.c
@@ -197,6 +197,21 @@ static bool 
throttle_recurse_is_first_non_filter(BlockDriverState *bs,
 return bdrv_recurse_is_first_non_filter(bs->file->bs, candidate);
 }
 
+static void coroutine_fn throttle_co_drain_begin(BlockDriverState *bs)
+{
+ThrottleGroupMember *tgm = bs->opaque;
+if (atomic_fetch_inc(&tgm->io_limits_disabled) == 0) {
+throttle_group_restart_tgm(tgm);
+}
+}
+
+static void coroutine_fn throttle_co_drain_end(BlockDriverState *bs)
+{
+ThrottleGroupMember *tgm = bs->opaque;
+assert(tgm->io_limits_disabled);
+atomic_dec(&tgm->io_limits_disabled);
+}
+
 static BlockDriver bdrv_throttle = {
 .format_name=   "throttle",
 .protocol_name  =   "throttle",
@@ -226,6 +241,9 @@ static BlockDriver bdrv_throttle = {
 .bdrv_reopen_abort  =   throttle_reopen_abort,
 .bdrv_co_get_block_status   =   bdrv_co_get_block_status_from_file,
 
+.bdrv_co_drain_begin=   throttle_co_drain_begin,
+.bdrv_co_drain_end  =   throttle_co_drain_end,
+
 .is_filter  =   true,
 };
 
-- 
2.11.0

[Qemu-devel] [PATCH v2 0/3] add bdrv_co_drain_begin/end BlockDriver callbacks

[Manos Pitsidianakis]

This patch series renames bdrv_co_drain to bdrv_co_drain_begin and adds a new 
bdrv_co_drain_end callback to match bdrv_drained_begin/end and 
drained_begin/end of BdrvChild. This is needed because the throttle driver 
(block/throttle.c) needs a way to mark the end of the drain in order to toggle 
io_limits_disabled correctly.

Based-on: <20170918202529.28379-1-el13...@mail.ntua.gr>
"block/throttle-groups.c: allocate RestartData on the heap"
Which fixes a coroutine crash in block/throttle-groups.c

v2: 
  add doc for callbacks and change order of request polling for completion 
  [Stefan]

Manos Pitsidianakis (3):
  block: add bdrv_co_drain_end callback
  block: rename bdrv_co_drain to bdrv_co_drain_begin
  block/throttle.c: add bdrv_co_drain_begin/end callbacks

 include/block/block_int.h |  8 +++-
 block/io.c| 48 +--
 block/qed.c   |  6 +++---
 block/throttle.c  | 18 ++
 4 files changed, 62 insertions(+), 18 deletions(-)

-- 
2.11.0

[Qemu-devel] [PATCH v2 1/3] block: add bdrv_co_drain_end callback

[Manos Pitsidianakis]

BlockDriverState has a bdrv_do_drain() callback but no equivalent for the end
of the drain. The throttle driver (block/throttle.c) needs a way to mark the
end of the drain in order to toggle io_limits_disabled correctly, thus
bdrv_co_drain_end is needed.

Signed-off-by: Manos Pitsidianakis 
---
 include/block/block_int.h |  6 ++
 block/io.c| 48 +--
 2 files changed, 40 insertions(+), 14 deletions(-)

diff --git a/include/block/block_int.h b/include/block/block_int.h
index ba4c383393..21950cfda3 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -356,8 +356,14 @@ struct BlockDriver {
 /**
  * Drain and stop any internal sources of requests in the driver, and
  * remain so until next I/O callback (e.g. bdrv_co_writev) is called.
+ *
+ * The callbacks are called at the beginning and ending of the drain
+ * respectively. They should be implemented if the driver needs to e.g.
+ * manage scheduled I/O requests, or toggle an internal state. After an
+ * bdrv_co_drain_end() invocation new requests will continue normally.
  */
 void coroutine_fn (*bdrv_co_drain)(BlockDriverState *bs);
+void coroutine_fn (*bdrv_co_drain_end)(BlockDriverState *bs);
 
 void (*bdrv_add_child)(BlockDriverState *parent, BlockDriverState *child,
Error **errp);
diff --git a/block/io.c b/block/io.c
index 4378ae4c7d..b0a10ad3ef 100644
--- a/block/io.c
+++ b/block/io.c
@@ -153,6 +153,7 @@ typedef struct {
 Coroutine *co;
 BlockDriverState *bs;
 bool done;
+bool begin;
 } BdrvCoDrainData;
 
 static void coroutine_fn bdrv_drain_invoke_entry(void *opaque)
@@ -160,18 +161,23 @@ static void coroutine_fn bdrv_drain_invoke_entry(void 
*opaque)
 BdrvCoDrainData *data = opaque;
 BlockDriverState *bs = data->bs;
 
-bs->drv->bdrv_co_drain(bs);
+if (data->begin) {
+bs->drv->bdrv_co_drain(bs);
+} else {
+bs->drv->bdrv_co_drain_end(bs);
+}
 
 /* Set data->done before reading bs->wakeup.  */
 atomic_mb_set(&data->done, true);
 bdrv_wakeup(bs);
 }
 
-static void bdrv_drain_invoke(BlockDriverState *bs)
+static void bdrv_drain_invoke(BlockDriverState *bs, bool begin)
 {
-BdrvCoDrainData data = { .bs = bs, .done = false };
+BdrvCoDrainData data = { .bs = bs, .done = false, .begin = begin};
 
-if (!bs->drv || !bs->drv->bdrv_co_drain) {
+if (!bs->drv || (begin && !bs->drv->bdrv_co_drain) ||
+(!begin && !bs->drv->bdrv_co_drain_end)) {
 return;
 }
 
@@ -180,15 +186,16 @@ static void bdrv_drain_invoke(BlockDriverState *bs)
 BDRV_POLL_WHILE(bs, !data.done);
 }
 
-static bool bdrv_drain_recurse(BlockDriverState *bs)
+static bool bdrv_drain_recurse(BlockDriverState *bs, bool begin)
 {
 BdrvChild *child, *tmp;
 bool waited;
 
-waited = BDRV_POLL_WHILE(bs, atomic_read(&bs->in_flight) > 0);
-
 /* Ensure any pending metadata writes are submitted to bs->file.  */
-bdrv_drain_invoke(bs);
+bdrv_drain_invoke(bs, begin);
+
+/* Wait for drained requests to finish */
+waited = BDRV_POLL_WHILE(bs, atomic_read(&bs->in_flight) > 0);
 
 QLIST_FOREACH_SAFE(child, &bs->children, next, tmp) {
 BlockDriverState *bs = child->bs;
@@ -205,7 +212,7 @@ static bool bdrv_drain_recurse(BlockDriverState *bs)
  */
 bdrv_ref(bs);
 }
-waited |= bdrv_drain_recurse(bs);
+waited |= bdrv_drain_recurse(bs, begin);
 if (in_main_loop) {
 bdrv_unref(bs);
 }
@@ -221,12 +228,18 @@ static void bdrv_co_drain_bh_cb(void *opaque)
 BlockDriverState *bs = data->bs;
 
 bdrv_dec_in_flight(bs);
-bdrv_drained_begin(bs);
+if (data->begin) {
+bdrv_drained_begin(bs);
+} else {
+bdrv_drained_end(bs);
+}
+
 data->done = true;
 aio_co_wake(co);
 }
 
-static void coroutine_fn bdrv_co_yield_to_drain(BlockDriverState *bs)
+static void coroutine_fn bdrv_co_yield_to_drain(BlockDriverState *bs,
+bool begin)
 {
 BdrvCoDrainData data;
 
@@ -239,6 +252,7 @@ static void coroutine_fn 
bdrv_co_yield_to_drain(BlockDriverState *bs)
 .co = qemu_coroutine_self(),
 .bs = bs,
 .done = false,
+.begin = begin,
 };
 bdrv_inc_in_flight(bs);
 aio_bh_schedule_oneshot(bdrv_get_aio_context(bs),
@@ -253,7 +267,7 @@ static void coroutine_fn 
bdrv_co_yield_to_drain(BlockDriverState *bs)
 void bdrv_drained_begin(BlockDriverState *bs)
 {
 if (qemu_in_coroutine()) {
-bdrv_co_yield_to_drain(bs);
+bdrv_co_yield_to_drain(bs, true);
 return;
 }
 
@@ -262,17 +276,22 @@ void bdrv_drained_begin(BlockDriverState *bs)
 bdrv_parent_drained_begin(bs);
 }
 
-

[Qemu-devel] [PATCH v2 2/3] block: rename bdrv_co_drain to bdrv_co_drain_begin

[Manos Pitsidianakis]

Reviewed-by: Stefan Hajnoczi 
Signed-off-by: Manos Pitsidianakis 
---
 include/block/block_int.h | 2 +-
 block/io.c| 4 ++--
 block/qed.c   | 6 +++---
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/include/block/block_int.h b/include/block/block_int.h
index 21950cfda3..205d088271 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -362,7 +362,7 @@ struct BlockDriver {
  * manage scheduled I/O requests, or toggle an internal state. After an
  * bdrv_co_drain_end() invocation new requests will continue normally.
  */
-void coroutine_fn (*bdrv_co_drain)(BlockDriverState *bs);
+void coroutine_fn (*bdrv_co_drain_begin)(BlockDriverState *bs);
 void coroutine_fn (*bdrv_co_drain_end)(BlockDriverState *bs);
 
 void (*bdrv_add_child)(BlockDriverState *parent, BlockDriverState *child,
diff --git a/block/io.c b/block/io.c
index b0a10ad3ef..65e8094d7c 100644
--- a/block/io.c
+++ b/block/io.c
@@ -162,7 +162,7 @@ static void coroutine_fn bdrv_drain_invoke_entry(void 
*opaque)
 BlockDriverState *bs = data->bs;
 
 if (data->begin) {
-bs->drv->bdrv_co_drain(bs);
+bs->drv->bdrv_co_drain_begin(bs);
 } else {
 bs->drv->bdrv_co_drain_end(bs);
 }
@@ -176,7 +176,7 @@ static void bdrv_drain_invoke(BlockDriverState *bs, bool 
begin)
 {
 BdrvCoDrainData data = { .bs = bs, .done = false, .begin = begin};
 
-if (!bs->drv || (begin && !bs->drv->bdrv_co_drain) ||
+if (!bs->drv || (begin && !bs->drv->bdrv_co_drain_begin) ||
 (!begin && !bs->drv->bdrv_co_drain_end)) {
 return;
 }
diff --git a/block/qed.c b/block/qed.c
index 28e2ec89e8..821dcaa055 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -265,7 +265,7 @@ static bool qed_plug_allocating_write_reqs(BDRVQEDState *s)
 assert(!s->allocating_write_reqs_plugged);
 if (s->allocating_acb != NULL) {
 /* Another allocating write came concurrently.  This cannot happen
- * from bdrv_qed_co_drain, but it can happen when the timer runs.
+ * from bdrv_qed_co_drain_begin, but it can happen when the timer runs.
  */
 qemu_co_mutex_unlock(&s->table_lock);
 return false;
@@ -358,7 +358,7 @@ static void bdrv_qed_attach_aio_context(BlockDriverState 
*bs,
 }
 }
 
-static void coroutine_fn bdrv_qed_co_drain(BlockDriverState *bs)
+static void coroutine_fn bdrv_qed_co_drain_begin(BlockDriverState *bs)
 {
 BDRVQEDState *s = bs->opaque;
 
@@ -1608,7 +1608,7 @@ static BlockDriver bdrv_qed = {
 .bdrv_check   = bdrv_qed_check,
 .bdrv_detach_aio_context  = bdrv_qed_detach_aio_context,
 .bdrv_attach_aio_context  = bdrv_qed_attach_aio_context,
-.bdrv_co_drain= bdrv_qed_co_drain,
+.bdrv_co_drain_begin  = bdrv_qed_co_drain_begin,
 };
 
 static void bdrv_qed_init(void)
-- 
2.11.0