Re: [PATCH 3/5] tools/vhost-user-i2c: Add backend driver

[Viresh Kumar]

On 25-03-21, 17:16, Arnd Bergmann wrote:
> On Wed, Mar 24, 2021 at 8:33 AM Viresh Kumar  wrote:
> > +static void vi2c_handle_ctrl(VuDev *dev, int qidx)
> > +{
> > +VuVirtq *vq = vu_get_queue(dev, qidx);
> > +struct i2c_msg msg;
> > +struct virtio_i2c_out_hdr *out_hdr;
> > +struct virtio_i2c_in_hdr *in_hdr;
> > +bool fail_next = false;
> > +size_t len, in_hdr_len;
> > +
> > +for (;;) {
> > +VuVirtqElement *elem;
> > +
> > +elem = vu_queue_pop(dev, vq, sizeof(VuVirtqElement));
> > +if (!elem) {
> > +break;
> > +}
> > +
> > +g_debug("%s: got queue (in %d, out %d)", __func__, elem->in_num,
> > +elem->out_num);
> > +
> > +/* Validate size of out header */
> > +if (elem->out_sg[0].iov_len != sizeof(*out_hdr)) {
> > +g_warning("%s: Invalid out hdr %zu : %zu\n", __func__,
> > +  elem->out_sg[0].iov_len, sizeof(*out_hdr));
> > +continue;
> > +}
> > +
> > +out_hdr = elem->out_sg[0].iov_base;
> > +
> > +/* Bit 0 is reserved in virtio spec */
> > +msg.addr = out_hdr->addr >> 1;
> > +
> > +/* Read Operation */
> > +if (elem->out_num == 1 && elem->in_num == 2) {
> > +len = elem->in_sg[0].iov_len;
> > +if (!len) {
> > +g_warning("%s: Read buffer length can't be zero\n", 
> > __func__);
> > +continue;
> > +}
> 
> 
> It looks like you are not handling endianness conversion here. As far as I
> can tell, the protocol requires little-endian data, but the code might
> run on a big-endian CPU.

I hope this is all we are required to do here, right ?

@@ -442,7 +421,7 @@ static void vi2c_handle_ctrl(VuDev *dev, int qidx)
 out_hdr = elem->out_sg[0].iov_base;
 
 /* Bit 0 is reserved in virtio spec */
-msg.addr = out_hdr->addr >> 1;
+msg.addr = le16toh(out_hdr->addr) >> 1;
 
 /* Read Operation */
 if (elem->out_num == 1 && elem->in_num == 2) {
@@ -489,7 +468,7 @@ static void vi2c_handle_ctrl(VuDev *dev, int qidx)
 in_hdr->status = fail_next ? VIRTIO_I2C_MSG_ERR : vi2c_xfer(dev, &msg);
 if (in_hdr->status == VIRTIO_I2C_MSG_ERR) {
 /* We need to fail remaining transfers as well */
-fail_next = out_hdr->flags & VIRTIO_I2C_FLAGS_FAIL_NEXT;
+fail_next = le32toh(out_hdr->flags) & VIRTIO_I2C_FLAGS_FAIL_NEXT;
 }
 
These are the only fields we are passing apart from buf, which goes
directly to the client device.

-- 
viresh

Re: [PATCH 0/6] Add debug interface to kick/call on purpose

[Jason Wang]

在 2021/3/26 下午1:44, Dongli Zhang 写道:

The virtio device/driver (e.g., vhost-scsi or vhost-net) may hang due to
the loss of doorbell kick, e.g.,

https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg01711.html

... or due to the loss of IRQ, e.g., as fixed by linux kernel commit
fe200ae48ef5 ("genirq: Mark polled irqs and defer the real handler").

This patch introduces a new debug interface 'DeviceEvent' to DeviceClass
to help narrow down if the issue is due to loss of irq/kick. So far the new
interface handles only two events: 'call' and 'kick'. Any device (e.g.,
virtio/vhost or VFIO) may implement the interface (e.g., via eventfd, MSI-X
or legacy IRQ).

The 'call' is to inject irq on purpose by admin for a specific device (e.g.,
vhost-scsi) from QEMU/host to VM, while the 'kick' is to kick the doorbell
on purpose by admin at QEMU/host side for a specific device.


This device can be used as a workaround if call/kick is lost due to
virtualization software (e.g., kernel or QEMU) issue.

We may also implement the interface for VFIO PCI, e.g., to write to
VFIOPCIDevice->msi_vectors[i].interrupt will be able to inject IRQ to VM
on purpose. This is considered future work once the virtio part is done.


Below is from live crash analysis. Initially, the queue=2 has count=15 for
'kick' eventfd_ctx. Suppose there is data in vring avail while there is no
used available. We suspect this is because vhost-scsi was not notified by
VM. In order to narrow down and analyze the issue, we use live crash to
dump the current counter of eventfd for queue=2.

crash> eventfd_ctx 8f67f6bbe700
struct eventfd_ctx {
   kref = {
 refcount = {
   refs = {
 counter = 4
   }
 }
   },
   wqh = {
 lock = {
   {
 rlock = {
   raw_lock = {
 val = {
   counter = 0
 }
   }
 }
   }
 },
 head = {
   next = 0x8f841dc08e18,
   prev = 0x8f841dc08e18
 }
   },
   count = 15, ---> eventfd is 15 !!!
   flags = 526336
}

Now we kick the doorbell for vhost-scsi queue=2 on purpose for diagnostic
with this interface.

{ "execute": "x-debug-device-event",
   "arguments": { "dev": "/machine/peripheral/vscsi0",
  "event": "kick", "queue": 2 } }

The counter is increased to 16. Suppose the hang issue is resolved, it
indicates something bad is in software that the 'kick' is lost.



What do you mean by "software" here? And it looks to me you're testing 
whether event_notifier_set() is called by virtio_queue_notify() here. If 
so, I'm not sure how much value could we gain from a dedicated debug 
interface like this consider there're a lot of exisinting general 
purpose debugging method like tracing or gdb. I'd say the path from 
virtio_queue_notify() to event_notifier_set() is only a very small 
fraction of the process of virtqueue kick which is unlikey to be buggy. 
Consider usually the ioeventfd will be offloaded to KVM, it's more a 
chance that something is wrong in setuping ioeventfd instead of here. 
Irq is even more complicated.


I think we could not gain much for introducing an dedicated mechanism 
for such a corner case.


Thanks




crash> eventfd_ctx 8f67f6bbe700
struct eventfd_ctx {
   kref = {
 refcount = {
   refs = {
 counter = 4
   }
 }
   },
   wqh = {
 lock = {
   {
 rlock = {
   raw_lock = {
 val = {
   counter = 0
 }
   }
 }
   }
 },
 head = {
   next = 0x8f841dc08e18,
   prev = 0x8f841dc08e18
 }
   },
   count = 16, ---> eventfd incremented to 16 !!!
   flags = 526336
}


Original RFC link:

https://lists.nongnu.org/archive/html/qemu-devel/2021-01/msg03441.html

Changed since RFC:
   - add support for more virtio/vhost pci devices
   - add log (toggled by DEBUG_VIRTIO_EVENT) to virtio.c to say that this
 mischeivous command had been used
   - fix grammer error (s/lost/loss/)
   - change version to 6.1
   - fix incorrect example in qapi/qdev.json
   - manage event types with enum/array, instead of hard coding


Dongli Zhang (6):
qdev: introduce qapi/hmp command for kick/call event
virtio: introduce helper function for kick/call device event
virtio-blk-pci: implement device event interface for kick/call
virtio-scsi-pci: implement device event interface for kick/call
vhost-scsi-pci: implement device event interface for kick/call
virtio-net-pci: implement device event interface for kick/call

  hmp-commands.hx | 14 
  hw/block/virtio-blk.c   |  9 +
  hw/net/virtio-net.c |  9 +
  hw/scsi/vhost-scsi.c|  6 
  hw/scsi/virtio-scsi.c   |  9 +
  hw/virtio/vhost-scsi-pci.c  | 10 ++
  hw/virtio/virtio-blk-pci.c  | 10 ++
  hw/virtio/virtio-net-pci.c  | 10 ++
  hw/virtio/virtio-scsi-pci.c | 10 ++
  hw/virtio/virtio.c

Re: [PATCH 7/7] block/nbd: stop manipulating in_flight counter

[Roman Kagan]

On Tue, Mar 16, 2021 at 09:37:13PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> 16.03.2021 19:08, Roman Kagan wrote:
> > On Mon, Mar 15, 2021 at 11:15:44PM +0300, Vladimir Sementsov-Ogievskiy 
> > wrote:
> > > 15.03.2021 09:06, Roman Kagan wrote:
> > > > As the reconnect logic no longer interferes with drained sections, it
> > > > appears unnecessary to explicitly manipulate the in_flight counter.
> > > > 
> > > > Fixes: 5ad81b4946 ("nbd: Restrict connection_co reentrance")
> > > 
> > > And here you actually allow qemu_aio_coroutine_enter() call in
> > > nbd_client_attach_aio_context_bh() to enter connection_co in any yield
> > > point which is possible during drained section. The analysis should be
> > > done to be sure that all these yield points are safe for reentering by
> > > external qemu_aio_coroutine_enter(). (By external I mean not by the
> > > actual enter() we are waiting for at the yield() point. For example
> > > qemu_channel_yield() supports reentering.. And therefore (as I
> > > understand after fast looking through) nbd_read() should support
> > > reentering too..
> > 
> > I'll do a more thorough analysis of how safe it is.
> > 
> > FWIW this hasn't triggered any test failures yet, but that assert in
> > patch 3 didn't ever go off either so I'm not sure I can trust the tests
> > on this.
> > 
> 
> Hmm. First, we should consider qemu_coroutine_yield() in
> nbd_co_establish_connection().
> 
> Most of nbd_co_establish_connection_cancel() purpose is to avoid
> reentering this yield()..

Unless I'm overlooking something, nbd_co_establish_connection() is fine
with spurious entering at this yield point.  What does look problematic,
though, is your next point:

> And I don't know, how to make it safely reenterable: keep in mind bh
> that may be already scheduled by connect_thread_func(). And if bh is
> already scheduled, can we cancel it? I'm not sure.
> 
> We have qemu_bh_delete(). But is it possible, that BH is near to be
> executed and already cannot be removed by qemu_bh_delete()? I don't
> know.
> 
> And if we can't safely drop the bh at any moment, we should wait in
> nbd_client_detach_aio_context until the scheduled bh enters the
> connection_co.. Or something like this

So I think it's not the reentry at this yield point per se which is
problematic, it's that that bh may have been scheduled before the
aio_context switch so once it runs it would wake up connection_co on the
old aio_context.  I think it may be possible to address by adding a
check into connect_bh().

Thanks,
Roman.

[Bug 1915063] Re: Windows 10 wil not install using qemu-system-x86_64

[Christian Ehrhardt ]

Thanks David,
I have no threadripper around atm, I think I can next week get hands on an EPYC 
Rome, but that isn't 100% the same.

But gladly you tried this on the latest qemu 5.2 and since it is failing there 
as well it might be worth to also report it upstream. That is a great community 
which might have ran things on a threadripper already and be able to point us 
to a qemu/kernel fix - or at least an existing discussions abut it.
For now I'm adding a qemu task here which will mirror this case to the mailing 
list.

** Also affects: qemu
   Importance: Undecided
   Status: New

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1915063

Title:
  Windows 10 wil not install using qemu-system-x86_64

Status in QEMU:
  New
Status in qemu package in Ubuntu:
  Confirmed

Bug description:
  Steps to reproduce
  install virt-manager and ovmf if nopt already there
  copy windows and virtio iso files to /var/lib/libvirt/images

  Use virt-manager from local machine to create your VMs with the disk, CPUs 
and memory required
  Select customize configuration then select OVMF(UEFI) instead of seabios
  set first CDROM to the windows installation iso (enable in boot options)
  add a second CDROM and load with the virtio iso
change spice display to VNC

Always get a security error from windows and it fails to launch the 
installer (works on RHEL and Fedora)
  I tried updating the qemu version from Focals 4.2 to Groovy 5.0 which was of 
no help
  --- 
  ProblemType: Bug
  ApportVersion: 2.20.11-0ubuntu27.14
  Architecture: amd64
  CasperMD5CheckResult: skip
  CurrentDesktop: ubuntu:GNOME
  DistributionChannelDescriptor:
   # This is the distribution channel descriptor for the OEM CDs
   # For more information see 
http://wiki.ubuntu.com/DistributionChannelDescriptor
   
canonical-oem-sutton-focal-amd64-20201030-422+pc-sutton-bachman-focal-amd64+X00
  DistroRelease: Ubuntu 20.04
  InstallationDate: Installed on 2021-01-20 (19 days ago)
  InstallationMedia: Ubuntu 20.04 "Focal" - Build amd64 LIVE Binary 
20201030-14:39
  MachineType: LENOVO 30E102Z
  NonfreeKernelModules: nvidia_modeset nvidia
  Package: linux (not installed)
  ProcEnviron:
   TERM=xterm-256color
   PATH=(custom, no user)
   XDG_RUNTIME_DIR=
   LANG=en_US.UTF-8
   SHELL=/bin/bash
  ProcFB: 0 EFI VGA
  ProcKernelCmdLine: BOOT_IMAGE=/boot/vmlinuz-5.6.0-1042-oem 
root=UUID=389cd165-fc52-4814-b837-a1090b9c2387 ro locale=en_US quiet splash 
vt.handoff=7
  ProcVersionSignature: Ubuntu 5.6.0-1042.46-oem 5.6.19
  RelatedPackageVersions:
   linux-restricted-modules-5.6.0-1042-oem N/A
   linux-backports-modules-5.6.0-1042-oem  N/A
   linux-firmware  1.187.8
  RfKill:
   
  Tags:  focal
  Uname: Linux 5.6.0-1042-oem x86_64
  UpgradeStatus: No upgrade log present (probably fresh install)
  UserGroups: adm cdrom dip docker kvm libvirt lpadmin plugdev sambashare sudo
  _MarkForUpload: True
  dmi.bios.date: 07/29/2020
  dmi.bios.vendor: LENOVO
  dmi.bios.version: S07KT08A
  dmi.board.name: 1046
  dmi.board.vendor: LENOVO
  dmi.board.version: Not Defined
  dmi.chassis.type: 3
  dmi.chassis.vendor: LENOVO
  dmi.chassis.version: None
  dmi.modalias: 
dmi:bvnLENOVO:bvrS07KT08A:bd07/29/2020:svnLENOVO:pn30E102Z:pvrThinkStationP620:rvnLENOVO:rn1046:rvrNotDefined:cvnLENOVO:ct3:cvrNone:
  dmi.product.family: INVALID
  dmi.product.name: 30E102Z
  dmi.product.sku: LENOVO_MT_30E1_BU_Think_FM_ThinkStation P620
  dmi.product.version: ThinkStation P620
  dmi.sys.vendor: LENOVO

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1915063/+subscriptions

[PATCH v4 0/4] yank: Add chardev tests and fixes

[Lukas Straub]

Hello Everyone,
These patches increase test coverage for yank, add tests and fix bugs and
crashes in yank in combination with chardev-change.
Please Review.

Regards,
Lukas Straub

Changes:
-v4:
 -test: fix CharChangeTestConfig structs on stack going out of scope
 -test: move after bugfixes

-v3:
 -Base on 
  ([PATCH 0/2] yank: Always link full yank code)
 -Drop patch 1 (tests: Use the normal yank code instead of stubs in relevant 
tests)

-v2:
 -test: add license
 -test: factorize testcases to a single function
 -test: test chardev_change with initialization of new chardev failing
 -fix chardev_change with initialization of new chardev failing
 -add reviewed-by and tested-by tags

Based-on: 
([PATCH 0/2] yank: Always link full yank code)

Lukas Straub (4):
  chardev/char.c: Move object_property_try_add_child out of chardev_new
  chardev/char.c: Always pass id to chardev_new
  chardev: Fix yank with the chardev-change case
  tests: Add tests for yank with the chardev-change case

 MAINTAINERS|   1 +
 chardev/char-socket.c  |  20 -
 chardev/char.c |  77 ++--
 include/chardev/char.h |   3 +
 tests/unit/meson.build |   3 +-
 tests/unit/test-yank.c | 199 +
 6 files changed, 274 insertions(+), 29 deletions(-)
 create mode 100644 tests/unit/test-yank.c

--
2.30.2


pgpZmV8Dice3V.pgp
Description: OpenPGP digital signature

[PATCH v4 1/4] chardev/char.c: Move object_property_try_add_child out of chardev_new

[Lukas Straub]

Move object_property_try_add_child out of chardev_new into it's
callers. This is a preparation for the next patches to fix yank
with the chardev-change case.

Signed-off-by: Lukas Straub 
Reviewed-by: Marc-André Lureau 
Tested-by: Li Zhang 
---
 chardev/char.c | 42 --
 1 file changed, 24 insertions(+), 18 deletions(-)

diff --git a/chardev/char.c b/chardev/char.c
index 140d6d9d36..48f321b3e1 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -975,7 +975,9 @@ static Chardev *chardev_new(const char *id, const char 
*typename,

 qemu_char_open(chr, backend, &be_opened, &local_err);
 if (local_err) {
-goto end;
+error_propagate(errp, local_err);
+object_unref(obj);
+return NULL;
 }

 if (!chr->filename) {
@@ -985,22 +987,6 @@ static Chardev *chardev_new(const char *id, const char 
*typename,
 qemu_chr_be_event(chr, CHR_EVENT_OPENED);
 }

-if (id) {
-object_property_try_add_child(get_chardevs_root(), id, obj,
-  &local_err);
-if (local_err) {
-goto end;
-}
-object_unref(obj);
-}
-
-end:
-if (local_err) {
-error_propagate(errp, local_err);
-object_unref(obj);
-return NULL;
-}
-
 return chr;
 }

@@ -1009,6 +995,7 @@ Chardev *qemu_chardev_new(const char *id, const char 
*typename,
   GMainContext *gcontext,
   Error **errp)
 {
+Chardev *chr;
 g_autofree char *genid = NULL;

 if (!id) {
@@ -1016,7 +1003,19 @@ Chardev *qemu_chardev_new(const char *id, const char 
*typename,
 id = genid;
 }

-return chardev_new(id, typename, backend, gcontext, errp);
+chr = chardev_new(id, typename, backend, gcontext, errp);
+if (!chr) {
+return NULL;
+}
+
+if (!object_property_try_add_child(get_chardevs_root(), id, OBJECT(chr),
+   errp)) {
+object_unref(OBJECT(chr));
+return NULL;
+}
+object_unref(OBJECT(chr));
+
+return chr;
 }

 ChardevReturn *qmp_chardev_add(const char *id, ChardevBackend *backend,
@@ -1037,6 +1036,13 @@ ChardevReturn *qmp_chardev_add(const char *id, 
ChardevBackend *backend,
 return NULL;
 }

+if (!object_property_try_add_child(get_chardevs_root(), id, OBJECT(chr),
+   errp)) {
+object_unref(OBJECT(chr));
+return NULL;
+}
+object_unref(OBJECT(chr));
+
 ret = g_new0(ChardevReturn, 1);
 if (CHARDEV_IS_PTY(chr)) {
 ret->pty = g_strdup(chr->filename + 4);
--
2.30.2



pgpbMC1O4rNpz.pgp
Description: OpenPGP digital signature

[PATCH v4 2/4] chardev/char.c: Always pass id to chardev_new

[Lukas Straub]

Always pass the id to chardev_new, since it is needed to register
the yank instance for the chardev. Also, after checking that
nothing calls chardev_new with id=NULL, assert() that id!=NULL.

This fixes a crash when using chardev-change to change a chardev
to chardev-socket, which attempts to register a yank instance.
This in turn tries to dereference the NULL-pointer.

Signed-off-by: Lukas Straub 
Reviewed-by: Marc-André Lureau 
Tested-by: Li Zhang 
---
 chardev/char.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/chardev/char.c b/chardev/char.c
index 48f321b3e1..75993f903f 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -967,6 +967,7 @@ static Chardev *chardev_new(const char *id, const char 
*typename,
 bool be_opened = true;

 assert(g_str_has_prefix(typename, "chardev-"));
+assert(id);

 obj = object_new(typename);
 chr = CHARDEV(obj);
@@ -1095,12 +1096,11 @@ ChardevReturn *qmp_chardev_change(const char *id, 
ChardevBackend *backend,
 return NULL;
 }

-chr_new = chardev_new(NULL, object_class_get_name(OBJECT_CLASS(cc)),
+chr_new = chardev_new(id, object_class_get_name(OBJECT_CLASS(cc)),
   backend, chr->gcontext, errp);
 if (!chr_new) {
 return NULL;
 }
-chr_new->label = g_strdup(id);

 if (chr->be_open && !chr_new->be_open) {
 qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
--
2.30.2



pgpw61i4Am2vp.pgp
Description: OpenPGP digital signature

[PATCH v4 4/4] tests: Add tests for yank with the chardev-change case

[Lukas Straub]

Add tests for yank with the chardev-change case.

Signed-off-by: Lukas Straub 
---
 MAINTAINERS|   1 +
 tests/unit/meson.build |   3 +-
 tests/unit/test-yank.c | 199 +
 3 files changed, 202 insertions(+), 1 deletion(-)
 create mode 100644 tests/unit/test-yank.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 77259c031d..accb683a55 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2821,6 +2821,7 @@ M: Lukas Straub 
 S: Odd fixes
 F: util/yank.c
 F: migration/yank_functions*
+F: tests/unit/test-yank.c
 F: include/qemu/yank.h
 F: qapi/yank.json

diff --git a/tests/unit/meson.build b/tests/unit/meson.build
index 4bfe4627ba..b3bc2109da 100644
--- a/tests/unit/meson.build
+++ b/tests/unit/meson.build
@@ -123,7 +123,8 @@ if have_system
 'test-util-sockets': ['socket-helpers.c'],
 'test-base64': [],
 'test-bufferiszero': [],
-'test-vmstate': [migration, io]
+'test-vmstate': [migration, io],
+'test-yank': ['socket-helpers.c', qom, io, chardev]
   }
   if 'CONFIG_INOTIFY1' in config_host
 tests += {'test-util-filemonitor': []}
diff --git a/tests/unit/test-yank.c b/tests/unit/test-yank.c
new file mode 100644
index 00..8bc8291a82
--- /dev/null
+++ b/tests/unit/test-yank.c
@@ -0,0 +1,199 @@
+/*
+ * Tests for QEMU yank feature
+ *
+ * Copyright (c) Lukas Straub 
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include 
+
+#include "qemu/config-file.h"
+#include "qemu/module.h"
+#include "qemu/option.h"
+#include "chardev/char-fe.h"
+#include "sysemu/sysemu.h"
+#include "qapi/error.h"
+#include "qapi/qapi-commands-char.h"
+#include "qapi/qapi-types-char.h"
+#include "qapi/qapi-commands-yank.h"
+#include "qapi/qapi-types-yank.h"
+#include "io/channel-socket.h"
+#include "socket-helpers.h"
+
+typedef struct {
+SocketAddress *addr;
+bool old_yank;
+bool new_yank;
+bool fail;
+} CharChangeTestConfig;
+
+static int chardev_change(void *opaque)
+{
+return 0;
+}
+
+static bool is_yank_instance_registered(void)
+{
+YankInstanceList *list;
+bool ret;
+
+list = qmp_query_yank(&error_abort);
+
+ret = !!list;
+
+qapi_free_YankInstanceList(list);
+
+return ret;
+}
+
+static void char_change_test(gconstpointer opaque)
+{
+CharChangeTestConfig *conf = (gpointer) opaque;
+SocketAddress *addr;
+Chardev *chr;
+CharBackend be;
+ChardevReturn *ret;
+QIOChannelSocket *ioc;
+
+/*
+ * Setup a listener socket and determine its address
+ * so we know the TCP port for the client later
+ */
+ioc = qio_channel_socket_new();
+g_assert_nonnull(ioc);
+qio_channel_socket_listen_sync(ioc, conf->addr, 1, &error_abort);
+addr = qio_channel_socket_get_local_address(ioc, &error_abort);
+g_assert_nonnull(addr);
+
+ChardevBackend backend[2] = {
+/* doesn't support yank */
+{ .type = CHARDEV_BACKEND_KIND_NULL },
+/* supports yank */
+{
+.type = CHARDEV_BACKEND_KIND_SOCKET,
+.u.socket.data = &(ChardevSocket) {
+.addr = &(SocketAddressLegacy) {
+.type = SOCKET_ADDRESS_LEGACY_KIND_INET,
+.u.inet.data = &addr->u.inet
+},
+.has_server = true,
+.server = false
+}
+} };
+
+ChardevBackend fail_backend[2] = {
+/* doesn't support yank */
+{
+.type = CHARDEV_BACKEND_KIND_UDP,
+.u.udp.data = &(ChardevUdp) {
+.remote = &(SocketAddressLegacy) {
+.type = SOCKET_ADDRESS_LEGACY_KIND_UNIX,
+.u.q_unix.data = &(UnixSocketAddress) {
+.path = (char *)""
+}
+}
+}
+},
+/* supports yank */
+{
+.type = CHARDEV_BACKEND_KIND_SOCKET,
+.u.socket.data = &(ChardevSocket) {
+.addr = &(SocketAddressLegacy) {
+.type = SOCKET_ADDRESS_LEGACY_KIND_INET,
+.u.inet.data = &(InetSocketAddress) {
+.host = (char *)"127.0.0.1",
+.port = (char *)"0"
+}
+},
+.has_server = true,
+.server = false
+}
+} };
+
+g_assert(!is_yank_instance_registered());
+
+ret = qmp_chardev_add("chardev", &backend[conf->old_yank], &error_abort);
+qapi_free_ChardevReturn(ret);
+chr = qemu_chr_find("chardev");
+g_assert_nonnull(chr);
+
+g_assert(is_yank_instance_registered() == conf->old_yank);
+
+qemu_chr_wait_connected(chr, &error_abort);
+qemu_chr_fe_init(&be, chr, &error_abort);
+/* allow chardev-change */
+qemu_chr_fe_set_handlers(&be, NULL, NULL,
+ NULL, chardev_change, NU

Re: [RFC PATCH v2 0/3] virtio-net: graceful drop of vhost for TAP

[Jason Wang]

在 2021/3/25 下午5:00, Yuri Benditovich 写道:

Hi Jason,

This was discussed earlier on the previous series of patches.
https://lists.gnu.org/archive/html/qemu-devel/2021-02/msg01829.html
There were strong objections from both Daniel and Michael and I feel
that the series was rejected.
There was Michael's claim:
"We did what this patch is trying to change for years now, in
particular KVM also seems to happily disable CPU features not supported
by kernel so I wonder why we can't keep doing it, with tweaks for some
corner cases."



So for cpu feautres, it works since the management have other tool to 
the cpuid. Then management will make sure the migration happens amongs 
the hosts that is compatibile with the same cpuid sets.


For vhost, we don't have such capabilities, that's why I think we need 
to have fallback.




https://lists.gnu.org/archive/html/qemu-devel/2021-02/msg03187.html
And it was Michael's question:
"Can we limit the change to when a VM is migrated in?"
https://lists.gnu.org/archive/html/qemu-devel/2021-02/msg03163.html
So I'm trying to suggest another approach:
- In case of conflicting features (for example RSS and vhost) we in
qemu we do not have enough information to prefer one or another.
- If we drop to userspace in the first set_features we say: "vhost is
less important than other requested features"
- This series keeps backward compatibility, i.e. if you start with
vhost and some features are not available - they are silently cleared.
- But in case the features are available on source machine - they are used
- In case of migration this series says: "We prefer successful
migration even if for that we need to drop to userspace"
- On the migration back to the 1st system we again work with all the
features and with vhost as all the features are available.



One issue for this approach is that. Consider we had two drivers:

1) Driver A that supports split only
2) Driver B that supports packed

Consider src support packed but dest doesn't

So switching driver A to driver B works without migration. But if we 
switch driver from A to B after migration it won't work?


Thanks




Thanks,
Yuri



On Thu, Mar 25, 2021 at 8:59 AM Jason Wang  wrote:


在 2021/3/22 下午8:24, Yuri Benditovich 写道:

Allow fallback to userspace only upon migration, only for specific features
and only if 'vhostforce' is not requested.

Changes from v1:
Patch 1 dropeed (will be submitted in another series)
Added device callback in case the migration should fail due to missing features


Hi Yuri:

Have a quick glance at the series. A questions is why we need to do the
fallback only during load?

I think we should do it in the device initializating. E.g when the vhost
features can not satisfy, we should disable vhost since there.

Thanks



Yuri Benditovich (3):
net: add ability to hide (disable) vhost_net
virtio: introduce 'missing_features_migrated' device callback
virtio-net: implement missing_features_migrated callback

   hw/net/vhost_net.c |  4 ++-
   hw/net/virtio-net.c| 51 ++
   hw/virtio/virtio.c |  8 ++
   include/hw/virtio/virtio.h |  8 ++
   include/net/net.h  |  1 +
   5 files changed, 71 insertions(+), 1 deletion(-)

[PATCH v4 3/4] chardev: Fix yank with the chardev-change case

[Lukas Straub]

When changing from chardev-socket (which supports yank) to
chardev-socket again, it fails, because the new chardev attempts
to register a new yank instance. This in turn fails, as there
still is the yank instance from the current chardev. Also,
the old chardev shouldn't unregister the yank instance when it
is freed.

To fix this, now the new chardev only registers a yank instance if
the current chardev doesn't support yank and thus hasn't registered
one already. Also, when the old chardev is freed, it now only
unregisters the yank instance if the new chardev doesn't need it.

If the initialization of the new chardev fails, it still has
chr->handover_yank_instance set and won't unregister the yank
instance when it is freed.

s->registered_yank is always true here, as chardev-change only works
on user-visible chardevs and those are guraranteed to register a
yank instance as they are initialized via
chardev_new()
 qemu_char_open()
  cc->open() (qmp_chardev_open_socket()).

Signed-off-by: Lukas Straub 
Tested-by: Li Zhang 
---
 chardev/char-socket.c  | 20 +---
 chardev/char.c | 35 ---
 include/chardev/char.h |  3 +++
 3 files changed, 48 insertions(+), 10 deletions(-)

diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 1d455ecca4..daa89fe5d1 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -1126,7 +1126,13 @@ static void char_socket_finalize(Object *obj)
 }
 g_free(s->tls_authz);
 if (s->registered_yank) {
-yank_unregister_instance(CHARDEV_YANK_INSTANCE(chr->label));
+/*
+ * In the chardev-change special-case, we shouldn't unregister the yank
+ * instance, as it still may be needed.
+ */
+if (!chr->handover_yank_instance) {
+yank_unregister_instance(CHARDEV_YANK_INSTANCE(chr->label));
+}
 }

 qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
@@ -1424,8 +1430,14 @@ static void qmp_chardev_open_socket(Chardev *chr,
 qemu_chr_set_feature(chr, QEMU_CHAR_FEATURE_FD_PASS);
 }

-if (!yank_register_instance(CHARDEV_YANK_INSTANCE(chr->label), errp)) {
-return;
+/*
+ * In the chardev-change special-case, we shouldn't register a new yank
+ * instance, as there already may be one.
+ */
+if (!chr->handover_yank_instance) {
+if (!yank_register_instance(CHARDEV_YANK_INSTANCE(chr->label), errp)) {
+return;
+}
 }
 s->registered_yank = true;

@@ -1567,6 +1579,8 @@ static void char_socket_class_init(ObjectClass *oc, void 
*data)
 {
 ChardevClass *cc = CHARDEV_CLASS(oc);

+cc->supports_yank = true;
+
 cc->parse = qemu_chr_parse_socket;
 cc->open = qmp_chardev_open_socket;
 cc->chr_wait_connected = tcp_chr_wait_connected;
diff --git a/chardev/char.c b/chardev/char.c
index 75993f903f..398f09df19 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -39,6 +39,7 @@
 #include "qemu/option.h"
 #include "qemu/id.h"
 #include "qemu/coroutine.h"
+#include "qemu/yank.h"

 #include "chardev-internal.h"

@@ -266,6 +267,7 @@ static void char_init(Object *obj)
 {
 Chardev *chr = CHARDEV(obj);

+chr->handover_yank_instance = false;
 chr->logfd = -1;
 qemu_mutex_init(&chr->chr_write_lock);

@@ -959,6 +961,7 @@ void qemu_chr_set_feature(Chardev *chr,
 static Chardev *chardev_new(const char *id, const char *typename,
 ChardevBackend *backend,
 GMainContext *gcontext,
+bool handover_yank_instance,
 Error **errp)
 {
 Object *obj;
@@ -971,6 +974,7 @@ static Chardev *chardev_new(const char *id, const char 
*typename,

 obj = object_new(typename);
 chr = CHARDEV(obj);
+chr->handover_yank_instance = handover_yank_instance;
 chr->label = g_strdup(id);
 chr->gcontext = gcontext;

@@ -1004,7 +1008,7 @@ Chardev *qemu_chardev_new(const char *id, const char 
*typename,
 id = genid;
 }

-chr = chardev_new(id, typename, backend, gcontext, errp);
+chr = chardev_new(id, typename, backend, gcontext, false, errp);
 if (!chr) {
 return NULL;
 }
@@ -1032,7 +1036,7 @@ ChardevReturn *qmp_chardev_add(const char *id, 
ChardevBackend *backend,
 }

 chr = chardev_new(id, object_class_get_name(OBJECT_CLASS(cc)),
-  backend, NULL, errp);
+  backend, NULL, false, errp);
 if (!chr) {
 return NULL;
 }
@@ -1057,9 +1061,10 @@ ChardevReturn *qmp_chardev_change(const char *id, 
ChardevBackend *backend,
   Error **errp)
 {
 CharBackend *be;
-const ChardevClass *cc;
+const ChardevClass *cc, *cc_new;
 Chardev *chr, *chr_new;
 bool closed_sent = false;
+bool handover_yank_instance;
 ChardevReturn *ret;

 chr = qemu_chr_find(id);
@@ -1091,13 +1096,20 @@ ChardevReturn *qmp_chardev_change(const char *id, 
ChardevBacken

Re: Bug with Windows tap network when running qemu-system-ppc with Mac OS 9 guest

[Howard Spoelstra]

Hi Bin,
(I forgot to reply to all)

> > In answer to your questions:
> >
> > 1. Yes, slirp works on Windows 10 with this setup.
> > 2. Yes, in Linux both tap and slirp work.
>
> Thanks! Just to be clear, the above testing was performed with commit
> 969e50b61a285b0cc8dea6d4d2ade3f758d5ecc7, right?
>

Yes, the test is based on current master, so including that commit.
As said, reverting it restores tap functionality in Windows.

> >
> > My Windows build is done with a fully up to date msys2 installation.
> >
> > I tried to debug in Windows:
> > (gdb) run
> > Starting program: c:\qemu-master-msys2\qemu-system-ppc.exe -L pc-bios
> > -M mac99 -m 128 -sdl -serial stdio -boot c -drive
> > "file=C:\Mac-disks\9.2-usb-pci-ddk.img,format=raw,media=disk" -device
> > "sungem,netdev=network01" -netdev "tap,ifname=TapQemu,id=network01" -S
> > [New Thread 13304.0x1f00]
> > [New Thread 13304.0x2f84]
> > [New Thread 13304.0x3524]
> > [New Thread 13304.0x2b8c]
> > [New Thread 13304.0x368c]
> > [New Thread 13304.0x3668]
> > [New Thread 13304.0xf4c]
> > [New Thread 13304.0x49c]
> > [New Thread 13304.0x1d4c]
> > [New Thread 13304.0x7fc]
> > [Thread 13304.0x7fc exited with code 0]
> > [New Thread 13304.0x357c]
> > [New Thread 13304.0x7c0]
> > [New Thread 13304.0x3564]
> > [New Thread 13304.0x26f4]
> > [New Thread 13304.0x2f68]
> >
> > Program received signal SIGSEGV, Segmentation fault.
> > 0x7ffb9edea991 in ?? () from c:\qemu-master-msys2\libglib-2.0-0.dll
> > (gdb) bt
> > #0  0x7ffb9edea991 in ?? () from c:\qemu-master-msys2\libglib-2.0-0.dll
> > #1  0x00080480bf50 in ?? ()
> > Backtrace stopped: previous frame inner to this frame (corrupt stack?)
> > (gdb)
> >
> > Even before I could attach to the process.
>
> Is QEMU crashed, or the MacOS guest crashed?

Well, that is an interesting question.
qemu-system-ppc -M mac99 uses openbios and it crashes while still in
the openbios window just when it tries to boot the hd.
Besides Mac OS 9.2, I now also tried booting a Mac OS X 10.3 image and
it crashes just the same.

Tracing the default sungem network device shows:

C:\qemu-master-msys2>qemu-system-ppc.exe -L pc-bios -M mac99 -m 128
-sdl -serial stdio -boot c -drive
file=C:\Mac-disks\9.2-usb-pci-ddk.img,format=raw,media=
disk -device sungem,netdev=network01 -netdev
tap,ifname=TapQemu,id=network01 -trace "sung*"
sungem_reset Full reset (PCI:1)
sungem_rx_reset RX reset
sungem_tx_reset TX reset
sungem_reset Full reset (PCI:1)
sungem_rx_reset RX reset
sungem_tx_reset TX reset
sungem_rx_mac_disabled Check RX MAC disabled
sungem_rx_mac_disabled Check RX MAC disabled
sungem_rx_mac_disabled Check RX MAC disabled
sungem_mmio_mac_read MMIO mac read from 0x80 val=0x3456
sungem_mmio_mac_read MMIO mac read from 0x84 val=0x12
sungem_mmio_mac_read MMIO mac read from 0x88 val=0x5254

>> =
>> OpenBIOS 1.1 [Mar 16 2021 08:16]
>> Configuration device id QEMU version 1 machine id 1
>> CPUs: 1
>> Memory: 128M
>> UUID: ----
>> CPU type PowerPC,G4
milliseconds isn't unique.

Best,
Howard

Re: [PATCH v2 00/10] Acceptance Test: introduce base class for Linux based tests

[Auger Eric]

Hi Wainer,

On 3/25/21 8:45 PM, Wainer dos Santos Moschetta wrote:
> Hi,
> 
> On 3/23/21 7:15 PM, Cleber Rosa wrote:
>> This introduces a base class for tests that need to interact with a
>> Linux guest.  It generalizes the "boot_linux.py" code, already been
>> used by the "virtiofs_submounts.py" and also SSH related code being
>> used by that and "linux_ssh_mips_malta.py".
> 
> I ran the linux_ssh_mips_malta.py tests, they all passed:
> 
> (11/34)
> tests/acceptance/linux_ssh_mips_malta.py:LinuxSSH.test_mips_malta32eb_kernel3_2_0:
> PASS (64.41 s)
> (12/34)
> tests/acceptance/linux_ssh_mips_malta.py:LinuxSSH.test_mips_malta32el_kernel3_2_0:
> PASS (63.43 s)
> (13/34)
> tests/acceptance/linux_ssh_mips_malta.py:LinuxSSH.test_mips_malta64eb_kernel3_2_0:
> PASS (63.76 s)
> (14/34)
> tests/acceptance/linux_ssh_mips_malta.py:LinuxSSH.test_mips_malta64el_kernel3_2_0:
> PASS (62.52 s)
> 
> Then I tried the virtiofs_submounts.py tests, it finishes with error.
> Something like that fixes it:
> 
> diff --git a/tests/acceptance/virtiofs_submounts.py
> b/tests/acceptance/virtiofs_submounts.py
> index d77ee35674..21ad7d792e 100644
> --- a/tests/acceptance/virtiofs_submounts.py
> +++ b/tests/acceptance/virtiofs_submounts.py
> @@ -195,7 +195,7 @@ def setUp(self):
> 
>  self.run(('ssh-keygen', '-N', '', '-t', 'ed25519', '-f',
> self.ssh_key))
> 
> -    pubkey = open(self.ssh_key + '.pub').read()
> +    pubkey = self.ssh_key + '.pub'

Yes I discovered that too when developping the SMMU test. Thanks for
mentionning

Eric
> 
>  super(VirtiofsSubmountsTest, self).setUp(pubkey)
> 
> 
>>
>> While at it, a number of fixes on hopeful improvements to those tests
>> were added.
>>
>> Changes from v1:
>>
>> * Majority of v1 patches have been merged.
>>
>> * New patches:
>>    - Acceptance Tests: make username/password configurable
>>    - Acceptance Tests: set up SSH connection by default after boot for
>> LinuxTest
>>    - tests/acceptance/virtiofs_submounts.py: remove launch_vm()
>>
>> * Allowed for the configuration of the network device type (defaulting
>>    to virtio-net) [Phil]
>>
>> * Fix module name typo (s/qemu.util/qemu.utils/) in the commit message
>>    [John]
>>
>> * Tests based on LinuxTest will have the SSH connection already prepared
>>
>> Cleber Rosa (10):
>>    tests/acceptance/virtiofs_submounts.py: add missing accel tag
>>    tests/acceptance/virtiofs_submounts.py: evaluate string not length
>>    Python: add utility function for retrieving port redirection
>>    Acceptance Tests: move useful ssh methods to base class
>>    Acceptance Tests: add port redirection for ssh by default
>>    Acceptance Tests: make username/password configurable
>>    Acceptance Tests: set up SSH connection by default after boot for
>>  LinuxTest
>>    tests/acceptance/virtiofs_submounts.py: remove launch_vm()
>>    Acceptance Tests: add basic documentation on LinuxTest base class
>>    Acceptance Tests: introduce CPU hotplug test
>>
>>   docs/devel/testing.rst    | 25 
>>   python/qemu/utils.py  | 35 
>>   tests/acceptance/avocado_qemu/__init__.py | 63 +++--
>>   tests/acceptance/hotplug_cpu.py   | 37 
>>   tests/acceptance/info_usernet.py  | 29 ++
>>   tests/acceptance/linux_ssh_mips_malta.py  | 44 ++-
>>   tests/acceptance/virtiofs_submounts.py    | 69 +++
>>   tests/vm/basevm.py    |  7 +--
>>   8 files changed, 198 insertions(+), 111 deletions(-)
>>   create mode 100644 python/qemu/utils.py
>>   create mode 100644 tests/acceptance/hotplug_cpu.py
>>   create mode 100644 tests/acceptance/info_usernet.py
>>

Re: [PATCH 0/7] block/nbd: decouple reconnect from drain

[Roman Kagan]

On Wed, Mar 17, 2021 at 11:35:31AM +0300, Vladimir Sementsov-Ogievskiy wrote:
> 15.03.2021 09:06, Roman Kagan wrote:
> > The reconnection logic doesn't need to stop while in a drained section.
> > Moreover it has to be active during the drained section, as the requests
> > that were caught in-flight with the connection to the server broken can
> > only usefully get drained if the connection is restored.  Otherwise such
> > requests can only either stall resulting in a deadlock (before
> > 8c517de24a), or be aborted defeating the purpose of the reconnection
> > machinery (after 8c517de24a).
> > 
> > This series aims to just stop messing with the drained section in the
> > reconnection code.
> > 
> > While doing so it undoes the effect of 5ad81b4946 ("nbd: Restrict
> > connection_co reentrance"); as I've missed the point of that commit I'd
> > appreciate more scrutiny in this area.
> > 
> > Roman Kagan (7):
> >block/nbd: avoid touching freed connect_thread
> >block/nbd: use uniformly nbd_client_connecting_wait
> >block/nbd: assert attach/detach runs in the proper context
> >block/nbd: transfer reconnection stuff across aio_context switch
> >block/nbd: better document a case in nbd_co_establish_connection
> >block/nbd: decouple reconnect from drain
> >block/nbd: stop manipulating in_flight counter
> > 
> >   block/nbd.c  | 191 +++
> >   nbd/client.c |   2 -
> >   2 files changed, 86 insertions(+), 107 deletions(-)
> > 
> 
> 
> Hmm. The huge source of problems for this series is weird logic around
> drain and aio context switch in NBD driver.
> 
> Why do we have all these too complicated logic with abuse of in_flight
> counter in NBD? The answer is connection_co. NBD differs from other
> drivers, it has a coroutine independent of request coroutines. And we
> have to move this coroutine carefully to new aio context. We can't
> just enter it from the new context, we want to be sure that
> connection_co is in one of yield points that supports reentering.
> 
> I have an idea of how to avoid this thing: drop connection_co at all.
> 
> 1. nbd negotiation goes to connection thread and becomes independent
> of any aio context.
> 
> 2. waiting for server reply goes to request code. So, instead of
> reading the replay from socket always in connection_co, we read in the
> request coroutine, after sending the request. We'll need a CoMutex for
> it (as only one request coroutine should read from socket), and be
> prepared to coming reply is not for _this_ request (in this case we
> should wake another request and continue read from socket).
> 
> but this may be too much for soft freeze.

This approach does look appealing to me, and I gave it a quick shot but
the amount of changes this involves exceeds the rc tolerance indeed.

> Another idea:
> 
> You want all the requests be completed on drain_begin(), not
> cancelled. Actually, you don't need reconnect runnning during drained
> section for it. It should be enough just wait for all currenct
> requests before disabling the reconnect in drain_begin handler.

So effectively you suggest doing nbd's own drain within
bdrv_co_drain_begin callback.  I'm not totally sure there are no
assumptions this may break, but I'll try to look into this possibility.

Thanks,
Roman.

Re: [PATCH v4 0/4] yank: Add chardev tests and fixes

[no-reply]

Patchew URL: https://patchew.org/QEMU/cover.1616744509.git.lukasstra...@web.de/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: cover.1616744509.git.lukasstra...@web.de
Subject: [PATCH v4 0/4] yank: Add chardev tests and fixes

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 - [tag update]  patchew/20210323183701.281152-1-f4...@amsat.org -> 
patchew/20210323183701.281152-1-f4...@amsat.org
 * [new tag] patchew/cover.1616744509.git.lukasstra...@web.de -> 
patchew/cover.1616744509.git.lukasstra...@web.de
Switched to a new branch 'test'
b0e7602 tests: Add tests for yank with the chardev-change case
f065000 chardev: Fix yank with the chardev-change case
e006dcc chardev/char.c: Always pass id to chardev_new
8a11307 chardev/char.c: Move object_property_try_add_child out of chardev_new

=== OUTPUT BEGIN ===
1/4 Checking commit 8a113074e682 (chardev/char.c: Move 
object_property_try_add_child out of chardev_new)
2/4 Checking commit e006dccfdf7e (chardev/char.c: Always pass id to chardev_new)
3/4 Checking commit f06500035576 (chardev: Fix yank with the chardev-change 
case)
4/4 Checking commit b0e7602e4800 (tests: Add tests for yank with the 
chardev-change case)
ERROR: Macros with multiple statements should be enclosed in a do - while loop
#228: FILE: tests/unit/test-yank.c:179:
+#define CHAR_CHANGE_TEST(name, _old_yank, _new_yank)   
\
+g_test_add_data_func("/yank/char_change/success/" # name,  
\
+ &(CharChangeTestConfig) { .addr = &tcpaddr,   
\
+   .old_yank = 
(_old_yank),\
+   .new_yank = 
(_new_yank),\
+   .fail = false },
\
+ char_change_test);
\
+g_test_add_data_func("/yank/char_change/fail/" # name, 
\
+ &(CharChangeTestConfig) { .addr = &tcpaddr,   
\
+   .old_yank = 
(_old_yank),\
+   .new_yank = 
(_new_yank),\
+   .fail = true }, 
\
+ char_change_test);

total: 1 errors, 0 warnings, 215 lines checked

Patch 4/4 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/cover.1616744509.git.lukasstra...@web.de/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-de...@redhat.com

Re: [PATCH v4 0/4] yank: Add chardev tests and fixes

[Lukas Straub]

On Fri, 26 Mar 2021 01:09:46 -0700 (PDT)
no-re...@patchew.org wrote:

> Patchew URL: 
> https://patchew.org/QEMU/cover.1616744509.git.lukasstra...@web.de/
> 
> 
> 
> Hi,
> 
> This series seems to have some coding style problems. See output below for
> more information:
> 
> Type: series
> Message-id: cover.1616744509.git.lukasstra...@web.de
> Subject: [PATCH v4 0/4] yank: Add chardev tests and fixes
> 
> === TEST SCRIPT BEGIN ===
> #!/bin/bash
> git rev-parse base > /dev/null || exit 0
> git config --local diff.renamelimit 0
> git config --local diff.renames True
> git config --local diff.algorithm histogram
> ./scripts/checkpatch.pl --mailback base..
> === TEST SCRIPT END ===
> 
> Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
> From https://github.com/patchew-project/qemu
>  - [tag update]  patchew/20210323183701.281152-1-f4...@amsat.org -> 
> patchew/20210323183701.281152-1-f4...@amsat.org
>  * [new tag] patchew/cover.1616744509.git.lukasstra...@web.de -> 
> patchew/cover.1616744509.git.lukasstra...@web.de
> Switched to a new branch 'test'
> b0e7602 tests: Add tests for yank with the chardev-change case
> f065000 chardev: Fix yank with the chardev-change case
> e006dcc chardev/char.c: Always pass id to chardev_new
> 8a11307 chardev/char.c: Move object_property_try_add_child out of chardev_new
> 
> === OUTPUT BEGIN ===
> 1/4 Checking commit 8a113074e682 (chardev/char.c: Move 
> object_property_try_add_child out of chardev_new)
> 2/4 Checking commit e006dccfdf7e (chardev/char.c: Always pass id to 
> chardev_new)
> 3/4 Checking commit f06500035576 (chardev: Fix yank with the chardev-change 
> case)
> 4/4 Checking commit b0e7602e4800 (tests: Add tests for yank with the 
> chardev-change case)
> ERROR: Macros with multiple statements should be enclosed in a do - while loop
> #228: FILE: tests/unit/test-yank.c:179:
> +#define CHAR_CHANGE_TEST(name, _old_yank, _new_yank) 
>   \
> +g_test_add_data_func("/yank/char_change/success/" # name,
>   \
> + &(CharChangeTestConfig) { .addr = &tcpaddr, 
>   \
> +   .old_yank = 
> (_old_yank),\
> +   .new_yank = 
> (_new_yank),\
> +   .fail = false },  
>   \
> + char_change_test);  
>   \
> +g_test_add_data_func("/yank/char_change/fail/" # name,   
>   \
> + &(CharChangeTestConfig) { .addr = &tcpaddr, 
>   \
> +   .old_yank = 
> (_old_yank),\
> +   .new_yank = 
> (_new_yank),\
> +   .fail = true },   
>   \
> + char_change_test);
> 
> total: 1 errors, 0 warnings, 215 lines checked

This is expected. It needs to be this way so the anonymous structs don't go out 
of
scope.

Regards,
Lukas Straub

> Patch 4/4 has style problems, please review.  If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
> 
> === OUTPUT END ===
> 
> Test command exited with code: 1
> 
> 
> The full log is available at
> http://patchew.org/logs/cover.1616744509.git.lukasstra...@web.de/testing.checkpatch/?type=message.
> ---
> Email generated automatically by Patchew [https://patchew.org/].
> Please send your feedback to patchew-de...@redhat.com


-- 



pgpPGTVmRqWwB.pgp
Description: OpenPGP digital signature

[Bug 1921468] Re: [UBUNTU 20.04] KVM guest fails to find zipl boot menu index

[Frank Heimes]

** Also affects: qemu
   Importance: Undecided
   Status: New

** No longer affects: qemu

** Also affects: ubuntu-z-systems
   Importance: Undecided
   Status: New

** Changed in: ubuntu-z-systems
 Assignee: (unassigned) => Skipper Bug Screeners (skipper-screen-team)

** Changed in: qemu (Ubuntu)
 Assignee: Skipper Bug Screeners (skipper-screen-team) => Canonical Server 
Team (canonical-server)

** Changed in: ubuntu-z-systems
   Importance: Undecided => Medium

** Changed in: qemu (Ubuntu)
   Importance: Undecided => Medium

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1921468

Title:
  [UBUNTU 20.04] KVM guest fails to find zipl boot menu index

Status in Ubuntu on IBM z Systems:
  New
Status in qemu package in Ubuntu:
  New

Bug description:
  ---Problem Description---
  A KVM guest fails to find the zipl boot menu index if the "zIPL" magic value 
is listed at the end of a disk block. 
   
  ---System Hang---
  System sits in disabled wait, last console display
  LOADPARM=[]
  Using virtio-blk.
  Using ECKD scheme (block size  4096), CDL
  VOLSER=[0X0067]
   
   
  ---Steps to Reproduce---
  1. Install Distro KVM guest from ISO on a DASD, e.g. using virt-install, my 
invocation was 
  $ virt-install --name secguest2 --memory 2048 --disk 
path=/dev/disk/by-path/ccw-0.0.af6a --cdrom /var/lib/libvirt/images/xx.iso

  2. Select DHCP networking and ASCII console, and accept all defaults
  of the installer

  3. Let the installer reboot after the installation completes

  It is possible to recover by editing the domain XML with an explicit loadparm 
to select a boot menu entry. E.g. I changed the disk definition to
 





  

  The patches are now upstream:
  5f97ba0c74cc ("pc-bios/s390-ccw: fix off-by-one error")
  468184ec9024 ("pc-bios/s390-ccw: break loop if a null block number is 
reached")

  Current versions of qemu within Ubuntu

  focal (20.04LTS) 1:4.2-3ubuntu6 [ports]: arm64 armhf ppc64el s390x
  focal-updates (metapackages): 1:4.2-3ubuntu6.14: amd64 arm64 armhf ppc64el 
s390x

  groovy (20.10) (metapackages): 1:5.0-5ubuntu9 [ports]: arm64 armhf ppc64el 
s390x
  groovy-updates (metapackages): 1:5.0-5ubuntu9.6: amd64 arm64 armhf ppc64el 
s390x

  hirsute (metapackages): 1:5.2+dfsg-9ubuntu1: amd64 arm64 armhf ppc64el
  s390x

  
  git-commits will apply seamlessley for the requested levels if not already 
integrated

To manage notifications about this bug go to:
https://bugs.launchpad.net/ubuntu-z-systems/+bug/1921468/+subscriptions

Re: Bug with Windows tap network when running qemu-system-ppc with Mac OS 9 guest

[BALATON Zoltan]

On Fri, 26 Mar 2021, Howard Spoelstra wrote:

On Fri, Mar 26, 2021 at 2:50 AM Bin Meng  wrote:


Hi Howard,

On Fri, Mar 26, 2021 at 1:35 AM Peter Maydell  wrote:


(adding the relevant people to the cc list)

On Thu, 25 Mar 2021 at 17:26, Howard Spoelstra  wrote:


Hi,

When running qemu-system-ppc with Mac OS guest, the guest crashes when
using a tap network connection. Openvpn 2.4.9-I601-win10 is installed
with TAP-Windows 9.24.2. A tap connection called TapQemu is bridged
with the default ethernet connection. It gets activated when I start
qemu.

To reproduce, compile qemu-system-ppc from current source and run:

qemu-system-ppc.exe ^
-L pc-bios ^
-M mac99 ^
-m 128 ^
-sdl -serial stdio ^
-boot c ^
-drive file=C:\Mac-disks\9.2.img,format=raw,media=disk ^
-device sungem,netdev=network01 -netdev tap,ifname=TapQemu,id=network01

I bisected to the commit below. Thanks for looking into this.


Thanks for reporting.

Can you please provide some further information:

1. Does "-net user" work on Windows?
2. If running QEMU under Linux, does "-net tap" or "-net user" work?

Regards,
Bin


Hello Bin,

Thanks for getting back to me. I forgot to mention that reverting the
above patch restores functionality. And that other applications using
the same tap device work correctly.
In answer to your questions:

1. Yes, slirp works on Windows 10 with this setup.
2. Yes, in Linux both tap and slirp work.

My Windows build is done with a fully up to date msys2 installation.

I tried to debug in Windows:
(gdb) run
Starting program: c:\qemu-master-msys2\qemu-system-ppc.exe -L pc-bios
-M mac99 -m 128 -sdl -serial stdio -boot c -drive
"file=C:\Mac-disks\9.2-usb-pci-ddk.img,format=raw,media=disk" -device
"sungem,netdev=network01" -netdev "tap,ifname=TapQemu,id=network01" -S
[New Thread 13304.0x1f00]
[New Thread 13304.0x2f84]
[New Thread 13304.0x3524]
[New Thread 13304.0x2b8c]
[New Thread 13304.0x368c]
[New Thread 13304.0x3668]
[New Thread 13304.0xf4c]
[New Thread 13304.0x49c]
[New Thread 13304.0x1d4c]
[New Thread 13304.0x7fc]
[Thread 13304.0x7fc exited with code 0]
[New Thread 13304.0x357c]
[New Thread 13304.0x7c0]
[New Thread 13304.0x3564]
[New Thread 13304.0x26f4]
[New Thread 13304.0x2f68]

Program received signal SIGSEGV, Segmentation fault.
0x7ffb9edea991 in ?? () from c:\qemu-master-msys2\libglib-2.0-0.dll
(gdb) bt
#0  0x7ffb9edea991 in ?? () from c:\qemu-master-msys2\libglib-2.0-0.dll
#1  0x00080480bf50 in ?? ()
Backtrace stopped: previous frame inner to this frame (corrupt stack?)
(gdb)

Even before I could attach to the process.


If you run QEMU under gdb you don't have to attach to it but to get a 
meaningful backtrace you should configure and compile QEMU with 
--enable-debug (this will make it run slower so not recommended normally 
but for debugging that would be needed). If the stack is really corrupted 
then you may not get a useful backtrace or it may be a problem with gdb on 
Windows. I've found that gdb on Windows works for simple things but could 
give bad results for more complex stuff. WinDbg may be better but it's 
harder to use (needs some registry change I think to enable core dumps 
then you could open and analyze core dumps with it or it should be able 
to run command directly but I don't know how that works).


Another idea: maybe you could check other threads in gdb. Not sure if that 
would reveal anything but may worth a try. I think the commands you need 
are "info threads" and "apply all bt" or something similar.


Regards,
BALATON Zoltan

Re: [PATCH 3/5] tools/vhost-user-i2c: Add backend driver

[Arnd Bergmann]

On Fri, Mar 26, 2021 at 8:14 AM Viresh Kumar  wrote:
> On 25-03-21, 17:16, Arnd Bergmann wrote:
> > On Wed, Mar 24, 2021 at 8:33 AM Viresh Kumar  
> > wrote:

> >
> > It looks like you are not handling endianness conversion here. As far as I
> > can tell, the protocol requires little-endian data, but the code might
> > run on a big-endian CPU.
>
> I hope this is all we are required to do here, right ?
>
> @@ -442,7 +421,7 @@ static void vi2c_handle_ctrl(VuDev *dev, int qidx)
>  out_hdr = elem->out_sg[0].iov_base;
>
>  /* Bit 0 is reserved in virtio spec */
> -msg.addr = out_hdr->addr >> 1;
> +msg.addr = le16toh(out_hdr->addr) >> 1;
>
>  /* Read Operation */
>  if (elem->out_num == 1 && elem->in_num == 2) {
> @@ -489,7 +468,7 @@ static void vi2c_handle_ctrl(VuDev *dev, int qidx)
>  in_hdr->status = fail_next ? VIRTIO_I2C_MSG_ERR : vi2c_xfer(dev, 
> &msg);
>  if (in_hdr->status == VIRTIO_I2C_MSG_ERR) {
>  /* We need to fail remaining transfers as well */
> -fail_next = out_hdr->flags & VIRTIO_I2C_FLAGS_FAIL_NEXT;
> +fail_next = le32toh(out_hdr->flags) & VIRTIO_I2C_FLAGS_FAIL_NEXT;
>  }
>
> These are the only fields we are passing apart from buf, which goes
> directly to the client device.

I think so, the in_hdr is only one byte long, so it doesn't have an
endianness.

   Arnd

Re: [PATCH v4 1/4] virtio:add support in configure interrupt

[Jason Wang]

在 2021/3/25 下午3:15, Cindy Lu 写道:

+enum virtio_config_status {
+VIRTIO_CONFIG_SUPPORT,
+VIRTIO_CONFIG_WORK,
+VIRTIO_CONFIG_STOP,
+VIRTIO_CONFIG_STATUS_UNKNOWN,

Any reason for this extra state? I think we can know whether the config
interrupt is being used through a

Thanks


The problem is I need to split the backend devices into 3 types,
1） normal device
2）vdpa support config interrupt. and the configur interrupt is active now
3）vdpa not support config interrupt.
So I  add this bit and this bit will init in vpda /vhost modules and
qemu can check this bit to know the  which behariver we will do in
virtio bus  and other modules,



I wonder whether it's a must. We can setup guest notifiers 
unconditionally, so if it's an vhost bakcend without config interrupt 
support, such notifiers won't be used.


Thanks



  Maybe I need to change this bit's name
to make it more clearly

Thanks
Cindy

Re: [PATCH 3/5] tools/vhost-user-i2c: Add backend driver

[Arnd Bergmann]

On Fri, Mar 26, 2021 at 7:01 AM Viresh Kumar  wrote:
> On 25-03-21, 17:16, Arnd Bergmann wrote:
> > On Wed, Mar 24, 2021 at 8:33 AM Viresh Kumar  
> > wrote:
> >
> > > +static uint8_t vi2c_xfer(VuDev *dev, struct i2c_msg *msg)
> > > +{
> > > +VuI2c *i2c = container_of(dev, VuI2c, dev.parent);
> > > +struct i2c_rdwr_ioctl_data data;
> > > +VI2cAdapter *adapter;
> > > +
> > > +adapter = vi2c_find_adapter(i2c, msg->addr);
> > > +if (!adapter) {
> > > +g_printerr("Failed to find adapter for address: %x\n", 
> > > msg->addr);
> > > +return VIRTIO_I2C_MSG_ERR;
> > > +}
> > > +
> > > +data.nmsgs = 1;
> > > +data.msgs = msg;
> > > +
> > > +if (ioctl(adapter->fd, I2C_RDWR, &data) < 0) {
> > > +g_printerr("Failed to transfer data to address %x : %d\n", 
> > > msg->addr, errno);
> > > +return VIRTIO_I2C_MSG_ERR;
> > > +}
> >
> > As you found during testing, this doesn't work for host kernels
> > that only implement the SMBUS protocol. Since most i2c clients
> > only need simple register read/write operations, I think you should
> > at least handle the common ones (and one two byte read/write)
> > here to make it more useful.
>
> I am thinking if that is what we really want to support, then
> shouldn't the i2c virtio spec be updated first to support SMBUS type
> transfers as well?

As far as I can tell, all the simple devices should just work, with
I2C_FUNC_SMBUS_READ_BLOCK_DATA being the main exception,
but it seems that has practically no users.

   Arnd

Re: Fail to create sev-guest object on 6.0.0-rc0

[Paolo Bonzini]

On 25/03/21 21:31, Tom Lendacky wrote:

On 3/25/21 1:51 PM, Brijesh Singh wrote:

Hi All,

It seems creating the sev-guest object is broken rc0 tag. The following
command is no longer able to create the sev-guest object

$QEMU \

  -machine ...,confidential-guest-support=sev0 \

  -object sev-guest,id=sev0,policy=0x1 \

It fails with "-object sev-guest,id=sev0: Invalid parameter
'sev-guest'". I will try to bisect the broken commit but if someone has
already looked into it then let me know.


I bisected it to:

bc2f4fcb1d ("qom: move user_creatable_add_opts logic to vl.c and QAPIfy it")


Will fix.

Paolo

Re: [PATCH 0/3] s390x: modularize virtio-gpu-ccw

[Gerd Hoffmann]

  Hi,

> Compared to Daniels suggestion, I find that one conceptually clearer,
> and even more flexible. Implementation-wise I find this patch-set
> simpler.

Given we are in fixes-only mode I think we should go for the simple
solution.  Should be easy enough to revert in case we want replace
this with something else after the 6.0 release.

> Reviewed-by: Halil Pasic 
> Tested-by: Halil Pasic 

Added & queued.
CI running, pull req later today when all goes well.

thanks,
  Gerd

Re: Bug with Windows tap network when running qemu-system-ppc with Mac OS 9 guest

[Jason Wang]

在 2021/3/26 下午4:21, BALATON Zoltan 写道:

On Fri, 26 Mar 2021, Howard Spoelstra wrote:

On Fri, Mar 26, 2021 at 2:50 AM Bin Meng  wrote:


Hi Howard,

On Fri, Mar 26, 2021 at 1:35 AM Peter Maydell 
 wrote:


(adding the relevant people to the cc list)

On Thu, 25 Mar 2021 at 17:26, Howard Spoelstra  
wrote:


Hi,

When running qemu-system-ppc with Mac OS guest, the guest crashes 
when

using a tap network connection. Openvpn 2.4.9-I601-win10 is installed
with TAP-Windows 9.24.2. A tap connection called TapQemu is bridged
with the default ethernet connection. It gets activated when I start
qemu.

To reproduce, compile qemu-system-ppc from current source and run:

qemu-system-ppc.exe ^
-L pc-bios ^
-M mac99 ^
-m 128 ^
-sdl -serial stdio ^
-boot c ^
-drive file=C:\Mac-disks\9.2.img,format=raw,media=disk ^
-device sungem,netdev=network01 -netdev 
tap,ifname=TapQemu,id=network01


I bisected to the commit below. Thanks for looking into this.


Thanks for reporting.

Can you please provide some further information:

1. Does "-net user" work on Windows?
2. If running QEMU under Linux, does "-net tap" or "-net user" work?

Regards,
Bin


Hello Bin,

Thanks for getting back to me. I forgot to mention that reverting the
above patch restores functionality. And that other applications using
the same tap device work correctly.
In answer to your questions:

1. Yes, slirp works on Windows 10 with this setup.
2. Yes, in Linux both tap and slirp work.

My Windows build is done with a fully up to date msys2 installation.

I tried to debug in Windows:
(gdb) run
Starting program: c:\qemu-master-msys2\qemu-system-ppc.exe -L pc-bios
-M mac99 -m 128 -sdl -serial stdio -boot c -drive
"file=C:\Mac-disks\9.2-usb-pci-ddk.img,format=raw,media=disk" -device
"sungem,netdev=network01" -netdev "tap,ifname=TapQemu,id=network01" -S
[New Thread 13304.0x1f00]
[New Thread 13304.0x2f84]
[New Thread 13304.0x3524]
[New Thread 13304.0x2b8c]
[New Thread 13304.0x368c]
[New Thread 13304.0x3668]
[New Thread 13304.0xf4c]
[New Thread 13304.0x49c]
[New Thread 13304.0x1d4c]
[New Thread 13304.0x7fc]
[Thread 13304.0x7fc exited with code 0]
[New Thread 13304.0x357c]
[New Thread 13304.0x7c0]
[New Thread 13304.0x3564]
[New Thread 13304.0x26f4]
[New Thread 13304.0x2f68]

Program received signal SIGSEGV, Segmentation fault.
0x7ffb9edea991 in ?? () from c:\qemu-master-msys2\libglib-2.0-0.dll
(gdb) bt
#0  0x7ffb9edea991 in ?? () from 
c:\qemu-master-msys2\libglib-2.0-0.dll

#1  0x00080480bf50 in ?? ()
Backtrace stopped: previous frame inner to this frame (corrupt stack?)
(gdb)

Even before I could attach to the process.


If you run QEMU under gdb you don't have to attach to it but to get a 
meaningful backtrace you should configure and compile QEMU with 
--enable-debug (this will make it run slower so not recommended 
normally but for debugging that would be needed). If the stack is 
really corrupted then you may not get a useful backtrace or it may be 
a problem with gdb on Windows. I've found that gdb on Windows works 
for simple things but could give bad results for more complex stuff. 
WinDbg may be better but it's harder to use (needs some registry 
change I think to enable core dumps then you could open and analyze 
core dumps with it or it should be able to run command directly but I 
don't know how that works).


Another idea: maybe you could check other threads in gdb. Not sure if 
that would reveal anything but may worth a try. I think the commands 
you need are "info threads" and "apply all bt" or something similar.


Regards,
BALATON Zoltan



It looks to me the patch tires to recycle a temporary buffer to tap thread.

Please try to attached fix to see it if works.

Thanks

From bdd7b4b7e13264f30d4abbc6f0f32c8af935ff17 Mon Sep 17 00:00:00 2001
From: Jason Wang 
Date: Fri, 26 Mar 2021 16:46:43 +0800
Subject: [PATCH] tap-win32: correctly recycle buffers

Commit 969e50b61a28 ("net: Pad short frames to minimum size before
sending from SLiRP/TAP") tries to pad frames but try to recyle the
local array that is used for padding to tap thread. This patch fixes
this by recyling the original buffer.

Fixes: 969e50b61a28 ("net: Pad short frames to minimum size before sending from 
SLiRP/TAP")
Signed-off-by: Jason Wang 
---
 net/tap-win32.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/net/tap-win32.c b/net/tap-win32.c
index d7c2a8759c..95dacbd171 100644
--- a/net/tap-win32.c
+++ b/net/tap-win32.c
@@ -686,7 +686,7 @@ static ssize_t tap_receive(NetClientState *nc, const 
uint8_t *buf, size_t size)
 static void tap_win32_send(void *opaque)
 {
 TAPState *s = opaque;
-uint8_t *buf;
+uint8_t *buf, orig_buf;
 int max_size = 4096;
 int size;
 uint8_t min_pkt[ETH_ZLEN];
@@ -694,6 +694,8 @@ static void tap_win32_send(void *opaque)
 
 size = tap_win32_read(s->handle, &buf, max_size);
 if (size > 0) {
+orig_buf = buf;
+
 if (!s->nc.peer->do_not_pad) {
 if (eth_pad_short_frame

Re: [PATCH] MAINTAINERS: add/replace backups for some s390 areas

[Christian Borntraeger]

On 25.03.21 14:55, Matthew Rosato wrote:

S390 PCI currently has no backup, add one.  Add an additional backup
for vfio-ccw and refresh the backup for vfio-ap.

Signed-off-by: Matthew Rosato 


Acked-by: Christian Borntraeger 

---
  MAINTAINERS | 5 +++--
  1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 554be84..5620fc8 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1516,6 +1516,7 @@ L: qemu-s3...@nongnu.org
  
  S390 PCI

  M: Matthew Rosato 
+M: Eric Farman 
  S: Supported
  F: hw/s390x/s390-pci*
  F: include/hw/s390x/s390-pci*
@@ -1830,6 +1831,7 @@ F: docs/igd-assign.txt
  vfio-ccw
  M: Cornelia Huck 
  M: Eric Farman 
+M: Matthew Rosato 
  S: Supported
  F: hw/vfio/ccw.c
  F: hw/s390x/s390-ccw.c
@@ -1839,10 +1841,9 @@ T: git https://gitlab.com/cohuck/qemu.git s390-next
  L: qemu-s3...@nongnu.org
  
  vfio-ap

-M: Christian Borntraeger 
  M: Tony Krowiak 
  M: Halil Pasic 
-M: Pierre Morel 
+M: Jason Herne 
  S: Supported
  F: hw/s390x/ap-device.c
  F: hw/s390x/ap-bridge.c

Re: Bug with Windows tap network when running qemu-system-ppc with Mac OS 9 guest

[Bin Meng]

On Fri, Mar 26, 2021 at 4:49 PM Jason Wang  wrote:
>
>
> 在 2021/3/26 下午4:21, BALATON Zoltan 写道:
> > On Fri, 26 Mar 2021, Howard Spoelstra wrote:
> >> On Fri, Mar 26, 2021 at 2:50 AM Bin Meng  wrote:
> >>>
> >>> Hi Howard,
> >>>
> >>> On Fri, Mar 26, 2021 at 1:35 AM Peter Maydell
> >>>  wrote:
> 
>  (adding the relevant people to the cc list)
> 
>  On Thu, 25 Mar 2021 at 17:26, Howard Spoelstra 
>  wrote:
> >
> > Hi,
> >
> > When running qemu-system-ppc with Mac OS guest, the guest crashes
> > when
> > using a tap network connection. Openvpn 2.4.9-I601-win10 is installed
> > with TAP-Windows 9.24.2. A tap connection called TapQemu is bridged
> > with the default ethernet connection. It gets activated when I start
> > qemu.
> >
> > To reproduce, compile qemu-system-ppc from current source and run:
> >
> > qemu-system-ppc.exe ^
> > -L pc-bios ^
> > -M mac99 ^
> > -m 128 ^
> > -sdl -serial stdio ^
> > -boot c ^
> > -drive file=C:\Mac-disks\9.2.img,format=raw,media=disk ^
> > -device sungem,netdev=network01 -netdev
> > tap,ifname=TapQemu,id=network01
> >
> > I bisected to the commit below. Thanks for looking into this.
> >>>
> >>> Thanks for reporting.
> >>>
> >>> Can you please provide some further information:
> >>>
> >>> 1. Does "-net user" work on Windows?
> >>> 2. If running QEMU under Linux, does "-net tap" or "-net user" work?
> >>>
> >>> Regards,
> >>> Bin
> >>
> >> Hello Bin,
> >>
> >> Thanks for getting back to me. I forgot to mention that reverting the
> >> above patch restores functionality. And that other applications using
> >> the same tap device work correctly.
> >> In answer to your questions:
> >>
> >> 1. Yes, slirp works on Windows 10 with this setup.
> >> 2. Yes, in Linux both tap and slirp work.
> >>
> >> My Windows build is done with a fully up to date msys2 installation.
> >>
> >> I tried to debug in Windows:
> >> (gdb) run
> >> Starting program: c:\qemu-master-msys2\qemu-system-ppc.exe -L pc-bios
> >> -M mac99 -m 128 -sdl -serial stdio -boot c -drive
> >> "file=C:\Mac-disks\9.2-usb-pci-ddk.img,format=raw,media=disk" -device
> >> "sungem,netdev=network01" -netdev "tap,ifname=TapQemu,id=network01" -S
> >> [New Thread 13304.0x1f00]
> >> [New Thread 13304.0x2f84]
> >> [New Thread 13304.0x3524]
> >> [New Thread 13304.0x2b8c]
> >> [New Thread 13304.0x368c]
> >> [New Thread 13304.0x3668]
> >> [New Thread 13304.0xf4c]
> >> [New Thread 13304.0x49c]
> >> [New Thread 13304.0x1d4c]
> >> [New Thread 13304.0x7fc]
> >> [Thread 13304.0x7fc exited with code 0]
> >> [New Thread 13304.0x357c]
> >> [New Thread 13304.0x7c0]
> >> [New Thread 13304.0x3564]
> >> [New Thread 13304.0x26f4]
> >> [New Thread 13304.0x2f68]
> >>
> >> Program received signal SIGSEGV, Segmentation fault.
> >> 0x7ffb9edea991 in ?? () from c:\qemu-master-msys2\libglib-2.0-0.dll
> >> (gdb) bt
> >> #0  0x7ffb9edea991 in ?? () from
> >> c:\qemu-master-msys2\libglib-2.0-0.dll
> >> #1  0x00080480bf50 in ?? ()
> >> Backtrace stopped: previous frame inner to this frame (corrupt stack?)
> >> (gdb)
> >>
> >> Even before I could attach to the process.
> >
> > If you run QEMU under gdb you don't have to attach to it but to get a
> > meaningful backtrace you should configure and compile QEMU with
> > --enable-debug (this will make it run slower so not recommended
> > normally but for debugging that would be needed). If the stack is
> > really corrupted then you may not get a useful backtrace or it may be
> > a problem with gdb on Windows. I've found that gdb on Windows works
> > for simple things but could give bad results for more complex stuff.
> > WinDbg may be better but it's harder to use (needs some registry
> > change I think to enable core dumps then you could open and analyze
> > core dumps with it or it should be able to run command directly but I
> > don't know how that works).
> >
> > Another idea: maybe you could check other threads in gdb. Not sure if
> > that would reveal anything but may worth a try. I think the commands
> > you need are "info threads" and "apply all bt" or something similar.
> >
> > Regards,
> > BALATON Zoltan
> >
>
> It looks to me the patch tires to recycle a temporary buffer to tap thread.
>
> Please try to attached fix to see it if works.

Yep, good catch, thanks! This patch looks correct to me.

Regards,
Bin

Re: [PATCH] linux-user: allow NULL msg in recvfrom

[Laurent Vivier]

Le 26/03/2021 à 05:05, Zach Reizner a écrit :
> The kernel allows a NULL msg in recvfrom so that he size of the next
> message may be queried before allocating a correctly sized buffer. This
> change allows the syscall translator to pass along the NULL msg pointer
> instead of returning early with EFAULT.
> 
> Signed-off-by: Zach Reizner 
> ---
>  linux-user/syscall.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 1e508576c7..332544b43c 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -3680,8 +3680,6 @@ static abi_long do_recvfrom(int fd, abi_ulong
> msg, size_t len, int flags,
>  abi_long ret;
> 
>  host_msg = lock_user(VERIFY_WRITE, msg, len, 0);
> -if (!host_msg)
> -return -TARGET_EFAULT;
>  if (target_addr) {
>  if (get_user_u32(addrlen, target_addrlen)) {
>  ret = -TARGET_EFAULT;
> 

Reviewed-by: Laurent Vivier 

Re: [RFC PATCH v2 0/3] virtio-net: graceful drop of vhost for TAP

[Yuri Benditovich]

On Fri, Mar 26, 2021 at 10:51 AM Jason Wang  wrote:
>
>
> 在 2021/3/25 下午5:00, Yuri Benditovich 写道:
> > Hi Jason,
> >
> > This was discussed earlier on the previous series of patches.
> > https://lists.gnu.org/archive/html/qemu-devel/2021-02/msg01829.html
> > There were strong objections from both Daniel and Michael and I feel
> > that the series was rejected.
> > There was Michael's claim:
> > "We did what this patch is trying to change for years now, in
> > particular KVM also seems to happily disable CPU features not supported
> > by kernel so I wonder why we can't keep doing it, with tweaks for some
> > corner cases."
>
>
> So for cpu feautres, it works since the management have other tool to
> the cpuid. Then management will make sure the migration happens amongs
> the hosts that is compatibile with the same cpuid sets.
>
> For vhost, we don't have such capabilities, that's why I think we need
> to have fallback.
>
Hi Jason,
What, from your POV was the result of v1 discussion?
IMO, there was one critical comment that the patch does not address
'forcevhost' properly (indeed).
IMO, there are many comments from Daniel and Michael that in the sum
say that this change is not what they would like.
If I'm mistaken please let me know.

I have no problem to send v3 = v1 + handling of ''forcevhost'
If this is what you want, please let me know also.

>
> > https://lists.gnu.org/archive/html/qemu-devel/2021-02/msg03187.html
> > And it was Michael's question:
> > "Can we limit the change to when a VM is migrated in?"
> > https://lists.gnu.org/archive/html/qemu-devel/2021-02/msg03163.html
> > So I'm trying to suggest another approach:
> > - In case of conflicting features (for example RSS and vhost) we in
> > qemu we do not have enough information to prefer one or another.
> > - If we drop to userspace in the first set_features we say: "vhost is
> > less important than other requested features"
> > - This series keeps backward compatibility, i.e. if you start with
> > vhost and some features are not available - they are silently cleared.
> > - But in case the features are available on source machine - they are used
> > - In case of migration this series says: "We prefer successful
> > migration even if for that we need to drop to userspace"
> > - On the migration back to the 1st system we again work with all the
> > features and with vhost as all the features are available.
>
>
> One issue for this approach is that. Consider we had two drivers:
>
> 1) Driver A that supports split only
> 2) Driver B that supports packed
>
> Consider src support packed but dest doesn't
>
> So switching driver A to driver B works without migration. But if we
> switch driver from A to B after migration it won't work?

I assume that  both src and dest started with vhost=on.

As driver B supports both packed and split, you can switch from driver
A to driver B after migration
and driver B will work with split. Exactly as it does today.

The key question is what is more important - vhost or features that
vhost does not support?
current code says: vhost is more important always
v1 patch says: features are more important always.
v2 patch says: vhost is more important at init time, features are more
important at migration time.
Because we are able to drop vhost but we can't drop features when we
have a running driver.
Do you agree?

>
> Thanks
>
>
> >
> > Thanks,
> > Yuri
> >
> >
> >
> > On Thu, Mar 25, 2021 at 8:59 AM Jason Wang  wrote:
> >>
> >> 在 2021/3/22 下午8:24, Yuri Benditovich 写道:
> >>> Allow fallback to userspace only upon migration, only for specific 
> >>> features
> >>> and only if 'vhostforce' is not requested.
> >>>
> >>> Changes from v1:
> >>> Patch 1 dropeed (will be submitted in another series)
> >>> Added device callback in case the migration should fail due to missing 
> >>> features
> >>
> >> Hi Yuri:
> >>
> >> Have a quick glance at the series. A questions is why we need to do the
> >> fallback only during load?
> >>
> >> I think we should do it in the device initializating. E.g when the vhost
> >> features can not satisfy, we should disable vhost since there.
> >>
> >> Thanks
> >>
> >>
> >>> Yuri Benditovich (3):
> >>> net: add ability to hide (disable) vhost_net
> >>> virtio: introduce 'missing_features_migrated' device callback
> >>> virtio-net: implement missing_features_migrated callback
> >>>
> >>>hw/net/vhost_net.c |  4 ++-
> >>>hw/net/virtio-net.c| 51 ++
> >>>hw/virtio/virtio.c |  8 ++
> >>>include/hw/virtio/virtio.h |  8 ++
> >>>include/net/net.h  |  1 +
> >>>5 files changed, 71 insertions(+), 1 deletion(-)
> >>>
>

Re: [RFC PATCH v2 0/3] virtio-net: graceful drop of vhost for TAP

[Jason Wang]

在 2021/3/26 下午5:09, Yuri Benditovich 写道:

On Fri, Mar 26, 2021 at 10:51 AM Jason Wang  wrote:


在 2021/3/25 下午5:00, Yuri Benditovich 写道:

Hi Jason,

This was discussed earlier on the previous series of patches.
https://lists.gnu.org/archive/html/qemu-devel/2021-02/msg01829.html
There were strong objections from both Daniel and Michael and I feel
that the series was rejected.
There was Michael's claim:
"We did what this patch is trying to change for years now, in
particular KVM also seems to happily disable CPU features not supported
by kernel so I wonder why we can't keep doing it, with tweaks for some
corner cases."


So for cpu feautres, it works since the management have other tool to
the cpuid. Then management will make sure the migration happens amongs
the hosts that is compatibile with the same cpuid sets.

For vhost, we don't have such capabilities, that's why I think we need
to have fallback.


Hi Jason,
What, from your POV was the result of v1 discussion?



It looks to me we don't have an agreement on that, sorry.



IMO, there was one critical comment that the patch does not address
'forcevhost' properly (indeed).
IMO, there are many comments from Daniel and Michael that in the sum
say that this change is not what they would like.
If I'm mistaken please let me know.



I think I will open a new thread and summarize the different approaches 
and then we can come a conclusion.





I have no problem to send v3 = v1 + handling of ''forcevhost'
If this is what you want, please let me know also.


https://lists.gnu.org/archive/html/qemu-devel/2021-02/msg03187.html
And it was Michael's question:
"Can we limit the change to when a VM is migrated in?"
https://lists.gnu.org/archive/html/qemu-devel/2021-02/msg03163.html
So I'm trying to suggest another approach:
- In case of conflicting features (for example RSS and vhost) we in
qemu we do not have enough information to prefer one or another.
- If we drop to userspace in the first set_features we say: "vhost is
less important than other requested features"
- This series keeps backward compatibility, i.e. if you start with
vhost and some features are not available - they are silently cleared.
- But in case the features are available on source machine - they are used
- In case of migration this series says: "We prefer successful
migration even if for that we need to drop to userspace"
- On the migration back to the 1st system we again work with all the
features and with vhost as all the features are available.


One issue for this approach is that. Consider we had two drivers:

1) Driver A that supports split only
2) Driver B that supports packed

Consider src support packed but dest doesn't

So switching driver A to driver B works without migration. But if we
switch driver from A to B after migration it won't work?

I assume that  both src and dest started with vhost=on.

As driver B supports both packed and split, you can switch from driver
A to driver B after migration
and driver B will work with split. Exactly as it does today.

The key question is what is more important - vhost or features that
vhost does not support?
current code says: vhost is more important always
v1 patch says: features are more important always.
v2 patch says: vhost is more important at init time, features are more
important at migration time.
Because we are able to drop vhost but we can't drop features when we
have a running driver.
Do you agree?



I think what came from cli is the most important. So if I understand 
correclty:


- vhost=on means "turn on vhost when possible" it implies that fallback 
is allowed (we had already had fallback codes)
- vhostforce=on means "turn on vhost unconditonally" it implies that we 
can't do fallback


So my understanding is that:

- "vhost=on, packed=on", we can fallback to userspace but must keep 
packed virtqueue works
- "vhost=on,vhostforce=on,packed=on", we can't fallback and must keep 
both vhost and packed virtqueue work, if we can't we need to fail


Thanks





Thanks



Thanks,
Yuri



On Thu, Mar 25, 2021 at 8:59 AM Jason Wang  wrote:

在 2021/3/22 下午8:24, Yuri Benditovich 写道:

Allow fallback to userspace only upon migration, only for specific features
and only if 'vhostforce' is not requested.

Changes from v1:
Patch 1 dropeed (will be submitted in another series)
Added device callback in case the migration should fail due to missing features

Hi Yuri:

Have a quick glance at the series. A questions is why we need to do the
fallback only during load?

I think we should do it in the device initializating. E.g when the vhost
features can not satisfy, we should disable vhost since there.

Thanks



Yuri Benditovich (3):
 net: add ability to hide (disable) vhost_net
 virtio: introduce 'missing_features_migrated' device callback
 virtio-net: implement missing_features_migrated callback

hw/net/vhost_net.c |  4 ++-
hw/net/virtio-net.c| 51 ++
hw/virtio/virtio.c

Re: [PATCH] Document qemu-img options data_file and data_file_raw

[Max Reitz]

On 01.03.21 18:28, Connor Kuehl wrote:

The contents of this patch were initially developed and posted by Han
Han[1], however, it appears the original patch was not applied. Since
then, the relevant documentation has been moved and adapted to a new
format.

I've taken most of the original wording and tweaked it according to
some of the feedback from the original patch submission. I've also
adapted it to restructured text, which is the format the documentation
currently uses.

[1] https://lists.nongnu.org/archive/html/qemu-block/2019-10/msg01253.html

Reported-by: Han Han 
Co-developed-by: Han Han 
Fixes: https://bugzilla.redhat.com/1763105
Signed-off-by: Connor Kuehl 
---
  docs/tools/qemu-img.rst | 12 
  1 file changed, 12 insertions(+)

diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst
index b615aa8419..5cc585dc27 100644
--- a/docs/tools/qemu-img.rst
+++ b/docs/tools/qemu-img.rst
@@ -866,6 +866,18 @@ Supported image file formats:
  issue ``lsattr filename`` to check if the NOCOW flag is set or not
  (Capital 'C' is NOCOW flag).
  
+  ``data_file``

+Pathname that refers to a file that will store all guest data. If
+this option is used, the qcow2 file will only contain the image's
+metadata.


I think I would like a note here about the fact that when passing this 
option to qemu-img create, the given data file will be newly created, 
i.e. if it already contains data, all that data will be lost.  And 
perhaps also note that qemu-img amend on the other hand will only change 
the reference in the qcow2 file, so the given file should already exist 
and will not be overwritten.


(“Pathname that refers to a file” sounds like the file may already exist 
before this operation, which may give people ideas.  (Not that the ideas 
were bad, it’s just that they have to take care.  Referencing qemu-img 
amend should give them a hint on how to do it right.))


Max


+
+  ``data_file_raw``
+If this option is set to ``on``, QEMU will always keep the external
+data file consistent as a standalone read-only raw image. The default
+value is ``off``.
+
+This option can only be enabled if ``data_file`` is set.
+
  ``Other``
  
QEMU also supports various other image file formats for

[PATCH v3 1/7] ui: add clipboard infrastructure

[Gerd Hoffmann]

Add some infrastructure to manage the clipboard in qemu.

TODO: Add API docs.

Signed-off-by: Gerd Hoffmann 
---
 include/ui/clipboard.h | 68 +++
 ui/clipboard.c | 92 ++
 ui/meson.build |  1 +
 3 files changed, 161 insertions(+)
 create mode 100644 include/ui/clipboard.h
 create mode 100644 ui/clipboard.c

diff --git a/include/ui/clipboard.h b/include/ui/clipboard.h
new file mode 100644
index ..00ff559425ee
--- /dev/null
+++ b/include/ui/clipboard.h
@@ -0,0 +1,68 @@
+#ifndef QEMU_CLIPBOARD_H
+#define QEMU_CLIPBOARD_H
+
+#include "qemu/notify.h"
+
+typedef enum QemuClipboardType QemuClipboardType;
+typedef enum QemuClipboardSelection QemuClipboardSelection;
+typedef struct QemuClipboardPeer QemuClipboardPeer;
+typedef struct QemuClipboardInfo QemuClipboardInfo;
+
+enum QemuClipboardType {
+QEMU_CLIPBOARD_TYPE_TEXT,  /* text/plain; charset=utf-8 */
+QEMU_CLIPBOARD_TYPE__COUNT,
+};
+
+/* same as VD_AGENT_CLIPBOARD_SELECTION_* */
+enum QemuClipboardSelection {
+QEMU_CLIPBOARD_SELECTION_CLIPBOARD,
+QEMU_CLIPBOARD_SELECTION_PRIMARY,
+QEMU_CLIPBOARD_SELECTION_SECONDARY,
+QEMU_CLIPBOARD_SELECTION__COUNT,
+};
+
+struct QemuClipboardPeer {
+const char *name;
+Notifier update;
+void (*request)(QemuClipboardInfo *info,
+QemuClipboardType type);
+};
+
+struct QemuClipboardInfo {
+uint32_t refcount;
+QemuClipboardPeer *owner;
+QemuClipboardSelection selection;
+struct {
+bool available;
+bool requested;
+size_t size;
+void *data;
+} types[QEMU_CLIPBOARD_TYPE__COUNT];
+};
+
+struct QemuClipboardData {
+uint32_t refcount;
+QemuClipboardInfo *info;
+QemuClipboardType type;
+};
+
+void qemu_clipboard_peer_register(QemuClipboardPeer *peer);
+void qemu_clipboard_peer_unregister(QemuClipboardPeer *peer);
+
+QemuClipboardInfo *qemu_clipboard_info_new(QemuClipboardPeer *owner,
+   QemuClipboardSelection selection);
+QemuClipboardInfo *qemu_clipboard_info_get(QemuClipboardInfo *info);
+void qemu_clipboard_info_put(QemuClipboardInfo *info);
+
+void qemu_clipboard_update(QemuClipboardInfo *info);
+void qemu_clipboard_request(QemuClipboardInfo *info,
+QemuClipboardType type);
+
+void qemu_clipboard_set_data(QemuClipboardPeer *peer,
+ QemuClipboardInfo *info,
+ QemuClipboardType type,
+ uint32_t size,
+ void *data,
+ bool update);
+
+#endif /* QEMU_CLIPBOARD_H */
diff --git a/ui/clipboard.c b/ui/clipboard.c
new file mode 100644
index ..556531c578a1
--- /dev/null
+++ b/ui/clipboard.c
@@ -0,0 +1,92 @@
+#include "qemu/osdep.h"
+#include "ui/clipboard.h"
+
+static NotifierList clipboard_notifiers =
+NOTIFIER_LIST_INITIALIZER(clipboard_notifiers);
+
+void qemu_clipboard_peer_register(QemuClipboardPeer *peer)
+{
+notifier_list_add(&clipboard_notifiers, &peer->update);
+}
+
+void qemu_clipboard_peer_unregister(QemuClipboardPeer *peer)
+{
+notifier_remove(&peer->update);
+}
+
+void qemu_clipboard_update(QemuClipboardInfo *info)
+{
+notifier_list_notify(&clipboard_notifiers, info);
+}
+
+QemuClipboardInfo *qemu_clipboard_info_new(QemuClipboardPeer *owner,
+   QemuClipboardSelection selection)
+{
+QemuClipboardInfo *info = g_new0(QemuClipboardInfo, 1);
+
+info->owner = owner;
+info->selection = selection;
+info->refcount = 1;
+
+return info;
+}
+
+QemuClipboardInfo *qemu_clipboard_info_get(QemuClipboardInfo *info)
+{
+info->refcount++;
+return info;
+}
+
+void qemu_clipboard_info_put(QemuClipboardInfo *info)
+{
+uint32_t type;
+
+if (!info) {
+return;
+}
+
+info->refcount--;
+if (info->refcount > 0) {
+return;
+}
+
+for (type = 0; type < QEMU_CLIPBOARD_TYPE__COUNT; type++) {
+g_free(info->types[type].data);
+}
+g_free(info);
+}
+
+void qemu_clipboard_request(QemuClipboardInfo *info,
+QemuClipboardType type)
+{
+if (info->types[type].data ||
+info->types[type].requested ||
+!info->types[type].available ||
+!info->owner)
+return;
+
+info->types[type].requested = true;
+info->owner->request(info, type);
+}
+
+void qemu_clipboard_set_data(QemuClipboardPeer *peer,
+ QemuClipboardInfo *info,
+ QemuClipboardType type,
+ uint32_t size,
+ void *data,
+ bool update)
+{
+if (!info ||
+info->owner != peer) {
+return;
+}
+
+g_free(info->types[type].data);
+info->types[type].data = g_memdup(data, size);
+info->types[type].size = size;
+info-

[PATCH v3 2/7] ui/vdagent: core infrastructure

[Gerd Hoffmann]

The vdagent protocol allows the guest agent (spice-vdagent) and the
spice client exchange messages to implement features which require
guest cooperation, for example clipboard support.

This is a qemu implementation of the spice client side.  This allows
the spice guest agent talk to qemu directly when not using the spice
protocol.

usage: qemu \
  -chardev vdagent,id=vdagent \
  -device virtserialport,chardev=vdagent,name=com.redhat.spice.0

This patch adds just the protocol basics: initial handshake and
capability negotiation.

Signed-off-by: Gerd Hoffmann 
---
 ui/vdagent.c| 314 
 qapi/char.json  |  13 ++
 ui/meson.build  |   1 +
 ui/trace-events |   8 ++
 4 files changed, 336 insertions(+)
 create mode 100644 ui/vdagent.c

diff --git a/ui/vdagent.c b/ui/vdagent.c
new file mode 100644
index ..98d1a2ee3415
--- /dev/null
+++ b/ui/vdagent.c
@@ -0,0 +1,314 @@
+#include "qemu/osdep.h"
+#include "include/qemu-common.h"
+#include "chardev/char.h"
+#include "trace.h"
+
+#include "qapi/qapi-types-char.h"
+
+#include "spice/vd_agent.h"
+
+struct VDAgentChardev {
+Chardev parent;
+
+/* guest vdagent */
+uint32_t caps;
+VDIChunkHeader chunk;
+uint32_t chunksize;
+uint8_t *msgbuf;
+uint32_t msgsize;
+uint8_t *xbuf;
+uint32_t xoff, xsize;
+};
+typedef struct VDAgentChardev VDAgentChardev;
+
+#define TYPE_CHARDEV_VDAGENT "chardev-vdagent"
+
+DECLARE_INSTANCE_CHECKER(VDAgentChardev, VDAGENT_CHARDEV,
+ TYPE_CHARDEV_VDAGENT);
+
+/* -- */
+/* names, for debug logging   */
+
+static const char *cap_name[] = {
+[VD_AGENT_CAP_MOUSE_STATE]= "mouse-state",
+[VD_AGENT_CAP_MONITORS_CONFIG]= "monitors-config",
+[VD_AGENT_CAP_REPLY]  = "reply",
+[VD_AGENT_CAP_CLIPBOARD]  = "clipboard",
+[VD_AGENT_CAP_DISPLAY_CONFIG] = "display-config",
+[VD_AGENT_CAP_CLIPBOARD_BY_DEMAND]= "clipboard-by-demand",
+[VD_AGENT_CAP_CLIPBOARD_SELECTION]= "clipboard-selection",
+[VD_AGENT_CAP_SPARSE_MONITORS_CONFIG] = "sparse-monitors-config",
+[VD_AGENT_CAP_GUEST_LINEEND_LF]   = "guest-lineend-lf",
+[VD_AGENT_CAP_GUEST_LINEEND_CRLF] = "guest-lineend-crlf",
+[VD_AGENT_CAP_MAX_CLIPBOARD]  = "max-clipboard",
+[VD_AGENT_CAP_AUDIO_VOLUME_SYNC]  = "audio-volume-sync",
+[VD_AGENT_CAP_MONITORS_CONFIG_POSITION]   = "monitors-config-position",
+[VD_AGENT_CAP_FILE_XFER_DISABLED] = "file-xfer-disabled",
+[VD_AGENT_CAP_FILE_XFER_DETAILED_ERRORS]  = 
"file-xfer-detailed-errors",
+#if 0
+[VD_AGENT_CAP_GRAPHICS_DEVICE_INFO]   = "graphics-device-info",
+[VD_AGENT_CAP_CLIPBOARD_NO_RELEASE_ON_REGRAB] = 
"clipboard-no-release-on-regrab",
+[VD_AGENT_CAP_CLIPBOARD_GRAB_SERIAL]  = "clipboard-grab-serial",
+#endif
+};
+
+static const char *msg_name[] = {
+[VD_AGENT_MOUSE_STATE]   = "mouse-state",
+[VD_AGENT_MONITORS_CONFIG]   = "monitors-config",
+[VD_AGENT_REPLY] = "reply",
+[VD_AGENT_CLIPBOARD] = "clipboard",
+[VD_AGENT_DISPLAY_CONFIG]= "display-config",
+[VD_AGENT_ANNOUNCE_CAPABILITIES] = "announce-capabilities",
+[VD_AGENT_CLIPBOARD_GRAB]= "clipboard-grab",
+[VD_AGENT_CLIPBOARD_REQUEST] = "clipboard-request",
+[VD_AGENT_CLIPBOARD_RELEASE] = "clipboard-release",
+[VD_AGENT_FILE_XFER_START]   = "file-xfer-start",
+[VD_AGENT_FILE_XFER_STATUS]  = "file-xfer-status",
+[VD_AGENT_FILE_XFER_DATA]= "file-xfer-data",
+[VD_AGENT_CLIENT_DISCONNECTED]   = "client-disconnected",
+[VD_AGENT_MAX_CLIPBOARD] = "max-clipboard",
+[VD_AGENT_AUDIO_VOLUME_SYNC] = "audio-volume-sync",
+#if 0
+[VD_AGENT_GRAPHICS_DEVICE_INFO]  = "graphics-device-info",
+#endif
+};
+
+#define GET_NAME(_m, _v) \
+(((_v) < ARRAY_SIZE(_m) && (_m[_v])) ? (_m[_v]) : "???")
+
+/* -- */
+/* send messages  */
+
+static void vdagent_send_buf(VDAgentChardev *vd, void *ptr, uint32_t msgsize)
+{
+uint8_t *msgbuf = ptr;
+uint32_t len, pos = 0;
+
+while (pos < msgsize) {
+len = qemu_chr_be_can_write(CHARDEV(vd));
+if (len > msgsize - pos) {
+len = msgsize - pos;
+}
+qemu_chr_be_write(CHARDEV(vd), msgbuf + pos, len);
+pos += len;
+}
+}
+
+static void vdagent_send_msg(VDAgentChardev *vd, VDAgentMessage *msg)
+{
+uint8_t *msgbuf = (void *)msg;
+uint32_t msgsize = sizeof(VDAgentMessage) + msg->size;
+uint32_t msgoff = 0;
+VDIChunkHeader chunk;
+
+trace_vdagent_send(GET_NA

Re: [PATCH for-6.0 1/4] include/hw/boards.h: Document machine_class_allow_dynamic_sysbus_dev()

[Auger Eric]

Hi Peter,

On 3/25/21 4:33 PM, Peter Maydell wrote:
> The function machine_class_allow_dynamic_sysbus_dev() is currently
> undocumented; add a doc comment.
> 
> Signed-off-by: Peter Maydell 
> ---
>  include/hw/boards.h | 14 ++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 4a90549ad85..27106abc11d 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -36,7 +36,21 @@ void machine_set_cpu_numa_node(MachineState *machine,
> const CpuInstanceProperties *props,
> Error **errp);
>  
> +/**
> + * machine_class_allow_dynamic_sysbus_dev: Add type to list of valid devices
nit: s/of valid devices/of dynamically instantiable sysbus devices ?
> + * @mc: Machine class
> + * @type: type to whitelist (should be a subtype of TYPE_SYS_BUS_DEVICE)
> + *
> + * Add the QOM type @type to the list of devices of which are subtypes
> + * of TYPE_SYS_BUS_DEVICE but which are still permitted to be dynamically
> + * created (eg by the user on the command line with -device).
> + * By default if the user tries to create any devices on the command line
> + * that are subtypes of TYPE_SYS_BUS_DEVICE they will get an error message;
> + * for the special cases which are permitted for this machine model, the
> + * machine model class init code must call this function to whitelist them.
> + */
>  void machine_class_allow_dynamic_sysbus_dev(MachineClass *mc, const char 
> *type);
> +
>  /*
>   * Checks that backend isn't used, preps it for exclusive usage and
>   * returns migratable MemoryRegion provided by backend.
> 
Besides

Reviewed-by: Eric Auger 

Thanks

Eric

[PATCH v3 0/7] ui: add vdagent implementation and clipboard support.

[Gerd Hoffmann]

Fist sketch of cut+paste support for vnc.  On the guest side we are
going to reuse the spice vdagent, so things should work out-of-the-box
with guests in the wild.  So this patch set brings a qemu implemenation
of the vdagent protocol.

Beside that there is the clipboard infrastructure of course.  For now
only text support is there.  The design allows adding more data types,
so we can add image support and maybe more later on.  So far vdagent,
vnc server and gtk ui are hooked up.

Usage: qemu \
  -chardev vdagent,id=vdagent,clipboard=on \
  -device virtio-serial-pci \
  -device virtserialport,chardev=vdagent,name=com.redhat.spice.0

v2:
 - add a bunch of sanity checks.
 - add proper chunking.
 - use autofree.
v3:
 - support agents without VD_AGENT_CAP_CLIPBOARD_SELECTION.
 - properly parse chunked messages.
 - test with windows guests, minor fixes.
 - set display_id for agent mouse events.

Gerd Hoffmann (7):
  ui: add clipboard infrastructure
  ui/vdagent: core infrastructure
  ui/vdagent: add mouse support
  ui/vdagent: add clipboard support
  ui/vnc: clipboard support
  ui/gtk: move struct GtkDisplayState to ui/gtk.h
  ui/gtk: add clipboard support

 include/ui/clipboard.h |  68 
 include/ui/gtk.h   |  67 
 ui/vnc.h   |  24 ++
 chardev/char.c |   6 +
 ui/clipboard.c |  92 +
 ui/gtk-clipboard.c | 192 +++
 ui/gtk.c   |  56 +--
 ui/vdagent.c   | 756 +
 ui/vnc-clipboard.c | 323 ++
 ui/vnc.c   |  20 +-
 qapi/char.json |  17 +
 ui/meson.build |   5 +-
 ui/trace-events|  10 +
 13 files changed, 1574 insertions(+), 62 deletions(-)
 create mode 100644 include/ui/clipboard.h
 create mode 100644 ui/clipboard.c
 create mode 100644 ui/gtk-clipboard.c
 create mode 100644 ui/vdagent.c
 create mode 100644 ui/vnc-clipboard.c

-- 
2.30.2

[PATCH v3 5/7] ui/vnc: clipboard support

[Gerd Hoffmann]

This patch adds support for cut+paste to the qemu vnc server, which
allows the vnc client exchange clipbaord data with qemu and other peers
like the qemu vdagent implementation.

Signed-off-by: Gerd Hoffmann 
---
 ui/vnc.h   |  24 
 ui/vnc-clipboard.c | 323 +
 ui/vnc.c   |  20 ++-
 ui/meson.build |   1 +
 4 files changed, 362 insertions(+), 6 deletions(-)
 create mode 100644 ui/vnc-clipboard.c

diff --git a/ui/vnc.h b/ui/vnc.h
index d4f3e1555809..a7149831f906 100644
--- a/ui/vnc.h
+++ b/ui/vnc.h
@@ -29,6 +29,7 @@
 
 #include "qemu/queue.h"
 #include "qemu/thread.h"
+#include "ui/clipboard.h"
 #include "ui/console.h"
 #include "audio/audio.h"
 #include "qemu/bitmap.h"
@@ -348,6 +349,10 @@ struct VncState
 
 Notifier mouse_mode_notifier;
 
+QemuClipboardPeer cbpeer;
+QemuClipboardInfo *cbinfo;
+uint32_t cbpending;
+
 QTAILQ_ENTRY(VncState) next;
 };
 
@@ -417,6 +422,7 @@ enum {
 #define VNC_ENCODING_XVP  0XFECB /* -309 */
 #define VNC_ENCODING_ALPHA_CURSOR 0XFEC6 /* -314 */
 #define VNC_ENCODING_WMVi 0x574D5669
+#define VNC_ENCODING_CLIPBOARD_EXT0xc0a1e5ce
 
 /*
  *
@@ -458,6 +464,7 @@ enum VncFeatures {
 VNC_FEATURE_ZYWRLE,
 VNC_FEATURE_LED_STATE,
 VNC_FEATURE_XVP,
+VNC_FEATURE_CLIPBOARD_EXT,
 };
 
 #define VNC_FEATURE_RESIZE_MASK  (1 << VNC_FEATURE_RESIZE)
@@ -474,6 +481,7 @@ enum VncFeatures {
 #define VNC_FEATURE_ZYWRLE_MASK  (1 << VNC_FEATURE_ZYWRLE)
 #define VNC_FEATURE_LED_STATE_MASK   (1 << VNC_FEATURE_LED_STATE)
 #define VNC_FEATURE_XVP_MASK (1 << VNC_FEATURE_XVP)
+#define VNC_FEATURE_CLIPBOARD_EXT_MASK   (1 <<  VNC_FEATURE_CLIPBOARD_EXT)
 
 
 /* Client -> Server message IDs */
@@ -535,6 +543,17 @@ enum VncFeatures {
 #define VNC_XVP_ACTION_REBOOT 3
 #define VNC_XVP_ACTION_RESET 4
 
+/* extended clipboard flags  */
+#define VNC_CLIPBOARD_TEXT (1 << 0)
+#define VNC_CLIPBOARD_RTF  (1 << 1)
+#define VNC_CLIPBOARD_HTML (1 << 2)
+#define VNC_CLIPBOARD_DIB  (1 << 3)
+#define VNC_CLIPBOARD_FILES(1 << 4)
+#define VNC_CLIPBOARD_CAPS (1 << 24)
+#define VNC_CLIPBOARD_REQUEST  (1 << 25)
+#define VNC_CLIPBOARD_PEEK (1 << 26)
+#define VNC_CLIPBOARD_NOTIFY   (1 << 27)
+#define VNC_CLIPBOARD_PROVIDE  (1 << 28)
 
 /*
  *
@@ -618,4 +637,9 @@ int vnc_zrle_send_framebuffer_update(VncState *vs, int x, 
int y, int w, int h);
 int vnc_zywrle_send_framebuffer_update(VncState *vs, int x, int y, int w, int 
h);
 void vnc_zrle_clear(VncState *vs);
 
+/* vnc-clipboard.c */
+void vnc_server_cut_text_caps(VncState *vs);
+void vnc_client_cut_text(VncState *vs, size_t len, uint8_t *text);
+void vnc_client_cut_text_ext(VncState *vs, int32_t len, uint32_t flags, 
uint8_t *data);
+
 #endif /* QEMU_VNC_H */
diff --git a/ui/vnc-clipboard.c b/ui/vnc-clipboard.c
new file mode 100644
index ..10ada8004f38
--- /dev/null
+++ b/ui/vnc-clipboard.c
@@ -0,0 +1,323 @@
+/*
+ * QEMU VNC display driver -- clipboard support
+ *
+ * Copyright (C) 2021 Gerd Hoffmann 
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to 
deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu-common.h"
+#include "vnc.h"
+#include "vnc-jobs.h"
+
+static uint8_t *inflate_buffer(uint8_t *in, uint32_t in_len, uint32_t *size)
+{
+z_stream stream = {
+.next_in  = in,
+.avail_in = in_len,
+.zalloc   = Z_NULL,
+.zfree= Z_NULL,
+};
+uint32_t out_len = 8;
+uint8_t *out = g_malloc(out_len);
+int ret;
+
+stream.next_out = out + stream.total_out;
+stream.avail_out = out_len - stream.total_out;
+
+ret = inflateInit(&stream);
+if (ret != Z_OK) {
+goto err;
+}
+
+while (stream.avail_in) {
+

[PATCH v3 6/7] ui/gtk: move struct GtkDisplayState to ui/gtk.h

[Gerd Hoffmann]

Want place gtk clipboard code in a separate C file, which in turn
requires GtkDisplayState being in a header file.  So move it.  No
functional change.

Signed-off-by: Gerd Hoffmann 
---
 include/ui/gtk.h | 57 
 ui/gtk.c | 55 --
 2 files changed, 57 insertions(+), 55 deletions(-)

diff --git a/include/ui/gtk.h b/include/ui/gtk.h
index 5ae0ad60a600..6e751794043f 100644
--- a/include/ui/gtk.h
+++ b/include/ui/gtk.h
@@ -18,12 +18,15 @@
 #include 
 #endif
 
+#include "ui/console.h"
 #include "ui/kbd-state.h"
 #if defined(CONFIG_OPENGL)
 #include "ui/egl-helpers.h"
 #include "ui/egl-context.h"
 #endif
 
+#define MAX_VCS 10
+
 typedef struct GtkDisplayState GtkDisplayState;
 
 typedef struct VirtualGfxConsole {
@@ -83,6 +86,60 @@ typedef struct VirtualConsole {
 };
 } VirtualConsole;
 
+struct GtkDisplayState {
+GtkWidget *window;
+
+GtkWidget *menu_bar;
+
+GtkAccelGroup *accel_group;
+
+GtkWidget *machine_menu_item;
+GtkWidget *machine_menu;
+GtkWidget *pause_item;
+GtkWidget *reset_item;
+GtkWidget *powerdown_item;
+GtkWidget *quit_item;
+
+GtkWidget *view_menu_item;
+GtkWidget *view_menu;
+GtkWidget *full_screen_item;
+GtkWidget *copy_item;
+GtkWidget *zoom_in_item;
+GtkWidget *zoom_out_item;
+GtkWidget *zoom_fixed_item;
+GtkWidget *zoom_fit_item;
+GtkWidget *grab_item;
+GtkWidget *grab_on_hover_item;
+
+int nb_vcs;
+VirtualConsole vc[MAX_VCS];
+
+GtkWidget *show_tabs_item;
+GtkWidget *untabify_item;
+GtkWidget *show_menubar_item;
+
+GtkWidget *vbox;
+GtkWidget *notebook;
+int button_mask;
+gboolean last_set;
+int last_x;
+int last_y;
+int grab_x_root;
+int grab_y_root;
+VirtualConsole *kbd_owner;
+VirtualConsole *ptr_owner;
+
+gboolean full_screen;
+
+GdkCursor *null_cursor;
+Notifier mouse_mode_notifier;
+gboolean free_scale;
+
+bool external_pause_update;
+
+DisplayOptions *opts;
+};
+
 extern bool gtk_use_gl_area;
 
 /* ui/gtk.c */
diff --git a/ui/gtk.c b/ui/gtk.c
index 1ea12535284a..7da288a25156 100644
--- a/ui/gtk.c
+++ b/ui/gtk.c
@@ -60,7 +60,6 @@
 #include "chardev/char.h"
 #include "qom/object.h"
 
-#define MAX_VCS 10
 #define VC_WINDOW_X_MIN  320
 #define VC_WINDOW_Y_MIN  240
 #define VC_TERM_X_MIN 80
@@ -119,60 +118,6 @@
 static const guint16 *keycode_map;
 static size_t keycode_maplen;
 
-struct GtkDisplayState {
-GtkWidget *window;
-
-GtkWidget *menu_bar;
-
-GtkAccelGroup *accel_group;
-
-GtkWidget *machine_menu_item;
-GtkWidget *machine_menu;
-GtkWidget *pause_item;
-GtkWidget *reset_item;
-GtkWidget *powerdown_item;
-GtkWidget *quit_item;
-
-GtkWidget *view_menu_item;
-GtkWidget *view_menu;
-GtkWidget *full_screen_item;
-GtkWidget *copy_item;
-GtkWidget *zoom_in_item;
-GtkWidget *zoom_out_item;
-GtkWidget *zoom_fixed_item;
-GtkWidget *zoom_fit_item;
-GtkWidget *grab_item;
-GtkWidget *grab_on_hover_item;
-
-int nb_vcs;
-VirtualConsole vc[MAX_VCS];
-
-GtkWidget *show_tabs_item;
-GtkWidget *untabify_item;
-GtkWidget *show_menubar_item;
-
-GtkWidget *vbox;
-GtkWidget *notebook;
-int button_mask;
-gboolean last_set;
-int last_x;
-int last_y;
-int grab_x_root;
-int grab_y_root;
-VirtualConsole *kbd_owner;
-VirtualConsole *ptr_owner;
-
-gboolean full_screen;
-
-GdkCursor *null_cursor;
-Notifier mouse_mode_notifier;
-gboolean free_scale;
-
-bool external_pause_update;
-
-DisplayOptions *opts;
-};
-
 struct VCChardev {
 Chardev parent;
 VirtualConsole *console;
-- 
2.30.2

[PATCH v3 4/7] ui/vdagent: add clipboard support

[Gerd Hoffmann]

This patch adds support for clipboard messages to the qemu vdagent
implementation, which allows the guest exchange clipboard data with
qemu.  Clipboard support can be enabled/disabled using the new
'clipboard' parameter for the vdagent chardev.  Default is off.

Signed-off-by: Gerd Hoffmann 
---
 chardev/char.c  |   3 +
 ui/vdagent.c| 292 
 qapi/char.json  |   4 +-
 ui/trace-events |   2 +
 4 files changed, 300 insertions(+), 1 deletion(-)

diff --git a/chardev/char.c b/chardev/char.c
index aa15e6cbdd27..14b38df61eb4 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -933,6 +933,9 @@ QemuOptsList qemu_chardev_opts = {
 },{
 .name = "mouse",
 .type = QEMU_OPT_BOOL,
+},{
+.name = "clipboard",
+.type = QEMU_OPT_BOOL,
 #ifdef CONFIG_LINUX
 },{
 .name = "tight",
diff --git a/ui/vdagent.c b/ui/vdagent.c
index e914a33bae20..67044aa5b9f0 100644
--- a/ui/vdagent.c
+++ b/ui/vdagent.c
@@ -3,6 +3,7 @@
 #include "chardev/char.h"
 #include "hw/qdev-core.h"
 #include "qemu/option.h"
+#include "ui/clipboard.h"
 #include "ui/console.h"
 #include "ui/input.h"
 #include "trace.h"
@@ -13,12 +14,14 @@
 #include "spice/vd_agent.h"
 
 #define VDAGENT_MOUSE_DEFAULT true
+#define VDAGENT_CLIPBOARD_DEFAULT false
 
 struct VDAgentChardev {
 Chardev parent;
 
 /* config */
 bool mouse;
+bool clipboard;
 
 /* guest vdagent */
 uint32_t caps;
@@ -36,6 +39,11 @@ struct VDAgentChardev {
 uint32_t mouse_btn;
 uint32_t mouse_display;
 QemuInputHandlerState *mouse_hs;
+
+/* clipboard */
+QemuClipboardPeer cbpeer;
+QemuClipboardInfo *cbinfo[QEMU_CLIPBOARD_SELECTION__COUNT];
+uint32_t cbpending[QEMU_CLIPBOARD_SELECTION__COUNT];
 };
 typedef struct VDAgentChardev VDAgentChardev;
 
@@ -91,6 +99,24 @@ static const char *msg_name[] = {
 #endif
 };
 
+static const char *sel_name[] = {
+[VD_AGENT_CLIPBOARD_SELECTION_CLIPBOARD] = "clipboard",
+[VD_AGENT_CLIPBOARD_SELECTION_PRIMARY]   = "primary",
+[VD_AGENT_CLIPBOARD_SELECTION_SECONDARY] = "secondary",
+};
+
+static const char *type_name[] = {
+[VD_AGENT_CLIPBOARD_NONE]   = "none",
+[VD_AGENT_CLIPBOARD_UTF8_TEXT]  = "text",
+[VD_AGENT_CLIPBOARD_IMAGE_PNG]  = "png",
+[VD_AGENT_CLIPBOARD_IMAGE_BMP]  = "bmp",
+[VD_AGENT_CLIPBOARD_IMAGE_TIFF] = "tiff",
+[VD_AGENT_CLIPBOARD_IMAGE_JPG]  = "jpg",
+#if 0
+[VD_AGENT_CLIPBOARD_FILE_LIST]  = "files",
+#endif
+};
+
 #define GET_NAME(_m, _v) \
 (((_v) < ARRAY_SIZE(_m) && (_m[_v])) ? (_m[_v]) : "???")
 
@@ -147,6 +173,10 @@ static void vdagent_send_caps(VDAgentChardev *vd)
 if (vd->mouse) {
 caps->caps[0] |= (1 << VD_AGENT_CAP_MOUSE_STATE);
 }
+if (vd->clipboard) {
+caps->caps[0] |= (1 << VD_AGENT_CAP_CLIPBOARD_BY_DEMAND);
+caps->caps[0] |= (1 << VD_AGENT_CAP_CLIPBOARD_SELECTION);
+}
 
 vdagent_send_msg(vd, msg);
 }
@@ -247,6 +277,243 @@ static QemuInputHandler vdagent_mouse_handler = {
 .sync  = vdagent_pointer_sync,
 };
 
+/* -- */
+/* clipboard  */
+
+static bool have_clipboard(VDAgentChardev *vd)
+{
+return vd->clipboard &&
+(vd->caps & (1 << VD_AGENT_CAP_CLIPBOARD_BY_DEMAND));
+}
+
+static bool have_selection(VDAgentChardev *vd)
+{
+return vd->caps & (1 << VD_AGENT_CAP_CLIPBOARD_SELECTION);
+}
+
+static uint32_t type_qemu_to_vdagent(enum QemuClipboardType type)
+{
+switch (type) {
+case QEMU_CLIPBOARD_TYPE_TEXT:
+return VD_AGENT_CLIPBOARD_UTF8_TEXT;
+default:
+return VD_AGENT_CLIPBOARD_NONE;
+}
+}
+
+static void vdagent_send_clipboard_grab(VDAgentChardev *vd,
+QemuClipboardInfo *info)
+{
+g_autofree VDAgentMessage *msg = g_malloc0(sizeof(VDAgentMessage) +
+   sizeof(uint32_t) * 
(QEMU_CLIPBOARD_TYPE__COUNT + 1));
+uint8_t *s = msg->data;
+uint32_t *data = (uint32_t *)msg->data;
+uint32_t q, type;
+
+if (have_selection(vd)) {
+*s = info->selection;
+data++;
+msg->size += sizeof(uint32_t);
+} else if (info->selection != QEMU_CLIPBOARD_SELECTION_CLIPBOARD) {
+return;
+}
+
+for (q = 0; q < QEMU_CLIPBOARD_TYPE__COUNT; q++) {
+type = type_qemu_to_vdagent(q);
+if (type != VD_AGENT_CLIPBOARD_NONE && info->types[q].available) {
+*data = type;
+data++;
+msg->size += sizeof(uint32_t);
+}
+}
+
+msg->type = VD_AGENT_CLIPBOARD_GRAB;
+vdagent_send_msg(vd, msg);
+}
+
+static void vdagent_send_clipboard_data(VDAgentChardev *vd,
+QemuClipboardInfo *info,
+QemuClipboardType type)
+{
+g_autofree VDAgentMessage *msg = g_malloc0(sizeof(V

[PATCH v3 7/7] ui/gtk: add clipboard support

[Gerd Hoffmann]

This patch adds clipboard support to the qemu gtk ui.

Signed-off-by: Gerd Hoffmann 
---
 include/ui/gtk.h   |  10 +++
 ui/gtk-clipboard.c | 192 +
 ui/gtk.c   |   1 +
 ui/meson.build |   2 +-
 4 files changed, 204 insertions(+), 1 deletion(-)
 create mode 100644 ui/gtk-clipboard.c

diff --git a/include/ui/gtk.h b/include/ui/gtk.h
index 6e751794043f..9516670ebc87 100644
--- a/include/ui/gtk.h
+++ b/include/ui/gtk.h
@@ -18,6 +18,7 @@
 #include 
 #endif
 
+#include "ui/clipboard.h"
 #include "ui/console.h"
 #include "ui/kbd-state.h"
 #if defined(CONFIG_OPENGL)
@@ -137,6 +138,12 @@ struct GtkDisplayState {
 
 bool external_pause_update;
 
+QemuClipboardPeer cbpeer;
+QemuClipboardInfo *cbinfo[QEMU_CLIPBOARD_SELECTION__COUNT];
+uint32_t cbpending[QEMU_CLIPBOARD_SELECTION__COUNT];
+GtkClipboard *gtkcb[QEMU_CLIPBOARD_SELECTION__COUNT];
+bool cbowner[QEMU_CLIPBOARD_SELECTION__COUNT];
+
 DisplayOptions *opts;
 };
 
@@ -207,4 +214,7 @@ void gtk_gl_area_init(void);
 int gd_gl_area_make_current(DisplayChangeListener *dcl,
 QEMUGLContext ctx);
 
+/* gtk-clipboard.c */
+void gd_clipboard_init(GtkDisplayState *gd);
+
 #endif /* UI_GTK_H */
diff --git a/ui/gtk-clipboard.c b/ui/gtk-clipboard.c
new file mode 100644
index ..21f8b39e9c54
--- /dev/null
+++ b/ui/gtk-clipboard.c
@@ -0,0 +1,192 @@
+/*
+ * GTK UI -- clipboard support
+ *
+ * Copyright (C) 2021 Gerd Hoffmann 
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see .
+ *
+ */
+
+#include "qemu/osdep.h"
+#include "qemu-common.h"
+#include "qemu/main-loop.h"
+
+#include "ui/gtk.h"
+
+static QemuClipboardSelection gd_find_selection(GtkDisplayState *gd,
+GtkClipboard *clipboard)
+{
+QemuClipboardSelection s;
+
+for (s = 0; s < QEMU_CLIPBOARD_SELECTION__COUNT; s++) {
+if (gd->gtkcb[s] == clipboard) {
+return s;
+}
+}
+return QEMU_CLIPBOARD_SELECTION_CLIPBOARD;
+}
+
+static void gd_clipboard_get_data(GtkClipboard *clipboard,
+  GtkSelectionData *selection_data,
+  guint selection_info,
+  gpointer  data)
+{
+GtkDisplayState *gd = data;
+QemuClipboardSelection s = gd_find_selection(gd, clipboard);
+QemuClipboardType type = QEMU_CLIPBOARD_TYPE_TEXT;
+QemuClipboardInfo *info = qemu_clipboard_info_get(gd->cbinfo[s]);
+
+qemu_clipboard_request(info, type);
+while (info == gd->cbinfo[s] &&
+   info->types[type].available &&
+   info->types[type].data == NULL) {
+main_loop_wait(false);
+}
+
+if (info == gd->cbinfo[s] && gd->cbowner[s]) {
+gtk_selection_data_set_text(selection_data,
+info->types[type].data,
+info->types[type].size);
+} else {
+/* clipboard owner changed while waiting for the data */
+}
+
+qemu_clipboard_info_put(info);
+}
+
+static void gd_clipboard_clear(GtkClipboard *clipboard,
+   gpointer data)
+{
+GtkDisplayState *gd = data;
+QemuClipboardSelection s = gd_find_selection(gd, clipboard);
+
+gd->cbowner[s] = false;
+}
+
+static void gd_clipboard_notify(Notifier *notifier, void *data)
+{
+GtkDisplayState *gd = container_of(notifier, GtkDisplayState, 
cbpeer.update);
+QemuClipboardInfo *info = data;
+QemuClipboardSelection s = info->selection;
+bool self_update = info->owner == &gd->cbpeer;
+
+if (info != gd->cbinfo[s]) {
+qemu_clipboard_info_put(gd->cbinfo[s]);
+gd->cbinfo[s] = qemu_clipboard_info_get(info);
+gd->cbpending[s] = 0;
+if (!self_update) {
+GtkTargetList *list;
+GtkTargetEntry *targets;
+gint n_targets;
+
+list = gtk_target_list_new(NULL, 0);
+if (info->types[QEMU_CLIPBOARD_TYPE_TEXT].available) {
+gtk_target_list_add_text_targets(list, 0);
+}
+targets = gtk_target_table_new_from_list(list, &n_targets);
+
+gtk_clipboard_clear(gd->gtkcb[s]);
+gd->cbowner[s] = true;
+gtk_clipboard_set_with_data(gd->gtkcb[s],
+targets

[PATCH v3 3/7] ui/vdagent: add mouse support

[Gerd Hoffmann]

This patch adds support for mouse messages to the vdagent
implementation.  This can be enabled/disabled using the new
'mouse' parameter for the vdagent chardev.  Default is on.

Signed-off-by: Gerd Hoffmann 
---
 chardev/char.c |   3 +
 ui/vdagent.c   | 150 +
 qapi/char.json |   4 +-
 3 files changed, 156 insertions(+), 1 deletion(-)

diff --git a/chardev/char.c b/chardev/char.c
index 140d6d9d369a..aa15e6cbdd27 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -930,6 +930,9 @@ QemuOptsList qemu_chardev_opts = {
 },{
 .name = "logappend",
 .type = QEMU_OPT_BOOL,
+},{
+.name = "mouse",
+.type = QEMU_OPT_BOOL,
 #ifdef CONFIG_LINUX
 },{
 .name = "tight",
diff --git a/ui/vdagent.c b/ui/vdagent.c
index 98d1a2ee3415..e914a33bae20 100644
--- a/ui/vdagent.c
+++ b/ui/vdagent.c
@@ -1,15 +1,25 @@
 #include "qemu/osdep.h"
 #include "include/qemu-common.h"
 #include "chardev/char.h"
+#include "hw/qdev-core.h"
+#include "qemu/option.h"
+#include "ui/console.h"
+#include "ui/input.h"
 #include "trace.h"
 
 #include "qapi/qapi-types-char.h"
+#include "qapi/qapi-types-ui.h"
 
 #include "spice/vd_agent.h"
 
+#define VDAGENT_MOUSE_DEFAULT true
+
 struct VDAgentChardev {
 Chardev parent;
 
+/* config */
+bool mouse;
+
 /* guest vdagent */
 uint32_t caps;
 VDIChunkHeader chunk;
@@ -18,6 +28,14 @@ struct VDAgentChardev {
 uint32_t msgsize;
 uint8_t *xbuf;
 uint32_t xoff, xsize;
+
+/* mouse */
+DeviceState mouse_dev;
+uint32_t mouse_x;
+uint32_t mouse_y;
+uint32_t mouse_btn;
+uint32_t mouse_display;
+QemuInputHandlerState *mouse_hs;
 };
 typedef struct VDAgentChardev VDAgentChardev;
 
@@ -122,13 +140,113 @@ static void vdagent_send_caps(VDAgentChardev *vd)
 g_autofree VDAgentMessage *msg = g_malloc0(sizeof(VDAgentMessage) +

sizeof(VDAgentAnnounceCapabilities) +
sizeof(uint32_t));
+VDAgentAnnounceCapabilities *caps = (void *)msg->data;
 
 msg->type = VD_AGENT_ANNOUNCE_CAPABILITIES;
 msg->size = sizeof(VDAgentAnnounceCapabilities) + sizeof(uint32_t);
+if (vd->mouse) {
+caps->caps[0] |= (1 << VD_AGENT_CAP_MOUSE_STATE);
+}
 
 vdagent_send_msg(vd, msg);
 }
 
+/* -- */
+/* mouse events   */
+
+static bool have_mouse(VDAgentChardev *vd)
+{
+return vd->mouse &&
+(vd->caps & (1 << VD_AGENT_CAP_MOUSE_STATE));
+}
+
+static void vdagent_send_mouse(VDAgentChardev *vd)
+{
+g_autofree VDAgentMessage *msg = g_malloc0(sizeof(VDAgentMessage) +
+   sizeof(VDAgentMouseState));
+VDAgentMouseState *mouse = (void *)msg->data;
+
+msg->type = VD_AGENT_MOUSE_STATE;
+msg->size = sizeof(VDAgentMouseState);
+
+mouse->x  = vd->mouse_x;
+mouse->y  = vd->mouse_y;
+mouse->buttons= vd->mouse_btn;
+mouse->display_id = vd->mouse_display;
+
+vdagent_send_msg(vd, msg);
+}
+
+static void vdagent_pointer_event(DeviceState *dev, QemuConsole *src,
+  InputEvent *evt)
+{
+static const int bmap[INPUT_BUTTON__MAX] = {
+[INPUT_BUTTON_LEFT]= VD_AGENT_LBUTTON_MASK,
+[INPUT_BUTTON_RIGHT]   = VD_AGENT_RBUTTON_MASK,
+[INPUT_BUTTON_MIDDLE]  = VD_AGENT_MBUTTON_MASK,
+[INPUT_BUTTON_WHEEL_UP]= VD_AGENT_UBUTTON_MASK,
+[INPUT_BUTTON_WHEEL_DOWN]  = VD_AGENT_DBUTTON_MASK,
+#if 0
+[INPUT_BUTTON_SIDE]= VD_AGENT_SBUTTON_MASK,
+[INPUT_BUTTON_EXTRA]   = VD_AGENT_EBUTTON_MASK,
+#endif
+};
+
+VDAgentChardev *vd = container_of(dev, struct VDAgentChardev, mouse_dev);
+InputMoveEvent *move;
+InputBtnEvent *btn;
+uint32_t xres, yres;
+
+switch (evt->type) {
+case INPUT_EVENT_KIND_ABS:
+move = evt->u.abs.data;
+xres = qemu_console_get_width(src, 1024);
+yres = qemu_console_get_height(src, 768);
+if (move->axis == INPUT_AXIS_X) {
+vd->mouse_x = qemu_input_scale_axis(move->value,
+INPUT_EVENT_ABS_MIN,
+INPUT_EVENT_ABS_MAX,
+0, xres);
+} else if (move->axis == INPUT_AXIS_Y) {
+vd->mouse_y = qemu_input_scale_axis(move->value,
+INPUT_EVENT_ABS_MIN,
+INPUT_EVENT_ABS_MAX,
+0, yres);
+}
+vd->mouse_display = qemu_console_get_index(src);
+break;
+
+case INPUT_EVENT_KIND_BTN:
+btn = evt->u.btn.data;
+if (btn->down) {
+  

Re: [PATCH for-6.0 2/4] machine: Provide a function to check the dynamic sysbus whitelist

[Auger Eric]

Hi Peter,

On 3/25/21 4:33 PM, Peter Maydell wrote:
> Provide a new function dynamic_sysbus_dev_allowed() which checks
> the per-machine whitelist of dynamic sysbus devices and returns
> a boolean result indicating whether the device is whitelisted.
> We can use this in the implementation of validate_sysbus_device(),
> but we will also need it so that machine hotplug callbacks can
> validate devices rather than assuming that any sysbus device
> might be hotpluggable into the platform bus.
> 
> Signed-off-by: Peter Maydell 
Reviewed-by: Eric Auger 

Thanks

Eric

> ---
>  include/hw/boards.h | 24 
>  hw/core/machine.c   | 21 -
>  2 files changed, 40 insertions(+), 5 deletions(-)
> 
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 27106abc11d..609112a4e1a 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -51,6 +51,30 @@ void machine_set_cpu_numa_node(MachineState *machine,
>   */
>  void machine_class_allow_dynamic_sysbus_dev(MachineClass *mc, const char 
> *type);
>  
> +/**
> + * device_is_dynamic_sysbus: test whether device is a dynamic sysbus device
> + * @mc: Machine class
> + * @dev: device to check
> + *
> + * Returns: true if @dev is a sysbus device on the machine's whitelist
> + * of dynamically pluggable sysbus devices; otherwise false.
> + *
> + * This function checks whether @dev is a valid dynamic sysbus device,
> + * by first confirming that it is a sysbus device and then checking it
> + * against the whitelist of permitted dynamic sysbus devices which has
> + * been set up by the machine using machine_class_allow_dynamic_sysbus_dev().
> + *
> + * It is valid to call this with something that is not a subclass of
> + * TYPE_SYS_BUS_DEVICE; the function will return false in this case.
> + * This allows hotplug callback functions to be written as:
> + * if (device_is_dynamic_sysbus(mc, dev)) {
> + * handle dynamic sysbus case;
> + * } else if (some other kind of hotplug) {
> + * handle that;
> + * }
> + */
> +bool device_is_dynamic_sysbus(MachineClass *mc, DeviceState *dev);
> +
>  /*
>   * Checks that backend isn't used, preps it for exclusive usage and
>   * returns migratable MemoryRegion provided by backend.
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 9935c6ddd56..8d97094736a 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -529,20 +529,31 @@ void 
> machine_class_allow_dynamic_sysbus_dev(MachineClass *mc, const char *type)
>  QAPI_LIST_PREPEND(mc->allowed_dynamic_sysbus_devices, g_strdup(type));
>  }
>  
> -static void validate_sysbus_device(SysBusDevice *sbdev, void *opaque)
> +bool device_is_dynamic_sysbus(MachineClass *mc, DeviceState *dev)
>  {
> -MachineState *machine = opaque;
> -MachineClass *mc = MACHINE_GET_CLASS(machine);
>  bool allowed = false;
>  strList *wl;
> +Object *obj = OBJECT(dev);
> +
> +if (!object_dynamic_cast(obj, TYPE_SYS_BUS_DEVICE)) {
> +return false;
> +}
>  
>  for (wl = mc->allowed_dynamic_sysbus_devices;
>   !allowed && wl;
>   wl = wl->next) {
> -allowed |= !!object_dynamic_cast(OBJECT(sbdev), wl->value);
> +allowed |= !!object_dynamic_cast(obj, wl->value);
>  }
>  
> -if (!allowed) {
> +return allowed;
> +}
> +
> +static void validate_sysbus_device(SysBusDevice *sbdev, void *opaque)
> +{
> +MachineState *machine = opaque;
> +MachineClass *mc = MACHINE_GET_CLASS(machine);
> +
> +if (!device_is_dynamic_sysbus(mc, DEVICE(sbdev))) {
>  error_report("Option '-device %s' cannot be handled by this machine",
>   object_class_get_name(object_get_class(OBJECT(sbdev;
>  exit(1);
> 

Re: [PATCH for-6.0 3/4] hw/arm/virt: Only try to add valid dynamic sysbus devices to platform bus

[Auger Eric]

Hi Peter,

On 3/25/21 4:33 PM, Peter Maydell wrote:
> The virt machine device plug callback currently calls
> platform_bus_link_device() for any sysbus device.  This is overly
> broad, because platform_bus_link_device() will unconditionally grab
> the IRQs and MMIOs of the device it is passed, whether it was
> intended for the platform bus or not.  Restrict hotpluggability of
> sysbus devices to only those devices on the dynamic sysbus whitelist.
> 
> We were mostly getting away with this because the board creates the
> platform bus as the last device it creates, and so the hotplug
> callback did not do anything for all the sysbus devices created by
> the board itself.  However if the user plugged in a device which
> itself uses a sysbus device internally we would have mishandled this
> and probably asserted.
> 
> Signed-off-by: Peter Maydell 
Reviewed-by: Eric Auger 

Thanks

Eric
> ---
>  hw/arm/virt.c | 8 ++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index aa2bbd14e09..8625152a735 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -2443,7 +2443,9 @@ static void virt_machine_device_plug_cb(HotplugHandler 
> *hotplug_dev,
>  VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);
>  
>  if (vms->platform_bus_dev) {
> -if (object_dynamic_cast(OBJECT(dev), TYPE_SYS_BUS_DEVICE)) {
> +MachineClass *mc = MACHINE_GET_CLASS(vms);
> +
> +if (device_is_dynamic_sysbus(mc, dev)) {
>  
> platform_bus_link_device(PLATFORM_BUS_DEVICE(vms->platform_bus_dev),
>   SYS_BUS_DEVICE(dev));
>  }
> @@ -2527,7 +2529,9 @@ static void 
> virt_machine_device_unplug_cb(HotplugHandler *hotplug_dev,
>  static HotplugHandler *virt_machine_get_hotplug_handler(MachineState 
> *machine,
>  DeviceState *dev)
>  {
> -if (object_dynamic_cast(OBJECT(dev), TYPE_SYS_BUS_DEVICE) ||
> +MachineClass *mc = MACHINE_GET_CLASS(machine);
> +
> +if (device_is_dynamic_sysbus(mc, dev) ||
> (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM))) {
>  return HOTPLUG_HANDLER(machine);
>  }
> 

Re: [PATCH for-6.0 4/4] hw/ppc/e500plat: Only try to add valid dynamic sysbus devices to platform bus

[Auger Eric]

On 3/25/21 4:33 PM, Peter Maydell wrote:
> The e500plat machine device plug callback currently calls
> platform_bus_link_device() for any sysbus device.  This is overly
> broad, because platform_bus_link_device() will unconditionally grab
> the IRQs and MMIOs of the device it is passed, whether it was
> intended for the platform bus or not.  Restrict hotpluggability of
> sysbus devices to only those devices on the dynamic sysbus whitelist.
> 
> We were mostly getting away with this because the board creates the
> platform bus as the last device it creates, and so the hotplug
> callback did not do anything for all the sysbus devices created by
> the board itself.  However if the user plugged in a device which
> itself uses a sysbus device internally we would have mishandled this
> and probably asserted. An example of this is:
>  qemu-system-ppc64 -M ppce500 -device macio-oldworld
> 
> This isn't a sensible command because the macio-oldworld device
> is really specific to the 'g3beige' machine, but we now fail
> with a reasonable error message rather than asserting:
> qemu-system-ppc64: Device heathrow is not supported by this machine yet.
> 
> Signed-off-by: Peter Maydell 
Reviewed-by: Eric Auger 

Eric
> ---
>  hw/ppc/e500plat.c | 8 ++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ppc/e500plat.c b/hw/ppc/e500plat.c
> index bddd5e7c48f..fc911bbb7bd 100644
> --- a/hw/ppc/e500plat.c
> +++ b/hw/ppc/e500plat.c
> @@ -48,7 +48,9 @@ static void e500plat_machine_device_plug_cb(HotplugHandler 
> *hotplug_dev,
>  PPCE500MachineState *pms = PPCE500_MACHINE(hotplug_dev);
>  
>  if (pms->pbus_dev) {
> -if (object_dynamic_cast(OBJECT(dev), TYPE_SYS_BUS_DEVICE)) {
> +MachineClass *mc = MACHINE_GET_CLASS(pms);
> +
> +if (device_is_dynamic_sysbus(mc, dev)) {
>  platform_bus_link_device(pms->pbus_dev, SYS_BUS_DEVICE(dev));
>  }
>  }
> @@ -58,7 +60,9 @@ static
>  HotplugHandler *e500plat_machine_get_hotpug_handler(MachineState *machine,
>  DeviceState *dev)
>  {
> -if (object_dynamic_cast(OBJECT(dev), TYPE_SYS_BUS_DEVICE)) {
> +MachineClass *mc = MACHINE_GET_CLASS(machine);
> +
> +if (device_is_dynamic_sysbus(mc, dev)) {
>  return HOTPLUG_HANDLER(machine);
>  }
>  
> 

Re: [PATCH v3 0/7] ui: add vdagent implementation and clipboard support.

[no-reply]

Patchew URL: https://patchew.org/QEMU/20210326092448.367016-1-kra...@redhat.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20210326092448.367016-1-kra...@redhat.com
Subject: [PATCH v3 0/7] ui: add vdagent implementation and clipboard support.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 - [tag update]  
patchew/1616680509-8339-1-git-send-email-mjros...@linux.ibm.com -> 
patchew/1616680509-8339-1-git-send-email-mjros...@linux.ibm.com
 - [tag update]  patchew/20210325153310.9131-1-peter.mayd...@linaro.org -> 
patchew/20210325153310.9131-1-peter.mayd...@linaro.org
 * [new tag] patchew/20210326092448.367016-1-kra...@redhat.com -> 
patchew/20210326092448.367016-1-kra...@redhat.com
Switched to a new branch 'test'
5146fb5 ui/gtk: add clipboard support
ca86b02 ui/gtk: move struct GtkDisplayState to ui/gtk.h
dd72abb ui/vnc: clipboard support
7420f0a ui/vdagent: add clipboard support
ccc2f36 ui/vdagent: add mouse support
a76ff23 ui/vdagent: core infrastructure
abe9fe7 ui: add clipboard infrastructure

=== OUTPUT BEGIN ===
1/7 Checking commit abe9fe76aacd (ui: add clipboard infrastructure)
Use of uninitialized value $acpi_testexpected in string eq at 
./scripts/checkpatch.pl line 1529.
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#20: 
new file mode 100644

total: 0 errors, 1 warnings, 167 lines checked

Patch 1/7 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
2/7 Checking commit a76ff2385723 (ui/vdagent: core infrastructure)
Use of uninitialized value $acpi_testexpected in string eq at 
./scripts/checkpatch.pl line 1529.
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#91: 
new file mode 100644

ERROR: if this code is redundant consider removing it
#143: FILE: ui/vdagent.c:48:
+#if 0

WARNING: line over 80 characters
#145: FILE: ui/vdagent.c:50:
+[VD_AGENT_CAP_CLIPBOARD_NO_RELEASE_ON_REGRAB] = 
"clipboard-no-release-on-regrab",

ERROR: if this code is redundant consider removing it
#166: FILE: ui/vdagent.c:71:
+#if 0

WARNING: line over 80 characters
#218: FILE: ui/vdagent.c:123:
+   
sizeof(VDAgentAnnounceCapabilities) +

total: 2 errors, 3 warnings, 357 lines checked

Patch 2/7 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

3/7 Checking commit ccc2f369055f (ui/vdagent: add mouse support)
ERROR: if this code is redundant consider removing it
#146: FILE: ui/vdagent.c:189:
+#if 0

total: 1 errors, 0 warnings, 237 lines checked

Patch 3/7 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

4/7 Checking commit 7420f0a6dd9e (ui/vdagent: add clipboard support)
ERROR: if this code is redundant consider removing it
#120: FILE: ui/vdagent.c:115:
+#if 0

ERROR: line over 90 characters
#170: FILE: ui/vdagent.c:308:
+   sizeof(uint32_t) * 
(QEMU_CLIPBOARD_TYPE__COUNT + 1));

WARNING: line over 80 characters
#439: FILE: ui/vdagent.c:729:
+cfg->clipboard = qemu_opt_get_bool(opts, "clipboard", 
VDAGENT_CLIPBOARD_DEFAULT);

total: 2 errors, 1 warnings, 392 lines checked

Patch 4/7 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

5/7 Checking commit dd72abb02968 (ui/vnc: clipboard support)
Use of uninitialized value $acpi_testexpected in string eq at 
./scripts/checkpatch.pl line 1529.
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#33: 
new file mode 100644

WARNING: line over 80 characters
#280: FILE: ui/vnc-clipboard.c:243:
+void vnc_client_cut_text_ext(VncState *vs, int32_t len, uint32_t flags, 
uint8_t *data)

WARNING: line over 80 characters
#289: FILE: ui/vnc-clipboard.c:252:
+qemu_clipboard_info_new(&vs->cbpeer, 
QEMU_CLIPBOARD_SELECTION_CLIPBOARD);

WARNING: line over 80 characters
#333: FILE: ui/vnc-clipboard.c:296:
+qemu_clipboard_info_new(&vs->cbpeer, 
QEMU_CLIPBOARD_SELECTION_CLIPBOARD);

ERROR: line over 90 characters
#420: FILE: ui/vnc.c:2458:
+vnc_client_cut_text_ext(vs, abs(read_s32(data, 4)), read_u32(data, 
8), data + 12);

WARNING: line over 80 characters
#499: FILE: ui/vnc.h:643:
+void vnc_client_cut_text_ext(VncState *vs, int32_t len, uint32_t flags, 
uint8_t *data);

total: 1 errors, 5 warnings, 450 lines checked

Patch 5/7 has 

[PATCH for-6.0] hw/usb/hcd-ehci: Fix crash when showing help of EHCI devices

[Thomas Huth]

QEMU crashes with certain targets when trying to show the help
output of EHCI devices:

$ ./qemu-system-aarch64 -device ich9-usb-ehci1,help
qemu-system-aarch64: ../../devel/qemu/softmmu/physmem.c:1154: phys_section_add:
 Assertion `map->sections_nb < TARGET_PAGE_SIZE' failed.
Aborted (core dumped)

This happens because the device is doing things at "instance_init" time
that should be done at "realize" time instead. So move the related code
to the realize() function instead. (NB: This now also matches the
memory_region_del_subregion() calls which are done in usb_ehci_unrealize(),
and not during finalize()).

Suggested-by: Richard Henderson 
Signed-off-by: Thomas Huth 
---
 hw/usb/hcd-ehci.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
index f71af0ad2d..6caa7ac6c2 100644
--- a/hw/usb/hcd-ehci.c
+++ b/hw/usb/hcd-ehci.c
@@ -2514,6 +2514,11 @@ void usb_ehci_realize(EHCIState *s, DeviceState *dev, 
Error **errp)
 return;
 }
 
+memory_region_add_subregion(&s->mem, s->capsbase, &s->mem_caps);
+memory_region_add_subregion(&s->mem, s->opregbase, &s->mem_opreg);
+memory_region_add_subregion(&s->mem, s->opregbase + s->portscbase,
+&s->mem_ports);
+
 usb_bus_new(&s->bus, sizeof(s->bus), s->companion_enable ?
 &ehci_bus_ops_companion : &ehci_bus_ops_standalone, dev);
 for (i = 0; i < s->portnr; i++) {
@@ -2581,11 +2586,6 @@ void usb_ehci_init(EHCIState *s, DeviceState *dev)
   "operational", s->portscbase);
 memory_region_init_io(&s->mem_ports, OBJECT(dev), &ehci_mmio_port_ops, s,
   "ports", 4 * s->portnr);
-
-memory_region_add_subregion(&s->mem, s->capsbase, &s->mem_caps);
-memory_region_add_subregion(&s->mem, s->opregbase, &s->mem_opreg);
-memory_region_add_subregion(&s->mem, s->opregbase + s->portscbase,
-&s->mem_ports);
 }
 
 void usb_ehci_finalize(EHCIState *s)
-- 
2.27.0

Re: [PATCH v2] qapi: introduce 'query-cpu-model-cpuid' action

[Vladimir Sementsov-Ogievskiy]

25.03.2021 19:57, Valeriy Vdovin wrote:

Introducing new qapi method 'query-cpu-model-cpuid'. This method can be used to
get virtualized cpu model info generated by QEMU during VM initialization in
the form of cpuid representation.

Diving into more details about virtual cpu generation: QEMU first parses '-cpu'
command line option. From there it takes the name of the model as the basis for
feature set of the new virtual cpu. After that it uses trailing '-cpu' options,
that state if additional cpu features should be present on the virtual cpu or
excluded from it (tokens '+'/'-' or '=on'/'=off').
After that QEMU checks if the host's cpu can actually support the derived
feature set and applies host limitations to it.
After this initialization procedure, virtual cpu has it's model and
vendor names, and a working feature set and is ready for identification
instructions such as CPUID.

Currently full output for this method is only supported for x86 cpus.

To learn exactly how virtual cpu is presented to the guest machine via CPUID
instruction, new qapi method can be used. By calling 'query-cpu-model-cpuid'
method, one can get a full listing of all CPUID leafs with subleafs which are
supported by the initialized virtual cpu.

Other than debug, the method is useful in cases when we would like to
utilize QEMU's virtual cpu initialization routines and put the retrieved
values into kernel CPUID overriding mechanics for more precise control
over how various processes perceive its underlying hardware with
container processes as a good example.

Output format:
The core part of the returned JSON object can be described as a list of lists
with top level list contains leaf-level elements and the bottom level
containing subleafs, where 'leaf' is CPUID argument passed in EAX register and
'subleaf' is a value passed to CPUID in ECX register for some specific
leafs, that support that. Each most basic CPUID result is passed in a
maximum of 4 registers EAX, EBX, ECX and EDX, with most leafs not utilizing
all 4 registers at once.
Also note that 'subleaf' is a kind of extension, used by only a couple of
leafs, while most of the leafs don't have this. Nevertheless, the output
data structure presents ALL leafs as having at least a single 'subleaf'.
This is done for data structure uniformity, so that it could be
processed in a more straightforward manner, in this case no one suffers
from such simplification.

Use example:
virsh qemu-monitor-command VM --pretty '{ "execute": "query-cpu-model-cpuid" }'
{
   "return": {
 "cpuid": {
   "leafs": [
 {
   "leaf": 0,
   "subleafs": [
 {
   "eax": 13,
   "edx": 1231384169,
   "ecx": 1818588270,
   "ebx": 1970169159,
   "subleaf": 0
 }
   ]
 },
 {
   "leaf": 1,
   "subleafs": [
 {
   "eax": 329443,
   "edx": 529267711,
   "ecx": 4160369187,
   "ebx": 133120,
   "subleaf": 0
 }
   ]
 },
 {
   "leaf": 2,
   "subleafs": [
 {
   "eax": 1,
   "edx": 2895997,
   "ecx": 0,
   "ebx": 0,
   "subleaf": 0
 }
   ]
 },
   ]
 },
 "vendor": "GenuineIntel",
 "class-name": "Skylake-Client-IBRS-x86_64-cpu",
 "model-id": "Intel Core Processor (Skylake, IBRS)"
   },
   "id": "libvirt-40"
}

Signed-off-by: Valeriy Vdovin 
---
v2: - Removed leaf/subleaf iterators.
 - Modified cpu_x86_cpuid to return false in cases when count is
   greater than supported subleaves.

  qapi/machine-target.json | 122 +++
  target/i386/cpu.h|   2 +-
  target/i386/cpu.c| 209 +++
  3 files changed, 315 insertions(+), 18 deletions(-)

diff --git a/qapi/machine-target.json b/qapi/machine-target.json
index e7811654b7..c5b137aa5c 100644
--- a/qapi/machine-target.json
+++ b/qapi/machine-target.json
@@ -329,3 +329,125 @@
  ##
  { 'command': 'query-cpu-definitions', 'returns': ['CpuDefinitionInfo'],
'if': 'defined(TARGET_PPC) || defined(TARGET_ARM) || defined(TARGET_I386) 
|| defined(TARGET_S390X) || defined(TARGET_MIPS)' }
+##
+
+
+# @CpuidSubleaf:
+#
+# CPUID leaf extension information, based on ECX value.
+#
+# CPUID x86 instruction has 'leaf' argument passed in EAX register. Leaf
+# argument identifies the type of information, the caller wants to retrieve in
+# single call to CPUID.
+# Some but not all leaves depend on the value passed in ECX register as an
+# additional argument to CPUID. This argument is present in cpuid documentation
+# as 'subleaf'.
+# If CPUID ignores the value in ECX, normally this means that leaf does not
+# have subleaves. Another way to see it is that each leaf has at least one
+# subleaf (one type of output).
+#
+# @subleaf: value passed 

[PATCH for-6.0] qapi: qom: do not use target-specific conditionals

[Paolo Bonzini]

ObjectType and ObjectOptions are defined in a target-independent file,
therefore they do not have access to target-specific configuration
symbols such as CONFIG_PSERIES or CONFIG_SEV.  For this reason,
pef-guest and sev-guest are currently omitted when compiling the
generated QAPI files.  In addition, this causes ObjectType to have
different definitions depending on the file that is including
qapi-types-qom.h (currently this is not causing any issues, but it
is wrong).

Define the two enum entries and the SevGuestProperties type
unconditionally to avoid the issue.  We do not expect to have
many target-dependent user-creatable classes, so it is not
particularly problematic.

Reported-by: Tom Lendacky 
Signed-off-by: Paolo Bonzini 
---
 qapi/qom.json | 10 --
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/qapi/qom.json b/qapi/qom.json
index 2056edc072..db5ac419b1 100644
--- a/qapi/qom.json
+++ b/qapi/qom.json
@@ -733,8 +733,7 @@
 '*policy': 'uint32',
 '*handle': 'uint32',
 '*cbitpos': 'uint32',
-'reduced-phys-bits': 'uint32' },
-  'if': 'defined(CONFIG_SEV)' }
+'reduced-phys-bits': 'uint32' } }
 
 ##
 # @ObjectType:
@@ -768,14 +767,14 @@
 { 'name': 'memory-backend-memfd',
   'if': 'defined(CONFIG_LINUX)' },
 'memory-backend-ram',
-{'name': 'pef-guest', 'if': 'defined(CONFIG_PSERIES)' },
+'pef-guest',
 'pr-manager-helper',
 'rng-builtin',
 'rng-egd',
 'rng-random',
 'secret',
 'secret_keyring',
-{'name': 'sev-guest', 'if': 'defined(CONFIG_SEV)' },
+'sev-guest',
 's390-pv-guest',
 'throttle-group',
 'tls-creds-anon',
@@ -831,8 +830,7 @@
   'rng-random': 'RngRandomProperties',
   'secret': 'SecretProperties',
   'secret_keyring': 'SecretKeyringProperties',
-  'sev-guest':  { 'type': 'SevGuestProperties',
-  'if': 'defined(CONFIG_SEV)' },
+  'sev-guest':  'SevGuestProperties',
   'throttle-group': 'ThrottleGroupProperties',
   'tls-creds-anon': 'TlsCredsAnonProperties',
   'tls-creds-psk':  'TlsCredsPskProperties',
-- 
2.26.2

[Bug 1920752] Re: USB SoundCard Passthrough not working on arm64

[Gerd Hoffmann]

Can you record usb traffic (add pcap= to usb-host)?

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1920752

Title:
  USB SoundCard Passthrough not working on arm64

Status in QEMU:
  New

Bug description:
  Hello,

  I am virtualizing a armhf guest on a aarch64 host and was to use my
  Sound Blaster USB Soundcard as passthrough.

  armhf Guest is: Debian Buster 
  aarch64 host is a jetson nano. KVM is enabled.

  Latest qemu is built from sources.
  The command I use for running is as follows:

  ../qemu/build/qemu-system-aarch64 -M virt -m 2048 -smp 2 -cpu 
host,aarch64=off -enable-kvm  \
  -kernel vmlinuz-4.19.0-14-armmp-lpae  -initrd initrd.img-4.19.0-14-armmp-lpae 
-append 'root=/dev/vda2' \
  -device nec-usb-xhci -device usb-kbd  -device usb-mouse -device 
usb-host,hostbus=1,hostport=2.3  -serial stdio  \
  -device virtio-gpu-pci,virgl=on,xres=1600,yres=900 -display sdl,gl=on \
  -drive if=none,file=hda2.qcow2,format=qcow2,id=hd   -device 
virtio-blk-device,drive=hd   \
  -netdev user,id=mynet   -device virtio-net-device,netdev=mynet \
  -bios edk2-arm-code.fd -no-reboot

  
  Where are my lsusb -t shows:

  rreddy78@jetson-nano:~/Downloads$ lsusb -t
  /:  Bus 02.Port 1: Dev 1, Class=root_hub, Driver=tegra-xusb/4p, 5000M
  |__ Port 1: Dev 3, If 0, Class=Hub, Driver=hub/4p, 5000M
  /:  Bus 01.Port 1: Dev 1, Class=root_hub, Driver=tegra-xusb/5p, 480M
  |__ Port 2: Dev 6, If 0, Class=Hub, Driver=hub/4p, 480M
  |__ Port 1: Dev 7, If 0, Class=Human Interface Device, Driver=usbhid, 
1.5M
  |__ Port 1: Dev 7, If 1, Class=Human Interface Device, Driver=usbhid, 
1.5M
  |__ Port 3: Dev 8, If 3, Class=Human Interface Device, Driver=usbhid, 
12M
  |__ Port 3: Dev 8, If 1, Class=Audio, Driver=snd-usb-audio, 12M
  |__ Port 3: Dev 8, If 2, Class=Audio, Driver=snd-usb-audio, 12M
  |__ Port 3: Dev 8, If 0, Class=Audio, Driver=snd-usb-audio, 12M
  |__ Port 4: Dev 9, If 0, Class=Human Interface Device, Driver=usbhid, 
1.5M

  Within the VM I can see the usb as follows

  rreddy78@debian:~$ lsusb -t
  /:  Bus 02.Port 1: Dev 1, Class=root_hub, Driver=xhci_hcd/4p, 5000M
  /:  Bus 01.Port 1: Dev 1, Class=root_hub, Driver=xhci_hcd/4p, 480M
  |__ Port 1: Dev 2, If 0, Class=Human Interface Device, Driver=usbhid, 480M
  |__ Port 2: Dev 3, If 0, Class=Human Interface Device, Driver=usbhid, 480M

  
  Its looks like some passthrough as but it seems like only for

   _ Port 3: Dev 8, If 3, Class=Human Interface Device, Driver=usbhid,
  12M

  I am not sure if passthrough  even works because this post I saw

  https://community.arm.com/developer/ip-products/system/f/embedded-
  forum/48031/usb-pass-through-in-qemu-command-line-for-arm-
  machines/168764#168764

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1920752/+subscriptions

Re: [PATCH V3] target/riscv: Align the data type of reset vector address

[Dylan Jhong]

On Fri, Mar 26, 2021 at 04:19:09AM +0800, Alistair Francis wrote:
> On Thu, Mar 25, 2021 at 5:43 AM Dylan Jhong  wrote:
> >
> > Signed-off-by: Dylan Jhong 
> > Signed-off-by: Ruinland ChuanTzu Tsai 
> > ---
> >  target/riscv/cpu.c | 6 +-
> >  target/riscv/cpu.h | 2 +-
> >  2 files changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > index 7d6ed80f6b..8a5f18bcb0 100644
> > --- a/target/riscv/cpu.c
> > +++ b/target/riscv/cpu.c
> > @@ -137,7 +137,7 @@ static void set_feature(CPURISCVState *env, int feature)
> >  env->features |= (1ULL << feature);
> >  }
> >
> > -static void set_resetvec(CPURISCVState *env, int resetvec)
> > +static void set_resetvec(CPURISCVState *env, target_ulong resetvec)
> >  {
> >  #ifndef CONFIG_USER_ONLY
> >  env->resetvec = resetvec;
> > @@ -554,7 +554,11 @@ static Property riscv_cpu_properties[] = {
> >  DEFINE_PROP_UINT16("elen", RISCVCPU, cfg.elen, 64),
> >  DEFINE_PROP_BOOL("mmu", RISCVCPU, cfg.mmu, true),
> >  DEFINE_PROP_BOOL("pmp", RISCVCPU, cfg.pmp, true),
> > +#if defined(TARGET_RISCV32)
> > +DEFINE_PROP_UINT32("resetvec", RISCVCPU, cfg.resetvec, DEFAULT_RSTVEC),
> > +#elif defined(TARGET_RISCV64)
> >  DEFINE_PROP_UINT64("resetvec", RISCVCPU, cfg.resetvec, DEFAULT_RSTVEC),
> > +#endif
> 
> Thanks for the patch!
> 
> I don't want to introduce any more define(TARGET_* macros as we are
> trying to make RISC-V QEMU xlen independent.
> 
> The hexagon port has an example of how you can use target_ulong here:
> 
> DEFINE_PROP_UNSIGNED("lldb-stack-adjust", HexagonCPU, lldb_stack_adjust,
>  0, qdev_prop_uint32, target_ulong);
> 
> can you do something like that instead?
> 
> Alistair
>

Hi Alistair,

Thanks for the comments.
But so far I did not see a way to satisfy both 32/64bit reset vector define.

The problem occurs in the 5th parameter of DEFINE_PROP_UNSIGNED(_name, _state, 
_field, _defval, _prop, _type).
We need to specify the _prop parameter to one of the PropertyInfo struct as 
shown below:

extern const PropertyInfo qdev_prop_bit;
extern const PropertyInfo qdev_prop_bit64;
extern const PropertyInfo qdev_prop_bool;
extern const PropertyInfo qdev_prop_enum;
extern const PropertyInfo qdev_prop_uint8;
extern const PropertyInfo qdev_prop_uint16;
extern const PropertyInfo qdev_prop_uint32;
extern const PropertyInfo qdev_prop_int32;
extern const PropertyInfo qdev_prop_uint64;
extern const PropertyInfo qdev_prop_int64;
extern const PropertyInfo qdev_prop_size;
extern const PropertyInfo qdev_prop_string;
extern const PropertyInfo qdev_prop_on_off_auto;
extern const PropertyInfo qdev_prop_size32;
extern const PropertyInfo qdev_prop_arraylen;
extern const PropertyInfo qdev_prop_link;

Currently, there is no structure like "qdev_prop_target_ulong".
So, we still need to use an if-else condition to determine the attributes of 
the 5th parameter.
Something like this:
#if defined(TARGET_RISCV32)
DEFINE_PROP_UNSIGNED("resetvec", RISCVCPU, cfg.resetvec, 
DEFAULT_RSTVEC, qdev_prop_uint32 target_ulong),
#elif defined(TARGET_RISCV64)
DEFINE_PROP_UNSIGNED("resetvec", RISCVCPU, cfg.resetvec, 
DEFAULT_RSTVEC, qdev_prop_uint64 target_ulong),
#endif
I think this is not be what you meant.

The other architectures seem to ignore this, they just choose one of the 
attributes(qdev_prop_uint32/64) as their parameter.

So now we have 2 options:
1. Use "qdev_prop_uint64" as the 5th parameter
DEFINE_PROP_UNSIGNED("resetvec", RISCVCPU, cfg.resetvec, DEFAULT_RSTVEC, 
qdev_prop_uint64 target_ulong),

2. Use if-else condition
[patch v3]

Or if you have other opinions, please bring them up and discuss them together.

Thanks,
Dylan
 
> >  DEFINE_PROP_END_OF_LIST(),
> >  };
> >
> > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> > index 0a33d387ba..d9d7891666 100644
> > --- a/target/riscv/cpu.h
> > +++ b/target/riscv/cpu.h
> > @@ -303,7 +303,7 @@ struct RISCVCPU {
> >  uint16_t elen;
> >  bool mmu;
> >  bool pmp;
> > -uint64_t resetvec;
> > +target_ulong resetvec;
> >  } cfg;
> >  };
> >
> > --
> > 2.17.1
> >
> >

Re: [PULL 0/2] Block patches

[Peter Maydell]

On Wed, 24 Mar 2021 at 14:52, Stefan Hajnoczi  wrote:
>
> The following changes since commit 67c1115edd98f388ca89dd38322ea3fadf034523:
>
>   Merge remote-tracking branch 'remotes/kraxel/tags/ui-20210323-pull-request' 
> into staging (2021-03-23 23:47:30 +)
>
> are available in the Git repository at:
>
>   https://gitlab.com/stefanha/qemu.git tags/block-pull-request
>
> for you to fetch changes up to 3460fd7f3959d1fa7bcc255796844aa261c805a4:
>
>   migrate-bitmaps-postcopy-test: check that we can't remove in-flight bitmaps 
> (2021-03-24 13:41:19 +)
>
> 
> Pull request
>
> This dirty bitmap fix solves a crash that can be triggered in the destination
> QEMU process during live migration.
>


Applied, thanks.

Please update the changelog at https://wiki.qemu.org/ChangeLog/6.0
for any user-visible changes.

-- PMM

Re: [PATCH for-6.0 1/4] include/hw/boards.h: Document machine_class_allow_dynamic_sysbus_dev()

[Peter Maydell]

On Fri, 26 Mar 2021 at 09:27, Auger Eric  wrote:
>
> Hi Peter,
>
> On 3/25/21 4:33 PM, Peter Maydell wrote:
> > The function machine_class_allow_dynamic_sysbus_dev() is currently
> > undocumented; add a doc comment.
> >
> > Signed-off-by: Peter Maydell 
> > ---
> >  include/hw/boards.h | 14 ++
> >  1 file changed, 14 insertions(+)
> >
> > diff --git a/include/hw/boards.h b/include/hw/boards.h
> > index 4a90549ad85..27106abc11d 100644
> > --- a/include/hw/boards.h
> > +++ b/include/hw/boards.h
> > @@ -36,7 +36,21 @@ void machine_set_cpu_numa_node(MachineState *machine,
> > const CpuInstanceProperties *props,
> > Error **errp);
> >
> > +/**
> > + * machine_class_allow_dynamic_sysbus_dev: Add type to list of valid 
> > devices
> nit: s/of valid devices/of dynamically instantiable sysbus devices ?

I was trying to keep the summary line to be one line, which
doesn't give much space for nuance with a function name this long...


-- PMM

Re: [PATCH for-6.0 1/4] include/hw/boards.h: Document machine_class_allow_dynamic_sysbus_dev()

[Auger Eric]

Hi Peter,

On 3/26/21 11:20 AM, Peter Maydell wrote:
> On Fri, 26 Mar 2021 at 09:27, Auger Eric  wrote:
>>
>> Hi Peter,
>>
>> On 3/25/21 4:33 PM, Peter Maydell wrote:
>>> The function machine_class_allow_dynamic_sysbus_dev() is currently
>>> undocumented; add a doc comment.
>>>
>>> Signed-off-by: Peter Maydell 
>>> ---
>>>  include/hw/boards.h | 14 ++
>>>  1 file changed, 14 insertions(+)
>>>
>>> diff --git a/include/hw/boards.h b/include/hw/boards.h
>>> index 4a90549ad85..27106abc11d 100644
>>> --- a/include/hw/boards.h
>>> +++ b/include/hw/boards.h
>>> @@ -36,7 +36,21 @@ void machine_set_cpu_numa_node(MachineState *machine,
>>> const CpuInstanceProperties *props,
>>> Error **errp);
>>>
>>> +/**
>>> + * machine_class_allow_dynamic_sysbus_dev: Add type to list of valid 
>>> devices
>> nit: s/of valid devices/of dynamically instantiable sysbus devices ?
> 
> I was trying to keep the summary line to be one line, which
> doesn't give much space for nuance with a function name this long...

OK no worries

Thanks

Eric
> 
> 
> -- PMM
> 

Re: [PATCH for-6.0] qapi: qom: do not use target-specific conditionals

[Markus Armbruster]

Paolo Bonzini  writes:

> ObjectType and ObjectOptions are defined in a target-independent file,
> therefore they do not have access to target-specific configuration
> symbols such as CONFIG_PSERIES or CONFIG_SEV.  For this reason,
> pef-guest and sev-guest are currently omitted when compiling the
> generated QAPI files.  In addition, this causes ObjectType to have
> different definitions depending on the file that is including
> qapi-types-qom.h (currently this is not causing any issues, but it
> is wrong).
>
> Define the two enum entries and the SevGuestProperties type
> unconditionally to avoid the issue.  We do not expect to have
> many target-dependent user-creatable classes, so it is not
> particularly problematic.
>
> Reported-by: Tom Lendacky 
> Signed-off-by: Paolo Bonzini 
> ---
>  qapi/qom.json | 10 --
>  1 file changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/qapi/qom.json b/qapi/qom.json
> index 2056edc072..db5ac419b1 100644
> --- a/qapi/qom.json
> +++ b/qapi/qom.json
> @@ -733,8 +733,7 @@
>  '*policy': 'uint32',
>  '*handle': 'uint32',
>  '*cbitpos': 'uint32',
> -'reduced-phys-bits': 'uint32' },
> -  'if': 'defined(CONFIG_SEV)' }
> +'reduced-phys-bits': 'uint32' } }
>  
>  ##
>  # @ObjectType:
> @@ -768,14 +767,14 @@
>  { 'name': 'memory-backend-memfd',
>'if': 'defined(CONFIG_LINUX)' },
>  'memory-backend-ram',
> -{'name': 'pef-guest', 'if': 'defined(CONFIG_PSERIES)' },
> +'pef-guest',
>  'pr-manager-helper',
>  'rng-builtin',
>  'rng-egd',
>  'rng-random',
>  'secret',
>  'secret_keyring',
> -{'name': 'sev-guest', 'if': 'defined(CONFIG_SEV)' },
> +'sev-guest',
>  's390-pv-guest',
>  'throttle-group',
>  'tls-creds-anon',
> @@ -831,8 +830,7 @@
>'rng-random': 'RngRandomProperties',
>'secret': 'SecretProperties',
>'secret_keyring': 'SecretKeyringProperties',
> -  'sev-guest':  { 'type': 'SevGuestProperties',
> -  'if': 'defined(CONFIG_SEV)' },
> +  'sev-guest':  'SevGuestProperties',
>'throttle-group': 'ThrottleGroupProperties',
>'tls-creds-anon': 'TlsCredsAnonProperties',
>'tls-creds-psk':  'TlsCredsPskProperties',

No branch for tag value 'pef-guest', i.e. no tag-specific members.
There are two more: can_bus, s390_pv_guest.  I assume this is
intentional.

Links a bit of dead code into the other qemu-system-FOO, but that's
okay.

If we genuinely needed (or wanted) target-dependent -object, we'd split
qom-target.json off qom.json, and put the target-dependent parts there,
including the enum and the union, with the obvious ripple effects.  Not
now, maybe not ever.

Would adding "only for CONFIG_MUMBLE" to the doc comments make sense?
It's what we did before we had 'if'.

Re: [PATCH V3] target/riscv: Align the data type of reset vector address

[Peter Maydell]

On Fri, 26 Mar 2021 at 10:21, Dylan Jhong  wrote:
> Currently, there is no structure like "qdev_prop_target_ulong".
> So, we still need to use an if-else condition to determine the attributes of 
> the 5th parameter.
> Something like this:
> #if defined(TARGET_RISCV32)
> DEFINE_PROP_UNSIGNED("resetvec", RISCVCPU, cfg.resetvec, 
> DEFAULT_RSTVEC, qdev_prop_uint32 target_ulong),
> #elif defined(TARGET_RISCV64)
> DEFINE_PROP_UNSIGNED("resetvec", RISCVCPU, cfg.resetvec, 
> DEFAULT_RSTVEC, qdev_prop_uint64 target_ulong),
> #endif
> I think this is not be what you meant.
>
> The other architectures seem to ignore this, they just choose one of the 
> attributes(qdev_prop_uint32/64) as their parameter.
>
> So now we have 2 options:
> 1. Use "qdev_prop_uint64" as the 5th parameter
> DEFINE_PROP_UNSIGNED("resetvec", RISCVCPU, cfg.resetvec, DEFAULT_RSTVEC, 
> qdev_prop_uint64 target_ulong),
>
> 2. Use if-else condition
> [patch v3]
>
> Or if you have other opinions, please bring them up and discuss them together.

I would recommend that you just use DEFINE_PROP_UINT64 for this property
(and store the value in a uint64_t cfg.resetvec) regardless of whether the
guest CPU happens to be 32 or 64 bit. In the case where it's 32 bits you
can just ignore the top 32 bits (or if you're feeling enthusiastic, report
an error if they're non-zero). This is simpler to code, avoids ifdefs,
and tends to mean that most code doesn't need to care about the 32-vs-64
difference.

thanks
-- PMM

Re: [PATCH 1/4] qcow2: Improve refcount structure rebuilding

[Vladimir Sementsov-Ogievskiy]

10.03.2021 18:59, Max Reitz wrote:

When rebuilding the refcount structures (when qemu-img check -r found
errors with refcount = 0, but reference count > 0), the new refcount
table defaults to being put at the image file end[1].  There is no good
reason for that except that it means we will not have to rewrite any
refblocks we already wrote to disk.

Changing the code to rewrite those refblocks is not too difficult,
though, so let us do that.  That is beneficial for images on block
devices, where we cannot really write beyond the end of the image file.

[1] Unless there is something allocated in the area pointed to by the
 last refblock, so we have to write that refblock.  In that case, we
 try to put the reftable in there.

Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1519071
Signed-off-by: Max Reitz 
---
  block/qcow2-refcount.c | 126 ++---
  1 file changed, 67 insertions(+), 59 deletions(-)

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 8e649b008e..162caeeb8e 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -2352,8 +2352,9 @@ static int rebuild_refcount_structure(BlockDriverState 
*bs,
int64_t *nb_clusters)
  {
  BDRVQcow2State *s = bs->opaque;
-int64_t first_free_cluster = 0, reftable_offset = -1, cluster = 0;
+int64_t first_free_cluster = 0, reftable_offset = -1, cluster;
  int64_t refblock_offset, refblock_start, refblock_index;
+int64_t first_cluster, end_cluster;
  uint32_t reftable_size = 0;
  uint64_t *on_disk_reftable = NULL;
  void *on_disk_refblock;
@@ -2365,8 +2366,11 @@ static int rebuild_refcount_structure(BlockDriverState 
*bs,
  
  qcow2_cache_empty(bs, s->refcount_block_cache);
  
+first_cluster = 0;

+end_cluster = *nb_clusters;
+
  write_refblocks:
-for (; cluster < *nb_clusters; cluster++) {
+for (cluster = first_cluster; cluster < end_cluster; cluster++) {
  if (!s->get_refcount(*refcount_table, cluster)) {
  continue;
  }
@@ -2374,65 +2378,68 @@ write_refblocks:
  refblock_index = cluster >> s->refcount_block_bits;
  refblock_start = refblock_index << s->refcount_block_bits;
  
-/* Don't allocate a cluster in a refblock already written to disk */

-if (first_free_cluster < refblock_start) {
-first_free_cluster = refblock_start;
-}
-refblock_offset = alloc_clusters_imrt(bs, 1, refcount_table,
-  nb_clusters, 
&first_free_cluster);
-if (refblock_offset < 0) {
-fprintf(stderr, "ERROR allocating refblock: %s\n",
-strerror(-refblock_offset));
-res->check_errors++;
-ret = refblock_offset;
-goto fail;
-}
-
-if (reftable_size <= refblock_index) {
-uint32_t old_reftable_size = reftable_size;
-uint64_t *new_on_disk_reftable;
+if (reftable_size > refblock_index &&
+on_disk_reftable[refblock_index])
+{
+refblock_offset = on_disk_reftable[refblock_index];


In this branch, we assign it to ..


+} else {
+int64_t refblock_cluster_index;
  
-reftable_size = ROUND_UP((refblock_index + 1) * REFTABLE_ENTRY_SIZE,

- s->cluster_size) / REFTABLE_ENTRY_SIZE;
-new_on_disk_reftable = g_try_realloc(on_disk_reftable,
- reftable_size *
- REFTABLE_ENTRY_SIZE);
-if (!new_on_disk_reftable) {
+/* Don't allocate a cluster in a refblock already written to disk 
*/
+if (first_free_cluster < refblock_start) {
+first_free_cluster = refblock_start;
+}
+refblock_offset = alloc_clusters_imrt(bs, 1, refcount_table,
+  nb_clusters,
+  &first_free_cluster);
+if (refblock_offset < 0) {
+fprintf(stderr, "ERROR allocating refblock: %s\n",
+strerror(-refblock_offset));
  res->check_errors++;
-ret = -ENOMEM;
+ret = refblock_offset;
  goto fail;
  }
-on_disk_reftable = new_on_disk_reftable;
  
-memset(on_disk_reftable + old_reftable_size, 0,

-   (reftable_size - old_reftable_size) * REFTABLE_ENTRY_SIZE);
+refblock_cluster_index = refblock_offset / s->cluster_size;
+if (refblock_cluster_index >= end_cluster) {
+/*
+ * We must write the refblock that holds this refblock's
+ * refcount
+ */
+end_cluster = refblock_cluster_index + 1;
+}
  
-

Re: [PATCH for-6.0] qapi: qom: do not use target-specific conditionals

[Paolo Bonzini]

On 26/03/21 11:57, Markus Armbruster wrote:

'rng-random': 'RngRandomProperties',
'secret': 'SecretProperties',
'secret_keyring': 'SecretKeyringProperties',
-  'sev-guest':  { 'type': 'SevGuestProperties',
-  'if': 'defined(CONFIG_SEV)' },
+  'sev-guest':  'SevGuestProperties',
'throttle-group': 'ThrottleGroupProperties',
'tls-creds-anon': 'TlsCredsAnonProperties',
'tls-creds-psk':  'TlsCredsPskProperties',


No branch for tag value 'pef-guest', i.e. no tag-specific members.
There are two more: can_bus, s390_pv_guest.  I assume this is
intentional.


Yes, they have no properties.


Links a bit of dead code into the other qemu-system-FOO, but that's
okay.

If we genuinely needed (or wanted) target-dependent -object, we'd split
qom-target.json off qom.json, and put the target-dependent parts there,
including the enum and the union, with the obvious ripple effects.  Not
now, maybe not ever.

Would adding "only for CONFIG_MUMBLE" to the doc comments make sense?
It's what we did before we had 'if'.


In this specific case we had not documentation at all for objects.  We 
can add the information on relevant targets in the documentation for the 
*Properties types.


Paolo

Re: [PATCH for-6.0] qapi: qom: do not use target-specific conditionals

[Markus Armbruster]

Paolo Bonzini  writes:

> On 26/03/21 11:57, Markus Armbruster wrote:
>>> 'rng-random': 'RngRandomProperties',
>>> 'secret': 'SecretProperties',
>>> 'secret_keyring': 'SecretKeyringProperties',
>>> -  'sev-guest':  { 'type': 'SevGuestProperties',
>>> -  'if': 'defined(CONFIG_SEV)' },
>>> +  'sev-guest':  'SevGuestProperties',
>>> 'throttle-group': 'ThrottleGroupProperties',
>>> 'tls-creds-anon': 'TlsCredsAnonProperties',
>>> 'tls-creds-psk':  'TlsCredsPskProperties',
>> No branch for tag value 'pef-guest', i.e. no tag-specific members.
>> There are two more: can_bus, s390_pv_guest.  I assume this is
>> intentional.
>
> Yes, they have no properties.
>
>> Links a bit of dead code into the other qemu-system-FOO, but that's
>> okay.
>> If we genuinely needed (or wanted) target-dependent -object, we'd
>> split
>> qom-target.json off qom.json, and put the target-dependent parts there,
>> including the enum and the union, with the obvious ripple effects.  Not
>> now, maybe not ever.
>> Would adding "only for CONFIG_MUMBLE" to the doc comments make
>> sense?
>> It's what we did before we had 'if'.
>
> In this specific case we had not documentation at all for objects.

... until Kevin added some when he QAPIfied.  Looks like this (copied
from qemu-qmp-ref.7)[*]:

   SevGuestProperties (Object)
   Properties for sev-guest objects.

   Members
   sev-device: string (optional)
  SEV device to use (default: "/dev/sev")

   dh-cert-file: string (optional)
  guest owners DH certificate (encoded with base64)

   session-file: string (optional)
  guest owners session parameters (encoded with base64)

   policy: int (optional)
  SEV policy value (default: 0x1)

   handle: int (optional)
  SEV firmware handle (default: 0)

   cbitpos: int (optional)
  C-bit location in page table entry (default: 0)

   reduced-phys-bits: int
  number  of  bits  in  physical addresses that become unavailable
  when SEV is enabled

   Since
   2.12

   If
   defined(CONFIG_SEV)

Your patch drops the last three lines, without a replacement.

> We
> can add the information on relevant targets in the documentation for
> the *Properties types.

Yes, please.

Preferably with that squashed in:
Reviewed-by: Markus Armbruster 


[*] I lied.  It actually looks like

   If
   defined(CONFIG_SEV).SS ObjectType (Enum)

The running together of "defined(CONFIG_SEV)" and ".SS ObjectType
(Enum)" is a bug.  I'll investigate.

Re: [Qemu-devel] [PULL v5 39/43] hw/hppa: Implement DINO system board

[Richard Henderson]

On 3/25/21 5:17 PM, Philippe Mathieu-Daudé wrote:

+/* Set up windows into PCI bus memory.  */
+for (i = 1; i < 31; i++) {
+uint32_t addr = 0xf000 + i * DINO_MEM_CHUNK_SIZE;
+char *name = g_strdup_printf("PCI Outbound Window %d", i);
+memory_region_init_alias(&s->pci_mem_alias[i], OBJECT(s),
+ name, &s->pci_mem, addr,
+ DINO_MEM_CHUNK_SIZE);


Where are these aliases mapped?


gsc_to_pci_forwarding


r~

Re: [PATCH v5 10/10] target/ppc: Validate hflags with CONFIG_DEBUG_TCG

[Richard Henderson]

On 3/25/21 2:47 AM, David Gibson wrote:

On Wed, Mar 24, 2021 at 11:12:20AM +1100, David Gibson wrote:

On Tue, Mar 23, 2021 at 12:43:40PM -0600, Richard Henderson wrote:

Verify that hflags was updated correctly whenever we change
cpu state that is used by hflags.

Signed-off-by: Richard Henderson 


Applied to ppc-for-6.0, thanks.


Alas, this appears to cause a failure in the 'build-user' test on
gitlab CI :(.  I haven't managed to reproduce it locally, so I'm not
sure what's going on.


I guess you mean e.g.

https://gitlab.com/dgibson/qemu/-/jobs/1126364147

?  I'll have a look.


r~

Re: [PATCH v3 0/8] [RfC] fix tracing for modules

[Gerd Hoffmann]

  Hi,

> eg a trace point "dma_map_wait" gets mapped to probes in many
> .stp files, once per target, because we need to match based on
> the executable path:
> 
>   probe qemu.system.x86_64.dma_map_wait = 
> process("/usr/libexec/qemu-system-x86_64").mark("dma_map_wait")
>   probe qemu.system.x86_64.dma_map_wait = 
> process("/usr/libexec/qemu-system-ppc64").mark("dma_map_wait")

Probe qemu.system.ppc64.dma_map_wait = ...

Can I trace qemu started from build directory?
Seems scripts/qemu-trace-stap doesn't support that.

>   qxl.destroy_primary(int qid) "%d"
>   qxl.enter_vga_mode(int qid) "%d"
>   qxl.exit_vga_mode(int qid) "%d"
> 
> this would be backwards compatible, as we can turn the "." back into
> a "_" for all existing trace backends, except stp.

Hmm, not sure I like this inconsistency.  I think names should be the
same no matter which trace backend is used.

take care,
  Gerd

[PULL 0/9] Fixes 20210326 patches

[Gerd Hoffmann]

The following changes since commit 9e2e9fe3df9f539f8b6941ceb96d25355fdae47e:

  Update version for v6.0.0-rc0 release (2021-03-24 19:50:49 +)

are available in the Git repository at:

  git://git.kraxel.org/qemu tags/fixes-20210326-pull-request

for you to fetch changes up to db0b034185824ac33e1a85ba62ab2030eb17b00d:

  hw/usb/hcd-ehci: Fix crash when showing help of EHCI devices (2021-03-26 
11:10:49 +0100)


fixes for usb, virtio-gpu and vhost-gpu



Gerd Hoffmann (3):
  s390x: move S390_ADAPTER_SUPPRESSIBLE
  s390x: add have_virtio_ccw
  s390x: modularize virtio-gpu-ccw

Marc-André Lureau (3):
  vhost-user-gpu: glFlush before notifying clients
  vhost-user-gpu: fix vugbm_device_init fallback
  vhost-user-gpu: fix cursor move/update

Philippe Mathieu-Daudé (1):
  hw/usb/hcd-ehci-sysbus: Free USBPacket on instance finalize()

Thomas Huth (2):
  usb: Remove "-usbdevice ccid"
  hw/usb/hcd-ehci: Fix crash when showing help of EHCI devices

 contrib/vhost-user-gpu/vugbm.h  |  2 +-
 hw/s390x/virtio-ccw.h   |  5 +++
 include/hw/s390x/css.h  |  7 
 include/hw/s390x/s390_flic.h|  3 ++
 target/s390x/cpu.h  |  9 +++--
 contrib/vhost-user-gpu/vhost-user-gpu.c | 24 +++---
 contrib/vhost-user-gpu/virgl.c  |  3 ++
 contrib/vhost-user-gpu/vugbm.c  | 44 +++--
 hw/s390x/virtio-ccw-gpu.c   |  4 ++-
 hw/s390x/virtio-ccw.c   |  2 ++
 hw/usb/dev-smartcard-reader.c   |  1 -
 hw/usb/hcd-ehci-sysbus.c|  9 +
 hw/usb/hcd-ehci.c   | 10 +++---
 util/module.c   |  1 +
 contrib/vhost-user-gpu/meson.build  |  2 +-
 docs/system/removed-features.rst|  6 
 hw/s390x/meson.build|  8 -
 qemu-options.hx |  3 --
 18 files changed, 85 insertions(+), 58 deletions(-)

-- 
2.30.2

[PULL 3/9] vhost-user-gpu: fix vugbm_device_init fallback

[Gerd Hoffmann]

From: Marc-André Lureau 

vugbm implements GBM device wrapping, udmabuf and memory fallback.
However, the fallback/detection logic is flawed, as if "/dev/udmabuf"
failed to be opened, it will not initialize vugbm and crash later.

Rework the vugbm_device_init() logic to initialize correctly in all
cases.

Signed-off-by: Marc-André Lureau 
Message-Id: <20210312100108.2706195-4-marcandre.lur...@redhat.com>
Signed-off-by: Gerd Hoffmann 
---
 contrib/vhost-user-gpu/vugbm.h  |  2 +-
 contrib/vhost-user-gpu/vhost-user-gpu.c |  6 +---
 contrib/vhost-user-gpu/vugbm.c  | 44 +++--
 3 files changed, 22 insertions(+), 30 deletions(-)

diff --git a/contrib/vhost-user-gpu/vugbm.h b/contrib/vhost-user-gpu/vugbm.h
index 66f1520764a6..82bc4934e1ba 100644
--- a/contrib/vhost-user-gpu/vugbm.h
+++ b/contrib/vhost-user-gpu/vugbm.h
@@ -54,7 +54,7 @@ struct vugbm_buffer {
 uint32_t format;
 };
 
-bool vugbm_device_init(struct vugbm_device *dev, int fd);
+void vugbm_device_init(struct vugbm_device *dev, int fd);
 void vugbm_device_destroy(struct vugbm_device *dev);
 
 bool vugbm_buffer_create(struct vugbm_buffer *buffer, struct vugbm_device *dev,
diff --git a/contrib/vhost-user-gpu/vhost-user-gpu.c 
b/contrib/vhost-user-gpu/vhost-user-gpu.c
index b27990ffdbc0..ef40fbccbbd9 100644
--- a/contrib/vhost-user-gpu/vhost-user-gpu.c
+++ b/contrib/vhost-user-gpu/vhost-user-gpu.c
@@ -1186,11 +1186,7 @@ main(int argc, char *argv[])
 exit(EXIT_FAILURE);
 }
 
-if (g.drm_rnode_fd >= 0) {
-if (!vugbm_device_init(&g.gdev, g.drm_rnode_fd)) {
-g_warning("Failed to init DRM device, using fallback path");
-}
-}
+vugbm_device_init(&g.gdev, g.drm_rnode_fd);
 
 if ((!!opt_socket_path + (opt_fdnum != -1)) != 1) {
 g_printerr("Please specify either --fd or --socket-path\n");
diff --git a/contrib/vhost-user-gpu/vugbm.c b/contrib/vhost-user-gpu/vugbm.c
index f5304ada2f1b..fb15d0372c25 100644
--- a/contrib/vhost-user-gpu/vugbm.c
+++ b/contrib/vhost-user-gpu/vugbm.c
@@ -199,55 +199,51 @@ vugbm_device_destroy(struct vugbm_device *dev)
 dev->device_destroy(dev);
 }
 
-bool
+void
 vugbm_device_init(struct vugbm_device *dev, int fd)
 {
-dev->fd = fd;
+assert(!dev->inited);
 
 #ifdef CONFIG_GBM
-dev->dev = gbm_create_device(fd);
-#endif
-
-if (0) {
-/* nothing */
+if (fd >= 0) {
+dev->dev = gbm_create_device(fd);
 }
-#ifdef CONFIG_GBM
-else if (dev->dev != NULL) {
+if (dev->dev != NULL) {
+dev->fd = fd;
 dev->alloc_bo = alloc_bo;
 dev->free_bo = free_bo;
 dev->get_fd = get_fd;
 dev->map_bo = map_bo;
 dev->unmap_bo = unmap_bo;
 dev->device_destroy = device_destroy;
+dev->inited = true;
 }
 #endif
 #ifdef CONFIG_MEMFD
-else if (g_file_test("/dev/udmabuf", G_FILE_TEST_EXISTS)) {
+if (!dev->inited && g_file_test("/dev/udmabuf", G_FILE_TEST_EXISTS)) {
 dev->fd = open("/dev/udmabuf", O_RDWR);
-if (dev->fd < 0) {
-return false;
+if (dev->fd >= 0) {
+g_debug("Using experimental udmabuf backend");
+dev->alloc_bo = udmabuf_alloc_bo;
+dev->free_bo = udmabuf_free_bo;
+dev->get_fd = udmabuf_get_fd;
+dev->map_bo = udmabuf_map_bo;
+dev->unmap_bo = udmabuf_unmap_bo;
+dev->device_destroy = udmabuf_device_destroy;
+dev->inited = true;
 }
-g_debug("Using experimental udmabuf backend");
-dev->alloc_bo = udmabuf_alloc_bo;
-dev->free_bo = udmabuf_free_bo;
-dev->get_fd = udmabuf_get_fd;
-dev->map_bo = udmabuf_map_bo;
-dev->unmap_bo = udmabuf_unmap_bo;
-dev->device_destroy = udmabuf_device_destroy;
 }
 #endif
-else {
+if (!dev->inited) {
 g_debug("Using mem fallback");
 dev->alloc_bo = mem_alloc_bo;
 dev->free_bo = mem_free_bo;
 dev->map_bo = mem_map_bo;
 dev->unmap_bo = mem_unmap_bo;
 dev->device_destroy = mem_device_destroy;
-return false;
+dev->inited = true;
 }
-
-dev->inited = true;
-return true;
+assert(dev->inited);
 }
 
 static bool
-- 
2.30.2

[PULL 2/9] vhost-user-gpu: glFlush before notifying clients

[Gerd Hoffmann]

From: Marc-André Lureau 

For similar reasons as commit 3af1671852 ("spice: flush on GL update
before notifying client"), vhost-user-gpu must ensure the GL state is
flushed before sharing its rendering result.

Signed-off-by: Marc-André Lureau 
Message-Id: <20210312100108.2706195-3-marcandre.lur...@redhat.com>
Signed-off-by: Gerd Hoffmann 
---
 contrib/vhost-user-gpu/virgl.c | 3 +++
 contrib/vhost-user-gpu/meson.build | 2 +-
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/contrib/vhost-user-gpu/virgl.c b/contrib/vhost-user-gpu/virgl.c
index 8bb3c563d975..9e6660c7ab87 100644
--- a/contrib/vhost-user-gpu/virgl.c
+++ b/contrib/vhost-user-gpu/virgl.c
@@ -16,6 +16,8 @@
 #include 
 #include "virgl.h"
 
+#include 
+
 void
 vg_virgl_update_cursor_data(VuGpu *g, uint32_t resource_id,
 gpointer data)
@@ -372,6 +374,7 @@ virgl_cmd_resource_flush(VuGpu *g,
 
 VUGPU_FILL_CMD(rf);
 
+glFlush();
 if (!rf.resource_id) {
 g_debug("bad resource id for flush..?");
 return;
diff --git a/contrib/vhost-user-gpu/meson.build 
b/contrib/vhost-user-gpu/meson.build
index 2fc2320b52fe..0ce1515a10e5 100644
--- a/contrib/vhost-user-gpu/meson.build
+++ b/contrib/vhost-user-gpu/meson.build
@@ -2,7 +2,7 @@ if 'CONFIG_TOOLS' in config_host and 'CONFIG_VIRGL' in 
config_host \
 and 'CONFIG_GBM' in config_host and 'CONFIG_LINUX' in config_host \
 and pixman.found()
   executable('vhost-user-gpu', files('vhost-user-gpu.c', 'virgl.c', 'vugbm.c'),
- dependencies: [qemuutil, pixman, gbm, virgl, vhost_user],
+ dependencies: [qemuutil, pixman, gbm, virgl, vhost_user, opengl],
  install: true,
  install_dir: get_option('libexecdir'))
 
-- 
2.30.2

[PULL 6/9] s390x: move S390_ADAPTER_SUPPRESSIBLE

[Gerd Hoffmann]

The definition S390_ADAPTER_SUPPRESSIBLE was moved to "cpu.h", per
suggestion of Thomas Huth. From interface design perspective, IMHO, not
a good thing as it belongs to the public interface of
css_register_io_adapters(). We did this because CONFIG_KVM requeires
NEED_CPU_H and Thomas, and other commenters did not like the
consequences of that.

Moving the interrupt related declarations to s390_flic.h was suggested
by Cornelia Huck.

Signed-off-by: Halil Pasic 
Signed-off-by: Gerd Hoffmann 
Reviewed-by: Halil Pasic 
Tested-by: Halil Pasic 
Message-Id: <20210317095622.2839895-2-kra...@redhat.com>
Signed-off-by: Gerd Hoffmann 
---
 include/hw/s390x/css.h   | 7 ---
 include/hw/s390x/s390_flic.h | 3 +++
 target/s390x/cpu.h   | 9 ++---
 3 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h
index 7901ab276ce9..bba7593d2eaa 100644
--- a/include/hw/s390x/css.h
+++ b/include/hw/s390x/css.h
@@ -12,7 +12,6 @@
 #ifndef CSS_H
 #define CSS_H
 
-#include "cpu.h"
 #include "hw/s390x/adapter.h"
 #include "hw/s390x/s390_flic.h"
 #include "hw/s390x/ioinst.h"
@@ -233,12 +232,6 @@ uint32_t css_get_adapter_id(CssIoAdapterType type, uint8_t 
isc);
 void css_register_io_adapters(CssIoAdapterType type, bool swap, bool maskable,
   uint8_t flags, Error **errp);
 
-#ifndef CONFIG_KVM
-#define S390_ADAPTER_SUPPRESSIBLE 0x01
-#else
-#define S390_ADAPTER_SUPPRESSIBLE KVM_S390_ADAPTER_SUPPRESSIBLE
-#endif
-
 #ifndef CONFIG_USER_ONLY
 SubchDev *css_find_subch(uint8_t m, uint8_t cssid, uint8_t ssid,
  uint16_t schid);
diff --git a/include/hw/s390x/s390_flic.h b/include/hw/s390x/s390_flic.h
index e91b15d2d6af..3907a13d0766 100644
--- a/include/hw/s390x/s390_flic.h
+++ b/include/hw/s390x/s390_flic.h
@@ -134,6 +134,9 @@ void s390_flic_init(void);
 S390FLICState *s390_get_flic(void);
 QEMUS390FLICState *s390_get_qemu_flic(S390FLICState *fs);
 S390FLICStateClass *s390_get_flic_class(S390FLICState *fs);
+void s390_crw_mchk(void);
+void s390_io_interrupt(uint16_t subchannel_id, uint16_t subchannel_nr,
+   uint32_t io_int_parm, uint32_t io_int_word);
 bool ais_needed(void *opaque);
 
 #endif /* HW_S390_FLIC_H */
diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index 468b4430f339..2464d4076c0a 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -40,6 +40,12 @@
 
 #define S390_MAX_CPUS 248
 
+#ifndef CONFIG_KVM
+#define S390_ADAPTER_SUPPRESSIBLE 0x01
+#else
+#define S390_ADAPTER_SUPPRESSIBLE KVM_S390_ADAPTER_SUPPRESSIBLE
+#endif
+
 typedef struct PSW {
 uint64_t mask;
 uint64_t addr;
@@ -811,9 +817,6 @@ int cpu_s390x_signal_handler(int host_signum, void *pinfo, 
void *puc);
 
 
 /* interrupt.c */
-void s390_crw_mchk(void);
-void s390_io_interrupt(uint16_t subchannel_id, uint16_t subchannel_nr,
-   uint32_t io_int_parm, uint32_t io_int_word);
 #define RA_IGNORED  0
 void s390_program_interrupt(CPUS390XState *env, uint32_t code, uintptr_t ra);
 /* service interrupts are floating therefore we must not pass an cpustate */
-- 
2.30.2

[PULL 1/9] usb: Remove "-usbdevice ccid"

[Gerd Hoffmann]

From: Thomas Huth 

"-usbdevice ccid" was not documented and -usbdevice itself was marked
as deprecated before QEMU v6.0. And searching for "-usbdevice ccid"
in the internet does not show any useful results, so likely nobody
was using the ccid device via the -usbdevice option. Remove it now.

Signed-off-by: Thomas Huth 
Message-Id: <20210311092829.1479051-1-th...@redhat.com>
Signed-off-by: Gerd Hoffmann 
---
 hw/usb/dev-smartcard-reader.c| 1 -
 docs/system/removed-features.rst | 6 ++
 qemu-options.hx  | 3 ---
 3 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/hw/usb/dev-smartcard-reader.c b/hw/usb/dev-smartcard-reader.c
index 80109fa55168..bc3d94092a23 100644
--- a/hw/usb/dev-smartcard-reader.c
+++ b/hw/usb/dev-smartcard-reader.c
@@ -1492,7 +1492,6 @@ static void ccid_register_types(void)
 type_register_static(&ccid_bus_info);
 type_register_static(&ccid_card_type_info);
 type_register_static(&ccid_info);
-usb_legacy_register(TYPE_USB_CCID_DEV, "ccid", NULL);
 }
 
 type_init(ccid_register_types)
diff --git a/docs/system/removed-features.rst b/docs/system/removed-features.rst
index f28387f183cc..29e90601a51a 100644
--- a/docs/system/removed-features.rst
+++ b/docs/system/removed-features.rst
@@ -120,6 +120,12 @@ Drives with interface types other than ``if=none`` are for 
onboard
 devices.  Drives the board doesn't pick up can no longer be used with
 -device.  Use ``if=none`` instead.
 
+``-usbdevice ccid`` (removed in 6.0)
+'
+
+This option was undocumented and not used in the field.
+Use `-device usb-ccid`` instead.
+
 
 QEMU Machine Protocol (QMP) commands
 
diff --git a/qemu-options.hx b/qemu-options.hx
index d60a03d3a973..fd21002bd61d 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -1743,9 +1743,6 @@ SRST
 corresponding ``braille`` chardev automatically beside the
 ``usb-braille`` USB device).
 
-``ccid``
-Smartcard reader device
-
 ``keyboard``
 Standard USB keyboard. Will override the PS/2 keyboard (if present).
 
-- 
2.30.2

[PULL 7/9] s390x: add have_virtio_ccw

[Gerd Hoffmann]

Introduce a symbol which can be used to prevent ccw modules
being loaded into system emulators without ccw support.

Signed-off-by: Gerd Hoffmann 
Reviewed-by: Halil Pasic 
Tested-by: Halil Pasic 
Message-Id: <20210317095622.2839895-3-kra...@redhat.com>
Signed-off-by: Gerd Hoffmann 
---
 hw/s390x/virtio-ccw.h | 5 +
 hw/s390x/virtio-ccw.c | 2 ++
 2 files changed, 7 insertions(+)

diff --git a/hw/s390x/virtio-ccw.h b/hw/s390x/virtio-ccw.h
index 49a2b8ca42df..0168232e3b8d 100644
--- a/hw/s390x/virtio-ccw.h
+++ b/hw/s390x/virtio-ccw.h
@@ -63,6 +63,11 @@ typedef struct VirtioBusClass VirtioCcwBusClass;
 DECLARE_OBJ_CHECKERS(VirtioCcwBusState, VirtioCcwBusClass,
  VIRTIO_CCW_BUS, TYPE_VIRTIO_CCW_BUS)
 
+/*
+ * modules can reference this symbol to avoid being loaded
+ * into system emulators without ccw support
+ */
+extern bool have_virtio_ccw;
 
 struct VirtIOCCWDeviceClass {
 CCWDeviceClass parent_class;
diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index 06c06056814b..314ed7b24566 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -35,6 +35,8 @@
 
 #define NR_CLASSIC_INDICATOR_BITS 64
 
+bool have_virtio_ccw = true;
+
 static int virtio_ccw_dev_post_load(void *opaque, int version_id)
 {
 VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(opaque);
-- 
2.30.2

[PULL 5/9] hw/usb/hcd-ehci-sysbus: Free USBPacket on instance finalize()

[Gerd Hoffmann]

From: Philippe Mathieu-Daudé 

When building with --enable-sanitizers we get:

  Direct leak of 32 byte(s) in 2 object(s) allocated from:
  #0 0x5618479ec7cf in malloc (qemu-system-aarch64+0x233b7cf)
  #1 0x7f675745f958 in g_malloc (/lib64/libglib-2.0.so.0+0x58958)
  #2 0x561847f02ca2 in usb_packet_init hw/usb/core.c:531:5
  #3 0x561848df4df4 in usb_ehci_init hw/usb/hcd-ehci.c:2575:5
  #4 0x561847c119ac in ehci_sysbus_init hw/usb/hcd-ehci-sysbus.c:73:5
  #5 0x56184a5bdab8 in object_init_with_type qom/object.c:375:9
  #6 0x56184a5bd955 in object_init_with_type qom/object.c:371:9
  #7 0x56184a5a2bda in object_initialize_with_type qom/object.c:517:5
  #8 0x56184a5a24d5 in object_initialize qom/object.c:536:5
  #9 0x56184a5a2f6c in object_initialize_child_with_propsv 
qom/object.c:566:5
  #10 0x56184a5a2e60 in object_initialize_child_with_props 
qom/object.c:549:10
  #11 0x56184a5a3a1e in object_initialize_child_internal qom/object.c:603:5
  #12 0x561849542d18 in npcm7xx_init hw/arm/npcm7xx.c:427:5

Similarly to commit d710e1e7bd3 ("usb: ehci: fix memory leak in
ehci"), fix by calling usb_ehci_finalize() to free the USBPacket.

Fixes: 7341ea075c0
Signed-off-by: Philippe Mathieu-Daudé 
Reviewed-by: Thomas Huth 
Message-Id: <20210323183701.281152-1-f4...@amsat.org>
Signed-off-by: Gerd Hoffmann 
---
 hw/usb/hcd-ehci-sysbus.c | 9 +
 1 file changed, 9 insertions(+)

diff --git a/hw/usb/hcd-ehci-sysbus.c b/hw/usb/hcd-ehci-sysbus.c
index e3758db1b18e..a12e21884894 100644
--- a/hw/usb/hcd-ehci-sysbus.c
+++ b/hw/usb/hcd-ehci-sysbus.c
@@ -74,6 +74,14 @@ static void ehci_sysbus_init(Object *obj)
 sysbus_init_mmio(d, &s->mem);
 }
 
+static void ehci_sysbus_finalize(Object *obj)
+{
+EHCISysBusState *i = SYS_BUS_EHCI(obj);
+EHCIState *s = &i->ehci;
+
+usb_ehci_finalize(s);
+}
+
 static void ehci_sysbus_class_init(ObjectClass *klass, void *data)
 {
 DeviceClass *dc = DEVICE_CLASS(klass);
@@ -94,6 +102,7 @@ static const TypeInfo ehci_type_info = {
 .parent= TYPE_SYS_BUS_DEVICE,
 .instance_size = sizeof(EHCISysBusState),
 .instance_init = ehci_sysbus_init,
+.instance_finalize = ehci_sysbus_finalize,
 .abstract  = true,
 .class_init= ehci_sysbus_class_init,
 .class_size= sizeof(SysBusEHCIClass),
-- 
2.30.2

[PULL 4/9] vhost-user-gpu: fix cursor move/update

[Gerd Hoffmann]

From: Marc-André Lureau 

"move" is incorrectly initialized.

Fix it by using a switch statement and also treating unknown commands
with a fallback.

Signed-off-by: Marc-André Lureau 
Message-Id: <20210312100108.2706195-5-marcandre.lur...@redhat.com>
Signed-off-by: Gerd Hoffmann 
---
 contrib/vhost-user-gpu/vhost-user-gpu.c | 18 --
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/contrib/vhost-user-gpu/vhost-user-gpu.c 
b/contrib/vhost-user-gpu/vhost-user-gpu.c
index ef40fbccbbd9..f73f292c9f72 100644
--- a/contrib/vhost-user-gpu/vhost-user-gpu.c
+++ b/contrib/vhost-user-gpu/vhost-user-gpu.c
@@ -892,11 +892,8 @@ update_cursor_data_simple(VuGpu *g, uint32_t resource_id, 
gpointer data)
 static void
 vg_process_cursor_cmd(VuGpu *g, struct virtio_gpu_update_cursor *cursor)
 {
-bool move = cursor->hdr.type != VIRTIO_GPU_CMD_MOVE_CURSOR;
-
-g_debug("%s move:%d\n", G_STRFUNC, move);
-
-if (move) {
+switch (cursor->hdr.type) {
+case VIRTIO_GPU_CMD_MOVE_CURSOR: {
 VhostUserGpuMsg msg = {
 .request = cursor->resource_id ?
 VHOST_USER_GPU_CURSOR_POS : VHOST_USER_GPU_CURSOR_POS_HIDE,
@@ -907,8 +904,11 @@ vg_process_cursor_cmd(VuGpu *g, struct 
virtio_gpu_update_cursor *cursor)
 .y = cursor->pos.y,
 }
 };
+g_debug("%s: move", G_STRFUNC);
 vg_send_msg(g, &msg, -1);
-} else {
+break;
+}
+case VIRTIO_GPU_CMD_UPDATE_CURSOR: {
 VhostUserGpuMsg msg = {
 .request = VHOST_USER_GPU_CURSOR_UPDATE,
 .size = sizeof(VhostUserGpuCursorUpdate),
@@ -922,6 +922,7 @@ vg_process_cursor_cmd(VuGpu *g, struct 
virtio_gpu_update_cursor *cursor)
 .hot_y = cursor->hot_y,
 }
 };
+g_debug("%s: update", G_STRFUNC);
 if (g->virgl) {
 vg_virgl_update_cursor_data(g, cursor->resource_id,
 msg.payload.cursor_update.data);
@@ -930,6 +931,11 @@ vg_process_cursor_cmd(VuGpu *g, struct 
virtio_gpu_update_cursor *cursor)
   msg.payload.cursor_update.data);
 }
 vg_send_msg(g, &msg, -1);
+break;
+}
+default:
+g_debug("%s: unknown cmd %d", G_STRFUNC, cursor->hdr.type);
+break;
 }
 }
 
-- 
2.30.2

[PULL 8/9] s390x: modularize virtio-gpu-ccw

[Gerd Hoffmann]

Since the virtio-gpu-ccw device depends on the hw-display-virtio-gpu
module, which provides the type virtio-gpu-device, packaging the
hw-display-virtio-gpu module as a separate package that may or may not
be installed along with the qemu package leads to problems. Namely if
the hw-display-virtio-gpu is absent, qemu continues to advertise
virtio-gpu-ccw, but it aborts not only when one attempts using
virtio-gpu-ccw, but also when libvirtd's capability probing tries
to instantiate the type to introspect it.

Let us thus introduce a module named hw-s390x-virtio-gpu-ccw that
is going to provide the virtio-gpu-ccw device. The hw-s390x prefix
was chosen because it is not a portable device.

With virtio-gpu-ccw built as a module, the correct way to package a
modularized qemu is to require that hw-display-virtio-gpu must be
installed whenever the module hw-s390x-virtio-gpu-ccw.

Signed-off-by: Halil Pasic 
Signed-off-by: Gerd Hoffmann 
Reviewed-by: Halil Pasic 
Tested-by: Halil Pasic 
Message-Id: <20210317095622.2839895-4-kra...@redhat.com>
Signed-off-by: Gerd Hoffmann 
---
 hw/s390x/virtio-ccw-gpu.c | 4 +++-
 util/module.c | 1 +
 hw/s390x/meson.build  | 8 +++-
 3 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/hw/s390x/virtio-ccw-gpu.c b/hw/s390x/virtio-ccw-gpu.c
index c301e2586bde..75a9e4bb3908 100644
--- a/hw/s390x/virtio-ccw-gpu.c
+++ b/hw/s390x/virtio-ccw-gpu.c
@@ -62,7 +62,9 @@ static const TypeInfo virtio_ccw_gpu = {
 
 static void virtio_ccw_gpu_register(void)
 {
-type_register_static(&virtio_ccw_gpu);
+if (have_virtio_ccw) {
+type_register_static(&virtio_ccw_gpu);
+}
 }
 
 type_init(virtio_ccw_gpu_register)
diff --git a/util/module.c b/util/module.c
index c65060c167df..cbe89fede628 100644
--- a/util/module.c
+++ b/util/module.c
@@ -304,6 +304,7 @@ static struct {
 { "virtio-gpu-pci-base",   "hw-", "display-virtio-gpu-pci" },
 { "virtio-gpu-pci","hw-", "display-virtio-gpu-pci" },
 { "vhost-user-gpu-pci","hw-", "display-virtio-gpu-pci" },
+{ "virtio-gpu-ccw","hw-", "s390x-virtio-gpu-ccw"   },
 { "virtio-vga-base",   "hw-", "display-virtio-vga"},
 { "virtio-vga","hw-", "display-virtio-vga"},
 { "vhost-user-vga","hw-", "display-virtio-vga"},
diff --git a/hw/s390x/meson.build b/hw/s390x/meson.build
index 91495b563146..327e9c93afa9 100644
--- a/hw/s390x/meson.build
+++ b/hw/s390x/meson.build
@@ -34,7 +34,6 @@ virtio_ss.add(files('virtio-ccw.c'))
 virtio_ss.add(when: 'CONFIG_VIRTIO_BALLOON', if_true: 
files('virtio-ccw-balloon.c'))
 virtio_ss.add(when: 'CONFIG_VIRTIO_BLK', if_true: files('virtio-ccw-blk.c'))
 virtio_ss.add(when: 'CONFIG_VIRTIO_CRYPTO', if_true: 
files('virtio-ccw-crypto.c'))
-virtio_ss.add(when: 'CONFIG_VIRTIO_GPU', if_true: files('virtio-ccw-gpu.c'))
 virtio_ss.add(when: 'CONFIG_VIRTIO_INPUT', if_true: 
files('virtio-ccw-input.c'))
 virtio_ss.add(when: 'CONFIG_VIRTIO_NET', if_true: files('virtio-ccw-net.c'))
 virtio_ss.add(when: 'CONFIG_VIRTIO_RNG', if_true: files('virtio-ccw-rng.c'))
@@ -48,3 +47,10 @@ virtio_ss.add(when: 'CONFIG_VHOST_USER_FS', if_true: 
files('vhost-user-fs-ccw.c'
 s390x_ss.add_all(when: 'CONFIG_VIRTIO_CCW', if_true: virtio_ss)
 
 hw_arch += {'s390x': s390x_ss}
+
+hw_s390x_modules = {}
+virtio_gpu_ccw_ss = ss.source_set()
+virtio_gpu_ccw_ss.add(when: ['CONFIG_VIRTIO_GPU', 'CONFIG_VIRTIO_CCW'],
+  if_true: [files('virtio-ccw-gpu.c'), pixman])
+hw_s390x_modules += {'virtio-gpu-ccw': virtio_gpu_ccw_ss}
+modules += {'hw-s390x': hw_s390x_modules}
-- 
2.30.2

[PULL 9/9] hw/usb/hcd-ehci: Fix crash when showing help of EHCI devices

[Gerd Hoffmann]

From: Thomas Huth 

QEMU crashes with certain targets when trying to show the help
output of EHCI devices:

$ ./qemu-system-aarch64 -device ich9-usb-ehci1,help
qemu-system-aarch64: ../../devel/qemu/softmmu/physmem.c:1154: phys_section_add:
 Assertion `map->sections_nb < TARGET_PAGE_SIZE' failed.
Aborted (core dumped)

This happens because the device is doing things at "instance_init" time
that should be done at "realize" time instead. So move the related code
to the realize() function instead. (NB: This now also matches the
memory_region_del_subregion() calls which are done in usb_ehci_unrealize(),
and not during finalize()).

Suggested-by: Richard Henderson 
Signed-off-by: Thomas Huth 
Message-Id: <20210326095155.1994604-1-th...@redhat.com>
Signed-off-by: Gerd Hoffmann 
---
 hw/usb/hcd-ehci.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
index f71af0ad2d8f..6caa7ac6c28f 100644
--- a/hw/usb/hcd-ehci.c
+++ b/hw/usb/hcd-ehci.c
@@ -2514,6 +2514,11 @@ void usb_ehci_realize(EHCIState *s, DeviceState *dev, 
Error **errp)
 return;
 }
 
+memory_region_add_subregion(&s->mem, s->capsbase, &s->mem_caps);
+memory_region_add_subregion(&s->mem, s->opregbase, &s->mem_opreg);
+memory_region_add_subregion(&s->mem, s->opregbase + s->portscbase,
+&s->mem_ports);
+
 usb_bus_new(&s->bus, sizeof(s->bus), s->companion_enable ?
 &ehci_bus_ops_companion : &ehci_bus_ops_standalone, dev);
 for (i = 0; i < s->portnr; i++) {
@@ -2581,11 +2586,6 @@ void usb_ehci_init(EHCIState *s, DeviceState *dev)
   "operational", s->portscbase);
 memory_region_init_io(&s->mem_ports, OBJECT(dev), &ehci_mmio_port_ops, s,
   "ports", 4 * s->portnr);
-
-memory_region_add_subregion(&s->mem, s->capsbase, &s->mem_caps);
-memory_region_add_subregion(&s->mem, s->opregbase, &s->mem_opreg);
-memory_region_add_subregion(&s->mem, s->opregbase + s->portscbase,
-&s->mem_ports);
 }
 
 void usb_ehci_finalize(EHCIState *s)
-- 
2.30.2

Re: [RFC PATCH-for-6.1 00/10] hw/misc: Add memory_region_add_subregion_aliased() helper

[Richard Henderson]

On 3/25/21 6:27 PM, Philippe Mathieu-Daudé wrote:

This series introduce the memory_region_add_subregion_aliased()
helper which basically create a device which maps a subregion
multiple times.


I must say, the example mtree changes are persuasive.
I'll look in more detail later.


r~

Re: [PATCH v3] i386/cpu_dump: support AVX512 ZMM regs dump

[Richard Henderson]

On 3/25/21 7:47 PM, Robert Hoo wrote:

On Thu, 2021-03-25 at 06:39 -0600, Richard Henderson wrote:

On 3/24/21 9:15 PM, Robert Hoo wrote:

+} else if (env->xcr0 & XFEATURE_AVX) {


This is normally a 2-bit test.


I beg your pardon. What 2 bits?


I forget the names, but isn't the usual test xcr0 & 6 == 6?


6 stands for SSE state-component and AVX state-component.
I'm not sure about this.
Can you remember where did you this "xcr0 & 6 == 6"? I can look into
that.


IA-64 and IA32 Software developers manual, Vol 1 Basic Architecture, Section 
14.3 Detection of AVX instructions.



r~

Re: [PATCH] linux-user: allow NULL msg in recvfrom

[Laurent Vivier]

Le 26/03/2021 à 05:05, Zach Reizner a écrit :
> The kernel allows a NULL msg in recvfrom so that he size of the next
> message may be queried before allocating a correctly sized buffer. This
> change allows the syscall translator to pass along the NULL msg pointer
> instead of returning early with EFAULT.
> 
> Signed-off-by: Zach Reizner 
> ---
>  linux-user/syscall.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 1e508576c7..332544b43c 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -3680,8 +3680,6 @@ static abi_long do_recvfrom(int fd, abi_ulong
> msg, size_t len, int flags,
>  abi_long ret;
> 
>  host_msg = lock_user(VERIFY_WRITE, msg, len, 0);
> -if (!host_msg)
> -return -TARGET_EFAULT;
>  if (target_addr) {
>  if (get_user_u32(addrlen, target_addrlen)) {
>  ret = -TARGET_EFAULT;
> 

Applied to my linux-user-for-6.0 branch

Thanks,
Laurent

Re: [PATCH] linux-user: allow NULL msg in recvfrom

[Peter Maydell]

On Fri, 26 Mar 2021 at 13:24, Laurent Vivier  wrote:
>
> Le 26/03/2021 à 05:05, Zach Reizner a écrit :
> > The kernel allows a NULL msg in recvfrom so that he size of the next
> > message may be queried before allocating a correctly sized buffer. This
> > change allows the syscall translator to pass along the NULL msg pointer
> > instead of returning early with EFAULT.
> >
> > Signed-off-by: Zach Reizner 
> > ---
> >  linux-user/syscall.c | 2 --
> >  1 file changed, 2 deletions(-)
> >
> > diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> > index 1e508576c7..332544b43c 100644
> > --- a/linux-user/syscall.c
> > +++ b/linux-user/syscall.c
> > @@ -3680,8 +3680,6 @@ static abi_long do_recvfrom(int fd, abi_ulong
> > msg, size_t len, int flags,
> >  abi_long ret;
> >
> >  host_msg = lock_user(VERIFY_WRITE, msg, len, 0);
> > -if (!host_msg)
> > -return -TARGET_EFAULT;
> >  if (target_addr) {
> >  if (get_user_u32(addrlen, target_addrlen)) {
> >  ret = -TARGET_EFAULT;
> >
>
> Applied to my linux-user-for-6.0 branch

Doesn't this mean we'll now incorrectly treat "guest passed
a bad address" the same as "guest passed NULL" ? lock_user()
returns NULL for errors, so if you need to handle NULL input
specially you want something like

   if (!msg) {
   host_msg = NULL;
   } else {
   host_msg = lock_user(VERIFY_WRITE, msg, len, 0);
   if (!host_msg) {
   return -TARGET_EFAULT;
   }
   }

I think ?

thanks
-- PMM

Re: [PATCH 1/2] hw/riscv: sifive_u: Allow passing custom DTB

[Bin Meng]

On Wed, Mar 24, 2021 at 9:41 PM Bin Meng  wrote:
>
> Hi Anup,
>
> On Thu, Oct 22, 2020 at 1:34 PM Anup Patel  wrote:
> >
> > Extend sifive_u machine to allow passing custom DTB using "-dtb"
> > command-line parameter. This will help users pass modified DTB
> > or Linux SiFive DTB to sifive_u machine.
> >
> > Signed-off-by: Anup Patel 
> > ---
> >  hw/riscv/sifive_u.c | 28 
> >  1 file changed, 20 insertions(+), 8 deletions(-)
> >
>
> I am using the following command to boot a Linux v5.11 kernel, but it hangs 
> at:
>
> $ qemu-system-riscv64 -M sifive_u -smp 5 -m 8G -display none -serial
> stdio -kernel arch/riscv/boot/Image -dtb
> arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dtb -append
> "earlycon=sbi console=ttySIF0"
>
> [0.00] smp: Bringing up secondary CPUs ...
>
> Removing -dtb makes the kernel continue booting.
>
> I am not sure what's missing ofusing "-dtb"?

I figured out what's missing, and will send a patch on documentation soon.

Regards,
Bin

[PATCH v3 1/1] docs/devel: Add VFIO device migration documentation

[Tarun Gupta]

Document interfaces used for VFIO device migration. Added flow of state changes
during live migration with VFIO device. Tested by building docs with the new
vfio-migration.rst file.

v3:
- Add introductory line about VM migration in general.
- Remove occurcences of vfio_pin_pages() to describe pinning.
- Incorporated comments from v2

v2:
- Included the new vfio-migration.rst file in index.rst
- Updated dirty page tracking section, also added details about
  'pre-copy-dirty-page-tracking' opt-out option.
- Incorporated comments around wording of doc.

Signed-off-by: Tarun Gupta 
Signed-off-by: Kirti Wankhede 
---
 MAINTAINERS   |   1 +
 docs/devel/index.rst  |   1 +
 docs/devel/vfio-migration.rst | 143 ++
 3 files changed, 145 insertions(+)
 create mode 100644 docs/devel/vfio-migration.rst

diff --git a/MAINTAINERS b/MAINTAINERS
index 738786146d..a2a80eee59 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1801,6 +1801,7 @@ M: Alex Williamson 
 S: Supported
 F: hw/vfio/*
 F: include/hw/vfio/
+F: docs/devel/vfio-migration.rst
 
 vfio-ccw
 M: Cornelia Huck 
diff --git a/docs/devel/index.rst b/docs/devel/index.rst
index ae664da00c..5330f1ca1d 100644
--- a/docs/devel/index.rst
+++ b/docs/devel/index.rst
@@ -39,3 +39,4 @@ Contents:
qom
block-coroutine-wrapper
multi-process
+   vfio-migration
diff --git a/docs/devel/vfio-migration.rst b/docs/devel/vfio-migration.rst
new file mode 100644
index 00..24cb55991a
--- /dev/null
+++ b/docs/devel/vfio-migration.rst
@@ -0,0 +1,143 @@
+=
+VFIO device Migration
+=
+
+Migration of virtual machine involves saving the state for each device that
+the guest is running on source host and restoring this saved state on the
+destination host. This document details how saving and restoring of VFIO
+devices is done in QEMU.
+
+Migration of VFIO devices consists of two phases: the optional pre-copy phase,
+and the stop-and-copy phase. The pre-copy phase is iterative and allows to
+accommodate VFIO devices that have a large amount of data that needs to be
+transferred. The iterative pre-copy phase of migration allows for the guest to
+continue whilst the VFIO device state is transferred to the destination, this
+helps to reduce the total downtime of the VM. VFIO devices can choose to skip
+the pre-copy phase of migration by returning pending_bytes as zero during the
+pre-copy phase.
+
+A detailed description of the UAPI for VFIO device migration can be found in
+the comment for the ``vfio_device_migration_info`` structure in the header
+file linux-headers/linux/vfio.h.
+
+VFIO device hooks for iterative approach:
+
+* A ``save_setup`` function that sets up the migration region, sets _SAVING
+  flag in the VFIO device state and informs the VFIO IOMMU module to start
+  dirty page tracking.
+
+* A ``load_setup`` function that sets up the migration region on the
+  destination and sets _RESUMING flag in the VFIO device state.
+
+* A ``save_live_pending`` function that reads pending_bytes from the vendor
+  driver, which indicates the amount of data that the vendor driver has yet to
+  save for the VFIO device.
+
+* A ``save_live_iterate`` function that reads the VFIO device's data from the
+  vendor driver through the migration region during iterative phase.
+
+* A ``save_live_complete_precopy`` function that resets _RUNNING flag from the
+  VFIO device state, saves the device config space, if any, and iteratively
+  copies the remaining data for the VFIO device until the vendor driver
+  indicates that no data remains (pending bytes is zero).
+
+* A ``load_state`` function that loads the config section and the data
+  sections that are generated by the save functions above
+
+* ``cleanup`` functions for both save and load that perform any migration
+  related cleanup, including unmapping the migration region
+
+A VM state change handler is registered to change the VFIO device state when
+the VM state changes.
+
+Similarly, a migration state change notifier is registered to get a
+notification on migration state change. These states are translated to the
+corresponding VFIO device state and conveyed to the vendor driver.
+
+System memory dirty pages tracking
+--
+
+A ``log_sync`` memory listener callback marks those system memory pages
+as dirty which are used for DMA by the VFIO device. The dirty pages bitmap is
+queried per container. All pages pinned by the vendor driver through external
+APIs have to be marked as dirty during migration. When there are CPU writes,
+CPU dirty page tracking can identify dirtied pages, but any page pinned by the
+vendor driver can also be written by device. There is currently no device or
+IOMMU support for dirty page tracking in hardware.
+
+By default, dirty pages are tracked when the device is in pre-copy as well as
+stop-and-copy phase. So, a page pinned by vendor driver will be copied to
+destination in both 

Re: [RFC v11 45/55] target/arm: cpu-sve: new module

[Claudio Fontana]

On 3/25/21 7:48 PM, Claudio Fontana wrote:
> On 3/25/21 7:40 PM, Richard Henderson wrote:
>> On 3/23/21 9:46 AM, Claudio Fontana wrote:
>>> extract the SVE-related cpu object properties and functions,
>>> and move them to a separate module.
>>>
>>> Disentangle SVE from pauth that is a separate, TCG-only feature.
>>>
>>> Signed-off-by: Claudio Fontana 
>>> ---
>>>   target/arm/cpu-sve.h |  40 +
>>>   target/arm/cpu.h |  22 +--
>>>   target/arm/cpu-sve.c | 360 +++
>>>   target/arm/cpu.c |   5 +-
>>>   target/arm/cpu64.c   | 333 +---
>>>   target/arm/kvm/kvm-cpu.c |   2 +-
>>>   target/arm/meson.build   |   1 +
>>>   7 files changed, 417 insertions(+), 346 deletions(-)
>>>   create mode 100644 target/arm/cpu-sve.h
>>>   create mode 100644 target/arm/cpu-sve.c
>>>
>>> diff --git a/target/arm/cpu-sve.h b/target/arm/cpu-sve.h
>>> new file mode 100644
>>> index 00..b1be575265
>>> --- /dev/null
>>> +++ b/target/arm/cpu-sve.h
>>> @@ -0,0 +1,40 @@
>>> +/*
>>> + * QEMU AArch64 CPU SVE Extensions for TARGET_AARCH64
>>> + *
>>> + * Copyright (c) 2013 Linaro Ltd
>>> + *
>>> + * This program is free software; you can redistribute it and/or
>>> + * modify it under the terms of the GNU General Public License
>>> + * as published by the Free Software Foundation; either version 2
>>> + * of the License, or (at your option) any later version.
>>> + *
>>> + * This program is distributed in the hope that it will be useful,
>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>> + * GNU General Public License for more details.
>>> + *
>>> + * You should have received a copy of the GNU General Public License
>>> + * along with this program; if not, see
>>> + * 
>>> + */
>>> +
>>> +#ifndef CPU_SVE_H
>>> +#define CPU_SVE_H
>>> +
>>> +/* note: SVE is an AARCH64-only option, only include this for 
>>> TARGET_AARCH64 */
>>> +
>>> +/* called by arm_cpu_finalize_features in realizefn */
>>> +void cpu_sve_finalize_features(ARMCPU *cpu, Error **errp);
>>> +
>>> +/* add the CPU SVE properties */
>>> +void cpu_sve_add_props(Object *obj);
>>> +
>>> +/* add the CPU SVE properties specific to the "MAX" CPU */
>>> +void cpu_sve_add_props_max(Object *obj);
>>> +
>>> +/* In AArch32 mode, predicate registers do not exist at all.  */
>>> +typedef struct ARMPredicateReg {
>>> +uint64_t p[DIV_ROUND_UP(2 * ARM_MAX_VQ, 8)] QEMU_ALIGNED(16);
>>> +} ARMPredicateReg;
>>
>> I don't agree with moving this out of cpu.h, next to the rest of the vector 
>> definitions.
>>
>> I agree that we should be using more files, but if we're going to have an 
>> cpu-sve.c, why did some of the sve functions go into cpu-common.c?


actually, now that the option of making those SVE functions in cpu-common.c 
TARGET_AARCH64-specific is open,
we could attempt to import them in cpu-sve.

I'll give it a try, lets see the result.


> 
> maybe cpu-sve-props.c would be a better name, it really contains only cpu sve 
> properties code.
> 
>>
>> I don't agree with moving functions and renaming them simultaneously.  I'm 
>> not 
>> even sure why you're renaming them, or why you've suddenly chosen 
>> "cpu_sve_*" 
>> as the prefix to use.
>>
>>
>> r~
>>
> 
> I think the idea was trying to create a cpu_sve module handling everything 
> related to sve,
> and consistently using the name of the module as the prefix.
> 
> It might be too early to attempt that anyway; as you noted, there is other 
> SVE-related functionality all over the place,
> so better to revisit this.
> 
> I'd suggest renaming this to cpu_sve_props, and moving everything not props 
> related out of it.
> 
> Thanks,
> 
> Claudio
> 
> 
> 
> 

Re: [PATCH for-6.0] hw/timer/renesas_tmr: Add default-case asserts in read_tcnt()

[Peter Maydell]

ping for review?

thanks
-- PMM

On Fri, 19 Mar 2021 at 16:24, Peter Maydell  wrote:
>
> In commit 81b3ddaf8772ec we fixed a use of uninitialized data
> in read_tcnt(). However this change wasn't enough to placate
> Coverity, which is not smart enough to see that if we read a
> 2 bit field and then handle cases 0, 1, 2 and 3 then there cannot
> be a flow of execution through the switch default. Add explicit
> default cases which assert that they can't be reached, which
> should help silence Coverity.
>
> Signed-off-by: Peter Maydell 
> ---
>  hw/timer/renesas_tmr.c | 4 
>  1 file changed, 4 insertions(+)
>
> diff --git a/hw/timer/renesas_tmr.c b/hw/timer/renesas_tmr.c
> index eed39917fec..d96002e1ee6 100644
> --- a/hw/timer/renesas_tmr.c
> +++ b/hw/timer/renesas_tmr.c
> @@ -146,6 +146,8 @@ static uint16_t read_tcnt(RTMRState *tmr, unsigned size, 
> int ch)
>  case CSS_CASCADING:
>  tcnt[1] = tmr->tcnt[1];
>  break;
> +default:
> +g_assert_not_reached();
>  }
>  switch (FIELD_EX8(tmr->tccr[0], TCCR, CSS)) {
>  case CSS_INTERNAL:
> @@ -159,6 +161,8 @@ static uint16_t read_tcnt(RTMRState *tmr, unsigned size, 
> int ch)
>  case CSS_EXTERNAL: /* QEMU doesn't implement this */
>  tcnt[0] = tmr->tcnt[0];
>  break;
> +default:
> +g_assert_not_reached();
>  }
>  } else {
>  tcnt[0] = tmr->tcnt[0];
> --
> 2.20.1

Re: [PATCH] linux-user: allow NULL msg in recvfrom

[Laurent Vivier]

Le 26/03/2021 à 14:28, Peter Maydell a écrit :
> On Fri, 26 Mar 2021 at 13:24, Laurent Vivier  wrote:
>>
>> Le 26/03/2021 à 05:05, Zach Reizner a écrit :
>>> The kernel allows a NULL msg in recvfrom so that he size of the next
>>> message may be queried before allocating a correctly sized buffer. This
>>> change allows the syscall translator to pass along the NULL msg pointer
>>> instead of returning early with EFAULT.
>>>
>>> Signed-off-by: Zach Reizner 
>>> ---
>>>  linux-user/syscall.c | 2 --
>>>  1 file changed, 2 deletions(-)
>>>
>>> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
>>> index 1e508576c7..332544b43c 100644
>>> --- a/linux-user/syscall.c
>>> +++ b/linux-user/syscall.c
>>> @@ -3680,8 +3680,6 @@ static abi_long do_recvfrom(int fd, abi_ulong
>>> msg, size_t len, int flags,
>>>  abi_long ret;
>>>
>>>  host_msg = lock_user(VERIFY_WRITE, msg, len, 0);
>>> -if (!host_msg)
>>> -return -TARGET_EFAULT;
>>>  if (target_addr) {
>>>  if (get_user_u32(addrlen, target_addrlen)) {
>>>  ret = -TARGET_EFAULT;
>>>
>>
>> Applied to my linux-user-for-6.0 branch
> 
> Doesn't this mean we'll now incorrectly treat "guest passed
> a bad address" the same as "guest passed NULL" ? lock_user()
> returns NULL for errors, so if you need to handle NULL input
> specially you want something like
> 
>if (!msg) {
>host_msg = NULL;
>} else {
>host_msg = lock_user(VERIFY_WRITE, msg, len, 0);
>if (!host_msg) {
>return -TARGET_EFAULT;
>}
>}
> 
> I think ?

Yes, you're right.

Zach, could you update your patch?

Thanks,
Laurent

Re: [PATCH v2 0/5] qemu-iotests: quality of life improvements

[Max Reitz]

On 23.03.21 19:19, Paolo Bonzini wrote:

This series adds a few usability improvements to qemu-iotests, in
particular:

- arguments can be passed to Python unittests scripts, for example
   to run only a subset of the test cases (patches 1-2)

- it is possible to do "./check -- ../../../tests/qemu-iotests/055 args..."
   and specify arbitrary arguments to be passed to a single test script.
   This allows to take advantage of the previous feature and ease debugging
   of Python tests.

Paolo

v1->v2: patches 1-2 are a rewrite of v1's patch 1
 moved print_env change to patch 4
 do not use argparse.REMAINDER


I’m afraid the changes to iotests.py don’t pass the scrutiny of iotest 
297, so I’m going to have to remove the series again. :/



=== pylint ===
* Module iotests
iotests.py:1288:0: R0205: Class 'ReproducibleStreamWrapper' inherits 
from object, can be safely removed from bases in python3 
(useless-object-inheritance)
iotests.py:1303:4: W1113: Keyword argument before variable positional 
arguments list in the definition of __init__ function 
(keyword-arg-before-vararg)

=== mypy ===
iotests.py:1303: error: Function is missing a type annotation for one or 
more arguments

iotests.py:1304: error: Missing type parameters for generic type "Type"
iotests.py:1311: error: Function is missing a return type annotation
Found 3 errors in 1 file (checked 1 source file)

iotests.py:1303: error: Function is missing a type annotation for one or 
more arguments

iotests.py:1304: error: Missing type parameters for generic type "Type"
iotests.py:1311: error: Function is missing a return type annotation
Found 3 errors in 1 file (checked 1 source file)

iotests.py:1303: error: Function is missing a type annotation for one or 
more arguments

iotests.py:1304: error: Missing type parameters for generic type "Type"
iotests.py:1311: error: Function is missing a return type annotation
Found 3 errors in 1 file (checked 1 source file)

iotests.py:1303: error: Function is missing a type annotation for one or 
more arguments

iotests.py:1304: error: Missing type parameters for generic type "Type"
iotests.py:1311: error: Function is missing a return type annotation
Found 3 errors in 1 file (checked 1 source file)

iotests.py:1303: error: Function is missing a type annotation for one or 
more arguments

iotests.py:1304: error: Missing type parameters for generic type "Type"
iotests.py:1311: error: Function is missing a return type annotation
Found 3 errors in 1 file (checked 1 source file)

iotests.py:1303: error: Function is missing a type annotation for one or 
more arguments

iotests.py:1304: error: Missing type parameters for generic type "Type"
iotests.py:1311: error: Function is missing a return type annotation
Found 3 errors in 1 file (checked 1 source file)

iotests.py:1303: error: Function is missing a type annotation for one or 
more arguments

iotests.py:1304: error: Missing type parameters for generic type "Type"
iotests.py:1311: error: Function is missing a return type annotation
Found 3 errors in 1 file (checked 1 source file)

iotests.py:1303: error: Function is missing a type annotation for one or 
more arguments

iotests.py:1304: error: Missing type parameters for generic type "Type"
iotests.py:1311: error: Function is missing a return type annotation
Found 3 errors in 1 file (checked 1 source file)

Re: [PATCH for-6.0] qapi: qom: do not use target-specific conditionals

[Tom Lendacky]

On 3/26/21 5:03 AM, Paolo Bonzini wrote:
> ObjectType and ObjectOptions are defined in a target-independent file,
> therefore they do not have access to target-specific configuration
> symbols such as CONFIG_PSERIES or CONFIG_SEV.  For this reason,
> pef-guest and sev-guest are currently omitted when compiling the
> generated QAPI files.  In addition, this causes ObjectType to have
> different definitions depending on the file that is including
> qapi-types-qom.h (currently this is not causing any issues, but it
> is wrong).
> 
> Define the two enum entries and the SevGuestProperties type
> unconditionally to avoid the issue.  We do not expect to have
> many target-dependent user-creatable classes, so it is not
> particularly problematic.
> 
> Reported-by: Tom Lendacky 
> Signed-off-by: Paolo Bonzini 

I'm once again able to launch SEV guests.

Tested-by: Tom Lendacky 

> ---
>  qapi/qom.json | 10 --
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/qapi/qom.json b/qapi/qom.json
> index 2056edc072..db5ac419b1 100644
> --- a/qapi/qom.json
> +++ b/qapi/qom.json
> @@ -733,8 +733,7 @@
>  '*policy': 'uint32',
>  '*handle': 'uint32',
>  '*cbitpos': 'uint32',
> -'reduced-phys-bits': 'uint32' },
> -  'if': 'defined(CONFIG_SEV)' }
> +'reduced-phys-bits': 'uint32' } }
>  
>  ##
>  # @ObjectType:
> @@ -768,14 +767,14 @@
>  { 'name': 'memory-backend-memfd',
>'if': 'defined(CONFIG_LINUX)' },
>  'memory-backend-ram',
> -{'name': 'pef-guest', 'if': 'defined(CONFIG_PSERIES)' },
> +'pef-guest',
>  'pr-manager-helper',
>  'rng-builtin',
>  'rng-egd',
>  'rng-random',
>  'secret',
>  'secret_keyring',
> -{'name': 'sev-guest', 'if': 'defined(CONFIG_SEV)' },
> +'sev-guest',
>  's390-pv-guest',
>  'throttle-group',
>  'tls-creds-anon',
> @@ -831,8 +830,7 @@
>'rng-random': 'RngRandomProperties',
>'secret': 'SecretProperties',
>'secret_keyring': 'SecretKeyringProperties',
> -  'sev-guest':  { 'type': 'SevGuestProperties',
> -  'if': 'defined(CONFIG_SEV)' },
> +  'sev-guest':  'SevGuestProperties',
>'throttle-group': 'ThrottleGroupProperties',
>'tls-creds-anon': 'TlsCredsAnonProperties',
>'tls-creds-psk':  'TlsCredsPskProperties',
> 

Re: [PATCH v3 2/3] spapr: nvdimm: Implement H_SCM_FLUSH hcall

[Shivaprasad G Bhat]

On 3/25/21 7:21 AM, David Gibson wrote:

On Wed, Mar 24, 2021 at 09:34:06AM +0530, Aneesh Kumar K.V wrote:

On 3/24/21 8:37 AM, David Gibson wrote:

On Tue, Mar 23, 2021 at 09:47:38AM -0400, Shivaprasad G Bhat wrote:

The patch adds support for the SCM flush hcall for the nvdimm devices.

...

collects all the hcall states from 'completed' list. The necessary
nvdimm flush specific vmstate structures are added to the spapr
machine vmstate.

Signed-off-by: Shivaprasad G Bhat 

An overal question: surely the same issue must arise on x86 with
file-backed NVDIMMs.  How do they handle this case?

On x86 we have different ways nvdimm can be discovered. ACPI NFIT, e820 map
and virtio_pmem. Among these virio_pmem always operated with synchronous dax
disabled and both ACPI and e820 doesn't have the ability to differentiate
support for synchronous dax.

Ok.  And for the virtio-pmem case, how are the extra flushes actually
done on x86?



virtio-pmem device has virtqueue with virtio_pmem_flush() as the handler

which gets called for all flush requests from guest. virtio_pmem_flush() is

offloading the flush to thread pool with a worker doing fsync() and the

completion callback notifying the guest with response.



With that I would expect users to use virtio_pmem when using using file
backed NVDIMMS

So... should we prevent advertising an NVDIMM through ACPI or e820 if
it doesn't have sync-dax enabled?



Is it possible to have different defaults for sync-dax based on 
architecture ?


The behaviour on x86 is sync-dax=on for nvdimms. So, it would be correct to

have the default as "on" for x86. For pseries -  "off" for new machines.

Looking at code, I didnt find much ways to achieve this. Can you suggest

what can be done ?

Re: [PATCH 1/4] qcow2: Improve refcount structure rebuilding

[Max Reitz]

On 26.03.21 12:48, Vladimir Sementsov-Ogievskiy wrote:

10.03.2021 18:59, Max Reitz wrote:

When rebuilding the refcount structures (when qemu-img check -r found
errors with refcount = 0, but reference count > 0), the new refcount
table defaults to being put at the image file end[1].  There is no good
reason for that except that it means we will not have to rewrite any
refblocks we already wrote to disk.

Changing the code to rewrite those refblocks is not too difficult,
though, so let us do that.  That is beneficial for images on block
devices, where we cannot really write beyond the end of the image file.

[1] Unless there is something allocated in the area pointed to by the
 last refblock, so we have to write that refblock.  In that case, we
 try to put the reftable in there.

Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1519071
Signed-off-by: Max Reitz 
---
  block/qcow2-refcount.c | 126 ++---
  1 file changed, 67 insertions(+), 59 deletions(-)

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 8e649b008e..162caeeb8e 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -2352,8 +2352,9 @@ static int 
rebuild_refcount_structure(BlockDriverState *bs,

    int64_t *nb_clusters)
  {
  BDRVQcow2State *s = bs->opaque;
-    int64_t first_free_cluster = 0, reftable_offset = -1, cluster = 0;
+    int64_t first_free_cluster = 0, reftable_offset = -1, cluster;
  int64_t refblock_offset, refblock_start, refblock_index;
+    int64_t first_cluster, end_cluster;
  uint32_t reftable_size = 0;
  uint64_t *on_disk_reftable = NULL;
  void *on_disk_refblock;
@@ -2365,8 +2366,11 @@ static int 
rebuild_refcount_structure(BlockDriverState *bs,

  qcow2_cache_empty(bs, s->refcount_block_cache);
+    first_cluster = 0;
+    end_cluster = *nb_clusters;
+
  write_refblocks:
-    for (; cluster < *nb_clusters; cluster++) {
+    for (cluster = first_cluster; cluster < end_cluster; cluster++) {
  if (!s->get_refcount(*refcount_table, cluster)) {
  continue;
  }
@@ -2374,65 +2378,68 @@ write_refblocks:
  refblock_index = cluster >> s->refcount_block_bits;
  refblock_start = refblock_index << s->refcount_block_bits;
-    /* Don't allocate a cluster in a refblock already written to 
disk */

-    if (first_free_cluster < refblock_start) {
-    first_free_cluster = refblock_start;
-    }
-    refblock_offset = alloc_clusters_imrt(bs, 1, refcount_table,
-  nb_clusters, 
&first_free_cluster);

-    if (refblock_offset < 0) {
-    fprintf(stderr, "ERROR allocating refblock: %s\n",
-    strerror(-refblock_offset));
-    res->check_errors++;
-    ret = refblock_offset;
-    goto fail;
-    }
-
-    if (reftable_size <= refblock_index) {
-    uint32_t old_reftable_size = reftable_size;
-    uint64_t *new_on_disk_reftable;
+    if (reftable_size > refblock_index &&
+    on_disk_reftable[refblock_index])
+    {
+    refblock_offset = on_disk_reftable[refblock_index];


In this branch, we assign it to ..


+    } else {
+    int64_t refblock_cluster_index;
-    reftable_size = ROUND_UP((refblock_index + 1) * 
REFTABLE_ENTRY_SIZE,
- s->cluster_size) / 
REFTABLE_ENTRY_SIZE;

-    new_on_disk_reftable = g_try_realloc(on_disk_reftable,
- reftable_size *
- REFTABLE_ENTRY_SIZE);
-    if (!new_on_disk_reftable) {
+    /* Don't allocate a cluster in a refblock already written 
to disk */

+    if (first_free_cluster < refblock_start) {
+    first_free_cluster = refblock_start;
+    }
+    refblock_offset = alloc_clusters_imrt(bs, 1, refcount_table,
+  nb_clusters,
+  &first_free_cluster);
+    if (refblock_offset < 0) {
+    fprintf(stderr, "ERROR allocating refblock: %s\n",
+    strerror(-refblock_offset));
  res->check_errors++;
-    ret = -ENOMEM;
+    ret = refblock_offset;
  goto fail;
  }
-    on_disk_reftable = new_on_disk_reftable;
-    memset(on_disk_reftable + old_reftable_size, 0,
-   (reftable_size - old_reftable_size) * 
REFTABLE_ENTRY_SIZE);

+    refblock_cluster_index = refblock_offset / s->cluster_size;
+    if (refblock_cluster_index >= end_cluster) {
+    /*
+ * We must write the refblock that holds this refblock's
+ * refcount
+ */
+    end_cluster = refbloc

Re: [PATCH] net/npcm7xx_emc.c: Fix handling of receiving packets when RSDR not set

[Peter Maydell]

On Fri, 19 Mar 2021 at 19:51, Doug Evans  wrote:
>
> Turning REG_MCMDR_RXON is enough to start receiving packets.
>
> Signed-off-by: Doug Evans 
> ---
>  hw/net/npcm7xx_emc.c   |  4 +++-
>  tests/qtest/npcm7xx_emc-test.c | 30 +-
>  2 files changed, 24 insertions(+), 10 deletions(-)



Applied to target-arm.next, thanks.

-- PMM

Re: [PATCH] hw/display/xlnx_dp: Free FIFOs adding xlnx_dp_finalize()

[Peter Maydell]

On Tue, 23 Mar 2021 at 18:30, Philippe Mathieu-Daudé  wrote:
>
> When building with --enable-sanitizers we get:
>
>   Direct leak of 16 byte(s) in 1 object(s) allocated from:
>   #0 0x5618479ec7cf in malloc (qemu-system-aarch64+0x233b7cf)
>   #1 0x7f675745f958 in g_malloc (/lib64/libglib-2.0.so.0+0x58958)
>   #2 0x561847c2dcc9 in xlnx_dp_init hw/display/xlnx_dp.c:1259:5
>   #3 0x56184a5bdab8 in object_init_with_type qom/object.c:375:9
>   #4 0x56184a5a2bda in object_initialize_with_type qom/object.c:517:5
>   #5 0x56184a5a24d5 in object_initialize qom/object.c:536:5
>   #6 0x56184a5a2f6c in object_initialize_child_with_propsv 
> qom/object.c:566:5
>   #7 0x56184a5a2e60 in object_initialize_child_with_props 
> qom/object.c:549:10
>   #8 0x56184a5a3a1e in object_initialize_child_internal qom/object.c:603:5
>   #9 0x5618495aa431 in xlnx_zynqmp_init hw/arm/xlnx-zynqmp.c:273:5
>
> The RX/TX FIFOs are created in xlnx_dp_init(), add xlnx_dp_finalize()
> to destroy them.
>
> Fixes: 58ac482a66d ("introduce xlnx-dp")
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  hw/display/xlnx_dp.c | 9 +
>  1 file changed, 9 insertions(+



Applied to target-arm.next, thanks.

-- PMM

Re: [PATCH] hw/arm/smmuv3: Drop unused CDM_VALID() and is_cd_valid()

[Peter Maydell]

On Thu, 25 Mar 2021 at 14:27, Zenghui Yu  wrote:
>
> They were introduced in commit 9bde7f0674fe ("hw/arm/smmuv3: Implement
> translate callback") but never actually used. Drop them.
>
> Signed-off-by: Zenghui Yu 
> ---



Applied to target-arm.next, thanks.

-- PMM

[PATCH] iotests/116: Fix reference output

[Max Reitz]

15ce94a68ca ("block/qed: bdrv_qed_do_open: deal with errp") has improved
the qed driver's error reporting, though sadly did not add a test for
it.
The good news are: There already is such a test, namely 116.
The bad news are: Its reference output was not adjusted, and so now it
fails.

Let's fix the reference output, which has the nice side effect of
demonstrating 15ce94a68ca's improvements.

Fixes: 15ce94a68ca6730466c565c3d29971aab3087bf1
   ("block/qed: bdrv_qed_do_open: deal with errp")
Signed-off-by: Max Reitz 
---
 tests/qemu-iotests/116.out | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/tests/qemu-iotests/116.out b/tests/qemu-iotests/116.out
index 49f9a261a0..5f6c6fffca 100644
--- a/tests/qemu-iotests/116.out
+++ b/tests/qemu-iotests/116.out
@@ -2,7 +2,7 @@ QA output created by 116
 
 == truncated header cluster ==
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
-qemu-io: can't open device TEST_DIR/t.qed: Could not open 'TEST_DIR/t.qed': 
Invalid argument
+qemu-io: can't open device TEST_DIR/t.qed: QED table offset is invalid
 
 == invalid header magic ==
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
@@ -10,21 +10,21 @@ qemu-io: can't open device TEST_DIR/t.qed: Image not in QED 
format
 
 == invalid cluster size ==
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
-qemu-io: can't open device TEST_DIR/t.qed: Could not open 'TEST_DIR/t.qed': 
Invalid argument
+qemu-io: can't open device TEST_DIR/t.qed: QED cluster size is invalid
 
 == invalid table size ==
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
-qemu-io: can't open device TEST_DIR/t.qed: Could not open 'TEST_DIR/t.qed': 
Invalid argument
+qemu-io: can't open device TEST_DIR/t.qed: QED table size is invalid
 
 == invalid header size ==
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
-qemu-io: can't open device TEST_DIR/t.qed: Could not open 'TEST_DIR/t.qed': 
Invalid argument
+qemu-io: can't open device TEST_DIR/t.qed: QED table offset is invalid
 
 == invalid L1 table offset ==
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
-qemu-io: can't open device TEST_DIR/t.qed: Could not open 'TEST_DIR/t.qed': 
Invalid argument
+qemu-io: can't open device TEST_DIR/t.qed: QED table offset is invalid
 
 == invalid image size ==
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
-qemu-io: can't open device TEST_DIR/t.qed: Could not open 'TEST_DIR/t.qed': 
Invalid argument
+qemu-io: can't open device TEST_DIR/t.qed: QED image size is invalid
 *** done
-- 
2.29.2

Re: [PATCH v3] i386/cpu_dump: support AVX512 ZMM regs dump

[Robert Hoo]

On Fri, 2021-03-26 at 07:11 -0600, Richard Henderson wrote:
> On 3/25/21 7:47 PM, Robert Hoo wrote:
> > On Thu, 2021-03-25 at 06:39 -0600, Richard Henderson wrote:
> > > On 3/24/21 9:15 PM, Robert Hoo wrote:
> > > > > > +} else if (env->xcr0 & XFEATURE_AVX) {
> > > > > 
> > > > > This is normally a 2-bit test.
> > > > 
> > > > I beg your pardon. What 2 bits?
> > > 
> > > I forget the names, but isn't the usual test xcr0 & 6 == 6?
> > 
> > 6 stands for SSE state-component and AVX state-component.
> > I'm not sure about this.
> > Can you remember where did you this "xcr0 & 6 == 6"? I can look
> > into
> > that.
> 
> IA-64 and IA32 Software developers manual, Vol 1 Basic Architecture,
> Section 
> 14.3 Detection of AVX instructions.

OK, thanks Richard. If use the feature detection criteria here, then
AVX512 case will also need XCR0[2:1]='11b'.
I'm going to send v4 soon.
> 
> 
> r~

Re: [PATCH] target/arm: Make number of counters in PMCR follow the CPU

[Richard Henderson]

On 3/11/21 10:59 AM, Peter Maydell wrote:

Currently we give all the v7-and-up CPUs a PMU with 4 counters.  This
means that we don't provide the 6 counters that are required by the
Arm BSA (Base System Architecture) specification if the CPU supports
the Virtualization extensions.

Instead of having a single PMCR_NUM_COUNTERS, make each CPU type
specify the PMCR reset value (obtained from the appropriate TRM), and
use the 'N' field of that value to define the number of counters
provided.

This means that we now supply 6 counters for Cortex-A53, A57, A72,
A15 and A9 as well as '-cpu max'; Cortex-A7 and A8 stay at 4; and
Cortex-R5 goes down to 3.

Note that because we now use the PMCR reset value of the specific
implementation, we no longer set the LC bit out of reset.  This has
an UNKNOWN value out of reset for all cores with any AArch32 support,
so guest software should be setting it anyway if it wants it.

Signed-off-by: Peter Maydell
---
This is pretty much untested (I just checked Linux still boots;
haven't tried it with KVM either). It's an alternative to
just bumping PMCR_NUM_COUNTERS to 6.
---
  target/arm/cpu.h |  1 +
  target/arm/cpu64.c   |  3 +++
  target/arm/cpu_tcg.c |  5 +
  target/arm/helper.c  | 29 +
  target/arm/kvm64.c   |  2 ++
  5 files changed, 28 insertions(+), 12 deletions(-)


Reviewed-by: Richard Henderson 


r~

Re: [PATCH] iotests/116: Fix reference output

[Eric Blake]

On 3/26/21 9:14 AM, Max Reitz wrote:
> 15ce94a68ca ("block/qed: bdrv_qed_do_open: deal with errp") has improved
> the qed driver's error reporting, though sadly did not add a test for
> it.
> The good news are: There already is such a test, namely 116.
> The bad news are: Its reference output was not adjusted, and so now it
> fails.
> 
> Let's fix the reference output, which has the nice side effect of
> demonstrating 15ce94a68ca's improvements.
> 
> Fixes: 15ce94a68ca6730466c565c3d29971aab3087bf1
>("block/qed: bdrv_qed_do_open: deal with errp")
> Signed-off-by: Max Reitz 
> ---
>  tests/qemu-iotests/116.out | 12 ++--
>  1 file changed, 6 insertions(+), 6 deletions(-)

Reviewed-by: Eric Blake 

Since the original went in through my tree, I can queue this one through
as well.

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

[PATCH v3 0/5] qemu-iotests: quality of life improvements

[Paolo Bonzini]

This series adds a few usability improvements to qemu-iotests, in
particular:

- arguments can be passed to Python unittests scripts, for example
  to run only a subset of the test cases (patches 1-2)

- it is possible to do "./check -- ../../../tests/qemu-iotests/055 args..."
  and specify arbitrary arguments to be passed to a single test script.
  This allows to take advantage of the previous feature and ease debugging
  of Python tests.

Paolo

v2->v3: fix pylint/mypy [Max]
fix patch 4 for shell-based tests [Emanuele]

Paolo Bonzini (5):
  qemu-iotests: do not buffer the test output
  qemu-iotests: allow passing unittest.main arguments to the test
scripts
  qemu-iotests: move command line and environment handling from
TestRunner to TestEnv
  qemu-iotests: let "check" spawn an arbitrary test command
  qemu-iotests: fix case of SOCK_DIR already in the environment

 tests/qemu-iotests/check | 18 ++-
 tests/qemu-iotests/iotests.py| 80 +++-
 tests/qemu-iotests/testenv.py| 22 +++--
 tests/qemu-iotests/testrunner.py | 15 +-
 4 files changed, 85 insertions(+), 50 deletions(-)

-- 
2.30.1

[PATCH v3 1/5] qemu-iotests: do not buffer the test output

[Paolo Bonzini]

Instead of buffering the test output into a StringIO, patch it on
the fly by wrapping sys.stdout's write method.  This can be
done unconditionally, even if using -d, which makes execute_unittest
a bit simpler.

Signed-off-by: Paolo Bonzini 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
Tested-by: Emanuele Giuseppe Esposito 
Message-Id: <20210323181928.311862-2-pbonz...@redhat.com>
---
 tests/qemu-iotests/iotests.py | 70 ---
 1 file changed, 41 insertions(+), 29 deletions(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 5af0182895..ccf3747ede 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -20,7 +20,6 @@
 import bz2
 from collections import OrderedDict
 import faulthandler
-import io
 import json
 import logging
 import os
@@ -32,7 +31,7 @@
 import sys
 import time
 from typing import (Any, Callable, Dict, Iterable,
-List, Optional, Sequence, Tuple, TypeVar)
+List, Optional, Sequence, TextIO, Tuple, Type, TypeVar)
 import unittest
 
 from contextlib import contextmanager
@@ -1271,37 +1270,50 @@ def func_wrapper(*args, **kwargs):
 return func(*args, **kwargs)
 return func_wrapper
 
+# We need to filter out the time taken from the output so that
+# qemu-iotest can reliably diff the results against master output,
+# and hide skipped tests from the reference output.
+
+class ReproducibleTestResult(unittest.TextTestResult):
+def addSkip(self, test, reason):
+# Same as TextTestResult, but print dot instead of "s"
+unittest.TestResult.addSkip(self, test, reason)
+if self.showAll:
+self.stream.writeln("skipped {0!r}".format(reason))
+elif self.dots:
+self.stream.write(".")
+self.stream.flush()
+
+class ReproducibleStreamWrapper:
+def __init__(self, stream: TextIO):
+self.stream = stream
+
+def __getattr__(self, attr):
+if attr in ('stream', '__getstate__'):
+raise AttributeError(attr)
+return getattr(self.stream, attr)
+
+def write(self, arg=None):
+arg = re.sub(r'Ran (\d+) tests? in [\d.]+s', r'Ran \1 tests', arg)
+arg = re.sub(r' \(skipped=\d+\)', r'', arg)
+self.stream.write(arg)
+
+class ReproducibleTestRunner(unittest.TextTestRunner):
+def __init__(self, stream: Optional[TextIO] = None,
+ resultclass: Type[unittest.TestResult] = ReproducibleTestResult,
+ **kwargs: Any) -> None:
+rstream = ReproducibleStreamWrapper(stream or sys.stdout)
+super().__init__(stream=rstream,   # type: ignore
+ descriptions=True,
+ resultclass=resultclass,
+ **kwargs)
+
 def execute_unittest(debug=False):
 """Executes unittests within the calling module."""
 
 verbosity = 2 if debug else 1
-
-if debug:
-output = sys.stdout
-else:
-# We need to filter out the time taken from the output so that
-# qemu-iotest can reliably diff the results against master output.
-output = io.StringIO()
-
-runner = unittest.TextTestRunner(stream=output, descriptions=True,
- verbosity=verbosity)
-try:
-# unittest.main() will use sys.exit(); so expect a SystemExit
-# exception
-unittest.main(testRunner=runner)
-finally:
-# We need to filter out the time taken from the output so that
-# qemu-iotest can reliably diff the results against master output.
-if not debug:
-out = output.getvalue()
-out = re.sub(r'Ran (\d+) tests? in [\d.]+s', r'Ran \1 tests', out)
-
-# Hide skipped tests from the reference output
-out = re.sub(r'OK \(skipped=\d+\)', 'OK', out)
-out_first_line, out_rest = out.split('\n', 1)
-out = out_first_line.replace('s', '.') + '\n' + out_rest
-
-sys.stderr.write(out)
+runner = unittest.ReproducibleTestRunner(verbosity=verbosity)
+unittest.main(testRunner=runner)
 
 def execute_setup_common(supported_fmts: Sequence[str] = (),
  supported_platforms: Sequence[str] = (),
-- 
2.30.1

[PATCH v3 3/5] qemu-iotests: move command line and environment handling from TestRunner to TestEnv

[Paolo Bonzini]

In the next patch, "check" will learn how to execute a test script without
going through TestRunner.  To enable this, keep only the text output
and subprocess handling in the TestRunner; move into TestEnv the logic
to prepare for running a subprocess.

Reviewed-by: Vladimir Sementsov-Ogievskiy 
Signed-off-by: Paolo Bonzini 
Tested-by: Emanuele Giuseppe Esposito 
Message-Id: <20210323181928.311862-4-pbonz...@redhat.com>
---
 tests/qemu-iotests/testenv.py| 17 -
 tests/qemu-iotests/testrunner.py | 14 +-
 2 files changed, 17 insertions(+), 14 deletions(-)

diff --git a/tests/qemu-iotests/testenv.py b/tests/qemu-iotests/testenv.py
index 6d27712617..fca3a609e0 100644
--- a/tests/qemu-iotests/testenv.py
+++ b/tests/qemu-iotests/testenv.py
@@ -25,7 +25,7 @@
 import random
 import subprocess
 import glob
-from typing import Dict, Any, Optional, ContextManager
+from typing import List, Dict, Any, Optional, ContextManager
 
 
 def isxfile(path: str) -> bool:
@@ -74,6 +74,21 @@ class TestEnv(ContextManager['TestEnv']):
  'CACHEMODE_IS_DEFAULT', 'IMGFMT_GENERIC', 'IMGOPTSSYNTAX',
  'IMGKEYSECRET', 'QEMU_DEFAULT_MACHINE', 'MALLOC_PERTURB_']
 
+def prepare_subprocess(self, args: List[str]) -> Dict[str, str]:
+if self.debug:
+args.append('-d')
+
+with open(args[0], encoding="utf-8") as f:
+try:
+if f.readline().rstrip() == '#!/usr/bin/env python3':
+args.insert(0, self.python)
+except UnicodeDecodeError:  # binary test? for future.
+pass
+
+os_env = os.environ.copy()
+os_env.update(self.get_env())
+return os_env
+
 def get_env(self) -> Dict[str, str]:
 env = {}
 for v in self.env_variables:
diff --git a/tests/qemu-iotests/testrunner.py b/tests/qemu-iotests/testrunner.py
index 1fc61fcaa3..519924dc81 100644
--- a/tests/qemu-iotests/testrunner.py
+++ b/tests/qemu-iotests/testrunner.py
@@ -129,7 +129,6 @@ class TestRunner(ContextManager['TestRunner']):
 def __init__(self, env: TestEnv, makecheck: bool = False,
  color: str = 'auto') -> None:
 self.env = env
-self.test_run_env = self.env.get_env()
 self.makecheck = makecheck
 self.last_elapsed = LastElapsedTime('.last-elapsed-cache', env)
 
@@ -243,18 +242,7 @@ def do_run_test(self, test: str) -> TestResult:
 silent_unlink(p)
 
 args = [str(f_test.resolve())]
-if self.env.debug:
-args.append('-d')
-
-with f_test.open(encoding="utf-8") as f:
-try:
-if f.readline().rstrip() == '#!/usr/bin/env python3':
-args.insert(0, self.env.python)
-except UnicodeDecodeError:  # binary test? for future.
-pass
-
-env = os.environ.copy()
-env.update(self.test_run_env)
+env = self.env.prepare_subprocess(args)
 
 t0 = time.time()
 with f_bad.open('w', encoding="utf-8") as f:
-- 
2.30.1

[PATCH v3 2/5] qemu-iotests: allow passing unittest.main arguments to the test scripts

[Paolo Bonzini]

Python test scripts that use unittest consist of multiple tests.
unittest.main allows selecting which tests to run, but currently this
is not possible because the iotests wrapper ignores sys.argv.

unittest.main command line options also allow the user to pick the
desired options for verbosity, failfast mode, etc.  While "-d" is
currently translated to "-v", it also enables extra debug output,
and other options are not available at all.

These command line options only work if the unittest.main testRunner
argument is a type, rather than a TestRunner instance.  Therefore, pass
the class name and "verbosity" argument to unittest.main, and adjust for
the different default warnings between TextTestRunner and unittest.main.

Signed-off-by: Paolo Bonzini 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
Tested-by: Emanuele Giuseppe Esposito 
Message-Id: <20210323181928.311862-3-pbonz...@redhat.com>
---
 tests/qemu-iotests/iotests.py | 14 +-
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index ccf3747ede..5ead94229f 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -1308,12 +1308,16 @@ def __init__(self, stream: Optional[TextIO] = None,
  resultclass=resultclass,
  **kwargs)
 
-def execute_unittest(debug=False):
+def execute_unittest(argv: List[str], debug: bool = False) -> None:
 """Executes unittests within the calling module."""
 
-verbosity = 2 if debug else 1
-runner = unittest.ReproducibleTestRunner(verbosity=verbosity)
-unittest.main(testRunner=runner)
+# Some tests have warnings, especially ResourceWarnings for unclosed
+# files and sockets.  Ignore them for now to ensure reproducibility of
+# the test output.
+unittest.main(argv=argv,
+  testRunner=ReproducibleTestRunner,
+  verbosity=2 if debug else 1,
+  warnings=None if sys.warnoptions else 'ignore')
 
 def execute_setup_common(supported_fmts: Sequence[str] = (),
  supported_platforms: Sequence[str] = (),
@@ -1350,7 +1354,7 @@ def execute_test(*args, test_function=None, **kwargs):
 
 debug = execute_setup_common(*args, **kwargs)
 if not test_function:
-execute_unittest(debug)
+execute_unittest(sys.argv, debug)
 else:
 test_function()
 
-- 
2.30.1

[PATCH v3 4/5] qemu-iotests: let "check" spawn an arbitrary test command

[Paolo Bonzini]

Right now there is no easy way for "check" to print a reproducer command.
Because such a reproducer command line would be huge, we can instead teach
check to start a command of our choice.  This can be for example a Python
unit test with arguments to only run a specific subtest.

Move the trailing empty line to print_env(), since it always looks better
and one caller was not adding it.

Signed-off-by: Paolo Bonzini 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
Tested-by: Emanuele Giuseppe Esposito 
Message-Id: <20210323181928.311862-5-pbonz...@redhat.com>
---
 tests/qemu-iotests/check | 18 +-
 tests/qemu-iotests/testenv.py|  3 ++-
 tests/qemu-iotests/testrunner.py |  1 -
 3 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
index d1c87ceaf1..df9fd733ff 100755
--- a/tests/qemu-iotests/check
+++ b/tests/qemu-iotests/check
@@ -19,6 +19,9 @@
 import os
 import sys
 import argparse
+import shutil
+from pathlib import Path
+
 from findtests import TestFinder
 from testenv import TestEnv
 from testrunner import TestRunner
@@ -101,7 +104,7 @@ def make_argparser() -> argparse.ArgumentParser:
'rerun failed ./check command, starting from the '
'middle of the process.')
 g_sel.add_argument('tests', metavar='TEST_FILES', nargs='*',
-   help='tests to run')
+   help='tests to run, or "--" followed by a command')
 
 return p
 
@@ -114,6 +117,19 @@ if __name__ == '__main__':
   imgopts=args.imgopts, misalign=args.misalign,
   debug=args.debug, valgrind=args.valgrind)
 
+if len(sys.argv) > 1 and sys.argv[-len(args.tests)-1] == '--':
+if not args.tests:
+sys.exit("missing command after '--'")
+cmd = args.tests
+env.print_env()
+exec_path = Path(shutil.which(cmd[0]))
+if exec_path is None:
+sys.exit('command not found: ' + cmd[0])
+cmd[0] = exec_path.resolve()
+full_env = env.prepare_subprocess(cmd)
+os.chdir(Path(exec_path).parent)
+os.execve(cmd[0], cmd, full_env)
+
 testfinder = TestFinder(test_dir=env.source_iotests)
 
 groups = args.groups.split(',') if args.groups else None
diff --git a/tests/qemu-iotests/testenv.py b/tests/qemu-iotests/testenv.py
index fca3a609e0..cd0e39b789 100644
--- a/tests/qemu-iotests/testenv.py
+++ b/tests/qemu-iotests/testenv.py
@@ -284,7 +284,8 @@ def print_env(self) -> None:
 PLATFORM  -- {platform}
 TEST_DIR  -- {TEST_DIR}
 SOCK_DIR  -- {SOCK_DIR}
-SOCKET_SCM_HELPER -- {SOCKET_SCM_HELPER}"""
+SOCKET_SCM_HELPER -- {SOCKET_SCM_HELPER}
+"""
 
 args = collections.defaultdict(str, self.get_env())
 
diff --git a/tests/qemu-iotests/testrunner.py b/tests/qemu-iotests/testrunner.py
index 519924dc81..2f56ac545d 100644
--- a/tests/qemu-iotests/testrunner.py
+++ b/tests/qemu-iotests/testrunner.py
@@ -316,7 +316,6 @@ def run_tests(self, tests: List[str]) -> bool:
 
 if not self.makecheck:
 self.env.print_env()
-print()
 
 test_field_width = max(len(os.path.basename(t)) for t in tests) + 2
 
-- 
2.30.1

[PATCH v3 5/5] qemu-iotests: fix case of SOCK_DIR already in the environment

[Paolo Bonzini]

Due to a typo, in this case the SOCK_DIR was not being created.

Reviewed-by: Vladimir Sementsov-Ogievskiy 
Signed-off-by: Paolo Bonzini 
Tested-by: Emanuele Giuseppe Esposito 
Message-Id: <20210323181928.311862-6-pbonz...@redhat.com>
---
 tests/qemu-iotests/testenv.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/testenv.py b/tests/qemu-iotests/testenv.py
index cd0e39b789..0c3fe75636 100644
--- a/tests/qemu-iotests/testenv.py
+++ b/tests/qemu-iotests/testenv.py
@@ -120,7 +120,7 @@ def init_directories(self) -> None:
 try:
 self.sock_dir = os.environ['SOCK_DIR']
 self.tmp_sock_dir = False
-Path(self.test_dir).mkdir(parents=True, exist_ok=True)
+Path(self.sock_dir).mkdir(parents=True, exist_ok=True)
 except KeyError:
 self.sock_dir = tempfile.mkdtemp()
 self.tmp_sock_dir = True
-- 
2.30.1

Re: [PATCH v2 1/3] hw/display/bcm2835_fb: Resize console on reset

[Peter Maydell]

On Tue, 23 Mar 2021 at 16:14, Philippe Mathieu-Daudé  wrote:
>
> We want to remove the bcm2835_fb_reset() call in bcm2835_fb_realize()
> but doing triggers:
>
>   hw/display/bcm2835_fb.c:131:13: runtime error: store to null pointer of 
> type 'uint32_t' (aka 'unsigned int')
>   SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior 
> hw/display/bcm2835_fb.c:131:13 in
>   AddressSanitizer:DEADLYSIGNAL
>   =
>   ==195864==ERROR: AddressSanitizer: SEGV on unknown address 0x 
> (pc 0x555d1d51e6d6 bp 0x7ffd25a31160 sp 0x7ffd25a30fb0 T0)
>   ==195864==The signal is caused by a WRITE memory access.
>   ==195864==Hint: address points to the zero page.
>  #0 0x555d1d51e6d6 in draw_line_src16 hw/display/bcm2835_fb.c:131:30
>  #1 0x555d1dd88d5f in framebuffer_update_display 
> hw/display/framebuffer.c:107:13
>  #2 0x555d1d51d081 in fb_update_display hw/display/bcm2835_fb.c:203:5
>  #3 0x555d1ccb93d6 in graphic_hw_update ui/console.c:279:9
>  #4 0x555d1dbc92cb in gd_refresh ui/gtk.c:492:5
>  #5 0x555d1ccef1fc in dpy_refresh ui/console.c:1734:13
>  #6 0x555d1ccee09c in gui_update ui/console.c:209:5
>  #7 0x555d201f3cf2 in timerlist_run_timers util/qemu-timer.c:586:9
>  #8 0x555d201f4061 in qemu_clock_run_timers util/qemu-timer.c:600:12
>  #9 0x555d201f5029 in qemu_clock_run_all_timers util/qemu-timer.c:682:25
> #10 0x555d200c6f6c in main_loop_wait util/main-loop.c:541:5
> #11 0x555d1f06ba93 in qemu_main_loop softmmu/runstate.c:725:9
> #12 0x555d1cafe6ae in main softmmu/main.c:50:5
> #13 0x7f6e6991b081 in __libc_start_main (/lib64/libc.so.6+0x27081)
> #14 0x555d1ca249ed in _start 
> (/mnt/scratch/qemu/sanitizer_aa64/qemu-system-aarch64+0x22999ed)
>
>   AddressSanitizer can not provide additional info.
>   SUMMARY: AddressSanitizer: SEGV hw/display/bcm2835_fb.c:131:30 in 
> draw_line_src16
>   ==195864==ABORTING
>
> The graphic console timer kicks before the display device is realized.
> By calling qemu_console_resize() in bcm2835_fb_reset() we force the
> creation of the graphic console surface early enough.
>
> Reported-by: Peter Maydell 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  hw/display/bcm2835_fb.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/hw/display/bcm2835_fb.c b/hw/display/bcm2835_fb.c
> index 2be77bdd3a0..3e63d58e0b2 100644
> --- a/hw/display/bcm2835_fb.c
> +++ b/hw/display/bcm2835_fb.c
> @@ -399,6 +399,7 @@ static void bcm2835_fb_reset(DeviceState *dev)
>  s->config = s->initial_config;
>
>  s->invalidate = true;
> +qemu_console_resize(s->con, s->initial_config.xres, 
> s->initial_config.yres);
>  s->lock = false;
>  }

I don't really understand how the commit message and the code
change relate. reset happens after realize, and realize
already calls qemu_console_resize(), so how can adding a
call to resize here in reset cause the console surface to
be created any earlier than it already is ?

I also don't understand how the GUI timer can call us before
the device is realized, given that we only register ourselves
via graphics_console_init() in the device realize.

More generally, I think we should probably start by figuring out
what the requirements on graphics devices vs the UI layer
are or should be. Is it possible to get the UI layer to
not start calling into graphics devices until after the
system has been reset for the first time, for instance?
Gerd, do you have any views here ?

thanks
-- PMM

Re: [PATCH 1/4] qcow2: Improve refcount structure rebuilding

[Vladimir Sementsov-Ogievskiy]

26.03.2021 16:47, Max Reitz wrote:

On 26.03.21 12:48, Vladimir Sementsov-Ogievskiy wrote:

10.03.2021 18:59, Max Reitz wrote:

When rebuilding the refcount structures (when qemu-img check -r found
errors with refcount = 0, but reference count > 0), the new refcount
table defaults to being put at the image file end[1].  There is no good
reason for that except that it means we will not have to rewrite any
refblocks we already wrote to disk.

Changing the code to rewrite those refblocks is not too difficult,
though, so let us do that.  That is beneficial for images on block
devices, where we cannot really write beyond the end of the image file.

[1] Unless there is something allocated in the area pointed to by the
 last refblock, so we have to write that refblock.  In that case, we
 try to put the reftable in there.

Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1519071
Signed-off-by: Max Reitz 
---
  block/qcow2-refcount.c | 126 ++---
  1 file changed, 67 insertions(+), 59 deletions(-)

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 8e649b008e..162caeeb8e 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -2352,8 +2352,9 @@ static int rebuild_refcount_structure(BlockDriverState 
*bs,
    int64_t *nb_clusters)
  {
  BDRVQcow2State *s = bs->opaque;
-    int64_t first_free_cluster = 0, reftable_offset = -1, cluster = 0;
+    int64_t first_free_cluster = 0, reftable_offset = -1, cluster;
  int64_t refblock_offset, refblock_start, refblock_index;
+    int64_t first_cluster, end_cluster;
  uint32_t reftable_size = 0;
  uint64_t *on_disk_reftable = NULL;
  void *on_disk_refblock;
@@ -2365,8 +2366,11 @@ static int rebuild_refcount_structure(BlockDriverState 
*bs,
  qcow2_cache_empty(bs, s->refcount_block_cache);
+    first_cluster = 0;
+    end_cluster = *nb_clusters;
+
  write_refblocks:
-    for (; cluster < *nb_clusters; cluster++) {
+    for (cluster = first_cluster; cluster < end_cluster; cluster++) {
  if (!s->get_refcount(*refcount_table, cluster)) {
  continue;
  }
@@ -2374,65 +2378,68 @@ write_refblocks:
  refblock_index = cluster >> s->refcount_block_bits;
  refblock_start = refblock_index << s->refcount_block_bits;
-    /* Don't allocate a cluster in a refblock already written to disk */
-    if (first_free_cluster < refblock_start) {
-    first_free_cluster = refblock_start;
-    }
-    refblock_offset = alloc_clusters_imrt(bs, 1, refcount_table,
-  nb_clusters, 
&first_free_cluster);
-    if (refblock_offset < 0) {
-    fprintf(stderr, "ERROR allocating refblock: %s\n",
-    strerror(-refblock_offset));
-    res->check_errors++;
-    ret = refblock_offset;
-    goto fail;
-    }
-
-    if (reftable_size <= refblock_index) {
-    uint32_t old_reftable_size = reftable_size;
-    uint64_t *new_on_disk_reftable;
+    if (reftable_size > refblock_index &&
+    on_disk_reftable[refblock_index])
+    {
+    refblock_offset = on_disk_reftable[refblock_index];


In this branch, we assign it to ..


+    } else {
+    int64_t refblock_cluster_index;
-    reftable_size = ROUND_UP((refblock_index + 1) * 
REFTABLE_ENTRY_SIZE,
- s->cluster_size) / REFTABLE_ENTRY_SIZE;
-    new_on_disk_reftable = g_try_realloc(on_disk_reftable,
- reftable_size *
- REFTABLE_ENTRY_SIZE);
-    if (!new_on_disk_reftable) {
+    /* Don't allocate a cluster in a refblock already written to disk 
*/
+    if (first_free_cluster < refblock_start) {
+    first_free_cluster = refblock_start;
+    }
+    refblock_offset = alloc_clusters_imrt(bs, 1, refcount_table,
+  nb_clusters,
+  &first_free_cluster);
+    if (refblock_offset < 0) {
+    fprintf(stderr, "ERROR allocating refblock: %s\n",
+    strerror(-refblock_offset));
  res->check_errors++;
-    ret = -ENOMEM;
+    ret = refblock_offset;
  goto fail;
  }
-    on_disk_reftable = new_on_disk_reftable;
-    memset(on_disk_reftable + old_reftable_size, 0,
-   (reftable_size - old_reftable_size) * REFTABLE_ENTRY_SIZE);
+    refblock_cluster_index = refblock_offset / s->cluster_size;
+    if (refblock_cluster_index >= end_cluster) {
+    /*
+ * We must write the refblock that holds this refblock's
+ * refcount
+ */
+   

Re: [PATCH 2/2] Support monitor chardev hotswap with QMP

[Markus Armbruster]

Li Zhang  writes:

> Hi,
>
> Any comments about this patch?

I wanted to review this before my Easter break, but I'm out of time :(

When I'm back (April 6), I'll check whether it still needs review, but I
do hope somebody else can look at it sooner.

Re: [PATCH 1/2] Fix the segment fault when calling yank_register_instance

[Markus Armbruster]

Looks like a bug fix.  Lukas, can you take care of it in time for 6.0?

Li Zhang  writes:

> From: Li Zhang 
>
> When executing the QMP commands "chardev-change" to change the
> backend device to socket, it will cause a segment fault because
> it assumes chr->label as non-NULL in function yank_register_instance.
> The function qmp_chardev_change calls chardev_new, which label
> is NULL when creating a new chardev. The label will be passed to
> yank_register_instance which causes a segment fault. The callchain
> is as the following:
> chardev_new ->
> qemu_char_open ->
> cc->open ->
> qmp_chardev_open_socket ->
> yank_register_instance
>
> Signed-off-by: Li Zhang 
> ---
>  chardev/char-socket.c | 8 +---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index c8bced76b7..26d5172682 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -1421,10 +1421,12 @@ static void qmp_chardev_open_socket(Chardev *chr,
>  qemu_chr_set_feature(chr, QEMU_CHAR_FEATURE_FD_PASS);
>  }
>  
> -if (!yank_register_instance(CHARDEV_YANK_INSTANCE(chr->label), errp)) {
> -return;
> +if (chr->label) {
> +if (!yank_register_instance(CHARDEV_YANK_INSTANCE(chr->label), 
> errp)) {
> +return;
> +}
> +s->registered_yank = true;
>  }
> -s->registered_yank = true;
>  
>  /* be isn't opened until we get a connection */
>  *be_opened = false;

Re: [PATCH] monitor/qmp: fix race on CHR_EVENT_CLOSED without OOB

[Markus Armbruster]

Wolfgang Bumiller  writes:

> On Thu, Mar 18, 2021 at 02:35:50PM +0100, Stefan Reiter wrote:
>> If OOB is disabled, events received in monitor_qmp_event will be handled
>> in the main context. Thus, we must not acquire a qmp_queue_lock there,
>> as the dispatcher coroutine holds one over a yield point, where it
>> expects to be rescheduled from the main context. If a CHR_EVENT_CLOSED
>> event is received just then, it can race and block the main thread by
>> waiting on the queue lock.
>> 
>> Run monitor_qmp_cleanup_queue_and_resume in a BH on the iohandler
>> thread, so the main thread can always make progress during the
>> reschedule.
>> 
>> The delaying of the cleanup is safe, since the dispatcher always moves
>> back to the iothread afterward, and thus the cleanup will happen before
>> it gets to its next iteration.
>> 
>> Signed-off-by: Stefan Reiter 
>> ---
>
> This is a tough one. It *may* be fine, but I wonder if we can approach
> this differently:

You guys make my head hurt.

I understand we're talking about a bug.  Is it a recent regression, or
an older bug?  How badly does the bug affect users?

I'm about to vanish for my Easter break...  If the bug must be fixed for
6.0, just waiting for me to come back seems unadvisable.

[...]

Re: [PATCH for-6.0] qapi: qom: do not use target-specific conditionals

[Paolo Bonzini]

On 26/03/21 13:27, Markus Armbruster wrote:

... until Kevin added some when he QAPIfied.  Looks like this (copied
from qemu-qmp-ref.7)[*]:

SevGuestProperties (Object)
Properties for sev-guest objects.

Members
sev-device: string (optional)
   SEV device to use (default: "/dev/sev")

dh-cert-file: string (optional)
   guest owners DH certificate (encoded with base64)

session-file: string (optional)
   guest owners session parameters (encoded with base64)

policy: int (optional)
   SEV policy value (default: 0x1)

handle: int (optional)
   SEV firmware handle (default: 0)

cbitpos: int (optional)
   C-bit location in page table entry (default: 0)

reduced-phys-bits: int
   number  of  bits  in  physical addresses that become unavailable
   when SEV is enabled

Since
2.12

If
defined(CONFIG_SEV)

Your patch drops the last three lines, without a replacement.


Yes, I mean the regression is not from 5.2.

Paolo