Re: [Virtio-fs] [PATCH 11/13] virtiofsd: Shutdown notification queue in the end

[Christophe de Dinechin]

On 2021-09-30 at 11:30 -04, Vivek Goyal  wrote...
> So far we did not have the notion of cross queue traffic. That is, we
> get request on a queue and send back response on same queue. So if a
> request be being processed and at the same time a stop queue request
> comes in, we wait for all pending requests to finish and then queue
> is stopped and associated data structure cleaned.
>
> But with notification queue, now it is possible that we get a locking
> request on request queue and send the notification back on a different
> queue (notificaiton queue). This means, we need to make sure that

typo: notification (I just saw Stefan noticed it too)

> notifiation queue has not already been shutdown or is not being

typo: notification ;-)

> shutdown in parallel while we are trying to send a notification back.
> Otherwise bad things are bound to happen.
>
> One way to solve this problem is that stop notification queue in the
> end. First stop hiprio and all request queues.

I do not understand that sentence. Maybe you meant to write "is to stop
notification queue in the end", but even so I don't understand if you mean
"in the end" (of what) or "last" (relative to other queues)? I guess you
meant last.

> That means by the
> time we are trying to stop notification queue, we know no other
> request can be in progress which can try to send something on
> notification queue.
>
> But problem is that currently we don't have any control on in what
> order queues should be stopped. If there was a notion of whole device
> being stopped, then we could decide in what order queues should be
> stopped.
>
> Stefan mentioned that there is a command to stop whole device
> VHOST_USER_SET_STATUS but it is not implemented in libvhost-user
> yet. Also we probably could not move away from per queue stop
> logic we have as of now.
>
> As an alternative, he said if we stop all queue when qidx 0 is
> being stopped, it should be fine and we can solve the issue of
> notification queue shutdown order.
>
> So in this patch I am shutting down all queues when queue 0
> is being shutdown. And also changed shutdown order in such a
> way that notification queue is shutdown last.

For my education: I assume there is no valid case where there is no queue
and only the notification queue?

>
> Suggested-by: Stefan Hajnoczi 
> Signed-off-by: Vivek Goyal 
> ---
>  tools/virtiofsd/fuse_virtio.c | 27 ++-
>  1 file changed, 26 insertions(+), 1 deletion(-)
>
> diff --git a/tools/virtiofsd/fuse_virtio.c b/tools/virtiofsd/fuse_virtio.c
> index c67c2e0e7a..a87e88e286 100644
> --- a/tools/virtiofsd/fuse_virtio.c
> +++ b/tools/virtiofsd/fuse_virtio.c
> @@ -826,6 +826,11 @@ static void fv_queue_cleanup_thread(struct fv_VuDev 
> *vud, int qidx)
>  assert(qidx < vud->nqueues);
>  ourqi = vud->qi[qidx];
>
> +/* Queue is already stopped */
> +if (!ourqi) {
> +return;
> +}
> +
>  /* qidx == 1 is the notification queue if notifications are enabled */
>  if (!se->notify_enabled || qidx != 1) {
>  /* Kill the thread */
> @@ -847,14 +852,25 @@ static void fv_queue_cleanup_thread(struct fv_VuDev 
> *vud, int qidx)
>
>  static void stop_all_queues(struct fv_VuDev *vud)
>  {
> +struct fuse_session *se = vud->se;
> +
>  for (int i = 0; i < vud->nqueues; i++) {
>  if (!vud->qi[i]) {
>  continue;
>  }
>
> +/* Shutdown notification queue in the end */
> +if (se->notify_enabled && i == 1) {
> +continue;
> +}
>  fuse_log(FUSE_LOG_INFO, "%s: Stopping queue %d thread\n", __func__, 
> i);
>  fv_queue_cleanup_thread(vud, i);
>  }
> +
> +if (se->notify_enabled) {
> +fuse_log(FUSE_LOG_INFO, "%s: Stopping queue %d thread\n", __func__, 
> 1);
> +fv_queue_cleanup_thread(vud, 1);
> +}
>  }
>
>  /* Callback from libvhost-user on start or stop of a queue */
> @@ -934,7 +950,16 @@ static void fv_queue_set_started(VuDev *dev, int qidx, 
> bool started)
>   * the queue thread doesn't block in virtio_send_msg().
>   */
>  vu_dispatch_unlock(vud);
> -fv_queue_cleanup_thread(vud, qidx);
> +
> +/*
> + * If queue 0 is being shutdown, treat it as if device is being
> + * shutdown and stop all queues.
> + */
> +if (qidx == 0) {
> +    stop_all_queues(vud);
> +} else {
> +fv_queue_cleanup_thread(vud, qidx);
> +}
>  vu_dispatch_wrlock(vud);
>  }
>  }

For my education: given that we dropped the write lock above, what prevents
queue 0 from being shutdown on one thread while another cleans up another
queue. What makes it safe in that case? I think this is worth a comment.

--
Cheers,
Christophe de Dinechin (IRC c3d)

Re: [Virtio-fs] [PATCH 12/13] virtiofsd: Implement blocking posix locks

[Christophe de Dinechin]

;
> +}
> +
>  static void lo_setlk(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info 
> *fi,
>   struct flock *lock, int sleep)
>  {
>  struct lo_data *lo = lo_data(req);
>  struct lo_inode *inode;
>  struct lo_inode_plock *plock;
> -int ret, saverr = 0;
> +int ret, saverr = 0, ofd;
> +uint64_t unique;
> +struct fuse_session *se = req->se;
> +bool blocking_lock = false;
>
>  fuse_log(FUSE_LOG_DEBUG,
>   "lo_setlk(ino=%" PRIu64 ", flags=%d)"
> @@ -2151,11 +2173,6 @@ static void lo_setlk(fuse_req_t req, fuse_ino_t ino, 
> struct fuse_file_info *fi,
>  return;
>  }
>
> -if (sleep) {
> -fuse_reply_err(req, EOPNOTSUPP);
> -return;
> -}
> -
>  inode = lo_inode(req, ino);
>  if (!inode) {
>  fuse_reply_err(req, EBADF);
> @@ -2168,21 +2185,56 @@ static void lo_setlk(fuse_req_t req, fuse_ino_t ino, 
> struct fuse_file_info *fi,
>
>  if (!plock) {
>  saverr = ret;
> +pthread_mutex_unlock(>plock_mutex);
>  goto out;
>  }
>
> +/*
> + * plock is now released when inode is going away. We already have
> + * a reference on inode, so it is guaranteed that plock->fd is
> + * still around even after dropping inode->plock_mutex lock
> + */
> +ofd = plock->fd;
> +pthread_mutex_unlock(>plock_mutex);
> +
> +/*
> + * If this lock request can block, request caller to wait for
> + * notification. Do not access req after this. Once lock is
> + * available, send a notification instead.
> + */
> +if (sleep && lock->l_type != F_UNLCK) {
> +        /*
> + * If notification queue is not enabled, can't support async
> + * locks.
> + */
> +if (!se->notify_enabled) {
> +saverr = EOPNOTSUPP;
> +goto out;
> +}
> +blocking_lock = true;
> +unique = req->unique;
> +fuse_reply_wait(req);
> +}
> +
>  /* TODO: Is it alright to modify flock? */
>  lock->l_pid = 0;
> -ret = fcntl(plock->fd, F_OFD_SETLK, lock);
> +if (blocking_lock) {
> +ret = fcntl(ofd, F_OFD_SETLKW, lock);
> +} else {
> +ret = fcntl(ofd, F_OFD_SETLK, lock);
> +}
>  if (ret == -1) {
>  saverr = errno;
>  }
>
>  out:
> -pthread_mutex_unlock(>plock_mutex);
>  lo_inode_put(lo, );
>
> -fuse_reply_err(req, saverr);
> +if (!blocking_lock) {
> +fuse_reply_err(req, saverr);
> +} else {
> +setlk_send_notification(se, unique, saverr);
> +}
>  }
>
>  static void lo_fsyncdir(fuse_req_t req, fuse_ino_t ino, int datasync,


--
Cheers,
Christophe de Dinechin (IRC c3d)

Re: [Virtio-fs] [PATCH 06/13] vhost-user-fs: Use helpers to create/cleanup virtqueue

[Christophe de Dinechin]

gt;req_vqs[i]);
> -}
> -g_free(fs->req_vqs);
> +vuf_cleanup_vqs(vdev);
>  virtio_cleanup(vdev);
> -g_free(fs->vhost_dev.vqs);
>  return;
>  }
>
> @@ -258,7 +282,6 @@ static void vuf_device_unrealize(DeviceState *dev)
>  {
>  VirtIODevice *vdev = VIRTIO_DEVICE(dev);
>  VHostUserFS *fs = VHOST_USER_FS(dev);
> -int i;
>
>  /* This will stop vhost backend if appropriate. */
>  vuf_set_status(vdev, 0);
> @@ -267,14 +290,8 @@ static void vuf_device_unrealize(DeviceState *dev)
>
>  vhost_user_cleanup(>vhost_user);
>
> -virtio_delete_queue(fs->hiprio_vq);
> -for (i = 0; i < fs->conf.num_request_queues; i++) {
> -virtio_delete_queue(fs->req_vqs[i]);
> -}
> -g_free(fs->req_vqs);
> +vuf_cleanup_vqs(vdev);
>  virtio_cleanup(vdev);
> -g_free(fs->vhost_dev.vqs);
> -fs->vhost_dev.vqs = NULL;
>  }
>
>  static const VMStateDescription vuf_vmstate = {


--
Cheers,
Christophe de Dinechin (IRC c3d)

Re: [Virtio-fs] [PATCH 09/13] virtiofsd: Specify size of notification buffer using config space

[Christophe de Dinechin]

 qidx)
>  return false;
>  }
>
> +static uint64_t fv_get_protocol_features(VuDev *dev)
> +{
> +return 1ull << VHOST_USER_PROTOCOL_F_CONFIG;
> +}
> +
> +static int fv_get_config(VuDev *dev, uint8_t *config, uint32_t len)
> +{
> +struct virtio_fs_config fscfg = {};
> +unsigned notify_size, roundto = 64;
> +union fuse_notify_union {
> +struct fuse_notify_poll_wakeup_out  wakeup_out;
> +struct fuse_notify_inval_inode_out  inode_out;
> +struct fuse_notify_inval_entry_out  entry_out;
> +struct fuse_notify_delete_out   delete_out;
> +struct fuse_notify_store_outstore_out;
> +struct fuse_notify_retrieve_out retrieve_out;
> +};
> +
> +notify_size = sizeof(struct fuse_out_header) +
> +  sizeof(union fuse_notify_union);
> +notify_size = ((notify_size + roundto) / roundto) * roundto;
> +
> +fscfg.notify_buf_size = notify_size;
> +memcpy(config, , len);
> +fuse_log(FUSE_LOG_DEBUG, "%s:Setting notify_buf_size=%d\n", __func__,
> + fscfg.notify_buf_size);
> +return 0;
> +}
> +
>  static const VuDevIface fv_iface = {
>  .get_features = fv_get_features,
>  .set_features = fv_set_features,
> @@ -864,6 +893,8 @@ static const VuDevIface fv_iface = {
>  .queue_set_started = fv_queue_set_started,
>
>  .queue_is_processed_in_order = fv_queue_order,
> +.get_protocol_features = fv_get_protocol_features,
> +.get_config = fv_get_config,
>  };
>
>  /*


--
Cheers,
Christophe de Dinechin (IRC c3d)

Re: [Virtio-fs] [PATCH 07/13] virtiofsd: Release file locks using F_UNLCK

[Christophe de Dinechin]

On 2021-09-30 at 11:30 -04, Vivek Goyal  wrote...
> We are emulating posix locks for guest using open file description locks
> in virtiofsd. When any of the fd is closed in guest, we find associated
> OFD lock fd (if there is one) and close it to release all the locks.
>
> Assumption here is that there is no other thread using lo_inode_plock
> structure or plock->fd, hence it is safe to do so.
>
> But now we are about to introduce blocking variant of locks (SETLKW),
> and that means we might be waiting to a lock to be available and
> using plock->fd. And that means there are still users of plock
> structure.
>
> So release locks using fcntl(SETLK, F_UNLCK) instead of closing fd
> and plock will be freed later when lo_inode is being freed.
>
> Signed-off-by: Vivek Goyal 
> Signed-off-by: Ioannis Angelakopoulos 
> ---
>  tools/virtiofsd/passthrough_ll.c | 21 +
>  1 file changed, 17 insertions(+), 4 deletions(-)
>
> diff --git a/tools/virtiofsd/passthrough_ll.c 
> b/tools/virtiofsd/passthrough_ll.c
> index 38b2af8599..6928662e22 100644
> --- a/tools/virtiofsd/passthrough_ll.c
> +++ b/tools/virtiofsd/passthrough_ll.c
> @@ -1557,9 +1557,6 @@ static void unref_inode(struct lo_data *lo, struct 
> lo_inode *inode, uint64_t n)
>  lo_map_remove(>ino_map, inode->fuse_ino);
>  g_hash_table_remove(lo->inodes, >key);
>  if (lo->posix_lock) {
> -if (g_hash_table_size(inode->posix_locks)) {
> -fuse_log(FUSE_LOG_WARNING, "Hash table is not empty\n");
> -}
>  g_hash_table_destroy(inode->posix_locks);
>  pthread_mutex_destroy(>plock_mutex);
>  }
> @@ -2266,6 +2263,8 @@ static void lo_flush(fuse_req_t req, fuse_ino_t ino, 
> struct fuse_file_info *fi)
>  (void)ino;
>  struct lo_inode *inode;
>  struct lo_data *lo = lo_data(req);
> +struct lo_inode_plock *plock;
> +struct flock flock;
>
>  inode = lo_inode(req, ino);
>  if (!inode) {
> @@ -2282,8 +2281,22 @@ static void lo_flush(fuse_req_t req, fuse_ino_t ino, 
> struct fuse_file_info *fi)
>  /* An fd is going away. Cleanup associated posix locks */
>  if (lo->posix_lock) {
>  pthread_mutex_lock(>plock_mutex);
> -g_hash_table_remove(inode->posix_locks,

I'm curious why the g_hash_table_remove above is not in the 'if' below?

> +plock = g_hash_table_lookup(inode->posix_locks,
>  GUINT_TO_POINTER(fi->lock_owner));
> +
> +if (plock) {
> +/*
> + * An fd is being closed. For posix locks, this means
> + * drop all the associated locks.
> + */
> +memset(, 0, sizeof(struct flock));
> +flock.l_type = F_UNLCK;
> +flock.l_whence = SEEK_SET;
> +/* Unlock whole file */
> +flock.l_start = flock.l_len = 0;
> +fcntl(plock->fd, F_OFD_SETLK, );
> +}
> +
>  pthread_mutex_unlock(>plock_mutex);
>  }
>  res = close(dup(lo_fi_fd(req, fi)));


--
Cheers,
Christophe de Dinechin (IRC c3d)

Re: [PATCH v2 2/9] backends/tpm: Replace qemu_mutex_lock calls with QEMU_LOCK_GUARD

[Christophe de Dinechin]

On 2021-05-06 at 15:37 CEST, Philippe Mathieu-Daudé wrote...
> Simplify the tpm_emulator_ctrlcmd() handler by replacing a pair of
> qemu_mutex_lock/qemu_mutex_unlock calls by the WITH_QEMU_LOCK_GUARD
> macro.
>
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  backends/tpm/tpm_emulator.c | 34 +++---
>  1 file changed, 15 insertions(+), 19 deletions(-)
>
> diff --git a/backends/tpm/tpm_emulator.c b/backends/tpm/tpm_emulator.c
> index a012adc1934..e5f1063ab6c 100644
> --- a/backends/tpm/tpm_emulator.c
> +++ b/backends/tpm/tpm_emulator.c
> @@ -30,6 +30,7 @@
>  #include "qemu/error-report.h"
>  #include "qemu/module.h"
>  #include "qemu/sockets.h"
> +#include "qemu/lockable.h"
>  #include "io/channel-socket.h"
>  #include "sysemu/tpm_backend.h"
>  #include "sysemu/tpm_util.h"
> @@ -124,31 +125,26 @@ static int tpm_emulator_ctrlcmd(TPMEmulator *tpm, 
> unsigned long cmd, void *msg,
>  uint32_t cmd_no = cpu_to_be32(cmd);
>  ssize_t n = sizeof(uint32_t) + msg_len_in;
>  uint8_t *buf = NULL;
> -int ret = -1;
>
> -qemu_mutex_lock(>mutex);
> +WITH_QEMU_LOCK_GUARD(>mutex) {
> +buf = g_alloca(n);
> +memcpy(buf, _no, sizeof(cmd_no));
> +memcpy(buf + sizeof(cmd_no), msg, msg_len_in);
>
> -buf = g_alloca(n);
> -memcpy(buf, _no, sizeof(cmd_no));
> -memcpy(buf + sizeof(cmd_no), msg, msg_len_in);
> -
> -n = qemu_chr_fe_write_all(dev, buf, n);
> -if (n <= 0) {
> -goto end;
> -}
> -
> -if (msg_len_out != 0) {
> -n = qemu_chr_fe_read_all(dev, msg, msg_len_out);
> +n = qemu_chr_fe_write_all(dev, buf, n);
>  if (n <= 0) {
> -goto end;
> +return -1;
> +}
> +
> +if (msg_len_out != 0) {
> +n = qemu_chr_fe_read_all(dev, msg, msg_len_out);
> +if (n <= 0) {
> +return -1;
> +}
>  }
>  }
>
> -ret = 0;
> -
> -end:
> -qemu_mutex_unlock(>mutex);
> -return ret;
> +return 0;
>  }
>
>  static int tpm_emulator_unix_tx_bufs(TPMEmulator *tpm_emu,

I really like the improvement, but it looks like it does not belong to the
top-level series (i.e. not related to replacing alloca() by g_malloc()).

Reviewed-by: Christophe de Dinechin 

--
Cheers,
Christophe de Dinechin (IRC c3d)

Re: vnc clipboard support

[Christophe de Dinechin]

> On 1 Feb 2021, at 17:56, Daniel P. Berrangé  wrote:
> 
> On Mon, Feb 01, 2021 at 05:31:52PM +0100, Christophe de Dinechin wrote:
>> 
>> 
>>> On 1 Feb 2021, at 16:51, Daniel P. Berrangé  wrote:
>>> 
>>> On Mon, Feb 01, 2021 at 04:27:43PM +0100, Christophe de Dinechin wrote:
>>>> 
>>>> 
>>>>> On 29 Jan 2021, at 15:32, Daniel P. Berrangé  wrote:
>>>>> 
>>>>> On Fri, Jan 29, 2021 at 03:19:45PM +0100, Christophe de Dinechin wrote:
>>>>>> 
>>>>>> 
>>>>>>> On 29 Jan 2021, at 12:08, Daniel P. Berrangé  
>>>>>>> wrote:
>>>>>>> 
>>>>>>> On Fri, Jan 29, 2021 at 11:50:10AM +0100, Christophe de Dinechin wrote:
>>>>>>>> 
>>>>>>>> 
>>>>>>>>> On 29 Jan 2021, at 09:03, Gerd Hoffmann  wrote:
>>>>>>>>> 
>>>>>>>>> Hi,
>>>>>>>>> 
>>>>>>>>>>> (1) Have some guest agent (spice does it that way).
>>>>>>>>>>> Advantage: more flexible, allows more features.
>>>>>>>>>>> Disadvantage: requires agent in the guest.
>>>>>>>>>> 
>>>>>>>>>> What about running the option to relay data to a VNC server in the
>>>>>>>>>> guest if there is one running? In other words, using an existing
>>>>>>>>>> VNC server as an agent, with the option of having a stripped-down,
>>>>>>>>>> clipboard only VNC server as a later optimization.
>>>>>>>>> 
>>>>>>>>> Well, if you run Xvnc in the guest anyway why use the qemu vnc server
>>>>>>>>> in the first place?
>>>>>>>> 
>>>>>>>> I assume that if you use the qemu VNC, it's because you you don't want 
>>>>>>>> to
>>>>>>>> run Xvnc on some external network, or care about accessing the guest
>>>>>>>> before Xvnc can be launched. There can be many reasons.
>>>>>>>> 
>>>>>>>> Again, I want to make it clear that my suggestion is _not_ simply to 
>>>>>>>> access
>>>>>>>> the existing Xvnc as is, but rather to stick with some VNC server code 
>>>>>>>> to handle
>>>>>>>> the clipboard if / when possible.
>>>>>>>> 
>>>>>>>> Let me try to imagine a scenario, where I'll use a macOS guest 
>>>>>>>> intentionally
>>>>>>>> to clarify what I was thinking about.
>>>>>>>> 
>>>>>>>> - During early boot, there is no in-guest VNC server, so to address 
>>>>>>>> that,
>>>>>>>> you connect to the qemu VNC. At this stage, all screen refresh is 
>>>>>>>> handled
>>>>>>>> by the qemu VNC, and the best you can do if you want to support any
>>>>>>>> kind of copy-paste is to convert it to virtual keystrokes. The same 
>>>>>>>> would
>>>>>>>> be true for Linux on a text console.
>>>>>>>> 
>>>>>>>> - Then comes up you macOS guest, and it still has no VNC port open,
>>>>>>>> so you are stuck with qemu-vnc doing all the work. But now you can
>>>>>>>> enable screen sharing, and that launches the Apple-maintained macOS
>>>>>>>> VNC server.
>>>>>>>> 
>>>>>>>> - Let's assume for illustration that this listens on some private 
>>>>>>>> network
>>>>>>>> that qemu can access, but that is not visible externally. In this case,
>>>>>>>> you could not VNC to the guest, but you can still VNC to qemu.
>>>>>>>> 
>>>>>>>> - What I'm suggesting is that qemu-vnc could then switch to simply
>>>>>>>> relaying VNC traffic to that in-guest server. You'd get the smart 
>>>>>>>> update
>>>>>>>> algorithm that Apple has put in place to deal with transparency and the
>>>>>>>> like, as well as a level of guest OS integration that would otherwise 
>>>>>>>> be
>>>>>>>> m

Re: vnc clipboard support

[Christophe de Dinechin]

> On 29 Jan 2021, at 16:04, Gerd Hoffmann  wrote:
> 
>  Hi,
> 
>> Unless we para-virtualize the keyboard?
> 
> The main advantage of paste via key events is that it doesn't require
> guest support.

The main disadvantage, though, is that it does not work at all ;-)

Imagine pasting Ademar's favorite signature, :wq!
Or pasting "ahs" to a guest that thinks the control key is down
(a not uncommon occurence with VNC, in my personal experience).
So now you pasted "Select All, Delete, Save" in many GUI apps.

(insert infinite variations on the "doom" theme here)

>  Requiring any kind of software in the guest (paravirt
> keyboard driver, agent figuring the keymap, whatever else) kills that
> advantage.

Indeed. I was pointing out that any option that brought any modicum
of reasonable paste behavior (ignoring copy for the moment) required
some in-guest piece of software. So we might as well go for one that
is known to work, and also deals with the copy-from-guest issue.


>  And if we need guest cooperation anyway it is much better
> to just read/write the guest clipboard directly.

… which is exactly what VNC in the guest knows how to do in a
reasonable way, on a large number of platforms. So I don't see
any benefit to reinvent that wheel. I'd rather leverage the VNC
code to build a solid, multi-platform agent. You can extract the
clipboard code into some smaller agent that talks to a specific qemu
character device if you think it's a worthy optimization.


> 
> Standard keyboard seems to not be an option either.  The HID specs
> (https://usb.org/document-library/hid-usage-tables-121) lists a
> Unicode Page (Section 18), which looks like it could be used to
> send unicode chars to the guest.  Problem is (a) this is 16bit
> only (so no emoji) and (b) widely unimplemented in guest OSes.

Some operating systems have a way to enter unicode code points.
On Linux, you can sometimes type shift-control u (hex code), but it's a bit
dependent on the some alignment of mysterious planets.
On Windows, you can type  alt-x. Sometimes. Or alt + .
On macOS, you need to enable Unicode Hex Input, so unreliable.

Not that I would recommend trying any of that ;-)

Re: vnc clipboard support

[Christophe de Dinechin]

> On 1 Feb 2021, at 16:51, Daniel P. Berrangé  wrote:
> 
> On Mon, Feb 01, 2021 at 04:27:43PM +0100, Christophe de Dinechin wrote:
>> 
>> 
>>> On 29 Jan 2021, at 15:32, Daniel P. Berrangé  wrote:
>>> 
>>> On Fri, Jan 29, 2021 at 03:19:45PM +0100, Christophe de Dinechin wrote:
>>>> 
>>>> 
>>>>> On 29 Jan 2021, at 12:08, Daniel P. Berrangé  wrote:
>>>>> 
>>>>> On Fri, Jan 29, 2021 at 11:50:10AM +0100, Christophe de Dinechin wrote:
>>>>>> 
>>>>>> 
>>>>>>> On 29 Jan 2021, at 09:03, Gerd Hoffmann  wrote:
>>>>>>> 
>>>>>>> Hi,
>>>>>>> 
>>>>>>>>> (1) Have some guest agent (spice does it that way).
>>>>>>>>>  Advantage: more flexible, allows more features.
>>>>>>>>>  Disadvantage: requires agent in the guest.
>>>>>>>> 
>>>>>>>> What about running the option to relay data to a VNC server in the
>>>>>>>> guest if there is one running? In other words, using an existing
>>>>>>>> VNC server as an agent, with the option of having a stripped-down,
>>>>>>>> clipboard only VNC server as a later optimization.
>>>>>>> 
>>>>>>> Well, if you run Xvnc in the guest anyway why use the qemu vnc server
>>>>>>> in the first place?
>>>>>> 
>>>>>> I assume that if you use the qemu VNC, it's because you you don't want to
>>>>>> run Xvnc on some external network, or care about accessing the guest
>>>>>> before Xvnc can be launched. There can be many reasons.
>>>>>> 
>>>>>> Again, I want to make it clear that my suggestion is _not_ simply to 
>>>>>> access
>>>>>> the existing Xvnc as is, but rather to stick with some VNC server code 
>>>>>> to handle
>>>>>> the clipboard if / when possible.
>>>>>> 
>>>>>> Let me try to imagine a scenario, where I'll use a macOS guest 
>>>>>> intentionally
>>>>>> to clarify what I was thinking about.
>>>>>> 
>>>>>> - During early boot, there is no in-guest VNC server, so to address that,
>>>>>> you connect to the qemu VNC. At this stage, all screen refresh is handled
>>>>>> by the qemu VNC, and the best you can do if you want to support any
>>>>>> kind of copy-paste is to convert it to virtual keystrokes. The same would
>>>>>> be true for Linux on a text console.
>>>>>> 
>>>>>> - Then comes up you macOS guest, and it still has no VNC port open,
>>>>>> so you are stuck with qemu-vnc doing all the work. But now you can
>>>>>> enable screen sharing, and that launches the Apple-maintained macOS
>>>>>> VNC server.
>>>>>> 
>>>>>> - Let's assume for illustration that this listens on some private network
>>>>>> that qemu can access, but that is not visible externally. In this case,
>>>>>> you could not VNC to the guest, but you can still VNC to qemu.
>>>>>> 
>>>>>> - What I'm suggesting is that qemu-vnc could then switch to simply
>>>>>> relaying VNC traffic to that in-guest server. You'd get the smart update
>>>>>> algorithm that Apple has put in place to deal with transparency and the
>>>>>> like, as well as a level of guest OS integration that would otherwise be
>>>>>> much harder to replicate.
>>>>> 
>>>>> IMHO that's an awful lot of complexity to add to the system
>>>>> that isn't especially useful and this opens up new attack
>>>>> vectors for the guest to exploit the host.
>>>>> 
>>>>> If people have VNC/RDP/whatever configured inside their guest
>>>>> OS, then there's really little to no reason for them to want
>>>>> to connect to the QEMU VNC server, as viewing initial bootup
>>>>> phase is not required in normal case. The only time such
>>>>> people will need to use the QEMU VNC server is if the guest
>>>>> OS has broken in some way before it fully booted and thus failed
>>>>> to start the guest VNC server. There is no guest VNC server
>>>>> to hand off to in this scenario.
>>>> 
>&g

Re: vnc clipboard support

[Christophe de Dinechin]

> On 29 Jan 2021, at 15:32, Daniel P. Berrangé  wrote:
> 
> On Fri, Jan 29, 2021 at 03:19:45PM +0100, Christophe de Dinechin wrote:
>> 
>> 
>>> On 29 Jan 2021, at 12:08, Daniel P. Berrangé  wrote:
>>> 
>>> On Fri, Jan 29, 2021 at 11:50:10AM +0100, Christophe de Dinechin wrote:
>>>> 
>>>> 
>>>>> On 29 Jan 2021, at 09:03, Gerd Hoffmann  wrote:
>>>>> 
>>>>> Hi,
>>>>> 
>>>>>>> (1) Have some guest agent (spice does it that way).
>>>>>>>   Advantage: more flexible, allows more features.
>>>>>>>   Disadvantage: requires agent in the guest.
>>>>>> 
>>>>>> What about running the option to relay data to a VNC server in the
>>>>>> guest if there is one running? In other words, using an existing
>>>>>> VNC server as an agent, with the option of having a stripped-down,
>>>>>> clipboard only VNC server as a later optimization.
>>>>> 
>>>>> Well, if you run Xvnc in the guest anyway why use the qemu vnc server
>>>>> in the first place?
>>>> 
>>>> I assume that if you use the qemu VNC, it's because you you don't want to
>>>> run Xvnc on some external network, or care about accessing the guest
>>>> before Xvnc can be launched. There can be many reasons.
>>>> 
>>>> Again, I want to make it clear that my suggestion is _not_ simply to access
>>>> the existing Xvnc as is, but rather to stick with some VNC server code to 
>>>> handle
>>>> the clipboard if / when possible.
>>>> 
>>>> Let me try to imagine a scenario, where I'll use a macOS guest 
>>>> intentionally
>>>> to clarify what I was thinking about.
>>>> 
>>>> - During early boot, there is no in-guest VNC server, so to address that,
>>>> you connect to the qemu VNC. At this stage, all screen refresh is handled
>>>> by the qemu VNC, and the best you can do if you want to support any
>>>> kind of copy-paste is to convert it to virtual keystrokes. The same would
>>>> be true for Linux on a text console.
>>>> 
>>>> - Then comes up you macOS guest, and it still has no VNC port open,
>>>> so you are stuck with qemu-vnc doing all the work. But now you can
>>>> enable screen sharing, and that launches the Apple-maintained macOS
>>>> VNC server.
>>>> 
>>>> - Let's assume for illustration that this listens on some private network
>>>> that qemu can access, but that is not visible externally. In this case,
>>>> you could not VNC to the guest, but you can still VNC to qemu.
>>>> 
>>>> - What I'm suggesting is that qemu-vnc could then switch to simply
>>>> relaying VNC traffic to that in-guest server. You'd get the smart update
>>>> algorithm that Apple has put in place to deal with transparency and the
>>>> like, as well as a level of guest OS integration that would otherwise be
>>>> much harder to replicate.
>>> 
>>> IMHO that's an awful lot of complexity to add to the system
>>> that isn't especially useful and this opens up new attack
>>> vectors for the guest to exploit the host.
>>> 
>>> If people have VNC/RDP/whatever configured inside their guest
>>> OS, then there's really little to no reason for them to want
>>> to connect to the QEMU VNC server, as viewing initial bootup
>>> phase is not required in normal case. The only time such
>>> people will need to use the QEMU VNC server is if the guest
>>> OS has broken in some way before it fully booted and thus failed
>>> to start the guest VNC server. There is no guest VNC server
>>> to hand off to in this scenario.
>> 
>> It's a matter of what you want to do with that qemu vnc.
>> 
>> If it's only a backup when there's nothing in the guest to help,
>> then maybe trying to support copy-paste is not a good idea.
>> 
>> If it's a standard go-to access point for connecting to your
>> guest, then making it smart is hard, but useful.
>> 
>>> 
>>> The value of the QEMU host side VNC server is that it works
>>> for all possible guest OS, no matter whether they are running
>>> normally or broken, regardless of what the guest admin has
>>> configured in terms of guest level remote desktop.
>> 
>> Understood.
>> 
>> The downside is that there are things it can't do. It cann

Re: vnc clipboard support

[Christophe de Dinechin]

> On 29 Jan 2021, at 12:08, Daniel P. Berrangé  wrote:
> 
> On Fri, Jan 29, 2021 at 11:50:10AM +0100, Christophe de Dinechin wrote:
>> 
>> 
>>> On 29 Jan 2021, at 09:03, Gerd Hoffmann  wrote:
>>> 
>>> Hi,
>>> 
>>>>> (1) Have some guest agent (spice does it that way).
>>>>>Advantage: more flexible, allows more features.
>>>>>Disadvantage: requires agent in the guest.
>>>> 
>>>> What about running the option to relay data to a VNC server in the
>>>> guest if there is one running? In other words, using an existing
>>>> VNC server as an agent, with the option of having a stripped-down,
>>>> clipboard only VNC server as a later optimization.
>>> 
>>> Well, if you run Xvnc in the guest anyway why use the qemu vnc server
>>> in the first place?
>> 
>> I assume that if you use the qemu VNC, it's because you you don't want to
>> run Xvnc on some external network, or care about accessing the guest
>> before Xvnc can be launched. There can be many reasons.
>> 
>> Again, I want to make it clear that my suggestion is _not_ simply to access
>> the existing Xvnc as is, but rather to stick with some VNC server code to 
>> handle
>> the clipboard if / when possible.
>> 
>> Let me try to imagine a scenario, where I'll use a macOS guest intentionally
>> to clarify what I was thinking about.
>> 
>> - During early boot, there is no in-guest VNC server, so to address that,
>> you connect to the qemu VNC. At this stage, all screen refresh is handled
>> by the qemu VNC, and the best you can do if you want to support any
>> kind of copy-paste is to convert it to virtual keystrokes. The same would
>> be true for Linux on a text console.
>> 
>> - Then comes up you macOS guest, and it still has no VNC port open,
>> so you are stuck with qemu-vnc doing all the work. But now you can
>> enable screen sharing, and that launches the Apple-maintained macOS
>> VNC server.
>> 
>> - Let's assume for illustration that this listens on some private network
>> that qemu can access, but that is not visible externally. In this case,
>> you could not VNC to the guest, but you can still VNC to qemu.
>> 
>> - What I'm suggesting is that qemu-vnc could then switch to simply
>> relaying VNC traffic to that in-guest server. You'd get the smart update
>> algorithm that Apple has put in place to deal with transparency and the
>> like, as well as a level of guest OS integration that would otherwise be
>> much harder to replicate.
> 
> IMHO that's an awful lot of complexity to add to the system
> that isn't especially useful and this opens up new attack
> vectors for the guest to exploit the host.
> 
> If people have VNC/RDP/whatever configured inside their guest
> OS, then there's really little to no reason for them to want
> to connect to the QEMU VNC server, as viewing initial bootup
> phase is not required in normal case. The only time such
> people will need to use the QEMU VNC server is if the guest
> OS has broken in some way before it fully booted and thus failed
> to start the guest VNC server. There is no guest VNC server
> to hand off to in this scenario.

It's a matter of what you want to do with that qemu vnc.

If it's only a backup when there's nothing in the guest to help,
then maybe trying to support copy-paste is not a good idea.

If it's a standard go-to access point for connecting to your
guest, then making it smart is hard, but useful.

> 
> The value of the QEMU host side VNC server is that it works
> for all possible guest OS, no matter whether they are running
> normally or broken, regardless of what the guest admin has
> configured in terms of guest level remote desktop.

Understood.

The downside is that there are things it can't do. It cannot correctly
determine the keyboard map, because that's controlled in software
in the guest.

Unless we para-virtualize the keyboard?

> 
> IOW, if you have a guest remote desktop solution you'll just
> use that 99% of the time. If you don't have that, then you'll
> use QEMU VNC/SPICE server, and there won't be anything in the
> guest for that to proxy to/from.

If really the assumption is there is nothing in the guest to help
us, then I'd say "paste" should come up with a warning "do you want
me to try and translate that into keystrokes", and I don't see how to
implement copy from a graphical display without OCR…


Cheers
Christophe

Re: vnc clipboard support

[Christophe de Dinechin]

> On 29 Jan 2021, at 12:49, Gerd Hoffmann  wrote:
> 
>  Hi,
> 
>> - What I'm suggesting is that qemu-vnc could then switch to simply
>> relaying VNC traffic to that in-guest server. You'd get the smart update
>> algorithm that Apple has put in place to deal with transparency and the
>> like, as well as a level of guest OS integration that would otherwise be
>> much harder to replicate.
> 
> Ok, so basically add a vnc proxy mode and allow switching between
> internals server and proxy mode (automatically if possible, or
> manually).

Yes.

> 
>> If you don't have it, you fallback to virtual keystrokes, which you can't
>> dispense with because of the early boot case, but which will at best
>> deal with simple ASCII (making that work with a clipboard containing
>> tabs and Control-C/Control-V characters is left as an exercise for the
>> reader  ;-)
> 
> Well, even simple ascii is tricky.  How do you send 'z', as KEY_Z or as
> KEY_Y ?

Exactly. Scancode / keymap conversions are no joke. and qemu has
very little information about the current state of the guest in that respect.

> 
>> Finally, an optimization I alluded to is to have an agent which is basically
>> a stripped-down VNC server dealing with only the clipboard aspect, and
>> that is your agent. In other words, you don't' invent yet another protocol,
>> but you furiously copy-paste the existing VNC code.
> 
> Third mode: proxy only clipboard messages.
> 
>> Hope this sounds a bit less crazy explained that way…
> 
> Well, I can't see a way to make all that work nicely fully automatic ...

I see many aspects that are difficult, like disconnecting / reconnecting,
dealing with versions of the protocol, capability changes, and so on.

It was quite hard for SPICE to switch between console and streaming
agent, so I can understand why the prospect does not sound appealing.
Maybe that was just a bad idea.

> 
> take care,
>  Gerd
> 

Re: vnc clipboard support

[Christophe de Dinechin]

> On 29 Jan 2021, at 09:03, Gerd Hoffmann  wrote:
> 
>  Hi,
> 
>>> (1) Have some guest agent (spice does it that way).
>>> Advantage: more flexible, allows more features.
>>> Disadvantage: requires agent in the guest.
>> 
>> What about running the option to relay data to a VNC server in the
>> guest if there is one running? In other words, using an existing
>> VNC server as an agent, with the option of having a stripped-down,
>> clipboard only VNC server as a later optimization.
> 
> Well, if you run Xvnc in the guest anyway why use the qemu vnc server
> in the first place?

I assume that if you use the qemu VNC, it's because you you don't want to
run Xvnc on some external network, or care about accessing the guest
before Xvnc can be launched. There can be many reasons.

Again, I want to make it clear that my suggestion is _not_ simply to access
the existing Xvnc as is, but rather to stick with some VNC server code to handle
the clipboard if / when possible.

Let me try to imagine a scenario, where I'll use a macOS guest intentionally
to clarify what I was thinking about.

- During early boot, there is no in-guest VNC server, so to address that,
you connect to the qemu VNC. At this stage, all screen refresh is handled
by the qemu VNC, and the best you can do if you want to support any
kind of copy-paste is to convert it to virtual keystrokes. The same would
be true for Linux on a text console.

- Then comes up you macOS guest, and it still has no VNC port open,
so you are stuck with qemu-vnc doing all the work. But now you can
enable screen sharing, and that launches the Apple-maintained macOS
VNC server.

- Let's assume for illustration that this listens on some private network
that qemu can access, but that is not visible externally. In this case,
you could not VNC to the guest, but you can still VNC to qemu.

- What I'm suggesting is that qemu-vnc could then switch to simply
relaying VNC traffic to that in-guest server. You'd get the smart update
algorithm that Apple has put in place to deal with transparency and the
like, as well as a level of guest OS integration that would otherwise be
much harder to replicate.

I believe that the same holds true with X as well. If you can activate or
detect an Xvnc, you are much better off using _that_ to deal with the
idiosyncrasies of Xclipboard rather than try to convert keycodes.

If you don't have it, you fallback to virtual keystrokes, which you can't
dispense with because of the early boot case, but which will at best
deal with simple ASCII (making that work with a clipboard containing
tabs and Control-C/Control-V characters is left as an exercise for the
reader  ;-)

Finally, an optimization I alluded to is to have an agent which is basically
a stripped-down VNC server dealing with only the clipboard aspect, and
that is your agent. In other words, you don't' invent yet another protocol,
but you furiously copy-paste the existing VNC code.


Hope this sounds a bit less crazy explained that way…
Thanks
Christophe

Re: vnc clipboard support

[Christophe de Dinechin]

Christophe
(Typos are from my iPhone)

> Le 28 janv. 2021 à 21:24, Marc-André Lureau  a 
> écrit :
> 
> 
> Hi
> 
>> On Thu, Jan 28, 2021 at 9:14 PM Gerd Hoffmann  wrote:
>>   Hi folks,
>> 
>> I'm looking for a good way to implement cut+paste support for vnc.
>> 
>> The vnc core protocol has support for text/plain cut+paste, and there
>> is an extension adding support for other formats.  That'll cover one
>> part of the problem, exchanging cut+paste data between vnc client and
>> qemu vnc server.
>> 
>> The tricky part is the second: the guest <=> qemu communication.
>> I see basically two possible approaches here:
>> 
>>   (1) Have some guest agent (spice does it that way).
>>   Advantage: more flexible, allows more features.
>>   Disadvantage: requires agent in the guest.
>> 
>>   (2) Send text as key events.
>>   Advantage: no guest agent needed.
>>   Disadvantage: is translated by guests keyboard map, so qemu
>>   needs to know the map for proper char -> key event translation.
>>   Only works for text/plain and only for chars you can easily
>>   type, anything needing input methods (emoji  for example)
>>   isn't going to fly.
>> 
>> I think that (1) is clearly the better way.  Given that the agent
>> would need to run in user wayland/xorg session context the existing
>> qemu-guest-agent will not work.  Also linking against some UI library
>> like gtk3 for clipboard handling is not something we want for the
>> qemu-guest-agent.  So we need another one, I'll name it
>> qemu-clipboard-agent for the rest of this mail.  And we need a
>> communication channel.
>> 
>> I'd tend to model the qemu-clipboard-agent simliar to the
>> qemu-guest-agent, i.e. have some stream as communication path and run
>> some stream protocol over it.
>> 
>> Stream options I see are (in order of personal preference):
>> 
>>   (1) New virtio-serial port.  virtio-serial likely is there anyway
>>   for the qemu-guest-agent ...
>> 
>>   (2) Have qemu-clipboard-agent and qemu-guest-agent share the agent
>>   channel, i.e. qemu-clipboard-agent will proxy everything through
>>   qemu-guest-agent (spice does it that way).
>> 
>> Protocol options I see are (not sure yet which to prefer, need to have
>> a closer look at the candidates):
>> 
>>   (1) Add clipboard commands to QMP and use these.
>> 
>>   (2) Reuse the clipboard bits of the vnc protocol (forward
>>   VNC_MSG_CLIENT_CUT_TEXT messages to the guest agent)
>> 
>>   (3) Reuse the clipboard bits of the spice-agent protocol.
>> 
>>   (4) Reuse the clipboard bits of the wayland protocol.
>> 
>> Once we have sorted the qemu <-> guest communication path it should be
>> possible to also hook up other UIs (specifically gtk) without too much
>> effort.  Which probably makes (2) a rather poor choice.
>> 
>> Comments?
>> Suggestions?
>> Other ideas?
> 
> 
> 
> I also had recently some thoughts about how to implement clipboard sharing in 
> a more general way for QEMU.
> 
> I admit I like Christophe's suggestion ("it's somebody else problem"), but it 
> falls short to me as I don't know of a common open-source remoting solution 
> for various operating systems, and I don't see how it could integrate well 
> with our UI and remote protocols. Or look at reusing some VirtualBox code 
> perhaps?

Just to clarify, I did not think of my suggestion as “SEP” but more as a way to 
leverage existing knowledge regarding guest OS clipboard. More a “what should 
we steal” approach. 

> 
> Some things I keep in mind:
> - the spice protocol had a number of iterations to fix some races. It would 
> be great not to repeat the same mistakes, and I don't know if VNC have the 
> same flaws or not.
> - the spice agent design isn't great: the system agent proxies messages to 
> the active session. It would be nice if the new solution didn't have such a 
> middle-man.
> - with wayland, clipboard sharing isn't really possible. Or not in a seamless 
> way at least. Today it kinda works with some X11 compatibility extensions, 
> but this will eventually go away or change behaviour.
> - the GNOME desktop is working on remoting using RDP, and they are 
> implementing a DBus interface for it 
> (https://gitlab.gnome.org/jadahl/mutter/-/commits/wip/remote-desktop-clipboard)
> - it's not just about clipboard. We would also want to have some kind of drag 
> and drop (even if limited to files like Spice atm). We may want some 
> windowing integration. We may also want to have access to some desktop 
> services: apps, documents etc.. And what's not.
> 
> That leads me to think that virtio-serial is not very well suited, as it 
> doesn't allow various services / processes to come and go. I see vsock as a 
> much better alternative. (I also wonder if it handles control flow any better 
> btw)
> 
> I think we shoud work on getting the free desktops our best-class support. To 
> me, this means we need to speak the lingua franca, which is DBus. The great 
> thing is that DBus is 

Re: vnc clipboard support

[Christophe de Dinechin]

> On 28 Jan 2021, at 18:12, Gerd Hoffmann  wrote:
> 
>  Hi folks,
> 
> I'm looking for a good way to implement cut+paste support for vnc.
> 
> The vnc core protocol has support for text/plain cut+paste, and there
> is an extension adding support for other formats.  That'll cover one
> part of the problem, exchanging cut+paste data between vnc client and
> qemu vnc server.
> 
> The tricky part is the second: the guest <=> qemu communication.
> I see basically two possible approaches here:
> 
>  (1) Have some guest agent (spice does it that way).
>  Advantage: more flexible, allows more features.
>  Disadvantage: requires agent in the guest.

What about running the option to relay data to a VNC server in the
guest if there is one running? In other words, using an existing
VNC server as an agent, with the option of having a stripped-down,
clipboard only VNC server as a later optimization.

The benefit I see is that the work of adjusting between the VNC
clipboard protocol and the local OS clipboard is something that is
likely already done when porting VNC, so you leverage that, even
for exotic operating systems. And in many cases, VNC may already
be part of the distro / OS, or at least easy to find.

Then it's a matter of choice whether you want to talk to some
real guest network, or use a character device and route data
through some other non-network path.

> 
>  (2) Send text as key events.
>  Advantage: no guest agent needed.
>  Disadvantage: is translated by guests keyboard map, so qemu
>  needs to know the map for proper char -> key event translation.
>  Only works for text/plain and only for chars you can easily
>  type, anything needing input methods (emoji  for example)
>  isn't going to fly.


> 
> I think that (1) is clearly the better way.  Given that the agent
> would need to run in user wayland/xorg session context the existing
> qemu-guest-agent will not work.  Also linking against some UI library
> like gtk3 for clipboard handling is not something we want for the
> qemu-guest-agent.  So we need another one, I'll name it
> qemu-clipboard-agent for the rest of this mail.  And we need a
> communication channel.
> 
> I'd tend to model the qemu-clipboard-agent simliar to the
> qemu-guest-agent, i.e. have some stream as communication path and run
> some stream protocol over it.
> 
> Stream options I see are (in order of personal preference):
> 
>  (1) New virtio-serial port.  virtio-serial likely is there anyway
>  for the qemu-guest-agent ...
> 
>  (2) Have qemu-clipboard-agent and qemu-guest-agent share the agent
>  channel, i.e. qemu-clipboard-agent will proxy everything through
>  qemu-guest-agent (spice does it that way).
> 
> Protocol options I see are (not sure yet which to prefer, need to have
> a closer look at the candidates):
> 
>  (1) Add clipboard commands to QMP and use these.
> 
>  (2) Reuse the clipboard bits of the vnc protocol (forward
>  VNC_MSG_CLIENT_CUT_TEXT messages to the guest agent)
> 
>  (3) Reuse the clipboard bits of the spice-agent protocol.
> 
>  (4) Reuse the clipboard bits of the wayland protocol.
> 
> Once we have sorted the qemu <-> guest communication path it should be
> possible to also hook up other UIs (specifically gtk) without too much
> effort.  Which probably makes (2) a rather poor choice.
> 
> Comments?
> Suggestions?
> Other ideas?
> 
> take care,
>  Gerd
> 
> 

Re: [PATCH] x86/cpu: Add AVX512_FP16 cpu feature

[Christophe de Dinechin]

On 2020-12-16 at 23:40 CET, Cathy Zhang wrote...
> AVX512 Half-precision floating point (FP16) has better performance
> compared to FP32 if the presicion or magnitude requirements are met.

spelling: precision

> It's defined as CPUID.(EAX=7,ECX=0):EDX[bit 23].
>
> Refer to
> https://software.intel.com/content/www/us/en/develop/download/\
> intel-architecture-instruction-set-extensions-programming-reference.html
>
> Signed-off-by: Cathy Zhang 
> ---
>  target/i386/cpu.c | 2 +-
>  target/i386/cpu.h | 2 ++
>  2 files changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index ca997a68cd..c4d623b8b9 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -979,7 +979,7 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = 
> {
>  "avx512-vp2intersect", NULL, "md-clear", NULL,
>  NULL, NULL, "serialize", NULL,
>  "tsx-ldtrk", NULL, NULL /* pconfig */, NULL,
> -NULL, NULL, NULL, NULL,
> +NULL, NULL, NULL, "avx512-fp16",
>  NULL, NULL, "spec-ctrl", "stibp",
>  NULL, "arch-capabilities", "core-capability", "ssbd",
>  },
> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> index c4a49c06a8..6fd675c654 100644
> --- a/target/i386/cpu.h
> +++ b/target/i386/cpu.h
> @@ -784,6 +784,8 @@ typedef uint64_t FeatureWordArray[FEATURE_WORDS];
>  #define CPUID_7_0_EDX_SERIALIZE (1U << 14)
>  /* TSX Suspend Load Address Tracking instruction */
>  #define CPUID_7_0_EDX_TSX_LDTRK (1U << 16)
> +/* AVX512_FP16 instruction */
> +#define CPUID_7_0_EDX_AVX512_FP16   (1U << 23)
>  /* Speculation Control */
>  #define CPUID_7_0_EDX_SPEC_CTRL (1U << 26)
>  /* Single Thread Indirect Branch Predictors */


--
Cheers,
Christophe de Dinechin (IRC c3d)

Re: VFIO Migration

[Christophe de Dinechin]

> On 3 Nov 2020, at 19:49, Dr. David Alan Gilbert  wrote:
> 
> * Stefan Hajnoczi (stefa...@redhat.com ) wrote:
>> On Tue, Nov 03, 2020 at 12:17:09PM +, Dr. David Alan Gilbert wrote:
>>> * Stefan Hajnoczi (stefa...@redhat.com) wrote:
 Device Models
 -
 Devices have a *hardware interface* consisting of hardware registers,
 interrupts, and so on.
 
 The hardware interface together with the device state representation is 
 called
 a *device model*. Device models can be assigned URIs such as
 https://qemu.org/devices/e1000e to uniquely identify them.
>>> 
>>> I think this is a unique identifier, not actually a URI; the https://
>>> isn't needed since no one expects to ever connect to this.
>> 
>> Yes, it could be any unique string. If the URI idea is not popular we
>> can use any similar scheme.
> 
> I'm OK with it being a URI; just drop the https.

I completely agree. https gives the wrong idea about what this represents. 
Unless you give it https semantics, by requiring a doc or a schema or whatever 
to be at the URL, but then you enter another universe of cans of worms.

Re: VFIO Migration

[Christophe de Dinechin]

ine between configuration and state may be
a bit fuzzy, even for a single device model, depending on the use case.

>
> Device Versions
> ---
> As a device evolves, the number of configuration parameters required may 
> become
> inconvenient for users to express in full. A device configuration can be
> aliased by a *device version*, which is a shorthand for the full device
> configuration. This makes it easy to apply a standard device configuration
> without listing every configuration parameter explicitly.
>
> For example, if address filtering support was added to a network card then
> device versions and the corresponding configurations may look like this:
> * ``version=1`` - Behaves as if ``rx-filter-size=0``
> * ``version=2`` - ``rx-filter-size=32``

To me, this corresponds to default settings, see below.

If two devices have different versions, do you allow migration?

>
> Device States
> -
> The details of the device state representation are not covered in this 
> document
> but the general requirements are discussed here.
>
> The device state consists of data accessible through the device's hardware
> interface and internal state that is needed to restore device operation.
> State in the hardware interface includes the values of hardware registers.
> An example of internal state is an index value needed to avoid processing
> queued requests more than once.



>
> Changes can be made to the device state representation as follows. Each change
> to device state must have a corresponding device configuration parameter that
> allows the change to toggled:
>
> * When the parameter is disabled the hardware interface and device state
>   representation are unchanged. This allows old device states to be loaded.
>
> * When the parameter is enabled the change comes into effect.
>
> * The parameter's default value disables the change. Therefore old versions do
>   not have to explicitly specify the parameter.

I see a problem with this. Imagine a new card has new parameter foo.
Now, you once had a VM on this card that had foo=42. So it has
foo-enabled=true and foo=42. Then you migrate there something that does not
know about foo. Most likely, that would not even touch foo-enabled.

So I think that you need to add that the migration starts with a "reset
state" where all featured are disabled by default.

If that's the case, then you don't really need the "enabled" flag. You
simply need to state that the reset state is compatible with earlier
versions. If you know about the new feature, you set it. If you don't know,
you have the state of the previous version.

Setting a version could allow you to quickly change the defaults.



>
> The following example illustrates migration from an old device
> implementation to a new one. A version=1 network card is migrated to a
> new device implementation that is also capable of version=2 and adds the
> rx-filter-size=32 parameter. The new device is instantiated with
> version=1, which disables rx-filter-size and is capable of loading the
> version=1 device state. The migration completes successfully but note
> the device is still operating at version=1 level in the new device.
>
> The following example illustrates migration from a new device
> implementation back to an older one. The new device implementation
> supports version=1 and version=2. The old device implementation supports
> version=1 only. Therefore the device can only be migrated when
> instantiated with version=1 or the equivalent full configuration
> parameters.
>
> Orchestrating Migrations
> 
> The following steps must be followed to migrate devices:
>
> 1. Check that the source and destination devices support the same device 
> model.
>
> 2. Check that the destination device supports the source device's
>configuration. Each configuration parameter must be accepted by the
>destination in order to ensure that it will be possible to load the device
>state.
>
> 3. The device state is saved on the source and loaded on the destination.
>
> 4. If migration succeeds then the destination resumes operation and the source
>must not resume operation. If the migration fails then the source resumes
>operation and the destination must not resume operation.
>
> VFIO Implementation
> ---
> The following applies both to kernel VFIO/mdev drivers and vfio-user device
> backends.
>
> Devices are instantiated based on a version and/or configuration parameters:
> * ``version=1`` - use the device configuration aliased by version 1
> * ``version=2,rx-filter-size=64`` - use version 1 and override 
> ``rx-filter-size``
> * ``rx-filter-size=0`` - directly set configuration parameters without using 
> a version
>
> Device creation fails if the version and/or configuration parameters are not
> supported.
>
> There must be a mechanism to query the "latest" configuration for a device
> model. It may simply report the ``version=5`` where 5 is the latest version 
> but
> it could also report all configuration parameters instead of using a version
> alias.

Instead of "latest", we could have a query that lists the "supported"
configurations. Again, vGPUs are a good example where this would be
useful. A same card can be partitioned in a number of ways, and you can't
really claim that "M10-2B" or "M10-0Q" is "latest".

You could arguably assign a unique URI to each sub-model. Maybe that's how
you were envisioning things?

--
Cheers,
Christophe de Dinechin (IRC c3d)

Re: [PATCH v2 5/6] tools/virtiofsd: xattr name mappings: Map server xattr names

[Christophe de Dinechin]

On 2020-08-27 at 17:36 CEST, Dr. David Alan Gilbert (git) wrote...
> From: "Dr. David Alan Gilbert" 
>
> Map xattr names coming from the server, i.e. the host filesystem;
> currently this is only from listxattr.
>
> Signed-off-by: Dr. David Alan Gilbert 
> ---
>  tools/virtiofsd/passthrough_ll.c | 88 
>  1 file changed, 88 insertions(+)
>
> diff --git a/tools/virtiofsd/passthrough_ll.c 
> b/tools/virtiofsd/passthrough_ll.c
> index 9b9c8f3ab1..7cd99186f7 100644
> --- a/tools/virtiofsd/passthrough_ll.c
> +++ b/tools/virtiofsd/passthrough_ll.c
> @@ -2188,6 +2188,42 @@ static int xattr_map_client(const char *client_name, 
> char **out_name)
>  abort();
>  }
>
> +/*
> + * For use with listxattr where the server fs gives us a name and we may need
> + * to sanitize this for the client.
> + * Returns a pointer to the result in *out_name
> + *   This is always the original string or the current string with some 
> prefix
> + *   removed; no reallocation is done.
> + * Returns 0 on success
> + * Can return -ENODATA to indicate the name should be dropped from the list.
> + */
> +static int xattr_map_server(const char *server_name, const char **out_name)
> +{
> +const XattrMapEntry *cur_entry;
> +for (cur_entry = xattr_map_list; ; cur_entry++) {
> +if ((cur_entry->flags & XATTR_MAP_FLAG_SERVER) &&
> +(!strncmp(cur_entry->prepend,
> +  server_name,
> +  strlen(cur_entry->prepend {

Overall, the same remarks apply as for the client side.

> +if (cur_entry->flags & XATTR_MAP_FLAG_END_BAD) {
> +return -ENODATA;
> +}
> +if (cur_entry->flags & XATTR_MAP_FLAG_END_OK) {
> +*out_name = server_name;
> +return 0;
> +}
> +if (cur_entry->flags & XATTR_MAP_FLAG_PREFIX) {
> +/* Remove prefix */
> +*out_name = server_name + strlen(cur_entry->prepend);
> +return 0;
> +}
> +}
> +}
> +
> +/* Shouldn't get here - rules should have an END_* */
> +abort();
> +}
> +
>  static void lo_getxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name,
>  size_t size)
>  {
> @@ -2342,8 +2378,60 @@ static void lo_listxattr(fuse_req_t req, fuse_ino_t 
> ino, size_t size)
>  if (ret == 0) {
>  goto out;
>  }
> +
> +if (lo->xattrmap) {
> +/*
> + * Map the names back, some attributes might be dropped,
> + * some shortened, but not increased, so we shouldn't
> + * run out of room.
> + */
> +size_t out_index, in_index;
> +out_index = 0;
> +in_index = 0;
> +while (in_index < ret) {
> +const char *map_out;
> +char *in_ptr = value + in_index;
> +/* Length of current attribute name */
> +size_t in_len = strlen(value + in_index) + 1;
> +
> +int mapret = xattr_map_server(in_ptr, _out);
> +if (mapret != -ENODATA && mapret != 0) {
> +/* Shouldn't happen */
> +saverr = -mapret;
> +goto out;
> +}
> +if (mapret == 0) {
> +/* Either unchanged, or truncated */
> +size_t out_len;
> +if (map_out != in_ptr) {
> +/* +1 copies the NIL */
> +out_len = strlen(map_out) + 1;
> +} else {
> +/* No change */
> +out_len = in_len;
> +}
> +/*
> + * Move result along, may still be needed for an 
> unchanged
> + * entry if a previous entry was changed.
> + */
> +memmove(value + out_index, map_out, out_len);
> +
> +out_index += out_len;
> +}
> +in_index += in_len;
> +}
> +ret = out_index;
> +    if (ret == 0) {
> +goto out;
> +}
> +}
>  fuse_reply_buf(req, value, ret);
>  } else {
> +/*
> + * xattrmap only ever shortens the result,
> + * so we don't need to do anything clever with the
> + * allocation length here.
> + */
>  fuse_reply_xattr(req, ret);
>  }
>  out_free:


--
Cheers,
Christophe de Dinechin (IRC c3d)

Re: [PATCH v2 5/6] tools/virtiofsd: xattr name mappings: Map server xattr names

[Christophe de Dinechin]

On 2020-08-27 at 17:36 CEST, Dr. David Alan Gilbert (git) wrote...
> From: "Dr. David Alan Gilbert" 
>
> Map xattr names coming from the server, i.e. the host filesystem;
> currently this is only from listxattr.
>
> Signed-off-by: Dr. David Alan Gilbert 
> ---
>  tools/virtiofsd/passthrough_ll.c | 88 
>  1 file changed, 88 insertions(+)
>
> diff --git a/tools/virtiofsd/passthrough_ll.c 
> b/tools/virtiofsd/passthrough_ll.c
> index 9b9c8f3ab1..7cd99186f7 100644
> --- a/tools/virtiofsd/passthrough_ll.c
> +++ b/tools/virtiofsd/passthrough_ll.c
> @@ -2188,6 +2188,42 @@ static int xattr_map_client(const char *client_name, 
> char **out_name)
>  abort();
>  }
>
> +/*
> + * For use with listxattr where the server fs gives us a name and we may need
> + * to sanitize this for the client.
> + * Returns a pointer to the result in *out_name
> + *   This is always the original string or the current string with some 
> prefix
> + *   removed; no reallocation is done.
> + * Returns 0 on success
> + * Can return -ENODATA to indicate the name should be dropped from the list.
> + */
> +static int xattr_map_server(const char *server_name, const char **out_name)

> +{
> +const XattrMapEntry *cur_entry;
> +for (cur_entry = xattr_map_list; ; cur_entry++) {
> +if ((cur_entry->flags & XATTR_MAP_FLAG_SERVER) &&
> +(!strncmp(cur_entry->prepend,
> +  server_name,
> +  strlen(cur_entry->prepend {

Might be slightly clearer (and possibly faster) as

strstart(cur_entry->prepend, server_name, )

> +if (cur_entry->flags & XATTR_MAP_FLAG_END_BAD) {
> +return -ENODATA;
> +}
> +if (cur_entry->flags & XATTR_MAP_FLAG_END_OK) {
> +*out_name = server_name;
> +return 0;
> +}
> +if (cur_entry->flags & XATTR_MAP_FLAG_PREFIX) {
> +/* Remove prefix */
> +*out_name = server_name + strlen(cur_entry->prepend);

With the above, that would be

*out_name = end;


> +return 0;
> +}
> +}
> +}
> +
> +/* Shouldn't get here - rules should have an END_* */

You probably want to point the finger back to parse_xattrmap() in the comment?

> +abort();

> +}
> +
>  static void lo_getxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name,
>  size_t size)
>  {
> @@ -2342,8 +2378,60 @@ static void lo_listxattr(fuse_req_t req, fuse_ino_t 
> ino, size_t size)
>  if (ret == 0) {
>  goto out;
>  }
> +
> +if (lo->xattrmap) {

If you put the "XattrMapEntry" pointer in lo, then you would probably test that.

> +/*
> + * Map the names back, some attributes might be dropped,
> + * some shortened, but not increased, so we shouldn't
> + * run out of room.
> + */
> +size_t out_index, in_index;
> +out_index = 0;
> +in_index = 0;
> +while (in_index < ret) {
> +const char *map_out;
> +char *in_ptr = value + in_index;
> +/* Length of current attribute name */
> +size_t in_len = strlen(value + in_index) + 1;
> +
> +int mapret = xattr_map_server(in_ptr, _out);
> +if (mapret != -ENODATA && mapret != 0) {
> +/* Shouldn't happen */
> +saverr = -mapret;
> +goto out;
> +}
> +if (mapret == 0) {
> +/* Either unchanged, or truncated */
> +size_t out_len;
> +if (map_out != in_ptr) {
> +/* +1 copies the NIL */
> +out_len = strlen(map_out) + 1;
> +} else {
> +/* No change */
> +out_len = in_len;
> +}
> +/*
> + * Move result along, may still be needed for an 
> unchanged
> + * entry if a previous entry was changed.
> + */
> +memmove(value + out_index, map_out, out_len);
> +
> +out_index += out_len;
> +}
> +in_index += in_len;
> +}
> +ret = out_index;
> +if (ret == 0) {
> +goto out;
> +    }
> +    }
>  fuse_reply_buf(req, value, ret);
>  } else {
> +/*
> + * xattrmap only ever shortens the result,
> + * so we don't need to do anything clever with the
> + * allocation length here.
> + */

I don't understand the comment above. We are in the !lo->xattrmap) case, no?

>  fuse_reply_xattr(req, ret);
>  }
>  out_free:


--
Cheers,
Christophe de Dinechin (IRC c3d)

Re: [PATCH v2 3/6] tools/virtiofsd: xattr name mappings: Add option

[Christophe de Dinechin]

 g_strndup(map, tmp - map);
> +map = tmp + 1;
> +
> +/* At start of 'prepend' field */
> +tmp = strchr(map, sep);
> +if (!tmp) {
> +fuse_log(FUSE_LOG_ERR,
> + "%s: Missing '%c' at end of prepend field of rule %zu",
> + __func__, sep, nentries);
> +exit(1);
> +}
> +res[nentries - 1].prepend = g_strndup(map, tmp - map);
> +map = tmp + 1;
> +/* End of rule - go around again for another rule */
> +}
> +
> +if (!nentries) {
> +fuse_log(FUSE_LOG_ERR, "Empty xattr map\n");
> +exit(1);
> +}
> +
> +/* Add a terminator to error in cases the user hasn't specified */
> +res = g_realloc_n(res, ++nentries, sizeof(XattrMapEntry));
> +res[nentries - 1].flags = XATTR_MAP_FLAG_ALL | XATTR_MAP_FLAG_END_BAD;
> +res[nentries - 1].key = g_strdup("");
> +res[nentries - 1].prepend = g_strdup("");
> +
> +return res;
> +}
> +
>  static void lo_getxattr(fuse_req_t req, fuse_ino_t ino, const char *name,
>  size_t size)
>  {
> @@ -2909,6 +3052,11 @@ int main(int argc, char *argv[])
>  } else {
>  lo.source = strdup("/");
>  }
> +
> +if (lo.xattrmap) {
> +xattr_map_list = parse_xattrmap(lo.xattrmap);

This is never freed. If you put the static in struct lo_data, you could
naturally clean it up in fuse_lo_data_cleanup.

> +}
> +
>  if (!lo.timeout_set) {
>  switch (lo.cache) {
>  case CACHE_NONE:


--
Cheers,
Christophe de Dinechin (IRC c3d)

Re: Python docstrings and licensing/authorship

[Christophe de Dinechin]

> On 16 Sep 2020, at 17:47, John Snow  wrote:
> 
> For some of the Python cleanup work I am doing, I am moving preamble comments 
> into docstrings. These docstrings are visible in interactive editors and may 
> be visible when using Sphinx to generate documentation manuals for Python 
> code.
> 
> My instinct is to remove the licensing and authorship information from the 
> preamble and leave the docstring only with information functionally relevant 
> to the module, while leaving licensing and authorship information in a 
> comment (above? below?).
> 
> The end effect would be that using e.g. `help(qapi.parser)` in the 
> interactive Python shell would not show licensing or copyright for the 
> module, but it would still be visible in the source file, as always.
> 
> Is this in bad taste? Do we have strong feelings about authorship and 
> licensing being visible in help output and generated documentation?

What about putting that in a separate pseudo-entity, so that help(copyright) 
would show it?

> 
> --js
> 
> 

Qemu web site down?

[Christophe de Dinechin]

Hi Stefan,


The link from https://wiki.qemu.org/Documentation pointing to 
https://qemu.weilnetz.de/doc/qemu-doc.html seems to be dead. Is the problem on 
your web site, or should the wiki be updated?


Thanks
Christophe

Re: [PATCH v2 2/3] trace: Add support for recorder back-end

[Christophe de Dinechin]

On 2020-07-30 at 10:13 CEST, Markus Armbruster wrote...
> Christophe de Dinechin  writes:
>
>> On 2020-07-29 at 13:53 CEST, Markus Armbruster wrote...
>>> Christophe de Dinechin  writes:
>>>
>>>> On 2020-07-27 at 10:23 CEST, Markus Armbruster wrote...
>>>>> Christophe de Dinechin  writes:
>>>>>
>>>>>> On 2020-07-23 at 16:06 CEST, Markus Armbruster wrote...
>>>>>>> Christophe de Dinechin  writes:
>>> [...]
>>>>>>>> I'm certainly not against adding a command-line option to activate 
>>>>>>>> recorder
>>>>>>>> options specifically, but as I understand, the option -trace already 
>>>>>>>> exists,
>>>>>>>> and its semantics is sufficiently different from the one in recorder
>>>>>>>> patterns that I decided to not connect the two for now. For example, to
>>>>>>>> disable trace foo, you'd pass "-foo" to the -trace option, but "foo=0" 
>>>>>>>> to
>>>>>>>> RECORDER_TRACES. The parsing of graphing options and other related
>>>>>>>> recorder-specific stuff is a bit difficult to integrate with -trace 
>>>>>>>> too.
>>>>>>>
>>>>>>> We need proper integration with the existing trace UI.
>>>>>>
>>>>>> I agree, but I don't think this belongs to this particular patch series.
>>>>>> See below why.
>>>>>>
>>>>>>>
>>>>>>> In particular, the ability to enable and disable trace events
>>>>>>> dynamically provided by QMP commands trace-event-get-state,
>>>>>>> trace-event-set-state, and HMP command trace-event is really useful.
>>>>>>
>>>>>> That ability exists, but given the many differences between the
>>>>>> recorder and other tracing mechanisms, I found it useful to add a 
>>>>>> specific
>>>>>> "recorder" command.
>>>>>
>>>>> Precedence for commands specific to a trace backend: trace-file.
>>>>>
>>>>> Name yours trace-recorder?
>>>>
>>>> But then "recorder dump" does not fit with any "trace" concept.
>>>
>>> Once you make the recorder a trace backend, whatever the recorder does
>>> becomes a trace concept :)
>>
>> I understand your point, but I want to make a distinction between recorder
>> tracing and other recorder features. Does that make sense?
>
> Maybe :)  My thoughts haven't settled, yet.
>
>>>>>> For example, assuming I built with both recorder and log trace backends,
>>>>>> from the monitor, I can type:
>>>>>>
>>>>>>   trace-event kvm_run_exit on
>>>>>>
>>>>>> What I get then is something like that:
>>>>>>
>>>>>>   2091091@1595518935.441273:kvm_run_exit cpu_index 0, reason 2
>>>>>>   2091091@1595518935.441292:kvm_run_exit cpu_index 0, reason 2
>>>>>>   2091091@1595518935.441301:kvm_run_exit cpu_index 0, reason 2
>>>>>>   2091091@1595518935.441309:kvm_run_exit cpu_index 0, reason 2
>>>>>>   2091091@1595518935.441254:kvm_run_exit cpu_index 0, reason 2
>>>>>>
>>>>>> It would not be very useful to activate recorder traces as well when that
>>>>>> happens, which would have the undesired side effect of purging any
>>>>>>> corresponding entry from a following recorder dump.
>>>>>
>>>>> I'm afraid I don't understand, because the gaps in my understanding of
>>>>> what the recorder can do are just too big.
>>>>
>>>> There is a video at the top of https://github.com/c3d/recorder, or direct
>>>> link https://www.youtube.com/watch?v=kEnQY1zFa0Y. Hope this helps.
>>>>
>>>>
>>>>>
>>>>> From your cover letter:
>>>>>
>>>>> 1. Flight recorder: Dump information on recent events in case of crash
>>>>>
>>>>> Define "recent events", please.  Is it all trace events (except for the
>>>>> ones disabled at build time, of course)?
>>>>
>>>> For event categories only known through qemu trace definitions, by default,
>>>
>>> Right now, there are 

Re: [PATCH 2/7] build: fix device module builds

[Christophe de Dinechin]

On 2020-07-28 at 18:37 CEST, Philippe Mathieu-Daudé wrote...
> On 7/23/20 7:46 PM, Christophe de Dinechin wrote:
>> From: Gerd Hoffmann 
>>
>> See comment.  Feels quite hackish.  Better ideas anyone?
>
> I don't understand this patch, how is it related to the rest of
> your series?

It's a leftover from an earlier workaround. For some reason, only some
spurious submodule changes remained over time. That patch can now be
dropped, I believe.

>
>>
>> Signed-off-by: Gerd Hoffmann 
>> Signed-off-by: Christophe de Dinechin 
>> ---
>>  dtc   | 2 +-
>>  roms/SLOF | 2 +-
>>  roms/openbios | 2 +-
>>  roms/opensbi  | 2 +-
>>  roms/seabios  | 2 +-
>>  slirp | 2 +-
>>  6 files changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/dtc b/dtc
>> index 85e5d83984..88f18909db 16
>> --- a/dtc
>> +++ b/dtc
>> @@ -1 +1 @@
>> -Subproject commit 85e5d839847af54efab170f2b1331b2a6421e647
>> +Subproject commit 88f18909db731a627456f26d779445f84e449536
>> diff --git a/roms/SLOF b/roms/SLOF
>> index e18ddad851..9546892a80 16
>> --- a/roms/SLOF
>> +++ b/roms/SLOF
>> @@ -1 +1 @@
>> -Subproject commit e18ddad8516ff2cfe36ec130200318f7251aa78c
>> +Subproject commit 9546892a80d5a4c73deea6719de46372f007f4a6
>> diff --git a/roms/openbios b/roms/openbios
>> index 75fbb41d28..7e5b89e429 16
>> --- a/roms/openbios
>> +++ b/roms/openbios
>> @@ -1 +1 @@
>> -Subproject commit 75fbb41d2857d93208c74a8e0228b29fd7bf04c0
>> +Subproject commit 7e5b89e4295063d8eba55b9c8ce8bc681c2d129a
>> diff --git a/roms/opensbi b/roms/opensbi
>> index 9f1b72ce66..be92da280d 16
>> --- a/roms/opensbi
>> +++ b/roms/opensbi
>> @@ -1 +1 @@
>> -Subproject commit 9f1b72ce66d659e91013b358939e832fb27223f5
>> +Subproject commit be92da280d87c38a2e0adc5d3f43bab7b5468f09
>> diff --git a/roms/seabios b/roms/seabios
>> index 88ab0c1552..f21b5a4aeb 16
>> --- a/roms/seabios
>> +++ b/roms/seabios
>> @@ -1 +1 @@
>> -Subproject commit 88ab0c15525ced2eefe39220742efe4769089ad8
>> +Subproject commit f21b5a4aeb020f2a5e2c6503f906a9349dd2f069
>> diff --git a/slirp b/slirp
>> index 2faae0f778..126c04acba 16
>> --- a/slirp
>> +++ b/slirp
>> @@ -1 +1 @@
>> -Subproject commit 2faae0f778f818fadc873308f983289df697eb93
>> +Subproject commit 126c04acbabd7ad32c2b018fe10dfac2a3bc1210
>>


--
Cheers,
Christophe de Dinechin (IRC c3d)

Re: [PATCH v2 2/3] trace: Add support for recorder back-end

[Christophe de Dinechin]

On 2020-07-29 at 13:53 CEST, Markus Armbruster wrote...
> Christophe de Dinechin  writes:
>
>> On 2020-07-27 at 10:23 CEST, Markus Armbruster wrote...
>>> Christophe de Dinechin  writes:
>>>
>>>> On 2020-07-23 at 16:06 CEST, Markus Armbruster wrote...
>>>>> Christophe de Dinechin  writes:
> [...]
>>>>>> I'm certainly not against adding a command-line option to activate 
>>>>>> recorder
>>>>>> options specifically, but as I understand, the option -trace already 
>>>>>> exists,
>>>>>> and its semantics is sufficiently different from the one in recorder
>>>>>> patterns that I decided to not connect the two for now. For example, to
>>>>>> disable trace foo, you'd pass "-foo" to the -trace option, but "foo=0" to
>>>>>> RECORDER_TRACES. The parsing of graphing options and other related
>>>>>> recorder-specific stuff is a bit difficult to integrate with -trace too.
>>>>>
>>>>> We need proper integration with the existing trace UI.
>>>>
>>>> I agree, but I don't think this belongs to this particular patch series.
>>>> See below why.
>>>>
>>>>>
>>>>> In particular, the ability to enable and disable trace events
>>>>> dynamically provided by QMP commands trace-event-get-state,
>>>>> trace-event-set-state, and HMP command trace-event is really useful.
>>>>
>>>> That ability exists, but given the many differences between the
>>>> recorder and other tracing mechanisms, I found it useful to add a specific
>>>> "recorder" command.
>>>
>>> Precedence for commands specific to a trace backend: trace-file.
>>>
>>> Name yours trace-recorder?
>>
>> But then "recorder dump" does not fit with any "trace" concept.
>
> Once you make the recorder a trace backend, whatever the recorder does
> becomes a trace concept :)

I understand your point, but I want to make a distinction between recorder
tracing and other recorder features. Does that make sense?


>
>>>> For example, assuming I built with both recorder and log trace backends,
>>>> from the monitor, I can type:
>>>>
>>>>   trace-event kvm_run_exit on
>>>>
>>>> What I get then is something like that:
>>>>
>>>>   2091091@1595518935.441273:kvm_run_exit cpu_index 0, reason 2
>>>>   2091091@1595518935.441292:kvm_run_exit cpu_index 0, reason 2
>>>>   2091091@1595518935.441301:kvm_run_exit cpu_index 0, reason 2
>>>>   2091091@1595518935.441309:kvm_run_exit cpu_index 0, reason 2
>>>>   2091091@1595518935.441254:kvm_run_exit cpu_index 0, reason 2
>>>>
>>>> It would not be very useful to activate recorder traces as well when that
>>>> happens, which would have the undesired side effect of purging any
>>>> corresponding entry from a following recorder dump.
>>>
>>> I'm afraid I don't understand, because the gaps in my understanding of
>>> what the recorder can do are just too big.
>>
>> There is a video at the top of https://github.com/c3d/recorder, or direct
>> link https://www.youtube.com/watch?v=kEnQY1zFa0Y. Hope this helps.
>>
>>
>>>
>>> From your cover letter:
>>>
>>> 1. Flight recorder: Dump information on recent events in case of crash
>>>
>>> Define "recent events", please.  Is it all trace events (except for the
>>> ones disabled at build time, of course)?
>>
>> For event categories only known through qemu trace definitions, by default,
>
> Right now, there are no others, aren't there?

There is one in the second patch, the example. That was actually the whole
point of that second patch, which is not otherwise particularly exciting.

>
>> it's the last 8 events. If you manually declare a recorder, then you can
>> specify any size.
>>
>> (The difference between this manual recorder usage and the recorder backend
>> generated code is similar to the difference between the log backend and
>> "DPRINTF")
>
> I'm not sure I get the parenthesis.

I took that example because it's mentioned in docs/devel/tracing.txt:

=== Log ===

The "log" backend sends trace events directly to standard error.  This
effectively turns trace events into debug printfs.

This is the simplest backend and can be used together with existing code 
that
uses DPRINTF().

Just the s

Re: Missing qapi_free_Type in error case for qapi generated code?

[Christophe de Dinechin]

On 2020-07-29 at 10:34 CEST, Markus Armbruster wrote...
> Eric Blake  writes:
>
>> On 7/28/20 10:26 AM, Christophe de Dinechin wrote:
>>> The qapi generated code for qmp_marshal_query_spice seems to be missing a
>>> resource deallocation for "retval". For example, for SpiceInfo:
>>>
>>
>>>  retval = qmp_query_spice();
>>>  error_propagate(errp, err);
>>>  if (err) {
>>> /* retval not freed here */
>>
>> Because it should be NULL here.  Returning an error AND an object is
>> frowned on.
>
> It's forbidden, actually.  The QMP handler must either succeed and
> return a value, or fail cleanly.

OK. Then I guess Eric's suggestion to add an assert is the correct
approach, with the caveat you identified.

>
> Since it has to return a value even when it fails, it returns an error
> value then.  "Cleanly" means the error value does not require cleanup.
>
> The generated marshalling function relies on this: it *ignores* the
> error value.
>
>>> /* Missing: qapi_free_SpiceInfo(retval); */
>>>  goto out;
>>>  }
>>>
>>>  qmp_marshal_output_SpiceInfo(retval, ret, errp);
>>
>> And here, retval was non-NULL, but is cleaned as a side-effect of
>> qmp_marshal_output_SpiceInfo.
>>
>>>
>>> out:
>>
>> So no matter how you get to the label, retval is no longer valid
>> memory that can be leaked.
>>
>>>  visit_free(v);
>>>  v = qapi_dealloc_visitor_new();
>>>  visit_start_struct(v, NULL, NULL, 0, NULL);
>>>  visit_end_struct(v, NULL);
>>>  visit_free(v);
>>> }
>>> #endif /* defined(CONFIG_SPICE) */
>>>
>>> Questions:
>>>
>>> - Is the query code supposed to always return NULL in case of error?
>>
>> Yes.  If not, that is a bug in qmp_query_spice.
>
> Correct.
>
>>> In the
>>>case of hmp_info_spice, there is no check for info==NULL, so on the
>
> I'm blind.  Where?

In hmp_info_spice, there is this code:

info = qmp_query_spice(NULL);

if (!info->enabled) {
monitor_printf(mon, "Server: disabled\n");
goto out;
}

I guess this code relies on qmp_query_spice never returning an error.
Why is that a safe assumption?

This came to my attention because I wanted to return an error and a NULL
value for modular spice if the module is not available.

>
>>>contrary, it seems to indicate that a non-null result is always expected,
>>>and that function does call qapi_free_SpiceInfo
>>
>> Calling qapi_free_SpiceInfo(NULL) is a safe no-op.  Or if you expect
>> the function to always succeed, you could pass _abort as the
>> errp parameter.
>>
>>>
>>> - If not, is there an existing shortcut to generate the correct deallocation
>>>code for return types that need it? You can't just use
>>>qapi_free_%(c_type)s because that would generate an extra * character,
>>>i.e. I get "SpiceInfo *" and not "SpiceInfo".
>>
>> Ah, you're debating about editing scripts/qapi/commands.py.  If
>> anything, an edit to add 'assert(!retval)' if qmp_COMMAND failed might
>> be smarter than trying to add code to free retval.
>
> This is more complicated than it may seem.
>
> The "natural" error value for a pointer-valued function is NULL.  I'm
> confident the handlers use it.  assert(!retval) should work.
>
> For functions returning something else, people may have different ideas
> on what to return on error.  To make assert(!retval) work, they need to
> return something "falsish".  I'm not ready to bet my own money on all of
> them doing that.

That's what I was thinking too. Glad to confirm I was not reading that code
too wrong.

>
> Aside: only functions in pragma returns-whitelist can return
> non-pointer.
>
>>> - If not, is there any good way to know if the type is a pointer type?
>>>(A quick look in cripts/qapi/types.py does not show anything obvious)
>
> No clean way exists, simply because there has been no need.  So far,
> we've always found a reasonable way to generate code that works whether
> types are pointers in C or not.
>
>> Look at scripts/qapi/schema.py; each QAPI metatype has implementations
>> of .c_name and .c_type that determine how to represent that QAPI
>> object in C.  You probably want c_name instead of c_type when
>> constructing the name of a qapi_free_FOO function, but that goes back
>> to my question of whether such a call is even needed.
>
> Method c_name() returns a string you can interpolate into C identifiers.
>
> Method c_type() returns a string you can use as C type in generated
> code.

That part I understood. I was looking for something like method c_basetype()
or something like that for the non-pointer type of a pointer.

>
> The QAPI scripts could use more comments.

+1.

--
Cheers,
Christophe de Dinechin (IRC c3d)

Re: [PATCH v7 01/47] block: Add child access functions

[Christophe de Dinechin]

On 2020-07-13 at 11:06 CEST, Vladimir Sementsov-Ogievskiy wrote...
> 25.06.2020 18:21, Max Reitz wrote:
>> There are BDS children that the general block layer code can access,
>> namely bs->file and bs->backing.  Since the introduction of filters and
>> external data files, their meaning is not quite clear.  bs->backing can
>> be a COW source, or it can be a filtered child; bs->file can be a
>> filtered child, it can be data and metadata storage, or it can be just
>> metadata storage.
>>
>> This overloading really is not helpful.  This patch adds functions that
>> retrieve the correct child for each exact purpose.  Later patches in
>> this series will make use of them.  Doing so will allow us to handle
>> filter nodes in a meaningful way.
>>
>> Signed-off-by: Max Reitz 
>> ---
>
> [..]
>
>> +/*
>> + * Return the primary child of this node: For filters, that is the
>> + * filtered child.  For other nodes, that is usually the child storing
>> + * metadata.
>> + * (A generally more helpful description is that this is (usually) the
>> + * child that has the same filename as @bs.)
>> + *
>> + * Drivers do not necessarily have a primary child; for example quorum
>> + * does not.
>> + */
>> +BdrvChild *bdrv_primary_child(BlockDriverState *bs)
>> +{
>> +BdrvChild *c;
>> +
>> +QLIST_FOREACH(c, >children, next) {
>> +if (c->role & BDRV_CHILD_PRIMARY) {
>> +return c;
>> +}
>> +}
>> +
>> +return NULL;
>> +}
>>
>
> Suggest squash-in to also assert that not more than one primary child:
> --- a/block.c
> +++ b/block.c
> @@ -6998,13 +6998,14 @@ BdrvChild *bdrv_filter_or_cow_child(BlockDriverState 
> *bs)
>*/
>   BdrvChild *bdrv_primary_child(BlockDriverState *bs)
>   {
> -BdrvChild *c;
> +BdrvChild *c, *found = NULL;
>
>   QLIST_FOREACH(c, >children, next) {
>   if (c->role & BDRV_CHILD_PRIMARY) {
> -return c;
> +assert(!found);
> +found = c;
>   }
>   }
>
> -return NULL;
> +return c;

Shouldn't that be "return found"?
>   }
>
>
> with or without:
> Reviewed-by: Vladimir Sementsov-Ogievskiy 


--
Cheers,
Christophe de Dinechin (IRC c3d)

Missing qapi_free_Type in error case for qapi generated code?

[Christophe de Dinechin]

The qapi generated code for qmp_marshal_query_spice seems to be missing a
resource deallocation for "retval". For example, for SpiceInfo:

#if defined(CONFIG_SPICE)
void qmp_marshal_query_spice(QDict *args, QObject **ret, Error **errp)
{
Error *err = NULL;
bool ok = false;
Visitor *v;
SpiceInfo *retval;

v = qobject_input_visitor_new(QOBJECT(args));
if (!visit_start_struct(v, NULL, NULL, 0, errp)) {
goto out;
}
ok = visit_check_struct(v, errp);
visit_end_struct(v, NULL);
if (!ok) {
goto out;
}

retval = qmp_query_spice();
error_propagate(errp, err);
if (err) {
/* retval not freed here */
/* Missing: qapi_free_SpiceInfo(retval); */
goto out;
}

qmp_marshal_output_SpiceInfo(retval, ret, errp);

out:
visit_free(v);
v = qapi_dealloc_visitor_new();
visit_start_struct(v, NULL, NULL, 0, NULL);
visit_end_struct(v, NULL);
visit_free(v);
}
#endif /* defined(CONFIG_SPICE) */

Questions:

- Is the query code supposed to always return NULL in case of error? In the
  case of hmp_info_spice, there is no check for info==NULL, so on the
  contrary, it seems to indicate that a non-null result is always expected,
  and that function does call qapi_free_SpiceInfo

- If not, is there an existing shortcut to generate the correct deallocation
  code for return types that need it? You can't just use
  qapi_free_%(c_type)s because that would generate an extra * character,
  i.e. I get "SpiceInfo *" and not "SpiceInfo".

- If not, is there any good way to know if the type is a pointer type?
  (A quick look in cripts/qapi/types.py does not show anything obvious)

--
Cheers,
Christophe de Dinechin (IRC c3d)

Re: [PATCH v2 2/3] trace: Add support for recorder back-end

[Christophe de Dinechin]

On 2020-07-27 at 10:23 CEST, Markus Armbruster wrote...
> Christophe de Dinechin  writes:
>
>> On 2020-07-23 at 16:06 CEST, Markus Armbruster wrote...
>>> Christophe de Dinechin  writes:
>>>
>>>> On 2020-06-30 at 15:02 CEST, Daniel P. Berrangé wrote...
>>>>> On Fri, Jun 26, 2020 at 06:27:05PM +0200, Christophe de Dinechin wrote:
>>>>>> The recorder library provides support for low-cost continuous
>>>>>> recording of events, which can then be replayed. This makes it
>>>>>> possible to collect data "after the fact",for example to show the
>>>>>> events that led to a crash.
>>>>>>
>>>>>> Recorder support in qemu is implemented using the existing tracing
>>>>>> interface. In addition, it is possible to individually enable
>>>>>> recorders that are not traces, although this is probably not
>>>>>> recommended.
>>>>>>
>>>>>> HMP COMMAND:
>>>>>> The 'recorder' hmp command has been added, which supports two
>>>>>> sub-commands:
>>>>>> - recorder dump: Dump the current state of the recorder. You can
>>>>>> - recorder trace: Set traces using the recorder_trace_set() syntax.
>>>>>>   You can use "recorder trace help" to list all available recorders.
>>>>>>
>>>>>> Signed-off-by: Christophe de Dinechin 
>>>>>> ---
>>>>>>  configure |  5 +++
>>>>>>  hmp-commands.hx   | 19 --
>>>>>>  monitor/misc.c| 27 ++
>>>>>>  scripts/tracetool/backend/recorder.py | 51 +++
>>>>>>  trace/Makefile.objs   |  2 ++
>>>>>>  trace/control.c   |  7 
>>>>>>  trace/recorder.c  | 22 
>>>>>>  trace/recorder.h  | 34 ++
>>>>>>  util/module.c |  8 +
>>>>>>  9 files changed, 173 insertions(+), 2 deletions(-)
>>>>>>  create mode 100644 scripts/tracetool/backend/recorder.py
>>>>>>  create mode 100644 trace/recorder.c
>>>>>>  create mode 100644 trace/recorder.h
>>>>>
>>>>>> +RECORDER_CONSTRUCTOR
>>>>>> +void recorder_trace_init(void)
>>>>>> +{
>>>>>> +recorder_trace_set(getenv("RECORDER_TRACES"));
>>>>>> +
>>>>>> +// Allow a dump in case we receive some unhandled signal
>>>>>> +// For example, send USR2 to a hung process to get a dump
>>>>>> +if (getenv("RECORDER_TRACES"))
>>>>>> +recorder_dump_on_common_signals(0,0);
>>>>>> +}
>>>>>
>>>>> What is the syntax of this RECORDER_TRACES env variable,
>>>>
>>>> It's basically a colon-separated list of regexps,
>>>> e.g. ".*_error:.*_warning", with special syntax for some additional
>>>> functionality such as real-time graphing.
>>>>
>>>> Here are a few examples:
>>>>
>>>> - Activate the foo, bar and baz traces:
>>>>   foo:bar:baz
>>>>
>>>> - Activate all traces that contain "lock", as well as "recorder" trace:
>>>>   *lock.*:recorder
>>>>
>>>> - Deactivate traces ending in _error
>>>>   .*_error=0
>>>>
>>>> There are also a few tweaks and special names, for example you can adjust
>>>> the output to show the function name and source code location as follows::
>>>>
>>>> - Show source information in the traces
>>>>   recorder_function:recorder_location
>>>>
>>>>   As is not very useful in qemu because it sohws the wrapper location:
>>>>  % RECORDER_TRACES=vm_state_notify qemu-system-x86_64
>>>>  [35225 7.092175] vm_state_notify: running 1 reason 9 (running)
>>>>
>>>>  % RECORDER_TRACES=vm_state_notify:recorder_function:recorder_location 
>>>> qemu-system-x86_64
>>>>  
>>>> /home/ddd/Work/qemu/trace-root.h:346:_nocheck__trace_vm_state_notify:[94277
>>>>  0.294906] vm_state_notify: running 1 reason 9 (running)
>>>>

[PATCH 6/7] build: Add SPICE_CFLAGS and SPICE_LIBS to relevant files

[Christophe de Dinechin]

Instead of adding the spice build flags to the top-level build
options, add them where they are necessary. This is a step to move the
burden of linking with spice libraries away from the top-level qemu.

Signed-off-by: Christophe de Dinechin 
---
 configure| 4 ++--
 hw/display/Makefile.objs | 1 +
 hw/i386/pc.c | 1 -
 monitor/Makefile.objs| 3 +++
 softmmu/Makefile.objs| 2 +-
 ui/Makefile.objs | 4 ++--
 6 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/configure b/configure
index 054aab31be..86fc03699a 100755
--- a/configure
+++ b/configure
@@ -5223,8 +5223,6 @@ EOF
  $pkg_config --atleast-version=0.12.3 spice-protocol && \
  compile_prog "$spice_cflags" "$spice_libs" ; then
 spice="yes"
-libs_softmmu="$libs_softmmu $spice_libs"
-QEMU_CFLAGS="$QEMU_CFLAGS $spice_cflags"
 spice_protocol_version=$($pkg_config --modversion spice-protocol)
 spice_server_version=$($pkg_config --modversion spice-server)
   else
@@ -7535,6 +7533,8 @@ fi
 
 if test "$spice" = "yes" ; then
   echo "CONFIG_SPICE=m" >> $config_host_mak
+  echo "SPICE_CFLAGS=$spice_cflags" >> $config_host_mak
+  echo "SPICE_LIBS=$spice_libs" >> $config_host_mak
 fi
 
 if test "$smartcard" = "yes" ; then
diff --git a/hw/display/Makefile.objs b/hw/display/Makefile.objs
index d619594ad4..3963fd1dcd 100644
--- a/hw/display/Makefile.objs
+++ b/hw/display/Makefile.objs
@@ -47,6 +47,7 @@ obj-$(CONFIG_VGA) += vga.o
 ifeq ($(CONFIG_QXL),y)
 common-obj-m += qxl.mo
 qxl.mo-objs = qxl.o qxl-logger.o qxl-render.o
+qxl.mo-cflags += $(SPICE_CFLAGS)
 endif
 
 common-obj-$(CONFIG_VIRTIO_GPU) += virtio-gpu-base.o virtio-gpu.o 
virtio-gpu-3d.o
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 3d419d5991..9f28a91df9 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -64,7 +64,6 @@
 #include "kvm_i386.h"
 #include "hw/xen/xen.h"
 #include "hw/xen/start_info.h"
-#include "ui/qemu-spice.h"
 #include "exec/memory.h"
 #include "exec/address-spaces.h"
 #include "sysemu/arch_init.h"
diff --git a/monitor/Makefile.objs b/monitor/Makefile.objs
index a8533c9dd7..fd58d80195 100644
--- a/monitor/Makefile.objs
+++ b/monitor/Makefile.objs
@@ -2,5 +2,8 @@ obj-y += misc.o
 common-obj-y += monitor.o qmp.o hmp.o
 common-obj-y += qmp-cmds.o qmp-cmds-control.o
 common-obj-y += hmp-cmds.o
+qmp-cmds.o-cflags += $(SPICE_CFLAGS)
+hmp-cmds.o-cflags += $(SPICE_CFLAGS)
+misc.o-cflags += $(SPICE_CFLAGS)
 
 storage-daemon-obj-y += monitor.o qmp.o qmp-cmds-control.o
diff --git a/softmmu/Makefile.objs b/softmmu/Makefile.objs
index a414a74c50..4e36ff47a2 100644
--- a/softmmu/Makefile.objs
+++ b/softmmu/Makefile.objs
@@ -11,4 +11,4 @@ obj-y += memory_mapping.o
 obj-y += qtest.o
 
 obj-y += vl.o
-vl.o-cflags := $(GPROF_CFLAGS) $(SDL_CFLAGS)
+vl.o-cflags := $(GPROF_CFLAGS) $(SDL_CFLAGS) $(SPICE_CFLAGS)
diff --git a/ui/Makefile.objs b/ui/Makefile.objs
index 1ab515e23d..6a6fda2f06 100644
--- a/ui/Makefile.objs
+++ b/ui/Makefile.objs
@@ -57,8 +57,8 @@ spice-app.mo-objs += spice-core.o spice-input.o 
spice-display.o
 ifeq ($(CONFIG_GIO)$(CONFIG_SPICE),ym)
 spice-app.mo-objs += spice-app.o
 endif
-spice-app.mo-cflags := $(GIO_CFLAGS)
-spice-app.mo-libs := $(GIO_LIBS)
+spice-app.mo-cflags := $(GIO_CFLAGS) $(SPICE_CFLAGS)
+spice-app.mo-libs := $(GIO_LIBS) $(SPICE_LIBS)
 
 common-obj-$(CONFIG_OPENGL) += shader.o
 common-obj-$(CONFIG_OPENGL) += console-gl.o
-- 
2.26.2

[PATCH 2/7] build: fix device module builds

[Christophe de Dinechin]

From: Gerd Hoffmann 

See comment.  Feels quite hackish.  Better ideas anyone?

Signed-off-by: Gerd Hoffmann 
Signed-off-by: Christophe de Dinechin 
---
 dtc   | 2 +-
 roms/SLOF | 2 +-
 roms/openbios | 2 +-
 roms/opensbi  | 2 +-
 roms/seabios  | 2 +-
 slirp | 2 +-
 6 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/dtc b/dtc
index 85e5d83984..88f18909db 16
--- a/dtc
+++ b/dtc
@@ -1 +1 @@
-Subproject commit 85e5d839847af54efab170f2b1331b2a6421e647
+Subproject commit 88f18909db731a627456f26d779445f84e449536
diff --git a/roms/SLOF b/roms/SLOF
index e18ddad851..9546892a80 16
--- a/roms/SLOF
+++ b/roms/SLOF
@@ -1 +1 @@
-Subproject commit e18ddad8516ff2cfe36ec130200318f7251aa78c
+Subproject commit 9546892a80d5a4c73deea6719de46372f007f4a6
diff --git a/roms/openbios b/roms/openbios
index 75fbb41d28..7e5b89e429 16
--- a/roms/openbios
+++ b/roms/openbios
@@ -1 +1 @@
-Subproject commit 75fbb41d2857d93208c74a8e0228b29fd7bf04c0
+Subproject commit 7e5b89e4295063d8eba55b9c8ce8bc681c2d129a
diff --git a/roms/opensbi b/roms/opensbi
index 9f1b72ce66..be92da280d 16
--- a/roms/opensbi
+++ b/roms/opensbi
@@ -1 +1 @@
-Subproject commit 9f1b72ce66d659e91013b358939e832fb27223f5
+Subproject commit be92da280d87c38a2e0adc5d3f43bab7b5468f09
diff --git a/roms/seabios b/roms/seabios
index 88ab0c1552..f21b5a4aeb 16
--- a/roms/seabios
+++ b/roms/seabios
@@ -1 +1 @@
-Subproject commit 88ab0c15525ced2eefe39220742efe4769089ad8
+Subproject commit f21b5a4aeb020f2a5e2c6503f906a9349dd2f069
diff --git a/slirp b/slirp
index 2faae0f778..126c04acba 16
--- a/slirp
+++ b/slirp
@@ -1 +1 @@
-Subproject commit 2faae0f778f818fadc873308f983289df697eb93
+Subproject commit 126c04acbabd7ad32c2b018fe10dfac2a3bc1210
-- 
2.26.2

[PATCH 7/7] spice: Call qemu spice functions indirectly

[Christophe de Dinechin]

In order to be able to move all of spice in a shared library that is
loaded dynamically only on demand, this commit puts the functions
called by the main binary in a QemuSpiceOps structure, and use static
inline stubs to preserve the call sites.

With these changes, the following shared libraries are no longer
necessary in the top-level qemu binary when building with CONFIG_MDULES:

libspice-server.so.1 => /lib64/libspice-server.so.1 (HEX)
libopus.so.0 => /lib64/libopus.so.0 (HEX)
liblz4.so.1 => /lib64/liblz4.so.1 (HEX)
libgstapp-1.0.so.0 => /lib64/libgstapp-1.0.so.0 (HEX)
libgstvideo-1.0.so.0 => /lib64/libgstvideo-1.0.so.0 (HEX)
libgstbase-1.0.so.0 => /lib64/libgstbase-1.0.so.0 (HEX)
libgstreamer-1.0.so.0 => /lib64/libgstreamer-1.0.so.0 (HEX)
libssl.so.1.1 => /lib64/libssl.so.1.1 (HEX)
liborc-0.4.so.0 => /lib64/liborc-0.4.so.0 (HEX)

Signed-off-by: Christophe de Dinechin 
---
 include/ui/qemu-spice.h | 74 +++--
 monitor/hmp-cmds.c  | 13 
 monitor/misc.c  |  2 +-
 monitor/qmp-cmds.c  |  6 ++--
 softmmu/vl.c| 28 ++--
 ui/spice-core.c | 33 --
 6 files changed, 107 insertions(+), 49 deletions(-)

diff --git a/include/ui/qemu-spice.h b/include/ui/qemu-spice.h
index d34cea2e0f..9721d8817e 100644
--- a/include/ui/qemu-spice.h
+++ b/include/ui/qemu-spice.h
@@ -25,21 +25,28 @@
 #include 
 #include "qemu/config-file.h"
 
-extern int using_spice;
+#define using_spice (qemu_spice.in_use())
 
-void qemu_spice_init(void);
 void qemu_spice_input_init(void);
 void qemu_spice_audio_init(void);
-void qemu_spice_display_init(void);
-int qemu_spice_display_add_client(int csock, int skipauth, int tls);
 int qemu_spice_add_interface(SpiceBaseInstance *sin);
 bool qemu_spice_have_display_interface(QemuConsole *con);
 int qemu_spice_add_display_interface(QXLInstance *qxlin, QemuConsole *con);
+
+#if !defined(CONFIG_MODULES) || defined(BUILD_DSO)
+bool qemu_is_using_spice(void);
+void qemu_start_using_spice(void);
+void qemu_spice_init(void);
+void qemu_spice_display_init(void);
+int qemu_spice_display_add_client(int csock, int skipauth, int tls);
 int qemu_spice_set_passwd(const char *passwd,
   bool fail_if_connected, bool 
disconnect_if_connected);
 int qemu_spice_set_pw_expire(time_t expires);
 int qemu_spice_migrate_info(const char *hostname, int port, int tls_port,
 const char *subject);
+struct SpiceInfo *qemu_spice_query(Error **errp);
+#endif /* !CONFIG_MODULES || BUILD_DSO */
+
 
 #if !defined(SPICE_SERVER_VERSION) || (SPICE_SERVER_VERSION < 0xc06)
 #define SPICE_NEEDS_SET_MM_TIME 1
@@ -47,46 +54,33 @@ int qemu_spice_migrate_info(const char *hostname, int port, 
int tls_port,
 #define SPICE_NEEDS_SET_MM_TIME 0
 #endif
 
-#else  /* CONFIG_SPICE */
-
-#include "qemu/error-report.h"
-
-#define using_spice 0
-#define spice_displays 0
-static inline int qemu_spice_set_passwd(const char *passwd,
-bool fail_if_connected,
-bool disconnect_if_connected)
-{
-return -1;
-}
-static inline int qemu_spice_set_pw_expire(time_t expires)
-{
-return -1;
-}
-static inline int qemu_spice_migrate_info(const char *h, int p, int t,
-  const char *s)
-{
-return -1;
-}
+#else /* !CONFIG_SPICE */
 
-static inline int qemu_spice_display_add_client(int csock, int skipauth,
-int tls)
-{
-return -1;
-}
+#define using_spice 0
 
-static inline void qemu_spice_display_init(void)
-{
-/* This must never be called if CONFIG_SPICE is disabled */
-error_report("spice support is disabled");
-abort();
-}
+#endif /* CONFIG_SPICE */
 
-static inline void qemu_spice_init(void)
+/* Module high-level inteface for SPICE */
+struct QemuSpiceOps
 {
-}
-
-#endif /* CONFIG_SPICE */
+bool (*in_use)(void);
+void (*init)(void);
+void (*display_init)(void);
+int (*display_add_client)(int csock, int skipauth, int tls);
+
+int (*set_passwd)(const char *passwd,
+  bool fail_if_connected,
+  bool disconnect_if_connected);
+int (*set_pw_expire)(time_t expires);
+int (*migrate_info)(const char *hostname,
+int port, int tls_port,
+const char *subject);
+struct SpiceInfo * (*query)(Error **errp);
+};
+typedef struct QemuSpiceOps QemuSpiceOps;
+void qemu_spice_ops_register(QemuSpiceOps *ops);
+
+extern struct QemuSpiceOps qemu_spice;
 
 static inline bool qemu_using_spice(Error **errp)
 {
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index ae4b6a4246..8e10af188f 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -57,6 +57,7 @@
 
 #ifdef CONFIG_SPICE
 #include 

[PATCH 1/7] spice: simplify chardev setup

[Christophe de Dinechin]

From: Gerd Hoffmann 

Initialize spice before chardevs.  That allows to register the spice
chardevs directly in the init function and removes the need to maintain
a linked list of chardevs just for registration.

Signed-off-by: Gerd Hoffmann 
Signed-off-by: Christophe de Dinechin 
---
 chardev/spice.c | 29 ++---
 include/chardev/spice.h |  1 -
 include/ui/qemu-spice.h |  1 -
 softmmu/vl.c|  9 +
 ui/spice-app.c  | 17 +
 ui/spice-core.c |  2 --
 6 files changed, 20 insertions(+), 39 deletions(-)

diff --git a/chardev/spice.c b/chardev/spice.c
index bf7ea1e294..9733f06716 100644
--- a/chardev/spice.c
+++ b/chardev/spice.c
@@ -14,9 +14,6 @@ typedef struct SpiceCharSource {
 SpiceChardev   *scd;
 } SpiceCharSource;
 
-static QLIST_HEAD(, SpiceChardev) spice_chars =
-QLIST_HEAD_INITIALIZER(spice_chars);
-
 static int vmc_write(SpiceCharDeviceInstance *sin, const uint8_t *buf, int len)
 {
 SpiceChardev *scd = container_of(sin, SpiceChardev, sin);
@@ -216,8 +213,6 @@ static void char_spice_finalize(Object *obj)
 
 vmc_unregister_interface(s);
 
-QLIST_SAFE_REMOVE(s, next);
-
 g_free((char *)s->sin.subtype);
 g_free((char *)s->sin.portname);
 }
@@ -256,8 +251,6 @@ static void chr_open(Chardev *chr, const char *subtype)
 
 s->active = false;
 s->sin.subtype = g_strdup(subtype);
-
-QLIST_INSERT_HEAD(_chars, s, next);
 }
 
 static void qemu_chr_open_spice_vmc(Chardev *chr,
@@ -310,28 +303,18 @@ void qemu_chr_open_spice_port(Chardev *chr,
 return;
 }
 
+if (!using_spice) {
+error_setg(errp, "spice not enabled");
+return;
+}
+
 chr_open(chr, "port");
 
 *be_opened = false;
 s = SPICE_CHARDEV(chr);
 s->sin.portname = g_strdup(name);
 
-if (using_spice) {
-/* spice server already created */
-vmc_register_interface(s);
-}
-}
-
-void qemu_spice_register_ports(void)
-{
-SpiceChardev *s;
-
-QLIST_FOREACH(s, _chars, next) {
-if (s->sin.portname == NULL) {
-continue;
-}
-vmc_register_interface(s);
-}
+vmc_register_interface(s);
 }
 
 static void qemu_chr_parse_spice_vmc(QemuOpts *opts, ChardevBackend *backend,
diff --git a/include/chardev/spice.h b/include/chardev/spice.h
index 1f7339b649..2013255f34 100644
--- a/include/chardev/spice.h
+++ b/include/chardev/spice.h
@@ -12,7 +12,6 @@ typedef struct SpiceChardev {
 bool  blocked;
 const uint8_t *datapos;
 int   datalen;
-QLIST_ENTRY(SpiceChardev) next;
 } SpiceChardev;
 
 #define TYPE_CHARDEV_SPICE "chardev-spice"
diff --git a/include/ui/qemu-spice.h b/include/ui/qemu-spice.h
index 8c23dfe717..d34cea2e0f 100644
--- a/include/ui/qemu-spice.h
+++ b/include/ui/qemu-spice.h
@@ -46,7 +46,6 @@ int qemu_spice_migrate_info(const char *hostname, int port, 
int tls_port,
 #else
 #define SPICE_NEEDS_SET_MM_TIME 0
 #endif
-void qemu_spice_register_ports(void);
 
 #else  /* CONFIG_SPICE */
 
diff --git a/softmmu/vl.c b/softmmu/vl.c
index f476ef89ed..bc0dcc4f58 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -4125,6 +4125,11 @@ void qemu_init(int argc, char **argv, char **envp)
 page_size_init();
 socket_init();
 
+/* spice needs the timers to be initialized by this point */
+/* spice must initialize before audio as it changes the default auiodev */
+/* spice must initialize before chardevs (for spicevmc and spiceport) */
+qemu_spice_init();
+
 qemu_opts_foreach(qemu_find_opts("object"),
   user_creatable_add_opts_foreach,
   object_create_initial, _fatal);
@@ -4139,10 +4144,6 @@ void qemu_init(int argc, char **argv, char **envp)
   fsdev_init_func, NULL, _fatal);
 #endif
 
-/* spice needs the timers to be initialized by this point */
-/* spice must initialize before audio as it changes the default auiodev */
-qemu_spice_init();
-
 /*
  * Note: we need to create audio and block backends before
  * machine_set_property(), so machine properties can refer to
diff --git a/ui/spice-app.c b/ui/spice-app.c
index 40fb2ef573..03d971b15f 100644
--- a/ui/spice-app.c
+++ b/ui/spice-app.c
@@ -117,7 +117,6 @@ static void spice_app_atexit(void)
 static void spice_app_display_early_init(DisplayOptions *opts)
 {
 QemuOpts *qopts;
-ChardevBackend *be = chr_spice_backend_new();
 GError *err = NULL;
 
 if (opts->has_full_screen) {
@@ -162,6 +161,15 @@ static void spice_app_display_early_init(DisplayOptions 
*opts)
 qemu_opt_set(qopts, "gl", opts->has_gl ? "on" : "off", _abort);
 display_opengl = opts->has_gl;
 #endif
+}
+
+static void spice_app_display_init(DisplayState *ds, DisplayOptions *opts)
+{
+ChardevBackend *be = chr_spice_backend_new();
+QemuOp

[PATCH 5/7] spice: Move all the spice-related code in spice-app.so

[Christophe de Dinechin]

If we want to build spice as a separately loadable module, we need to
put all the spice code in one loadable module, because the build
system does not know how to deal with dependencies yet.

Signed-off-by: Christophe de Dinechin 
---
 audio/Makefile.objs   | 2 +-
 chardev/Makefile.objs | 3 +--
 ui/Makefile.objs  | 8 
 3 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/audio/Makefile.objs b/audio/Makefile.objs
index b4a4c11f31..298c895ff5 100644
--- a/audio/Makefile.objs
+++ b/audio/Makefile.objs
@@ -1,5 +1,5 @@
 common-obj-y = audio.o audio_legacy.o noaudio.o wavaudio.o mixeng.o
-common-obj-$(CONFIG_SPICE) += spiceaudio.o
+spice-app.mo-objs += ../audio/spiceaudio.o
 common-obj-$(CONFIG_AUDIO_COREAUDIO) += coreaudio.o
 common-obj-$(CONFIG_AUDIO_DSOUND) += dsoundaudio.o
 common-obj-$(CONFIG_AUDIO_WIN_INT) += audio_win_int.o
diff --git a/chardev/Makefile.objs b/chardev/Makefile.objs
index 7cf05c9541..2add4319a9 100644
--- a/chardev/Makefile.objs
+++ b/chardev/Makefile.objs
@@ -26,5 +26,4 @@ baum.o-cflags := $(SDL_CFLAGS)
 baum.o-libs := $(BRLAPI_LIBS)
 endif
 
-common-obj-$(CONFIG_SPICE) += spice.mo
-spice.mo-objs := spice.o
+spice-app.mo-objs += ../chardev/spice.o
diff --git a/ui/Makefile.objs b/ui/Makefile.objs
index 504b196479..1ab515e23d 100644
--- a/ui/Makefile.objs
+++ b/ui/Makefile.objs
@@ -11,7 +11,6 @@ common-obj-y += keymaps.o console.o cursor.o qemu-pixman.o
 common-obj-y += input.o input-keymap.o input-legacy.o kbd-state.o
 common-obj-y += input-barrier.o
 common-obj-$(CONFIG_LINUX) += input-linux.o
-common-obj-$(CONFIG_SPICE) += spice-core.o spice-input.o spice-display.o
 common-obj-$(CONFIG_COCOA) += cocoa.o
 common-obj-$(CONFIG_VNC) += $(vnc-obj-y)
 common-obj-$(call lnot,$(CONFIG_VNC)) += vnc-stubs.o
@@ -53,10 +52,11 @@ curses.mo-objs := curses.o
 curses.mo-cflags := $(CURSES_CFLAGS) $(ICONV_CFLAGS)
 curses.mo-libs := $(CURSES_LIBS) $(ICONV_LIBS)
 
-ifeq ($(CONFIG_GIO)$(CONFIG_SPICE),yy)
-common-obj-$(if $(CONFIG_MODULES),m,y) += spice-app.mo
+common-obj-$(CONFIG_SPICE) += spice-app.mo
+spice-app.mo-objs += spice-core.o spice-input.o spice-display.o
+ifeq ($(CONFIG_GIO)$(CONFIG_SPICE),ym)
+spice-app.mo-objs += spice-app.o
 endif
-spice-app.mo-objs := spice-app.o
 spice-app.mo-cflags := $(GIO_CFLAGS)
 spice-app.mo-libs := $(GIO_LIBS)
 
-- 
2.26.2

[PATCH 4/7] spice: Make spice a module configuration

[Christophe de Dinechin]

This commit changes the spice configuration 'm' by default, and moves
the spice components to obj-m variables. It is sufficient to build
without modules enable, but does not link correctly yet, since no
shims have been created for the missing functions yet.

Signed-off-by: Christophe de Dinechin 
---
 chardev/Makefile.objs | 3 ++-
 configure | 2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/chardev/Makefile.objs b/chardev/Makefile.objs
index 3783dadc4c..7cf05c9541 100644
--- a/chardev/Makefile.objs
+++ b/chardev/Makefile.objs
@@ -26,4 +26,5 @@ baum.o-cflags := $(SDL_CFLAGS)
 baum.o-libs := $(BRLAPI_LIBS)
 endif
 
-common-obj-$(CONFIG_SPICE) += spice.o
+common-obj-$(CONFIG_SPICE) += spice.mo
+spice.mo-objs := spice.o
diff --git a/configure b/configure
index 4bd80ed507..054aab31be 100755
--- a/configure
+++ b/configure
@@ -7534,7 +7534,7 @@ if test "$posix_memalign" = "yes" ; then
 fi
 
 if test "$spice" = "yes" ; then
-  echo "CONFIG_SPICE=y" >> $config_host_mak
+  echo "CONFIG_SPICE=m" >> $config_host_mak
 fi
 
 if test "$smartcard" = "yes" ; then
-- 
2.26.2

[PATCH 3/7] minikconf: Pass variables for modules

[Christophe de Dinechin]

Signed-off-by: Christophe de Dinechin 
---
 scripts/minikconf.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/scripts/minikconf.py b/scripts/minikconf.py
index bcd91015d3..d60add97f6 100755
--- a/scripts/minikconf.py
+++ b/scripts/minikconf.py
@@ -690,10 +690,10 @@ if __name__ == '__main__':
 parser = KconfigParser(data)
 external_vars = set()
 for arg in argv[3:]:
-m = re.match(r'^(CONFIG_[A-Z0-9_]+)=([yn]?)$', arg)
+m = re.match(r'^(CONFIG_[A-Z0-9_]+)=([ymn]?)$', arg)
 if m is not None:
 name, value = m.groups()
-parser.do_assignment(name, value == 'y')
+parser.do_assignment(name, value == 'y' or value == 'm')
 external_vars.add(name[7:])
 else:
 fp = open(arg, 'rt', encoding='utf-8')
-- 
2.26.2

[PATCH 0/7] Make SPICE a load module

[Christophe de Dinechin]

This series builds the qemu SPICE code into a load module in order to
remove the number of shared libraries that the main qemu binary
depends on.

It is intended to be built on the work that Gerd shared recently
regarding modular devices and chardev initialization. I left the
patch I used in the series since Gerd may still be working on it.

With these changes, the following shared libraries are no longer
needed in the main binary:

libspice-server.so.1
libopus.so.0
liblz4.so.1
libgstapp-1.0.so.0
libgstvideo-1.0.so.0
libgstbase-1.0.so.0
libgstreamer-1.0.so.0
libssl.so.1.1
liborc-0.4.so.0

There are still some not-so-nice changes in the makefiles,
e.g. references to ../directory/foo.o. I don't want to invest too much
time in fixing it the right way if Meson changes the way these things
are built.

Christophe de Dinechin (5):
  minikconf: Pass variables for modules
  spice: Make spice a module configuration
  spice: Move all the spice-related code in spice-app.so
  build: Add SPICE_CFLAGS and SPICE_LIBS to relevant files
  spice: Call qemu spice functions indirectly

Gerd Hoffmann (2):
  spice: simplify chardev setup
  build: fix device module builds

 audio/Makefile.objs  |  2 +-
 chardev/Makefile.objs|  2 +-
 chardev/spice.c  | 29 
 configure|  6 ++--
 dtc  |  2 +-
 hw/display/Makefile.objs |  1 +
 hw/i386/pc.c |  1 -
 include/chardev/spice.h  |  1 -
 include/ui/qemu-spice.h  | 75 ++--
 monitor/Makefile.objs|  3 ++
 monitor/hmp-cmds.c   | 13 +++
 monitor/misc.c   |  2 +-
 monitor/qmp-cmds.c   |  6 ++--
 roms/SLOF|  2 +-
 roms/openbios|  2 +-
 roms/opensbi |  2 +-
 roms/seabios |  2 +-
 scripts/minikconf.py |  4 +--
 slirp|  2 +-
 softmmu/Makefile.objs|  2 +-
 softmmu/vl.c | 35 ---
 ui/Makefile.objs | 12 +++
 ui/spice-app.c   | 17 -
 ui/spice-core.c  | 35 ---
 24 files changed, 150 insertions(+), 108 deletions(-)

-- 
2.26.2

Re: [PATCH v2 2/3] trace: Add support for recorder back-end

[Christophe de Dinechin]

On 2020-07-23 at 16:06 CEST, Markus Armbruster wrote...
> Christophe de Dinechin  writes:
>
>> On 2020-06-30 at 15:02 CEST, Daniel P. Berrangé wrote...
>>> On Fri, Jun 26, 2020 at 06:27:05PM +0200, Christophe de Dinechin wrote:
>>>> The recorder library provides support for low-cost continuous
>>>> recording of events, which can then be replayed. This makes it
>>>> possible to collect data "after the fact",for example to show the
>>>> events that led to a crash.
>>>>
>>>> Recorder support in qemu is implemented using the existing tracing
>>>> interface. In addition, it is possible to individually enable
>>>> recorders that are not traces, although this is probably not
>>>> recommended.
>>>>
>>>> HMP COMMAND:
>>>> The 'recorder' hmp command has been added, which supports two
>>>> sub-commands:
>>>> - recorder dump: Dump the current state of the recorder. You can
>>>> - recorder trace: Set traces using the recorder_trace_set() syntax.
>>>>   You can use "recorder trace help" to list all available recorders.
>>>>
>>>> Signed-off-by: Christophe de Dinechin 
>>>> ---
>>>>  configure |  5 +++
>>>>  hmp-commands.hx   | 19 --
>>>>  monitor/misc.c| 27 ++
>>>>  scripts/tracetool/backend/recorder.py | 51 +++
>>>>  trace/Makefile.objs   |  2 ++
>>>>  trace/control.c   |  7 
>>>>  trace/recorder.c  | 22 
>>>>  trace/recorder.h  | 34 ++
>>>>  util/module.c |  8 +
>>>>  9 files changed, 173 insertions(+), 2 deletions(-)
>>>>  create mode 100644 scripts/tracetool/backend/recorder.py
>>>>  create mode 100644 trace/recorder.c
>>>>  create mode 100644 trace/recorder.h
>>>
>>>> +RECORDER_CONSTRUCTOR
>>>> +void recorder_trace_init(void)
>>>> +{
>>>> +recorder_trace_set(getenv("RECORDER_TRACES"));
>>>> +
>>>> +// Allow a dump in case we receive some unhandled signal
>>>> +// For example, send USR2 to a hung process to get a dump
>>>> +if (getenv("RECORDER_TRACES"))
>>>> +recorder_dump_on_common_signals(0,0);
>>>> +}
>>>
>>> What is the syntax of this RECORDER_TRACES env variable,
>>
>> It's basically a colon-separated list of regexps,
>> e.g. ".*_error:.*_warning", with special syntax for some additional
>> functionality such as real-time graphing.
>>
>> Here are a few examples:
>>
>> - Activate the foo, bar and baz traces:
>>   foo:bar:baz
>>
>> - Activate all traces that contain "lock", as well as "recorder" trace:
>>   *lock.*:recorder
>>
>> - Deactivate traces ending in _error
>>   .*_error=0
>>
>> There are also a few tweaks and special names, for example you can adjust
>> the output to show the function name and source code location as follows::
>>
>> - Show source information in the traces
>>   recorder_function:recorder_location
>>
>>   As is not very useful in qemu because it sohws the wrapper location:
>>  % RECORDER_TRACES=vm_state_notify qemu-system-x86_64
>>  [35225 7.092175] vm_state_notify: running 1 reason 9 (running)
>>
>>  % RECORDER_TRACES=vm_state_notify:recorder_function:recorder_location 
>> qemu-system-x86_64
>>  
>> /home/ddd/Work/qemu/trace-root.h:346:_nocheck__trace_vm_state_notify:[94277 
>> 0.294906] vm_state_notify: running 1 reason 9 (running)
>>
>>   This is not as good as what you get with "real" record entries:
>>  % RECORDER_TRACES=recorder_function:recorder_location:recorder 
>> qemu-system-x86_64
>>  recorder.c:2036:recorder_allocate_alt_stack:[29561 0.006434] recorder: 
>> Set alt stack to 0x7fc567b87000 size 8192
>>
>> - Get some help on available traces:
>>   help
>>
>> - Enable real-time graphing for trace "perfdata"
>>   perfdata=bytes,loops
>>
>> The last one assumes that you would have a record that looks like:
>>
>>  record(perfdata, "Transferred %lu bytes in %lu iterations",
>> bytes, itercount);
>>
>> You could 

[PATCH v4 1/2] trace: Add support for recorder back-end

[Christophe de Dinechin]

The recorder library provides support for low-cost continuous
recording of events, which can then be replayed. This makes it
possible to collect data after the fact, for example to show the
events that led to a crash or unexpected condition.

In this series, minimal recorder support in qemu is implemented using
the existing tracing interface. For each trace, a corresponding
recorder is created with a generic name and a default size of 8 entries.

In addition, it is possible to explicitly enable recorders that are
not qemu traces, for example in order to use actually record events
rather than trace them, or to use the real-time graphing capabilities.
For that reason, a limited set of recorder-related macros are defined
as no-ops even if the recorder trace backend is not configured.

Recorder-specific features, notably the ability to perform a
post-mortem dump and to group traces by topic, are not integrated in
this series, as doing so would require modifying the trace
infrastructure, which is a non-objective here. This may be the topic
of later series if there is any interest for it.

HMP COMMAND:
The 'recorder' hmp command has been added, which supports two
sub-commands:
* recorder dump: Dump the current state of the recorder. You can
  give that command a recorder name, to only dump that recorder.
* recorder trace: Set traces using the recorder_trace_set() syntax.
  You can use "recorder trace help" to list all available recorders.

Signed-off-by: Christophe de Dinechin 
---
 MAINTAINERS   |  1 +
 configure | 14 
 hmp-commands.hx   | 23 +++-
 monitor/misc.c| 25 +
 scripts/tracetool/backend/recorder.py | 52 +++
 trace/Makefile.objs   |  1 +
 trace/control.c   |  5 +++
 trace/recorder.c  | 27 ++
 trace/recorder.h  | 32 +
 util/module.c |  6 
 10 files changed, 185 insertions(+), 1 deletion(-)
 create mode 100644 scripts/tracetool/backend/recorder.py
 create mode 100644 trace/recorder.c
 create mode 100644 trace/recorder.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 3395abd4e1..764b5915d7 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2484,6 +2484,7 @@ F: stubs/
 Tracing
 M: Stefan Hajnoczi 
 S: Maintained
+R: Christophe de Dinechin 
 F: trace/
 F: trace-events
 F: docs/qemu-option-trace.rst.inc
diff --git a/configure b/configure
index 4bd80ed507..3770ff873d 100755
--- a/configure
+++ b/configure
@@ -7761,6 +7761,20 @@ fi
 if have_backend "log"; then
   echo "CONFIG_TRACE_LOG=y" >> $config_host_mak
 fi
+if have_backend "recorder"; then
+recorder_minver="1.0.10"
+if $pkg_config --atleast-version=$recorder_minver recorder ; then
+recorder_cflags="$($pkg_config --cflags recorder)"
+recorder_libs="$($pkg_config --libs recorder)"
+LIBS="$recorder_libs $LIBS"
+libs_qga="$recorder_libs $libs_qga"
+QEMU_CFLAGS="$QEMU_CFLAGS $recorder_cflags"
+zstd="yes"
+echo "CONFIG_TRACE_RECORDER=y" >> $config_host_mak
+else
+feature_not_found "recorder" "Install recorder-devel package"
+fi
+fi
 if have_backend "ust"; then
   echo "CONFIG_TRACE_UST=y" >> $config_host_mak
 fi
diff --git a/hmp-commands.hx b/hmp-commands.hx
index d548a3ab74..f164ff6d65 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -302,6 +302,28 @@ SRST
   Open, close, or flush the trace file.  If no argument is given, the
   status of the trace file is displayed.
 ERST
+#endif
+
+#if defined(CONFIG_TRACE_RECORDER)
+{
+.name   = "recorder",
+.args_type  = "op:s?,arg:s?",
+.params = "trace|dump [arg]",
+.help   = "trace selected recorders or print recorder dump",
+.cmd= hmp_recorder,
+},
+
+SRST
+``trace`` *cmds*
+  Activate or deactivate tracing for individual recorder traces.
+  See recorder_trace_set(3) for the syntax of *cmds*
+  For example, to activate trace ``foo`` and disable alll traces
+  ending in ``_warning``, use ``foo:.*_warning=0``.
+  Using ``help`` will list available traces and their current setting.
+``dump`` [*name*]
+  Dump the recorder. If *name* is given, only the specific named recorder
+  will be dumped.
+ERST
 #endif
 
 {
@@ -1828,4 +1850,3 @@ ERST
 .sub_table  = hmp_info_cmds,
 .flags  = "p",
 },
-
diff --git a/monitor/misc.c b/monitor/misc.c
index e847b58a8c..aff72158d7 100644
--- a/monitor/misc.c
+++ b/monitor/misc.c
@@ -60,6 +60,7 @@
 #ifdef CONFIG_TRACE_SIMPLE
 #include "trace/simple.h"
 #endif
+#include "trace/recorder.h"
 #include

[PATCH v4 2/2] trace: Example of non-tracing recorder use

[Christophe de Dinechin]

This patch is a simple example showing how the recorder can be used to
have one "topic" covering multiple entries. Here, the topic is "lock",
the idea being to have the latest lock changes for instance in case of
a crash or hang.

Here are a few use cases:

* Tracing  lock updates:
RECORDER_TRACES=lock qemu
* Showing lock changes prior to a hang
RECORDER_TRACES=lock qemu &
# Wait until hang
killall -USR2 qemu  # This will trigger a dump
* Graphic visualization of lock states:
RECORDER_TRACES="lock=state,id" qemu &
recorder_scope state
# Hit the 't' key to toggle timing display
# Hit the 'c' key to dump the screen data as CSV
cat recorder_scope_data-1.csv

Signed-off-by: Christophe de Dinechin 
---
 util/qemu-thread-common.h | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/util/qemu-thread-common.h b/util/qemu-thread-common.h
index 2af6b12085..0de07a471f 100644
--- a/util/qemu-thread-common.h
+++ b/util/qemu-thread-common.h
@@ -15,6 +15,9 @@
 
 #include "qemu/thread.h"
 #include "trace.h"
+#include "trace/recorder.h"
+
+RECORDER_DEFINE(lock, 16, "Lock state");
 
 static inline void qemu_mutex_post_init(QemuMutex *mutex)
 {
@@ -23,12 +26,14 @@ static inline void qemu_mutex_post_init(QemuMutex *mutex)
 mutex->line = 0;
 #endif
 mutex->initialized = true;
+record(lock, "Init state %d for %p", -1, mutex);
 }
 
 static inline void qemu_mutex_pre_lock(QemuMutex *mutex,
const char *file, int line)
 {
 trace_qemu_mutex_lock(mutex, file, line);
+record(lock, "Locking state %d for %p", 1, mutex);
 }
 
 static inline void qemu_mutex_post_lock(QemuMutex *mutex,
@@ -39,6 +44,7 @@ static inline void qemu_mutex_post_lock(QemuMutex *mutex,
 mutex->line = line;
 #endif
 trace_qemu_mutex_locked(mutex, file, line);
+record(lock, "Locked state %d for %p", 2, mutex);
 }
 
 static inline void qemu_mutex_pre_unlock(QemuMutex *mutex,
@@ -49,6 +55,7 @@ static inline void qemu_mutex_pre_unlock(QemuMutex *mutex,
 mutex->line = 0;
 #endif
 trace_qemu_mutex_unlock(mutex, file, line);
+record(lock, "Unkocked state %d for %p", 0, mutex);
 }
 
 #endif
-- 
2.26.2

[PATCH v4 0/2] trace: Add a trace backend for the recorder library

[Christophe de Dinechin]

The recorder library implements low-cost always-on tracing, with three
usage models:

1. Flight recorder: Dump information on recent events in case of crash
2. Tracing: Individual traces can be enabled using environment variables
3. Real-time graphing / control, using the recorder_scope application

This short series introduces a new "recorder" back-end which connects
to the recorder. Traces using the recorder are intentionally "always on",
because the recorder library is primarily designed to record
information for later playback in case of crash, tracing being only a
secondary capability.

An example is given of how the recorder can also be used separately
from generated traces. The example uses locking, which can make sense
for both post-mortem and real-time graphing.

Changes in v3:
* Address coding style issues (C++ comments, wrong include, etc)
* Fix args type for HMP command (for now, still a single command)
* Add basic help for HMP command
* Use pkg-config for recorder information. This requires recorder
  1.0.10 or later.

Changes in v4:
* Rebased on current master
* Fix GPL v2-only license
* Remove confusing #ifdef around #include "trace/recorder.h"
* Added myself as a reviewer for trace subsystem

Later patches wil address larger topics that were discussed that
would impact other tracing mechanisms, as well as GitHub / GitLab
build tests.

Christophe de Dinechin (2):
  trace: Add support for recorder back-end
  trace: Example of non-tracing recorder use

 MAINTAINERS   |  1 +
 configure | 14 
 hmp-commands.hx   | 23 +++-
 monitor/misc.c| 25 +
 scripts/tracetool/backend/recorder.py | 52 +++
 trace/Makefile.objs   |  1 +
 trace/control.c   |  5 +++
 trace/recorder.c  | 27 ++
 trace/recorder.h  | 32 +
 util/module.c |  6 
 util/qemu-thread-common.h |  7 
 11 files changed, 192 insertions(+), 1 deletion(-)
 create mode 100644 scripts/tracetool/backend/recorder.py
 create mode 100644 trace/recorder.c
 create mode 100644 trace/recorder.h

-- 
2.26.2

Re: [PATCH v2 2/3] trace: Add support for recorder back-end

[Christophe de Dinechin]

On 2020-06-30 at 15:02 CEST, Daniel P. Berrangé wrote...
> On Fri, Jun 26, 2020 at 06:27:05PM +0200, Christophe de Dinechin wrote:
>> The recorder library provides support for low-cost continuous
>> recording of events, which can then be replayed. This makes it
>> possible to collect data "after the fact",for example to show the
>> events that led to a crash.
>>
>> Recorder support in qemu is implemented using the existing tracing
>> interface. In addition, it is possible to individually enable
>> recorders that are not traces, although this is probably not
>> recommended.
>>
>> HMP COMMAND:
>> The 'recorder' hmp command has been added, which supports two
>> sub-commands:
>> - recorder dump: Dump the current state of the recorder. You can
>> - recorder trace: Set traces using the recorder_trace_set() syntax.
>>   You can use "recorder trace help" to list all available recorders.
>>
>> Signed-off-by: Christophe de Dinechin 
>> ---
>>  configure |  5 +++
>>  hmp-commands.hx   | 19 --
>>  monitor/misc.c| 27 ++
>>  scripts/tracetool/backend/recorder.py | 51 +++
>>  trace/Makefile.objs   |  2 ++
>>  trace/control.c   |  7 
>>  trace/recorder.c  | 22 
>>  trace/recorder.h  | 34 ++
>>  util/module.c |  8 +
>>  9 files changed, 173 insertions(+), 2 deletions(-)
>>  create mode 100644 scripts/tracetool/backend/recorder.py
>>  create mode 100644 trace/recorder.c
>>  create mode 100644 trace/recorder.h
>
>> +RECORDER_CONSTRUCTOR
>> +void recorder_trace_init(void)
>> +{
>> +recorder_trace_set(getenv("RECORDER_TRACES"));
>> +
>> +// Allow a dump in case we receive some unhandled signal
>> +// For example, send USR2 to a hung process to get a dump
>> +if (getenv("RECORDER_TRACES"))
>> +recorder_dump_on_common_signals(0,0);
>> +}
>
> What is the syntax of this RECORDER_TRACES env variable,

It's basically a colon-separated list of regexps,
e.g. ".*_error:.*_warning", with special syntax for some additional
functionality such as real-time graphing.

Here are a few examples:

- Activate the foo, bar and baz traces:
  foo:bar:baz

- Activate all traces that contain "lock", as well as "recorder" trace:
  *lock.*:recorder

- Deactivate traces ending in _error
  .*_error=0

There are also a few tweaks and special names, for example you can adjust
the output to show the function name and source code location as follows::

- Show source information in the traces
  recorder_function:recorder_location

  As is not very useful in qemu because it sohws the wrapper location:
 % RECORDER_TRACES=vm_state_notify qemu-system-x86_64
 [35225 7.092175] vm_state_notify: running 1 reason 9 (running)

 % RECORDER_TRACES=vm_state_notify:recorder_function:recorder_location 
qemu-system-x86_64
 
/home/ddd/Work/qemu/trace-root.h:346:_nocheck__trace_vm_state_notify:[94277 
0.294906] vm_state_notify: running 1 reason 9 (running)

  This is not as good as what you get with "real" record entries:
 % RECORDER_TRACES=recorder_function:recorder_location:recorder 
qemu-system-x86_64
 recorder.c:2036:recorder_allocate_alt_stack:[29561 0.006434] recorder: Set 
alt stack to 0x7fc567b87000 size 8192

- Get some help on available traces:
  help

- Enable real-time graphing for trace "perfdata"
  perfdata=bytes,loops

The last one assumes that you would have a record that looks like:

 record(perfdata, "Transferred %lu bytes in %lu iterations",
bytes, itercount);

You could then have a real-time graph of the values for variables "bytes"
and "itercount" using the recorder_scope program, and using the names you
gave to the channels in your RECORDER_TRACES variable, i.e. bytes and loops:

 recorder_scope bytes loops

See man recorder_trace_set(3), recorder_scope(1) or
https://github.com/c3d/recorder#recorder-tracing for more information.


> and perhaps more importantly should we have this modelled as a command
> line arg instead of an env variable. We've generally been aiming to get
> rid of env variables and have QAPI modelled CLI. QAPI modelling would be
> particularly important if we want to expose the ablity to change settings
> on the fly via QMP.

The rationale for the recorder using an environment variable is that it was
initially designed to be able to trace libraries, notably SPICE or the
recorder library itself. 

Re: [PATCH] spice: simplify chardev setup

[Christophe de Dinechin]

On 2020-07-22 at 13:18 CEST, Gerd Hoffmann wrote...
> On Wed, Jul 22, 2020 at 12:19:43PM +0200, Christophe de Dinechin wrote:
>>
>> On 2020-07-22 at 11:20 CEST, Christophe de Dinechin wrote...
>> > On 2020-07-22 at 10:49 CEST, Gerd Hoffmann wrote...
>> >> Initialize spice before chardevs.  That allows to register the spice
>> >> chardevs directly in the init function and removes the need to maintain
>> >> a linked list of chardevs just for registration.
>> >>
>> >> Signed-off-by: Gerd Hoffmann 
>> >
>> > Looks good to me, but I still need to test how this integrates with my work
>> > on putting SPICE in a module.
>>
>> That part does not seem to work so well. It looks like with this move, the
>> init happens before the module is loaded:
>>
>>qemu-system-x86_64 -display spice-app
>>Unexpected error in qemu_chr_open_spice_port() at 
>> ui/../chardev/spice.c:307:
>>qemu-system-x86_64: spice not enabled
>>
>> I'll try to find the correct fix. Any idea how to address this?
>
> move chardev init from early to normal:

Works for me.

Reviewed-by: Christophe de Dinechin 
Tested-by: Christophe de Dinechin 

>
> commit 77297a71e6be60997caf2c55c09ce01a8c492bc2
> Author: Gerd Hoffmann 
> Date:   Wed Jul 22 13:17:28 2020 +0200
>
> [fixup] spice app
>
> diff --git a/ui/spice-app.c b/ui/spice-app.c
> index 40fb2ef57399..03d971b15f0c 100644
> --- a/ui/spice-app.c
> +++ b/ui/spice-app.c
> @@ -117,7 +117,6 @@ static void spice_app_atexit(void)
>  static void spice_app_display_early_init(DisplayOptions *opts)
>  {
>  QemuOpts *qopts;
> -ChardevBackend *be = chr_spice_backend_new();
>  GError *err = NULL;
>
>  if (opts->has_full_screen) {
> @@ -162,6 +161,15 @@ static void spice_app_display_early_init(DisplayOptions 
> *opts)
>  qemu_opt_set(qopts, "gl", opts->has_gl ? "on" : "off", _abort);
>  display_opengl = opts->has_gl;
>  #endif
> +}
> +
> +static void spice_app_display_init(DisplayState *ds, DisplayOptions *opts)
> +{
> +ChardevBackend *be = chr_spice_backend_new();
> +QemuOpts *qopts;
> +GError *err = NULL;
> +gchar *uri;
> +
>  be->u.spiceport.data->fqdn = g_strdup("org.qemu.monitor.qmp.0");
>  qemu_chardev_new("org.qemu.monitor.qmp", TYPE_CHARDEV_SPICEPORT,
>   be, NULL, _abort);
> @@ -171,13 +179,6 @@ static void spice_app_display_early_init(DisplayOptions 
> *opts)
>  qemu_opt_set(qopts, "mode", "control", _abort);
>
>  qapi_free_ChardevBackend(be);
> -}
> -
> -static void spice_app_display_init(DisplayState *ds, DisplayOptions *opts)
> -{
> -GError *err = NULL;
> -gchar *uri;
> -
>  uri = g_strjoin("", "spice+unix://", app_dir, "/", "spice.sock", NULL);
>  info_report("Launching display with URI: %s", uri);
>  g_app_info_launch_default_for_uri(uri, NULL, );


--
Cheers,
Christophe de Dinechin (IRC c3d)

Re: [PATCH v5 03/10] qdev: device module support

[Christophe de Dinechin]

On 2020-07-22 at 13:05 CEST, Gerd Hoffmann wrote...
> On Wed, Jul 22, 2020 at 10:05:51AM +0200, Christophe de Dinechin wrote:
>>
>> On 2020-07-21 at 16:27 CEST, Gerd Hoffmann wrote...
>> >   Hi,
>> >
>> >> >  DeviceState *qdev_new(const char *name)
>> >> >  {
>> >> > +if (!object_class_by_name(name)) {
>> >> > +module_load_qom_one(name);
>> >> > +}
>> >>
>> >> Curious why you don't you call module_object_class_by_name here?
>> >
>> > Because object_new() wants a name not an ObjectClass ...
>>
>> I'm talking about the two lines above.
>>
>> if (!object_class_by_name(name)) {
>> module_load_qom_one(name);
>> }
>>
>> Thi9s code looks very similar to the code below:
>>
>> ObjectClass *module_object_class_by_name(const char *typename)
>> {
>> ObjectClass *oc;
>>
>> oc = object_class_by_name(typename);
>> #ifdef CONFIG_MODULES
>> if (!oc) {
>> module_load_qom_one(typename);
>> oc = object_class_by_name(typename);
>> }
>> #endif
>> return oc;
>> }
>>
>> Both call module_load_qom_one and object_class_by_name using the name as
>> input, so I don't see the difference (except for the order).
>
> Yes, calling module_object_class_by_name then throw away the result
> would work too.  I don't like the idea to hide the module loading
> though.

Why do you consider calling a function called "module_object_class_by_name"
as hiding the module loading?

More importantly, why is it better to have two ways to do the same thing
that are slightly different? The reason for the slight difference is really
unclear to me. If we later do a change to module_object_class_by_name, are
there cases where we won't need the same change in qdev_new?

>
> take care,
>   Gerd


--
Cheers,
Christophe de Dinechin (IRC c3d)

Re: [PATCH] spice: simplify chardev setup

[Christophe de Dinechin]

On 2020-07-22 at 11:20 CEST, Christophe de Dinechin wrote...
> On 2020-07-22 at 10:49 CEST, Gerd Hoffmann wrote...
>> Initialize spice before chardevs.  That allows to register the spice
>> chardevs directly in the init function and removes the need to maintain
>> a linked list of chardevs just for registration.
>>
>> Signed-off-by: Gerd Hoffmann 
>
> Looks good to me, but I still need to test how this integrates with my work
> on putting SPICE in a module.

That part does not seem to work so well. It looks like with this move, the
init happens before the module is loaded:

   qemu-system-x86_64 -display spice-app
   Unexpected error in qemu_chr_open_spice_port() at ui/../chardev/spice.c:307:
   qemu-system-x86_64: spice not enabled

I'll try to find the correct fix. Any idea how to address this?

>
> Reviewed-by: Christophe de Dinechin 
>
>> ---
>>  include/chardev/spice.h |  1 -
>>  include/ui/qemu-spice.h |  1 -
>>  chardev/spice.c | 29 ++---
>>  softmmu/vl.c|  9 +
>>  ui/spice-core.c |  2 --
>>  5 files changed, 11 insertions(+), 31 deletions(-)
>>
>> diff --git a/include/chardev/spice.h b/include/chardev/spice.h
>> index 1f7339b649dc..2013255f34fd 100644
>> --- a/include/chardev/spice.h
>> +++ b/include/chardev/spice.h
>> @@ -12,7 +12,6 @@ typedef struct SpiceChardev {
>>  bool  blocked;
>>  const uint8_t *datapos;
>>  int   datalen;
>> -QLIST_ENTRY(SpiceChardev) next;
>>  } SpiceChardev;
>>
>>  #define TYPE_CHARDEV_SPICE "chardev-spice"
>> diff --git a/include/ui/qemu-spice.h b/include/ui/qemu-spice.h
>> index 8c23dfe71797..d34cea2e0fcd 100644
>> --- a/include/ui/qemu-spice.h
>> +++ b/include/ui/qemu-spice.h
>> @@ -46,7 +46,6 @@ int qemu_spice_migrate_info(const char *hostname, int 
>> port, int tls_port,
>>  #else
>>  #define SPICE_NEEDS_SET_MM_TIME 0
>>  #endif
>> -void qemu_spice_register_ports(void);
>>
>>  #else  /* CONFIG_SPICE */
>>
>> diff --git a/chardev/spice.c b/chardev/spice.c
>> index bf7ea1e2940d..9733f0671699 100644
>> --- a/chardev/spice.c
>> +++ b/chardev/spice.c
>> @@ -14,9 +14,6 @@ typedef struct SpiceCharSource {
>>  SpiceChardev   *scd;
>>  } SpiceCharSource;
>>
>> -static QLIST_HEAD(, SpiceChardev) spice_chars =
>> -QLIST_HEAD_INITIALIZER(spice_chars);
>> -
>>  static int vmc_write(SpiceCharDeviceInstance *sin, const uint8_t *buf, int 
>> len)
>>  {
>>  SpiceChardev *scd = container_of(sin, SpiceChardev, sin);
>> @@ -216,8 +213,6 @@ static void char_spice_finalize(Object *obj)
>>
>>  vmc_unregister_interface(s);
>>
>> -QLIST_SAFE_REMOVE(s, next);
>> -
>>  g_free((char *)s->sin.subtype);
>>  g_free((char *)s->sin.portname);
>>  }
>> @@ -256,8 +251,6 @@ static void chr_open(Chardev *chr, const char *subtype)
>>
>>  s->active = false;
>>  s->sin.subtype = g_strdup(subtype);
>> -
>> -QLIST_INSERT_HEAD(_chars, s, next);
>>  }
>>
>>  static void qemu_chr_open_spice_vmc(Chardev *chr,
>> @@ -310,28 +303,18 @@ void qemu_chr_open_spice_port(Chardev *chr,
>>  return;
>>  }
>>
>> +if (!using_spice) {
>> +error_setg(errp, "spice not enabled");
>> +return;
>> +}
>> +
>>  chr_open(chr, "port");
>>
>>  *be_opened = false;
>>  s = SPICE_CHARDEV(chr);
>>  s->sin.portname = g_strdup(name);
>>
>> -if (using_spice) {
>> -/* spice server already created */
>> -vmc_register_interface(s);
>> -}
>> -}
>> -
>> -void qemu_spice_register_ports(void)
>> -{
>> -SpiceChardev *s;
>> -
>> -QLIST_FOREACH(s, _chars, next) {
>> -if (s->sin.portname == NULL) {
>> -continue;
>> -}
>> -vmc_register_interface(s);
>> -}
>> +vmc_register_interface(s);
>>  }
>>
>>  static void qemu_chr_parse_spice_vmc(QemuOpts *opts, ChardevBackend 
>> *backend,
>> diff --git a/softmmu/vl.c b/softmmu/vl.c
>> index f476ef89edb7..bc0dcc4f58bd 100644
>> --- a/softmmu/vl.c
>> +++ b/softmmu/vl.c
>> @@ -4125,6 +4125,11 @@ void qemu_init(int argc, char **argv, char **envp)
>>  page_size_init();
>>  socket_init();
>>
>> +/* spice needs the timers to be initialized by this point */
>> + 

Re: [PATCH] spice: simplify chardev setup

[Christophe de Dinechin]

On 2020-07-22 at 10:49 CEST, Gerd Hoffmann wrote...
> Initialize spice before chardevs.  That allows to register the spice
> chardevs directly in the init function and removes the need to maintain
> a linked list of chardevs just for registration.
>
> Signed-off-by: Gerd Hoffmann 

Looks good to me, but I still need to test how this integrates with my work
on putting SPICE in a module.

Reviewed-by: Christophe de Dinechin 

> ---
>  include/chardev/spice.h |  1 -
>  include/ui/qemu-spice.h |  1 -
>  chardev/spice.c | 29 ++---
>  softmmu/vl.c|  9 +
>  ui/spice-core.c |  2 --
>  5 files changed, 11 insertions(+), 31 deletions(-)
>
> diff --git a/include/chardev/spice.h b/include/chardev/spice.h
> index 1f7339b649dc..2013255f34fd 100644
> --- a/include/chardev/spice.h
> +++ b/include/chardev/spice.h
> @@ -12,7 +12,6 @@ typedef struct SpiceChardev {
>  bool  blocked;
>  const uint8_t *datapos;
>  int   datalen;
> -QLIST_ENTRY(SpiceChardev) next;
>  } SpiceChardev;
>
>  #define TYPE_CHARDEV_SPICE "chardev-spice"
> diff --git a/include/ui/qemu-spice.h b/include/ui/qemu-spice.h
> index 8c23dfe71797..d34cea2e0fcd 100644
> --- a/include/ui/qemu-spice.h
> +++ b/include/ui/qemu-spice.h
> @@ -46,7 +46,6 @@ int qemu_spice_migrate_info(const char *hostname, int port, 
> int tls_port,
>  #else
>  #define SPICE_NEEDS_SET_MM_TIME 0
>  #endif
> -void qemu_spice_register_ports(void);
>
>  #else  /* CONFIG_SPICE */
>
> diff --git a/chardev/spice.c b/chardev/spice.c
> index bf7ea1e2940d..9733f0671699 100644
> --- a/chardev/spice.c
> +++ b/chardev/spice.c
> @@ -14,9 +14,6 @@ typedef struct SpiceCharSource {
>  SpiceChardev   *scd;
>  } SpiceCharSource;
>
> -static QLIST_HEAD(, SpiceChardev) spice_chars =
> -QLIST_HEAD_INITIALIZER(spice_chars);
> -
>  static int vmc_write(SpiceCharDeviceInstance *sin, const uint8_t *buf, int 
> len)
>  {
>  SpiceChardev *scd = container_of(sin, SpiceChardev, sin);
> @@ -216,8 +213,6 @@ static void char_spice_finalize(Object *obj)
>
>  vmc_unregister_interface(s);
>
> -QLIST_SAFE_REMOVE(s, next);
> -
>  g_free((char *)s->sin.subtype);
>  g_free((char *)s->sin.portname);
>  }
> @@ -256,8 +251,6 @@ static void chr_open(Chardev *chr, const char *subtype)
>
>  s->active = false;
>  s->sin.subtype = g_strdup(subtype);
> -
> -QLIST_INSERT_HEAD(_chars, s, next);
>  }
>
>  static void qemu_chr_open_spice_vmc(Chardev *chr,
> @@ -310,28 +303,18 @@ void qemu_chr_open_spice_port(Chardev *chr,
>  return;
>  }
>
> +if (!using_spice) {
> +error_setg(errp, "spice not enabled");
> +return;
> +}
> +
>  chr_open(chr, "port");
>
>  *be_opened = false;
>  s = SPICE_CHARDEV(chr);
>  s->sin.portname = g_strdup(name);
>
> -if (using_spice) {
> -/* spice server already created */
> -vmc_register_interface(s);
> -}
> -}
> -
> -void qemu_spice_register_ports(void)
> -{
> -SpiceChardev *s;
> -
> -QLIST_FOREACH(s, _chars, next) {
> -if (s->sin.portname == NULL) {
> -continue;
> -}
> -vmc_register_interface(s);
> -}
> +vmc_register_interface(s);
>  }
>
>  static void qemu_chr_parse_spice_vmc(QemuOpts *opts, ChardevBackend *backend,
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index f476ef89edb7..bc0dcc4f58bd 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -4125,6 +4125,11 @@ void qemu_init(int argc, char **argv, char **envp)
>  page_size_init();
>  socket_init();
>
> +/* spice needs the timers to be initialized by this point */
> +/* spice must initialize before audio as it changes the default auiodev 
> */
> +/* spice must initialize before chardevs (for spicevmc and spiceport) */
> +qemu_spice_init();
> +
>  qemu_opts_foreach(qemu_find_opts("object"),
>user_creatable_add_opts_foreach,
>object_create_initial, _fatal);
> @@ -4139,10 +4144,6 @@ void qemu_init(int argc, char **argv, char **envp)
>fsdev_init_func, NULL, _fatal);
>  #endif
>
> -/* spice needs the timers to be initialized by this point */
> -/* spice must initialize before audio as it changes the default auiodev 
> */
> -qemu_spice_init();
> -
>  /*
>   * Note: we need to create audio and block backends before
>   * machine_set_property(), so machine properties can refer to
> diff --git a/ui/spice-core.c b/ui/spice-core.c
> index ecc2ec2c55c2..37dd68f2aba2 100644
> --- a/ui/spice-core.c
> +++ b/ui/spice-core.c
> @@ -813,8 +813,6 @@ void qemu_spice_init(void)
>  g_free(x509_cert_file);
>  g_free(x509_cacert_file);
>
> -qemu_spice_register_ports();
> -
>  #ifdef HAVE_SPICE_GL
>  if (qemu_opt_get_bool(opts, "gl", 0)) {
>  if ((port != 0) || (tls_port != 0)) {


--
Cheers,
Christophe de Dinechin (IRC c3d)

Re: [PATCH v5 02/10] object: qom module support

[Christophe de Dinechin]

On 2020-06-24 at 15:10 CEST, Gerd Hoffmann wrote...
> Little helper function to load modules on demand.  In most cases adding
> module loading support for devices and other objects is just
> s/object_class_by_name/module_object_class_by_name/ in the right spot.
>
> Signed-off-by: Gerd Hoffmann 
> ---
>  include/qom/object.h | 12 
>  qom/object.c | 14 ++
>  2 files changed, 26 insertions(+)
>
> diff --git a/include/qom/object.h b/include/qom/object.h
> index 94a61ccc3fe8..51f188137f1f 100644
> --- a/include/qom/object.h
> +++ b/include/qom/object.h
> @@ -994,6 +994,18 @@ bool object_class_is_abstract(ObjectClass *klass);
>   */
>  ObjectClass *object_class_by_name(const char *typename);
>
> +/**
> + * module_object_class_by_name:
> + * @typename: The QOM typename to obtain the class for.
> + *
> + * For objects which might be provided by a module.  Behaves like
> + * object_class_by_name, but additionally tries to load the module
> + * needed in case the class is not available.
> + *
> + * Returns: The class for @typename or %NULL if not found.
> + */
> +ObjectClass *module_object_class_by_name(const char *typename);
> +
>  void object_class_foreach(void (*fn)(ObjectClass *klass, void *opaque),
>const char *implements_type, bool include_abstract,
>void *opaque);
> diff --git a/qom/object.c b/qom/object.c
> index 6ece96bc2bfc..34daaf1280f5 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -985,6 +985,20 @@ ObjectClass *object_class_by_name(const char *typename)
>  return type->class;
>  }
>
> +ObjectClass *module_object_class_by_name(const char *typename)

Re-reading this, it occurred to me that typename is a keyword in C++.

I don't know if we want to make it a rule to not use C++ keywords just in
case we would some day compile that code with a C++ compiler.

> +{
> +ObjectClass *oc;
> +
> +oc = object_class_by_name(typename);
> +#ifdef CONFIG_MODULES
> +if (!oc) {
> +module_load_qom_one(typename);
> +oc = object_class_by_name(typename);
> +}
> +#endif
> +return oc;
> +}
> +
>  ObjectClass *object_class_get_parent(ObjectClass *class)
>  {
>  TypeImpl *type = type_get_parent(class->type);


--
Cheers,
Christophe de Dinechin (IRC c3d)

Re: [PATCH v5 05/10] ccid: build smartcard as module

[Christophe de Dinechin]

On 2020-07-21 at 16:33 CEST, Gerd Hoffmann wrote...
>> >  ifeq ($(CONFIG_USB_SMARTCARD),y)
>> >  common-obj-y  += dev-smartcard-reader.o
>>
>> I'm curious why you don't use something like:
>>
>> common-obj-$(CONFIG_USB_SMARTCARD)
>>
>> Do we want to be able to configure individual elements as modules?
>> Or is the intent to force as module things that are marked as 'y'?
>
> qemu kconfig miniconf handles bool only, not tristate.

Ah, I guess that right, I had a small "fix" for part of that in a recent
RFC, but you don't have it. OK.

>
> So, yes, for now we can do only "all modules" or "no modules" but
> nothing inbetween.
>
> take care,
>   Gerd


--
Cheers,
Christophe de Dinechin (IRC c3d)

Re: [PATCH v5 03/10] qdev: device module support

[Christophe de Dinechin]

On 2020-07-21 at 16:27 CEST, Gerd Hoffmann wrote...
>   Hi,
>
>> >  DeviceState *qdev_new(const char *name)
>> >  {
>> > +if (!object_class_by_name(name)) {
>> > +module_load_qom_one(name);
>> > +}
>>
>> Curious why you don't you call module_object_class_by_name here?
>
> Because object_new() wants a name not an ObjectClass ...

I'm talking about the two lines above.

if (!object_class_by_name(name)) {
module_load_qom_one(name);
}

Thi9s code looks very similar to the code below:

ObjectClass *module_object_class_by_name(const char *typename)
{
ObjectClass *oc;

oc = object_class_by_name(typename);
#ifdef CONFIG_MODULES
if (!oc) {
module_load_qom_one(typename);
oc = object_class_by_name(typename);
}
#endif
return oc;
}

Both call module_load_qom_one and object_class_by_name using the name as
input, so I don't see the difference (except for the order).

Am I reading this wrong?

>
>> >      return DEVICE(object_new(name));
>> >  }
>
> take care,
>   Gerd


--
Cheers,
Christophe de Dinechin (IRC c3d)

Re: [PATCH v5 10/10] chardev: enable modules, use for braille

[Christophe de Dinechin]

On 2020-06-24 at 15:10 CEST, Gerd Hoffmann wrote...
> Removes brlapi library dependency from core qemu.
>
> Signed-off-by: Gerd Hoffmann 
> ---
>  Makefile.objs | 1 +
>  chardev/char.c| 2 +-
>  util/module.c | 1 +
>  chardev/Makefile.objs | 5 -
>  4 files changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/Makefile.objs b/Makefile.objs
> index ca555ede0710..2dfcd19713f8 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -71,6 +71,7 @@ common-obj-$(CONFIG_TPM) += tpm.o
>
>  common-obj-y += backends/
>  common-obj-y += chardev/
> +common-obj-m += chardev/
>
>  common-obj-$(CONFIG_SECCOMP) += qemu-seccomp.o
>  qemu-seccomp.o-cflags := $(SECCOMP_CFLAGS)
> diff --git a/chardev/char.c b/chardev/char.c
> index e3051295ac37..df697f3ce9e0 100644
> --- a/chardev/char.c
> +++ b/chardev/char.c
> @@ -527,7 +527,7 @@ static const ChardevClass *char_get_class(const char 
> *driver, Error **errp)
>  const ChardevClass *cc;
>  char *typename = g_strdup_printf("chardev-%s", driver);
>
> -oc = object_class_by_name(typename);
> +oc = module_object_class_by_name(typename);
>  g_free(typename);
>
>  if (!object_class_dynamic_cast(oc, TYPE_CHARDEV)) {
> diff --git a/util/module.c b/util/module.c
> index a74214eac052..32b0547b8266 100644
> --- a/util/module.c
> +++ b/util/module.c
> @@ -272,6 +272,7 @@ static struct {
>  { "vhost-user-gpu-device", "hw-", "display-virtio-gpu"},
>  { "vhost-user-gpu-pci","hw-", "display-virtio-gpu"},
>  { "vhost-user-vga","hw-", "display-virtio-gpu"},
> +{ "chardev-braille",   "chardev-", "baum" },
>  };
>
>  static bool module_loaded_qom_all;
> diff --git a/chardev/Makefile.objs b/chardev/Makefile.objs
> index d68e1347f9af..3a58c9d329d6 100644
> --- a/chardev/Makefile.objs
> +++ b/chardev/Makefile.objs
> @@ -18,8 +18,11 @@ chardev-obj-$(CONFIG_WIN32) += char-win.o
>  chardev-obj-$(CONFIG_WIN32) += char-win-stdio.o
>
>  common-obj-y += msmouse.o wctablet.o testdev.o
> -common-obj-$(CONFIG_BRLAPI) += baum.o
> +
> +ifeq ($(CONFIG_BRLAPI),y)
> +common-obj-m += baum.o

Shouldn't that be a .mo?

>  baum.o-cflags := $(SDL_CFLAGS)
>  baum.o-libs := $(BRLAPI_LIBS)
> +endif
>
>  common-obj-$(CONFIG_SPICE) += spice.o


--
Cheers,
Christophe de Dinechin (IRC c3d)

Re: [PATCH v5 09/10] vga: build virtio-gpu as module

[Christophe de Dinechin]

On 2020-06-24 at 15:10 CEST, Gerd Hoffmann wrote...
> Drops libvirglrenderer.so dependency from core qemu.
>
> Signed-off-by: Gerd Hoffmann 
> ---
>  util/module.c|  6 ++
>  hw/display/Makefile.objs | 23 +--
>  2 files changed, 19 insertions(+), 10 deletions(-)
>
> diff --git a/util/module.c b/util/module.c
> index 7c76d2a84b94..a74214eac052 100644
> --- a/util/module.c
> +++ b/util/module.c
> @@ -266,6 +266,12 @@ static struct {
>  { "usb-redir", "hw-", "usb-redirect"  },
>  { "qxl-vga",   "hw-", "display-qxl"   },
>  { "qxl",   "hw-", "display-qxl"   },
> +{ "virtio-gpu-device", "hw-", "display-virtio-gpu"},
> +{ "virtio-gpu-pci","hw-", "display-virtio-gpu"},
> +{ "virtio-vga","hw-", "display-virtio-gpu"},
> +{ "vhost-user-gpu-device", "hw-", "display-virtio-gpu"},
> +{ "vhost-user-gpu-pci","hw-", "display-virtio-gpu"},
> +{ "vhost-user-vga","hw-", "display-virtio-gpu"},
>  };
>
>  static bool module_loaded_qom_all;
> diff --git a/hw/display/Makefile.objs b/hw/display/Makefile.objs
> index d619594ad4d3..e907f3182b0c 100644
> --- a/hw/display/Makefile.objs
> +++ b/hw/display/Makefile.objs
> @@ -49,16 +49,19 @@ common-obj-m += qxl.mo
>  qxl.mo-objs = qxl.o qxl-logger.o qxl-render.o
>  endif
>
> -common-obj-$(CONFIG_VIRTIO_GPU) += virtio-gpu-base.o virtio-gpu.o 
> virtio-gpu-3d.o
> -common-obj-$(CONFIG_VHOST_USER_GPU) += vhost-user-gpu.o
> -common-obj-$(call land,$(CONFIG_VIRTIO_GPU),$(CONFIG_VIRTIO_PCI)) += 
> virtio-gpu-pci.o
> -common-obj-$(call land,$(CONFIG_VHOST_USER_GPU),$(CONFIG_VIRTIO_PCI)) += 
> vhost-user-gpu-pci.o
> -common-obj-$(CONFIG_VIRTIO_VGA) += virtio-vga.o
> -common-obj-$(CONFIG_VHOST_USER_VGA) += vhost-user-vga.o
> -virtio-gpu.o-cflags := $(VIRGL_CFLAGS)
> -virtio-gpu.o-libs += $(VIRGL_LIBS)
> -virtio-gpu-3d.o-cflags := $(VIRGL_CFLAGS)
> -virtio-gpu-3d.o-libs += $(VIRGL_LIBS)
> +ifeq ($(CONFIG_VIRTIO_GPU),y)
> +common-obj-m += virtio-gpu.mo
> +virtio-gpu-obj-$(CONFIG_VIRTIO_GPU) += virtio-gpu-base.o virtio-gpu.o 
> virtio-gpu-3d.o
> +virtio-gpu-obj-$(CONFIG_VHOST_USER_GPU) += vhost-user-gpu.o
> +virtio-gpu-obj-$(call land,$(CONFIG_VIRTIO_GPU),$(CONFIG_VIRTIO_PCI)) += 
> virtio-gpu-pci.o
> +virtio-gpu-obj-$(call land,$(CONFIG_VHOST_USER_GPU),$(CONFIG_VIRTIO_PCI)) += 
> vhost-user-gpu-pci.o
> +virtio-gpu-obj-$(CONFIG_VIRTIO_VGA) += virtio-vga.o
> +virtio-gpu-obj-$(CONFIG_VHOST_USER_VGA) += vhost-user-vga.o
> +virtio-gpu.mo-objs := $(virtio-gpu-obj-y)
> +virtio-gpu.mo-cflags := $(VIRGL_CFLAGS)
> +virtio-gpu.mo-libs := $(VIRGL_LIBS)
> +endif
> +
>  common-obj-$(CONFIG_DPCD) += dpcd.o
>  common-obj-$(CONFIG_XLNX_ZYNQMP_ARM) += xlnx_dp.o

Reviewed-by: Christophe de Dinechin 

--
Cheers,
Christophe de Dinechin (IRC c3d)

Re: [PATCH v5 07/10] vga: build qxl as module

[Christophe de Dinechin]

On 2020-06-24 at 15:10 CEST, Gerd Hoffmann wrote...
> First step in making spice support modular.
>
> Signed-off-by: Gerd Hoffmann 
> ---
>  util/module.c| 2 ++
>  hw/Makefile.objs | 1 +
>  hw/display/Makefile.objs | 5 -
>  3 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/util/module.c b/util/module.c
> index e3226165e91c..7c76d2a84b94 100644
> --- a/util/module.c
> +++ b/util/module.c
> @@ -264,6 +264,8 @@ static struct {
>  { "ccid-card-passthru","hw-", "usb-smartcard" },
>  { "ccid-card-emulated","hw-", "usb-smartcard" },
>  { "usb-redir", "hw-", "usb-redirect"  },
> +{ "qxl-vga",   "hw-", "display-qxl"   },
> +{ "qxl",   "hw-", "display-qxl"   },
>  };
>
>  static bool module_loaded_qom_all;
> diff --git a/hw/Makefile.objs b/hw/Makefile.objs
> index af8fd9a510ed..14b7ea4eb62e 100644
> --- a/hw/Makefile.objs
> +++ b/hw/Makefile.objs
> @@ -43,5 +43,6 @@ devices-dirs-y += smbios/
>  endif
>
>  common-obj-y += $(devices-dirs-y)
> +common-obj-m += display/
>  common-obj-m += usb/
>  obj-y += $(devices-dirs-y)
> diff --git a/hw/display/Makefile.objs b/hw/display/Makefile.objs
> index 77a7d622bd2d..76b3571e4902 100644
> --- a/hw/display/Makefile.objs
> +++ b/hw/display/Makefile.objs
> @@ -44,7 +44,10 @@ common-obj-$(CONFIG_ARTIST) += artist.o
>
>  obj-$(CONFIG_VGA) += vga.o
>
> -common-obj-$(CONFIG_QXL) += qxl.o qxl-logger.o qxl-render.o
> +ifeq ($(CONFIG_QXL),y)
> +common-obj-m += qxl.mo
> +qxl.mo-objs = qxl.o qxl-logger.o qxl-render.o
> +endif
>
>  obj-$(CONFIG_VIRTIO_GPU) += virtio-gpu-base.o virtio-gpu.o virtio-gpu-3d.o
>  obj-$(CONFIG_VHOST_USER_GPU) += vhost-user-gpu.o

Reviewed-by: Christophe de Dinechin 

--
Cheers,
Christophe de Dinechin (IRC c3d)

Re: [PATCH v5 06/10] usb: build usb-redir as module

[Christophe de Dinechin]

On 2020-06-24 at 15:10 CEST, Gerd Hoffmann wrote...
> Drops libusbredirparser.so dependency from core qemu.
>
> Signed-off-by: Gerd Hoffmann 
> ---
>  util/module.c| 1 +
>  hw/usb/Makefile.objs | 9 ++---
>  2 files changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/util/module.c b/util/module.c
> index 89da9a3cce05..e3226165e91c 100644
> --- a/util/module.c
> +++ b/util/module.c
> @@ -263,6 +263,7 @@ static struct {
>  } const qom_modules[] = {
>  { "ccid-card-passthru","hw-", "usb-smartcard" },
>  { "ccid-card-emulated","hw-", "usb-smartcard" },
> +{ "usb-redir", "hw-", "usb-redirect"  },
>  };
>
>  static bool module_loaded_qom_all;
> diff --git a/hw/usb/Makefile.objs b/hw/usb/Makefile.objs
> index 3c5b3d4fadd3..e342ff59fab0 100644
> --- a/hw/usb/Makefile.objs
> +++ b/hw/usb/Makefile.objs
> @@ -43,9 +43,12 @@ endif
>
>  # usb redirection
>  ifeq ($(CONFIG_USB),y)
> -common-obj-$(CONFIG_USB_REDIR) += redirect.o quirks.o
> -redirect.o-cflags = $(USB_REDIR_CFLAGS)
> -redirect.o-libs = $(USB_REDIR_LIBS)
> +ifeq ($(CONFIG_USB_REDIR),y)
> +common-obj-m += redirect.mo
> +redirect.mo-objs = redirect.o quirks.o
> +redirect.mo-cflags = $(USB_REDIR_CFLAGS)
> +redirect.mo-libs = $(USB_REDIR_LIBS)
> +endif
>  endif
>
>  # usb pass-through

With the same questions as for earlier patches

Reviewed-by: Christophe de Dinechin 


--
Cheers,
Christophe de Dinechin (IRC c3d)

Re: [PATCH v5 05/10] ccid: build smartcard as module

[Christophe de Dinechin]

On 2020-06-24 at 15:10 CEST, Gerd Hoffmann wrote...
> Drops libcacard.so dependency from core qemu.
>
> Signed-off-by: Gerd Hoffmann 
> ---
>  Makefile.objs| 1 +
>  util/module.c| 2 ++
>  hw/Makefile.objs | 1 +
>  hw/usb/Makefile.objs | 4 +++-
>  4 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/Makefile.objs b/Makefile.objs
> index 7ce2588b89a3..ca555ede0710 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -59,6 +59,7 @@ common-obj-y += migration/
>  common-obj-y += audio/
>  common-obj-m += audio/
>  common-obj-y += hw/
> +common-obj-m += hw/
>
>  common-obj-y += replay/
>
> diff --git a/util/module.c b/util/module.c
> index ee560a4b4269..89da9a3cce05 100644
> --- a/util/module.c
> +++ b/util/module.c
> @@ -261,6 +261,8 @@ static struct {
>  const char *prefix;
>  const char *module;
>  } const qom_modules[] = {
> +{ "ccid-card-passthru","hw-", "usb-smartcard" },
> +{ "ccid-card-emulated","hw-", "usb-smartcard" },
>  };
>
>  static bool module_loaded_qom_all;
> diff --git a/hw/Makefile.objs b/hw/Makefile.objs
> index 4cbe5e4e57d6..af8fd9a510ed 100644
> --- a/hw/Makefile.objs
> +++ b/hw/Makefile.objs
> @@ -43,4 +43,5 @@ devices-dirs-y += smbios/
>  endif
>
>  common-obj-y += $(devices-dirs-y)
> +common-obj-m += usb/
>  obj-y += $(devices-dirs-y)
> diff --git a/hw/usb/Makefile.objs b/hw/usb/Makefile.objs
> index fa5c3fa1b877..3c5b3d4fadd3 100644
> --- a/hw/usb/Makefile.objs
> +++ b/hw/usb/Makefile.objs
> @@ -29,11 +29,13 @@ common-obj-$(CONFIG_USB_NETWORK)  += dev-network.o
>
>  ifeq ($(CONFIG_USB_SMARTCARD),y)
>  common-obj-y  += dev-smartcard-reader.o

I'm curious why you don't use something like:

common-obj-$(CONFIG_USB_SMARTCARD)

Do we want to be able to configure individual elements as modules?
Or is the intent to force as module things that are marked as 'y'?

> -common-obj-$(CONFIG_SMARTCARD)+= smartcard.mo
> +ifeq ($(CONFIG_SMARTCARD),y)
> +common-obj-m  += smartcard.mo
>  smartcard.mo-objs := ccid-card-passthru.o ccid-card-emulated.o
>  smartcard.mo-cflags := $(SMARTCARD_CFLAGS)
>  smartcard.mo-libs := $(SMARTCARD_LIBS)
>  endif
> +endif
>
>  ifeq ($(CONFIG_POSIX),y)
>  common-obj-$(CONFIG_USB_STORAGE_MTP)  += dev-mtp.o


--
Cheers,
Christophe de Dinechin (IRC c3d)

Re: [PATCH v5 03/10] qdev: device module support

[Christophe de Dinechin]

On 2020-06-24 at 15:10 CEST, Gerd Hoffmann wrote...
> Hook module loading into the places where we
> need it when building devices as modules.
>
> Signed-off-by: Gerd Hoffmann 
> ---
>  hw/core/qdev.c | 6 --
>  qdev-monitor.c | 5 +++--
>  qom/qom-qmp-cmds.c | 3 ++-
>  softmmu/vl.c   | 4 ++--
>  4 files changed, 11 insertions(+), 7 deletions(-)
>
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 2131c7f951dd..9de16eae05b7 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -137,6 +137,9 @@ void qdev_set_parent_bus(DeviceState *dev, BusState *bus)
>   */
>  DeviceState *qdev_new(const char *name)
>  {
> +if (!object_class_by_name(name)) {
> +module_load_qom_one(name);
> +}

Curious why you don't you call module_object_class_by_name here?


>  return DEVICE(object_new(name));
>  }
>
> @@ -147,10 +150,9 @@ DeviceState *qdev_new(const char *name)
>   */
>  DeviceState *qdev_try_new(const char *name)
>  {
> -if (!object_class_by_name(name)) {
> +if (!module_object_class_by_name(name)) {
>  return NULL;
>  }
> -
>  return DEVICE(object_new(name));
>  }
>
> diff --git a/qdev-monitor.c b/qdev-monitor.c
> index 22da107484c5..8e7a7f7bbdbc 100644
> --- a/qdev-monitor.c
> +++ b/qdev-monitor.c
> @@ -147,6 +147,7 @@ static void qdev_print_devinfos(bool show_no_user)
>  int i;
>  bool cat_printed;
>
> +module_load_qom_all();
>  list = object_class_get_list_sorted(TYPE_DEVICE, false);
>
>  for (i = 0; i <= DEVICE_CATEGORY_MAX; i++) {
> @@ -215,13 +216,13 @@ static DeviceClass *qdev_get_device_class(const char 
> **driver, Error **errp)
>  DeviceClass *dc;
>  const char *original_name = *driver;
>
> -oc = object_class_by_name(*driver);
> +oc = module_object_class_by_name(*driver);
>  if (!oc) {
>  const char *typename = find_typename_by_alias(*driver);
>
>  if (typename) {
>  *driver = typename;
> -oc = object_class_by_name(*driver);
> +oc = module_object_class_by_name(*driver);
>  }
>  }
>
> diff --git a/qom/qom-qmp-cmds.c b/qom/qom-qmp-cmds.c
> index c5249e44d020..5e2c8cbf333f 100644
> --- a/qom/qom-qmp-cmds.c
> +++ b/qom/qom-qmp-cmds.c
> @@ -116,6 +116,7 @@ ObjectTypeInfoList *qmp_qom_list_types(bool 
> has_implements,
>  {
>  ObjectTypeInfoList *ret = NULL;
>
> +module_load_qom_all();
>  object_class_foreach(qom_list_types_tramp, implements, abstract, );
>
>  return ret;
> @@ -130,7 +131,7 @@ ObjectPropertyInfoList *qmp_device_list_properties(const 
> char *typename,
>  ObjectPropertyIterator iter;
>  ObjectPropertyInfoList *prop_list = NULL;
>
> -klass = object_class_by_name(typename);
> +klass = module_object_class_by_name(typename);
>  if (klass == NULL) {
>  error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
>"Device '%s' not found", typename);
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index f669c06ede4a..1297815a5f5b 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -1772,8 +1772,8 @@ static bool vga_interface_available(VGAInterfaceType t)
>
>  assert(t < VGA_TYPE_MAX);
>  return !ti->class_names[0] ||
> -   object_class_by_name(ti->class_names[0]) ||
> -   object_class_by_name(ti->class_names[1]);
> +   module_object_class_by_name(ti->class_names[0]) ||
> +   module_object_class_by_name(ti->class_names[1]);
>  }
>
>  static const char *


--
Cheers,
Christophe de Dinechin (IRC c3d)

Re: [PATCH v5 02/10] object: qom module support

[Christophe de Dinechin]

On 2020-06-24 at 15:10 CEST, Gerd Hoffmann wrote...
> Little helper function to load modules on demand.  In most cases adding
> module loading support for devices and other objects is just
> s/object_class_by_name/module_object_class_by_name/ in the right spot.
>
> Signed-off-by: Gerd Hoffmann 
> ---
>  include/qom/object.h | 12 
>  qom/object.c | 14 ++
>  2 files changed, 26 insertions(+)
>
> diff --git a/include/qom/object.h b/include/qom/object.h
> index 94a61ccc3fe8..51f188137f1f 100644
> --- a/include/qom/object.h
> +++ b/include/qom/object.h
> @@ -994,6 +994,18 @@ bool object_class_is_abstract(ObjectClass *klass);
>   */
>  ObjectClass *object_class_by_name(const char *typename);
>
> +/**
> + * module_object_class_by_name:
> + * @typename: The QOM typename to obtain the class for.
> + *
> + * For objects which might be provided by a module.  Behaves like
> + * object_class_by_name, but additionally tries to load the module
> + * needed in case the class is not available.
> + *
> + * Returns: The class for @typename or %NULL if not found.
> + */
> +ObjectClass *module_object_class_by_name(const char *typename);
> +
>  void object_class_foreach(void (*fn)(ObjectClass *klass, void *opaque),
>const char *implements_type, bool include_abstract,
>void *opaque);
> diff --git a/qom/object.c b/qom/object.c
> index 6ece96bc2bfc..34daaf1280f5 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -985,6 +985,20 @@ ObjectClass *object_class_by_name(const char *typename)
>  return type->class;
>  }
>
> +ObjectClass *module_object_class_by_name(const char *typename)
> +{
> +ObjectClass *oc;
> +
> +oc = object_class_by_name(typename);
> +#ifdef CONFIG_MODULES
> +if (!oc) {
> +module_load_qom_one(typename);
> +oc = object_class_by_name(typename);
> +}
> +#endif

I'm wondering if there is any reason to only trigger the module load when
you don't find the object class. You could simply call module_load_qom_one
under #ifdef CONFIG_MODULES.

Performance wise, I don't think this makes much of a difference, and it
simplifies the logical flow IMO.

> +return oc;
> +}
> +
>  ObjectClass *object_class_get_parent(ObjectClass *class)
>  {
>  TypeImpl *type = type_get_parent(class->type);


--
Cheers,
Christophe de Dinechin (IRC c3d)

Re: [PATCH v5 01/10] module: qom module support

[Christophe de Dinechin]

On 2020-06-24 at 15:10 CEST, Gerd Hoffmann wrote...
> Add support for qom types provided by modules.  For starters use a
> manually maintained list which maps qom type to module and prefix.
>
> Two load functions are added:  One to load the module for a specific
> type, and one to load all modules (needed for object/device lists as
> printed by -- for example -- qemu -device help).
>
> Signed-off-by: Gerd Hoffmann 
> ---
>  include/qemu/module.h |  2 ++
>  util/module.c | 55 +++
>  2 files changed, 57 insertions(+)
>
> diff --git a/include/qemu/module.h b/include/qemu/module.h
> index 011ae1ae7605..9121a475c1b6 100644
> --- a/include/qemu/module.h
> +++ b/include/qemu/module.h
> @@ -70,5 +70,7 @@ void register_dso_module_init(void (*fn)(void), 
> module_init_type type);
>
>  void module_call_init(module_init_type type);
>  bool module_load_one(const char *prefix, const char *lib_name);
> +void module_load_qom_one(const char *type);
> +void module_load_qom_all(void);
>
>  #endif
> diff --git a/util/module.c b/util/module.c
> index e48d9aacc05a..ee560a4b4269 100644
> --- a/util/module.c
> +++ b/util/module.c
> @@ -245,3 +245,58 @@ bool module_load_one(const char *prefix, const char 
> *lib_name)
>  #endif
>  return success;
>  }
> +
> +/*
> + * Building devices and other qom objects modular is mostly useful in
> + * case they have dependencies to external shared libraries, so we can
> + * cut down the core qemu library dependencies.  Which is the case for
> + * only a very few devices & objects.
> + *
> + * So with the expectation that this will be rather the exception than
> + * to rule and the list will not gain that many entries go with a

Nit: Add a comma after "entries"

I would also indicate that this list needs to be sorted by prefix/module

> + * simple manually maintained list for now.
> + */
> +static struct {
> +const char *type;
> +const char *prefix;
> +const char *module;

Because of the sorting rule, it is more intuitive to put the module first
and the type last, for cases where you have

mod1 prefix1 type1
mod1 prefix1 type2
mod1 prefix1 type3
mod2 prefix2 type1

Visually, I expect the constants to come first.


> +} const qom_modules[] = {
> +};
> +
> +static bool module_loaded_qom_all;
> +
> +void module_load_qom_one(const char *type)
> +{
> +int i;
> +
> +if (module_loaded_qom_all) {
> +return;
> +}
> +for (i = 0; i < ARRAY_SIZE(qom_modules); i++) {
> +if (strcmp(qom_modules[i].type, type) == 0) {
> +module_load_one(qom_modules[i].prefix,
> +qom_modules[i].module);
> +return;
> +}
> +}
> +}
> +
> +void module_load_qom_all(void)
> +{
> +int i;
> +
> +if (module_loaded_qom_all) {
> +return;
> +}
> +for (i = 0; i < ARRAY_SIZE(qom_modules); i++) {
> +if (i > 0 && (strcmp(qom_modules[i - 1].module,
> + qom_modules[i].module) == 0 &&
> +  strcmp(qom_modules[i - 1].prefix,
> + qom_modules[i].prefix) == 0)) {
> +/* one module implementing multiple types -> load only once */

This is the sorting requirement I'm talking about

> +continue;
> +}
> +module_load_one(qom_modules[i].prefix, qom_modules[i].module);
> +}
> +module_loaded_qom_all = true;
> +}


--
Cheers,
Christophe de Dinechin (IRC c3d)

Re: [PATCH v5 04/10] build: fix device module builds

[Christophe de Dinechin]

On 2020-06-24 at 15:10 CEST, Gerd Hoffmann wrote...
> See comment.  Feels quite hackish.  Better ideas anyone?
>
> Signed-off-by: Gerd Hoffmann 
> ---
>  Makefile.target | 7 +++
>  1 file changed, 7 insertions(+)
>
> diff --git a/Makefile.target b/Makefile.target
> index 8ed1eba95b9c..c70325df5796 100644
> --- a/Makefile.target
> +++ b/Makefile.target
> @@ -179,6 +179,13 @@ endif # CONFIG_SOFTMMU
>  dummy := $(call unnest-vars,,obj-y)
>  all-obj-y := $(obj-y)
>
> +#
> +# common-obj-m has some crap here, probably as side effect from
> +# filling obj-y.  Clear it.  Fixes suspious dependency errors when

Typo: suspicious

> +# building devices as modules.
> +#

(As an aside: I'm also not filled with confidence by this comment ;-)

> +common-obj-m :=
> +
>  include $(SRC_PATH)/Makefile.objs
>  dummy := $(call unnest-vars,.., \
> authz-obj-y \


--
Cheers,
Christophe de Dinechin (IRC c3d)

Re: build: haiky system build fix

[Christophe de Dinechin]

nit: typo in the mail subject (haiky instead of haiku)

> On 25 Jun 2020, at 20:36, David CARLIER  wrote:
> 
> From 25adbdcdc17ef51a41759f16576901338ed8a469 Mon Sep 17 00:00:00 2001
> From: David Carlier 
> Date: Thu, 25 Jun 2020 19:32:42 +
> Subject: [PATCH] build: haiku system build fix
> 
> Most of missing features resides in the bsd library.
> Also defining constant equivalence.
> 
> Signed-off-by: David Carlier 
> ---
> configure|  4 ++--
> include/qemu/bswap.h |  2 ++
> os-posix.c   |  4 
> util/compatfd.c  |  2 ++
> util/drm.c   |  2 ++
> util/main-loop.c |  3 +++
> util/oslib-posix.c   | 20 
> util/qemu-openpty.c  |  2 +-
> 8 files changed, 36 insertions(+), 3 deletions(-)
> 
> diff --git a/configure b/configure
> index ba88fd1824..3732ad4e35 100755
> --- a/configure
> +++ b/configure
> @@ -901,8 +901,8 @@ SunOS)
> ;;
> Haiku)
>   haiku="yes"
> -  QEMU_CFLAGS="-DB_USE_POSITIVE_POSIX_ERRORS $QEMU_CFLAGS"
> -  LIBS="-lposix_error_mapper -lnetwork $LIBS"
> +  QEMU_CFLAGS="-DB_USE_POSITIVE_POSIX_ERRORS -D_BSD_SOURCE $QEMU_CFLAGS"
> +  LIBS="-lposix_error_mapper -lnetwork -lbsd $LIBS"
> ;;
> Linux)
>   audio_drv_list="try-pa oss"
> diff --git a/include/qemu/bswap.h b/include/qemu/bswap.h
> index 2a9f3fe783..1d3e4c24e4 100644
> --- a/include/qemu/bswap.h
> +++ b/include/qemu/bswap.h
> @@ -8,6 +8,8 @@
> # include 
> #elif defined(__FreeBSD__)
> # include 
> +#elif defined(__HAIKU__)
> +# include 
> #elif defined(CONFIG_BYTESWAP_H)
> # include 
> 
> diff --git a/os-posix.c b/os-posix.c
> index 3cd52e1e70..ca07b7702d 100644
> --- a/os-posix.c
> +++ b/os-posix.c
> @@ -42,6 +42,8 @@
> #include 
> #endif
> 
> +#include 
> +
> /*
>  * Must set all three of these at once.
>  * Legal combinations are  unset   by name   by uid
> @@ -339,10 +341,12 @@ int os_mlock(void)
> {
> int ret = 0;
> 
> +#if !defined(__HAIKU__)
> ret = mlockall(MCL_CURRENT | MCL_FUTURE);
> if (ret < 0) {
> error_report("mlockall: %s", strerror(errno));
> }
> 
> +#endif
> return ret;
> }
> diff --git a/util/compatfd.c b/util/compatfd.c
> index c296f55d14..ee47dd8089 100644
> --- a/util/compatfd.c
> +++ b/util/compatfd.c
> @@ -16,7 +16,9 @@
> #include "qemu/osdep.h"
> #include "qemu/thread.h"
> 
> +#if defined(CONFIG_SIGNALFD)
> #include 
> +#endif
> 
> struct sigfd_compat_info
> {
> diff --git a/util/drm.c b/util/drm.c
> index a23ff24538..8540578c09 100644
> --- a/util/drm.c
> +++ b/util/drm.c
> @@ -38,9 +38,11 @@ int qemu_drm_rendernode_open(const char *rendernode)
> 
> fd = -1;
> while ((e = readdir(dir))) {
> +#if !defined(__HAIKU__)
> if (e->d_type != DT_CHR) {
> continue;
> }
> +#endif
> 
> if (strncmp(e->d_name, "renderD", 7)) {
> continue;
> diff --git a/util/main-loop.c b/util/main-loop.c
> index eda63fe4e0..1ce3081ced 100644
> --- a/util/main-loop.c
> +++ b/util/main-loop.c
> @@ -85,6 +85,9 @@ static int qemu_signal_init(Error **errp)
>  * by sigwait() in the signal thread. Otherwise, the cpu thread will
>  * not catch it reliably.
>  */
> +#ifndef SIGIO
> +#define SIGIO SIGPOLL
> +#endif
> sigemptyset();
> sigaddset(, SIG_IPI);
> sigaddset(, SIGIO);
> diff --git a/util/oslib-posix.c b/util/oslib-posix.c
> index 39ddc77c85..a18d90a68a 100644
> --- a/util/oslib-posix.c
> +++ b/util/oslib-posix.c
> @@ -38,7 +38,12 @@
> #include "qemu/sockets.h"
> #include "qemu/thread.h"
> #include 
> +#if !defined(__HAIKU__)
> #include 
> +#else
> +#include 
> +#include 
> +#endif
> #include "qemu/cutils.h"
> 
> #ifdef CONFIG_LINUX
> @@ -390,6 +395,21 @@ void qemu_init_exec_dir(const char *argv0)
> }
> }
> }
> +#elif defined(__HAIKU__)
> +{
> +image_info ii;
> +int32_t c = 0;
> +
> +*buf = '\0';
> +while (get_next_image_info(0, , ) == B_OK) {
> +if (ii.type == B_APP_IMAGE) {
> +strncpy(buf, ii.name, sizeof(buf));
> +buf[sizeof(buf) - 1] = '\0';
> +p = buf;
> +break;
> +}
> +}
> +}
> #endif
> /* If we don't have any way of figuring out the actual executable
>location then try argv[0].  */
> diff --git a/util/qemu-openpty.c b/util/qemu-openpty.c
> index 2e8b43bdf5..c29fc2cbf9 100644
> --- a/util/qemu-openpty.c
> +++ b/util/qemu-openpty.c
> @@ -35,7 +35,7 @@
> #include "qemu/osdep.h"
> #include "qemu-common.h"
> 
> -#if defined(__GLIBC__)
> +#if defined(__GLIBC__) || defined(__HAIKU__)
> # include 
> #elif defined CONFIG_BSD
> # include 
> -- 
> 2.26.0
> 

[PATCH v3 2/2] trace: Example of non-tracing recorder use

[Christophe de Dinechin]

This patch is a simple example showing how the recorder can be used to
have one "topic" covering multiple entries. Here, the topic is "lock",
the idea being to have the latest lock changes for instance in case of
a crash or hang.

Here are a few use cases:

* Tracing  lock updates:
RECORDER_TRACES=lock qemu
* Showing lock changes prior to a hang
RECORDER_TRACES=lock qemu &
# Wait until hang
killall -USR2 qemu  # This will trigger a dump
* Graphic visualization of lock states:
RECORDER_TRACES="lock=state,id" qemu &
recorder_scope state
# Hit the 't' key to toggle timing display
# Hit the 'c' key to dump the screen data as CSV
cat recorder_scope_data-1.csv

Signed-off-by: Christophe de Dinechin 
---
 util/qemu-thread-common.h | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/util/qemu-thread-common.h b/util/qemu-thread-common.h
index 2af6b12085..0de07a471f 100644
--- a/util/qemu-thread-common.h
+++ b/util/qemu-thread-common.h
@@ -15,6 +15,9 @@
 
 #include "qemu/thread.h"
 #include "trace.h"
+#include "trace/recorder.h"
+
+RECORDER_DEFINE(lock, 16, "Lock state");
 
 static inline void qemu_mutex_post_init(QemuMutex *mutex)
 {
@@ -23,12 +26,14 @@ static inline void qemu_mutex_post_init(QemuMutex *mutex)
 mutex->line = 0;
 #endif
 mutex->initialized = true;
+record(lock, "Init state %d for %p", -1, mutex);
 }
 
 static inline void qemu_mutex_pre_lock(QemuMutex *mutex,
const char *file, int line)
 {
 trace_qemu_mutex_lock(mutex, file, line);
+record(lock, "Locking state %d for %p", 1, mutex);
 }
 
 static inline void qemu_mutex_post_lock(QemuMutex *mutex,
@@ -39,6 +44,7 @@ static inline void qemu_mutex_post_lock(QemuMutex *mutex,
 mutex->line = line;
 #endif
 trace_qemu_mutex_locked(mutex, file, line);
+record(lock, "Locked state %d for %p", 2, mutex);
 }
 
 static inline void qemu_mutex_pre_unlock(QemuMutex *mutex,
@@ -49,6 +55,7 @@ static inline void qemu_mutex_pre_unlock(QemuMutex *mutex,
 mutex->line = 0;
 #endif
 trace_qemu_mutex_unlock(mutex, file, line);
+record(lock, "Unkocked state %d for %p", 0, mutex);
 }
 
 #endif
-- 
2.26.2

[PATCH v3 1/2] trace: Add support for recorder back-end

[Christophe de Dinechin]

The recorder library provides support for low-cost continuous
recording of events, which can then be replayed. This makes it
possible to collect data after the fact, for example to show the
events that led to a crash or unexpected condition.

In this series, minimal recorder support in qemu is implemented using
the existing tracing interface. For each trace, a corresponding
recorder is created with a generic name and a default size of 8 entries.

In addition, it is possible to explicitly enable recorders that are
not qemu traces, for example in order to use actually record events
rather than trace them, or to use the real-time graphing capabilities.
For that reason, a limited set of recorder-related macros are defined
as no-ops even if the recorder trace backend is not configured.

Recorder-specific features, notably the ability to perform a
post-mortem dump and to group traces by topic, are not integrated in
this series, as doing so would require modifying the trace
infrastructure, which is a non-objective here. This may be the topic
of later series if there is any interest for it.

HMP COMMAND:
The 'recorder' hmp command has been added, which supports two
sub-commands:
* recorder dump: Dump the current state of the recorder. You can
  give that command a recorder name, to only dump that recorder.
* recorder trace: Set traces using the recorder_trace_set() syntax.
  You can use "recorder trace help" to list all available recorders.

Signed-off-by: Christophe de Dinechin 
---
 configure | 14 
 hmp-commands.hx   | 23 +++-
 monitor/misc.c| 27 ++
 scripts/tracetool/backend/recorder.py | 52 +++
 trace/Makefile.objs   |  1 +
 trace/control.c   |  7 
 trace/recorder.c  | 25 +
 trace/recorder.h  | 32 +
 util/module.c |  8 +
 9 files changed, 188 insertions(+), 1 deletion(-)
 create mode 100644 scripts/tracetool/backend/recorder.py
 create mode 100644 trace/recorder.c
 create mode 100644 trace/recorder.h

diff --git a/configure b/configure
index 8a65240d4a..3c30f2defd 100755
--- a/configure
+++ b/configure
@@ -7727,6 +7727,20 @@ fi
 if have_backend "log"; then
   echo "CONFIG_TRACE_LOG=y" >> $config_host_mak
 fi
+if have_backend "recorder"; then
+recorder_minver="1.0.10"
+if $pkg_config --atleast-version=$recorder_minver recorder ; then
+recorder_cflags="$($pkg_config --cflags recorder)"
+recorder_libs="$($pkg_config --libs recorder)"
+LIBS="$recorder_libs $LIBS"
+libs_qga="$recorder_libs $libs_qga"
+QEMU_CFLAGS="$QEMU_CFLAGS $recorder_cflags"
+zstd="yes"
+echo "CONFIG_TRACE_RECORDER=y" >> $config_host_mak
+else
+feature_not_found "recorder" "Install recorder-devel package"
+fi
+fi
 if have_backend "ust"; then
   echo "CONFIG_TRACE_UST=y" >> $config_host_mak
 fi
diff --git a/hmp-commands.hx b/hmp-commands.hx
index d548a3ab74..f164ff6d65 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -302,6 +302,28 @@ SRST
   Open, close, or flush the trace file.  If no argument is given, the
   status of the trace file is displayed.
 ERST
+#endif
+
+#if defined(CONFIG_TRACE_RECORDER)
+{
+.name   = "recorder",
+.args_type  = "op:s?,arg:s?",
+.params = "trace|dump [arg]",
+.help   = "trace selected recorders or print recorder dump",
+.cmd= hmp_recorder,
+},
+
+SRST
+``trace`` *cmds*
+  Activate or deactivate tracing for individual recorder traces.
+  See recorder_trace_set(3) for the syntax of *cmds*
+  For example, to activate trace ``foo`` and disable alll traces
+  ending in ``_warning``, use ``foo:.*_warning=0``.
+  Using ``help`` will list available traces and their current setting.
+``dump`` [*name*]
+  Dump the recorder. If *name* is given, only the specific named recorder
+  will be dumped.
+ERST
 #endif
 
 {
@@ -1828,4 +1850,3 @@ ERST
 .sub_table  = hmp_info_cmds,
 .flags  = "p",
 },
-
diff --git a/monitor/misc.c b/monitor/misc.c
index 89bb970b00..0094b1860f 100644
--- a/monitor/misc.c
+++ b/monitor/misc.c
@@ -61,6 +61,9 @@
 #ifdef CONFIG_TRACE_SIMPLE
 #include "trace/simple.h"
 #endif
+#ifdef CONFIG_TRACE_RECORDER
+#include "trace/recorder.h"
+#endif
 #include "exec/memory.h"
 #include "exec/exec-all.h"
 #include "qemu/option.h"
@@ -227,6 +230,30 @@ static void hmp_trace_file(Monitor *mon, const QDict 
*qdict)
 }
 #endif
 
+#ifdef CONFIG_TRACE_RECORDER
+static void hmp_recorder(Monitor *mon, const QDict *

[PATCH v3 0/2] trace: Add a trace backend for the recorder library

[Christophe de Dinechin]

The recorder library implements low-cost always-on tracing, with three
usage models:

1. Flight recorder: Dump information on recent events in case of crash
2. Tracing: Individual traces can be enabled using environment variables
3. Real-time graphing / control, using the recorder_scope application

This short series introduces a new "recorder" back-end which connects
to the recorder. Traces using the recorder are intentionally "always on",
because the recorder library is primarily designed to record
information for later playback in case of crash, tracing being only a
secondary capability.

An example is given of how the recorder can also be used separately
from generated traces. The example uses locking, which can make sense
for both post-mortem and real-time graphing.

Changes in v3:
* Address coding style issues (C++ comments, wrong include, etc)
* Fix args type for HMP command (for now, still a single command)
* Add basic help for HMP command
* Use pkg-config for recorder information. This requires recorder
  1.0.10 or later.

Later patches wil address larger topics that were discussed that
would impact other tracing mechanisms, as well as GitHub / GitLab
build tests.

Christophe de Dinechin (2):
  trace: Add support for recorder back-end
  trace: Example of non-tracing recorder use

 configure | 14 
 hmp-commands.hx   | 23 +++-
 monitor/misc.c| 27 ++
 scripts/tracetool/backend/recorder.py | 52 +++
 trace/Makefile.objs   |  1 +
 trace/control.c   |  7 
 trace/recorder.c  | 25 +
 trace/recorder.h  | 32 +
 util/module.c |  8 +
 util/qemu-thread-common.h |  7 
 10 files changed, 195 insertions(+), 1 deletion(-)
 create mode 100644 scripts/tracetool/backend/recorder.py
 create mode 100644 trace/recorder.c
 create mode 100644 trace/recorder.h

-- 
2.26.2

[PATCH] trivial: Remove trailing whitespaces

[Christophe de Dinechin]

There are a number of unnecessary trailing whitespaces that have
accumulated over time in the source code. They cause stray changes
in patches if you use tools that automatically remove them.

Tested by doing a `git diff -w` after the change.

This could probably be turned into a pre-commit hook.

Signed-off-by: Christophe de Dinechin 
---
 block/iscsi.c |   2 +-
 disas/cris.c  |   2 +-
 disas/microblaze.c|  80 +++---
 disas/nios2.c | 256 +-
 hmp-commands.hx   |   2 +-
 hw/alpha/typhoon.c|   6 +-
 hw/arm/gumstix.c  |   6 +-
 hw/arm/omap1.c|   2 +-
 hw/arm/stellaris.c|   2 +-
 hw/char/etraxfs_ser.c |   2 +-
 hw/core/ptimer.c  |   2 +-
 hw/cris/axis_dev88.c  |   2 +-
 hw/cris/boot.c|   2 +-
 hw/display/qxl.c  |   2 +-
 hw/dma/etraxfs_dma.c  |  18 +-
 hw/dma/i82374.c   |   2 +-
 hw/i2c/bitbang_i2c.c  |   2 +-
 hw/input/tsc2005.c|   2 +-
 hw/input/tsc210x.c|   2 +-
 hw/intc/etraxfs_pic.c |   8 +-
 hw/intc/sh_intc.c |  10 +-
 hw/intc/xilinx_intc.c |   2 +-
 hw/misc/imx25_ccm.c   |   6 +-
 hw/misc/imx31_ccm.c   |   2 +-
 hw/net/vmxnet3.h  |   2 +-
 hw/net/xilinx_ethlite.c   |   2 +-
 hw/pci/pcie.c |   2 +-
 hw/sd/omap_mmc.c  |   2 +-
 hw/sh4/shix.c |   2 +-
 hw/sparc64/sun4u.c|   2 +-
 hw/timer/etraxfs_timer.c  |   2 +-
 hw/timer/xilinx_timer.c   |   4 +-
 hw/usb/hcd-musb.c |  10 +-
 hw/usb/hcd-ohci.c |   6 +-
 hw/usb/hcd-uhci.c |   2 +-
 hw/virtio/virtio-pci.c|   2 +-
 include/hw/cris/etraxfs_dma.h |   4 +-
 include/hw/net/lance.h|   2 +-
 include/hw/ppc/spapr.h|   2 +-
 include/hw/xen/interface/io/ring.h|  34 +--
 include/qemu/log.h|   2 +-
 include/qom/object.h  |   4 +-
 linux-user/cris/cpu_loop.c|  16 +-
 linux-user/microblaze/cpu_loop.c  |  16 +-
 linux-user/mmap.c |   8 +-
 linux-user/sparc/signal.c |   4 +-
 linux-user/syscall.c  |  24 +-
 linux-user/syscall_defs.h |   2 +-
 linux-user/uaccess.c  |   2 +-
 os-posix.c|   2 +-
 qapi/qapi-util.c  |   2 +-
 qemu-img.c|   2 +-
 qemu-options.hx   |  26 +-
 qom/object.c  |   2 +-
 target/cris/translate.c   |  28 +-
 target/cris/translate_v10.inc.c   |   6 +-
 target/i386/hvf/hvf.c |   4 +-
 target/i386/hvf/x86.c |   4 +-
 target/i386/hvf/x86_decode.c  |  20 +-
 target/i386/hvf/x86_decode.h  |   4 +-
 target/i386/hvf/x86_descr.c   |   2 +-
 target/i386/hvf/x86_emu.c |   2 +-
 target/i386/hvf/x86_mmu.c |   6 +-
 target/i386/hvf/x86_task.c|   2 +-
 target/i386/hvf/x86hvf.c  |  42 +--
 target/i386/translate.c   |   8 +-
 target/microblaze/mmu.c   |   2 +-
 target/microblaze/translate.c |   2 +-
 target/sh4/op_helper.c|   4 +-
 target/xtensa/core-de212/core-isa.h   |   6 +-
 .../xtensa/core-sample_controller/core-isa.h  |   6 +-
 target/xtensa/core-test_kc705_be/core-isa.h   |   2 +-
 tcg/sparc/tcg-target.inc.c|   2 +-
 tcg/tcg.c |  32 +--
 tests/tcg/multiarch/test-mmap.c   |  72 ++---
 ui/curses.c   |   4 +-
 ui/curses_keys.h  |   4 +-
 util/cutils.c |   2 +-
 78 files changed, 440 insertions(+), 440 deletions(-)

diff --git a/block/iscsi.c b/block/iscsi.c
index a8b76979d8..884075f4e1 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -1412,7 +1412,7 @@ static void iscsi_readcapacity_sync(IscsiLun *iscsilun, 
Error

Re: [PATCH v2 3/3] trace: Example of "centralized" recorder tracing

[Christophe de Dinechin]

On 2020-07-03 at 15:08 CEST, Daniel P. Berrangé wrote...
> On Fri, Jul 03, 2020 at 12:12:14PM +0200, Christophe de Dinechin wrote:
>>
>> On 2020-07-02 at 15:47 CEST, Stefan Hajnoczi wrote...
>> > On Wed, Jul 01, 2020 at 05:15:01PM +0100, Daniel P. Berrangé wrote:
>> > At most it should require an
>> >> extra annotation in the trace-events file to take the extra parameter
>> >> for grouping, and other trace backends can ignore that.
>> >
>> > It's true, it may be possible to put this functionality in the
>> > trace-events.
>>
>> Let me think more about integrating these features with other trace
>> backends. See below for short-term impact.
>>
>> > Christophe: how does this differ from regular trace events and what
>> > extra information is needed?
>>
>> - Grouping, as indicated above, mostly useful in practice to make selection
>>   of tracing topics easy (e.g. "modules") but also for real-time graphing,
>>   because typically a state change occurs in different functions, which is
>>   why I used locking as an example.
>
> In many cases we already have a form of grouping via the backdoor by
> using a common string prefix on trace point names. eg
>
> qio_task_new(void *task, void *source, void *func, void *opaque) "Task new 
> task=%p source=%p func=%p opaque=%p"
> qio_task_complete(void *task) "Task complete task=%p"
> qio_task_thread_start(void *task, void *worker, void *opaque) "Task thread 
> start task=%p worker=%p opaque=%p"
> qio_task_thread_run(void *task) "Task thread run task=%p"
> qio_task_thread_exit(void *task) "Task thread exit task=%p"
> qio_task_thread_result(void *task) "Task thread result task=%p"
> qio_task_thread_source_attach(void *task, void *source) "Task thread source 
> attach task=%p source=%p"
> qio_task_thread_source_cancel(void *task, void *source) "Task thread source 
> cancel task=%p source=%p"
>
> We could turn this into an explicit multi-level grouping concept,
> using "." separator for each level of the group. eg

Interesting idea.

>
> qio.task.new(void *task, void *source, void *func, void *opaque) "Task new 
> task=%p source=%p func=%p opaque=%p"
> qio.task.complete(void *task) "Task complete task=%p"
> qio.task.thread_start(void *task, void *worker, void *opaque) "Task thread 
> start task=%p worker=%p opaque=%p"
> qio.task.thread_run(void *task) "Task thread run task=%p"
> qio.task.thread_exit(void *task) "Task thread exit task=%p"
> qio.task.thread_result(void *task) "Task thread result task=%p"
> qio.task.thread_source_attach(void *task, void *source) "Task thread source 
> attach task=%p source=%p"
> qio.task.thread_source_cancel(void *task, void *source) "Task thread source 
> cancel task=%p source=%p"
>
>
> For backends that dont support groups, the "." can be turned back
> into a "_", and everything is unchanged. For backends that do support
> groups, we now have the info required.
>
> This would be useful for the systemtap backend because that has an
> explicit grouping concept already. ie we have probes definitions
> like:
>
> probe qemu.system.x86_64.qio_task_new = 
> process("/usr/bin/qemu-system-x86_64").mark("qio_task_new")
> {
>   task = $arg1;
>   source = $arg2;
>   func = $arg3;
>   opaque = $arg4;
> }
>
>
> which we can now turn into
>
> probe qemu.system.x86_64.qio.task.new = 
> process("/usr/bin/qemu-system-x86_64").mark("qio_task_new")
> {
>   task = $arg1;
>   source = $arg2;
>   func = $arg3;
>   opaque = $arg4;
> }
>
>
> the generated C trace macro would still use "_" not "." of course
> so  qio_task_new(src, func, opaque)
>
>> - Self-documentation. Right now, the recorder back-end generates rather
>>   unhelpful help messages.
>
> Not sure I understand the problem here.

RECORDER_DEFINE specifies a text that is listed when you pass "help" as a
trace name. There is no equivalent concept that I'm aware of in the existing
qemu trace infrastructure.

>
>> - Trace buffer size. This is important to make post-mortem dumps usable if
>>   you record infrequent events alongside much higher-rate ones. For example,
>>   you may want a large buffer to record info about command-line option
>>   processing, the default 8 is definitely too small.
>
> Sure, that's a problem for all the trace backends. They all need to be
> good at filtering / controlling what probes are fired to avoid si

Re: [PATCH v2 0/3] trace: Add a trace backend for the recorder library

[Christophe de Dinechin]

On 2020-06-30 at 14:59 CEST, Stefan Hajnoczi wrote...
> On Fri, Jun 26, 2020 at 06:27:03PM +0200, Christophe de Dinechin wrote:
>> The recorder library implements low-cost always-on tracing, with three
>> usage models:
>>
>> 1. Flight recorder: Dump information on recent events in case of crash
>> 2. Tracing: Individual traces can be enabled using environment variables
>> 3. Real-time graphing / control, using the recorder_scope application
>>
>> This short series introduces a new "recorder" back-end which connects
>> to the recorder. Traces using the recorder are intentionally "always on".
>> An example is given of how the recorder can also be used separately
>> from generated traces. This can be useful if you want to enable
>> multiple related traces for a particular topic.
>>
>> This series requires a small makefile fix submitted earlier, included
>> here for convenience.
>>
>> Christophe de Dinechin (3):
>>   Makefile: Compute libraries for libqemuutil.a and libvhost-user.a
>>   trace: Add support for recorder back-end
>>   trace: Example of "centralized" recorder tracing
>
> Please add a build to .travis.yml that enables recorder. That way we'll
> catch build failures.

There is no recorder package in Xenial.

>
> Thanks,
> Stefan



--
Cheers,
Christophe de Dinechin (IRC c3d)

Re: [PATCH v2 3/3] trace: Example of "centralized" recorder tracing

[Christophe de Dinechin]

On 2020-07-02 at 15:47 CEST, Stefan Hajnoczi wrote...
> On Wed, Jul 01, 2020 at 05:15:01PM +0100, Daniel P. Berrangé wrote:
>> On Wed, Jul 01, 2020 at 05:09:06PM +0100, Stefan Hajnoczi wrote:
>> > On Tue, Jun 30, 2020 at 01:41:36PM +0100, Daniel P. Berrangé wrote:
>> > > On Fri, Jun 26, 2020 at 06:27:06PM +0200, Christophe de Dinechin wrote:
>> > > IMHO the whole point of having the pluggable trace backend impls, is
>> > > precisely that we don't have to add multiple different calls in the
>> > > code. A single trace_qemu_mutex_unlock() is supposed to work with
>> > > any backend.
>> >
>> > I think an exception is okay when the other trace backends do not offer
>> > equivalent functionality.
>> >
>> > Who knows if anyone other than Christophe will use this functionality,
>> > but it doesn't cost much to allow it.
>>
>> This patch is just an example though, suggesting this kind of usage is
>> expected to done in other current trace probe locations. The trace wrapper
>> has most of the information required already including a format string,
>> so I'd think it could be wired up to the generator so we don't add extra
>> record() statements through the codebase.

The primary purpose of the recorder is post-mortem dumps, flight recorder
style. Tracing is only a secondary benefit. Not sure if it's worth making a
distinction between events you want to record and those you want to trace.
(Example: You might want to record all command line options, but almost
never trace them)

> At most it should require an
>> extra annotation in the trace-events file to take the extra parameter
>> for grouping, and other trace backends can ignore that.
>
> It's true, it may be possible to put this functionality in the
> trace-events.

Let me think more about integrating these features with other trace
backends. See below for short-term impact.

> Christophe: how does this differ from regular trace events and what
> extra information is needed?

- Grouping, as indicated above, mostly useful in practice to make selection
  of tracing topics easy (e.g. "modules") but also for real-time graphing,
  because typically a state change occurs in different functions, which is
  why I used locking as an example.

- Self-documentation. Right now, the recorder back-end generates rather
  unhelpful help messages.

- Trace buffer size. This is important to make post-mortem dumps usable if
  you record infrequent events alongside much higher-rate ones. For example,
  you may want a large buffer to record info about command-line option
  processing, the default 8 is definitely too small.

- Support for %+s, which tells that a string is safe to print later (e.g. it
  is a compile-time constant, or never ever freed).

- Support for custom formats, e.g. I use %v in the XL compiler for LLVM
  value pointers. This is a bit more advanced, but it would be neat to be
  able to print out QOM objects using %q :-)

For the short term, what about providing trace-named wrappers around these
additional recorder features?

--
Cheers,
Christophe de Dinechin (IRC c3d)

Re: [PATCH] Makefile: Compute libraries for libqemuutil.a and libvhost-user.a

[Christophe de Dinechin]

On 2020-06-23 at 16:44 CEST, Stefan Hajnoczi wrote...
> On Tue, Jun 16, 2020 at 06:18:14PM +0200, Christophe de Dinechin wrote:
>> In util/Makefile.objs, there is a setting for dbus.o-libs.
>> Trying to copy-paste that to add a library for module.o that was was
>> not otherwise linked yields link errors on a number of binaries,
>> e.g. qemu-ga, with unsatisfied symbols in libqemuutil.a(module.o).
>> The reason is that library dependencies are not propagated to the .a
>> files automatically.
>>
>> Adding a call to extract-libs to get the libraries for the two .a
>> files that are used elsewhere.
>>
>> Signed-off-by: Christophe de Dinechin 
>> ---
>>  Makefile | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/Makefile b/Makefile
>> index 2e93068894..5fb0c78a0b 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -598,6 +598,8 @@ Makefile: $(version-obj-y)
>>  ##
>>  # Build libraries
>>
>> +libqemuutil.a-libs += $(call extract-libs, $(util-obj-y) $(trace-obj-y) 
>> $(stub-obj-y))
>> +libvhost-user.a-libs += $(call extract-libs, $(libvhost-user-obj-y) 
>> $(util-obj-y) $(stub-obj-y))
>>  libqemuutil.a: $(util-obj-y) $(trace-obj-y) $(stub-obj-y)
>>  libvhost-user.a: $(libvhost-user-obj-y) $(util-obj-y) $(stub-obj-y)
>
> I wonder if there is GNU Make syntax to avoid duplicating the
> dependencies of libqemuutil.a and libvhost-user.a?

The dependencies are not identical. Would you want two lines, one with the
shared dependencies, i.e. something like:

  libqemuutils.a libhost-user.a: $(util-obj-y) $(stub-obj-y)
  libqemuutils.a: $(trace-obj-y)
  libvhost-user.a: $(libvhost-user-obj-y)

Or do you mean something else?

>
> Another thing I wonder about: the purpose of the .a files is to compile
> all object files and only link those .o files needed by the program
> (i.e. a subset of the .a file).

I believe that what you are saying is that by passing the required libraries
automatically, the binaries that use libqemuutil.a will inherit undesired
ldd dependencies. Indeed, a quick experiment shows that if you pass a -l
option, the library dependency is recorded even if no symbol in that library
is used. I saw no obvious linker option to address that.

Maybe add a comment, then, because otherwise it is surprising to have an
unsat despite something like:

  foo.o-libs = -lbar

As I pointed out in my commit comment, there is at least one case where this
is used, dbus.o-libs, suggesting that this is expected to work. This seems
to be the only case that goes in util-obj-y today. Apparently, this works
because the link of qemu-system-x86_64 already gets $(GIO_LIBS) from
some .o file that is not going through libqemuutil.a

>
> Now that libqemuutil.a-libs is defined, do programs using libqemuutil.a
> link libs required by .o files that aren't being used?

Yes. The alternative being that you get unsatisfied symbols if you do use
the .o file.

>
> For example, say we had util/mp3encoder.o which depends on an MP3
> encoder library. A utility like qemu-img does not depend on mp3encoder.o
> from libqemuutil.a. Therefore -lmp3enc or whatever shouldn't be on
> qemu-img's linker command-line.

If that's the intent, then util/mp3encoder.o should ideally not be lumped
into the library. Otherwise, you will get unsatisfied symbols when you use
it through the library, but not when you use it directly.

One can certainly argue that it's better to have an explicit "unsatisfied
symbol" error than a silent addition of an unwanted library dependency. If
that's the consensus, then, just add a comment explaining how this works.

Given that discussion, I'm now inclined to believe that the correct solution
is:

a) add a comment in the makefile explaining that .o-libs flags are not
   passed through .a files with a pointer to this discussion.

b) remove the existing dbus.o-libs line which has no effect, to avoid
   monkey-do copy-paste inheritance

--
Cheers,
Christophe de Dinechin (IRC c3d)

Re: [PATCH 08/10] build: Add SPICE_CFLAGS and SPICE_LIBS to relevant files

[Christophe de Dinechin]

On 2020-06-30 at 01:08 CEST, Gerd Hoffmann wrote...
>   Hi,
>
>>  obj-$(CONFIG_PC) += pc.o pc_sysfw.o
>> +pc.o-cflags += $(SPICE_CFLAGS)
>
> Hmm, looks strange.  Why does pc.c need spice?

It includes ui/qemu-spice.h, and I did not check why.
Turns out this is not needed. So I'll remove it.

>
>> +qmp-cmds.o-cflags += $(SPICE_CFLAGS)
>> +hmp-cmds.o-cflags += $(SPICE_CFLAGS)
>
> spice monitor commands need this I guess?

Yes.

>
>> +misc.o-cflags += $(SPICE_CFLAGS)
>
> Why this?

qemu_using_spice and qemu_spice_migrate_info

>
>> +vl.o-cflags := $(GPROF_CFLAGS) $(SDL_CFLAGS) $(SPICE_CFLAGS)
>
> spice init probably

Yes.
.
>
>> -stub-obj-y += vmgenid.o
>>  stub-obj-y += sysbus.o
>>  stub-obj-y += tpm.o
>>  stub-obj-y += trace-control.o
>> +stub-obj-y += vmgenid.o
>
> Huh?

I sent it separately as a trivial patch. Wrong alphabetical order, and where
that change was placed was causing a conflict on each rebase with a spice.c
stub I had at some point "at the right spot" ;-)

>
>> -spice-app.mo-cflags := $(GIO_CFLAGS)
>> -spice-app.mo-libs := $(GIO_LIBS)
>> +spice-app.mo-cflags := $(GIO_CFLAGS) $(SPICE_CFLAGS)
>> +spice-app.mo-libs := $(GIO_LIBS) $(SPICE_LIBS)
>
> Good.
>
>> --- a/util/module.c
>> +++ b/util/module.c
>> @@ -22,11 +22,11 @@
>>  #ifdef CONFIG_MODULE_UPGRADES
>>  #include "qemu-version.h"
>>  #endif
>> -#ifdef CONFIG_TRACE_RECORDER
>>  #include "trace/recorder.h"
>> -#endif
>>
>>
>> +RECORDER(modules, 16, "QEMU load modules");
>> +
>>  typedef struct ModuleEntry
>>  {
>>  void (*init)(void);
>> @@ -85,6 +85,15 @@ void register_dso_module_init(void (*fn)(void), 
>> module_init_type type)
>>  {
>>  ModuleEntry *e;
>>
>> +#ifdef CONFIG_TRACE_RECORDER
>> +static const char *name[] = {
>> +"MIGRATION", "BLOCK", "OPTS", "QOM",
>> +"TRACE", "XEN_BACKEND", "LIBQOS", "FUZZ_TARGET",
>> +"MAX"
>> +};
>> +#endif
>> +record(modules, "Register DSO module init %p type %u %+s",
>> +   fn, type, name[type]);
>>  init_lists();
>>
>>  e = g_malloc0(sizeof(*e));
>
> Unrelated change.
>
> (the recorder stuff should probably integrate with qemu trace support,
> so you can record any trace point qemu has, but that'll be another patch
> series ...)

I sent it separately, and fixed the leftover patch.

>
> take care,
>   Gerd


--
Cheers,
Christophe de Dinechin (IRC c3d)

Re: [PATCH 01/10] modules: Provide macros making it easier to identify module exports

[Christophe de Dinechin]

On 2020-06-29 at 12:13 CEST, Claudio Fontana wrote...
> Hello Christophe,
>
> On 6/26/20 6:42 PM, Christophe de Dinechin wrote:
>> In order to facilitate the move of large chunks of functionality to
>> load modules, it is simpler to create a wrapper with the same name
>> that simply relays the implementation. For efficiency, this is
>> typically done using inline functions in the header for the
>> corresponding functionality. In that case, we rename the actual
>> implementation by appending _implementation to its name. This makes it
>> easier to select which function you want to put a breakpoint on.
>>
>> Signed-off-by: Christophe de Dinechin 
>> ---
>>  include/qemu/module.h | 24 
>>  1 file changed, 24 insertions(+)
>>
>> diff --git a/include/qemu/module.h b/include/qemu/module.h
>> index 011ae1ae76..1922a0293c 100644
>> --- a/include/qemu/module.h
>> +++ b/include/qemu/module.h
>> @@ -39,6 +39,30 @@ static void __attribute__((constructor)) do_qemu_init_ ## 
>> function(void)\
>>  }
>>  #endif
>>
>> +#ifdef CONFIG_MODULES
>> +/* Identify which functions are replaced by a callback stub */
>> +#ifdef MODULE_STUBS
>> +#define MODIFACE(Ret,Name,Args) \
>> +Ret (*Name)Args;\
>> +extern Ret Name##_implementation Args
>> +#else /* !MODULE_STUBS */
>> +#define MODIFACE(Ret,Name,Args) \
>> +extern Ret (*Name)Args; \
>> +extern Ret Name##_implementation Args
>> +#endif /* MODULE_STUBS */
>> +
>> +#define MODIMPL(Ret,Name,Args)  \
>> +static void __attribute__((constructor)) Name##_register(void)  \
>> +{   \
>> +Name = Name##_implementation;   \
>> +}   \
>> +Ret Name##_implementation Args
>> +#else /* !CONFIG_MODULES */
>> +/* When not using a module, such functions are called directly */
>> +#define MODIFACE(Ret,Name,Args) Ret Name Args
>> +#define MODIMPL(Ret,Name,Args)  Ret Name Args
>> +#endif /* CONFIG_MODULES */
>> +
>>  typedef enum {
>>  MODULE_INIT_MIGRATION,
>>  MODULE_INIT_BLOCK,
>>
>
> Just as background, I am interested in all modules-related work, because of 
> my long term plan to have target-specific modules as well:
>
>  https://lists.gnu.org/archive/html/qemu-devel/2020-05/msg04628.html
>
> I am not 100% clear on what is the goal and expected usage of this
> preprocessor code, despite the commit message, maybe you could clarify a
> bit with more verbosity?

Well, so far, the preference seems to be to go through a more verbose
approach with an explicit table of functions.

What the preprocessor code did was:

- If you build without modules, nothing changes, you get a direct call

- If you build with modules:

 + In the DSO, foo is replaced with foo_implementation
 + Elsewhere, foo is replaced with a function pointer also called foo.
 + The implementation adds constructor code that sets foo to point to 
foo_implementation


>
> Additionally if you happen to be interested, maybe you know already or
> could think about what this could mean for target-specific modules, which
> will require some improvements to the modules "subsystem"(?) as well.

So far, I've only integrated Gerd's workaround for target-specific
modules. Some additional mechanics is needed to name target-specific
modules, e.g. put them in some target directory.

>
> In my experimentation I didn't have to do this preprocessor work, instead
> I had to fine tune a bit the makefile support (rules.mak and makefiles) to
> be able to accomodate for (even large) modules in target/ as well.

It's probably because the modules you were dealing with already had the
required indirection and module_init calls, i.e. they were only invoked
using QOM already.

The mechanism I was proposing is to quickly add the indirection for
qemu functionality that does not have such indirect calls yet.

The consensus so far seems to be that the syntax I proposed is not nice, and
that it's better to make it more explicit through a table and indirect
calls, even if that means changing the call sites.

>
> Thanks!
>
> CLaudio


--
Cheers,
Christophe de Dinechin (IRC c3d)

Re: [PATCH 01/10] modules: Provide macros making it easier to identify module exports

[Christophe de Dinechin]

On 2020-06-30 at 11:38 CEST, Michael S. Tsirkin wrote...
> On Fri, Jun 26, 2020 at 06:42:58PM +0200, Christophe de Dinechin wrote:
>> In order to facilitate the move of large chunks of functionality to
>> load modules, it is simpler to create a wrapper with the same name
>> that simply relays the implementation. For efficiency, this is
>> typically done using inline functions in the header for the
>> corresponding functionality. In that case, we rename the actual
>> implementation by appending _implementation to its name. This makes it
>> easier to select which function you want to put a breakpoint on.
>>
>> Signed-off-by: Christophe de Dinechin 
>> ---
>>  include/qemu/module.h | 24 
>>  1 file changed, 24 insertions(+)
>>
>> diff --git a/include/qemu/module.h b/include/qemu/module.h
>> index 011ae1ae76..1922a0293c 100644
>> --- a/include/qemu/module.h
>> +++ b/include/qemu/module.h
>> @@ -39,6 +39,30 @@ static void __attribute__((constructor)) do_qemu_init_ ## 
>> function(void)\
>>  }
>>  #endif
>>
>> +#ifdef CONFIG_MODULES
>> +/* Identify which functions are replaced by a callback stub */
>> +#ifdef MODULE_STUBS
>> +#define MODIFACE(Ret,Name,Args) \
>> +Ret (*Name)Args;\
>> +extern Ret Name##_implementation Args
>> +#else /* !MODULE_STUBS */
>> +#define MODIFACE(Ret,Name,Args) \
>> +extern Ret (*Name)Args; \
>> +extern Ret Name##_implementation Args
>> +#endif /* MODULE_STUBS */
>> +
>> +#define MODIMPL(Ret,Name,Args)  \
>> +static void __attribute__((constructor)) Name##_register(void)  \
>> +{   \
>> +Name = Name##_implementation;   \
>> +}   \
>> +Ret Name##_implementation Args
>> +#else /* !CONFIG_MODULES */
>> +/* When not using a module, such functions are called directly */
>> +#define MODIFACE(Ret,Name,Args) Ret Name Args
>> +#define MODIMPL(Ret,Name,Args)  Ret Name Args
>> +#endif /* CONFIG_MODULES */
>> +
>>  typedef enum {
>>  MODULE_INIT_MIGRATION,
>>  MODULE_INIT_BLOCK,
>
> Hmm that's quite a bit of overhead for each call across modules.

Do you mean runtime overhead, i.e. the difference between
foo(x) and (*foo)(x)?


> Can't code patching be used somehow?

I've considered it, but this seems like a bit of a hammer for half a dozen
calls that are mostly init, so not performance sensitive.

In the old times, ld.so used to do that for you. All you had to do was to
mark one symbol as weak. Apparently for security reasons, that feature was
dropped several years ago. At least, that put call patching where it
belonged, i.e. in the loader.

>
>
>> --
>> 2.26.2


--
Cheers,
Christophe de Dinechin (IRC c3d)

Re: [PATCH v2 2/3] trace: Add support for recorder back-end

[Christophe de Dinechin]

On 2020-06-30 at 11:05 CEST, Dr. David Alan Gilbert wrote...
> * Christophe de Dinechin (dinec...@redhat.com) wrote:
>> The recorder library provides support for low-cost continuous
>> recording of events, which can then be replayed. This makes it
>> possible to collect data "after the fact",for example to show the
>> events that led to a crash.
>>
>> Recorder support in qemu is implemented using the existing tracing
>> interface. In addition, it is possible to individually enable
>> recorders that are not traces, although this is probably not
>> recommended.
>>
>> HMP COMMAND:
>> The 'recorder' hmp command has been added, which supports two
>> sub-commands:
>> - recorder dump: Dump the current state of the recorder. You can
>   
> is that intended?

No. I think the intent was to indicate that you could pass the name of a
recorder to dump as an arg. I'll fix.

>
>> - recorder trace: Set traces using the recorder_trace_set() syntax.
>>   You can use "recorder trace help" to list all available recorders.
>>
>> Signed-off-by: Christophe de Dinechin 
>> ---
>>  configure |  5 +++
>>  hmp-commands.hx   | 19 --
>>  monitor/misc.c| 27 ++
>>  scripts/tracetool/backend/recorder.py | 51 +++
>>  trace/Makefile.objs   |  2 ++
>>  trace/control.c   |  7 
>>  trace/recorder.c  | 22 
>>  trace/recorder.h  | 34 ++
>>  util/module.c |  8 +
>>  9 files changed, 173 insertions(+), 2 deletions(-)
>>  create mode 100644 scripts/tracetool/backend/recorder.py
>>  create mode 100644 trace/recorder.c
>>  create mode 100644 trace/recorder.h
>>
>> diff --git a/configure b/configure
>> index ae8737d5a2..130630b98f 100755
>> --- a/configure

>> +++ b/configure
>> @@ -7702,6 +7702,11 @@ fi
>>  if have_backend "log"; then
>>echo "CONFIG_TRACE_LOG=y" >> $config_host_mak
>>  fi
>> +if have_backend "recorder"; then
>> +  echo "CONFIG_TRACE_RECORDER=y" >> $config_host_mak
>> +  # This is a bit brutal, but there is currently a bug in the makefiles
>> +  LIBS="$LIBS -lrecorder"
>> +fi
>>  if have_backend "ust"; then
>>echo "CONFIG_TRACE_UST=y" >> $config_host_mak
>>  fi
>> diff --git a/hmp-commands.hx b/hmp-commands.hx
>> index 60f395c276..565f518d4b 100644
>> --- a/hmp-commands.hx
>> +++ b/hmp-commands.hx
>> @@ -297,6 +297,22 @@ ERST
>>  .cmd= hmp_trace_file,
>>  },
>>
>> +SRST
>> +``trace-file on|off|flush``
>> +  Open, close, or flush the trace file.  If no argument is given, the
>> +  status of the trace file is displayed.
>> +ERST
>> +#endif
>> +
>> +#if defined(CONFIG_TRACE_RECORDER)
>> +{
>> +.name   = "recorder",
>> +.args_type  = "op:s?,arg:F?",
>
> Having 'arg' as a filename is a bit odd; using op/arg is very generic
> for adding extra commands; but it suddenly becomes less generic if
> arg is always a filename.
>
>> +.params = "trace|dump [arg]",
>> +.help   = "trace selected recorders or print recorder dump",
>> +.cmd= hmp_recorder,
>> +},
>> +
>>  SRST
>>  ``trace-file on|off|flush``
>>Open, close, or flush the trace file.  If no argument is given, the
>
> I think this SRST chunk is the one that needs updating for recorder.
> (The diff has made a bit of a mess, but I think you've copy pasted the
> trace-file chunk, but forgotten to update the SRST section).

Indeed. I had forgotten to get back to the .hx file and add the documentation.

>
>> @@ -1120,7 +1136,7 @@ ERST
>>
>>  SRST
>>  ``dump-guest-memory [-p]`` *filename* *begin* *length*
>> -  \
>> +  \
>>  ``dump-guest-memory [-z|-l|-s|-w]`` *filename*
>>Dump guest memory to *protocol*. The file can be processed with crash or
>>gdb. Without ``-z|-l|-s|-w``, the dump format is ELF.
>> @@ -1828,4 +1844,3 @@ ERST
>>  .sub_table  = hmp_info_cmds,
>>  .flags  = "p",
>>  },
>> -
>> diff --git a/monitor/misc.c b/monitor/misc.c
>> index 89bb970b00..0094b1860f 100644
>> --- a/monitor/misc.c
&

Re: [PATCH 09/10] spice: Put spice functions in a separate load module

[Christophe de Dinechin]

On 2020-06-30 at 01:00 CEST, Gerd Hoffmann wrote...
>   Hi,
>
>> > If so the more normal approach would be to have a struct defining
>> > a set of callbacks, that can be registered. Or if there's a natural
>> > fit with QOM, then a QOM interface that can then have a QOM object
>> > impl registered as a singleton.
>>
>> That was my second attempt (after the weak symbols). I cleaned it up a bit
>> and put it here: https://github.com/c3d/qemu/commits/spice-vtable.
>
> I think this is the direction we should take.
>
>> What made me switch to the approach in this series is the following
>> considerations:
>>
>> - A vtable is useful if there can be multiple values for a method, e.g. to
>>   implement inheritance, or if you have multiple instances. This is not the
>>   case here.
>
> Well, we'll have two.  The normal functions.  And the stubs.
>
> The stubs are inline functions right now, in include/ui/qemu-spice.h, in
> the !CONFIG_SPICE section.  We can turn them into normal functions, move
> them to some C file.  Let QemuSpiceOpts function pointers point to the
> stubs initially.  When spice initializes (no matter whenever modular or
> not) it'll set QemuSpiceOpts to the normal implementation.

Good idea. I'll do that in the next round.

>
> That way we'll also remove some spice #ifdefs as part of the spice
> modularization effort.
>
> Things like the "using_spice" variable which don't depend on the spice
> shared libraries can also be moved to the new C file with the spice
> stubs.

OK.

>
> I don't think we need to hide QemuSpiceOpts with inline functions like
> qemu_spice_migrate_info().  I would simply use ...
>
>   struct QemuSpiceOps {
>   [ ... ]
>   int (*migrate_info)(...);
>   [ ... ]
>   } qemu_spice;
>
> ... then change the ...
>
>   qemu_spice_migrate_info(...)
>
> .. callsites into ...
>
>   qemu_spice.migrate_info(...)
>

OK.

>> - Overloading QOM for that purpose looked more confusing than anything else.
>>   It looked like I was mixing unrelated concepts. Maybe that's just me.
>
> Hmm?  Not sure what you mean.  There is no need for QOM here (and I
> can't see anything like that in your spice-vtable branch either).

I was responding to Daniels's earlier comment:

> Or if there's a natural fit with QOM, then a QOM interface that can then
> have a QOM object impl registered as a singleton.

>
>> - The required change with a vtable ends up being more extensive. Instead of
>>   changing a single line to put an entry point in a DSO, you need to create
>>   the vtable, add functions to it, add a register function, etc. I was
>>   looking for an easier and more scalable way.
>
> IMHO it isn't too much overhead, and I find the code is more readable
> that way.

There is certainly very little overhead in that case, since none of the
functions is performance critical.

>
>> - In particular, with a vtable, you cannot take advantage of the syntactic
>>   trick I used here, which is that foo(x) is a shortcut for (*foo)(x).
>>   So for a vtable, you need to manually write wrappers.
>
> See above, I don't think we need wrappers.

Well, so far that's two for two for the vtable approach. So unless someone
else agrees with my arguments for pointer patching, that will be my next
iteration of that series :-)

>
> take care,
>   Gerd


--
Cheers,
Christophe de Dinechin (IRC c3d)

Re: [PATCH 10/10] REMOVE: Instrumentation to show the module functions being replaced

[Christophe de Dinechin]

On 2020-06-26 at 19:29 CEST, Daniel P. Berrangé wrote...
> On Fri, Jun 26, 2020 at 06:43:07PM +0200, Christophe de Dinechin wrote:
>> Signed-off-by: Christophe de Dinechin 
>> ---
>>  include/qemu/module.h | 4 
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/include/qemu/module.h b/include/qemu/module.h
>> index 1922a0293c..8d6e10ba81 100644
>> --- a/include/qemu/module.h
>> +++ b/include/qemu/module.h
>> @@ -14,10 +14,13 @@
>>  #ifndef QEMU_MODULE_H
>>  #define QEMU_MODULE_H
>>
>> +#include "trace/recorder.h"
>>
>>  #define DSO_STAMP_FUN glue(qemu_stamp, CONFIG_STAMP)
>>  #define DSO_STAMP_FUN_STR stringify(DSO_STAMP_FUN)
>>
>> +RECORDER_DECLARE(modules);
>> +
>>  #ifdef BUILD_DSO
>>  void DSO_STAMP_FUN(void);
>>  /* This is a dummy symbol to identify a loaded DSO as a QEMU module, so we 
>> can
>> @@ -55,6 +58,7 @@ static void __attribute__((constructor)) do_qemu_init_ ## 
>> function(void)\
>>  static void __attribute__((constructor)) Name##_register(void)  \
>>  {   \
>>  Name = Name##_implementation;   \
>> +record(modules, "Setting " #Name " to %p", Name);   \
>>  }   \
>>  Ret Name##_implementation Args
>>  #else /* !CONFIG_MODULES */
>
> Contrary to the commit $SUBJECT, I think you should keep this, not remove
> it. It should use QEMU's trace backend though.

OK. Will add a trace backend version in next iteration.

>
> Regards,
> Daniel


--
Cheers,
Christophe de Dinechin (IRC c3d)

Re: [PATCH 09/10] spice: Put spice functions in a separate load module

[Christophe de Dinechin]

On 2020-06-26 at 19:35 CEST, Daniel P. Berrangé wrote...
> On Fri, Jun 26, 2020 at 06:43:06PM +0200, Christophe de Dinechin wrote:
>> Use the MODIFACE and MODIMPL macros to to redirect the highest-level
>> qemu_spice functions into the spice-app.so load module when SPICE is
>> compiled as a module.
>>
>> With these changes, the following shared libraries are no longer
>> necessary in the top-level qemu binary:
>>
>>  libspice-server.so.1 => /lib64/libspice-server.so.1 (HEX)
>>  libopus.so.0 => /lib64/libopus.so.0 (HEX)
>>  liblz4.so.1 => /lib64/liblz4.so.1 (HEX)
>>  libgstapp-1.0.so.0 => /lib64/libgstapp-1.0.so.0 (HEX)
>>  libgstvideo-1.0.so.0 => /lib64/libgstvideo-1.0.so.0 (HEX)
>>  libgstbase-1.0.so.0 => /lib64/libgstbase-1.0.so.0 (HEX)
>>  libgstreamer-1.0.so.0 => /lib64/libgstreamer-1.0.so.0 (HEX)
>>  libssl.so.1.1 => /lib64/libssl.so.1.1 (HEX)
>>  liborc-0.4.so.0 => /lib64/liborc-0.4.so.0 (HEX)
>>
>> Signed-off-by: Christophe de Dinechin 
>> ---
>>  include/ui/qemu-spice.h | 24 +++-
>>  monitor/hmp-cmds.c  |  6 ++
>>  softmmu/vl.c|  1 +
>>  ui/spice-core.c | 31 +--
>>  ui/spice-display.c  |  2 +-
>>  5 files changed, 44 insertions(+), 20 deletions(-)
>>
>> diff --git a/include/ui/qemu-spice.h b/include/ui/qemu-spice.h
>> index 8c23dfe717..0f7e139da5 100644
>> --- a/include/ui/qemu-spice.h
>> +++ b/include/ui/qemu-spice.h
>> @@ -24,22 +24,28 @@
>>
>>  #include 
>>  #include "qemu/config-file.h"
>> +#include "qemu/module.h"
>>
>> -extern int using_spice;
>> +#define using_spice (qemu_is_using_spice())
>>
>> -void qemu_spice_init(void);
>> +MODIFACE(bool, qemu_is_using_spice,(void));
>> +MODIFACE(void, qemu_start_using_spice, (void));
>> +MODIFACE(void, qemu_spice_init, (void));
>>  void qemu_spice_input_init(void);
>>  void qemu_spice_audio_init(void);
>> -void qemu_spice_display_init(void);
>> -int qemu_spice_display_add_client(int csock, int skipauth, int tls);
>> +MODIFACE(void, qemu_spice_display_init, (void));
>> +MODIFACE(int, qemu_spice_display_add_client, (int csock, int skipauth, int 
>> tls));
>>  int qemu_spice_add_interface(SpiceBaseInstance *sin);
>>  bool qemu_spice_have_display_interface(QemuConsole *con);
>>  int qemu_spice_add_display_interface(QXLInstance *qxlin, QemuConsole *con);
>> -int qemu_spice_set_passwd(const char *passwd,
>> -  bool fail_if_connected, bool 
>> disconnect_if_connected);
>> -int qemu_spice_set_pw_expire(time_t expires);
>> -int qemu_spice_migrate_info(const char *hostname, int port, int tls_port,
>> -const char *subject);
>> +MODIFACE(int, qemu_spice_set_passwd, (const char *passwd,
>> +  bool fail_if_connected,
>> +  bool disconnect_if_connected));
>> +MODIFACE(int, qemu_spice_set_pw_expire,(time_t expires));
>> +MODIFACE(int, qemu_spice_migrate_info,(const char *hostname,
>> +   int port, int tls_port,
>> +   const char *subject));
>> +MODIFACE(struct SpiceInfo *,qemu_spice_query, (Error **errp));
>
> This macro usage looks kind of unpleasant and its hard to understand
> just what is going on, especially why some methods are changed but
> others are not.

The functions that are changed are the module interface between qemu and the
DSO. For example, qemu_spice_init is called from vl.c, i.e. the main binary,
but qemu_spice_audio_init is called from ui/spice-core.c and
ui/spice-input.c, which are both in the DSO after the commit.

The existing function-based interface in qemu-spice.h was not really
carefully designed for modularization, so this list was determined by
following the initialization path. It may not be the smallest possible cut.
It may be neat to add a patch to reorder functions based on whether they are
inside the DSO or exported from it.

As for the macro syntax, I see it as somewhat transient. I wanted to propose
a working and scalable mechanism before adding some nice syntactic sugar
tooling to it. I expect the syntax to turn into something like:

MODIFACE void  qemu_spice_display_init (void);
MODIIMPL void  qemu_spice_display_init (void) { ... }

But it feels a bit too early to do that. I prefer to experiment with a
simple macro for now.

>
> IIUC, the goal is to turn all these into weak symbols so they don't
> need to be resolved immediately at startup, and inste

[PATCH] trivial: Respect alphabetical order of .o files in Makefile.objs

[Christophe de Dinechin]

The vmgenid.o is the only file that is not in alphabetical order.

Signed-off-by: Christophe de Dinechin 
---
 stubs/Makefile.objs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
index f32b9e47a3..1df8bb3814 100644
--- a/stubs/Makefile.objs
+++ b/stubs/Makefile.objs
@@ -19,10 +19,10 @@ stub-obj-y += replay.o
 stub-obj-y += runstate-check.o
 stub-obj-$(CONFIG_SOFTMMU) += semihost.o
 stub-obj-y += set-fd-handler.o
-stub-obj-y += vmgenid.o
 stub-obj-y += sysbus.o
 stub-obj-y += tpm.o
 stub-obj-y += trace-control.o
+stub-obj-y += vmgenid.o
 stub-obj-y += vmstate.o
 stub-obj-$(CONFIG_SOFTMMU) += win32-kbd-hook.o
 
-- 
2.26.2

Re: [PATCH 08/10] build: Add SPICE_CFLAGS and SPICE_LIBS to relevant files

[Christophe de Dinechin]

On 2020-06-26 at 19:26 CEST, Daniel P. Berrangé wrote...
> On Fri, Jun 26, 2020 at 06:43:05PM +0200, Christophe de Dinechin wrote:
>> Instead of adding the spice build flags to the top-level build
>> options, add them where they are necessary. This is a step to move the
>> burden of linking with spice libraries away from the top-level qemu.
>>
>> Signed-off-by: Christophe de Dinechin 
>> ---
>>  configure|  4 ++--
>>  hw/display/Makefile.objs |  1 +
>>  hw/i386/Makefile.objs|  1 +
>>  monitor/Makefile.objs|  3 +++
>>  softmmu/Makefile.objs|  2 +-
>>  stubs/Makefile.objs  |  2 +-
>>  ui/Makefile.objs |  4 ++--
>>  util/module.c| 13 +++--
>>  8 files changed, 22 insertions(+), 8 deletions(-)
>
>> diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
>> index f32b9e47a3..1df8bb3814 100644
>> --- a/stubs/Makefile.objs
>> +++ b/stubs/Makefile.objs
>> @@ -19,10 +19,10 @@ stub-obj-y += replay.o
>>  stub-obj-y += runstate-check.o
>>  stub-obj-$(CONFIG_SOFTMMU) += semihost.o
>>  stub-obj-y += set-fd-handler.o
>> -stub-obj-y += vmgenid.o
>>  stub-obj-y += sysbus.o
>>  stub-obj-y += tpm.o
>>  stub-obj-y += trace-control.o
>> +stub-obj-y += vmgenid.o
>>  stub-obj-y += vmstate.o
>>  stub-obj-$(CONFIG_SOFTMMU) += win32-kbd-hook.o
>>
>
> This looks unrelated to this series.

I'll send a separate trivial patch to fix the alphabetical ordering.
I used to have a spice.c stub here, which conflicted every time. This is how
I noticed the alphabetical order was not respected here.

>
>
>
>> diff --git a/util/module.c b/util/module.c
>> index 2fa93561fe..29b4806520 100644
>> --- a/util/module.c
>> +++ b/util/module.c
>> @@ -22,11 +22,11 @@
>>  #ifdef CONFIG_MODULE_UPGRADES
>>  #include "qemu-version.h"
>>  #endif
>> -#ifdef CONFIG_TRACE_RECORDER
>>  #include "trace/recorder.h"
>> -#endif
>>
>>
>> +RECORDER(modules, 16, "QEMU load modules");
>> +
>>  typedef struct ModuleEntry
>>  {
>>  void (*init)(void);
>> @@ -85,6 +85,15 @@ void register_dso_module_init(void (*fn)(void), 
>> module_init_type type)
>>  {
>>  ModuleEntry *e;
>>
>> +#ifdef CONFIG_TRACE_RECORDER
>> +static const char *name[] = {
>> +"MIGRATION", "BLOCK", "OPTS", "QOM",
>> +    "TRACE", "XEN_BACKEND", "LIBQOS", "FUZZ_TARGET",
>> +"MAX"
>> +};
>> +#endif
>> +record(modules, "Register DSO module init %p type %u %+s",
>> +   fn, type, name[type]);
>>  init_lists();
>
> This looks unrelated too, but in general debugging should go via QEMU's
> standard trace backends.
>

Yes. I apparently botched a fixup. That was supposed to be a private patch
for my own use.



--
Cheers,
Christophe de Dinechin (IRC c3d)

Re: [PATCH 04/10] spice: Move all the spice-related code in spice-app.so

[Christophe de Dinechin]

On 2020-06-26 at 19:20 CEST, Daniel P. Berrangé wrote...
> On Fri, Jun 26, 2020 at 06:43:01PM +0200, Christophe de Dinechin wrote:
>> If we want to build spice as a separately loadable module, we need to
>> put all the spice code in one loadable module, because the build
>> system does not know how to deal with dependencies yet.
>>
>> Signed-off-by: Christophe de Dinechin 
>> ---
>>  audio/Makefile.objs   | 2 +-
>>  chardev/Makefile.objs | 3 +--
>>  ui/Makefile.objs  | 8 
>>  3 files changed, 6 insertions(+), 7 deletions(-)
>>
>> diff --git a/audio/Makefile.objs b/audio/Makefile.objs
>> index b4a4c11f31..298c895ff5 100644
>> --- a/audio/Makefile.objs
>> +++ b/audio/Makefile.objs
>> @@ -1,5 +1,5 @@
>>  common-obj-y = audio.o audio_legacy.o noaudio.o wavaudio.o mixeng.o
>> -common-obj-$(CONFIG_SPICE) += spiceaudio.o
>> +spice-app.mo-objs += ../audio/spiceaudio.o
>
> Explicitly showing paths in the variables doesn't look right. The
> make recipes are supposed to automatically expand bare file names
> to add the right path. This is usually dealt with by a call to
> the "unnest-vars" function.

I agree. I mentioned this in the cover letter:

> - Adding various non-UI files into spice-app.so, which required a
>   couple of ../pwd/foo.o references in the makefile. Not very nice,
>   but I did not want to spend too much time fixing the makefiles.

I did not find an elegant way to integrate a non-UI file into a .mo that is
declared in the ui section.

I considered various solutions:

a) Having separate load modules for different source directories.
   This exposes details of the build system into the generated libs.

b) Moving the source
   This breaks the logical organization of the sources

c) Manually managing this specific .o one level up
   This seems even harder to read / understand

So I thought I would dedicate a bit more time to add that capability to the
unnest-vars function. I did not want to defer review for that, and vaguely
hoped that there was an existing correct way to do it that someone more
experienced in the build system could point me to.

>
>>  common-obj-$(CONFIG_AUDIO_COREAUDIO) += coreaudio.o
>>  common-obj-$(CONFIG_AUDIO_DSOUND) += dsoundaudio.o
>>  common-obj-$(CONFIG_AUDIO_WIN_INT) += audio_win_int.o
>> diff --git a/chardev/Makefile.objs b/chardev/Makefile.objs
>> index fc9910d4f2..955fac0cf9 100644
>> --- a/chardev/Makefile.objs
>> +++ b/chardev/Makefile.objs
>> @@ -22,5 +22,4 @@ common-obj-$(CONFIG_BRLAPI) += baum.o
>>  baum.o-cflags := $(SDL_CFLAGS)
>>  baum.o-libs := $(BRLAPI_LIBS)
>>
>> -common-obj-$(CONFIG_SPICE) += spice.mo
>> -spice.mo-objs := spice.o
>> +spice-app.mo-objs += ../chardev/spice.o
>> diff --git a/ui/Makefile.objs b/ui/Makefile.objs
>> index 504b196479..1ab515e23d 100644
>> --- a/ui/Makefile.objs
>> +++ b/ui/Makefile.objs
>> @@ -11,7 +11,6 @@ common-obj-y += keymaps.o console.o cursor.o qemu-pixman.o
>>  common-obj-y += input.o input-keymap.o input-legacy.o kbd-state.o
>>  common-obj-y += input-barrier.o
>>  common-obj-$(CONFIG_LINUX) += input-linux.o
>> -common-obj-$(CONFIG_SPICE) += spice-core.o spice-input.o spice-display.o
>>  common-obj-$(CONFIG_COCOA) += cocoa.o
>>  common-obj-$(CONFIG_VNC) += $(vnc-obj-y)
>>  common-obj-$(call lnot,$(CONFIG_VNC)) += vnc-stubs.o
>> @@ -53,10 +52,11 @@ curses.mo-objs := curses.o
>>  curses.mo-cflags := $(CURSES_CFLAGS) $(ICONV_CFLAGS)
>>  curses.mo-libs := $(CURSES_LIBS) $(ICONV_LIBS)
>>
>> -ifeq ($(CONFIG_GIO)$(CONFIG_SPICE),yy)
>> -common-obj-$(if $(CONFIG_MODULES),m,y) += spice-app.mo
>> +common-obj-$(CONFIG_SPICE) += spice-app.mo
>> +spice-app.mo-objs += spice-core.o spice-input.o spice-display.o
>> +ifeq ($(CONFIG_GIO)$(CONFIG_SPICE),ym)
>> +spice-app.mo-objs += spice-app.o
>>  endif
>> -spice-app.mo-objs := spice-app.o
>>  spice-app.mo-cflags := $(GIO_CFLAGS)
>>  spice-app.mo-libs := $(GIO_LIBS)
>>
>> --
>> 2.26.2
>>
>>
>
> Regards,
> Daniel


--
Cheers,
Christophe de Dinechin (IRC c3d)

[PATCH 09/10] spice: Put spice functions in a separate load module

[Christophe de Dinechin]

Use the MODIFACE and MODIMPL macros to to redirect the highest-level
qemu_spice functions into the spice-app.so load module when SPICE is
compiled as a module.

With these changes, the following shared libraries are no longer
necessary in the top-level qemu binary:

libspice-server.so.1 => /lib64/libspice-server.so.1 (HEX)
libopus.so.0 => /lib64/libopus.so.0 (HEX)
liblz4.so.1 => /lib64/liblz4.so.1 (HEX)
libgstapp-1.0.so.0 => /lib64/libgstapp-1.0.so.0 (HEX)
libgstvideo-1.0.so.0 => /lib64/libgstvideo-1.0.so.0 (HEX)
libgstbase-1.0.so.0 => /lib64/libgstbase-1.0.so.0 (HEX)
libgstreamer-1.0.so.0 => /lib64/libgstreamer-1.0.so.0 (HEX)
libssl.so.1.1 => /lib64/libssl.so.1.1 (HEX)
liborc-0.4.so.0 => /lib64/liborc-0.4.so.0 (HEX)

Signed-off-by: Christophe de Dinechin 
---
 include/ui/qemu-spice.h | 24 +++-
 monitor/hmp-cmds.c  |  6 ++
 softmmu/vl.c|  1 +
 ui/spice-core.c | 31 +--
 ui/spice-display.c  |  2 +-
 5 files changed, 44 insertions(+), 20 deletions(-)

diff --git a/include/ui/qemu-spice.h b/include/ui/qemu-spice.h
index 8c23dfe717..0f7e139da5 100644
--- a/include/ui/qemu-spice.h
+++ b/include/ui/qemu-spice.h
@@ -24,22 +24,28 @@
 
 #include 
 #include "qemu/config-file.h"
+#include "qemu/module.h"
 
-extern int using_spice;
+#define using_spice (qemu_is_using_spice())
 
-void qemu_spice_init(void);
+MODIFACE(bool, qemu_is_using_spice,(void));
+MODIFACE(void, qemu_start_using_spice, (void));
+MODIFACE(void, qemu_spice_init, (void));
 void qemu_spice_input_init(void);
 void qemu_spice_audio_init(void);
-void qemu_spice_display_init(void);
-int qemu_spice_display_add_client(int csock, int skipauth, int tls);
+MODIFACE(void, qemu_spice_display_init, (void));
+MODIFACE(int, qemu_spice_display_add_client, (int csock, int skipauth, int 
tls));
 int qemu_spice_add_interface(SpiceBaseInstance *sin);
 bool qemu_spice_have_display_interface(QemuConsole *con);
 int qemu_spice_add_display_interface(QXLInstance *qxlin, QemuConsole *con);
-int qemu_spice_set_passwd(const char *passwd,
-  bool fail_if_connected, bool 
disconnect_if_connected);
-int qemu_spice_set_pw_expire(time_t expires);
-int qemu_spice_migrate_info(const char *hostname, int port, int tls_port,
-const char *subject);
+MODIFACE(int, qemu_spice_set_passwd, (const char *passwd,
+  bool fail_if_connected,
+  bool disconnect_if_connected));
+MODIFACE(int, qemu_spice_set_pw_expire,(time_t expires));
+MODIFACE(int, qemu_spice_migrate_info,(const char *hostname,
+   int port, int tls_port,
+   const char *subject));
+MODIFACE(struct SpiceInfo *,qemu_spice_query, (Error **errp));
 
 #if !defined(SPICE_SERVER_VERSION) || (SPICE_SERVER_VERSION < 0xc06)
 #define SPICE_NEEDS_SET_MM_TIME 1
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index 2b0b58a336..6bd9c52658 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -56,6 +56,7 @@
 #include "migration/misc.h"
 
 #ifdef CONFIG_SPICE
+#include "ui/qemu-spice.h"
 #include 
 #endif
 
@@ -573,6 +574,11 @@ void hmp_info_vnc(Monitor *mon, const QDict *qdict)
 #endif
 
 #ifdef CONFIG_SPICE
+SpiceInfo *qmp_query_spice(Error **errp)
+{
+return qemu_spice_query(errp);
+}
+
 void hmp_info_spice(Monitor *mon, const QDict *qdict)
 {
 SpiceChannelList *chan;
diff --git a/softmmu/vl.c b/softmmu/vl.c
index 3e15ee2435..c94b4fa49b 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -22,6 +22,7 @@
  * THE SOFTWARE.
  */
 
+#define MODULE_STUBS
 #include "qemu/osdep.h"
 #include "qemu-common.h"
 #include "qemu/units.h"
diff --git a/ui/spice-core.c b/ui/spice-core.c
index ecc2ec2c55..dbc1886b77 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -48,7 +48,7 @@ static time_t auth_expires = TIME_MAX;
 static int spice_migration_completed;
 static int spice_display_is_running;
 static int spice_have_target_host;
-int using_spice = 0;
+static int is_using_spice = 0;
 
 static QemuThread me;
 
@@ -503,7 +503,7 @@ static QemuOptsList qemu_spice_opts = {
 },
 };
 
-SpiceInfo *qmp_query_spice(Error **errp)
+MODIMPL(SpiceInfo *,qemu_spice_query,(Error **errp))
 {
 QemuOpts *opts = QTAILQ_FIRST(_spice_opts.head);
 int port, tls_port;
@@ -579,8 +579,9 @@ static void migration_state_notifier(Notifier *notifier, 
void *data)
 }
 }
 
-int qemu_spice_migrate_info(const char *hostname, int port, int tls_port,
-const char *subject)
+MODIMPL(int, qemu_spice_migrate_info, (const char *hostname,
+   int port, int tls_port,
+   const char *subject))
 {
 int ret;
 
@@ -634,7 +635,17 

[PATCH 10/10] REMOVE: Instrumentation to show the module functions being replaced

[Christophe de Dinechin]

Signed-off-by: Christophe de Dinechin 
---
 include/qemu/module.h | 4 
 1 file changed, 4 insertions(+)

diff --git a/include/qemu/module.h b/include/qemu/module.h
index 1922a0293c..8d6e10ba81 100644
--- a/include/qemu/module.h
+++ b/include/qemu/module.h
@@ -14,10 +14,13 @@
 #ifndef QEMU_MODULE_H
 #define QEMU_MODULE_H
 
+#include "trace/recorder.h"
 
 #define DSO_STAMP_FUN glue(qemu_stamp, CONFIG_STAMP)
 #define DSO_STAMP_FUN_STR stringify(DSO_STAMP_FUN)
 
+RECORDER_DECLARE(modules);
+
 #ifdef BUILD_DSO
 void DSO_STAMP_FUN(void);
 /* This is a dummy symbol to identify a loaded DSO as a QEMU module, so we can
@@ -55,6 +58,7 @@ static void __attribute__((constructor)) do_qemu_init_ ## 
function(void)\
 static void __attribute__((constructor)) Name##_register(void)  \
 {   \
 Name = Name##_implementation;   \
+record(modules, "Setting " #Name " to %p", Name);   \
 }   \
 Ret Name##_implementation Args
 #else /* !CONFIG_MODULES */
-- 
2.26.2

[PATCH 06/10] trivial: Remove extra trailing whitespace

[Christophe de Dinechin]

Signed-off-by: Christophe de Dinechin 
---
 hw/display/qxl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index d5627119ec..28caf878cd 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -51,7 +51,7 @@
 #undef ALIGN
 #define ALIGN(a, b) (((a) + ((b) - 1)) & ~((b) - 1))
 
-#define PIXEL_SIZE 0.2936875 //1280x1024 is 14.8" x 11.9" 
+#define PIXEL_SIZE 0.2936875 //1280x1024 is 14.8" x 11.9"
 
 #define QXL_MODE(_x, _y, _b, _o)  \
 {   .x_res = _x,  \
-- 
2.26.2

[PATCH 08/10] build: Add SPICE_CFLAGS and SPICE_LIBS to relevant files

[Christophe de Dinechin]

Instead of adding the spice build flags to the top-level build
options, add them where they are necessary. This is a step to move the
burden of linking with spice libraries away from the top-level qemu.

Signed-off-by: Christophe de Dinechin 
---
 configure|  4 ++--
 hw/display/Makefile.objs |  1 +
 hw/i386/Makefile.objs|  1 +
 monitor/Makefile.objs|  3 +++
 softmmu/Makefile.objs|  2 +-
 stubs/Makefile.objs  |  2 +-
 ui/Makefile.objs |  4 ++--
 util/module.c| 13 +++--
 8 files changed, 22 insertions(+), 8 deletions(-)

diff --git a/configure b/configure
index 2de1715800..ac83aea242 100755
--- a/configure
+++ b/configure
@@ -5148,8 +5148,6 @@ EOF
  $pkg_config --atleast-version=0.12.3 spice-protocol && \
  compile_prog "$spice_cflags" "$spice_libs" ; then
 spice="yes"
-libs_softmmu="$libs_softmmu $spice_libs"
-QEMU_CFLAGS="$QEMU_CFLAGS $spice_cflags"
 spice_protocol_version=$($pkg_config --modversion spice-protocol)
 spice_server_version=$($pkg_config --modversion spice-server)
   else
@@ -7472,6 +7470,8 @@ fi
 
 if test "$spice" = "yes" ; then
   echo "CONFIG_SPICE=m" >> $config_host_mak
+  echo "SPICE_CFLAGS=$spice_cflags" >> $config_host_mak
+  echo "SPICE_LIBS=$spice_libs" >> $config_host_mak
 fi
 
 if test "$smartcard" = "yes" ; then
diff --git a/hw/display/Makefile.objs b/hw/display/Makefile.objs
index f51411619b..273a956d96 100644
--- a/hw/display/Makefile.objs
+++ b/hw/display/Makefile.objs
@@ -46,6 +46,7 @@ obj-$(CONFIG_VGA) += vga.o
 
 common-obj-$(CONFIG_QXL:y=m) += qxl.mo
 qxl.mo-objs := qxl.o qxl-logger.o qxl-render.o
+qxl.mo-cflags += $(SPICE_CFLAGS)
 
 obj-$(CONFIG_VIRTIO_GPU) += virtio-gpu-base.o virtio-gpu.o virtio-gpu-3d.o
 obj-$(CONFIG_VHOST_USER_GPU) += vhost-user-gpu.o
diff --git a/hw/i386/Makefile.objs b/hw/i386/Makefile.objs
index 6abc74551a..bf9856be2a 100644
--- a/hw/i386/Makefile.objs
+++ b/hw/i386/Makefile.objs
@@ -2,6 +2,7 @@ obj-$(CONFIG_KVM) += kvm/
 obj-y += e820_memory_layout.o multiboot.o
 obj-y += x86.o
 obj-$(CONFIG_PC) += pc.o pc_sysfw.o
+pc.o-cflags += $(SPICE_CFLAGS)
 obj-$(CONFIG_I440FX) += pc_piix.o
 obj-$(CONFIG_Q35) += pc_q35.o
 obj-$(CONFIG_MICROVM) += microvm.o
diff --git a/monitor/Makefile.objs b/monitor/Makefile.objs
index a8533c9dd7..fd58d80195 100644
--- a/monitor/Makefile.objs
+++ b/monitor/Makefile.objs
@@ -2,5 +2,8 @@ obj-y += misc.o
 common-obj-y += monitor.o qmp.o hmp.o
 common-obj-y += qmp-cmds.o qmp-cmds-control.o
 common-obj-y += hmp-cmds.o
+qmp-cmds.o-cflags += $(SPICE_CFLAGS)
+hmp-cmds.o-cflags += $(SPICE_CFLAGS)
+misc.o-cflags += $(SPICE_CFLAGS)
 
 storage-daemon-obj-y += monitor.o qmp.o qmp-cmds-control.o
diff --git a/softmmu/Makefile.objs b/softmmu/Makefile.objs
index dd15c24346..0e7605bd32 100644
--- a/softmmu/Makefile.objs
+++ b/softmmu/Makefile.objs
@@ -1,3 +1,3 @@
 softmmu-main-y = softmmu/main.o
 obj-y += vl.o
-vl.o-cflags := $(GPROF_CFLAGS) $(SDL_CFLAGS)
+vl.o-cflags := $(GPROF_CFLAGS) $(SDL_CFLAGS) $(SPICE_CFLAGS)
diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
index f32b9e47a3..1df8bb3814 100644
--- a/stubs/Makefile.objs
+++ b/stubs/Makefile.objs
@@ -19,10 +19,10 @@ stub-obj-y += replay.o
 stub-obj-y += runstate-check.o
 stub-obj-$(CONFIG_SOFTMMU) += semihost.o
 stub-obj-y += set-fd-handler.o
-stub-obj-y += vmgenid.o
 stub-obj-y += sysbus.o
 stub-obj-y += tpm.o
 stub-obj-y += trace-control.o
+stub-obj-y += vmgenid.o
 stub-obj-y += vmstate.o
 stub-obj-$(CONFIG_SOFTMMU) += win32-kbd-hook.o
 
diff --git a/ui/Makefile.objs b/ui/Makefile.objs
index 1ab515e23d..6a6fda2f06 100644
--- a/ui/Makefile.objs
+++ b/ui/Makefile.objs
@@ -57,8 +57,8 @@ spice-app.mo-objs += spice-core.o spice-input.o 
spice-display.o
 ifeq ($(CONFIG_GIO)$(CONFIG_SPICE),ym)
 spice-app.mo-objs += spice-app.o
 endif
-spice-app.mo-cflags := $(GIO_CFLAGS)
-spice-app.mo-libs := $(GIO_LIBS)
+spice-app.mo-cflags := $(GIO_CFLAGS) $(SPICE_CFLAGS)
+spice-app.mo-libs := $(GIO_LIBS) $(SPICE_LIBS)
 
 common-obj-$(CONFIG_OPENGL) += shader.o
 common-obj-$(CONFIG_OPENGL) += console-gl.o
diff --git a/util/module.c b/util/module.c
index 2fa93561fe..29b4806520 100644
--- a/util/module.c
+++ b/util/module.c
@@ -22,11 +22,11 @@
 #ifdef CONFIG_MODULE_UPGRADES
 #include "qemu-version.h"
 #endif
-#ifdef CONFIG_TRACE_RECORDER
 #include "trace/recorder.h"
-#endif
 
 
+RECORDER(modules, 16, "QEMU load modules");
+
 typedef struct ModuleEntry
 {
 void (*init)(void);
@@ -85,6 +85,15 @@ void register_dso_module_init(void (*fn)(void), 
module_init_type type)
 {
 ModuleEntry *e;
 
+#ifdef CONFIG_TRACE_RECORDER
+static const char *name[] = {
+"MIGRATION", "BLOCK", "OPTS", "QOM",
+"TRACE", "XEN_BACKEND", "LIBQOS", "FUZZ_TARGET",
+"MAX"
+};
+#endif
+record(modules, "Register DSO module init %p type %u %+s",
+   fn, type, name[type]);
 init_lists();
 
 e = g_malloc0(sizeof(*e));
-- 
2.26.2

[PATCH 07/10] qxl - FIXME: Build as module

[Christophe de Dinechin]

Forcibly build qxl as a module to see if we can load it

Signed-off-by: Christophe de Dinechin 
---
 hw/display/Makefile.objs | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/display/Makefile.objs b/hw/display/Makefile.objs
index 77a7d622bd..f51411619b 100644
--- a/hw/display/Makefile.objs
+++ b/hw/display/Makefile.objs
@@ -44,7 +44,8 @@ common-obj-$(CONFIG_ARTIST) += artist.o
 
 obj-$(CONFIG_VGA) += vga.o
 
-common-obj-$(CONFIG_QXL) += qxl.o qxl-logger.o qxl-render.o
+common-obj-$(CONFIG_QXL:y=m) += qxl.mo
+qxl.mo-objs := qxl.o qxl-logger.o qxl-render.o
 
 obj-$(CONFIG_VIRTIO_GPU) += virtio-gpu-base.o virtio-gpu.o virtio-gpu-3d.o
 obj-$(CONFIG_VHOST_USER_GPU) += vhost-user-gpu.o
-- 
2.26.2

[PATCH 00/10] RFC: Move SPICE to a load module

[Christophe de Dinechin]

There has been a bit of discussion regarding the possibility to move
SPICE code to a module in order to reduce the attack surface and
memory usage when SPICE is not used.

This WIP series proposes one way to do it that is relatively cheap in
terms of code changes, relative to other ideas I tested, and seems to
be working on simple test cases, unlike a couple of more
"sophisiticated" ideas I tried where I ran into rather nasty SPICE
initialization order issues.

Furthermore, the approach presented here requires relatively few code
changes in order to apply to other components as well. It relies on a
couple of macros added to the module.h file, MODIFACE and MODIMPLE.

MODIFACE declare the interface for a module function. A module
function is transformed into a pointer when you build with modules,
and that pointer is used instead of the original function.

MODIMPL implements a MODIFACE, and patches the pointer at load time to
call the function in the shared library.

Thanks to some suggestions from Gerd, I also moved QXL to a module,
although at the moment it does not load correctly.

There are a few known hacks in the present state, including:

- Adding various non-UI files into spice-app.so, which required a
  couple of ../pwd/foo.o references in the makefile. Not very nice,
  but I did not want to spend too much time fixing the makefiles.

- qmp_query_spice had to be dealt with "manually" because its
  interface is generated.

With these changes, the following shared libraries are no longer
needed in the main binary:

libspice-server.so.1 => /lib64/libspice-server.so.1 (HEX)
libopus.so.0 => /lib64/libopus.so.0 (HEX)
liblz4.so.1 => /lib64/liblz4.so.1 (HEX)
libgstapp-1.0.so.0 => /lib64/libgstapp-1.0.so.0 (HEX)
libgstvideo-1.0.so.0 => /lib64/libgstvideo-1.0.so.0 (HEX)
libgstbase-1.0.so.0 => /lib64/libgstbase-1.0.so.0 (HEX)
libgstreamer-1.0.so.0 => /lib64/libgstreamer-1.0.so.0 (HEX)
libssl.so.1.1 => /lib64/libssl.so.1.1 (HEX)
liborc-0.4.so.0 => /lib64/liborc-0.4.so.0 (HEX)

I will keep pushing updates on branch "modular-spice"
on https://github.com/c3d/qemu.git

Christophe de Dinechin (10):
  modules: Provide macros making it easier to identify module exports
  minikconf: Pass variables for modules
  spice: Make spice a module configuration
  spice: Move all the spice-related code in spice-app.so
  build: Avoid build failure when building drivers as modules
  trivial: Remove extra trailing whitespace
  qxl - FIXME: Build as module
  build: Add SPICE_CFLAGS and SPICE_LIBS to relevant files
  spice: Put spice functions in a separate load module
  REMOVE: Instrumentation to show the module functions being replaced

 Makefile |  1 +
 Makefile.objs|  2 ++
 Makefile.target  |  7 +++
 audio/Makefile.objs  |  2 +-
 chardev/Makefile.objs|  2 +-
 configure|  6 +++---
 hw/Makefile.objs |  1 +
 hw/display/Makefile.objs |  4 +++-
 hw/display/qxl.c |  2 +-
 hw/i386/Makefile.objs|  1 +
 include/qemu/module.h| 28 
 include/ui/qemu-spice.h  | 24 +++-
 monitor/Makefile.objs|  3 +++
 monitor/hmp-cmds.c   |  6 ++
 scripts/minikconf.py |  4 ++--
 softmmu/Makefile.objs|  2 +-
 softmmu/vl.c |  1 +
 stubs/Makefile.objs  |  2 +-
 ui/Makefile.objs | 12 ++--
 ui/spice-core.c  | 31 +--
 ui/spice-display.c   |  2 +-
 util/module.c| 13 +++--
 22 files changed, 117 insertions(+), 39 deletions(-)

-- 
2.26.2

[PATCH 05/10] build: Avoid build failure when building drivers as modules

[Christophe de Dinechin]

Signed-off-by: Gerd Hoffmann 
Signed-off-by: Christophe de Dinechin 
---
 Makefile.objs| 1 +
 Makefile.target  | 7 +++
 hw/Makefile.objs | 1 +
 3 files changed, 9 insertions(+)

diff --git a/Makefile.objs b/Makefile.objs
index e38768c8d5..6703353493 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -86,6 +86,7 @@ endif # CONFIG_SOFTMMU
 # Target-independent parts used in system and user emulation
 common-obj-y += cpus-common.o
 common-obj-y += hw/
+common-obj-m += hw/
 common-obj-y += qom/
 common-obj-y += disas/
 
diff --git a/Makefile.target b/Makefile.target
index 8ed1eba95b..3f3b5ee058 100644
--- a/Makefile.target
+++ b/Makefile.target
@@ -179,6 +179,13 @@ endif # CONFIG_SOFTMMU
 dummy := $(call unnest-vars,,obj-y)
 all-obj-y := $(obj-y)
 
+#
+# common-obj-m has some crap here, probably as side effect from
+# filling obj-y.  Clear it.  Fixes suspicious dependency errors when
+# building devices as modules.
+#
+common-obj-m :=
+
 include $(SRC_PATH)/Makefile.objs
 dummy := $(call unnest-vars,.., \
authz-obj-y \
diff --git a/hw/Makefile.objs b/hw/Makefile.objs
index 4cbe5e4e57..d6d387b74b 100644
--- a/hw/Makefile.objs
+++ b/hw/Makefile.objs
@@ -43,4 +43,5 @@ devices-dirs-y += smbios/
 endif
 
 common-obj-y += $(devices-dirs-y)
+common-obj-m := display/
 obj-y += $(devices-dirs-y)
-- 
2.26.2

[PATCH 04/10] spice: Move all the spice-related code in spice-app.so

[Christophe de Dinechin]

If we want to build spice as a separately loadable module, we need to
put all the spice code in one loadable module, because the build
system does not know how to deal with dependencies yet.

Signed-off-by: Christophe de Dinechin 
---
 audio/Makefile.objs   | 2 +-
 chardev/Makefile.objs | 3 +--
 ui/Makefile.objs  | 8 
 3 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/audio/Makefile.objs b/audio/Makefile.objs
index b4a4c11f31..298c895ff5 100644
--- a/audio/Makefile.objs
+++ b/audio/Makefile.objs
@@ -1,5 +1,5 @@
 common-obj-y = audio.o audio_legacy.o noaudio.o wavaudio.o mixeng.o
-common-obj-$(CONFIG_SPICE) += spiceaudio.o
+spice-app.mo-objs += ../audio/spiceaudio.o
 common-obj-$(CONFIG_AUDIO_COREAUDIO) += coreaudio.o
 common-obj-$(CONFIG_AUDIO_DSOUND) += dsoundaudio.o
 common-obj-$(CONFIG_AUDIO_WIN_INT) += audio_win_int.o
diff --git a/chardev/Makefile.objs b/chardev/Makefile.objs
index fc9910d4f2..955fac0cf9 100644
--- a/chardev/Makefile.objs
+++ b/chardev/Makefile.objs
@@ -22,5 +22,4 @@ common-obj-$(CONFIG_BRLAPI) += baum.o
 baum.o-cflags := $(SDL_CFLAGS)
 baum.o-libs := $(BRLAPI_LIBS)
 
-common-obj-$(CONFIG_SPICE) += spice.mo
-spice.mo-objs := spice.o
+spice-app.mo-objs += ../chardev/spice.o
diff --git a/ui/Makefile.objs b/ui/Makefile.objs
index 504b196479..1ab515e23d 100644
--- a/ui/Makefile.objs
+++ b/ui/Makefile.objs
@@ -11,7 +11,6 @@ common-obj-y += keymaps.o console.o cursor.o qemu-pixman.o
 common-obj-y += input.o input-keymap.o input-legacy.o kbd-state.o
 common-obj-y += input-barrier.o
 common-obj-$(CONFIG_LINUX) += input-linux.o
-common-obj-$(CONFIG_SPICE) += spice-core.o spice-input.o spice-display.o
 common-obj-$(CONFIG_COCOA) += cocoa.o
 common-obj-$(CONFIG_VNC) += $(vnc-obj-y)
 common-obj-$(call lnot,$(CONFIG_VNC)) += vnc-stubs.o
@@ -53,10 +52,11 @@ curses.mo-objs := curses.o
 curses.mo-cflags := $(CURSES_CFLAGS) $(ICONV_CFLAGS)
 curses.mo-libs := $(CURSES_LIBS) $(ICONV_LIBS)
 
-ifeq ($(CONFIG_GIO)$(CONFIG_SPICE),yy)
-common-obj-$(if $(CONFIG_MODULES),m,y) += spice-app.mo
+common-obj-$(CONFIG_SPICE) += spice-app.mo
+spice-app.mo-objs += spice-core.o spice-input.o spice-display.o
+ifeq ($(CONFIG_GIO)$(CONFIG_SPICE),ym)
+spice-app.mo-objs += spice-app.o
 endif
-spice-app.mo-objs := spice-app.o
 spice-app.mo-cflags := $(GIO_CFLAGS)
 spice-app.mo-libs := $(GIO_LIBS)
 
-- 
2.26.2

[PATCH 03/10] spice: Make spice a module configuration

[Christophe de Dinechin]

This commit changes the spice configuration 'm' by default, and moves
the spice components to obj-m variables. It is sufficient to build
without modules enable, but does not link correctly yet, since no
shims have been created for the missing functions yet.

Signed-off-by: Christophe de Dinechin 
---
 Makefile  | 1 +
 Makefile.objs | 1 +
 chardev/Makefile.objs | 3 ++-
 configure | 2 +-
 4 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index b29b0eeefa..ee674971a5 100644
--- a/Makefile
+++ b/Makefile
@@ -477,6 +477,7 @@ dummy := $(call unnest-vars,, \
 common-obj-m \
 trace-obj-y)
 
+
 include $(SRC_PATH)/tests/Makefile.include
 
 all: $(DOCS) $(if $(BUILD_DOCS),sphinxdocs) $(TOOLS) $(HELPERS-y) recurse-all 
modules $(vhost-user-json-y)
diff --git a/Makefile.objs b/Makefile.objs
index 98383972ee..e38768c8d5 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -70,6 +70,7 @@ common-obj-$(CONFIG_TPM) += tpm.o
 
 common-obj-y += backends/
 common-obj-y += chardev/
+common-obj-m += chardev/
 
 common-obj-$(CONFIG_SECCOMP) += qemu-seccomp.o
 qemu-seccomp.o-cflags := $(SECCOMP_CFLAGS)
diff --git a/chardev/Makefile.objs b/chardev/Makefile.objs
index d68e1347f9..fc9910d4f2 100644
--- a/chardev/Makefile.objs
+++ b/chardev/Makefile.objs
@@ -22,4 +22,5 @@ common-obj-$(CONFIG_BRLAPI) += baum.o
 baum.o-cflags := $(SDL_CFLAGS)
 baum.o-libs := $(BRLAPI_LIBS)
 
-common-obj-$(CONFIG_SPICE) += spice.o
+common-obj-$(CONFIG_SPICE) += spice.mo
+spice.mo-objs := spice.o
diff --git a/configure b/configure
index 130630b98f..2de1715800 100755
--- a/configure
+++ b/configure
@@ -7471,7 +7471,7 @@ if test "$posix_memalign" = "yes" ; then
 fi
 
 if test "$spice" = "yes" ; then
-  echo "CONFIG_SPICE=y" >> $config_host_mak
+  echo "CONFIG_SPICE=m" >> $config_host_mak
 fi
 
 if test "$smartcard" = "yes" ; then
-- 
2.26.2

[PATCH 02/10] minikconf: Pass variables for modules

[Christophe de Dinechin]

Signed-off-by: Christophe de Dinechin 
---
 scripts/minikconf.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/scripts/minikconf.py b/scripts/minikconf.py
index bcd91015d3..d60add97f6 100755
--- a/scripts/minikconf.py
+++ b/scripts/minikconf.py
@@ -690,10 +690,10 @@ if __name__ == '__main__':
 parser = KconfigParser(data)
 external_vars = set()
 for arg in argv[3:]:
-m = re.match(r'^(CONFIG_[A-Z0-9_]+)=([yn]?)$', arg)
+m = re.match(r'^(CONFIG_[A-Z0-9_]+)=([ymn]?)$', arg)
 if m is not None:
 name, value = m.groups()
-parser.do_assignment(name, value == 'y')
+parser.do_assignment(name, value == 'y' or value == 'm')
 external_vars.add(name[7:])
 else:
 fp = open(arg, 'rt', encoding='utf-8')
-- 
2.26.2

[PATCH 01/10] modules: Provide macros making it easier to identify module exports

[Christophe de Dinechin]

In order to facilitate the move of large chunks of functionality to
load modules, it is simpler to create a wrapper with the same name
that simply relays the implementation. For efficiency, this is
typically done using inline functions in the header for the
corresponding functionality. In that case, we rename the actual
implementation by appending _implementation to its name. This makes it
easier to select which function you want to put a breakpoint on.

Signed-off-by: Christophe de Dinechin 
---
 include/qemu/module.h | 24 
 1 file changed, 24 insertions(+)

diff --git a/include/qemu/module.h b/include/qemu/module.h
index 011ae1ae76..1922a0293c 100644
--- a/include/qemu/module.h
+++ b/include/qemu/module.h
@@ -39,6 +39,30 @@ static void __attribute__((constructor)) do_qemu_init_ ## 
function(void)\
 }
 #endif
 
+#ifdef CONFIG_MODULES
+/* Identify which functions are replaced by a callback stub */
+#ifdef MODULE_STUBS
+#define MODIFACE(Ret,Name,Args) \
+Ret (*Name)Args;\
+extern Ret Name##_implementation Args
+#else /* !MODULE_STUBS */
+#define MODIFACE(Ret,Name,Args) \
+extern Ret (*Name)Args; \
+extern Ret Name##_implementation Args
+#endif /* MODULE_STUBS */
+
+#define MODIMPL(Ret,Name,Args)  \
+static void __attribute__((constructor)) Name##_register(void)  \
+{   \
+Name = Name##_implementation;   \
+}   \
+Ret Name##_implementation Args
+#else /* !CONFIG_MODULES */
+/* When not using a module, such functions are called directly */
+#define MODIFACE(Ret,Name,Args) Ret Name Args
+#define MODIMPL(Ret,Name,Args)  Ret Name Args
+#endif /* CONFIG_MODULES */
+
 typedef enum {
 MODULE_INIT_MIGRATION,
 MODULE_INIT_BLOCK,
-- 
2.26.2

[PATCH v2 2/3] trace: Add support for recorder back-end

[Christophe de Dinechin]

The recorder library provides support for low-cost continuous
recording of events, which can then be replayed. This makes it
possible to collect data "after the fact",for example to show the
events that led to a crash.

Recorder support in qemu is implemented using the existing tracing
interface. In addition, it is possible to individually enable
recorders that are not traces, although this is probably not
recommended.

HMP COMMAND:
The 'recorder' hmp command has been added, which supports two
sub-commands:
- recorder dump: Dump the current state of the recorder. You can
- recorder trace: Set traces using the recorder_trace_set() syntax.
  You can use "recorder trace help" to list all available recorders.

Signed-off-by: Christophe de Dinechin 
---
 configure |  5 +++
 hmp-commands.hx   | 19 --
 monitor/misc.c| 27 ++
 scripts/tracetool/backend/recorder.py | 51 +++
 trace/Makefile.objs   |  2 ++
 trace/control.c   |  7 
 trace/recorder.c  | 22 
 trace/recorder.h  | 34 ++
 util/module.c |  8 +
 9 files changed, 173 insertions(+), 2 deletions(-)
 create mode 100644 scripts/tracetool/backend/recorder.py
 create mode 100644 trace/recorder.c
 create mode 100644 trace/recorder.h

diff --git a/configure b/configure
index ae8737d5a2..130630b98f 100755
--- a/configure
+++ b/configure
@@ -7702,6 +7702,11 @@ fi
 if have_backend "log"; then
   echo "CONFIG_TRACE_LOG=y" >> $config_host_mak
 fi
+if have_backend "recorder"; then
+  echo "CONFIG_TRACE_RECORDER=y" >> $config_host_mak
+  # This is a bit brutal, but there is currently a bug in the makefiles
+  LIBS="$LIBS -lrecorder"
+fi
 if have_backend "ust"; then
   echo "CONFIG_TRACE_UST=y" >> $config_host_mak
 fi
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 60f395c276..565f518d4b 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -297,6 +297,22 @@ ERST
 .cmd= hmp_trace_file,
 },
 
+SRST
+``trace-file on|off|flush``
+  Open, close, or flush the trace file.  If no argument is given, the
+  status of the trace file is displayed.
+ERST
+#endif
+
+#if defined(CONFIG_TRACE_RECORDER)
+{
+.name   = "recorder",
+.args_type  = "op:s?,arg:F?",
+.params = "trace|dump [arg]",
+.help   = "trace selected recorders or print recorder dump",
+.cmd= hmp_recorder,
+},
+
 SRST
 ``trace-file on|off|flush``
   Open, close, or flush the trace file.  If no argument is given, the
@@ -1120,7 +1136,7 @@ ERST
 
 SRST
 ``dump-guest-memory [-p]`` *filename* *begin* *length*
-  \ 
+  \
 ``dump-guest-memory [-z|-l|-s|-w]`` *filename*
   Dump guest memory to *protocol*. The file can be processed with crash or
   gdb. Without ``-z|-l|-s|-w``, the dump format is ELF.
@@ -1828,4 +1844,3 @@ ERST
 .sub_table  = hmp_info_cmds,
 .flags  = "p",
 },
-
diff --git a/monitor/misc.c b/monitor/misc.c
index 89bb970b00..0094b1860f 100644
--- a/monitor/misc.c
+++ b/monitor/misc.c
@@ -61,6 +61,9 @@
 #ifdef CONFIG_TRACE_SIMPLE
 #include "trace/simple.h"
 #endif
+#ifdef CONFIG_TRACE_RECORDER
+#include "trace/recorder.h"
+#endif
 #include "exec/memory.h"
 #include "exec/exec-all.h"
 #include "qemu/option.h"
@@ -227,6 +230,30 @@ static void hmp_trace_file(Monitor *mon, const QDict 
*qdict)
 }
 #endif
 
+#ifdef CONFIG_TRACE_RECORDER
+static void hmp_recorder(Monitor *mon, const QDict *qdict)
+{
+const char *op = qdict_get_try_str(qdict, "op");
+const char *arg = qdict_get_try_str(qdict, "arg");
+
+if (!op) {
+monitor_printf(mon, "missing recorder command\"%s\"\n", op);
+help_cmd(mon, "recorder");
+} else if (!strcmp(op, "trace")) {
+recorder_trace_set(arg);
+} else if (!strcmp(op, "dump")) {
+if (!arg || !*arg) {
+recorder_dump();
+} else {
+recorder_dump_for(arg);
+}
+} else {
+monitor_printf(mon, "unexpected recorder command \"%s\"\n", op);
+help_cmd(mon, "recorder");
+}
+}
+#endif
+
 static void hmp_info_help(Monitor *mon, const QDict *qdict)
 {
 help_cmd(mon, "info");
diff --git a/scripts/tracetool/backend/recorder.py 
b/scripts/tracetool/backend/recorder.py
new file mode 100644
index 00..79cc6f5b03
--- /dev/null
+++ b/scripts/tracetool/backend/recorder.py
@@ -0,0 +1,51 @@
+# -*- coding: utf-8 -*-
+
+"""
+Trace back-end for recorder library
+"""
+
+__author__ = "Christophe

[PATCH v2 3/3] trace: Example of "centralized" recorder tracing

[Christophe de Dinechin]

This is an example showing how the recorder can be used to have one
"topic" covering multiple entries. Here, the topic is "lock".

Here are a few use cases:

- Checking locks:
RECORDER_TRACES=lock qemu
- Graphic visualization of locks:
RECORDER_TRACES="lock=state,id" qemu &
recorder_scope state


cat recorder_scope_data-1.csv

Signed-off-by: Christophe de Dinechin 
---
 util/qemu-thread-common.h | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/util/qemu-thread-common.h b/util/qemu-thread-common.h
index 2af6b12085..0de07a471f 100644
--- a/util/qemu-thread-common.h
+++ b/util/qemu-thread-common.h
@@ -15,6 +15,9 @@
 
 #include "qemu/thread.h"
 #include "trace.h"
+#include "trace/recorder.h"
+
+RECORDER_DEFINE(lock, 16, "Lock state");
 
 static inline void qemu_mutex_post_init(QemuMutex *mutex)
 {
@@ -23,12 +26,14 @@ static inline void qemu_mutex_post_init(QemuMutex *mutex)
 mutex->line = 0;
 #endif
 mutex->initialized = true;
+record(lock, "Init state %d for %p", -1, mutex);
 }
 
 static inline void qemu_mutex_pre_lock(QemuMutex *mutex,
const char *file, int line)
 {
 trace_qemu_mutex_lock(mutex, file, line);
+record(lock, "Locking state %d for %p", 1, mutex);
 }
 
 static inline void qemu_mutex_post_lock(QemuMutex *mutex,
@@ -39,6 +44,7 @@ static inline void qemu_mutex_post_lock(QemuMutex *mutex,
 mutex->line = line;
 #endif
 trace_qemu_mutex_locked(mutex, file, line);
+record(lock, "Locked state %d for %p", 2, mutex);
 }
 
 static inline void qemu_mutex_pre_unlock(QemuMutex *mutex,
@@ -49,6 +55,7 @@ static inline void qemu_mutex_pre_unlock(QemuMutex *mutex,
 mutex->line = 0;
 #endif
 trace_qemu_mutex_unlock(mutex, file, line);
+record(lock, "Unkocked state %d for %p", 0, mutex);
 }
 
 #endif
-- 
2.26.2

[PATCH v2 0/3] trace: Add a trace backend for the recorder library

[Christophe de Dinechin]

The recorder library implements low-cost always-on tracing, with three
usage models:

1. Flight recorder: Dump information on recent events in case of crash
2. Tracing: Individual traces can be enabled using environment variables
3. Real-time graphing / control, using the recorder_scope application

This short series introduces a new "recorder" back-end which connects
to the recorder. Traces using the recorder are intentionally "always on".
An example is given of how the recorder can also be used separately
from generated traces. This can be useful if you want to enable
multiple related traces for a particular topic.

This series requires a small makefile fix submitted earlier, included
here for convenience.

Christophe de Dinechin (3):
  Makefile: Compute libraries for libqemuutil.a and libvhost-user.a
  trace: Add support for recorder back-end
  trace: Example of "centralized" recorder tracing

 Makefile  |  2 ++
 configure |  5 +++
 hmp-commands.hx   | 19 --
 monitor/misc.c| 27 ++
 scripts/tracetool/backend/recorder.py | 51 +++
 trace/Makefile.objs   |  2 ++
 trace/control.c   |  7 
 trace/recorder.c  | 22 
 trace/recorder.h  | 34 ++
 util/module.c |  8 +
 util/qemu-thread-common.h |  7 
 11 files changed, 182 insertions(+), 2 deletions(-)
 create mode 100644 scripts/tracetool/backend/recorder.py
 create mode 100644 trace/recorder.c
 create mode 100644 trace/recorder.h

-- 
2.26.2

[PATCH v2 1/3] Makefile: Compute libraries for libqemuutil.a and libvhost-user.a

[Christophe de Dinechin]

In util/Makefile.objs, there is a setting for dbus.o-libs.
Trying to copy-paste that to add a library for module.o that was was
not otherwise linked yields link errors on a number of binaries,
e.g. qemu-ga, with unsatisfied symbols in libqemuutil.a(module.o).
The reason is that library dependencies are not propagated to the .a
files automatically.

Adding a call to extract-libs to get the libraries for the two .a
files that are used elsewhere.

Signed-off-by: Christophe de Dinechin 
---
 Makefile | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Makefile b/Makefile
index a0092153af..b29b0eeefa 100644
--- a/Makefile
+++ b/Makefile
@@ -589,6 +589,8 @@ Makefile: $(version-obj-y)
 ##
 # Build libraries
 
+libqemuutil.a-libs += $(call extract-libs, $(util-obj-y) $(trace-obj-y) 
$(stub-obj-y))
+libvhost-user.a-libs += $(call extract-libs, $(libvhost-user-obj-y) 
$(util-obj-y) $(stub-obj-y))
 libqemuutil.a: $(util-obj-y) $(trace-obj-y) $(stub-obj-y)
 libvhost-user.a: $(libvhost-user-obj-y) $(util-obj-y) $(stub-obj-y)
 
-- 
2.26.2

Re: [PATCH] trivial: Remove extra character in configure help

[Christophe de Dinechin]

Please ignore. The =B appears intentional, even if it offsets the whole help 
text.

Maybe replace with =L to indicate a list is expected?

> On 24 Jun 2020, at 10:33, Christophe de Dinechin  wrote:
> 
> Signed-off-by: Christophe de Dinechin 
> ---
> configure | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/configure b/configure
> index ba88fd1824..c7a6a5adfe 100755
> --- a/configure
> +++ b/configure
> @@ -1787,7 +1787,7 @@ Advanced options (experts only):
>   --block-drv-ro-whitelist=L
>set block driver read-only whitelist
>(affects only QEMU, not qemu-img)
> -  --enable-trace-backends=B Set trace backend
> +  --enable-trace-backends= Set trace backend
>Available backends: $trace_backend_list
>   --with-trace-file=NAME   Full PATH,NAME of file to store traces
>Default:trace-
> -- 
> 2.26.2
> 
> 

[PATCH] trivial: Remove extra character in configure help

[Christophe de Dinechin]

Signed-off-by: Christophe de Dinechin 
---
 configure | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/configure b/configure
index ba88fd1824..c7a6a5adfe 100755
--- a/configure
+++ b/configure
@@ -1787,7 +1787,7 @@ Advanced options (experts only):
   --block-drv-ro-whitelist=L
set block driver read-only whitelist
(affects only QEMU, not qemu-img)
-  --enable-trace-backends=B Set trace backend
+  --enable-trace-backends= Set trace backend
Available backends: $trace_backend_list
   --with-trace-file=NAME   Full PATH,NAME of file to store traces
Default:trace-
-- 
2.26.2

Re: how to build QEMU with the peripheral device modules

[Christophe de Dinechin]

> Le 19 Jun 2020 à 15:24, casmac  a écrit :
> 
> Hi All,
>I am trying to add a DMA peripheral module. In hw/dma directory, a file  
> ti_dma.c is added.
>Also, in hw/dma/kconfig, I added the following lines:
> config TI_DMA
>   bool
>In hw/dma/makefile.ojb, added one line:
> common-obj-$(CONFIG_TI_DMA) += ti_dma.o

How did you set CONFIG_TI_DMA?

You can add it to the configure script if it's orthogonal to everything else, 
or have it set as some default in a default config e.g. for whatever board you 
have, or add a Kconfig dependency.

>However, ti_dma.c is not compiled as the QEMU project is built. Some 
> directories(eg. block, cpu...) under hw will be compiled though. 
>The makefile.obj seems to show that the DMA module would be built along 
> with  others:
> devices-dirs-y = core/
> ifeq ($(CONFIG_SOFTMMU), y)
> devices-dirs-$(call lor,$(CONFIG_VIRTIO_9P),$(call 
> land,$(CONFIG_VIRTFS),$(CONFIG_XEN))) += 9pfs/
> devices-dirs-y += acpi/
> devices-dirs-y += adc/
> devices-dirs-y += audio/
> devices-dirs-y += block/
> devices-dirs-y += bt/
> devices-dirs-y += char/
> devices-dirs-y += cpu/
> devices-dirs-y += display/
> devices-dirs-y += dma/
> devices-dirs-y += gpio/
> ..
> I am not sure what is missing here. Any advise would be appreciated. 
> Thanks.
> 
> xiaolei

Understanding initialization order for spice in qemu

[Christophe de Dinechin]

Hi Gerd,


When I build qemu on master with moduels enabled, and run with spice, I 
occasionally see:

 qemu-system-x86_64: util/module.c:136: module_load_file: Assertion 
`QTAILQ_EMPTY(_init_list)' failed.

Interestingly, I seem to have seen that only on master, but not on my own 
branch. Have you ever seen the same problem? Could there be a race condition 
here explaining why I sometimes see it, sometimes not? Or do you think it's 
more likely to be a missing build dependency? Asking because I don't recall 
ever seeing that on a clean build.

Command line:
% qemu-system-x86_64  -display spice-app /data/VMs/deb9.qcow2



Thanks
Christophe

Re: ovmf / PCI passthrough impaired due to very limiting PCI64 aperture

[Christophe de Dinechin]

> Le 16 Jun 2020 à 19:10, Eduardo Habkost  a écrit :
> 
> On Tue, Jun 16, 2020 at 05:57:46PM +0100, Dr. David Alan Gilbert wrote:
>> * Gerd Hoffmann (kra...@redhat.com) wrote:
>>>  Hi,
>>> 
 (a) We could rely in the guest physbits to calculate the PCI64 aperture.
>>> 
>>> I'd love to do that.  Move the 64-bit I/O window as high as possible and
>>> use -- say -- 25% of the physical address space for it.
>>> 
>>> Problem is we can't.
>>> 
 failure. Also, if the users are not setting the physbits in the guest,
 there must be a default (seems to be 40bit according to my experiments),
 seems to be a good idea to rely on that.
>>> 
>>> Yes, 40 is the default, and it is used *even if the host supports less
>>> than that*.  Typical values I've seen for intel hardware are 36 and 39.
>>> 39 is used even by recent hardware (not the xeons, but check out a
>>> laptop or a nuc).
>>> 
 If guest physbits is 40, why to have OVMF limiting it to 36, right?
>>> 
>>> Things will explode in case OVMF uses more physbits than the host
>>> supports (host physbits limit applies to ept too).  In other words: OVMF
>>> can't trust the guest physbits, so it is conservative to be on the safe
>>> side.
>>> 
>>> If we can somehow make a *trustable* physbits value available to the
>>> guest, then yes, we can go that route.  But the guest physbits we have
>>> today unfortunately don't cut it.
>> 
>> In downstream RH qemu, we run with host-physbits as default; so it's 
>> reasonably
>> trustworthy; of course that doesn't help you across a migration between
>> hosts with different sizes (e.g. an E5 Xeon to an E3).
>> Changing upstream to do the same would seem sensible to me, but it's not
>> a foolproof config.
> 
> Yeah, to make it really trustworthy we would need to prevent
> migration to hosts with mismatching phys sizes.

Wouldn't it be sufficient to prevent guestphysbits > hostphysbits?

>  We would need to
> communicate that to the guest somehow (with new hypervisor CPUID
> flags, maybe).
> 
> -- 
> Eduardo

Re: ovmf / PCI passthrough impaired due to very limiting PCI64 aperture

[Christophe de Dinechin]

> Le 16 Jun 2020 à 18:50, Gerd Hoffmann  a écrit :
> 
>  Hi,
> 
>> (a) We could rely in the guest physbits to calculate the PCI64 aperture.
> 
> I'd love to do that.  Move the 64-bit I/O window as high as possible and
> use -- say -- 25% of the physical address space for it.
> 
> Problem is we can't.

Is the only reason unreliable guest physbits?

> 
>> failure. Also, if the users are not setting the physbits in the guest,
>> there must be a default (seems to be 40bit according to my experiments),
>> seems to be a good idea to rely on that.
> 
> Yes, 40 is the default, and it is used *even if the host supports less
> than that*.  Typical values I've seen for intel hardware are 36 and 39.
> 39 is used even by recent hardware (not the xeons, but check out a
> laptop or a nuc).
> 
>> If guest physbits is 40, why to have OVMF limiting it to 36, right?
> 
> Things will explode in case OVMF uses more physbits than the host
> supports (host physbits limit applies to ept too).  In other words: OVMF
> can't trust the guest physbits, so it is conservative to be on the safe
> side.
> 
> If we can somehow make a *trustable* physbits value available to the
> guest, then yes, we can go that route.  But the guest physbits we have
> today unfortunately don't cut it.

What is the rationale for ever allowing guest physbits > host physbits?

> 
> take care,
>  Gerd
> 
> 

[PATCH] Makefile: Compute libraries for libqemuutil.a and libvhost-user.a

[Christophe de Dinechin]

In util/Makefile.objs, there is a setting for dbus.o-libs.
Trying to copy-paste that to add a library for module.o that was was
not otherwise linked yields link errors on a number of binaries,
e.g. qemu-ga, with unsatisfied symbols in libqemuutil.a(module.o).
The reason is that library dependencies are not propagated to the .a
files automatically.

Adding a call to extract-libs to get the libraries for the two .a
files that are used elsewhere.

Signed-off-by: Christophe de Dinechin 
---
 Makefile | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Makefile b/Makefile
index 2e93068894..5fb0c78a0b 100644
--- a/Makefile
+++ b/Makefile
@@ -598,6 +598,8 @@ Makefile: $(version-obj-y)
 ##
 # Build libraries
 
+libqemuutil.a-libs += $(call extract-libs, $(util-obj-y) $(trace-obj-y) 
$(stub-obj-y))
+libvhost-user.a-libs += $(call extract-libs, $(libvhost-user-obj-y) 
$(util-obj-y) $(stub-obj-y))
 libqemuutil.a: $(util-obj-y) $(trace-obj-y) $(stub-obj-y)
 libvhost-user.a: $(libvhost-user-obj-y) $(util-obj-y) $(stub-obj-y)
 
-- 
2.26.2