Re: [PATCH v6 11/11] qapi: Use QAPI_LIST_ADD() where possible

[David Gibson]

On Tue, Oct 27, 2020 at 12:05:56AM -0500, Eric Blake wrote:
> Anywhere we create a list of just one item or by prepending items
> (typically because order doesn't matter), we can use the now-public
> macro.  But places where we must keep the list in order by appending
> remain open-coded.
> 
> Signed-off-by: Eric Blake 
> ---
>  docs/devel/writing-qmp-commands.txt | 13 +++--
>  hw/net/rocker/rocker_fp.h   |  2 +-
>  block/gluster.c | 19 +
>  chardev/char.c  | 21 +++
>  hw/core/machine.c   |  6 +
>  hw/net/rocker/rocker.c  |  8 +++---
>  hw/net/rocker/rocker_fp.c   | 14 +-
>  hw/net/virtio-net.c | 21 +--
>  migration/migration.c   |  7 ++---
>  migration/postcopy-ram.c|  7 ++---
>  monitor/hmp-cmds.c  | 11 
>  qemu-img.c  |  5 ++--
>  qga/commands-posix.c| 13 +++--
>  qga/commands-win32.c| 17 +++-
>  qga/commands.c  |  6 +
>  qom/qom-qmp-cmds.c  | 29 ++--
>  target/arm/helper.c |  6 +
>  target/arm/monitor.c| 13 ++---
>  target/i386/cpu.c   |  6 +
>  target/mips/helper.c|  6 +
>  target/s390x/cpu_models.c   | 12 ++---
>  tests/test-clone-visitor.c  |  7 ++---
>  tests/test-qobject-output-visitor.c | 42 ++---
>  tests/test-visitor-serialization.c  |  5 +---
>  trace/qmp.c | 22 +++
>  ui/vnc.c| 21 +--
>  util/qemu-config.c  | 14 +++---
>  target/ppc/translate_init.c.inc | 12 ++---

ppc parts
Acked-by: David Gibson 

>  28 files changed, 119 insertions(+), 246 deletions(-)
> 
> diff --git a/docs/devel/writing-qmp-commands.txt 
> b/docs/devel/writing-qmp-commands.txt
> index 46a6c48683f5..3e11eeaa1893 100644
> --- a/docs/devel/writing-qmp-commands.txt
> +++ b/docs/devel/writing-qmp-commands.txt
> @@ -531,15 +531,10 @@ TimerAlarmMethodList *qmp_query_alarm_methods(Error 
> **errp)
>  bool current = true;
> 
>  for (p = alarm_timers; p->name; p++) {
> -TimerAlarmMethodList *info = g_malloc0(sizeof(*info));
> -info->value = g_malloc0(sizeof(*info->value));
> -info->value->method_name = g_strdup(p->name);
> -info->value->current = current;
> -
> -current = false;
> -
> -info->next = method_list;
> -method_list = info;
> + TimerAlarmMethod *value = g_new0(TimerAlarmMethod, 1);
> +value->method_name = g_strdup(p->name);
> +value->current = current;
> +QAPI_LIST_ADD(method_list, value);
>  }
> 
>  return method_list;
> diff --git a/hw/net/rocker/rocker_fp.h b/hw/net/rocker/rocker_fp.h
> index dbe1dd329a4b..4cb0bb9ccf81 100644
> --- a/hw/net/rocker/rocker_fp.h
> +++ b/hw/net/rocker/rocker_fp.h
> @@ -28,7 +28,7 @@ int fp_port_eg(FpPort *port, const struct iovec *iov, int 
> iovcnt);
> 
>  char *fp_port_get_name(FpPort *port);
>  bool fp_port_get_link_up(FpPort *port);
> -void fp_port_get_info(FpPort *port, RockerPortList *info);
> +void fp_port_get_info(FpPort *port, RockerPort *info);
>  void fp_port_get_macaddr(FpPort *port, MACAddr *macaddr);
>  void fp_port_set_macaddr(FpPort *port, MACAddr *macaddr);
>  uint8_t fp_port_get_learning(FpPort *port);
> diff --git a/block/gluster.c b/block/gluster.c
> index 4f1448e2bc88..cf446c23f85d 100644
> --- a/block/gluster.c
> +++ b/block/gluster.c
> @@ -359,8 +359,8 @@ static int qemu_gluster_parse_uri(BlockdevOptionsGluster 
> *gconf,
>  return -EINVAL;
>  }
> 
> -gconf->server = g_new0(SocketAddressList, 1);
> -gconf->server->value = gsconf = g_new0(SocketAddress, 1);
> +gsconf = g_new0(SocketAddress, 1);
> +QAPI_LIST_ADD(gconf->server, gsconf);
> 
>  /* transport */
>  if (!uri->scheme || !strcmp(uri->scheme, "gluster")) {
> @@ -514,7 +514,7 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster 
> *gconf,
>  {
>  QemuOpts *opts;
>  SocketAddress *gsconf = NULL;
> -SocketAddressList *curr = NULL;
> +SocketAddressList **curr;
>  QDict *backing_options = NULL;
>  Error *local_err = NULL;
>  char *str = NULL;
> @@ -547,6 +547,7 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster 
> *gconf,
>  }
>  gconf->path = g_strdup(ptr);
>  qemu_opts_del(opts);
> +curr = &gconf->server;
> 
>  for (i = 0; i < num_servers; i++) {
>  str = g_strdup_printf(GLUSTER_OPT_SERVER_PATTERN"%d.", i);
> @@ -655,15 +656,9 @@ static int 
> qemu_gluster_parse_json(BlockdevOptionsGluster *gconf,
>  qemu_opts_del(opts);
>  }
> 
> -if (gconf->server == NULL) {
> -gconf->server = g_new0(SocketAddressList, 1);
> -gco

[Bug 1897194] Re: Test failure in test-crypto-secret.c

[Toolybird]

strace shows the problem:

add_key("user", "qemu_test_secret", "Test Payload", 12,
KEY_SPEC_PROCESS_KEYRING) = -1 EPERM (Operation not permitted)

It appears systemd-nspawn containers don't have CAP_SYS_ADMIN which is
apparently needed for the keyring stuff to work. Fair enough.

But the underlying problem here is configure switch `--disable-keyring'
does not work. It previously worked in the 5.1.0 release but now it's
broken.

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

Title:
  Test failure in test-crypto-secret.c

Status in QEMU:
  New

Bug description:
  When running qemu test suite I'm seeing a test failure:

  ERROR:../qemu/tests/test-crypto-secret.c:144:test_secret_keyring_good:
  assertion failed: (key >= 0)

  Host is Arch Linux running in the standard Arch build environment
  (essentially an nspawn container).

  I first noticed this at release of 5.1.0 but it's still there on
  current trunk. For 5.1.0 I was able to sidestep the issue by building
  with `--disable-keyring' but this no longer works (I think due to
  9866a33cbb7046891dec3dcc9ca2015828673afe)

  Any clues on what might be the cause? Not sure how to debug.

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

Re: [PATCH 2/3] console: modify ppm_save to take a pixman image ref

[Markus Armbruster]

marcandre.lur...@redhat.com writes:

> From: Marc-André Lureau 
>
> The function is going to be called from a coroutine, and may yields.

s/yields/yield/

> Let's ensure our image reference doesn't change over time (due to resize
> etc) by keeping a ref.
>
> Signed-off-by: Marc-André Lureau 

Reviewed-by: Markus Armbruster 

Re: [PATCH 1/4] spapr: qemu_memalign() doesn't return NULL

[Greg Kurz]

On Tue, 27 Oct 2020 12:56:40 +1100
David Gibson  wrote:

> On Mon, Oct 26, 2020 at 03:46:47PM +0100, Greg Kurz wrote:
> > On Mon, 26 Oct 2020 14:43:08 +0100
> > Philippe Mathieu-Daudé  wrote:
> > 
> > > On 10/26/20 1:40 PM, Greg Kurz wrote:
> > > > qemu_memalign() aborts if OOM. Drop some dead code.
> > > > 
> > > > Signed-off-by: Greg Kurz 
> > > > ---
> > > >   hw/ppc/spapr.c   |6 --
> > > >   hw/ppc/spapr_hcall.c |8 ++--
> > > >   2 files changed, 2 insertions(+), 12 deletions(-)
> > > > 
> > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > > index 0cc19b5863a4..f098d0ee6d98 100644
> > > > --- a/hw/ppc/spapr.c
> > > > +++ b/hw/ppc/spapr.c
> > > > @@ -1521,12 +1521,6 @@ void spapr_reallocate_hpt(SpaprMachineState 
> > > > *spapr, int shift,
> > > >   int i;
> > > >   
> > > >   spapr->htab = qemu_memalign(size, size);
> > > > -if (!spapr->htab) {
> > > > -error_setg_errno(errp, errno,
> > > > - "Could not allocate HPT of order %d", 
> > > > shift);
> > > > -return;
> > > 
> > > Wasn't the idea to use qemu_try_memalign() here?
> > > 
> > 
> > Well... I have mixed feeling around this. The HTAB was first
> > introduced by commit:
> > 
> > commit f43e35255cffb6ac6230dd09d308f7909f823f96
> > Author: David Gibson 
> > Date:   Fri Apr 1 15:15:22 2011 +1100
> > 
> > Virtual hash page table handling on pSeries machine
> > 
> > using qemu_mallocz(), which was aborting on OOM. It then got
> > replaced by g_malloc0() when qemu_mallocz() got deprecated
> > and eventually by qemu_memalign() when KVM support was added.
> > 
> > Surviving OOM when allocating the HTAB never seemed to be an
> > option until this commit that introduced the check:
> > 
> > commit c5f54f3e31bf693f70a98d4d73ea5dbe05689857
> > Author: David Gibson 
> > Date:   Tue Feb 9 10:21:56 2016 +1000
> > 
> > pseries: Move hash page table allocation to reset time
> > 
> > I don't really see in the patch and in the changelog an obvious
> > desire to try to handle OOM.
> 
> 
> This one is probably ok.  AFAICT all failures returned here would be
> more or less fatal in the caller, one way or another (&error_fatal in
> two cases, and failure to load an incoming migration stream in the
> other).
> 
> > > > -}
> > > > -
> > > >   memset(spapr->htab, 0, size);
> > > >   spapr->htab_shift = shift;
> > > >   
> > > > diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> > > > index 607740150fa2..34e146f628fb 100644
> > > > --- a/hw/ppc/spapr_hcall.c
> > > > +++ b/hw/ppc/spapr_hcall.c
> > > > @@ -361,12 +361,8 @@ static void *hpt_prepare_thread(void *opaque)
> > > >   size_t size = 1ULL << pending->shift;
> > > >   
> > > >   pending->hpt = qemu_memalign(size, size);
> > > > -if (pending->hpt) {
> > > > -memset(pending->hpt, 0, size);
> > > > -pending->ret = H_SUCCESS;
> > > > -} else {
> > > > -pending->ret = H_NO_MEM;
> > > 
> > > Ditto.
> > > 
> > 
> > This one was introduced by commit:
> > 
> > commit 0b0b831016ae93bc14698a5d7202eb77feafea75
> > Author: David Gibson 
> > Date:   Fri May 12 15:46:49 2017 +1000
> > 
> > pseries: Implement HPT resizing
> > 
> > I agree that maybe the intent here could have been to use 
> > qemu_try_memalign(),
> > but again I don't quite see any strong justification to handle OOM in the
> > changelog.
> > 
> > David,
> > 
> > Any insight to share ?
> 
> Aborting on an HPT resize failure is definitely not ok, though.  This
> one needs to be a qemu_try_memalign().
> 

Ok, I'll fix that.


pgpO09rLrQxTv.pgp
Description: OpenPGP digital signature

[PULL 00/12] qemu-ga patch queue for soft-freeze

[Michael Roth]

The following changes since commit a46e72710566eea0f90f9c673a0f02da0064acce:

  Merge remote-tracking branch 'remotes/cohuck/tags/s390x-20201026' into 
staging (2020-10-26 14:50:03 +)

are available in the Git repository at:

  git://github.com/mdroth/qemu.git tags/qga-pull-2020-10-27-tag

for you to fetch changes up to 568979ea819d945e8af6c14658793b58bcd4485c:

  qga: add ssh-get-authorized-keys (2020-10-27 00:22:30 -0500)


qemu-ga patch queue for soft-freeze

* add guest-get-disks for w32/linux
* add guest-{add,remove,get}-authorized-keys
* fix API violations and schema documentation inconsistencies with
  recently-added guest-get-devices


Marc-André Lureau (5):
  glib-compat: add g_unix_get_passwd_entry_qemu()
  qga: add ssh-{add,remove}-authorized-keys
  qga: add *reset argument to ssh-add-authorized-keys
  meson: minor simplification
  qga: add ssh-get-authorized-keys

Markus Armbruster (4):
  qga: Rename guest-get-devices return member 'address' to 'id'
  qga: Use common time encoding for guest-get-devices 'driver-date'
  qga-win: Fix guest-get-devices error API violations
  qga: Flatten simple union GuestDeviceId

Tomáš Golembiovský (3):
  qga: add command guest-get-disks
  qga: add implementation of guest-get-disks for Linux
  qga: add implementation of guest-get-disks for Windows

 include/glib-compat.h|  26 +++
 qga/commands-posix-ssh.c | 516 +++
 qga/commands-posix.c | 290 +-
 qga/commands-win32.c | 140 +++--
 qga/meson.build  |  34 +++-
 qga/qapi-schema.json | 127 +++-
 6 files changed, 1092 insertions(+), 41 deletions(-)
 create mode 100644 qga/commands-posix-ssh.c

[PULL 02/12] qga: Use common time encoding for guest-get-devices 'driver-date'

[Michael Roth]

From: Markus Armbruster 

guest-get-devices returns 'driver-date' as string in the format
-MM-DD.  Goes back to recent commit 2e4211cee4 "qga: add command
guest-get-devices for reporting VirtIO devices".

We should avoid use of multiple encodings for the same kind of data.
Especially string encodings.  Change it to return nanoseconds since
the epoch, like guest-get-time does.

Signed-off-by: Markus Armbruster 
Reviewed-by: Marc-André Lureau 
Signed-off-by: Michael Roth 
---
 qga/commands-win32.c | 19 +++
 qga/qapi-schema.json |  4 ++--
 2 files changed, 13 insertions(+), 10 deletions(-)

diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 879b02b6c3..b01616a992 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -1641,6 +1641,12 @@ out:
 return head;
 }
 
+static int64_t filetime_to_ns(const FILETIME *tf)
+{
+return int64_t)tf->dwHighDateTime << 32) | tf->dwLowDateTime)
+- W32_FT_OFFSET) * 100;
+}
+
 int64_t qmp_guest_get_time(Error **errp)
 {
 SYSTEMTIME ts = {0};
@@ -1657,8 +1663,7 @@ int64_t qmp_guest_get_time(Error **errp)
 return -1;
 }
 
-return int64_t)tf.dwHighDateTime << 32) | tf.dwLowDateTime)
-- W32_FT_OFFSET) * 100;
+return filetime_to_ns(&tf);
 }
 
 void qmp_guest_set_time(bool has_time, int64_t time_ns, Error **errp)
@@ -2363,7 +2368,6 @@ GuestDeviceInfoList *qmp_guest_get_devices(Error **errp)
 slog("enumerating devices");
 for (i = 0; SetupDiEnumDeviceInfo(dev_info, i, &dev_info_data); i++) {
 bool skip = true;
-SYSTEMTIME utc_date;
 g_autofree LPWSTR name = NULL;
 g_autofree LPFILETIME date = NULL;
 g_autofree LPWSTR version = NULL;
@@ -2434,13 +2438,12 @@ GuestDeviceInfoList *qmp_guest_get_devices(Error **errp)
 slog("failed to get driver date");
 continue;
 }
-FileTimeToSystemTime(date, &utc_date);
-device->driver_date = g_strdup_printf("%04d-%02d-%02d",
-utc_date.wYear, utc_date.wMonth, utc_date.wDay);
+device->driver_date = filetime_to_ns(date);
 device->has_driver_date = true;
 
-slog("driver: %s\ndriver version: %s,%s\n", device->driver_name,
-device->driver_date, device->driver_version);
+slog("driver: %s\ndriver version: %" PRId64 ",%s\n",
+ device->driver_name, device->driver_date,
+ device->driver_version);
 item = g_new0(GuestDeviceInfoList, 1);
 item->value = g_steal_pointer(&device);
 if (!cur_item) {
diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
index f2c81cda2b..c7bfb8bf6a 100644
--- a/qga/qapi-schema.json
+++ b/qga/qapi-schema.json
@@ -1282,7 +1282,7 @@
 # @GuestDeviceInfo:
 #
 # @driver-name: name of the associated driver
-# @driver-date: driver release date in format -MM-DD
+# @driver-date: driver release date, in nanoseconds since the epoch
 # @driver-version: driver version
 # @id: device ID
 #
@@ -1291,7 +1291,7 @@
 { 'struct': 'GuestDeviceInfo',
   'data': {
   'driver-name': 'str',
-  '*driver-date': 'str',
+  '*driver-date': 'int',
   '*driver-version': 'str',
   '*id': 'GuestDeviceId'
   } }
-- 
2.25.1

[PULL 10/12] qga: add *reset argument to ssh-add-authorized-keys

[Michael Roth]

From: Marc-André Lureau 

I prefer 'reset' over 'clear', since 'clear' and keys may have some
other relations or meaning.

Signed-off-by: Marc-André Lureau 
Signed-off-by: Michael Roth 
---
 qga/commands-posix-ssh.c | 53 
 qga/qapi-schema.json |  3 ++-
 2 files changed, 50 insertions(+), 6 deletions(-)

diff --git a/qga/commands-posix-ssh.c b/qga/commands-posix-ssh.c
index a7bc9a1c24..f974bc4b64 100644
--- a/qga/commands-posix-ssh.c
+++ b/qga/commands-posix-ssh.c
@@ -168,6 +168,7 @@ read_authkeys(const char *path, Error **errp)
 
 void
 qmp_guest_ssh_add_authorized_keys(const char *username, strList *keys,
+  bool has_reset, bool reset,
   Error **errp)
 {
 g_autofree struct passwd *p = NULL;
@@ -178,6 +179,7 @@ qmp_guest_ssh_add_authorized_keys(const char *username, 
strList *keys,
 size_t nkeys, nauthkeys;
 
 ERRP_GUARD();
+reset = has_reset && reset;
 
 if (!check_openssh_pub_keys(keys, &nkeys, errp)) {
 return;
@@ -191,7 +193,9 @@ qmp_guest_ssh_add_authorized_keys(const char *username, 
strList *keys,
 ssh_path = g_build_filename(p->pw_dir, ".ssh", NULL);
 authkeys_path = g_build_filename(ssh_path, "authorized_keys", NULL);
 
-authkeys = read_authkeys(authkeys_path, NULL);
+if (!reset) {
+authkeys = read_authkeys(authkeys_path, NULL);
+}
 if (authkeys == NULL) {
 if (!g_file_test(ssh_path, G_FILE_TEST_IS_DIR) &&
 !mkdir_for_user(ssh_path, p, 0700, errp)) {
@@ -318,7 +322,7 @@ test_invalid_user(void)
 {
 Error *err = NULL;
 
-qmp_guest_ssh_add_authorized_keys("", NULL, &err);
+qmp_guest_ssh_add_authorized_keys("", NULL, FALSE, FALSE, &err);
 error_free_or_abort(&err);
 
 qmp_guest_ssh_remove_authorized_keys("", NULL, &err);
@@ -333,7 +337,8 @@ test_invalid_key(void)
 };
 Error *err = NULL;
 
-qmp_guest_ssh_add_authorized_keys(g_get_user_name(), &key, &err);
+qmp_guest_ssh_add_authorized_keys(g_get_user_name(), &key,
+  FALSE, FALSE, &err);
 error_free_or_abort(&err);
 
 qmp_guest_ssh_remove_authorized_keys(g_get_user_name(), &key, &err);
@@ -346,13 +351,17 @@ test_add_keys(void)
 Error *err = NULL;
 
 qmp_guest_ssh_add_authorized_keys(g_get_user_name(),
-  (strList *)&test_key2, &err);
+  (strList *)&test_key2,
+  FALSE, FALSE,
+  &err);
 g_assert_null(err);
 
 test_authorized_keys_equal("algo key2 comments");
 
 qmp_guest_ssh_add_authorized_keys(g_get_user_name(),
-  (strList *)&test_key1_2, &err);
+  (strList *)&test_key1_2,
+  FALSE, FALSE,
+  &err);
 g_assert_null(err);
 
 /*  key2 came first, and should'nt be duplicated */
@@ -360,6 +369,39 @@ test_add_keys(void)
"algo key1 comments");
 }
 
+static void
+test_add_reset_keys(void)
+{
+Error *err = NULL;
+
+qmp_guest_ssh_add_authorized_keys(g_get_user_name(),
+  (strList *)&test_key1_2,
+  FALSE, FALSE,
+  &err);
+g_assert_null(err);
+
+/* reset with key2 only */
+test_authorized_keys_equal("algo key1 comments\n"
+   "algo key2 comments");
+
+qmp_guest_ssh_add_authorized_keys(g_get_user_name(),
+  (strList *)&test_key2,
+  TRUE, TRUE,
+  &err);
+g_assert_null(err);
+
+test_authorized_keys_equal("algo key2 comments");
+
+/* empty should clear file */
+qmp_guest_ssh_add_authorized_keys(g_get_user_name(),
+  (strList *)NULL,
+  TRUE, TRUE,
+  &err);
+g_assert_null(err);
+
+test_authorized_keys_equal("");
+}
+
 static void
 test_remove_keys(void)
 {
@@ -393,6 +435,7 @@ int main(int argc, char *argv[])
 g_test_add_func("/qga/ssh/invalid_user", test_invalid_user);
 g_test_add_func("/qga/ssh/invalid_key", test_invalid_key);
 g_test_add_func("/qga/ssh/add_keys", test_add_keys);
+g_test_add_func("/qga/ssh/add_reset_keys", test_add_reset_keys);
 g_test_add_func("/qga/ssh/remove_keys", test_remove_keys);
 
 return g_test_run();
diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
index a2727ed86b..4ddea898fa 100644
--- a/qga/qapi-schema.json
+++ b/qga/qapi-schema.json
@@ -1352,6 +1352,7 @@
 #
 # @username: the user account to add the authorized keys
 # @keys: the public keys to add (in OpenSSH/sshd(8) authorized_keys format)
+# @reset: i

[PULL 08/12] glib-compat: add g_unix_get_passwd_entry_qemu()

[Michael Roth]

From: Marc-André Lureau 

The glib function was introduced in 2.64. It's a safer version of
getpwnam, and also simpler to use than getpwnam_r.

Currently, it's only use by the next patch in qemu-ga, which doesn't
(well well...) need the thread safety guarantees. Since the fallback
version is still unsafe, I would rather keep the _qemu postfix, to make
sure it's not being misused by mistake. When/if necessary, we can
implement a safer fallback and drop the _qemu suffix.

Signed-off-by: Marc-André Lureau 
Reviewed-by: Michal Privoznik 
Signed-off-by: Michael Roth 
---
 include/glib-compat.h | 26 ++
 1 file changed, 26 insertions(+)

diff --git a/include/glib-compat.h b/include/glib-compat.h
index 0b0ec76299..64e68aa730 100644
--- a/include/glib-compat.h
+++ b/include/glib-compat.h
@@ -30,6 +30,11 @@
 #pragma GCC diagnostic ignored "-Wdeprecated-declarations"
 
 #include 
+#if defined(G_OS_UNIX)
+#include 
+#include 
+#include 
+#endif
 
 /*
  * Note that because of the GLIB_VERSION_MAX_ALLOWED constant above, allowing
@@ -72,6 +77,27 @@
 gint g_poll_fixed(GPollFD *fds, guint nfds, gint timeout);
 #endif
 
+#if defined(G_OS_UNIX)
+/* Note: The fallback implementation is not MT-safe, and it returns a copy of
+ * the libc passwd (must be g_free() after use) but not the content. Because of
+ * these important differences the caller must be aware of, it's not #define 
for
+ * GLib API substitution. */
+static inline struct passwd *
+g_unix_get_passwd_entry_qemu(const gchar *user_name, GError **error)
+{
+#if GLIB_CHECK_VERSION(2, 64, 0)
+return g_unix_get_passwd_entry(user_name, error);
+#else
+struct passwd *p = getpwnam(user_name);
+if (!p) {
+g_set_error_literal(error, G_UNIX_ERROR, 0, g_strerror(errno));
+return NULL;
+}
+return (struct passwd *)g_memdup(p, sizeof(*p));
+#endif
+}
+#endif /* G_OS_UNIX */
+
 #pragma GCC diagnostic pop
 
 #endif
-- 
2.25.1

[PULL 11/12] meson: minor simplification

[Michael Roth]

From: Marc-André Lureau 

Signed-off-by: Marc-André Lureau 
Signed-off-by: Michael Roth 
---
 qga/meson.build | 7 +--
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/qga/meson.build b/qga/meson.build
index 6315bb357e..8340892139 100644
--- a/qga/meson.build
+++ b/qga/meson.build
@@ -22,12 +22,7 @@ qga_qapi_files = custom_target('QGA QAPI files',
depend_files: qapi_gen_depends)
 
 qga_ss = ss.source_set()
-i = 0
-foreach output: qga_qapi_outputs
-  qga_ss.add(qga_qapi_files[i])
-  i = i + 1
-endforeach
-
+qga_ss.add(qga_qapi_files.to_list())
 qga_ss.add(files(
   'commands.c',
   'guest-agent-command-state.c',
-- 
2.25.1

[PULL 09/12] qga: add ssh-{add,remove}-authorized-keys

[Michael Roth]

From: Marc-André Lureau 

Add new commands to add and remove SSH public keys from
~/.ssh/authorized_keys.

I took a different approach for testing, including the unit tests right
with the code. I wanted to overwrite the function to get the user
details, I couldn't easily do that over QMP. Furthermore, I prefer
having unit tests very close to the code, and unit files that are domain
specific (commands-posix is too crowded already). FWIW, that
coding/testing style is Rust-style (where tests can or should even be
part of the documentation!).

Fixes:
https://bugzilla.redhat.com/show_bug.cgi?id=1885332

Signed-off-by: Marc-André Lureau 
Reviewed-by: Michal Privoznik 
Reviewed-by: Daniel P. Berrangé 
*squashed in fix-ups for setting file ownership and use of QAPI
 conditionals for CONFIG_POSIX instead of stub definitions
Signed-off-by: Michael Roth 
---
 qga/commands-posix-ssh.c | 407 +++
 qga/meson.build  |  20 +-
 qga/qapi-schema.json |  35 
 3 files changed, 461 insertions(+), 1 deletion(-)
 create mode 100644 qga/commands-posix-ssh.c

diff --git a/qga/commands-posix-ssh.c b/qga/commands-posix-ssh.c
new file mode 100644
index 00..a7bc9a1c24
--- /dev/null
+++ b/qga/commands-posix-ssh.c
@@ -0,0 +1,407 @@
+ /*
+  * This work is licensed under the terms of the GNU GPL, version 2 or later.
+  * See the COPYING file in the top-level directory.
+  */
+#include "qemu/osdep.h"
+
+#include 
+#include 
+#include 
+#include 
+
+#include "qapi/error.h"
+#include "qga-qapi-commands.h"
+
+#ifdef QGA_BUILD_UNIT_TEST
+static struct passwd *
+test_get_passwd_entry(const gchar *user_name, GError **error)
+{
+struct passwd *p;
+int ret;
+
+if (!user_name || g_strcmp0(user_name, g_get_user_name())) {
+g_set_error(error, G_UNIX_ERROR, 0, "Invalid user name");
+return NULL;
+}
+
+p = g_new0(struct passwd, 1);
+p->pw_dir = (char *)g_get_home_dir();
+p->pw_uid = geteuid();
+p->pw_gid = getegid();
+
+ret = g_mkdir_with_parents(p->pw_dir, 0700);
+g_assert_cmpint(ret, ==, 0);
+
+return p;
+}
+
+#define g_unix_get_passwd_entry_qemu(username, err) \
+   test_get_passwd_entry(username, err)
+#endif
+
+static struct passwd *
+get_passwd_entry(const char *username, Error **errp)
+{
+g_autoptr(GError) err = NULL;
+struct passwd *p;
+
+ERRP_GUARD();
+
+p = g_unix_get_passwd_entry_qemu(username, &err);
+if (p == NULL) {
+error_setg(errp, "failed to lookup user '%s': %s",
+   username, err->message);
+return NULL;
+}
+
+return p;
+}
+
+static bool
+mkdir_for_user(const char *path, const struct passwd *p,
+   mode_t mode, Error **errp)
+{
+ERRP_GUARD();
+
+if (g_mkdir(path, mode) == -1) {
+error_setg(errp, "failed to create directory '%s': %s",
+   path, g_strerror(errno));
+return false;
+}
+
+if (chown(path, p->pw_uid, p->pw_gid) == -1) {
+error_setg(errp, "failed to set ownership of directory '%s': %s",
+   path, g_strerror(errno));
+return false;
+}
+
+if (chmod(path, mode) == -1) {
+error_setg(errp, "failed to set permissions of directory '%s': %s",
+   path, g_strerror(errno));
+return false;
+}
+
+return true;
+}
+
+static bool
+check_openssh_pub_key(const char *key, Error **errp)
+{
+ERRP_GUARD();
+
+/* simple sanity-check, we may want more? */
+if (!key || key[0] == '#' || strchr(key, '\n')) {
+error_setg(errp, "invalid OpenSSH public key: '%s'", key);
+return false;
+}
+
+return true;
+}
+
+static bool
+check_openssh_pub_keys(strList *keys, size_t *nkeys, Error **errp)
+{
+size_t n = 0;
+strList *k;
+
+ERRP_GUARD();
+
+for (k = keys; k != NULL; k = k->next) {
+if (!check_openssh_pub_key(k->value, errp)) {
+return false;
+}
+n++;
+}
+
+if (nkeys) {
+*nkeys = n;
+}
+return true;
+}
+
+static bool
+write_authkeys(const char *path, const GStrv keys,
+   const struct passwd *p, Error **errp)
+{
+g_autofree char *contents = NULL;
+g_autoptr(GError) err = NULL;
+
+ERRP_GUARD();
+
+contents = g_strjoinv("\n", keys);
+if (!g_file_set_contents(path, contents, -1, &err)) {
+error_setg(errp, "failed to write to '%s': %s", path, err->message);
+return false;
+}
+
+if (chown(path, p->pw_uid, p->pw_gid) == -1) {
+error_setg(errp, "failed to set ownership of directory '%s': %s",
+   path, g_strerror(errno));
+return false;
+}
+
+if (chmod(path, 0600) == -1) {
+error_setg(errp, "failed to set permissions of '%s': %s",
+   path, g_strerror(errno));
+return false;
+}
+
+return true;
+}
+
+static GStrv
+read_authkeys(const char *path, Error **errp)
+{
+g_autoptr(GError) err = NULL;
+g_autofr

[PULL 12/12] qga: add ssh-get-authorized-keys

[Michael Roth]

From: Marc-André Lureau 

Signed-off-by: Marc-André Lureau 
Signed-off-by: Michael Roth 
---
 qga/commands-posix-ssh.c | 66 
 qga/meson.build  | 11 +--
 qga/qapi-schema.json | 31 +++
 3 files changed, 106 insertions(+), 2 deletions(-)

diff --git a/qga/commands-posix-ssh.c b/qga/commands-posix-ssh.c
index f974bc4b64..4d75cb0113 100644
--- a/qga/commands-posix-ssh.c
+++ b/qga/commands-posix-ssh.c
@@ -268,6 +268,46 @@ qmp_guest_ssh_remove_authorized_keys(const char *username, 
strList *keys,
 write_authkeys(authkeys_path, new_keys, p, errp);
 }
 
+GuestAuthorizedKeys *
+qmp_guest_ssh_get_authorized_keys(const char *username, Error **errp)
+{
+g_autofree struct passwd *p = NULL;
+g_autofree char *authkeys_path = NULL;
+g_auto(GStrv) authkeys = NULL;
+g_autoptr(GuestAuthorizedKeys) ret = NULL;
+int i;
+
+ERRP_GUARD();
+
+p = get_passwd_entry(username, errp);
+if (p == NULL) {
+return NULL;
+}
+
+authkeys_path = g_build_filename(p->pw_dir, ".ssh",
+ "authorized_keys", NULL);
+authkeys = read_authkeys(authkeys_path, errp);
+if (authkeys == NULL) {
+return NULL;
+}
+
+ret = g_new0(GuestAuthorizedKeys, 1);
+for (i = 0; authkeys[i] != NULL; i++) {
+strList *new;
+
+g_strstrip(authkeys[i]);
+if (!authkeys[i][0] || authkeys[i][0] == '#') {
+continue;
+}
+
+new = g_new0(strList, 1);
+new->value = g_strdup(authkeys[i]);
+new->next = ret->keys;
+ret->keys = new;
+}
+
+return g_steal_pointer (&ret);
+}
 
 #ifdef QGA_BUILD_UNIT_TEST
 #if GLIB_CHECK_VERSION(2, 60, 0)
@@ -426,6 +466,31 @@ test_remove_keys(void)
"algo some-key another\n");
 }
 
+static void
+test_get_keys(void)
+{
+Error *err = NULL;
+static const char *authkeys =
+"algo key1 comments\n"
+"# a commented line\n"
+"algo some-key another\n";
+g_autoptr(GuestAuthorizedKeys) ret = NULL;
+strList *k;
+size_t len = 0;
+
+test_authorized_keys_set(authkeys);
+
+ret = qmp_guest_ssh_get_authorized_keys(g_get_user_name(), &err);
+g_assert_null(err);
+
+for (len = 0, k = ret->keys; k != NULL; k = k->next) {
+g_assert(g_str_has_prefix(k->value, "algo "));
+len++;
+}
+
+g_assert_cmpint(len, ==, 2);
+}
+
 int main(int argc, char *argv[])
 {
 setlocale(LC_ALL, "");
@@ -437,6 +502,7 @@ int main(int argc, char *argv[])
 g_test_add_func("/qga/ssh/add_keys", test_add_keys);
 g_test_add_func("/qga/ssh/add_reset_keys", test_add_reset_keys);
 g_test_add_func("/qga/ssh/remove_keys", test_remove_keys);
+g_test_add_func("/qga/ssh/get_keys", test_get_keys);
 
 return g_test_run();
 }
diff --git a/qga/meson.build b/qga/meson.build
index 8340892139..80e7487f32 100644
--- a/qga/meson.build
+++ b/qga/meson.build
@@ -90,8 +90,15 @@ test_env.set('G_TEST_SRCDIR', meson.current_source_dir())
 test_env.set('G_TEST_BUILDDIR', meson.current_build_dir())
 
 if 'CONFIG_POSIX' in config_host
-  qga_ssh_test = executable('qga-ssh-test',
-files('commands-posix-ssh.c'),
+  srcs = [files('commands-posix-ssh.c')]
+  i = 0
+  foreach output: qga_qapi_outputs
+if output.startswith('qga-qapi-types') or 
output.startswith('qga-qapi-visit')
+  srcs += qga_qapi_files[i]
+endif
+i = i + 1
+  endforeach
+  qga_ssh_test = executable('qga-ssh-test', srcs,
 dependencies: [qemuutil],
 c_args: ['-DQGA_BUILD_UNIT_TEST'])
 
diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
index 4ddea898fa..6ca85f995f 100644
--- a/qga/qapi-schema.json
+++ b/qga/qapi-schema.json
@@ -1347,6 +1347,37 @@
 { 'command': 'guest-get-devices',
   'returns': ['GuestDeviceInfo'] }
 
+##
+# @GuestAuthorizedKeys:
+#
+# @keys: public keys (in OpenSSH/sshd(8) authorized_keys format)
+#
+# Since: 5.2
+##
+{ 'struct': 'GuestAuthorizedKeys',
+  'data': {
+  'keys': ['str']
+  },
+  'if': 'defined(CONFIG_POSIX)' }
+
+
+##
+# @guest-ssh-get-authorized-keys:
+#
+# @username: the user account to add the authorized keys
+#
+# Return the public keys from user .ssh/authorized_keys on Unix systems (not
+# implemented for other systems).
+#
+# Returns: @GuestAuthorizedKeys
+#
+# Since: 5.2
+##
+{ 'command': 'guest-ssh-get-authorized-keys',
+  'data': { 'username': 'str' },
+  'returns': 'GuestAuthorizedKeys',
+  'if': 'defined(CONFIG_POSIX)' }
+
 ##
 # @guest-ssh-add-authorized-keys:
 #
-- 
2.25.1

[PULL 01/12] qga: Rename guest-get-devices return member 'address' to 'id'

[Michael Roth]

From: Markus Armbruster 

Member 'address' is union GuestDeviceAddress with a single branch
GuestDeviceAddressPCI, containing PCI vendor ID and device ID.  This
is not a PCI address.  Type GuestPCIAddress is.  Messed up in recent
commit 2e4211cee4 "qga: add command guest-get-devices for reporting
VirtIO devices".

Rename type GuestDeviceAddressPCI to GuestDeviceIdPCI, type
GuestDeviceAddress to GuestDeviceId, and member 'address' to 'id'.

Document the member properly while there.

Signed-off-by: Markus Armbruster 
Reviewed-by: Marc-André Lureau 
Reviewed-by: Philippe Mathieu-Daudé 
Signed-off-by: Michael Roth 
---
 qga/commands-win32.c | 16 
 qga/qapi-schema.json | 17 +
 2 files changed, 17 insertions(+), 16 deletions(-)

diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 0c3c05484f..879b02b6c3 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -2390,22 +2390,22 @@ GuestDeviceInfoList *qmp_guest_get_devices(Error **errp)
 }
 for (j = 0; hw_ids[j] != NULL; j++) {
 GMatchInfo *match_info;
-GuestDeviceAddressPCI *address;
+GuestDeviceIdPCI *id;
 if (!g_regex_match(device_pci_re, hw_ids[j], 0, &match_info)) {
 continue;
 }
 skip = false;
 
-address = g_new0(GuestDeviceAddressPCI, 1);
+id = g_new0(GuestDeviceIdPCI, 1);
 vendor_id = g_match_info_fetch(match_info, 1);
 device_id = g_match_info_fetch(match_info, 2);
-address->vendor_id = g_ascii_strtoull(vendor_id, NULL, 16);
-address->device_id = g_ascii_strtoull(device_id, NULL, 16);
+id->vendor_id = g_ascii_strtoull(vendor_id, NULL, 16);
+id->device_id = g_ascii_strtoull(device_id, NULL, 16);
 
-device->address = g_new0(GuestDeviceAddress, 1);
-device->has_address = true;
-device->address->type = GUEST_DEVICE_ADDRESS_KIND_PCI;
-device->address->u.pci.data = address;
+device->id = g_new0(GuestDeviceId, 1);
+device->has_id = true;
+device->id->type = GUEST_DEVICE_ID_KIND_PCI;
+device->id->u.pci.data = id;
 
 g_match_info_free(match_info);
 break;
diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
index cec98c7e06..f2c81cda2b 100644
--- a/qga/qapi-schema.json
+++ b/qga/qapi-schema.json
@@ -1257,26 +1257,26 @@
   'returns': 'GuestOSInfo' }
 
 ##
-# @GuestDeviceAddressPCI:
+# @GuestDeviceIdPCI:
 #
 # @vendor-id: vendor ID
 # @device-id: device ID
 #
 # Since: 5.2
 ##
-{ 'struct': 'GuestDeviceAddressPCI',
+{ 'struct': 'GuestDeviceIdPCI',
   'data': { 'vendor-id': 'uint16', 'device-id': 'uint16' } }
 
 ##
-# @GuestDeviceAddress:
+# @GuestDeviceId:
 #
-# Address of the device
-# - @pci: address of PCI device, since: 5.2
+# Id of the device
+# - @pci: PCI ID, since: 5.2
 #
 # Since: 5.2
 ##
-{ 'union': 'GuestDeviceAddress',
-  'data': { 'pci': 'GuestDeviceAddressPCI' } }
+{ 'union': 'GuestDeviceId',
+  'data': { 'pci': 'GuestDeviceIdPCI' } }
 
 ##
 # @GuestDeviceInfo:
@@ -1284,6 +1284,7 @@
 # @driver-name: name of the associated driver
 # @driver-date: driver release date in format -MM-DD
 # @driver-version: driver version
+# @id: device ID
 #
 # Since: 5.2
 ##
@@ -1292,7 +1293,7 @@
   'driver-name': 'str',
   '*driver-date': 'str',
   '*driver-version': 'str',
-  '*address': 'GuestDeviceAddress'
+  '*id': 'GuestDeviceId'
   } }
 
 ##
-- 
2.25.1

[PULL 04/12] qga: Flatten simple union GuestDeviceId

[Michael Roth]

From: Markus Armbruster 

Simple unions are simpler than flat unions in the schema, but more
complicated in C and on the QMP wire: there's extra indirection in C
and extra nesting on the wire, both pointless.  They should be avoided
in new code.

GuestDeviceId was recently added for guest-get-devices.  Convert it to
a flat union.

Signed-off-by: Markus Armbruster 
Reviewed-by: Marc-André Lureau 
Reviewed-by: Philippe Mathieu-Daudé 
Signed-off-by: Michael Roth 
---
 qga/commands-win32.c | 9 -
 qga/qapi-schema.json | 8 
 2 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 1efe3ba076..0c33d48aaa 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -2400,16 +2400,15 @@ GuestDeviceInfoList *qmp_guest_get_devices(Error **errp)
 }
 skip = false;
 
-id = g_new0(GuestDeviceIdPCI, 1);
 vendor_id = g_match_info_fetch(match_info, 1);
 device_id = g_match_info_fetch(match_info, 2);
-id->vendor_id = g_ascii_strtoull(vendor_id, NULL, 16);
-id->device_id = g_ascii_strtoull(device_id, NULL, 16);
 
 device->id = g_new0(GuestDeviceId, 1);
 device->has_id = true;
-device->id->type = GUEST_DEVICE_ID_KIND_PCI;
-device->id->u.pci.data = id;
+device->id->type = GUEST_DEVICE_TYPE_PCI;
+id = &device->id->u.pci;
+id->vendor_id = g_ascii_strtoull(vendor_id, NULL, 16);
+id->device_id = g_ascii_strtoull(device_id, NULL, 16);
 
 g_match_info_free(match_info);
 break;
diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
index c7bfb8bf6a..fe10631e4c 100644
--- a/qga/qapi-schema.json
+++ b/qga/qapi-schema.json
@@ -1256,6 +1256,12 @@
 { 'command': 'guest-get-osinfo',
   'returns': 'GuestOSInfo' }
 
+##
+# @GuestDeviceType:
+##
+{ 'enum': 'GuestDeviceType',
+  'data': [ 'pci' ] }
+
 ##
 # @GuestDeviceIdPCI:
 #
@@ -1276,6 +1282,8 @@
 # Since: 5.2
 ##
 { 'union': 'GuestDeviceId',
+  'base': { 'type': 'GuestDeviceType' },
+  'discriminator': 'type',
   'data': { 'pci': 'GuestDeviceIdPCI' } }
 
 ##
-- 
2.25.1

[PATCH] MAINTAINERS: update my email address

[Michael Roth]

I've recently switched employers and the current email address is out
of date.

Signed-off-by: Michael Roth 
---
 MAINTAINERS | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index ef6f5c7399..c72af7c2f5 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2395,7 +2395,7 @@ F: scripts/simplebench/
 
 QAPI
 M: Markus Armbruster 
-M: Michael Roth 
+M: Michael Roth 
 S: Supported
 F: qapi/
 X: qapi/*.json
@@ -2439,7 +2439,7 @@ F: tests/data/qobject/qdict.txt
 T: git https://repo.or.cz/qemu/armbru.git qapi-next
 
 QEMU Guest Agent
-M: Michael Roth 
+M: Michael Roth 
 S: Maintained
 F: qga/
 F: docs/interop/qemu-ga.rst
-- 
2.25.1

[PULL 03/12] qga-win: Fix guest-get-devices error API violations

[Michael Roth]

From: Markus Armbruster 

The Error ** argument must be NULL, &error_abort, &error_fatal, or a
pointer to a variable containing NULL.  Passing an argument of the
latter kind twice without clearing it in between is wrong: if the
first call sets an error, it no longer points to NULL for the second
call.

qmp_guest_get_devices() is wrong that way: it calls error_setg() in a
loop.

If no iteration fails, the function returns a value and sets no error.
Okay.

If exactly one iteration fails, the function returns a value and sets
an error.  Wrong.

If multiple iterations fail, the function trips error_setv()'s
assertion.

Fix it to return immediately on error.

Perhaps the failure to convert the driver version to UTF-8 should not
be an error.  We could simply not report the botched version string
instead.

Drop a superfluous continue while there.

Signed-off-by: Markus Armbruster 
Reviewed-by: Philippe Mathieu-Daudé 
Reviewed-by: Marc-André Lureau 
Signed-off-by: Michael Roth 
---
 qga/commands-win32.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index b01616a992..1efe3ba076 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -2385,7 +2385,7 @@ GuestDeviceInfoList *qmp_guest_get_devices(Error **errp)
 device->driver_name = g_utf16_to_utf8(name, -1, NULL, NULL, NULL);
 if (device->driver_name == NULL) {
 error_setg(errp, "conversion to utf8 failed (driver name)");
-continue;
+return NULL;
 }
 slog("querying device: %s", device->driver_name);
 hw_ids = ga_get_hardware_ids(dev_info_data.DevInst);
@@ -2428,7 +2428,7 @@ GuestDeviceInfoList *qmp_guest_get_devices(Error **errp)
 NULL, NULL);
 if (device->driver_version == NULL) {
 error_setg(errp, "conversion to utf8 failed (driver version)");
-continue;
+return NULL;
 }
 device->has_driver_version = true;
 
@@ -2452,7 +2452,6 @@ GuestDeviceInfoList *qmp_guest_get_devices(Error **errp)
 cur_item->next = item;
 cur_item = item;
 }
-continue;
 }
 
 if (dev_info != INVALID_HANDLE_VALUE) {
-- 
2.25.1

[PULL 05/12] qga: add command guest-get-disks

[Michael Roth]

From: Tomáš Golembiovský 

Add API and stubs for new guest-get-disks command.

The command guest-get-fsinfo can be used to list information about disks
and partitions but it is limited only to mounted disks with filesystem.
This new command should allow listing information about disks of the VM
regardles whether they are mounted or not. This can be usefull for
management applications for mapping virtualized devices or pass-through
devices to device names in the guest OS.

Signed-off-by: Tomáš Golembiovský 
Reviewed-by: Philippe Mathieu-Daudé 
Reviewed-by: Marc-André Lureau 
Signed-off-by: Michael Roth 
---
 qga/commands-posix.c |  6 ++
 qga/commands-win32.c |  6 ++
 qga/qapi-schema.json | 31 +++
 3 files changed, 43 insertions(+)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 3bffee99d4..422144bcff 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -3051,3 +3051,9 @@ GuestDeviceInfoList *qmp_guest_get_devices(Error **errp)
 
 return NULL;
 }
+
+GuestDiskInfoList *qmp_guest_get_disks(Error **errp)
+{
+error_setg(errp, QERR_UNSUPPORTED);
+return NULL;
+}
diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 0c33d48aaa..f7bdd5a8b5 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -2458,3 +2458,9 @@ GuestDeviceInfoList *qmp_guest_get_devices(Error **errp)
 }
 return head;
 }
+
+GuestDiskInfoList *qmp_guest_get_disks(Error **errp)
+{
+error_setg(errp, QERR_UNSUPPORTED);
+return NULL;
+}
diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
index fe10631e4c..e123a000be 100644
--- a/qga/qapi-schema.json
+++ b/qga/qapi-schema.json
@@ -865,6 +865,37 @@
'bus': 'int', 'target': 'int', 'unit': 'int',
'*serial': 'str', '*dev': 'str'} }
 
+##
+# @GuestDiskInfo:
+#
+# @name: device node (Linux) or device UNC (Windows)
+# @partition: whether this is a partition or disk
+# @dependents: list of dependent devices; e.g. for LVs of the LVM this will
+#  hold the list of PVs, for LUKS encrypted volume this will
+#  contain the disk where the volume is placed. (Linux)
+# @address: disk address information (only for non-virtual devices)
+# @alias: optional alias assigned to the disk, on Linux this is a name assigned
+# by device mapper
+#
+# Since 5.2
+##
+{ 'struct': 'GuestDiskInfo',
+  'data': {'name': 'str', 'partition': 'bool', 'dependents': ['str'],
+   '*address': 'GuestDiskAddress', '*alias': 'str'} }
+
+##
+# @guest-get-disks:
+#
+# Returns: The list of disks in the guest. For Windows these are only the
+#  physical disks. On Linux these are all root block devices of
+#  non-zero size including e.g. removable devices, loop devices,
+#  NBD, etc.
+#
+# Since: 5.2
+##
+{ 'command': 'guest-get-disks',
+  'returns': ['GuestDiskInfo'] }
+
 ##
 # @GuestFilesystemInfo:
 #
-- 
2.25.1

[PULL 06/12] qga: add implementation of guest-get-disks for Linux

[Michael Roth]

From: Tomáš Golembiovský 

The command lists all disks (real and virtual) as well as disk
partitions. For each disk the list of dependent disks is also listed and
/dev path is used as a handle so it can be matched with "name" field of
other returned disk entries. For disk partitions the "dependents" list
is populated with the the parent device for easier tracking of
hierarchy.

Example output:
{
  "return": [
...
{
  "name": "/dev/dm-0",
  "partition": false,
  "dependents": [
"/dev/sda2"
  ],
  "alias": "luks-7062202e-5b9b-433e-81e8-6628c40da9f7"
},
{
  "name": "/dev/sda2",
  "partition": true,
  "dependents": [
"/dev/sda"
  ]
},
{
  "name": "/dev/sda",
  "partition": false,
  "address": {
"serial": "SAMSUNG_MZ7LN512HCHP-000L1_S1ZKNXAG822493",
"bus-type": "sata",
...
"dev": "/dev/sda",
"target": 0
  },
  "dependents": []
},
...
  ]
}

Signed-off-by: Tomáš Golembiovský 
Reviewed-by: Marc-André Lureau 
Signed-off-by: Michael Roth 
---
 qga/commands-posix.c | 296 +--
 1 file changed, 285 insertions(+), 11 deletions(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 422144bcff..14683dfbd5 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -1150,13 +1150,27 @@ static void build_guest_fsinfo_for_virtual_device(char 
const *syspath,
 closedir(dir);
 }
 
+static bool is_disk_virtual(const char *devpath, Error **errp)
+{
+g_autofree char *syspath = realpath(devpath, NULL);
+
+if (!syspath) {
+error_setg_errno(errp, errno, "realpath(\"%s\")", devpath);
+return false;
+}
+return strstr(syspath, "/devices/virtual/block/") != NULL;
+}
+
 /* Dispatch to functions for virtual/real device */
 static void build_guest_fsinfo_for_device(char const *devpath,
   GuestFilesystemInfo *fs,
   Error **errp)
 {
-char *syspath = realpath(devpath, NULL);
+ERRP_GUARD();
+g_autofree char *syspath = NULL;
+bool is_virtual = false;
 
+syspath = realpath(devpath, NULL);
 if (!syspath) {
 error_setg_errno(errp, errno, "realpath(\"%s\")", devpath);
 return;
@@ -1167,16 +1181,281 @@ static void build_guest_fsinfo_for_device(char const 
*devpath,
 }
 
 g_debug("  parse sysfs path '%s'", syspath);
-
-if (strstr(syspath, "/devices/virtual/block/")) {
+is_virtual = is_disk_virtual(syspath, errp);
+if (*errp != NULL) {
+return;
+}
+if (is_virtual) {
 build_guest_fsinfo_for_virtual_device(syspath, fs, errp);
 } else {
 build_guest_fsinfo_for_real_device(syspath, fs, errp);
 }
+}
+
+#ifdef CONFIG_LIBUDEV
+
+/*
+ * Wrapper around build_guest_fsinfo_for_device() for getting just
+ * the disk address.
+ */
+static GuestDiskAddress *get_disk_address(const char *syspath, Error **errp)
+{
+g_autoptr(GuestFilesystemInfo) fs = NULL;
+
+fs = g_new0(GuestFilesystemInfo, 1);
+build_guest_fsinfo_for_device(syspath, fs, errp);
+if (fs->disk != NULL) {
+return g_steal_pointer(&fs->disk->value);
+}
+return NULL;
+}
+
+static char *get_alias_for_syspath(const char *syspath)
+{
+struct udev *udev = NULL;
+struct udev_device *udevice = NULL;
+char *ret = NULL;
+
+udev = udev_new();
+if (udev == NULL) {
+g_debug("failed to query udev");
+goto out;
+}
+udevice = udev_device_new_from_syspath(udev, syspath);
+if (udevice == NULL) {
+g_debug("failed to query udev for path: %s", syspath);
+goto out;
+} else {
+const char *alias = udev_device_get_property_value(
+udevice, "DM_NAME");
+/*
+ * NULL means there was an error and empty string means there is no
+ * alias. In case of no alias we return NULL instead of empty string.
+ */
+if (alias == NULL) {
+g_debug("failed to query udev for device alias for: %s",
+syspath);
+} else if (*alias != 0) {
+ret = g_strdup(alias);
+}
+}
+
+out:
+udev_unref(udev);
+udev_device_unref(udevice);
+return ret;
+}
+
+static char *get_device_for_syspath(const char *syspath)
+{
+struct udev *udev = NULL;
+struct udev_device *udevice = NULL;
+char *ret = NULL;
+
+udev = udev_new();
+if (udev == NULL) {
+g_debug("failed to query udev");
+goto out;
+}
+udevice = udev_device_new_from_syspath(udev, syspath);
+if (udevice == NULL) {
+g_debug("failed to query udev for path: %s", syspath);
+goto out;
+} else {
+ret = g_strdup(udev_device_get_devnode(udevice));
+}
+
+out:
+udev_unref(udev);
+udev_device_unref(udevice);
+return ret;
+}
+
+static void get_disk_deps(const char *disk_dir, GuestDiskInfo *disk)
+{
+   

[PULL 07/12] qga: add implementation of guest-get-disks for Windows

[Michael Roth]

From: Tomáš Golembiovský 

The command lists all the physical disk drives. Unlike for Linux
partitions and virtual volumes are not listed.

Example output:

{
  "return": [
{
  "name": ".\\PhysicalDrive0",
  "partition": false,
  "address": {
"serial": "QM1",
"bus-type": "sata",
...
  },
  "dependents": []
}
  ]
}

Signed-off-by: Tomáš Golembiovský 
Signed-off-by: Michael Roth 
---
 qga/commands-win32.c | 107 ---
 1 file changed, 101 insertions(+), 6 deletions(-)

diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index f7bdd5a8b5..300b87c859 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -979,6 +979,101 @@ out:
 return list;
 }
 
+GuestDiskInfoList *qmp_guest_get_disks(Error **errp)
+{
+ERRP_GUARD();
+GuestDiskInfoList *new = NULL, *ret = NULL;
+HDEVINFO dev_info;
+SP_DEVICE_INTERFACE_DATA dev_iface_data;
+int i;
+
+dev_info = SetupDiGetClassDevs(&GUID_DEVINTERFACE_DISK, 0, 0,
+DIGCF_PRESENT | DIGCF_DEVICEINTERFACE);
+if (dev_info == INVALID_HANDLE_VALUE) {
+error_setg_win32(errp, GetLastError(), "failed to get device tree");
+return NULL;
+}
+
+g_debug("enumerating devices");
+dev_iface_data.cbSize = sizeof(SP_DEVICE_INTERFACE_DATA);
+for (i = 0;
+SetupDiEnumDeviceInterfaces(dev_info, NULL, &GUID_DEVINTERFACE_DISK,
+i, &dev_iface_data);
+i++) {
+GuestDiskAddress *address = NULL;
+GuestDiskInfo *disk = NULL;
+Error *local_err = NULL;
+g_autofree PSP_DEVICE_INTERFACE_DETAIL_DATA
+pdev_iface_detail_data = NULL;
+STORAGE_DEVICE_NUMBER sdn;
+HANDLE dev_file;
+DWORD size = 0;
+BOOL result;
+int attempt;
+
+g_debug("  getting device path");
+for (attempt = 0, result = FALSE; attempt < 2 && !result; attempt++) {
+result = SetupDiGetDeviceInterfaceDetail(dev_info,
+&dev_iface_data, pdev_iface_detail_data, size, &size, NULL);
+if (result) {
+break;
+}
+if (GetLastError() == ERROR_INSUFFICIENT_BUFFER) {
+pdev_iface_detail_data = g_realloc(pdev_iface_detail_data,
+size);
+pdev_iface_detail_data->cbSize =
+sizeof(*pdev_iface_detail_data);
+} else {
+g_debug("failed to get device interface details");
+break;
+}
+}
+if (!result) {
+g_debug("skipping device");
+continue;
+}
+
+g_debug("  device: %s", pdev_iface_detail_data->DevicePath);
+dev_file = CreateFile(pdev_iface_detail_data->DevicePath, 0,
+FILE_SHARE_READ, NULL, OPEN_EXISTING, 0, NULL);
+if (!DeviceIoControl(dev_file, IOCTL_STORAGE_GET_DEVICE_NUMBER,
+NULL, 0, &sdn, sizeof(sdn), &size, NULL)) {
+CloseHandle(dev_file);
+debug_error("failed to get storage device number");
+continue;
+}
+CloseHandle(dev_file);
+
+disk = g_new0(GuestDiskInfo, 1);
+disk->name = g_strdup_printf(".\\PhysicalDrive%lu",
+sdn.DeviceNumber);
+
+g_debug("  number: %lu", sdn.DeviceNumber);
+address = g_malloc0(sizeof(GuestDiskAddress));
+address->has_dev = true;
+address->dev = g_strdup(disk->name);
+get_single_disk_info(sdn.DeviceNumber, address, &local_err);
+if (local_err) {
+g_debug("failed to get disk info: %s",
+error_get_pretty(local_err));
+error_free(local_err);
+qapi_free_GuestDiskAddress(address);
+address = NULL;
+} else {
+disk->address = address;
+disk->has_address = true;
+}
+
+new = g_malloc0(sizeof(GuestDiskInfoList));
+new->value = disk;
+new->next = ret;
+ret = new;
+}
+
+SetupDiDestroyDeviceInfoList(dev_info);
+return ret;
+}
+
 #else
 
 static GuestDiskAddressList *build_guest_disk_info(char *guid, Error **errp)
@@ -986,6 +1081,12 @@ static GuestDiskAddressList *build_guest_disk_info(char 
*guid, Error **errp)
 return NULL;
 }
 
+GuestDiskInfoList *qmp_guest_get_disks(Error **errp)
+{
+error_setg(errp, QERR_UNSUPPORTED);
+return NULL;
+}
+
 #endif /* CONFIG_QGA_NTDDSCSI */
 
 static GuestFilesystemInfo *build_guest_fsinfo(char *guid, Error **errp)
@@ -2458,9 +2559,3 @@ GuestDeviceInfoList *qmp_guest_get_devices(Error **errp)
 }
 return head;
 }
-
-GuestDiskInfoList *qmp_guest_get_disks(Error **errp)
-{
-error_setg(errp, QERR_UNSUPPORTED);
-return NULL;
-}
-- 
2.25.1

Re: [PULL 00/12] qemu-ga patch queue for soft-freeze

[no-reply]

Patchew URL: 
https://patchew.org/QEMU/20201027055317.351868-1-michael.r...@amd.com/



Hi,

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

Type: series
Message-id: 20201027055317.351868-1-michael.r...@amd.com
Subject: [PULL 00/12] qemu-ga patch queue for soft-freeze

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

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 - [tag update]  
patchew/20201010204106.1368710-1-marcandre.lur...@redhat.com -> 
patchew/20201010204106.1368710-1-marcandre.lur...@redhat.com
 - [tag update]  patchew/20201026195131.13848-1-js...@redhat.com -> 
patchew/20201026195131.13848-1-js...@redhat.com
 - [tag update]  patchew/20201027050556.269064-1-ebl...@redhat.com -> 
patchew/20201027050556.269064-1-ebl...@redhat.com
 * [new tag] patchew/20201027055317.351868-1-michael.r...@amd.com -> 
patchew/20201027055317.351868-1-michael.r...@amd.com
Switched to a new branch 'test'
d9477b5 qga: add ssh-get-authorized-keys
22a meson: minor simplification
4c591be qga: add *reset argument to ssh-add-authorized-keys
4857cb4 qga: add ssh-{add,remove}-authorized-keys
8ec1e2c glib-compat: add g_unix_get_passwd_entry_qemu()
20a139e qga: add implementation of guest-get-disks for Windows
4b631f0 qga: add implementation of guest-get-disks for Linux
7ae8fe2 qga: add command guest-get-disks
0b92ab2 qga: Flatten simple union GuestDeviceId
46c759d qga-win: Fix guest-get-devices error API violations
be683aa qga: Use common time encoding for guest-get-devices 'driver-date'
c9ae53d qga: Rename guest-get-devices return member 'address' to 'id'

=== OUTPUT BEGIN ===
1/12 Checking commit c9ae53d3e18d (qga: Rename guest-get-devices return member 
'address' to 'id')
2/12 Checking commit be683aab37bc (qga: Use common time encoding for 
guest-get-devices 'driver-date')
3/12 Checking commit 46c759dcdc69 (qga-win: Fix guest-get-devices error API 
violations)
4/12 Checking commit 0b92ab2b2592 (qga: Flatten simple union GuestDeviceId)
5/12 Checking commit 7ae8fe2e031c (qga: add command guest-get-disks)
6/12 Checking commit 4b631f069f98 (qga: add implementation of guest-get-disks 
for Linux)
7/12 Checking commit 20a139e50765 (qga: add implementation of guest-get-disks 
for Windows)
8/12 Checking commit 8ec1e2cbda3f (glib-compat: add 
g_unix_get_passwd_entry_qemu())
WARNING: Block comments use a leading /* on a separate line
#42: FILE: include/glib-compat.h:81:
+/* Note: The fallback implementation is not MT-safe, and it returns a copy of

WARNING: Block comments use a trailing */ on a separate line
#45: FILE: include/glib-compat.h:84:
+ * GLib API substitution. */

total: 0 errors, 2 warnings, 38 lines checked

Patch 8/12 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
9/12 Checking commit 4857cb4903be (qga: add ssh-{add,remove}-authorized-keys)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#30: 
new file mode 100644

ERROR: Use g_assert or g_assert_not_reached
#67: FILE: qga/commands-posix-ssh.c:33:
+g_assert_cmpint(ret, ==, 0);

ERROR: Use g_assert or g_assert_not_reached
#328: FILE: qga/commands-posix-ssh.c:294:
+g_assert_cmpint(ret, ==, 0);

ERROR: Use g_assert or g_assert_not_reached
#333: FILE: qga/commands-posix-ssh.c:299:
+g_assert_no_error(err);

ERROR: Use g_assert or g_assert_not_reached
#345: FILE: qga/commands-posix-ssh.c:311:
+g_assert_no_error(err);

ERROR: Use g_assert or g_assert_not_reached
#347: FILE: qga/commands-posix-ssh.c:313:
+g_assert_cmpstr(contents, ==, expected);

ERROR: Use g_assert or g_assert_not_reached
#384: FILE: qga/commands-posix-ssh.c:350:
+g_assert_null(err);

ERROR: Use g_assert or g_assert_not_reached
#390: FILE: qga/commands-posix-ssh.c:356:
+g_assert_null(err);

ERROR: Use g_assert or g_assert_not_reached
#411: FILE: qga/commands-posix-ssh.c:377:
+g_assert_null(err);

ERROR: Use g_assert or g_assert_not_reached
#416: FILE: qga/commands-posix-ssh.c:382:
+g_assert_null(err);

total: 9 errors, 1 warnings, 474 lines checked

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

10/12 Checking commit 4c591be51737 (qga: add *reset argument to 
ssh-add-authorized-keys)
ERROR: Use g_assert or g_assert_not_reached
#96: FILE: qga/commands-posix-ssh.c:381:
+g_assert_null(err);

ERROR: Use g_assert or g_assert_not_reached
#106: FILE: qga/commands-posix-ssh.c:391:
+g_assert_null(err);

ERROR: Use g_assert or g_assert_not_reached
#115: FILE: qga/commands-posix-ssh.c:400:
+g_assert_null(err);

total: 3 errors, 

Re: [PATCH 6/9] dev-serial: add always-plugged property to ensure USB device is always attached

[Gerd Hoffmann]

>  case CHR_EVENT_OPENED:
> -if (!s->dev.attached) {
> +if (!s->always_plugged && !s->dev.attached) {
>  usb_device_attach(&s->dev, &error_abort);
>  }

Not needed (but doesn't hurt either).

>  break;
>  case CHR_EVENT_CLOSED:
> -if (s->dev.attached) {
> +if (!s->always_plugged && s->dev.attached) {
>  usb_device_detach(&s->dev);
>  }

Ok.

> -if (qemu_chr_fe_backend_open(&s->cs) && !dev->attached) {
> +if (s->always_plugged || (qemu_chr_fe_backend_open(&s->cs) &&
> +  !dev->attached)) {

The dev->attached check should not be skipped, i.e. the logic should be
((always_plugged || open) && !attached).

take care,
  Gerd

[PATCH] virtiofsd: Fix the help message of posix lock

[Jiachen Zhang]

The commit 88fc107956a5812649e5918e0c092d3f78bb28ad disabled remote
posix locks by default. But the --help message still says it is enabled
by default. So fix it to output no_posix_lock.

Signed-off-by: Jiachen Zhang 
---
 tools/virtiofsd/helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/virtiofsd/helper.c b/tools/virtiofsd/helper.c
index 85770d63f1..574dd09e91 100644
--- a/tools/virtiofsd/helper.c
+++ b/tools/virtiofsd/helper.c
@@ -161,7 +161,7 @@ void fuse_cmdline_help(void)
"   allowed (default: 10)\n"
"-o posix_lock|no_posix_lock\n"
"   enable/disable remote posix lock\n"
-   "   default: posix_lock\n"
+   "   default: no_posix_lock\n"
"-o readdirplus|no_readdirplus\n"
"   enable/disable readirplus\n"
"   default: readdirplus except with "
-- 
2.20.1

Re: [PATCH 0/2] hw/intc/bcm283x: Trivial tracing cleanup

[Philippe Mathieu-Daudé]

On Mon, Oct 26, 2020 at 6:07 PM Philippe Mathieu-Daudé  wrote:
>
> ping?

Oops wrong series, these patches are already merged, sorry.

>
> On 10/17/20 8:07 PM, Philippe Mathieu-Daudé wrote:
> > Add trace event for IRQ from CPU/GPU,
> > use definitions for IRQ numbers.
> >
> > Philippe Mathieu-Daudé (2):
> >hw/intc/bcm2835_ic: Trace GPU/CPU IRQ handlers
> >hw/intc/bcm2836_control: Use IRQ definitions instead of magic numbers

Re: [PATCH 12/15] s390: remove bios_name

[Cornelia Huck]

On Mon, 26 Oct 2020 10:30:25 -0400
Paolo Bonzini  wrote:

> Cc: Thomas Huth 
> Signed-off-by: Paolo Bonzini 
> ---
>  hw/s390x/ipl.c | 8 ++--
>  hw/s390x/s390-virtio-ccw.c | 3 ++-
>  2 files changed, 4 insertions(+), 7 deletions(-)

Reviewed-by: Cornelia Huck 

Re: [PATCH 12/15] s390: remove bios_name

[Christian Borntraeger]

On 26.10.20 15:30, Paolo Bonzini wrote:
> Cc: Thomas Huth 
> Signed-off-by: Paolo Bonzini 
> ---
>  hw/s390x/ipl.c | 8 ++--
>  hw/s390x/s390-virtio-ccw.c | 3 ++-
>  2 files changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
> index 3d2652d75a..61e8c967d3 100644
> --- a/hw/s390x/ipl.c
> +++ b/hw/s390x/ipl.c
> @@ -128,11 +128,7 @@ static void s390_ipl_realize(DeviceState *dev, Error 
> **errp)
>  if (!ipl->kernel || ipl->enforce_bios) {
>  uint64_t fwbase = (MIN(ram_size, 0x8000U) - 0x20) & 
> ~0xUL;
>  
> -if (bios_name == NULL) {
> -bios_name = ipl->firmware;
> -}
> -
> -bios_filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
> +bios_filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, ipl->firmware);
>  if (bios_filename == NULL) {
>  error_setg(errp, "could not find stage1 bootloader");
>  return;
> @@ -154,7 +150,7 @@ static void s390_ipl_realize(DeviceState *dev, Error 
> **errp)
>  g_free(bios_filename);
>  
>  if (bios_size == -1) {
> -error_setg(errp, "could not load bootloader '%s'", bios_name);
> +error_setg(errp, "could not load bootloader '%s'", 
> ipl->firmware);
>  return;
>  }
>  
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index e52182f946..a521eba673 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -258,7 +258,8 @@ static void ccw_init(MachineState *machine)
>  /* get a BUS */
>  css_bus = virtual_css_bus_init();
>  s390_init_ipl_dev(machine->kernel_filename, machine->kernel_cmdline,
> -  machine->initrd_filename, "s390-ccw.img",
> +  machine->initrd_filename,
> +  machine->firmware ?: "s390-ccw.img",

Adding the elvis operator is actually a fix, no?

Re: [PATCH 3/3] console: make QMP/HMP screendump run in coroutine

[Markus Armbruster]

marcandre.lur...@redhat.com writes:

> From: Marc-André Lureau 
>
> Thanks to the monitors coroutine support, the screendump handler can

monitors'

Suggest to add (merge commit b7092cda1b3) right before the comma.

> trigger a graphic_hw_update(), yield and let the main loop run until
> update is done. Then the handler is resumed, and ppm_save() will write
> the screen image to disk in the coroutine context (thus non-blocking).
>
> Potentially, during non-blocking write, some new graphic update could
> happen, and thus the image may have some glitches. Whether that
> behaviour is acceptable is discutable. Allocating new memory may not be

s/discutable/debatable/

> a good idea, as framebuffers can be quite large. Even then, QEMU may
> become less responsive as it requires paging in etc.

Tradeoff.  I'm okay with "simple & efficient, but might glitch".  It
should be documented, though.  Followup patch is fine.

> Related to:
> https://bugzilla.redhat.com/show_bug.cgi?id=1230527
>
> Signed-off-by: Marc-André Lureau 
> ---
>  hmp-commands.hx|  1 +
>  monitor/hmp-cmds.c |  3 ++-
>  qapi/ui.json   |  3 ++-
>  ui/console.c   | 27 ---
>  4 files changed, 29 insertions(+), 5 deletions(-)
>
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index cd068389de..ff2d7aa8f3 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -254,6 +254,7 @@ ERST
>  .help   = "save screen from head 'head' of display device 
> 'device' "
>"into PPM image 'filename'",
>  .cmd= hmp_screendump,
> +.coroutine  = true,
>  },
>  
>  SRST
> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> index 9789f4277f..91608bac6d 100644
> --- a/monitor/hmp-cmds.c
> +++ b/monitor/hmp-cmds.c
> @@ -1756,7 +1756,8 @@ err_out:
>  goto out;
>  }
>  
> -void hmp_screendump(Monitor *mon, const QDict *qdict)
> +void coroutine_fn
> +hmp_screendump(Monitor *mon, const QDict *qdict)
>  {
>  const char *filename = qdict_get_str(qdict, "filename");
>  const char *id = qdict_get_try_str(qdict, "device");
> diff --git a/qapi/ui.json b/qapi/ui.json
> index 9d6721037f..6c7b33cb72 100644
> --- a/qapi/ui.json
> +++ b/qapi/ui.json
> @@ -98,7 +98,8 @@
>  #
>  ##
>  { 'command': 'screendump',
> -  'data': {'filename': 'str', '*device': 'str', '*head': 'int'} }
> +  'data': {'filename': 'str', '*device': 'str', '*head': 'int'},
> +  'coroutine': true }
>  
>  ##
>  # == Spice
> diff --git a/ui/console.c b/ui/console.c
> index a56fe0dd26..0118f70d9a 100644
> --- a/ui/console.c
> +++ b/ui/console.c
> @@ -168,6 +168,7 @@ struct QemuConsole {
>  QEMUFIFO out_fifo;
>  uint8_t out_fifo_buf[16];
>  QEMUTimer *kbd_timer;
> +CoQueue dump_queue;
>  
>  QTAILQ_ENTRY(QemuConsole) next;
>  };
> @@ -263,6 +264,7 @@ static void gui_setup_refresh(DisplayState *ds)
>  
>  void graphic_hw_update_done(QemuConsole *con)
>  {
> +qemu_co_queue_restart_all(&con->dump_queue);
>  }
>  
>  void graphic_hw_update(QemuConsole *con)
> @@ -340,8 +342,15 @@ static bool ppm_save(int fd, pixman_image_t *image, 
> Error **errp)
>  return true;
>  }
>  
> -void qmp_screendump(const char *filename, bool has_device, const char 
> *device,
> -bool has_head, int64_t head, Error **errp)
> +static void graphic_hw_update_bh(void *con)
> +{
> +graphic_hw_update(con);
> +}
> +
> +/* Safety: coroutine-only, concurrent-coroutine safe, main thread only */
> +void coroutine_fn
> +qmp_screendump(const char *filename, bool has_device, const char *device,
> +   bool has_head, int64_t head, Error **errp)
>  {
>  g_autoptr(pixman_image_t) image = NULL;
>  QemuConsole *con;
> @@ -366,7 +375,15 @@ void qmp_screendump(const char *filename, bool 
> has_device, const char *device,
>  }
>  }
>  
> -graphic_hw_update(con);
> +if (qemu_co_queue_empty(&con->dump_queue)) {
> +/* Defer the update, it will restart the pending coroutines */
> +aio_bh_schedule_oneshot(qemu_get_aio_context(),
> +graphic_hw_update_bh, con);
> +}
> +qemu_co_queue_wait(&con->dump_queue, NULL);
> +
> +/* All pending coroutines are woken up, while BQL taken, no further 
> graphic
> + * update are possible until it is released, take an image ref before 
> that. */

"while BQL taken": I guess you mean "while the BQL is held".

Style nit: CODING_STYLE.rst asks for wings.

Recommend to limit comment line length for readability.

Recommend to turn a few commas into periods.

Together:

/*
 * All pending coroutines are woken up, while the BQL is held.  No
 * further graphic update are possible until it is released.  Take
 * an image ref before that.
 */

>  surface = qemu_console_surface(con);
>  if (!surface) {
>  error_setg(errp, "no surface");
> @@ -381,6 +398,9 @@ void qmp_screendump(const char *filename, bool 
> has_device, const char *device,
>  re

Re: [PATCH] virtiofsd: Fix the help message of posix lock

[Philippe Mathieu-Daudé]

On 10/27/20 9:15 AM, Jiachen Zhang wrote:
> The commit 88fc107956a5812649e5918e0c092d3f78bb28ad disabled remote
> posix locks by default. But the --help message still says it is enabled
> by default. So fix it to output no_posix_lock.
> 
> Signed-off-by: Jiachen Zhang 

Reviewed-by: Philippe Mathieu-Daudé 

> ---
>  tools/virtiofsd/helper.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/virtiofsd/helper.c b/tools/virtiofsd/helper.c
> index 85770d63f1..574dd09e91 100644
> --- a/tools/virtiofsd/helper.c
> +++ b/tools/virtiofsd/helper.c
> @@ -161,7 +161,7 @@ void fuse_cmdline_help(void)
> "   allowed (default: 10)\n"
> "-o posix_lock|no_posix_lock\n"
> "   enable/disable remote posix 
> lock\n"
> -   "   default: posix_lock\n"
> +   "   default: no_posix_lock\n"
> "-o readdirplus|no_readdirplus\n"
> "   enable/disable readirplus\n"
> "   default: readdirplus except with "
> 

Re: [PATCH 4/4] spapr: Improve spapr_reallocate_hpt() error reporting

[Philippe Mathieu-Daudé]

On 10/26/20 3:47 PM, Greg Kurz wrote:
> On Mon, 26 Oct 2020 14:49:34 +0100
> Philippe Mathieu-Daudé  wrote:
> 
>> On 10/26/20 1:40 PM, Greg Kurz wrote:
>>> spapr_reallocate_hpt() has three users, two of which pass &error_fatal
>>> and the third one, htab_load(), passes &local_err, uses it to detect
>>> failures and simply propagates -EINVAL up to vmstate_load(), which will
>>> cause QEMU to exit. It is thus confusing that spapr_reallocate_hpt()
>>> doesn't return right away when an error is detected in some cases. Also,
>>> the comment suggesting that the caller is welcome to try to carry on
>>> seems like a remnant in this respect.
>>>
>>> This can be improved:
>>> - change spapr_reallocate_hpt() to always report a negative errno on
>>>failure, either as reported by KVM or -ENOSPC if the HPT is smaller
>>>than what was asked,
>>> - use that to detect failures in htab_load() which is preferred over
>>>checking &local_err,
>>> - propagate this negative errno to vmstate_load() because it is more
>>>accurate than propagating -EINVAL for all possible errors.
>>>
>>> Signed-off-by: Greg Kurz 
...

>>>   if (rc < 0) {
>>> @@ -1504,8 +1503,7 @@ void spapr_reallocate_hpt(SpaprMachineState *spapr, 
>>> int shift,
>>>   error_setg_errno(errp, errno, "Failed to allocate KVM HPT of 
>>> order %d",
>>>shift);
>>>   error_append_hint(errp, "Try smaller maxmem?\n");
>>> -/* This is almost certainly fatal, but if the caller really
>>> - * wants to carry on with shift == 0, it's welcome to try */
>>> +return -errno;
>>
>> Maybe returning here should be in a previous patch.
>> Otherwise patch looks good.
>>
> 
> It could have been indeed...
> 
>>>   } else if (rc > 0) {
>>>   /* kernel-side HPT allocated */
>>>   if (rc != shift) {
>>> @@ -1513,6 +1511,7 @@ void spapr_reallocate_hpt(SpaprMachineState *spapr, 
>>> int shift,
>>>  "Requested order %d HPT, but kernel allocated 
>>> order %ld",
>>>  shift, rc);
>>>   error_append_hint(errp, "Try smaller maxmem?\n");
>>> +return -ENOSPC;
> 
> ... along with this one.
> 
> I didn't go this way because it doesn't really affect the final behavior since
> QEMU exits in all cases. It's mostly about propagating an appropriate errno up
> to VMState in the case of htab_load(). But if you find it clearer and I need
> to post a v2, I can certainly do that.

As a reviewer I prefer dumb obvious patches I can quickly understand.
If I stop, spend too long, am not sure, I spend time to ask, and usually
stop reviewing the series. Then you spend time to answer, eventually
respin. In doubt, KISS.

Patch is queued so no need for v2.

Re: [PATCH v3 00/15] raspi: add the bcm2835 cprman clock manager

[Philippe Mathieu-Daudé]

Hi Peter,

On 10/19/20 9:31 PM, Peter Maydell wrote:
> On Mon, 19 Oct 2020 at 16:45, Peter Maydell  wrote:
>>
>> On Sat, 10 Oct 2020 at 14:57, Luc Michel  wrote:
>>>
>>> v2 -> v3:
>>>   - patch 03: moved clock_new definition to hw/core/clock.c [Phil]
>>>   - patch 03: commit message typo [Clement]
>>>   - patch 10: clarifications around the CM_CTL/CM_DIBV mux registers.
>>>   reg_cm replaced with reg_ctl and reg_div. Add some
>>>   comments for clarity. [Phil]
>>>   - patch 10: fixed update_mux_from_cm not matching the CM_DIV offset
>>>   correctly. [Phil]
>>>   - patch 11: replaced manual bitfield extraction with extract32 [Phil]
>>>   - patch 11: added a visual representation of CM_DIV for clarity [Phil]
>>>   - patch 11: added a missing return in clock_mux_update.
>>
>>
>>
>> Applied to target-arm.next, thanks.
> 
> Dropped again due to segv in 'make check' when running with
> clang sanitizer, which I gather from irc that you're looking
> into.

The fix has been merged as commit a6e9b9123e7
("hw/core/qdev-clock: add a reference on aliased clocks")
and the series also got:
Tested-by: Guenter Roeck 

Hopefully it will make it for 5.2 :)

Thanks,

Phil.

Re: [PATCH 2/9] dev-serial: use USB_SERIAL QOM macro for USBSerialState assignments

[Philippe Mathieu-Daudé]

On 10/26/20 9:33 AM, Mark Cave-Ayland wrote:
> Signed-off-by: Mark Cave-Ayland 
> ---
>  hw/usb/dev-serial.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé 

Re: [PULL 00/13] 9p queue 2020-10-23

[Dr. David Alan Gilbert]

* Greg Kurz (gr...@kaod.org) wrote:
> On Mon, 26 Oct 2020 13:48:37 +0100
> Christian Schoenebeck  wrote:
> 
> > On Montag, 26. Oktober 2020 11:33:42 CET Peter Maydell wrote:
> > > On Fri, 23 Oct 2020 at 12:46, Christian Schoenebeck
> > > 
> > >  wrote:
> > > > The following changes since commit 
> > > > 4c5b97bfd0dd54dc27717ae8d1cd10e14eef1430:
> > > >   Merge remote-tracking branch
> > > >   'remotes/kraxel/tags/modules-20201022-pull-request' into staging
> > > >   (2020-10-22 12:33:21 +0100)> 
> > > > are available in the Git repository at:
> > > >   https://github.com/cschoenebeck/qemu.git tags/pull-9p-20201023
> > > > 
> > > > for you to fetch changes up to ee01926a11b1f9bffcd6cdec0961dd9d1882da71:
> > > >   tests/9pfs: add local Tunlinkat hard link test (2020-10-22 20:26:33
> > > >   +0200)
> > > > 
> > > > 
> > > > 9pfs: more tests using local fs driver
> > > > 
> > > > Only 9pfs test case changes this time:
> > > > 
> > > > * Refactor: Rename functions to make top-level test functions fs_*()
> > > > 
> > > >   easily distinguishable from utility test functions do_*().
> > > > 
> > > > * Refactor: Drop unnecessary function arguments in utility test
> > > > 
> > > >   functions.
> > > > 
> > > > * More test cases using the 9pfs 'local' filesystem driver backend,
> > > > 
> > > >   namely for the following 9p requests: Tunlinkat, Tlcreate, Tsymlink
> > > >   and Tlink.
> > > > 
> > > > 
> > > 
> > > I get a 'make check' failure on x86-64 Linux host:
> > > 
> > > PASS 54 qtest-x86_64: qos-test
> > > /x86_64/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-9p-pci/virtio-9p/virtio-
> > > 9p-tests/local/config PASS 55 qtest-x86_64: qos-test
> > > /x86_64/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-9p-pci/virtio-9p/virtio-
> > > 9p-tests/local/create_dir PASS 56 qtest-x86_64: qos-test
> > > /x86_64/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-9p-pci/virtio-9p/virtio-
> > > 9p-tests/local/unlinkat_dir PASS 57 qtest-x86_64: qos-test
> > > /x86_64/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-9p-pci/virtio-9p/virtio-
> > > 9p-tests/local/create_file PASS 58 qtest-x86_64: qos-test
> > > /x86_64/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-9p-pci/virtio-9p/virtio-
> > > 9p-tests/local/unlinkat_file PASS 59 qtest-x86_64: qos-test
> > > /x86_64/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-9p-pci/virtio-9p/virtio-
> > > 9p-tests/local/symlink_file Received response 7 (RLERROR) instead of 73
> > > (RMKDIR)
> > > Rlerror has errno 2 (No such file or directory)
> > > **
> > > ERROR:../../tests/qtest/virtio-9p-test.c:300:v9fs_req_recv: assertion
> > > failed (hdr.id == id): (7 == 73)
> 
> Not sure this is related to this PR actually. Dave Gilbert reported on irc 
> that
> he encountered a similar issue with 'make -j check', likely without these 
> patches.

I was running on current master as of yesterday; no 9p specific patches.

Dave

> > > ERROR qtest-x86_64: qos-test - Bail out!
> > > ERROR:../../tests/qtest/virtio-9ptest.c:300:v9fs_req_recv: assertion
> > > failed (hdr.id == id): (7 == 73)
> > > Makefile.mtest:3953: recipe for target 'run-test-492' failed
> > > 
> > > 
> > > thanks
> > > -- PMM
> > 
> > So the 9p server is already failing to create the test case directory
> > "./qtest-9p-local/05/" relative to your current working directory.
> > 
> > I would appreciate to get more info when you have some free cycles, as I'm
> > unable to reproduce this on any system unfortunately. But no hurry as
> > these tests only become relevant actually for QEMU 6.
> > 
> > What puzzles me is that the previous test cases succeeded there, which all
> > create their own test directory in the same way:
> > 
> > ./qtest-9p-local/01/
> > ./qtest-9p-local/02/  (<-- dir vanishes after that test completed)
> > ./qtest-9p-local/03/
> > ./qtest-9p-local/04/
> > ...
> > 
> > How does the "./qtest-9p-local/" directory look like after that
> > "local/symlink_file" test failed there? You can use this shortcut:
> > 
> > export QTEST_QEMU_BINARY=x86_64-softmmu/qemu-system-x86_64
> > cd build
> > tests/qtest/qos-test --verbose
> > ls -l qtest-9p-local
> > 
> > That latter qos-test run will also output the assembled qemu command
> > line the 9p local tests would run with, which might also be helpful,
> > e.g. the relevant output would be something like this:
> > 
> > GTest: run: 
> > /x86_64/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-9p-pci/virtio-9p/virtio-9p-tests/local/config
> > (MSG: starting QEMU: exec x86_64-softmmu/qemu-system-x86_64 -qtest 
> > unix:/tmp/qtest-7428.sock -qtest-log /dev/null -chardev 
> > socket,path=/tmp/qtest-7428.qmp,id=char0 -mon chardev=char0,mode=control 
> > -display none -M pc  -fsdev 
> > local,id=fsdev0,path='/home/me/git/qemu/build/qtest-9p-local',security_model=mapped-xattr
> >  -device virtio-9p-pci,fsdev=fsdev0,addr=04.0,mount_tag=qtest -accel qtest)
> > 
> > Would prob

Re: [PATCH 3/9] dev-serial: convert from DPRINTF to trace-events

[Philippe Mathieu-Daudé]

On 10/26/20 9:33 AM, Mark Cave-Ayland wrote:
> Signed-off-by: Mark Cave-Ayland 
> ---
>  hw/usb/dev-serial.c | 28 ++--
>  hw/usb/trace-events |  8 
>  2 files changed, 22 insertions(+), 14 deletions(-)
> 
> diff --git a/hw/usb/dev-serial.c b/hw/usb/dev-serial.c
> index 77ce89d38b..abc316c7bf 100644
> --- a/hw/usb/dev-serial.c
> +++ b/hw/usb/dev-serial.c
> @@ -20,15 +20,8 @@
>  #include "chardev/char-serial.h"
>  #include "chardev/char-fe.h"
>  #include "qom/object.h"
> +#include "trace.h"
>  
> -//#define DEBUG_Serial
> -
> -#ifdef DEBUG_Serial
> -#define DPRINTF(fmt, ...) \
> -do { printf("usb-serial: " fmt , ## __VA_ARGS__); } while (0)
> -#else
> -#define DPRINTF(fmt, ...) do {} while(0)
> -#endif
>  
>  #define RECV_BUF (512 - (2 * 8))
>  
> @@ -205,8 +198,9 @@ static void usb_serial_reset(USBSerialState *s)
>  static void usb_serial_handle_reset(USBDevice *dev)
>  {
>  USBSerialState *s = USB_SERIAL(dev);
> +USBBus *bus = usb_bus_from_device(dev);
>  
> -DPRINTF("Reset\n");
> +trace_usb_serial_reset(bus->busnr, dev->addr);
>  
>  usb_serial_reset(s);
>  /* TODO: Reset char device, send BREAK? */
> @@ -244,9 +238,11 @@ static void usb_serial_handle_control(USBDevice *dev, 
> USBPacket *p,
>int length, uint8_t *data)
>  {
>  USBSerialState *s = USB_SERIAL(dev);
> +USBBus *bus = usb_bus_from_device(dev);
>  int ret;
>  
> -DPRINTF("got control %x, value %x\n", request, value);
> +trace_usb_serial_handle_control(bus->busnr, dev->addr, request, value);
> +
>  ret = usb_desc_handle_control(dev, p, request, value, index, length, 
> data);
>  if (ret >= 0) {
>  return;
> @@ -326,7 +322,8 @@ static void usb_serial_handle_control(USBDevice *dev, 
> USBPacket *p,
>  s->params.parity = 'E';
>  break;
>  default:
> -DPRINTF("unsupported parity %d\n", value & FTDI_PARITY);
> +trace_usb_serial_unsupported_parity(bus->busnr, dev->addr,
> +value & FTDI_PARITY);
>  goto fail;
>  }
>  
> @@ -338,7 +335,8 @@ static void usb_serial_handle_control(USBDevice *dev, 
> USBPacket *p,
>  s->params.stop_bits = 2;
>  break;
>  default:
> -DPRINTF("unsupported stop bits %d\n", value & FTDI_STOP);
> +trace_usb_serial_unsupported_stopbits(bus->busnr, dev->addr,
> +  value & FTDI_STOP);
>  goto fail;
>  }
>  
> @@ -367,7 +365,8 @@ static void usb_serial_handle_control(USBDevice *dev, 
> USBPacket *p,
>  break;
>  default:
>  fail:
> -DPRINTF("got unsupported/bogus control %x, value %x\n", request, 
> value);
> +trace_usb_serial_unsupported_control(bus->busnr, dev->addr, request,
> + value);
>  p->status = USB_RET_STALL;
>  break;
>  }
> @@ -431,6 +430,7 @@ static void usb_serial_token_in(USBSerialState *s, 
> USBPacket *p)
>  static void usb_serial_handle_data(USBDevice *dev, USBPacket *p)
>  {
>  USBSerialState *s = USB_SERIAL(dev);
> +USBBus *bus = usb_bus_from_device(dev);
>  uint8_t devep = p->ep->nr;
>  struct iovec *iov;
>  int i;
> @@ -459,7 +459,7 @@ static void usb_serial_handle_data(USBDevice *dev, 
> USBPacket *p)
>  break;
>  
>  default:
> -DPRINTF("Bad token\n");
> +trace_usb_serial_bad_token(bus->busnr, dev->addr);
>  fail:
>  p->status = USB_RET_STALL;
>  break;
> diff --git a/hw/usb/trace-events b/hw/usb/trace-events
> index 72e4298780..e5871cbbbc 100644
> --- a/hw/usb/trace-events
> +++ b/hw/usb/trace-events
> @@ -320,3 +320,11 @@ usb_host_parse_interface(int bus, int addr, int num, int 
> alt, int active) "dev %
>  usb_host_parse_endpoint(int bus, int addr, int ep, const char *dir, const 
> char *type, int active) "dev %d:%d, ep %d, %s, %s, active %d"
>  usb_host_parse_error(int bus, int addr, const char *errmsg) "dev %d:%d, msg 
> %s"
>  usb_host_remote_wakeup_removed(int bus, int addr) "dev %d:%d"
> +
> +# dev-serial.c
> +usb_serial_reset(int bus, int addr) "dev %d:%d reset"
> +usb_serial_handle_control(int bus, int addr, int request, int value) "dev 
> %d:%d got control 0x%x, value 0x%x"
> +usb_serial_unsupported_parity(int bus, int addr, int value) "dev %d:%d 
> unsupported parity %d"
> +usb_serial_unsupported_stopbits(int bus, int addr, int value) "dev %d:%d 
> unsupported stop bits %d"
> +usb_serial_unsupported_control(int bus, int addr, int request, int value) 
> "dev %d:%d got unsupported/bogus control 0x%x, value 0x%x"
> +usb_serial_bad_token(int bus, int addr) "dev %d:%d bad token"

In all formats, 'addr' is unsigned -> "%u".

Using %u:
Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH 4/9] dev-serial: add trace-events for baud rate and data parameters

[Philippe Mathieu-Daudé]

On 10/26/20 9:33 AM, Mark Cave-Ayland wrote:
> Signed-off-by: Mark Cave-Ayland 
> ---
>  hw/usb/dev-serial.c | 3 +++
>  hw/usb/trace-events | 2 ++
>  2 files changed, 5 insertions(+)
> 
> diff --git a/hw/usb/dev-serial.c b/hw/usb/dev-serial.c
> index abc316c7bf..badf8785db 100644
> --- a/hw/usb/dev-serial.c
> +++ b/hw/usb/dev-serial.c
> @@ -307,6 +307,7 @@ static void usb_serial_handle_control(USBDevice *dev, 
> USBPacket *p,
>  }
>  
>  s->params.speed = (4800 / 2) / (8 * divisor + subdivisor8);
> +trace_usb_serial_set_baud(bus->busnr, dev->addr, s->params.speed);
>  qemu_chr_fe_ioctl(&s->cs, CHR_IOCTL_SERIAL_SET_PARAMS, &s->params);
>  break;
>  }
> @@ -340,6 +341,8 @@ static void usb_serial_handle_control(USBDevice *dev, 
> USBPacket *p,
>  goto fail;
>  }
>  
> +trace_usb_serial_set_data(bus->busnr, dev->addr, s->params.parity,
> +  s->params.data_bits, s->params.stop_bits);
>  qemu_chr_fe_ioctl(&s->cs, CHR_IOCTL_SERIAL_SET_PARAMS, &s->params);
>  /* TODO: TX ON/OFF */
>  break;
> diff --git a/hw/usb/trace-events b/hw/usb/trace-events
> index e5871cbbbc..9e984b2e0c 100644
> --- a/hw/usb/trace-events
> +++ b/hw/usb/trace-events
> @@ -328,3 +328,5 @@ usb_serial_unsupported_parity(int bus, int addr, int 
> value) "dev %d:%d unsupport
>  usb_serial_unsupported_stopbits(int bus, int addr, int value) "dev %d:%d 
> unsupported stop bits %d"
>  usb_serial_unsupported_control(int bus, int addr, int request, int value) 
> "dev %d:%d got unsupported/bogus control 0x%x, value 0x%x"
>  usb_serial_bad_token(int bus, int addr) "dev %d:%d bad token"
> +usb_serial_set_baud(int bus, int addr, int baud) "dev %d:%d baud rate %d"
> +usb_serial_set_data(int bus, int addr, int parity, int data, int stop) "dev 
> %d:%d parity %c, data bits %d, stop bits %d"
> 

Using "%u" for addr:
Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH 5/9] dev-serial: replace DeviceOutVendor/DeviceInVendor with equivalent macros from usb.h

[Philippe Mathieu-Daudé]

On 10/26/20 9:33 AM, Mark Cave-Ayland wrote:
> The DeviceOutVendor and DeviceInVendor macros can be replaced with their
> equivalent VendorDeviceOutRequest and VendorDeviceRequest macros from usb.h.
> 
> Signed-off-by: Mark Cave-Ayland 
> ---
>  hw/usb/dev-serial.c | 25 ++---
>  1 file changed, 10 insertions(+), 15 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH v2 02/19] block/nvme: Set request_alignment at initialization

[Stefan Hajnoczi]

On Mon, Oct 26, 2020 at 11:54:47AM +0100, Philippe Mathieu-Daudé wrote:
> When introducing this driver in commit bdd6a90a9e5
> ("block: Add VFIO based NVMe driver") we correctly
> set the request_alignment in nvme_refresh_limits()
> but forgot to set it at initialization. Do it now.

This patch is fine but the commit description does not explain why this
change is necessary. Is there a bug and how can it be triggered? Or is
this code change just for consistency?

> Reported-by: Stefan Hajnoczi 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  block/nvme.c | 1 +
>  1 file changed, 1 insertion(+)

Reviewed-by: Stefan Hajnoczi 


signature.asc
Description: PGP signature

Re: [PATCH] arm/trace: Fix hex printing

[Auger Eric]

Hi Peter,

On 10/19/20 9:26 PM, Peter Maydell wrote:
> On Wed, 14 Oct 2020 at 20:36, Dr. David Alan Gilbert (git)
>  wrote:
>>
>> From: "Dr. David Alan Gilbert" 
>>
>> Use of 0x%d - make up our mind as 0x%x
>>
>> Signed-off-by: Dr. David Alan Gilbert 
>> ---
>>  hw/arm/trace-events | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/hw/arm/trace-events b/hw/arm/trace-events
>> index c8a4d80f6b..a335ee891d 100644
>> --- a/hw/arm/trace-events
>> +++ b/hw/arm/trace-events
>> @@ -41,7 +41,7 @@ smmuv3_get_cd(uint64_t addr) "CD addr: 0x%"PRIx64
>>  smmuv3_decode_cd(uint32_t oas) "oas=%d"
>>  smmuv3_decode_cd_tt(int i, uint32_t tsz, uint64_t ttb, uint32_t granule_sz, 
>> bool had) "TT[%d]:tsz:%d ttb:0x%"PRIx64" granule_sz:%d had:%d"
>>  smmuv3_cmdq_cfgi_ste(int streamid) "streamid =%d"
>> -smmuv3_cmdq_cfgi_ste_range(int start, int end) "start=0x%d - end=0x%d"
>> +smmuv3_cmdq_cfgi_ste_range(int start, int end) "start=0x%x - end=0x%x"
> 
> Ah, I missed that you'd sent this patch before.
> 
> Eric, do we want to use hex here, or should we go for
> decimal the way we do with (almost) all the other
> tracing of stream IDs (eg mmuv3_cmdq_cfgi_ste in the line before)?
> 
> The other odd-one-out is smmuv3_find_ste which prints a hex
> SID; I think the other tracing of SIDs is always decimal.
I think my preference would be to use hexa here and in the other places.

Thanks

Eric
> 
> thanks
> -- PMM
> 

Re: [PULL 01/17] build: fix macOS --enable-modules build

[Philippe Mathieu-Daudé]

On 10/26/20 2:51 PM, Paolo Bonzini wrote:
> Apple's nm implementation includes empty lines in the output that are not
> found in GNU binutils.  This confuses scripts/undefsym.py, though it did
> not confuse the scripts/undefsym.sh script that it replaced.  To fix
> this, ignore lines that do not have two fields.
> 
> Reported-by: Emmanuel Blot 
> Tested-by: Emmanuel Blot 
> Fixes: 604f3e4e90 ("meson: Convert undefsym.sh to undefsym.py", 2020-09-08)
> Signed-off-by: Paolo Bonzini 
> ---
>  .cirrus.yml |  2 +-
>  scripts/undefsym.py | 11 +--
>  2 files changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/.cirrus.yml b/.cirrus.yml
> index 81a2960b1a..900437dd2a 100644
> --- a/.cirrus.yml
> +++ b/.cirrus.yml
> @@ -40,7 +40,7 @@ macos_xcode_task:
>script:
>  - mkdir build
>  - cd build
> -- ../configure --extra-cflags='-Wno-error=deprecated-declarations'
> +- ../configure --extra-cflags='-Wno-error=deprecated-declarations' 
> --enable-modules
> --enable-werror --cc=clang || { cat config.log 
> meson-logs/meson-log.txt; exit 1; }

I'm confused as this broke the catalina-xcode test:

PASS 5 qtest-aarch64/device-introspect-test
/aarch64/device/introspect/abstract-interfaces
missing object type 'virtio-gpu-device'
Broken pipe
../tests/qtest/libqtest.c:176: kill_qemu() detected QEMU death from
signal 6 (Abort trap: 6)
ERROR qtest-aarch64/device-introspect-test - too few tests run (expected
6, got 5)
gmake: *** [Makefile.mtest:905: run-test-111] Error 1

Re: [PATCH v2 03/19] block/nvme: Introduce device/iommu 'page_size_min' variables

[Stefan Hajnoczi]

On Mon, Oct 26, 2020 at 11:54:48AM +0100, Philippe Mathieu-Daudé wrote:
> Introduce device/iommu 'page_size_min' variables to make
> the code clearer.
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  block/nvme.c | 5 -
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/block/nvme.c b/block/nvme.c
> index aa290996679..5abd7257cac 100644
> --- a/block/nvme.c
> +++ b/block/nvme.c
> @@ -690,6 +690,8 @@ static int nvme_init(BlockDriverState *bs, const char 
> *device, int namespace,
>  uint64_t deadline, now;
>  Error *local_err = NULL;
>  volatile NvmeBar *regs = NULL;
> +size_t device_page_size_min;
> +size_t iommu_page_size_min = 4096;
>  
>  qemu_co_mutex_init(&s->dma_map_lock);
>  qemu_co_queue_init(&s->dma_flush_queue);
> @@ -724,7 +726,8 @@ static int nvme_init(BlockDriverState *bs, const char 
> *device, int namespace,
>  goto out;
>  }
>  
> -s->page_size = MAX(4096, 1u << (12 + NVME_CAP_MPSMIN(cap)));
> +device_page_size_min = 1u << (12 + NVME_CAP_MPSMIN(cap));
> +s->page_size = MAX(iommu_page_size_min, device_page_size_min);

It's not clear to me that the 4096 value is related to the IOMMU page
size here. The MAX(4096) expression seems like a sanity-check to me. An
MPS value of 0 is a 4KB page size, so it's never possible to express a
smaller page size. I guess MAX() was used in case a device incorrectly
reports MPSMIN.

I think introducing the concept of IOMMU page size is premature and
maybe not the intention of the existing code, but the concept will be
needed soon, so this patch is okay:

Reviewed-by: Stefan Hajnoczi 


signature.asc
Description: PGP signature

Re: [PATCH] sabre: use object_initialize_child() for iommu child object

[Mark Cave-Ayland]

On 25/10/2020 11:41, Philippe Mathieu-Daudé wrote:


On 10/25/20 12:11 PM, Mark Cave-Ayland wrote:

On 21/10/2020 12:43, Mark Cave-Ayland wrote:


Store the child object directly within the sabre object rather than using
link properties.

Signed-off-by: Mark Cave-Ayland 
---
  hw/pci-host/sabre.c | 10 --
  hw/sparc64/sun4u.c  |  8 +---
  include/hw/pci-host/sabre.h |  2 +-
  3 files changed, 6 insertions(+), 14 deletions(-)

diff --git a/hw/pci-host/sabre.c b/hw/pci-host/sabre.c
index f41a0cc301..aaa93acd6e 100644
--- a/hw/pci-host/sabre.c
+++ b/hw/pci-host/sabre.c
@@ -383,9 +383,10 @@ static void sabre_realize(DeviceState *dev, Error **errp)
  pci_create_simple(phb->bus, 0, TYPE_SABRE_PCI_DEVICE);
  /* IOMMU */
+    sysbus_realize(SYS_BUS_DEVICE(&s->iommu), &error_fatal);
  memory_region_add_subregion_overlap(&s->sabre_config, 0x200,
-    sysbus_mmio_get_region(SYS_BUS_DEVICE(s->iommu), 0), 1);
-    pci_setup_iommu(phb->bus, sabre_pci_dma_iommu, s->iommu);
+    sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->iommu), 0), 1);
+    pci_setup_iommu(phb->bus, sabre_pci_dma_iommu, &s->iommu);
  /* APB secondary busses */
  pci_dev = pci_new_multifunction(PCI_DEVFN(1, 0), true,
@@ -422,10 +423,7 @@ static void sabre_init(Object *obj)
  s->pci_irq_in = 0ULL;
  /* IOMMU */
-    object_property_add_link(obj, "iommu", TYPE_SUN4U_IOMMU,
- (Object **) &s->iommu,
- qdev_prop_allow_set_link_before_realize,
- 0);
+    object_initialize_child(obj, "iommu", &s->iommu, TYPE_SUN4U_IOMMU);
  /* sabre_config */
  memory_region_init_io(&s->sabre_config, OBJECT(s), &sabre_config_ops, s,
diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c
index 2f8fc670cf..a33f1eccfd 100644
--- a/hw/sparc64/sun4u.c
+++ b/hw/sparc64/sun4u.c
@@ -562,7 +562,7 @@ static void sun4uv_init(MemoryRegion *address_space_mem,
  PCIBus *pci_bus, *pci_busA, *pci_busB;
  PCIDevice *ebus, *pci_dev;
  SysBusDevice *s;
-    DeviceState *iommu, *dev;
+    DeviceState *dev;
  FWCfgState *fw_cfg;
  NICInfo *nd;
  MACAddr macaddr;
@@ -571,10 +571,6 @@ static void sun4uv_init(MemoryRegion *address_space_mem,
  /* init CPUs */
  cpu = sparc64_cpu_devinit(machine->cpu_type, hwdef->prom_addr);
-    /* IOMMU */
-    iommu = qdev_new(TYPE_SUN4U_IOMMU);
-    sysbus_realize_and_unref(SYS_BUS_DEVICE(iommu), &error_fatal);
-
  /* set up devices */
  ram_init(0, machine->ram_size);
@@ -584,8 +580,6 @@ static void sun4uv_init(MemoryRegion *address_space_mem,
  sabre = SABRE(qdev_new(TYPE_SABRE));
  qdev_prop_set_uint64(DEVICE(sabre), "special-base", PBM_SPECIAL_BASE);
  qdev_prop_set_uint64(DEVICE(sabre), "mem-base", PBM_MEM_BASE);
-    object_property_set_link(OBJECT(sabre), "iommu", OBJECT(iommu),
- &error_abort);
  sysbus_realize_and_unref(SYS_BUS_DEVICE(sabre), &error_fatal);
  /* sabre_config */
diff --git a/include/hw/pci-host/sabre.h b/include/hw/pci-host/sabre.h
index 01190241bb..05bf741cde 100644
--- a/include/hw/pci-host/sabre.h
+++ b/include/hw/pci-host/sabre.h
@@ -34,7 +34,7 @@ struct SabreState {
  MemoryRegion pci_mmio;
  MemoryRegion pci_ioport;
  uint64_t pci_irq_in;
-    IOMMUState *iommu;
+    IOMMUState iommu;
  PCIBridge *bridgeA;
  PCIBridge *bridgeB;
  uint32_t pci_control[16];


No further comments (and I'm happier that this is a better solution than having an 
"optional" link property) so I've applied this to my qemu-sparc branch.


Sorry I had this patch tagged for review but am having trouble
managing that folder. This is certainly better, thanks for this
cleanup.

Reviewed-by: Philippe Mathieu-Daudé 


Looks like this patch exposes another issue related to QOM lifecycle, so I'm dropping 
it from my qemu-sparc branch for now.



ATB,

Mark.

Re: [PATCH v2] CHANGELOG: remove disused file

[Alex Bennée]

John Snow  writes:

> There's no reason to keep this here; the versions described are
> ancient. Everything here is still mirrored on
> https://wiki.qemu.org/ChangeLog/old if anyone is curious; otherwise, use
> the git history.
>
> Signed-off-by: John Snow 

Reviewed-by: Alex Bennée 

-- 
Alex Bennée

Re: [PATCH v2 04/19] block/nvme: Trace controller capabilities

[Stefan Hajnoczi]

On Mon, Oct 26, 2020 at 11:54:49AM +0100, Philippe Mathieu-Daudé wrote:
> Controllers have different capabilities and report them in the
> CAP register. We are particularly interested by the page size
> limits.
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  block/nvme.c   | 10 ++
>  block/trace-events |  1 +
>  2 files changed, 11 insertions(+)
> 
> diff --git a/block/nvme.c b/block/nvme.c
> index 5abd7257cac..3b6d3972ec2 100644
> --- a/block/nvme.c
> +++ b/block/nvme.c
> @@ -720,6 +720,16 @@ static int nvme_init(BlockDriverState *bs, const char 
> *device, int namespace,
>   * Initialization". */
>  
>  cap = le64_to_cpu(regs->cap);
> +trace_nvme_controller_capability("Maximum Queue Entries Supported",
> + NVME_CAP_MQES(cap));
> +trace_nvme_controller_capability("Contiguous Queues Required",
> + NVME_CAP_CQR(cap));
> +trace_nvme_controller_capability("Subsystem  Reset Supported",
> + NVME_CAP_NSSRS(cap));
> +trace_nvme_controller_capability("Memory Page Size Minimum",
> + NVME_CAP_MPSMIN(cap));
> +trace_nvme_controller_capability("Memory Page Size Maximum",
> + NVME_CAP_MPSMAX(cap));

This works well for printf-style tracing but it can be tedious to
strcmp() or parse strings from SystemTap and other tracing scripts.

Another way of expressing these trace events is:

  trace_nvme_controller_capabilities(NVME_CAP_MQES(cap),
 NVME_CAP_CQR(cap),
 ...);

Or:

  trace_nvme_controller_capability_mqes(NVME_CAP_MQES(cap));
  trace_nvme_controller_capability_cqr(NVME_CAP_CQR(cap));

One view is that tracing is like printf (the style used in this patch).
Another view is that tracing produces records that scripts can process
(the style I showed). Trace events defined in one style may be hard to
use in the other style (i.e. less human-readable vs harder to process in
a script).

Reviewed-by: Stefan Hajnoczi 


signature.asc
Description: PGP signature

Re: [PATCH v2 14/19] util/vfio-helpers: Pass minimum page size to qemu_vfio_open_pci()

[Stefan Hajnoczi]

On Mon, Oct 26, 2020 at 11:54:59AM +0100, Philippe Mathieu-Daudé wrote:
> @@ -737,6 +738,17 @@ static int nvme_init(BlockDriverState *bs, const char 
> *device, int namespace,
>  }
>  
>  device_page_size_min = 1u << (12 + NVME_CAP_MPSMIN(cap));
> +device_page_size_max = 1u << (12 + NVME_CAP_MPSMAX(cap));
> +if (iommu_page_size_min > device_page_size_max) {
> +g_autofree char *iommu_page_size_s = 
> size_to_str(iommu_page_size_min);
> +g_autofree char *device_page_size_s = 
> size_to_str(device_page_size_max);
> +
> +error_setg(errp, "IOMMU minimum page size (%s)"
> + " too big for device (max %s)",
> +   iommu_page_size_s, device_page_size_s);
> +ret = -EINVAL;
> +goto out;
> +}

I thought you and Eric worked on a solution to support smaller device
pages on bigger IOMMU pages? For example, 4KB device page size on 64KB
IOMMU page size.

Won't this check be removed again very soon? Why add it at all?


signature.asc
Description: PGP signature

Re: [PATCH v2] CHANGELOG: remove disused file

[Laurent Vivier]

Le 26/10/2020 à 20:51, John Snow a écrit :
> There's no reason to keep this here; the versions described are
> ancient. Everything here is still mirrored on
> https://wiki.qemu.org/ChangeLog/old if anyone is curious; otherwise, use
> the git history.
> 
> Signed-off-by: John Snow 
> ---
> 
> V2: Add new blurb in README.rst to redirect to our wiki changelog.
> 
>  README.rst |   8 +
>  Changelog  | 580 -
>  2 files changed, 8 insertions(+), 580 deletions(-)
>  delete mode 100644 Changelog
> 
> diff --git a/README.rst b/README.rst
> index 7497709291d..58b9f2dc15c 100644
> --- a/README.rst
> +++ b/README.rst
> @@ -134,6 +134,14 @@ For additional information on bug reporting consult:
>  * ``_
>  
>  
> +ChangeLog
> +=
> +
> +For version history and release notes, please visit
> +``_ or look at the git history for
> +more detailed information.
> +
> +
>  Contact
>  ===
>  
> diff --git a/Changelog b/Changelog
> deleted file mode 100644
> index f7e178ccc01..000
> --- a/Changelog
> +++ /dev/null
> @@ -1,580 +0,0 @@
> -This file documents changes for QEMU releases 0.12 and earlier.
> -For changelog information for later releases, see
> -https://wiki.qemu.org/ChangeLog or look at the git history for
> -more detailed information.
> -
> -
> -version 0.12.0:
> -
> -  - Update to SeaBIOS 0.5.0
> -  - e1000: fix device link status in Linux (Anthony Liguori)
> -  - monitor: fix QMP for balloon command (Luiz Capitulino)
> -  - QMP: Return an empty dict by default (Luiz Capitulino)
> -  - QMP: Only handle converted commands (Luiz Capitulino)
> -  - pci: support PCI based option rom loading (Gerd Hoffman/Anthony Liguori)
> -  - Fix backcompat for hotplug of SCSI controllers (Daniel P. Berrange)
> -  - fdc: fix migration from 0.11 (Juan Quintela)
> -  - vmware-vga: fix segv on cursor resize. (Dave Airlie)
> -  - vmware-vga: various fixes (Dave Airlie/Anthony Liguori)
> -  - qdev: improve property error reporting. (Gerd Hoffmann)
> -  - fix vga names in default_list (Gerd Hoffmann)
> -  - usb-host: check mon before using it. (Gerd Hoffmann)
> -  - usb-net: use qdev for -usbdevice (Gerd Hoffmann)
> -  - monitor: Catch printing to non-existent monitor (Luiz Capitulino)
> -  - Avoid permanently disabled QEMU monitor when UNIX migration fails 
> (Daniel P. Berrange)
> -  - Fix loading of ELF multiboot kernels (Kevin Wolf)
> -  - qemu-io: Fix memory leak (Kevin Wolf)
> -  - Fix thinko in linuxboot.S (Paolo Bonzini)
> -  - target-i386: Fix evaluation of DR7 register (Jan Kiszka)
> -  - vnc: hextile: do not generate ForegroundSpecified and SubrectsColoured 
> tiles (Anthony Liguori)
> -  - S390: Bail out without KVM (Alexander Graf)
> -  - S390: Don't tell guest we're updating config space (Alexander Graf)
> -  - target-s390: Fail on unknown instructions (Alexander Graf)
> -  - osdep: Fix runtime failure on older Linux kernels (Andre Przywara)
> -  - Fix a make -j race (Juergen Lock)
> -  - target-alpha: Fix generic ctz64. (Richard Henderson)
> -  - s390: Fix buggy assignment (Stefan Weil)
> -  - target-mips: fix user-mode emulation startup (Nathan Froyd)
> -  - target-i386: Update CPUID feature set for TCG (Andre Przywara)
> -  - s390: fix build on 32 bit host (Michael S. Tsirkin)
> - 
> -version 0.12.0-rc2:
> -
> -  - v2: properly save kvm system time msr registers (Glauber Costa)
> -  - convert more monitor commands to qmp (Luiz Capitulino)
> -  - vnc: fix capslock tracking logic. (Gerd Hoffmann)
> -  - QemuOpts: allow larger option values. (Gerd Hoffmann)
> -  - scsi: fix drive hotplug. (Gerd Hoffmann)
> -  - pci: don't hw_error() when no slot is available. (Gerd Hoffmann)
> -  - pci: don't abort() when trying to hotplug with acpi off. (Gerd Hoffmann)
> -  - allow default devices to be implemented in config file (Gerd Hoffman)
> -  - vc: colorize chardev title line with blue background. (Gerd Hoffmann)
> -  - chardev: make chardevs specified in config file work. (Gerd Hoffmann)
> -  - qdev: also match bus name for global properties (Gerd Hoffmann)
> -  - qdev: add command line option to set global defaults for properties. 
> (Gerd Hoffmann)
> -  - kvm: x86: Save/restore exception_index (Jan Kiszka)
> -  - qdev: Replace device names containing whitespace (Markus Armbruster)
> -  - fix rtc-td-hack on host without high-res timers (Gleb Natapov)
> -  - virtio: verify features on load (Michael S. Tsirkin)
> -  - vmware_vga: add rom file so that it boots. (Dave Airlie)
> -  - Do not abort on qemu_malloc(0) in production builds (Anthony Liguori)
> -  - Fix ARM userspace strex implementation. (Paul Brook)
> -  - qemu: delete rule target on error (Michael S. Tsirkin)
> -  - QMP: add human-readable description to error response (Markus Armbruster)
> -  - convert more monitor commands to QError (Markus Armbruster)
> -  - monitor: Fix double-prompt after "change vnc passwd BLA" (Markus 
> Armb

Re: [PATCH v2 18/19] block/nvme: Switch to using the MSIX API

[Philippe Mathieu-Daudé]

On 10/26/20 9:32 PM, Auger Eric wrote:
> Hi Philippe,
> 
> On 10/26/20 11:55 AM, Philippe Mathieu-Daudé wrote:
>> In preparation of using multiple IRQs, switch to using the recently
>> introduced MSIX API. Instead of allocating and assigning IRQ in
>> a single step, we now have to use two distinct calls.
>>
>> Reviewed-by: Stefan Hajnoczi 
>> Signed-off-by: Philippe Mathieu-Daudé 
>> ---
>>  block/nvme.c | 14 --
>>  1 file changed, 12 insertions(+), 2 deletions(-)
>>
>> diff --git a/block/nvme.c b/block/nvme.c
>> index 46b09b3a3a7..191678540b6 100644
>> --- a/block/nvme.c
>> +++ b/block/nvme.c
>> @@ -693,6 +693,7 @@ static int nvme_init(BlockDriverState *bs, const char 
>> *device, int namespace,
>>  size_t device_page_size_min;
>>  size_t device_page_size_max;
>>  size_t iommu_page_size_min = 4096;
>> +unsigned irq_count = MSIX_IRQ_COUNT;
>>  
>>  qemu_co_mutex_init(&s->dma_map_lock);
>>  qemu_co_queue_init(&s->dma_flush_queue);
>> @@ -809,8 +810,17 @@ static int nvme_init(BlockDriverState *bs, const char 
>> *device, int namespace,
>>  }
>>  }
>>  
>> -ret = qemu_vfio_pci_init_irq(s->vfio, s->irq_notifier,
>> - VFIO_PCI_MSIX_IRQ_INDEX, errp);
>> +ret = qemu_vfio_pci_msix_init_irqs(s->vfio, &irq_count, errp);
>> +if (ret) {
>> +if (ret == -EOVERFLOW) {
>> +error_append_hint(errp, "%u IRQs requested but only %u 
>> available\n",
>> +  MSIX_IRQ_COUNT, irq_count);
> This message can be directly printed in qemu_vfio_pci_msix_init_irqs()

Good idea, thanks.

Re: [PATCH v2 00/19] util/vfio-helpers: Allow using multiple MSIX IRQs

[Stefan Hajnoczi]

On Mon, Oct 26, 2020 at 11:54:45AM +0100, Philippe Mathieu-Daudé wrote:
> This series allow using multiple MSIX IRQs
> We currently share a single IRQ between 2 NVMe queues
> (ADMIN and I/O). This series still uses 1 shared IRQ
> but prepare for using multiple ones.
> 
> Since v1:
> - Addressed Stefan comment in patch #14
>   "Pass minimum page size to qemu_vfio_open_pci"
>   (check the page size is in range with device)
> - To reduce (and simplify) changes in patch #14, added
>   new patch #2 "Introduce device/iommu 'page_size_min' variables"
> - Added "Trace controller capabilities" useful to test the
>   previous changes
> - "Set request_alignment at initialization" reported by Stefan
>   (and tested by Eric off-list).

The MSI-X patches look good.

I'm confused about the page size patches since they don't solve the 4KB
device page size on 64KB IOMMU page size issue that you and Eric were
working on. I would prefer to hold off on page size changes until that
work is complete.

Stefan


signature.asc
Description: PGP signature

[PULL 5/8] scripts: fix error from checkpatch.pl when no commits are found

[Alex Bennée]

From: Daniel P. Berrangé 

The error message was supposed to mention the input revision list start
point, not the branch flag.

Signed-off-by: Daniel P. Berrangé 
Signed-off-by: Alex Bennée 
Message-Id: <20201019143537.283094-3-berra...@redhat.com>
Message-Id: <20201021163136.27324-6-alex.ben...@linaro.org>

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 6ed34970f9..88c858f67c 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -392,7 +392,7 @@ if ($chk_branch) {
 
close $HASH;
 
-   die "$P: no revisions returned for revlist '$chk_branch'\n"
+   die "$P: no revisions returned for revlist '$ARGV[0]'\n"
unless @patches;
 
my $i = 1;
-- 
2.20.1

[PULL 1/8] Adding ani's email as an individual contributor

[Alex Bennée]

From: Ani Sinha 

Ani is an individual contributor into qemu project. Adding my email into the
correct file to reflect so.

Signed-off-by: Ani Sinha 
Signed-off-by: Alex Bennée 
Reviewed-by: Philippe Mathieu-Daudé 
Message-Id: <20201007161940.1478-1-...@anisinha.ca>
Message-Id: <20201021163136.27324-2-alex.ben...@linaro.org>

diff --git a/contrib/gitdm/group-map-individuals 
b/contrib/gitdm/group-map-individuals
index 641169fa63..d135f4b143 100644
--- a/contrib/gitdm/group-map-individuals
+++ b/contrib/gitdm/group-map-individuals
@@ -23,3 +23,4 @@ vr_q...@t-online.de
 nieklinnenb...@gmail.com
 devne...@gmail.com
 pauld...@gmail.com
+a...@anisinha.ca
-- 
2.20.1

[PULL 2/8] contrib/gitdm: Add more individual contributors

[Alex Bennée]

Signed-off-by: Alex Bennée 
Reviewed-by: Philippe Mathieu-Daudé 
Acked-by: Subbaraya Sundeep 
Acked-by: Michael Rolnik 
Acked-by: Thomas Huth 
Acked-by: James Hogan 
Acked-by: Artyom Tarasenko 
Message-id: <20201004182506.2038515-1-f4...@amsat.org>
Message-Id: <20201021163136.27324-3-alex.ben...@linaro.org>

diff --git a/contrib/gitdm/group-map-individuals 
b/contrib/gitdm/group-map-individuals
index d135f4b143..36bbb77c39 100644
--- a/contrib/gitdm/group-map-individuals
+++ b/contrib/gitdm/group-map-individuals
@@ -24,3 +24,8 @@ nieklinnenb...@gmail.com
 devne...@gmail.com
 pauld...@gmail.com
 a...@anisinha.ca
+sundeep.l...@gmail.com
+mrol...@gmail.com
+h...@tuxfamily.org
+jho...@kernel.org
+atar4q...@gmail.com
-- 
2.20.1

[PULL 0/8] testing and misc (gitdm, gitlab, docker, make)

[Alex Bennée]

The following changes since commit 1dc887329a10903940501b43e8c0cc67af7c06d5:

  Merge remote-tracking branch 'remotes/philmd-gitlab/tags/sd-next-20201026' 
into staging (2020-10-26 17:19:26 +)

are available in the Git repository at:

  https://github.com/stsquad/qemu.git tags/pull-testing-and-misc-271020-1

for you to fetch changes up to c8e6cfba1291df2202bf406bb5137c9d365505d1:

  makefile: handle -n / -k / -q correctly (2020-10-27 09:53:51 +)


Testing and gitdm updates

  - add some more individual contributors
  - include SDL2 in centos images
  - skip checkpatch check when no commits found
  - use random port for gdb reverse debugging
  - make gitlab use it's own mirrors to clone
  - fix detection of make -nqp


Alex Bennée (2):
  contrib/gitdm: Add more individual contributors
  tests/acceptance: pick a random gdb port for reverse debugging

Ani Sinha (1):
  Adding ani's email as an individual contributor

Daniel P. Berrangé (2):
  gitlab: skip checkpatch.pl checks if no commit delta on branch
  scripts: fix error from checkpatch.pl when no commits are found

Paolo Bonzini (1):
  makefile: handle -n / -k / -q correctly

Philippe Mathieu-Daudé (1):
  gitlab-ci: Clone from GitLab itself

Thomas Huth (1):
  tests/docker/dockerfiles/centos: Use SDL2 instead of SDL1

 Makefile| 10 +++---
 .gitlab-ci.d/check-patch.py |  8 
 .gitlab-ci.yml  |  1 +
 contrib/gitdm/group-map-individuals |  6 ++
 scripts/checkpatch.pl   |  2 +-
 tests/acceptance/reverse_debugging.py   | 12 +++-
 tests/docker/dockerfiles/centos7.docker |  2 +-
 tests/docker/dockerfiles/centos8.docker |  2 +-
 8 files changed, 32 insertions(+), 11 deletions(-)

-- 
2.20.1

[PULL 6/8] tests/acceptance: pick a random gdb port for reverse debugging

[Alex Bennée]

Currently the test randomly fails if you are using a shared machine
due to contention on the well known port 1234. We can ameliorate this
a bit by picking a random non-ephemeral port although it doesn't
totally avoid the problem. While we could use a totally unique socket
address for debugging it is fiddly to probe for gdb support. While gdb
socket debugging is not yet ubiquitous this a sub-optimal but workable
option.

Signed-off-by: Alex Bennée 
Reviewed-by: Thomas Huth 
Reviewed-by: Pavel Dovgalyuk 
Reviewed-by: Philippe Mathieu-Daudé 
Message-Id: <20201021163136.27324-7-alex.ben...@linaro.org>
Signed-off-by: Alex Bennée 

diff --git a/tests/acceptance/reverse_debugging.py 
b/tests/acceptance/reverse_debugging.py
index b72fdf6cdc..be01aca217 100644
--- a/tests/acceptance/reverse_debugging.py
+++ b/tests/acceptance/reverse_debugging.py
@@ -14,6 +14,7 @@ from avocado import skipIf
 from avocado_qemu import BUILD_DIR
 from avocado.utils import gdb
 from avocado.utils import process
+from avocado.utils.network.ports import find_free_port
 from avocado.utils.path import find_command
 from boot_linux_console import LinuxKernelTest
 
@@ -33,7 +34,7 @@ class ReverseDebugging(LinuxKernelTest):
 STEPS = 10
 endian_is_le = True
 
-def run_vm(self, record, shift, args, replay_path, image_path):
+def run_vm(self, record, shift, args, replay_path, image_path, port):
 logger = logging.getLogger('replay')
 vm = self.get_vm()
 vm.set_console()
@@ -43,7 +44,7 @@ class ReverseDebugging(LinuxKernelTest):
 else:
 logger.info('replaying the execution...')
 mode = 'replay'
-vm.add_args('-s', '-S')
+vm.add_args('-gdb', 'tcp::%d' % port, '-S')
 vm.add_args('-icount', 'shift=%s,rr=%s,rrfile=%s,rrsnapshot=init' %
 (shift, mode, replay_path),
 '-net', 'none')
@@ -109,9 +110,10 @@ class ReverseDebugging(LinuxKernelTest):
 process.run(cmd)
 
 replay_path = os.path.join(self.workdir, 'replay.bin')
+port = find_free_port()
 
 # record the log
-vm = self.run_vm(True, shift, args, replay_path, image_path)
+vm = self.run_vm(True, shift, args, replay_path, image_path, port)
 while self.vm_get_icount(vm) <= self.STEPS:
 pass
 last_icount = self.vm_get_icount(vm)
@@ -120,9 +122,9 @@ class ReverseDebugging(LinuxKernelTest):
 logger.info("recorded log with %s+ steps" % last_icount)
 
 # replay and run debug commands
-vm = self.run_vm(False, shift, args, replay_path, image_path)
+vm = self.run_vm(False, shift, args, replay_path, image_path, port)
 logger.info('connecting to gdbstub')
-g = gdb.GDBRemote('127.0.0.1', 1234, False, False)
+g = gdb.GDBRemote('127.0.0.1', port, False, False)
 g.connect()
 r = g.cmd(b'qSupported')
 if b'qXfer:features:read+' in r:
-- 
2.20.1

[PULL 7/8] gitlab-ci: Clone from GitLab itself

[Alex Bennée]

From: Philippe Mathieu-Daudé 

Let GitLab runners use GitLab repository directly.

Suggested-by: Paolo Bonzini 
Signed-off-by: Philippe Mathieu-Daudé 
Reviewed-by: Thomas Huth 
Message-Id: <20201022123302.2884788-1-phi...@redhat.com>
Signed-off-by: Alex Bennée 

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 5d6773efd2..3b15ae5c30 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -24,6 +24,7 @@ include:
   image: $CI_REGISTRY_IMAGE/qemu/$IMAGE:latest
   before_script:
 - JOBS=$(expr $(nproc) + 1)
+- sed -i s,git.qemu.org/git,gitlab.com/qemu-project, .gitmodules
   script:
 - mkdir build
 - cd build
-- 
2.20.1

[PULL 3/8] tests/docker/dockerfiles/centos: Use SDL2 instead of SDL1

[Alex Bennée]

From: Thomas Huth 

We do not support SDL1 in QEMU anymore. Use SDL2 instead.

Signed-off-by: Thomas Huth 
Signed-off-by: Alex Bennée 
Tested-by: Philippe Mathieu-Daudé 
Reviewed-by: Daniel P. Berrangé 
Reviewed-by: Philippe Mathieu-Daudé 
Message-Id: <20201021072308.9224-1-th...@redhat.com>
Message-Id: <20201021163136.27324-4-alex.ben...@linaro.org>

diff --git a/tests/docker/dockerfiles/centos7.docker 
b/tests/docker/dockerfiles/centos7.docker
index 8b273725ee..6f11af1989 100644
--- a/tests/docker/dockerfiles/centos7.docker
+++ b/tests/docker/dockerfiles/centos7.docker
@@ -31,7 +31,7 @@ ENV PACKAGES \
 perl-Test-Harness \
 pixman-devel \
 python3 \
-SDL-devel \
+SDL2-devel \
 spice-glib-devel \
 spice-server-devel \
 tar \
diff --git a/tests/docker/dockerfiles/centos8.docker 
b/tests/docker/dockerfiles/centos8.docker
index a589142114..54bc6d54cd 100644
--- a/tests/docker/dockerfiles/centos8.docker
+++ b/tests/docker/dockerfiles/centos8.docker
@@ -2,7 +2,7 @@ FROM centos:8.1.1911
 
 RUN dnf -y update
 ENV PACKAGES \
-SDL-devel \
+SDL2-devel \
 bzip2 \
 bzip2-devel \
 dbus-daemon \
-- 
2.20.1

[PULL 4/8] gitlab: skip checkpatch.pl checks if no commit delta on branch

[Alex Bennée]

From: Daniel P. Berrangé 

If the current branch is synced to the current upstream git master,
there are no commits that need checking. This causes checkpatch.pl
to print an error that it found no commits. We need to avoid calling
checkpatch.pl in this case.

Signed-off-by: Daniel P. Berrangé 
Signed-off-by: Alex Bennée 
Reviewed-by: Philippe Mathieu-Daudé 
Acked-by: Thomas Huth 
Message-Id: <20201019143537.283094-2-berra...@redhat.com>
Message-Id: <20201021163136.27324-5-alex.ben...@linaro.org>

diff --git a/.gitlab-ci.d/check-patch.py b/.gitlab-ci.d/check-patch.py
index 5a14a25b13..0ff30ee077 100755
--- a/.gitlab-ci.d/check-patch.py
+++ b/.gitlab-ci.d/check-patch.py
@@ -33,8 +33,16 @@ ancestor = subprocess.check_output(["git", "merge-base",
 
 ancestor = ancestor.strip()
 
+log = subprocess.check_output(["git", "log", "--format=%H %s",
+   ancestor + "..."],
+  universal_newlines=True)
+
 subprocess.check_call(["git", "remote", "rm", "check-patch"])
 
+if log == "":
+print("\nNo commits since %s, skipping checks\n" % ancestor)
+sys.exit(0)
+
 errors = False
 
 print("\nChecking all commits since %s...\n" % ancestor)
-- 
2.20.1

[PULL 8/8] makefile: handle -n / -k / -q correctly

[Alex Bennée]

From: Paolo Bonzini 

Use $(findstring) instead of $(filter) to detect -n/-k
as different versions of MAKE fill in $(MAKEFLAGS) differently.
Do not bother running ninja at all if -nq is passed.

Tested-by: Alex Bennée 
Signed-off-by: Paolo Bonzini 
Signed-off-by: Alex Bennée 
Message-Id: <20201026155854.3074290-1-pbonz...@redhat.com>

diff --git a/Makefile b/Makefile
index 4d1fa8bb3d..e7c1000f5c 100644
--- a/Makefile
+++ b/Makefile
@@ -146,9 +146,12 @@ endif
 # 4. Rules to bridge to other makefiles
 
 ifneq ($(NINJA),)
-NINJAFLAGS = $(if $V,-v,) \
+MAKE.n = $(findstring n,$(firstword $(MAKEFLAGS)))
+MAKE.k = $(findstring k,$(firstword $(MAKEFLAGS)))
+MAKE.q = $(findstring q,$(firstword $(MAKEFLAGS)))
+MAKE.nq = $(if $(word 2, $(MAKE.n) $(MAKE.q)),nq)
+NINJAFLAGS = $(if $V,-v) $(if $(MAKE.n), -n) $(if $(MAKE.k), -k0) \
 $(filter-out -j, $(lastword -j1 $(filter -l% -j%, $(MAKEFLAGS \
-$(subst -k, -k0, $(filter -n -k,$(MAKEFLAGS)))
 
 ninja-cmd-goals = $(or $(MAKECMDGOALS), all)
 ninja-cmd-goals += $(foreach t, $(.tests), $(.test.deps.$t))
@@ -165,7 +168,8 @@ $(ninja-targets): run-ninja
 # --output-sync line.
 run-ninja: config-host.mak
 ifneq ($(filter $(ninja-targets), $(ninja-cmd-goals)),)
-   +@$(NINJA) $(NINJAFLAGS) $(sort $(filter $(ninja-targets), 
$(ninja-cmd-goals))) | cat
+   +$(quiet-@)$(if $(MAKE.nq),@:, $(NINJA) \
+  $(NINJAFLAGS) $(sort $(filter $(ninja-targets), $(ninja-cmd-goals))) 
| cat)
 endif
 endif
 
-- 
2.20.1

Re: [PATCH] virtio-gpu: only compile virtio-gpu-3d.c for CONFIG_VIRGL=y

[Philippe Mathieu-Daudé]

On 10/26/20 3:28 PM, Gerd Hoffmann wrote:
> There is no actual code in the CONFIG_VIRGL=n case.  So building is
> (a) pointless and (b) makes macos ranlib complain.
> 
> Reported-by: Paolo Bonzini 
> Signed-off-by: Gerd Hoffmann 
> ---
>  hw/display/virtio-gpu-3d.c | 4 
>  hw/display/meson.build | 4 +++-
>  2 files changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/display/virtio-gpu-3d.c b/hw/display/virtio-gpu-3d.c
> index 1bd33d7aedc6..0b0c11474dd3 100644
> --- a/hw/display/virtio-gpu-3d.c
> +++ b/hw/display/virtio-gpu-3d.c
> @@ -17,8 +17,6 @@
>  #include "hw/virtio/virtio.h"
>  #include "hw/virtio/virtio-gpu.h"
>  
> -#ifdef CONFIG_VIRGL
> -
>  #include 
>  
>  static struct virgl_renderer_callbacks virtio_gpu_3d_cbs;
> @@ -633,5 +631,3 @@ int virtio_gpu_virgl_get_num_capsets(VirtIOGPU *g)
>  
>  return capset2_max_ver ? 2 : 1;
>  }
> -
> -#endif /* CONFIG_VIRGL */
> diff --git a/hw/display/meson.build b/hw/display/meson.build
> index 0d5ddecd6503..5906b96b830e 100644
> --- a/hw/display/meson.build
> +++ b/hw/display/meson.build
> @@ -57,7 +57,9 @@ softmmu_ss.add(when: [pixman, 'CONFIG_ATI_VGA'], if_true: 
> files('ati.c', 'ati_2d
>  if config_all_devices.has_key('CONFIG_VIRTIO_GPU')
>virtio_gpu_ss = ss.source_set()
>virtio_gpu_ss.add(when: 'CONFIG_VIRTIO_GPU',
> -if_true: [files('virtio-gpu-base.c', 'virtio-gpu.c', 
> 'virtio-gpu-3d.c'), pixman, virgl])
> +if_true: [files('virtio-gpu-base.c', 'virtio-gpu.c'), 
> pixman, virgl])
> +  virtio_gpu_ss.add(when: ['CONFIG_VIRTIO_GPU', 'CONFIG_VIRGL'],
> +if_true: [files('virtio-gpu-3d.c'), pixman, virgl])
>virtio_gpu_ss.add(when: 'CONFIG_VHOST_USER_GPU', if_true: 
> files('vhost-user-gpu.c'))
>hw_display_modules += {'virtio-gpu': virtio_gpu_ss}
>  endif

Mark suggested to test this patch to see if this solve the issue
introduced by commit da0dfe251d7 ("build: fix macOS --enable-modules
build") but it does not:

missing object type 'virtio-gpu-device'
Broken pipe
../tests/qtest/libqtest.c:176: kill_qemu() detected QEMU death from
signal 6 (Abort trap: 6)
ERROR qtest-aarch64/device-introspect-test - too few tests run (expected
6, got 5)
gmake: *** [Makefile.mtest:905: run-test-111] Error 1

Re: [PATCH v6 01/11] block: Simplify QAPI_LIST_ADD

[Vladimir Sementsov-Ogievskiy]

27.10.2020 08:05, Eric Blake wrote:

There is no need to rely on the verbosity of the gcc/clang compiler
extension of g_new(typeof(X), 1) when we can instead use the standard
g_malloc(sizeof(X)).  In general, we like g_new over g_malloc for
returning type X rather than void* to let the compiler catch more
potential typing mistakes, but in this particular macro, our other use
of typeof on the same line already ensures we are getting correct
results.

Suggested-by: Markus Armbruster
Signed-off-by: Eric Blake


Reviewed-by: Vladimir Sementsov-Ogievskiy 

--
Best regards,
Vladimir

Re: [PATCH] tests/vm: update openbsd to release 6.8

[Philippe Mathieu-Daudé]

On 10/27/20 6:30 AM, Brad Smith wrote:
> tests/vm: update openbsd to release 6.8
> 
> A double dash at the end of a package name removes ambiguity
> when the intent is to install a non-FLAVORed package.
> 
> Signed-off-by: Brad Smith 
> Reviewed-by: Gerd Hoffmann 
> Tested-by: Gerd Hoffmann 
> Reviewed-by: Philippe Mathieu-Daudé 

I confirm Brad sent us this patch off-list, and
- our review comments are addressed,
- the tags are correct.

The patch format itself seems broken... Like a copy/paste
into an email client...

---

> 
> 
> diff --git a/tests/vm/openbsd b/tests/vm/openbsd
> index 8356646f21..5ffa4f1b37 100755
> --- a/tests/vm/openbsd
> +++ b/tests/vm/openbsd
> @@ -22,8 +22,8 @@ class OpenBSDVM(basevm.BaseVM):
>  name = "openbsd"
>  arch = "x86_64"
>  
> -link = "https://cdn.openbsd.org/pub/OpenBSD/6.6/amd64/install66.iso";
> -csum = "b22e63df56e6266de6bbeed8e9be0fbe9ee2291551c5bc03f3cc2e4ab9436ee3"
> +link = "https://cdn.openbsd.org/pub/OpenBSD/6.8/amd64/install68.iso";
> +csum = "47e291fcc2d0c1a8ae0b66329f040b33af755b6adbd21739e20bb5ad56f62b6c"
>  size = "20G"
>  pkgs = [
>  # tools
> @@ -36,10 +36,10 @@ class OpenBSDVM(basevm.BaseVM):
>  "bash",
>  "gmake",
>  "gsed",
> -"gettext",
> +"gettext-tools",
>  
>  # libs: usb
> -"libusb1",
> +"libusb1--",
>  
>  # libs: crypto
>  "gnutls",
> 

Re: [PATCH v6 11/11] qapi: Use QAPI_LIST_ADD() where possible

[Markus Armbruster]

Eric Blake  writes:

> Anywhere we create a list of just one item or by prepending items
> (typically because order doesn't matter), we can use the now-public
> macro.  But places where we must keep the list in order by appending
> remain open-coded.

Should we rename the macro to QAPI_LIST_PREPEND()?

How many places append?  If it's more than just a few, an attempt to
factor out the common code is in order.  Not in this patch, of course.
Not even in this series.

>
> Signed-off-by: Eric Blake 
> ---
>  docs/devel/writing-qmp-commands.txt | 13 +++--
>  hw/net/rocker/rocker_fp.h   |  2 +-
>  block/gluster.c | 19 +
>  chardev/char.c  | 21 +++
>  hw/core/machine.c   |  6 +
>  hw/net/rocker/rocker.c  |  8 +++---
>  hw/net/rocker/rocker_fp.c   | 14 +-
>  hw/net/virtio-net.c | 21 +--
>  migration/migration.c   |  7 ++---
>  migration/postcopy-ram.c|  7 ++---
>  monitor/hmp-cmds.c  | 11 
>  qemu-img.c  |  5 ++--
>  qga/commands-posix.c| 13 +++--
>  qga/commands-win32.c| 17 +++-
>  qga/commands.c  |  6 +
>  qom/qom-qmp-cmds.c  | 29 ++--
>  target/arm/helper.c |  6 +
>  target/arm/monitor.c| 13 ++---
>  target/i386/cpu.c   |  6 +
>  target/mips/helper.c|  6 +
>  target/s390x/cpu_models.c   | 12 ++---
>  tests/test-clone-visitor.c  |  7 ++---
>  tests/test-qobject-output-visitor.c | 42 ++---
>  tests/test-visitor-serialization.c  |  5 +---
>  trace/qmp.c | 22 +++
>  ui/vnc.c| 21 +--
>  util/qemu-config.c  | 14 +++---
>  target/ppc/translate_init.c.inc | 12 ++---
>  28 files changed, 119 insertions(+), 246 deletions(-)

Nice diffstat.

>
> diff --git a/docs/devel/writing-qmp-commands.txt 
> b/docs/devel/writing-qmp-commands.txt
> index 46a6c48683f5..3e11eeaa1893 100644
> --- a/docs/devel/writing-qmp-commands.txt
> +++ b/docs/devel/writing-qmp-commands.txt
> @@ -531,15 +531,10 @@ TimerAlarmMethodList *qmp_query_alarm_methods(Error 
> **errp)
>  bool current = true;
>
>  for (p = alarm_timers; p->name; p++) {
> -TimerAlarmMethodList *info = g_malloc0(sizeof(*info));
> -info->value = g_malloc0(sizeof(*info->value));
> -info->value->method_name = g_strdup(p->name);
> -info->value->current = current;
> -
> -current = false;
> -
> -info->next = method_list;
> -method_list = info;
> + TimerAlarmMethod *value = g_new0(TimerAlarmMethod, 1);

Can just as well use g_new(), as QAPI_LIST_ADD() will set both members
of @value.  Same elsewhere.

> +value->method_name = g_strdup(p->name);
> +value->current = current;
> +QAPI_LIST_ADD(method_list, value);
>  }
>
>  return method_list;
> diff --git a/hw/net/rocker/rocker_fp.h b/hw/net/rocker/rocker_fp.h
> index dbe1dd329a4b..4cb0bb9ccf81 100644
> --- a/hw/net/rocker/rocker_fp.h
> +++ b/hw/net/rocker/rocker_fp.h
> @@ -28,7 +28,7 @@ int fp_port_eg(FpPort *port, const struct iovec *iov, int 
> iovcnt);
>
>  char *fp_port_get_name(FpPort *port);
>  bool fp_port_get_link_up(FpPort *port);
> -void fp_port_get_info(FpPort *port, RockerPortList *info);
> +void fp_port_get_info(FpPort *port, RockerPort *info);
>  void fp_port_get_macaddr(FpPort *port, MACAddr *macaddr);
>  void fp_port_set_macaddr(FpPort *port, MACAddr *macaddr);
>  uint8_t fp_port_get_learning(FpPort *port);
> diff --git a/block/gluster.c b/block/gluster.c
> index 4f1448e2bc88..cf446c23f85d 100644
> --- a/block/gluster.c
> +++ b/block/gluster.c
> @@ -359,8 +359,8 @@ static int qemu_gluster_parse_uri(BlockdevOptionsGluster 
> *gconf,
>  return -EINVAL;
>  }
>
> -gconf->server = g_new0(SocketAddressList, 1);
> -gconf->server->value = gsconf = g_new0(SocketAddress, 1);
> +gsconf = g_new0(SocketAddress, 1);
> +QAPI_LIST_ADD(gconf->server, gsconf);
>
>  /* transport */
>  if (!uri->scheme || !strcmp(uri->scheme, "gluster")) {
> @@ -514,7 +514,7 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster 
> *gconf,
>  {
>  QemuOpts *opts;
>  SocketAddress *gsconf = NULL;
> -SocketAddressList *curr = NULL;
> +SocketAddressList **curr;
>  QDict *backing_options = NULL;
>  Error *local_err = NULL;
>  char *str = NULL;
> @@ -547,6 +547,7 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster 
> *gconf,
>  }
>  gconf->path = g_strdup(ptr);
>  qemu_opts_del(opts);
> +curr = &gconf->server;
>
>  for (i = 0; i < num_servers; i++) {
>  str = g_strdup_printf(GLUSTER_OPT_SERVER_PATTERN"%d.", i);
> @@ -655

Re: [PATCH v2 4/6] cfi: Initial support for cfi-icall in QEMU

[Alex Bennée]

Daniele Buono  writes:

> LLVM/Clang, supports runtime checks for forward-edge Control-Flow
> Integrity (CFI).
>
> CFI on indirect function calls (cfi-icall) ensures that, in indirect
> function calls, the function called is of the right signature for the
> pointer type defined at compile time.
>
> For this check to work, the code must always respect the function
> signature when using function pointer, the function must be defined
> at compile time, and be compiled with link-time optimization.
>
> This rules out, for example, shared libraries that are dynamically loaded
> (given that functions are not known at compile time), and code that is
> dynamically generated at run-time.
>
> This patch:
>
> 1) Introduces the CONFIG_CFI flag to support cfi in QEMU
>
> 2) Introduces a decorator to allow the definition of "sensitive"
> functions, where a non-instrumented function may be called at runtime
> through a pointer. The decorator will take care of disabling cfi-icall
> checks on such functions, when cfi is enabled.
>
> 3) Marks functions currently in QEMU that exhibit such behavior,
> in particular:
> - The function in TCG that calls pre-compiled TBs
> - The function in TCI that interprets instructions
> - Functions in the plugin infrastructures that jump to callbacks
> - Functions in util that directly call a signal handler
>
> 4) Add a new section in MAINTAINERS with me as a maintainer for
> include/qemu/sanitizers.h, in case a maintainer is deemed
> necessary for this feature
>
> Signed-off-by: Daniele Buono 
> ---
>  MAINTAINERS   |  5 +
>  accel/tcg/cpu-exec.c  |  9 +
>  include/qemu/sanitizers.h | 22 ++
>  plugins/core.c| 25 +
>  plugins/loader.c  |  5 +

With the changes Paolo suggested (QEMU_DISABLE_CFI and use compilers.h)
then for the plugin bits:

Acked-by: Alex Bennée 

-- 
Alex Bennée

[PULL 2/2] scripts/qmp: delete 'qmp' script

[Markus Armbruster]

From: John Snow 

This script has not seen a patch that was specifically for this script
since it was moved to this location in 2013, and I doubt it is used. It
uses "man qmp" for its help message, which does not exist. It also
presumes there is a manual page for qmp-XXX, for each defined qmp
command XXX. I don't think that's true.

The format it expects arguments in is something like:

block-dirty-bitmap-add --node=foo --name=bar

and has no capacity to support nested JSON arguments, either.

Most developers use either qmp-shell or socat (or pasting JSON directly
into qmp stdio), so this duplication and additional alternate syntax is
not helpful.

Remove it. Leave a breadcrumb script just in case, to be removed next
release cycle.

Signed-off-by: John Snow 
Message-Id: <20201019210430.1063390-1-js...@redhat.com>
Reviewed-by: Eric Blake 
Reviewed-by: Thomas Huth 
Signed-off-by: Markus Armbruster 
---
 scripts/qmp/qmp | 131 +++-
 1 file changed, 7 insertions(+), 124 deletions(-)

diff --git a/scripts/qmp/qmp b/scripts/qmp/qmp
index 8e52e4a54d..0f12307c87 100755
--- a/scripts/qmp/qmp
+++ b/scripts/qmp/qmp
@@ -1,128 +1,11 @@
 #!/usr/bin/env python3
-#
-# QMP command line tool
-#
-# Copyright IBM, Corp. 2011
-#
-# Authors:
-#  Anthony Liguori 
-#
-# This work is licensed under the terms of the GNU GPLv2 or later.
-# See the COPYING file in the top-level directory.
 
-import sys, os
+import sys
 
-sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
-from qemu.qmp import QEMUMonitorProtocol
+print('''This unmaintained and undocumented script was removed in preference
+for qmp-shell. The assumption is that most users are using either
+qmp-shell, socat, or pasting/piping JSON into stdio. The duplication of
+facilities here is unwanted, and the divergence of syntax harmful.''',
+  file=sys.stderr)
 
-def print_response(rsp, prefix=[]):
-if type(rsp) == list:
-i = 0
-for item in rsp:
-if prefix == []:
-prefix = ['item']
-print_response(item, prefix[:-1] + ['%s[%d]' % (prefix[-1], i)])
-i += 1
-elif type(rsp) == dict:
-for key in rsp.keys():
-print_response(rsp[key], prefix + [key])
-else:
-if len(prefix):
-print('%s: %s' % ('.'.join(prefix), rsp))
-else:
-print('%s' % (rsp))
-
-def main(args):
-path = None
-
-# Use QMP_PATH if it's set
-if 'QMP_PATH' in os.environ:
-path = os.environ['QMP_PATH']
-
-while len(args):
-arg = args[0]
-
-if arg.startswith('--'):
-arg = arg[2:]
-if arg.find('=') == -1:
-value = True
-else:
-arg, value = arg.split('=', 1)
-
-if arg in ['path']:
-if type(value) == str:
-path = value
-elif arg in ['help']:
-os.execlp('man', 'man', 'qmp')
-else:
-print('Unknown argument "%s"' % arg)
-
-args = args[1:]
-else:
-break
-
-if not path:
-print("QMP path isn't set, use --path=qmp-monitor-address or set 
QMP_PATH")
-return 1
-
-if len(args):
-command, args = args[0], args[1:]
-else:
-print('No command found')
-print('Usage: "qmp [--path=qmp-monitor-address] qmp-cmd arguments"')
-return 1
-
-if command in ['help']:
-os.execlp('man', 'man', 'qmp')
-
-srv = QEMUMonitorProtocol(path)
-srv.connect()
-
-def do_command(srv, cmd, **kwds):
-rsp = srv.cmd(cmd, kwds)
-if 'error' in rsp:
-raise Exception(rsp['error']['desc'])
-return rsp['return']
-
-commands = map(lambda x: x['name'], do_command(srv, 'query-commands'))
-
-srv.close()
-
-if command not in commands:
-fullcmd = 'qmp-%s' % command
-try:
-os.environ['QMP_PATH'] = path
-os.execvp(fullcmd, [fullcmd] + args)
-except OSError as exc:
-if exc.errno == 2:
-print('Command "%s" not found.' % (fullcmd))
-return 1
-raise
-return 0
-
-srv = QEMUMonitorProtocol(path)
-srv.connect()
-
-arguments = {}
-for arg in args:
-if not arg.startswith('--'):
-print('Unknown argument "%s"' % arg)
-return 1
-
-arg = arg[2:]
-if arg.find('=') == -1:
-value = True
-else:
-arg, value = arg.split('=', 1)
-
-if arg in ['help']:
-os.execlp('man', 'man', 'qmp-%s' % command)
-return 1
-
-arguments[arg] = value
-
-rsp = do_command(srv, command, **arguments)
-print_response(rsp)
-
-if __name__ == '__main__':
-sys.exit(main(sys.argv[1:]))
+sys.exit(1)
-- 
2.26.2

[PULL 0/2] QMP patches patches for 2020-10-27

[Markus Armbruster]

The following changes since commit 1dc887329a10903940501b43e8c0cc67af7c06d5:

  Merge remote-tracking branch 'remotes/philmd-gitlab/tags/sd-next-20201026' 
into staging (2020-10-26 17:19:26 +)

are available in the Git repository at:

  git://repo.or.cz/qemu/armbru.git tags/pull-qmp-2020-10-27

for you to fetch changes up to 36006046675228cf2fec904e717d96ca816cc21a:

  scripts/qmp: delete 'qmp' script (2020-10-27 11:11:06 +0100)


QMP patches patches for 2020-10-27


David Edmondson (1):
  qmp-shell: Sort by key when pretty-printing

John Snow (1):
  scripts/qmp: delete 'qmp' script

 scripts/qmp/qmp   | 131 +++---
 scripts/qmp/qmp-shell |   2 +-
 2 files changed, 8 insertions(+), 125 deletions(-)

-- 
2.26.2

[PULL 1/2] qmp-shell: Sort by key when pretty-printing

[Markus Armbruster]

From: David Edmondson 

If the user selects pretty-printing (-p) the contents of any
dictionaries in the output are sorted by key.

Signed-off-by: David Edmondson 
Message-Id: <20201013141414.18398-1-david.edmond...@oracle.com>
Reviewed-by: Philippe Mathieu-Daudé 
Signed-off-by: Markus Armbruster 
---
 scripts/qmp/qmp-shell | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/qmp/qmp-shell b/scripts/qmp/qmp-shell
index c5eef06f3f..b4d06096ab 100755
--- a/scripts/qmp/qmp-shell
+++ b/scripts/qmp/qmp-shell
@@ -260,7 +260,7 @@ class QMPShell(qmp.QEMUMonitorProtocol):
 indent = None
 if self._pretty:
 indent = 4
-jsobj = json.dumps(qmp, indent=indent)
+jsobj = json.dumps(qmp, indent=indent, sort_keys=self._pretty)
 print(str(jsobj))
 
 def _execute_cmd(self, cmdline):
-- 
2.26.2

Re: [PULL 00/13] 9p queue 2020-10-23

[Christian Schoenebeck]

On Dienstag, 27. Oktober 2020 10:06:53 CET Dr. David Alan Gilbert wrote:
> * Greg Kurz (gr...@kaod.org) wrote:
> > On Mon, 26 Oct 2020 13:48:37 +0100
> > 
> > Christian Schoenebeck  wrote:
> > > On Montag, 26. Oktober 2020 11:33:42 CET Peter Maydell wrote:
> > > > On Fri, 23 Oct 2020 at 12:46, Christian Schoenebeck
> > > > 
> > > >  wrote:
> > > > > The following changes since commit 
4c5b97bfd0dd54dc27717ae8d1cd10e14eef1430:
> > > > >   Merge remote-tracking branch
> > > > >   'remotes/kraxel/tags/modules-20201022-pull-request' into staging
> > > > >   (2020-10-22 12:33:21 +0100)>
> > > > > 
> > > > > are available in the Git repository at:
> > > > >   https://github.com/cschoenebeck/qemu.git tags/pull-9p-20201023
> > > > > 
> > > > > for you to fetch changes up to 
ee01926a11b1f9bffcd6cdec0961dd9d1882da71:
> > > > >   tests/9pfs: add local Tunlinkat hard link test (2020-10-22
> > > > >   20:26:33
> > > > >   +0200)
> > > > > 
> > > > > 
> > > > > 9pfs: more tests using local fs driver
> > > > > 
> > > > > Only 9pfs test case changes this time:
> > > > > 
> > > > > * Refactor: Rename functions to make top-level test functions fs_*()
> > > > > 
> > > > >   easily distinguishable from utility test functions do_*().
> > > > > 
> > > > > * Refactor: Drop unnecessary function arguments in utility test
> > > > > 
> > > > >   functions.
> > > > > 
> > > > > * More test cases using the 9pfs 'local' filesystem driver backend,
> > > > > 
> > > > >   namely for the following 9p requests: Tunlinkat, Tlcreate,
> > > > >   Tsymlink
> > > > >   and Tlink.
> > > > > 
> > > > > 
> > > > 
> > > > I get a 'make check' failure on x86-64 Linux host:
> > > > 
> > > > PASS 54 qtest-x86_64: qos-test
> > > > /x86_64/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-9p-pci/virtio-9p/v
> > > > irtio- 9p-tests/local/config PASS 55 qtest-x86_64: qos-test
> > > > /x86_64/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-9p-pci/virtio-9p/v
> > > > irtio- 9p-tests/local/create_dir PASS 56 qtest-x86_64: qos-test
> > > > /x86_64/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-9p-pci/virtio-9p/v
> > > > irtio- 9p-tests/local/unlinkat_dir PASS 57 qtest-x86_64: qos-test
> > > > /x86_64/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-9p-pci/virtio-9p/v
> > > > irtio- 9p-tests/local/create_file PASS 58 qtest-x86_64: qos-test
> > > > /x86_64/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-9p-pci/virtio-9p/v
> > > > irtio- 9p-tests/local/unlinkat_file PASS 59 qtest-x86_64: qos-test
> > > > /x86_64/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-9p-pci/virtio-9p/v
> > > > irtio- 9p-tests/local/symlink_file Received response 7 (RLERROR)
> > > > instead of 73 (RMKDIR)
> > > > Rlerror has errno 2 (No such file or directory)
> > > > **
> > > > ERROR:../../tests/qtest/virtio-9p-test.c:300:v9fs_req_recv: assertion
> > > > failed (hdr.id == id): (7 == 73)
> > 
> > Not sure this is related to this PR actually. Dave Gilbert reported on irc
> > that he encountered a similar issue with 'make -j check', likely without
> > these patches.
> I was running on current master as of yesterday; no 9p specific patches.
> 
> Dave

They might be related as the "local/create_dir" test is already merged, but 
hard to say reliably without any data.

How is reproducibility, sometimes / always?

> 
> > > > ERROR qtest-x86_64: qos-test - Bail out!
> > > > ERROR:../../tests/qtest/virtio-9ptest.c:300:v9fs_req_recv: assertion
> > > > failed (hdr.id == id): (7 == 73)
> > > > Makefile.mtest:3953: recipe for target 'run-test-492' failed
> > > > 
> > > > 
> > > > thanks
> > > > -- PMM
> > > 
> > > So the 9p server is already failing to create the test case directory
> > > "./qtest-9p-local/05/" relative to your current working directory.
> > > 
> > > I would appreciate to get more info when you have some free cycles, as
> > > I'm
> > > unable to reproduce this on any system unfortunately. But no hurry as
> > > these tests only become relevant actually for QEMU 6.
> > > 
> > > What puzzles me is that the previous test cases succeeded there, which
> > > all
> > > 
> > > create their own test directory in the same way:
> > >   ./qtest-9p-local/01/
> > >   ./qtest-9p-local/02/  (<-- dir vanishes after that test completed)
> > >   ./qtest-9p-local/03/
> > >   ./qtest-9p-local/04/
> > >   ...
> > > 
> > > How does the "./qtest-9p-local/" directory look like after that
> > > "local/symlink_file" test failed there? You can use this shortcut:
> > > 
> > > export QTEST_QEMU_BINARY=x86_64-softmmu/qemu-system-x86_64
> > > cd build
> > > tests/qtest/qos-test --verbose
> > > ls -l qtest-9p-local
> > > 
> > > That latter qos-test run will also output the assembled qemu command
> > > line the 9p local tests would run with, which might also be helpful,
> > > e.g. the relevant output would be something like this:
> > > 
> > > GTest: run:
> > > /x86_64/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-9p-pci

[RFC PATCH v4 1/1] tests/vm: Add Haiku test based on their vagrant images

[Philippe Mathieu-Daudé]

From: Alexander von Gluck IV 

Signed-off-by: Alexander von Gluck IV 
[PMD: Avoid recreating the image each time]
Signed-off-by: Philippe Mathieu-Daudé 
---
 tests/keys/vagrant|  27 +
 tests/keys/vagrant.pub|   1 +
 tests/vm/Makefile.include |   3 +-
 tests/vm/basevm.py|   5 +-
 tests/vm/haiku.x86_64 | 116 ++
 5 files changed, 149 insertions(+), 3 deletions(-)
 create mode 100644 tests/keys/vagrant
 create mode 100644 tests/keys/vagrant.pub
 create mode 100755 tests/vm/haiku.x86_64

diff --git a/tests/keys/vagrant b/tests/keys/vagrant
new file mode 100644
index 000..7d6a083909e
--- /dev/null
+++ b/tests/keys/vagrant
@@ -0,0 +1,27 @@
+-BEGIN RSA PRIVATE KEY-
+MIIEogIBAAKCAQEA6NF8iallvQVp22WDkTkyrtvp9eWW6A8YVr+kz4TjGYe7gHzI
+w+niNltGEFHzD8+v1I2YJ6oXevct1YeS0o9HZyN1Q9qgCgzUFtdOKLv6IedplqoP
+kcmF0aYet2PkEDo3MlTBckFXPITAMzF8dJSIFo9D8HfdOV0IAdx4O7PtixWKn5y2
+hMNG0zQPyUecp4pzC6kivAIhyfHilFR61RGL+GPXQ2MWZWFYbAGjyiYJnAmCP3NO
+Td0jMZEnDkbUvxhMmBYSdETk1rRgm+R4LOzFUGaHqHDLKLX+FIPKcF96hrucXzcW
+yLbIbEgE98OHlnVYCzRdK8jlqm8tehUc9c9WhQIBIwKCAQEA4iqWPJXtzZA68mKd
+ELs4jJsdyky+ewdZeNds5tjcnHU5zUYE25K+ffJED9qUWICcLZDc81TGWjHyAqD1
+Bw7XpgUwFgeUJwUlzQurAv+/ySnxiwuaGJfhFM1CaQHzfXphgVml+fZUvnJUTvzf
+TK2Lg6EdbUE9TarUlBf/xPfuEhMSlIE5keb/Zz3/LUlRg8yDqz5w+QWVJ4utnKnK
+iqwZN0mwpwU7YSyJhlT4YV1F3n4YjLswM5wJs2oqm0jssQu/BT0tyEXNDYBLEF4A
+sClaWuSJ2kjq7KhrrYXzagqhnSei9ODYFShJu8UWVec3Ihb5ZXlzO6vdNQ1J9Xsf
+4m+2ywKBgQD6qFxx/Rv9CNN96l/4rb14HKirC2o/orApiHmHDsURs5rUKDx0f9iP
+cXN7S1uePXuJRK/5hsubaOCx3Owd2u9gD6Oq0CsMkE4CUSiJcYrMANtx54cGH7Rk
+EjFZxK8xAv1ldELEyxrFqkbE4BKd8QOt414qjvTGyAK+OLD3M2QdCQKBgQDtx8pN
+CAxR7yhHbIWT1AH66+XWN8bXq7l3RO/ukeaci98JfkbkxURZhtxV/HHuvUhnPLdX
+3TwygPBYZFNo4pzVEhzWoTtnEtrFueKxyc3+LjZpuo+mBlQ6ORtfgkr9gBVphXZG
+YEzkCD3lVdl8L4cw9BVpKrJCs1c5taGjDgdInQKBgHm/fVvv96bJxc9x1tffXAcj
+3OVdUN0UgXNCSaf/3A/phbeBQe9xS+3mpc4r6qvx+iy69mNBeNZ0xOitIjpjBo2+
+dBEjSBwLk5q5tJqHmy/jKMJL4n9ROlx93XS+njxgibTvU6Fp9w+NOFD/HvxB3Tcz
+6+jJF85D5BNAG3DBMKBjAoGBAOAxZvgsKN+JuENXsST7F89Tck2iTcQIT8g5rwWC
+P9Vt74yboe2kDT531w8+egz7nAmRBKNM751U/95P9t88EDacDI/Z2OwnuFQHCPDF
+llYOUI+SpLJ6/vURRbHSnnn8a/XG+nzedGH5JGqEJNQsz+xT2axM0/W/CRknmGaJ
+kda/AoGANWrLCz708y7VYgAtW2Uf1DPOIYMdvo6fxIB5i9ZfISgcJ/bbCUkFrhoH
++vq/5CIWxCPp0f85R4qxxQ5ihxJ0YDQT9Jpx4TMss4PSavPaBH3RXow5Ohe+bYoQ
+NE5OgEXk2wVfZczCZpigBKbKZHNYcelXtTt/nP3rsCuGcM4h53s=
+-END RSA PRIVATE KEY-
diff --git a/tests/keys/vagrant.pub b/tests/keys/vagrant.pub
new file mode 100644
index 000..b8d012d787f
--- /dev/null
+++ b/tests/keys/vagrant.pub
@@ -0,0 +1 @@
+ssh-rsa 
B3NzaC1yc2EBIwAAAQEA6NF8iallvQVp22WDkTkyrtvp9eWW6A8YVr+kz4TjGYe7gHzIw+niNltGEFHzD8+v1I2YJ6oXevct1YeS0o9HZyN1Q9qgCgzUFtdOKLv6IedplqoPkcmF0aYet2PkEDo3MlTBckFXPITAMzF8dJSIFo9D8HfdOV0IAdx4O7PtixWKn5y2hMNG0zQPyUecp4pzC6kivAIhyfHilFR61RGL+GPXQ2MWZWFYbAGjyiYJnAmCP3NOTd0jMZEnDkbUvxhMmBYSdETk1rRgm+R4LOzFUGaHqHDLKLX+FIPKcF96hrucXzcWyLbIbEgE98OHlnVYCzRdK8jlqm8tehUc9c9WhQ==
 well-known vagrant key for qemu-test, do not use on any machine exposed to an 
external network
diff --git a/tests/vm/Makefile.include b/tests/vm/Makefile.include
index 61f893ffdc0..e94d95ec541 100644
--- a/tests/vm/Makefile.include
+++ b/tests/vm/Makefile.include
@@ -4,7 +4,7 @@
 
 EFI_AARCH64 = $(wildcard $(BUILD_DIR)/pc-bios/edk2-aarch64-code.fd)
 
-IMAGES := freebsd netbsd openbsd centos fedora
+IMAGES := freebsd netbsd openbsd centos fedora haiku.x86_64
 ifneq ($(GENISOIMAGE),)
 IMAGES += ubuntu.i386 centos
 ifneq ($(EFI_AARCH64),)
@@ -41,6 +41,7 @@ endif
 else
@echo "  (install genisoimage to build centos/ubuntu images)"
 endif
+   @echo "  vm-build-haiku.x86_64   - Build QEMU in Haiku VM"
@echo ""
@echo "  vm-build-all- Build QEMU in all VMs"
@echo "  vm-clean-all- Clean up VM images"
diff --git a/tests/vm/basevm.py b/tests/vm/basevm.py
index 3fac20e929a..00f1d5ca8da 100644
--- a/tests/vm/basevm.py
+++ b/tests/vm/basevm.py
@@ -44,6 +44,7 @@
 'machine' : 'pc',
 'guest_user'  : "qemu",
 'guest_pass'  : "qemupass",
+'root_user'   : "root",
 'root_pass'   : "qemupass",
 'ssh_key_file': SSH_KEY_FILE,
 'ssh_pub_key_file': SSH_PUB_KEY_FILE,
@@ -245,13 +246,13 @@ def ssh(self, *cmd):
 return self._ssh_do(self._config["guest_user"], cmd, False)
 
 def ssh_root(self, *cmd):
-return self._ssh_do("root", cmd, False)
+return self._ssh_do(self._config["root_user"], cmd, False)
 
 def ssh_check(self, *cmd):
 self._ssh_do(self._config["guest_user"], cmd, True)
 
 def ssh_root_check(self, *cmd):
-self._ssh_do("root", cmd, True)
+self._ssh_do(self._config["root_user"], cmd, True)
 
 def build_image(self, img):
 raise NotImplementedError
diff --git a/tests/vm/haiku.x86_64 b/tests/vm/haiku.x86_64
new file mode 100755
index 000..634ef774870
--- /dev/null
+++ b/tests/vm/haiku.x86_64
@@ -0,0

[RFC PATCH v4 0/1] tests/vm: Add Haiku VM

[Philippe Mathieu-Daudé]

Intent to not get Alexander's patch lost.

Since v3:
- Grammar fix (eblake)

Since v2:
- No change, posted as single patch

Since v1:
- Addressed Thomas Huth review comments

Alexander von Gluck IV (1):
  tests/vm: Add Haiku test based on their vagrant images

 tests/keys/vagrant|  27 +
 tests/keys/vagrant.pub|   1 +
 tests/vm/Makefile.include |   3 +-
 tests/vm/basevm.py|   5 +-
 tests/vm/haiku.x86_64 | 116 ++
 5 files changed, 149 insertions(+), 3 deletions(-)
 create mode 100644 tests/keys/vagrant
 create mode 100644 tests/keys/vagrant.pub
 create mode 100755 tests/vm/haiku.x86_64

-- 
2.26.2

Re: [PATCH] tests/vm: update openbsd to release 6.8

[Brad Smith]

On Tue, Oct 27, 2020 at 11:05:20AM +0100, Philippe Mathieu-Daud?? wrote:
> On 10/27/20 6:30 AM, Brad Smith wrote:
> > tests/vm: update openbsd to release 6.8
> > 
> > A double dash at the end of a package name removes ambiguity
> > when the intent is to install a non-FLAVORed package.
> > 
> > Signed-off-by: Brad Smith 
> > Reviewed-by: Gerd Hoffmann 
> > Tested-by: Gerd Hoffmann 
> > Reviewed-by: Philippe Mathieu-Daud?? 
> 
> I confirm Brad sent us this patch off-list, and
> - our review comments are addressed,
> - the tags are correct.
> 
> The patch format itself seems broken... Like a copy/paste
> into an email client...

Well, git diff vs a format-patch.


Subject: [PATCH] tests/vm: update openbsd to release 6.8

A double dash at the end of a package name removes ambiguity
when the intent is to install a non-FLAVORed package.

Signed-off-by: Brad Smith 
Reviewed-by: Gerd Hoffmann 
Tested-by: Gerd Hoffmann 
Reviewed-by: Philippe Mathieu-Daud?? 
---
 tests/vm/openbsd | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/tests/vm/openbsd b/tests/vm/openbsd
index 8356646f21..5ffa4f1b37 100755
--- a/tests/vm/openbsd
+++ b/tests/vm/openbsd
@@ -22,8 +22,8 @@ class OpenBSDVM(basevm.BaseVM):
 name = "openbsd"
 arch = "x86_64"
 
-link = "https://cdn.openbsd.org/pub/OpenBSD/6.6/amd64/install66.iso";
-csum = "b22e63df56e6266de6bbeed8e9be0fbe9ee2291551c5bc03f3cc2e4ab9436ee3"
+link = "https://cdn.openbsd.org/pub/OpenBSD/6.8/amd64/install68.iso";
+csum = "47e291fcc2d0c1a8ae0b66329f040b33af755b6adbd21739e20bb5ad56f62b6c"
 size = "20G"
 pkgs = [
 # tools
@@ -36,10 +36,10 @@ class OpenBSDVM(basevm.BaseVM):
 "bash",
 "gmake",
 "gsed",
-"gettext",
+"gettext-tools",
 
 # libs: usb
-"libusb1",
+"libusb1--",
 
 # libs: crypto
 "gnutls",
-- 
2.28.0

Re: [PULL 0/2] bitmaps patches for 2020-10-26

[Peter Maydell]

On Mon, 26 Oct 2020 at 12:05, Eric Blake  wrote:
>
> The following changes since commit 4c5b97bfd0dd54dc27717ae8d1cd10e14eef1430:
>
>   Merge remote-tracking branch 
> 'remotes/kraxel/tags/modules-20201022-pull-request' into staging (2020-10-22 
> 12:33:21 +0100)
>
> are available in the Git repository at:
>
>   https://repo.or.cz/qemu/ericb.git tags/pull-bitmaps-2020-10-26
>
> for you to fetch changes up to a024890a64085d3d37ad7eda164775251285c14c:
>
>   migration/block-dirty-bitmap: fix uninitialized variable warning 
> (2020-10-26 06:56:24 -0500)
>
> 
> bitmaps patches for 2020-10-26
>
> - fix infloop on large bitmap granularity
> - silence compiler warning


Applied, thanks.

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

-- PMM

Re: [PULL 00/13] 9p queue 2020-10-23

[Dr. David Alan Gilbert]

* Christian Schoenebeck (qemu_...@crudebyte.com) wrote:
> On Dienstag, 27. Oktober 2020 10:06:53 CET Dr. David Alan Gilbert wrote:
> > * Greg Kurz (gr...@kaod.org) wrote:
> > > On Mon, 26 Oct 2020 13:48:37 +0100
> > > 
> > > Christian Schoenebeck  wrote:
> > > > On Montag, 26. Oktober 2020 11:33:42 CET Peter Maydell wrote:
> > > > > On Fri, 23 Oct 2020 at 12:46, Christian Schoenebeck
> > > > > 
> > > > >  wrote:
> > > > > > The following changes since commit 
> 4c5b97bfd0dd54dc27717ae8d1cd10e14eef1430:
> > > > > >   Merge remote-tracking branch
> > > > > >   'remotes/kraxel/tags/modules-20201022-pull-request' into staging
> > > > > >   (2020-10-22 12:33:21 +0100)>
> > > > > > 
> > > > > > are available in the Git repository at:
> > > > > >   https://github.com/cschoenebeck/qemu.git tags/pull-9p-20201023
> > > > > > 
> > > > > > for you to fetch changes up to 
> ee01926a11b1f9bffcd6cdec0961dd9d1882da71:
> > > > > >   tests/9pfs: add local Tunlinkat hard link test (2020-10-22
> > > > > >   20:26:33
> > > > > >   +0200)
> > > > > > 
> > > > > > 
> > > > > > 9pfs: more tests using local fs driver
> > > > > > 
> > > > > > Only 9pfs test case changes this time:
> > > > > > 
> > > > > > * Refactor: Rename functions to make top-level test functions fs_*()
> > > > > > 
> > > > > >   easily distinguishable from utility test functions do_*().
> > > > > > 
> > > > > > * Refactor: Drop unnecessary function arguments in utility test
> > > > > > 
> > > > > >   functions.
> > > > > > 
> > > > > > * More test cases using the 9pfs 'local' filesystem driver backend,
> > > > > > 
> > > > > >   namely for the following 9p requests: Tunlinkat, Tlcreate,
> > > > > >   Tsymlink
> > > > > >   and Tlink.
> > > > > > 
> > > > > > 
> > > > > 
> > > > > I get a 'make check' failure on x86-64 Linux host:
> > > > > 
> > > > > PASS 54 qtest-x86_64: qos-test
> > > > > /x86_64/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-9p-pci/virtio-9p/v
> > > > > irtio- 9p-tests/local/config PASS 55 qtest-x86_64: qos-test
> > > > > /x86_64/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-9p-pci/virtio-9p/v
> > > > > irtio- 9p-tests/local/create_dir PASS 56 qtest-x86_64: qos-test
> > > > > /x86_64/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-9p-pci/virtio-9p/v
> > > > > irtio- 9p-tests/local/unlinkat_dir PASS 57 qtest-x86_64: qos-test
> > > > > /x86_64/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-9p-pci/virtio-9p/v
> > > > > irtio- 9p-tests/local/create_file PASS 58 qtest-x86_64: qos-test
> > > > > /x86_64/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-9p-pci/virtio-9p/v
> > > > > irtio- 9p-tests/local/unlinkat_file PASS 59 qtest-x86_64: qos-test
> > > > > /x86_64/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-9p-pci/virtio-9p/v
> > > > > irtio- 9p-tests/local/symlink_file Received response 7 (RLERROR)
> > > > > instead of 73 (RMKDIR)
> > > > > Rlerror has errno 2 (No such file or directory)
> > > > > **
> > > > > ERROR:../../tests/qtest/virtio-9p-test.c:300:v9fs_req_recv: assertion
> > > > > failed (hdr.id == id): (7 == 73)
> > > 
> > > Not sure this is related to this PR actually. Dave Gilbert reported on irc
> > > that he encountered a similar issue with 'make -j check', likely without
> > > these patches.
> > I was running on current master as of yesterday; no 9p specific patches.
> > 
> > Dave
> 
> They might be related as the "local/create_dir" test is already merged, but 
> hard to say reliably without any data.
> 
> How is reproducibility, sometimes / always?

I think I was seeing a few different errors; but I was running make
check -j 32 ish

Dave

> > 
> > > > > ERROR qtest-x86_64: qos-test - Bail out!
> > > > > ERROR:../../tests/qtest/virtio-9ptest.c:300:v9fs_req_recv: assertion
> > > > > failed (hdr.id == id): (7 == 73)
> > > > > Makefile.mtest:3953: recipe for target 'run-test-492' failed
> > > > > 
> > > > > 
> > > > > thanks
> > > > > -- PMM
> > > > 
> > > > So the 9p server is already failing to create the test case directory
> > > > "./qtest-9p-local/05/" relative to your current working directory.
> > > > 
> > > > I would appreciate to get more info when you have some free cycles, as
> > > > I'm
> > > > unable to reproduce this on any system unfortunately. But no hurry as
> > > > these tests only become relevant actually for QEMU 6.
> > > > 
> > > > What puzzles me is that the previous test cases succeeded there, which
> > > > all
> > > > 
> > > > create their own test directory in the same way:
> > > > ./qtest-9p-local/01/
> > > > ./qtest-9p-local/02/  (<-- dir vanishes after that test 
> > > > completed)
> > > > ./qtest-9p-local/03/
> > > > ./qtest-9p-local/04/
> > > > ...
> > > > 
> > > > How does the "./qtest-9p-local/" directory look like after that
> > > > "local/symlink_file" test failed there? You can use this shortcut:
> > > > 
> > > > export QTEST_QEMU_BINARY=x86_64-softmmu/q

[PATCH] hw/i386: -cpu model,-feature,+feature should enable feature

[David Edmondson]

"Minus" features are applied after "plus" features, so ensure that a
later "plus" feature causes an earlier "minus" feature to be removed.

This has no effect on the existing "-feature,feature=on" backward
compatibility code (which warns and turns the feature off).

Signed-off-by: David Edmondson 
---
 target/i386/cpu.c | 31 ---
 1 file changed, 24 insertions(+), 7 deletions(-)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 0d8606958e..c3dcfb868c 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -4720,13 +4720,30 @@ static void x86_cpu_parse_featurestr(const char 
*typename, char *features,
 GlobalProperty *prop;
 
 /* Compatibility syntax: */
-if (featurestr[0] == '+') {
-plus_features = g_list_append(plus_features,
-  g_strdup(featurestr + 1));
-continue;
-} else if (featurestr[0] == '-') {
-minus_features = g_list_append(minus_features,
-   g_strdup(featurestr + 1));
+if (featurestr[0] == '+' || featurestr[0] == '-') {
+const char *feat = featurestr + 1;
+GList *val;
+char *data;
+
+/* Remove any existing +/- setting. */
+val = g_list_find_custom(minus_features, feat, compare_string);
+if (val) {
+data = val->data;
+minus_features = g_list_remove(minus_features, data);
+g_free(data);
+}
+val = g_list_find_custom(plus_features, feat, compare_string);
+if (val) {
+data = val->data;
+plus_features = g_list_remove(plus_features, data);
+g_free(data);
+}
+
+if (featurestr[0] == '+') {
+plus_features = g_list_append(plus_features, g_strdup(feat));
+} else {
+minus_features = g_list_append(minus_features, g_strdup(feat));
+}
 continue;
 }
 
-- 
2.28.0

Re: [PATCH v6 04/11] nbd: Update qapi to support exporting multiple bitmaps

[Vladimir Sementsov-Ogievskiy]

27.10.2020 08:05, Eric Blake wrote:

Since 'block-export-add' is new to 5.2, we can still tweak the
interface; there, allowing 'bitmaps':['str'] is nicer than
'bitmap':'str'.  This wires up the qapi and qemu-nbd changes to permit
passing multiple bitmaps as distinct metadata contexts that the NBD
client may request, but the actual support for more than one will
require a further patch to the server.

Note that there are no changes made to the existing deprecated
'nbd-server-add' command; this required splitting the QAPI type
BlockExportOptionsNbd, which fortunately does not affect QMP
introspection.

Signed-off-by: Eric Blake


Reviewed-by: Vladimir Sementsov-Ogievskiy 

--
Best regards,
Vladimir

Re: [PATCH v6 06/11] nbd: Refactor counting of metadata contexts

[Vladimir Sementsov-Ogievskiy]

27.10.2020 08:05, Eric Blake wrote:

Rather than open-code the count of negotiated contexts at several
sites, embed it directly into the struct.  This will make it easier
for upcoming commits to support even more simultaneous contexts.

Signed-off-by: Eric Blake


Reviewed-by: Vladimir Sementsov-Ogievskiy 

--
Best regards,
Vladimir

Re: [PATCH 1/3] coroutine: let CoQueue wake up outside a coroutine

[Kevin Wolf]

Am 10.10.2020 um 22:41 hat marcandre.lur...@redhat.com geschrieben:
> From: Marc-André Lureau 
> 
> The assert() was added in commit b681a1c73e15 ("block: Repair the
> throttling code."), when the qemu_co_queue_do_restart() function
> required to be running in a coroutine. It was later made unnecessary in
> commit a9d9235567e7 ("coroutine-lock: reschedule coroutine on the
> AioContext it was running on").
> 
> Signed-off-by: Marc-André Lureau 

Reviewed-by: Kevin Wolf 

Re: [PATCH] arm/trace: Fix hex printing

[Peter Maydell]

On Tue, 27 Oct 2020 at 09:24, Auger Eric  wrote:
>
> Hi Peter,
>
> On 10/19/20 9:26 PM, Peter Maydell wrote:
> > Eric, do we want to use hex here, or should we go for
> > decimal the way we do with (almost) all the other
> > tracing of stream IDs (eg mmuv3_cmdq_cfgi_ste in the line before)?
> >
> > The other odd-one-out is smmuv3_find_ste which prints a hex
> > SID; I think the other tracing of SIDs is always decimal.
> I think my preference would be to use hexa here and in the other places.

OK; I'll apply this patch to target-arm.next; feel free to send
a patch updating the other tracepoints to hex.

-- PMM

Re: [PATCH] arm/trace: Fix hex printing

[Auger Eric]

Hi Peter,

On 10/27/20 11:38 AM, Peter Maydell wrote:
> OK; I'll apply this patch to target-arm.next; feel free to send
> a patch updating the other tracepoints to hex.

sure, I will.

Thanks

Eric

[PULL 00/30] nvme emulation patches for 5.2

[Klaus Jensen]

From: Klaus Jensen 

Hi Peter,

The following changes since commit 1dc887329a10903940501b43e8c0cc67af7c06d5:

  Merge remote-tracking branch 'remotes/philmd-gitlab/tags/sd-next-20201026' 
into staging (2020-10-26 17:19:26 +)

are available in the Git repository at:

  git://git.infradead.org/qemu-nvme.git tags/nvme-next-pull-request

for you to fetch changes up to 843c8f91a7ad63f8f3e4e564d3f41f3d030ab8a9:

  hw/block/nvme: fix queue identifer validation (2020-10-27 11:29:25 +0100)


nvme emulation patches for 5.2

  - lots of cleanups
  - add support for scatter/gather lists
  - add support for multiple namespaces (adds new nvme-ns device)
  - change default pci vendor/device id
  - add support for per-namespace smart log



Dmitry Fomichev (1):
  hw/block/nvme: report actual LBA data shift in LBAF

Gollu Appalanaidu (4):
  hw/block/nvme: add support for sgl bit bucket descriptor
  hw/block/nvme: fix prp mapping status codes
  hw/block/nvme: fix create IO SQ/CQ status codes
  hw/block/nvme: fix queue identifer validation

Keith Busch (5):
  hw/block/nvme: remove pointless rw indirection
  hw/block/nvme: fix log page offset check
  hw/block/nvme: support per-namespace smart log
  hw/block/nvme: validate command set selected
  hw/block/nvme: support for admin-only command set

Klaus Jensen (20):
  hw/block/nvme: fix typo in trace event
  pci: pass along the return value of dma_memory_rw
  hw/block/nvme: handle dma errors
  hw/block/nvme: commonize nvme_rw error handling
  hw/block/nvme: alignment style fixes
  hw/block/nvme: add a lba to bytes helper
  hw/block/nvme: fix endian conversion
  hw/block/nvme: add symbolic command name to trace events
  hw/block/nvme: refactor aio submission
  hw/block/nvme: default request status to success
  hw/block/nvme: harden cmb access
  hw/block/nvme: add support for scatter gather lists
  hw/block/nvme: refactor identify active namespace id list
  hw/block/nvme: support multiple namespaces
  pci: allocate pci id for nvme
  hw/block/nvme: change controller pci id
  hw/block/nvme: update nsid when registered
  hw/block/nvme: reject io commands if only admin command set selected
  hw/block/nvme: add nsid to get/setfeat trace events
  hw/block/nvme: add trace event for requests with non-zero status code

 docs/specs/nvme.txt|  23 +
 docs/specs/pci-ids.txt |   1 +
 hw/block/nvme-ns.h |  74 
 hw/block/nvme.h|  83 +++-
 include/block/nvme.h   |  18 +-
 include/hw/pci/pci.h   |   4 +-
 hw/block/nvme-ns.c | 168 
 hw/block/nvme.c| 921 +
 hw/core/machine.c  |   1 +
 MAINTAINERS|   1 +
 hw/block/meson.build   |   2 +-
 hw/block/trace-events  |  32 +-
 12 files changed, 1025 insertions(+), 303 deletions(-)
 create mode 100644 docs/specs/nvme.txt
 create mode 100644 hw/block/nvme-ns.h
 create mode 100644 hw/block/nvme-ns.c

-- 
2.29.1

[PULL 02/30] pci: pass along the return value of dma_memory_rw

[Klaus Jensen]

From: Klaus Jensen 

Some devices might want to know the return value of dma_memory_rw, so
pass it along instead of ignoring it.

There are no existing users of the return value, so this patch should be
safe.

Signed-off-by: Klaus Jensen 
Reviewed-by: Philippe Mathieu-Daudé 
Reviewed-by: Michael S. Tsirkin 
Acked-by: Keith Busch 
---
 include/hw/pci/pci.h | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index 0a59a06b149d..f19ffe6b4fe8 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -783,8 +783,7 @@ static inline AddressSpace *pci_get_address_space(PCIDevice 
*dev)
 static inline int pci_dma_rw(PCIDevice *dev, dma_addr_t addr,
  void *buf, dma_addr_t len, DMADirection dir)
 {
-dma_memory_rw(pci_get_address_space(dev), addr, buf, len, dir);
-return 0;
+return dma_memory_rw(pci_get_address_space(dev), addr, buf, len, dir);
 }
 
 static inline int pci_dma_read(PCIDevice *dev, dma_addr_t addr,
-- 
2.29.1

[PULL 03/30] hw/block/nvme: handle dma errors

[Klaus Jensen]

From: Klaus Jensen 

Handling DMA errors gracefully is required for the device to pass the
block/011 test ("disable PCI device while doing I/O") in the blktests
suite.

With this patch the device sets the Controller Fatal Status bit in the
CSTS register when failing to read from a submission queue or writing to
a completion queue; expecting the host to reset the controller.

If DMA errors occur at any other point in the execution of the command
(say, while mapping the PRPs), the command is aborted with a Data
Transfer Error status code.

Signed-off-by: Klaus Jensen 
Reviewed-by: Keith Busch 
---
 hw/block/nvme.c   | 41 +++--
 hw/block/trace-events |  3 +++
 2 files changed, 34 insertions(+), 10 deletions(-)

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 44fa5b90769b..7d328c08b894 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -140,14 +140,14 @@ static inline void *nvme_addr_to_cmb(NvmeCtrl *n, hwaddr 
addr)
 return &n->cmbuf[addr - n->ctrl_mem.addr];
 }
 
-static void nvme_addr_read(NvmeCtrl *n, hwaddr addr, void *buf, int size)
+static int nvme_addr_read(NvmeCtrl *n, hwaddr addr, void *buf, int size)
 {
 if (n->bar.cmbsz && nvme_addr_is_cmb(n, addr)) {
 memcpy(buf, nvme_addr_to_cmb(n, addr), size);
-return;
+return 0;
 }
 
-pci_dma_read(&n->parent_obj, addr, buf, size);
+return pci_dma_read(&n->parent_obj, addr, buf, size);
 }
 
 static int nvme_check_sqid(NvmeCtrl *n, uint16_t sqid)
@@ -307,6 +307,7 @@ static uint16_t nvme_map_prp(NvmeCtrl *n, uint64_t prp1, 
uint64_t prp2,
 int num_prps = (len >> n->page_bits) + 1;
 uint16_t status;
 bool prp_list_in_cmb = false;
+int ret;
 
 QEMUSGList *qsg = &req->qsg;
 QEMUIOVector *iov = &req->iov;
@@ -347,7 +348,11 @@ static uint16_t nvme_map_prp(NvmeCtrl *n, uint64_t prp1, 
uint64_t prp2,
 
 nents = (len + n->page_size - 1) >> n->page_bits;
 prp_trans = MIN(n->max_prp_ents, nents) * sizeof(uint64_t);
-nvme_addr_read(n, prp2, (void *)prp_list, prp_trans);
+ret = nvme_addr_read(n, prp2, (void *)prp_list, prp_trans);
+if (ret) {
+trace_pci_nvme_err_addr_read(prp2);
+return NVME_DATA_TRAS_ERROR;
+}
 while (len != 0) {
 uint64_t prp_ent = le64_to_cpu(prp_list[i]);
 
@@ -364,8 +369,12 @@ static uint16_t nvme_map_prp(NvmeCtrl *n, uint64_t prp1, 
uint64_t prp2,
 i = 0;
 nents = (len + n->page_size - 1) >> n->page_bits;
 prp_trans = MIN(n->max_prp_ents, nents) * sizeof(uint64_t);
-nvme_addr_read(n, prp_ent, (void *)prp_list,
-prp_trans);
+ret = nvme_addr_read(n, prp_ent, (void *)prp_list,
+ prp_trans);
+if (ret) {
+trace_pci_nvme_err_addr_read(prp_ent);
+return NVME_DATA_TRAS_ERROR;
+}
 prp_ent = le64_to_cpu(prp_list[i]);
 }
 
@@ -457,6 +466,7 @@ static void nvme_post_cqes(void *opaque)
 NvmeCQueue *cq = opaque;
 NvmeCtrl *n = cq->ctrl;
 NvmeRequest *req, *next;
+int ret;
 
 QTAILQ_FOREACH_SAFE(req, &cq->req_list, entry, next) {
 NvmeSQueue *sq;
@@ -466,15 +476,21 @@ static void nvme_post_cqes(void *opaque)
 break;
 }
 
-QTAILQ_REMOVE(&cq->req_list, req, entry);
 sq = req->sq;
 req->cqe.status = cpu_to_le16((req->status << 1) | cq->phase);
 req->cqe.sq_id = cpu_to_le16(sq->sqid);
 req->cqe.sq_head = cpu_to_le16(sq->head);
 addr = cq->dma_addr + cq->tail * n->cqe_size;
+ret = pci_dma_write(&n->parent_obj, addr, (void *)&req->cqe,
+sizeof(req->cqe));
+if (ret) {
+trace_pci_nvme_err_addr_write(addr);
+trace_pci_nvme_err_cfs();
+n->bar.csts = NVME_CSTS_FAILED;
+break;
+}
+QTAILQ_REMOVE(&cq->req_list, req, entry);
 nvme_inc_cq_tail(cq);
-pci_dma_write(&n->parent_obj, addr, (void *)&req->cqe,
-sizeof(req->cqe));
 nvme_req_exit(req);
 QTAILQ_INSERT_TAIL(&sq->req_list, req, entry);
 }
@@ -1606,7 +1622,12 @@ static void nvme_process_sq(void *opaque)
 
 while (!(nvme_sq_empty(sq) || QTAILQ_EMPTY(&sq->req_list))) {
 addr = sq->dma_addr + sq->head * n->sqe_size;
-nvme_addr_read(n, addr, (void *)&cmd, sizeof(cmd));
+if (nvme_addr_read(n, addr, (void *)&cmd, sizeof(cmd))) {
+trace_pci_nvme_err_addr_read(addr);
+trace_pci_nvme_err_cfs();
+n->bar.csts = NVME_CSTS_FAILED;
+break;
+}
 nvme_inc_sq_head(sq);
 
 req = QTAILQ_FIRST(&sq->req_list);
diff --git a/hw/block/trace-events b/hw/block/trace-events
index 8ff4cb

[PULL 05/30] hw/block/nvme: alignment style fixes

[Klaus Jensen]

From: Klaus Jensen 

Style fixes.

Signed-off-by: Klaus Jensen 
Reviewed-by: Philippe Mathieu-Daudé 
Reviewed-by: Keith Busch 
---
 hw/block/nvme.c | 25 +
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 0ac9d856632e..d6d8324fa1de 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -634,7 +634,7 @@ static void nvme_rw_cb(void *opaque, int ret)
 static uint16_t nvme_flush(NvmeCtrl *n, NvmeRequest *req)
 {
 block_acct_start(blk_get_stats(n->conf.blk), &req->acct, 0,
- BLOCK_ACCT_FLUSH);
+ BLOCK_ACCT_FLUSH);
 req->aiocb = blk_aio_flush(n->conf.blk, nvme_rw_cb, req);
 
 return NVME_NO_COMPLETE;
@@ -663,7 +663,7 @@ static uint16_t nvme_write_zeroes(NvmeCtrl *n, NvmeRequest 
*req)
 block_acct_start(blk_get_stats(n->conf.blk), &req->acct, 0,
  BLOCK_ACCT_WRITE);
 req->aiocb = blk_aio_pwrite_zeroes(n->conf.blk, offset, count,
-BDRV_REQ_MAY_UNMAP, nvme_rw_cb, req);
+   BDRV_REQ_MAY_UNMAP, nvme_rw_cb, req);
 return NVME_NO_COMPLETE;
 }
 
@@ -803,7 +803,7 @@ static uint16_t nvme_del_sq(NvmeCtrl *n, NvmeRequest *req)
 }
 
 static void nvme_init_sq(NvmeSQueue *sq, NvmeCtrl *n, uint64_t dma_addr,
-uint16_t sqid, uint16_t cqid, uint16_t size)
+ uint16_t sqid, uint16_t cqid, uint16_t size)
 {
 int i;
 NvmeCQueue *cq;
@@ -1058,7 +1058,8 @@ static uint16_t nvme_del_cq(NvmeCtrl *n, NvmeRequest *req)
 }
 
 static void nvme_init_cq(NvmeCQueue *cq, NvmeCtrl *n, uint64_t dma_addr,
-uint16_t cqid, uint16_t vector, uint16_t size, uint16_t irq_enabled)
+ uint16_t cqid, uint16_t vector, uint16_t size,
+ uint16_t irq_enabled)
 {
 int ret;
 
@@ -1118,7 +1119,7 @@ static uint16_t nvme_create_cq(NvmeCtrl *n, NvmeRequest 
*req)
 
 cq = g_malloc0(sizeof(*cq));
 nvme_init_cq(cq, n, prp1, cqid, vector, qsize + 1,
-NVME_CQ_FLAGS_IEN(qflags));
+ NVME_CQ_FLAGS_IEN(qflags));
 
 /*
  * It is only required to set qs_created when creating a completion queue;
@@ -1515,7 +1516,7 @@ static uint16_t nvme_set_feature(NvmeCtrl *n, NvmeRequest 
*req)
 }
 
 if (((n->temperature >= n->features.temp_thresh_hi) ||
-(n->temperature <= n->features.temp_thresh_low)) &&
+ (n->temperature <= n->features.temp_thresh_low)) &&
 NVME_AEC_SMART(n->features.async_config) & NVME_SMART_TEMPERATURE) 
{
 nvme_enqueue_event(n, NVME_AER_TYPE_SMART,
NVME_AER_INFO_SMART_TEMP_THRESH,
@@ -1765,9 +1766,9 @@ static int nvme_start_ctrl(NvmeCtrl *n)
 n->cqe_size = 1 << NVME_CC_IOCQES(n->bar.cc);
 n->sqe_size = 1 << NVME_CC_IOSQES(n->bar.cc);
 nvme_init_cq(&n->admin_cq, n, n->bar.acq, 0, 0,
-NVME_AQA_ACQS(n->bar.aqa) + 1, 1);
+ NVME_AQA_ACQS(n->bar.aqa) + 1, 1);
 nvme_init_sq(&n->admin_sq, n, n->bar.asq, 0, 0,
-NVME_AQA_ASQS(n->bar.aqa) + 1);
+ NVME_AQA_ASQS(n->bar.aqa) + 1);
 
 nvme_set_timestamp(n, 0ULL);
 
@@ -1777,7 +1778,7 @@ static int nvme_start_ctrl(NvmeCtrl *n)
 }
 
 static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
-unsigned size)
+   unsigned size)
 {
 if (unlikely(offset & (sizeof(uint32_t) - 1))) {
 NVME_GUEST_ERR(pci_nvme_ub_mmiowr_misaligned32,
@@ -1920,7 +1921,7 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, 
uint64_t data,
"invalid write to PMRSWTP register, ignored");
 return;
 case 0xE14: /* TODO PMRMSC */
- break;
+break;
 default:
 NVME_GUEST_ERR(pci_nvme_ub_mmiowr_invalid,
"invalid MMIO write,"
@@ -2096,7 +2097,7 @@ static void nvme_process_db(NvmeCtrl *n, hwaddr addr, int 
val)
 }
 
 static void nvme_mmio_write(void *opaque, hwaddr addr, uint64_t data,
-unsigned size)
+unsigned size)
 {
 NvmeCtrl *n = (NvmeCtrl *)opaque;
 
@@ -2120,7 +2121,7 @@ static const MemoryRegionOps nvme_mmio_ops = {
 };
 
 static void nvme_cmb_write(void *opaque, hwaddr addr, uint64_t data,
-unsigned size)
+   unsigned size)
 {
 NvmeCtrl *n = (NvmeCtrl *)opaque;
 stn_le_p(&n->cmbuf[addr], size, data);
-- 
2.29.1

[PULL 04/30] hw/block/nvme: commonize nvme_rw error handling

[Klaus Jensen]

From: Klaus Jensen 

Move common error handling to a label.

Signed-off-by: Klaus Jensen 
Reviewed-by: Keith Busch 
---
 hw/block/nvme.c | 16 +---
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 7d328c08b894..0ac9d856632e 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -687,20 +687,18 @@ static uint16_t nvme_rw(NvmeCtrl *n, NvmeRequest *req)
 status = nvme_check_mdts(n, data_size);
 if (status) {
 trace_pci_nvme_err_mdts(nvme_cid(req), data_size);
-block_acct_invalid(blk_get_stats(n->conf.blk), acct);
-return status;
+goto invalid;
 }
 
 status = nvme_check_bounds(n, ns, slba, nlb);
 if (status) {
 trace_pci_nvme_err_invalid_lba_range(slba, nlb, ns->id_ns.nsze);
-block_acct_invalid(blk_get_stats(n->conf.blk), acct);
-return status;
+goto invalid;
 }
 
-if (nvme_map_dptr(n, data_size, req)) {
-block_acct_invalid(blk_get_stats(n->conf.blk), acct);
-return NVME_INVALID_FIELD | NVME_DNR;
+status = nvme_map_dptr(n, data_size, req);
+if (status) {
+goto invalid;
 }
 
 if (req->qsg.nsg > 0) {
@@ -722,6 +720,10 @@ static uint16_t nvme_rw(NvmeCtrl *n, NvmeRequest *req)
 }
 
 return NVME_NO_COMPLETE;
+
+invalid:
+block_acct_invalid(blk_get_stats(n->conf.blk), acct);
+return status;
 }
 
 static uint16_t nvme_io_cmd(NvmeCtrl *n, NvmeRequest *req)
-- 
2.29.1

[PULL 01/30] hw/block/nvme: fix typo in trace event

[Klaus Jensen]

From: Klaus Jensen 

Fix a typo in the sq doorbell trace event.

Signed-off-by: Klaus Jensen 
Reviewed-by: Philippe Mathieu-Daudé 
Reviewed-by: Keith Busch 
---
 hw/block/trace-events | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/block/trace-events b/hw/block/trace-events
index ec94c56a4165..8ff4cbc4932c 100644
--- a/hw/block/trace-events
+++ b/hw/block/trace-events
@@ -70,7 +70,7 @@ pci_nvme_enqueue_req_completion(uint16_t cid, uint16_t cqid, 
uint16_t status) "c
 pci_nvme_mmio_read(uint64_t addr) "addr 0x%"PRIx64""
 pci_nvme_mmio_write(uint64_t addr, uint64_t data) "addr 0x%"PRIx64" data 
0x%"PRIx64""
 pci_nvme_mmio_doorbell_cq(uint16_t cqid, uint16_t new_head) "cqid %"PRIu16" 
new_head %"PRIu16""
-pci_nvme_mmio_doorbell_sq(uint16_t sqid, uint16_t new_tail) "cqid %"PRIu16" 
new_tail %"PRIu16""
+pci_nvme_mmio_doorbell_sq(uint16_t sqid, uint16_t new_tail) "sqid %"PRIu16" 
new_tail %"PRIu16""
 pci_nvme_mmio_intm_set(uint64_t data, uint64_t new_mask) "wrote MMIO, 
interrupt mask set, data=0x%"PRIx64", new_mask=0x%"PRIx64""
 pci_nvme_mmio_intm_clr(uint64_t data, uint64_t new_mask) "wrote MMIO, 
interrupt mask clr, data=0x%"PRIx64", new_mask=0x%"PRIx64""
 pci_nvme_mmio_cfg(uint64_t data) "wrote MMIO, config controller 
config=0x%"PRIx64""
-- 
2.29.1

[PULL 06/30] hw/block/nvme: add a lba to bytes helper

[Klaus Jensen]

From: Klaus Jensen 

Add the nvme_l2b helper and use it for converting NLB and SLBA to byte
counts and offsets.

Signed-off-by: Klaus Jensen 
Reviewed-by: Keith Busch 
---
 hw/block/nvme.h |  6 ++
 hw/block/nvme.c | 12 
 2 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/hw/block/nvme.h b/hw/block/nvme.h
index 52ba794f2e9a..1675c1e0755c 100644
--- a/hw/block/nvme.h
+++ b/hw/block/nvme.h
@@ -77,6 +77,12 @@ static inline uint8_t nvme_ns_lbads(NvmeNamespace *ns)
 return nvme_ns_lbaf(ns)->ds;
 }
 
+/* convert an LBA to the equivalent in bytes */
+static inline size_t nvme_l2b(NvmeNamespace *ns, uint64_t lba)
+{
+return lba << nvme_ns_lbads(ns);
+}
+
 #define TYPE_NVME "nvme"
 #define NVME(obj) \
 OBJECT_CHECK(NvmeCtrl, (obj), TYPE_NVME)
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index d6d8324fa1de..59338b42328b 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -644,12 +644,10 @@ static uint16_t nvme_write_zeroes(NvmeCtrl *n, 
NvmeRequest *req)
 {
 NvmeRwCmd *rw = (NvmeRwCmd *)&req->cmd;
 NvmeNamespace *ns = req->ns;
-const uint8_t lba_index = NVME_ID_NS_FLBAS_INDEX(ns->id_ns.flbas);
-const uint8_t data_shift = ns->id_ns.lbaf[lba_index].ds;
 uint64_t slba = le64_to_cpu(rw->slba);
 uint32_t nlb  = le16_to_cpu(rw->nlb) + 1;
-uint64_t offset = slba << data_shift;
-uint32_t count = nlb << data_shift;
+uint64_t offset = nvme_l2b(ns, slba);
+uint32_t count = nvme_l2b(ns, nlb);
 uint16_t status;
 
 trace_pci_nvme_write_zeroes(nvme_cid(req), slba, nlb);
@@ -674,10 +672,8 @@ static uint16_t nvme_rw(NvmeCtrl *n, NvmeRequest *req)
 uint32_t nlb  = le32_to_cpu(rw->nlb) + 1;
 uint64_t slba = le64_to_cpu(rw->slba);
 
-uint8_t lba_index  = NVME_ID_NS_FLBAS_INDEX(ns->id_ns.flbas);
-uint8_t data_shift = ns->id_ns.lbaf[lba_index].ds;
-uint64_t data_size = (uint64_t)nlb << data_shift;
-uint64_t data_offset = slba << data_shift;
+uint64_t data_size = nvme_l2b(ns, nlb);
+uint64_t data_offset = nvme_l2b(ns, slba);
 int is_write = rw->opcode == NVME_CMD_WRITE ? 1 : 0;
 enum BlockAcctType acct = is_write ? BLOCK_ACCT_WRITE : BLOCK_ACCT_READ;
 uint16_t status;
-- 
2.29.1

[PULL 12/30] hw/block/nvme: add support for scatter gather lists

[Klaus Jensen]

From: Klaus Jensen 

For now, support the Data Block, Segment and Last Segment descriptor
types.

See NVM Express 1.3d, Section 4.4 ("Scatter Gather List (SGL)").

Signed-off-by: Klaus Jensen 
Reviewed-by: Keith Busch 
---
 include/block/nvme.h  |   6 +-
 hw/block/nvme.c   | 329 ++
 hw/block/trace-events |   4 +
 3 files changed, 279 insertions(+), 60 deletions(-)

diff --git a/include/block/nvme.h b/include/block/nvme.h
index 65e68a82c897..58647bcdad0b 100644
--- a/include/block/nvme.h
+++ b/include/block/nvme.h
@@ -412,9 +412,9 @@ typedef union NvmeCmdDptr {
 } NvmeCmdDptr;
 
 enum NvmePsdt {
-PSDT_PRP = 0x0,
-PSDT_SGL_MPTR_CONTIGUOUS = 0x1,
-PSDT_SGL_MPTR_SGL= 0x2,
+NVME_PSDT_PRP = 0x0,
+NVME_PSDT_SGL_MPTR_CONTIGUOUS = 0x1,
+NVME_PSDT_SGL_MPTR_SGL= 0x2,
 };
 
 typedef struct QEMU_PACKED NvmeCmd {
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index c0f1f8ccd473..63d0a17bc5f0 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -413,13 +413,262 @@ static uint16_t nvme_map_prp(NvmeCtrl *n, uint64_t prp1, 
uint64_t prp2,
 return NVME_SUCCESS;
 }
 
-static uint16_t nvme_dma_prp(NvmeCtrl *n, uint8_t *ptr, uint32_t len,
- uint64_t prp1, uint64_t prp2, DMADirection dir,
+/*
+ * Map 'nsgld' data descriptors from 'segment'. The function will subtract the
+ * number of bytes mapped in len.
+ */
+static uint16_t nvme_map_sgl_data(NvmeCtrl *n, QEMUSGList *qsg,
+  QEMUIOVector *iov,
+  NvmeSglDescriptor *segment, uint64_t nsgld,
+  size_t *len, NvmeRequest *req)
+{
+dma_addr_t addr, trans_len;
+uint32_t dlen;
+uint16_t status;
+
+for (int i = 0; i < nsgld; i++) {
+uint8_t type = NVME_SGL_TYPE(segment[i].type);
+
+switch (type) {
+case NVME_SGL_DESCR_TYPE_DATA_BLOCK:
+break;
+case NVME_SGL_DESCR_TYPE_SEGMENT:
+case NVME_SGL_DESCR_TYPE_LAST_SEGMENT:
+return NVME_INVALID_NUM_SGL_DESCRS | NVME_DNR;
+default:
+return NVME_SGL_DESCR_TYPE_INVALID | NVME_DNR;
+}
+
+dlen = le32_to_cpu(segment[i].len);
+if (!dlen) {
+continue;
+}
+
+if (*len == 0) {
+/*
+ * All data has been mapped, but the SGL contains additional
+ * segments and/or descriptors. The controller might accept
+ * ignoring the rest of the SGL.
+ */
+uint16_t sgls = le16_to_cpu(n->id_ctrl.sgls);
+if (sgls & NVME_CTRL_SGLS_EXCESS_LENGTH) {
+break;
+}
+
+trace_pci_nvme_err_invalid_sgl_excess_length(nvme_cid(req));
+return NVME_DATA_SGL_LEN_INVALID | NVME_DNR;
+}
+
+trans_len = MIN(*len, dlen);
+addr = le64_to_cpu(segment[i].addr);
+
+if (UINT64_MAX - addr < dlen) {
+return NVME_DATA_SGL_LEN_INVALID | NVME_DNR;
+}
+
+status = nvme_map_addr(n, qsg, iov, addr, trans_len);
+if (status) {
+return status;
+}
+
+*len -= trans_len;
+}
+
+return NVME_SUCCESS;
+}
+
+static uint16_t nvme_map_sgl(NvmeCtrl *n, QEMUSGList *qsg, QEMUIOVector *iov,
+ NvmeSglDescriptor sgl, size_t len,
  NvmeRequest *req)
+{
+/*
+ * Read the segment in chunks of 256 descriptors (one 4k page) to avoid
+ * dynamically allocating a potentially huge SGL. The spec allows the SGL
+ * to be larger (as in number of bytes required to describe the SGL
+ * descriptors and segment chain) than the command transfer size, so it is
+ * not bounded by MDTS.
+ */
+const int SEG_CHUNK_SIZE = 256;
+
+NvmeSglDescriptor segment[SEG_CHUNK_SIZE], *sgld, *last_sgld;
+uint64_t nsgld;
+uint32_t seg_len;
+uint16_t status;
+bool sgl_in_cmb = false;
+hwaddr addr;
+int ret;
+
+sgld = &sgl;
+addr = le64_to_cpu(sgl.addr);
+
+trace_pci_nvme_map_sgl(nvme_cid(req), NVME_SGL_TYPE(sgl.type), len);
+
+/*
+ * If the entire transfer can be described with a single data block it can
+ * be mapped directly.
+ */
+if (NVME_SGL_TYPE(sgl.type) == NVME_SGL_DESCR_TYPE_DATA_BLOCK) {
+status = nvme_map_sgl_data(n, qsg, iov, sgld, 1, &len, req);
+if (status) {
+goto unmap;
+}
+
+goto out;
+}
+
+/*
+ * If the segment is located in the CMB, the submission queue of the
+ * request must also reside there.
+ */
+if (nvme_addr_is_cmb(n, addr)) {
+if (!nvme_addr_is_cmb(n, req->sq->dma_addr)) {
+return NVME_INVALID_USE_OF_CMB | NVME_DNR;
+}
+
+sgl_in_cmb = true;
+}
+
+for (;;) {
+switch (NVME_SGL_TYPE(sgld->type)) {
+case NVME_SGL_DESCR_TYPE_SEGMENT:
+ca

[PULL 07/30] hw/block/nvme: fix endian conversion

[Klaus Jensen]

From: Klaus Jensen 

The raw NLB field is a 16 bit value, so use le16_to_cpu instead of
le32_to_cpu and cast to uint32_t before incrementing the value to not
wrap around.

Signed-off-by: Klaus Jensen 
Reviewed-by: Keith Busch 
Reviewed-by: Philippe Mathieu-Daudé 
---
 hw/block/nvme.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 59338b42328b..158843c14a29 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -645,7 +645,7 @@ static uint16_t nvme_write_zeroes(NvmeCtrl *n, NvmeRequest 
*req)
 NvmeRwCmd *rw = (NvmeRwCmd *)&req->cmd;
 NvmeNamespace *ns = req->ns;
 uint64_t slba = le64_to_cpu(rw->slba);
-uint32_t nlb  = le16_to_cpu(rw->nlb) + 1;
+uint32_t nlb = (uint32_t)le16_to_cpu(rw->nlb) + 1;
 uint64_t offset = nvme_l2b(ns, slba);
 uint32_t count = nvme_l2b(ns, nlb);
 uint16_t status;
@@ -669,7 +669,7 @@ static uint16_t nvme_rw(NvmeCtrl *n, NvmeRequest *req)
 {
 NvmeRwCmd *rw = (NvmeRwCmd *)&req->cmd;
 NvmeNamespace *ns = req->ns;
-uint32_t nlb  = le32_to_cpu(rw->nlb) + 1;
+uint32_t nlb = (uint32_t)le16_to_cpu(rw->nlb) + 1;
 uint64_t slba = le64_to_cpu(rw->slba);
 
 uint64_t data_size = nvme_l2b(ns, nlb);
-- 
2.29.1

[PULL 09/30] hw/block/nvme: refactor aio submission

[Klaus Jensen]

From: Klaus Jensen 

This pulls block layer aio submission/completion to common functions.

For completions, additionally map an AIO error to the Unrecovered Read
and Write Fault status codes.

Signed-off-by: Klaus Jensen 
Reviewed-by: Keith Busch 
---
 hw/block/nvme.h   |  14 +
 hw/block/nvme.c   | 136 ++
 hw/block/trace-events |   4 +-
 3 files changed, 114 insertions(+), 40 deletions(-)

diff --git a/hw/block/nvme.h b/hw/block/nvme.h
index ce9e931420d7..f355eccb323b 100644
--- a/hw/block/nvme.h
+++ b/hw/block/nvme.h
@@ -171,4 +171,18 @@ static inline uint64_t nvme_ns_nlbas(NvmeCtrl *n, 
NvmeNamespace *ns)
 return n->ns_size >> nvme_ns_lbads(ns);
 }
 
+static inline NvmeCQueue *nvme_cq(NvmeRequest *req)
+{
+NvmeSQueue *sq = req->sq;
+NvmeCtrl *n = sq->ctrl;
+
+return n->cq[sq->cqid];
+}
+
+static inline NvmeCtrl *nvme_ctrl(NvmeRequest *req)
+{
+NvmeSQueue *sq = req->sq;
+return sq->ctrl;
+}
+
 #endif /* HW_NVME_H */
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 961e6ffc5b67..84cde40fad56 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -614,30 +614,110 @@ static inline uint16_t nvme_check_bounds(NvmeCtrl *n, 
NvmeNamespace *ns,
 static void nvme_rw_cb(void *opaque, int ret)
 {
 NvmeRequest *req = opaque;
-NvmeSQueue *sq = req->sq;
-NvmeCtrl *n = sq->ctrl;
-NvmeCQueue *cq = n->cq[sq->cqid];
+NvmeCtrl *n = nvme_ctrl(req);
 
-trace_pci_nvme_rw_cb(nvme_cid(req));
+BlockBackend *blk = n->conf.blk;
+BlockAcctCookie *acct = &req->acct;
+BlockAcctStats *stats = blk_get_stats(blk);
+
+Error *local_err = NULL;
+
+trace_pci_nvme_rw_cb(nvme_cid(req), blk_name(blk));
 
 if (!ret) {
-block_acct_done(blk_get_stats(n->conf.blk), &req->acct);
+block_acct_done(stats, acct);
 req->status = NVME_SUCCESS;
 } else {
-block_acct_failed(blk_get_stats(n->conf.blk), &req->acct);
-req->status = NVME_INTERNAL_DEV_ERROR;
+uint16_t status;
+
+block_acct_failed(stats, acct);
+
+switch (req->cmd.opcode) {
+case NVME_CMD_READ:
+status = NVME_UNRECOVERED_READ;
+break;
+case NVME_CMD_FLUSH:
+case NVME_CMD_WRITE:
+case NVME_CMD_WRITE_ZEROES:
+status = NVME_WRITE_FAULT;
+break;
+default:
+status = NVME_INTERNAL_DEV_ERROR;
+break;
+}
+
+trace_pci_nvme_err_aio(nvme_cid(req), strerror(ret), status);
+
+error_setg_errno(&local_err, -ret, "aio failed");
+error_report_err(local_err);
+
+req->status = status;
 }
 
-nvme_enqueue_req_completion(cq, req);
+nvme_enqueue_req_completion(nvme_cq(req), req);
+}
+
+static uint16_t nvme_do_aio(BlockBackend *blk, int64_t offset, size_t len,
+NvmeRequest *req)
+{
+BlockAcctCookie *acct = &req->acct;
+BlockAcctStats *stats = blk_get_stats(blk);
+
+bool is_write = false;
+
+trace_pci_nvme_do_aio(nvme_cid(req), req->cmd.opcode,
+  nvme_io_opc_str(req->cmd.opcode), blk_name(blk),
+  offset, len);
+
+switch (req->cmd.opcode) {
+case NVME_CMD_FLUSH:
+block_acct_start(stats, acct, 0, BLOCK_ACCT_FLUSH);
+req->aiocb = blk_aio_flush(blk, nvme_rw_cb, req);
+break;
+
+case NVME_CMD_WRITE_ZEROES:
+block_acct_start(stats, acct, len, BLOCK_ACCT_WRITE);
+req->aiocb = blk_aio_pwrite_zeroes(blk, offset, len,
+   BDRV_REQ_MAY_UNMAP, nvme_rw_cb,
+   req);
+break;
+
+case NVME_CMD_WRITE:
+is_write = true;
+
+/* fallthrough */
+
+case NVME_CMD_READ:
+block_acct_start(stats, acct, len,
+ is_write ? BLOCK_ACCT_WRITE : BLOCK_ACCT_READ);
+
+if (req->qsg.sg) {
+if (is_write) {
+req->aiocb = dma_blk_write(blk, &req->qsg, offset,
+   BDRV_SECTOR_SIZE, nvme_rw_cb, req);
+} else {
+req->aiocb = dma_blk_read(blk, &req->qsg, offset,
+  BDRV_SECTOR_SIZE, nvme_rw_cb, req);
+}
+} else {
+if (is_write) {
+req->aiocb = blk_aio_pwritev(blk, offset, &req->iov, 0,
+ nvme_rw_cb, req);
+} else {
+req->aiocb = blk_aio_preadv(blk, offset, &req->iov, 0,
+nvme_rw_cb, req);
+}
+}
+
+break;
+}
+
+return NVME_NO_COMPLETE;
 }
 
 static uint16_t nvme_flush(NvmeCtrl *n, NvmeRequest *req)
 {
-block_acct_start(blk_get_stats(n->conf.blk), &req->acct, 0,
- BLOCK_ACCT_FLUSH);
-req->aiocb = blk_aio_flush(n->conf.blk, nvme_rw_cb, req);
-
-return NVME_NO_CO

[PULL 08/30] hw/block/nvme: add symbolic command name to trace events

[Klaus Jensen]

From: Klaus Jensen 

Add the symbolic command name to the pci_nvme_{io,admin}_cmd and
pci_nvme_rw trace events.

Signed-off-by: Klaus Jensen 
Reviewed-by: Philippe Mathieu-Daudé 
Reviewed-by: Keith Busch 
---
 hw/block/nvme.h   | 28 
 hw/block/nvme.c   |  8 +---
 hw/block/trace-events |  6 +++---
 3 files changed, 36 insertions(+), 6 deletions(-)

diff --git a/hw/block/nvme.h b/hw/block/nvme.h
index 1675c1e0755c..ce9e931420d7 100644
--- a/hw/block/nvme.h
+++ b/hw/block/nvme.h
@@ -32,6 +32,34 @@ typedef struct NvmeRequest {
 QTAILQ_ENTRY(NvmeRequest)entry;
 } NvmeRequest;
 
+static inline const char *nvme_adm_opc_str(uint8_t opc)
+{
+switch (opc) {
+case NVME_ADM_CMD_DELETE_SQ:return "NVME_ADM_CMD_DELETE_SQ";
+case NVME_ADM_CMD_CREATE_SQ:return "NVME_ADM_CMD_CREATE_SQ";
+case NVME_ADM_CMD_GET_LOG_PAGE: return "NVME_ADM_CMD_GET_LOG_PAGE";
+case NVME_ADM_CMD_DELETE_CQ:return "NVME_ADM_CMD_DELETE_CQ";
+case NVME_ADM_CMD_CREATE_CQ:return "NVME_ADM_CMD_CREATE_CQ";
+case NVME_ADM_CMD_IDENTIFY: return "NVME_ADM_CMD_IDENTIFY";
+case NVME_ADM_CMD_ABORT:return "NVME_ADM_CMD_ABORT";
+case NVME_ADM_CMD_SET_FEATURES: return "NVME_ADM_CMD_SET_FEATURES";
+case NVME_ADM_CMD_GET_FEATURES: return "NVME_ADM_CMD_GET_FEATURES";
+case NVME_ADM_CMD_ASYNC_EV_REQ: return "NVME_ADM_CMD_ASYNC_EV_REQ";
+default:return "NVME_ADM_CMD_UNKNOWN";
+}
+}
+
+static inline const char *nvme_io_opc_str(uint8_t opc)
+{
+switch (opc) {
+case NVME_CMD_FLUSH:return "NVME_NVM_CMD_FLUSH";
+case NVME_CMD_WRITE:return "NVME_NVM_CMD_WRITE";
+case NVME_CMD_READ: return "NVME_NVM_CMD_READ";
+case NVME_CMD_WRITE_ZEROES: return "NVME_NVM_CMD_WRITE_ZEROES";
+default:return "NVME_NVM_CMD_UNKNOWN";
+}
+}
+
 typedef struct NvmeSQueue {
 struct NvmeCtrl *ctrl;
 uint16_tsqid;
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 158843c14a29..961e6ffc5b67 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -678,7 +678,8 @@ static uint16_t nvme_rw(NvmeCtrl *n, NvmeRequest *req)
 enum BlockAcctType acct = is_write ? BLOCK_ACCT_WRITE : BLOCK_ACCT_READ;
 uint16_t status;
 
-trace_pci_nvme_rw(is_write ? "write" : "read", nlb, data_size, slba);
+trace_pci_nvme_rw(nvme_cid(req), nvme_io_opc_str(rw->opcode), nlb,
+  data_size, slba);
 
 status = nvme_check_mdts(n, data_size);
 if (status) {
@@ -727,7 +728,7 @@ static uint16_t nvme_io_cmd(NvmeCtrl *n, NvmeRequest *req)
 uint32_t nsid = le32_to_cpu(req->cmd.nsid);
 
 trace_pci_nvme_io_cmd(nvme_cid(req), nsid, nvme_sqid(req),
-  req->cmd.opcode);
+  req->cmd.opcode, nvme_io_opc_str(req->cmd.opcode));
 
 if (unlikely(nsid == 0 || nsid > n->num_namespaces)) {
 trace_pci_nvme_err_invalid_ns(nsid, n->num_namespaces);
@@ -1579,7 +1580,8 @@ static uint16_t nvme_aer(NvmeCtrl *n, NvmeRequest *req)
 
 static uint16_t nvme_admin_cmd(NvmeCtrl *n, NvmeRequest *req)
 {
-trace_pci_nvme_admin_cmd(nvme_cid(req), nvme_sqid(req), req->cmd.opcode);
+trace_pci_nvme_admin_cmd(nvme_cid(req), nvme_sqid(req), req->cmd.opcode,
+ nvme_adm_opc_str(req->cmd.opcode));
 
 switch (req->cmd.opcode) {
 case NVME_ADM_CMD_DELETE_SQ:
diff --git a/hw/block/trace-events b/hw/block/trace-events
index 5589db4a014f..024786f4833c 100644
--- a/hw/block/trace-events
+++ b/hw/block/trace-events
@@ -36,9 +36,9 @@ pci_nvme_dma_read(uint64_t prp1, uint64_t prp2) "DMA read, 
prp1=0x%"PRIx64" prp2
 pci_nvme_map_addr(uint64_t addr, uint64_t len) "addr 0x%"PRIx64" len %"PRIu64""
 pci_nvme_map_addr_cmb(uint64_t addr, uint64_t len) "addr 0x%"PRIx64" len 
%"PRIu64""
 pci_nvme_map_prp(uint64_t trans_len, uint32_t len, uint64_t prp1, uint64_t 
prp2, int num_prps) "trans_len %"PRIu64" len %"PRIu32" prp1 0x%"PRIx64" prp2 
0x%"PRIx64" num_prps %d"
-pci_nvme_io_cmd(uint16_t cid, uint32_t nsid, uint16_t sqid, uint8_t opcode) 
"cid %"PRIu16" nsid %"PRIu32" sqid %"PRIu16" opc 0x%"PRIx8""
-pci_nvme_admin_cmd(uint16_t cid, uint16_t sqid, uint8_t opcode) "cid %"PRIu16" 
sqid %"PRIu16" opc 0x%"PRIx8""
-pci_nvme_rw(const char *verb, uint32_t blk_count, uint64_t byte_count, 
uint64_t lba) "%s %"PRIu32" blocks (%"PRIu64" bytes) from LBA %"PRIu64""
+pci_nvme_io_cmd(uint16_t cid, uint32_t nsid, uint16_t sqid, uint8_t opcode, 
const char *opname) "cid %"PRIu16" nsid %"PRIu32" sqid %"PRIu16" opc 0x%"PRIx8" 
opname '%s'"
+pci_nvme_admin_cmd(uint16_t cid, uint16_t sqid, uint8_t opcode, const char 
*opname) "cid %"PRIu16" sqid %"PRIu16" opc 0x%"PRIx8" opname '%s'"
+pci_nvme_rw(uint16_t cid, const char *verb, uint32_t nlb, uint64_t count, 
uint64_t lba) "cid %"PRIu16" '%s' nlb %"PRIu32" count %"PRIu64" lba 0x%"PRIx64""
 pci_nvme_rw_cb(uint16_t cid) "cid %"PR

[PULL 17/30] hw/block/nvme: change controller pci id

[Klaus Jensen]

From: Klaus Jensen 

There are two reasons for changing this:

  1. The nvme device currently uses an internal Intel device id.

  2. Since commits "nvme: fix write zeroes offset and count" and "nvme:
 support multiple namespaces" the controller device no longer has
 the quirks that the Linux kernel think it has.

 As the quirks are applied based on pci vendor and device id, change
 them to get rid of the quirks.

To keep backward compatibility, add a new 'use-intel-id' parameter to
the nvme device to force use of the Intel vendor and device id. This is
off by default but add a compat property to set this for 5.1 machines
and older. If a 5.1 machine is booted (or the use-intel-id parameter is
explicitly set to true), the Linux kernel will just apply these
unnecessary quirks:

  1. NVME_QUIRK_IDENTIFY_CNS which says that the device does not support
 anything else than values 0x0 and 0x1 for CNS (Identify Namespace
 and Identify Namespace). With multiple namespace support, this just
 means that the kernel will "scan" namespaces instead of using
 "Active Namespace ID list" (CNS 0x2).

  2. NVME_QUIRK_DISABLE_WRITE_ZEROES. The nvme device started out with a
 broken Write Zeroes implementation which has since been fixed in
 commit 9d6459d21a6e ("nvme: fix write zeroes offset and count").

Signed-off-by: Klaus Jensen 
Reviewed-by: Keith Busch 
Reviewed-by: Maxim Levitsky 
---
 hw/block/nvme.h   |  1 +
 hw/block/nvme.c   | 12 ++--
 hw/core/machine.c |  1 +
 3 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/hw/block/nvme.h b/hw/block/nvme.h
index d96ec15cdffb..e080a2318a50 100644
--- a/hw/block/nvme.h
+++ b/hw/block/nvme.h
@@ -15,6 +15,7 @@ typedef struct NvmeParams {
 uint8_t  aerl;
 uint32_t aer_max_queued;
 uint8_t  mdts;
+bool use_intel_id;
 } NvmeParams;
 
 typedef struct NvmeAsyncEvent {
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 1af12f861ac0..5768a6804f41 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -2678,6 +2678,15 @@ static void nvme_init_pci(NvmeCtrl *n, PCIDevice 
*pci_dev, Error **errp)
 
 pci_conf[PCI_INTERRUPT_PIN] = 1;
 pci_config_set_prog_interface(pci_conf, 0x2);
+
+if (n->params.use_intel_id) {
+pci_config_set_vendor_id(pci_conf, PCI_VENDOR_ID_INTEL);
+pci_config_set_device_id(pci_conf, 0x5845);
+} else {
+pci_config_set_vendor_id(pci_conf, PCI_VENDOR_ID_REDHAT);
+pci_config_set_device_id(pci_conf, PCI_DEVICE_ID_REDHAT_NVME);
+}
+
 pci_config_set_class(pci_conf, PCI_CLASS_STORAGE_EXPRESS);
 pcie_endpoint_cap_init(pci_dev, 0x80);
 
@@ -2831,6 +2840,7 @@ static Property nvme_props[] = {
 DEFINE_PROP_UINT8("aerl", NvmeCtrl, params.aerl, 3),
 DEFINE_PROP_UINT32("aer_max_queued", NvmeCtrl, params.aer_max_queued, 64),
 DEFINE_PROP_UINT8("mdts", NvmeCtrl, params.mdts, 7),
+DEFINE_PROP_BOOL("use-intel-id", NvmeCtrl, params.use_intel_id, false),
 DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -2847,8 +2857,6 @@ static void nvme_class_init(ObjectClass *oc, void *data)
 pc->realize = nvme_realize;
 pc->exit = nvme_exit;
 pc->class_id = PCI_CLASS_STORAGE_EXPRESS;
-pc->vendor_id = PCI_VENDOR_ID_INTEL;
-pc->device_id = 0x5845;
 pc->revision = 2;
 
 set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
diff --git a/hw/core/machine.c b/hw/core/machine.c
index c5e0e79e6dbc..98b87f76cbbe 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -34,6 +34,7 @@ GlobalProperty hw_compat_5_1[] = {
 { "vhost-user-scsi", "num_queues", "1"},
 { "virtio-blk-device", "num-queues", "1"},
 { "virtio-scsi-device", "num_queues", "1"},
+{ "nvme", "use-intel-id", "on"},
 };
 const size_t hw_compat_5_1_len = G_N_ELEMENTS(hw_compat_5_1);
 
-- 
2.29.1

[PULL 10/30] hw/block/nvme: default request status to success

[Klaus Jensen]

From: Klaus Jensen 

Make the default request status NVME_SUCCESS so only error status codes
have to be set.

Signed-off-by: Klaus Jensen 
Reviewed-by: Philippe Mathieu-Daudé 
Reviewed-by: Keith Busch 
---
 hw/block/nvme.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 84cde40fad56..0e916d48d763 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -230,6 +230,7 @@ static void nvme_req_clear(NvmeRequest *req)
 {
 req->ns = NULL;
 memset(&req->cqe, 0x0, sizeof(req->cqe));
+req->status = NVME_SUCCESS;
 }
 
 static void nvme_req_exit(NvmeRequest *req)
@@ -546,8 +547,6 @@ static void nvme_process_aers(void *opaque)
 result->log_page = event->result.log_page;
 g_free(event);
 
-req->status = NVME_SUCCESS;
-
 trace_pci_nvme_aer_post_cqe(result->event_type, result->event_info,
 result->log_page);
 
@@ -626,7 +625,6 @@ static void nvme_rw_cb(void *opaque, int ret)
 
 if (!ret) {
 block_acct_done(stats, acct);
-req->status = NVME_SUCCESS;
 } else {
 uint16_t status;
 
-- 
2.29.1

[PULL 15/30] hw/block/nvme: support multiple namespaces

[Klaus Jensen]

From: Klaus Jensen 

This adds support for multiple namespaces by introducing a new 'nvme-ns'
device model. The nvme device creates a bus named from the device name
('id'). The nvme-ns devices then connect to this and registers
themselves with the nvme device.

This changes how an nvme device is created. Example with two namespaces:

  -drive file=nvme0n1.img,if=none,id=disk1
  -drive file=nvme0n2.img,if=none,id=disk2
  -device nvme,serial=deadbeef,id=nvme0
  -device nvme-ns,drive=disk1,bus=nvme0,nsid=1
  -device nvme-ns,drive=disk2,bus=nvme0,nsid=2

The drive property is kept on the nvme device to keep the change
backward compatible, but the property is now optional. Specifying a
drive for the nvme device will always create the namespace with nsid 1.

Signed-off-by: Klaus Jensen 
Reviewed-by: Keith Busch 
Reviewed-by: Minwoo Im 
---
 hw/block/nvme-ns.h|  74 +
 hw/block/nvme.h   |  46 
 hw/block/nvme-ns.c| 167 
 hw/block/nvme.c   | 249 +++---
 hw/block/meson.build  |   2 +-
 hw/block/trace-events |   6 +-
 6 files changed, 428 insertions(+), 116 deletions(-)
 create mode 100644 hw/block/nvme-ns.h
 create mode 100644 hw/block/nvme-ns.c

diff --git a/hw/block/nvme-ns.h b/hw/block/nvme-ns.h
new file mode 100644
index ..83734f4606e1
--- /dev/null
+++ b/hw/block/nvme-ns.h
@@ -0,0 +1,74 @@
+/*
+ * QEMU NVM Express Virtual Namespace
+ *
+ * Copyright (c) 2019 CNEX Labs
+ * Copyright (c) 2020 Samsung Electronics
+ *
+ * Authors:
+ *  Klaus Jensen  
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2. See the
+ * COPYING file in the top-level directory.
+ *
+ */
+
+#ifndef NVME_NS_H
+#define NVME_NS_H
+
+#define TYPE_NVME_NS "nvme-ns"
+#define NVME_NS(obj) \
+OBJECT_CHECK(NvmeNamespace, (obj), TYPE_NVME_NS)
+
+typedef struct NvmeNamespaceParams {
+uint32_t nsid;
+} NvmeNamespaceParams;
+
+typedef struct NvmeNamespace {
+DeviceState  parent_obj;
+BlockConfblkconf;
+int32_t  bootindex;
+int64_t  size;
+NvmeIdNs id_ns;
+
+NvmeNamespaceParams params;
+} NvmeNamespace;
+
+static inline uint32_t nvme_nsid(NvmeNamespace *ns)
+{
+if (ns) {
+return ns->params.nsid;
+}
+
+return -1;
+}
+
+static inline NvmeLBAF *nvme_ns_lbaf(NvmeNamespace *ns)
+{
+NvmeIdNs *id_ns = &ns->id_ns;
+return &id_ns->lbaf[NVME_ID_NS_FLBAS_INDEX(id_ns->flbas)];
+}
+
+static inline uint8_t nvme_ns_lbads(NvmeNamespace *ns)
+{
+return nvme_ns_lbaf(ns)->ds;
+}
+
+/* calculate the number of LBAs that the namespace can accomodate */
+static inline uint64_t nvme_ns_nlbas(NvmeNamespace *ns)
+{
+return ns->size >> nvme_ns_lbads(ns);
+}
+
+/* convert an LBA to the equivalent in bytes */
+static inline size_t nvme_l2b(NvmeNamespace *ns, uint64_t lba)
+{
+return lba << nvme_ns_lbads(ns);
+}
+
+typedef struct NvmeCtrl NvmeCtrl;
+
+int nvme_ns_setup(NvmeCtrl *n, NvmeNamespace *ns, Error **errp);
+void nvme_ns_drain(NvmeNamespace *ns);
+void nvme_ns_flush(NvmeNamespace *ns);
+
+#endif /* NVME_NS_H */
diff --git a/hw/block/nvme.h b/hw/block/nvme.h
index f355eccb323b..d96ec15cdffb 100644
--- a/hw/block/nvme.h
+++ b/hw/block/nvme.h
@@ -2,6 +2,9 @@
 #define HW_NVME_H
 
 #include "block/nvme.h"
+#include "nvme-ns.h"
+
+#define NVME_MAX_NAMESPACES 256
 
 typedef struct NvmeParams {
 char *serial;
@@ -90,26 +93,12 @@ typedef struct NvmeCQueue {
 QTAILQ_HEAD(, NvmeRequest) req_list;
 } NvmeCQueue;
 
-typedef struct NvmeNamespace {
-NvmeIdNsid_ns;
-} NvmeNamespace;
+#define TYPE_NVME_BUS "nvme-bus"
+#define NVME_BUS(obj) OBJECT_CHECK(NvmeBus, (obj), TYPE_NVME_BUS)
 
-static inline NvmeLBAF *nvme_ns_lbaf(NvmeNamespace *ns)
-{
-NvmeIdNs *id_ns = &ns->id_ns;
-return &id_ns->lbaf[NVME_ID_NS_FLBAS_INDEX(id_ns->flbas)];
-}
-
-static inline uint8_t nvme_ns_lbads(NvmeNamespace *ns)
-{
-return nvme_ns_lbaf(ns)->ds;
-}
-
-/* convert an LBA to the equivalent in bytes */
-static inline size_t nvme_l2b(NvmeNamespace *ns, uint64_t lba)
-{
-return lba << nvme_ns_lbads(ns);
-}
+typedef struct NvmeBus {
+BusState parent_bus;
+} NvmeBus;
 
 #define TYPE_NVME "nvme"
 #define NVME(obj) \
@@ -121,6 +110,7 @@ typedef struct NvmeFeatureVal {
 uint16_t temp_thresh_low;
 };
 uint32_tasync_config;
+uint32_tvwc;
 } NvmeFeatureVal;
 
 typedef struct NvmeCtrl {
@@ -128,8 +118,9 @@ typedef struct NvmeCtrl {
 MemoryRegion iomem;
 MemoryRegion ctrl_mem;
 NvmeBar  bar;
-BlockConfconf;
 NvmeParams   params;
+NvmeBus  bus;
+BlockConfconf;
 
 boolqs_created;
 uint32_tpage_size;
@@ -140,7 +131,6 @@ typedef struct NvmeCtrl {
 uint32_treg_size;
 uint32_tnum_namespaces;
 uint32_tmax_q_ents;
-uint64_tns_size;
 uint8_t outstanding_aers;
 uint8_t *cmbuf;
 uint32_tirq_status;
@@ -156,7 +146,8 @@ typedef struc

[PULL 14/30] hw/block/nvme: refactor identify active namespace id list

[Klaus Jensen]

From: Klaus Jensen 

Prepare to support inactive namespaces.

Signed-off-by: Klaus Jensen 
Reviewed-by: Maxim Levitsky 
Reviewed-by: Keith Busch 
---
 hw/block/nvme.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 4f08f55a76a0..924279d6027c 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -1473,7 +1473,7 @@ static uint16_t nvme_identify_nslist(NvmeCtrl *n, 
NvmeRequest *req)
 uint32_t min_nsid = le32_to_cpu(c->nsid);
 uint32_t *list;
 uint16_t ret;
-int i, j = 0;
+int j = 0;
 
 trace_pci_nvme_identify_nslist(min_nsid);
 
@@ -1488,11 +1488,11 @@ static uint16_t nvme_identify_nslist(NvmeCtrl *n, 
NvmeRequest *req)
 }
 
 list = g_malloc0(data_len);
-for (i = 0; i < n->num_namespaces; i++) {
-if (i < min_nsid) {
+for (int i = 1; i <= n->num_namespaces; i++) {
+if (i <= min_nsid) {
 continue;
 }
-list[j++] = cpu_to_le32(i + 1);
+list[j++] = cpu_to_le32(i);
 if (j == data_len / sizeof(uint32_t)) {
 break;
 }
-- 
2.29.1

[PULL 19/30] hw/block/nvme: remove pointless rw indirection

[Klaus Jensen]

From: Keith Busch 

The code switches on the opcode to invoke a function specific to that
opcode. There's no point in consolidating back to a common function that
just switches on that same opcode without any actual common code.
Restore the opcode specific behavior without going back through another
level of switches.

Signed-off-by: Keith Busch 
Signed-off-by: Klaus Jensen 
---
 hw/block/nvme.c   | 91 ++-
 hw/block/trace-events |  1 -
 2 files changed, 29 insertions(+), 63 deletions(-)

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 2225b944f935..a168f0bf4adb 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -927,68 +927,12 @@ static void nvme_rw_cb(void *opaque, int ret)
 nvme_enqueue_req_completion(nvme_cq(req), req);
 }
 
-static uint16_t nvme_do_aio(BlockBackend *blk, int64_t offset, size_t len,
-NvmeRequest *req)
-{
-BlockAcctCookie *acct = &req->acct;
-BlockAcctStats *stats = blk_get_stats(blk);
-
-bool is_write = false;
-
-trace_pci_nvme_do_aio(nvme_cid(req), req->cmd.opcode,
-  nvme_io_opc_str(req->cmd.opcode), blk_name(blk),
-  offset, len);
-
-switch (req->cmd.opcode) {
-case NVME_CMD_FLUSH:
-block_acct_start(stats, acct, 0, BLOCK_ACCT_FLUSH);
-req->aiocb = blk_aio_flush(blk, nvme_rw_cb, req);
-break;
-
-case NVME_CMD_WRITE_ZEROES:
-block_acct_start(stats, acct, len, BLOCK_ACCT_WRITE);
-req->aiocb = blk_aio_pwrite_zeroes(blk, offset, len,
-   BDRV_REQ_MAY_UNMAP, nvme_rw_cb,
-   req);
-break;
-
-case NVME_CMD_WRITE:
-is_write = true;
-
-/* fallthrough */
-
-case NVME_CMD_READ:
-block_acct_start(stats, acct, len,
- is_write ? BLOCK_ACCT_WRITE : BLOCK_ACCT_READ);
-
-if (req->qsg.sg) {
-if (is_write) {
-req->aiocb = dma_blk_write(blk, &req->qsg, offset,
-   BDRV_SECTOR_SIZE, nvme_rw_cb, req);
-} else {
-req->aiocb = dma_blk_read(blk, &req->qsg, offset,
-  BDRV_SECTOR_SIZE, nvme_rw_cb, req);
-}
-} else {
-if (is_write) {
-req->aiocb = blk_aio_pwritev(blk, offset, &req->iov, 0,
- nvme_rw_cb, req);
-} else {
-req->aiocb = blk_aio_preadv(blk, offset, &req->iov, 0,
-nvme_rw_cb, req);
-}
-}
-
-break;
-}
-
-return NVME_NO_COMPLETE;
-}
-
 static uint16_t nvme_flush(NvmeCtrl *n, NvmeRequest *req)
 {
-NvmeNamespace *ns = req->ns;
-return nvme_do_aio(ns->blkconf.blk, 0, 0, req);
+block_acct_start(blk_get_stats(req->ns->blkconf.blk), &req->acct, 0,
+ BLOCK_ACCT_FLUSH);
+req->aiocb = blk_aio_flush(req->ns->blkconf.blk, nvme_rw_cb, req);
+return NVME_NO_COMPLETE;
 }
 
 static uint16_t nvme_write_zeroes(NvmeCtrl *n, NvmeRequest *req)
@@ -1009,7 +953,11 @@ static uint16_t nvme_write_zeroes(NvmeCtrl *n, 
NvmeRequest *req)
 return status;
 }
 
-return nvme_do_aio(ns->blkconf.blk, offset, count, req);
+block_acct_start(blk_get_stats(req->ns->blkconf.blk), &req->acct, 0,
+ BLOCK_ACCT_WRITE);
+req->aiocb = blk_aio_pwrite_zeroes(req->ns->blkconf.blk, offset, count,
+   BDRV_REQ_MAY_UNMAP, nvme_rw_cb, req);
+return NVME_NO_COMPLETE;
 }
 
 static uint16_t nvme_rw(NvmeCtrl *n, NvmeRequest *req)
@@ -1023,6 +971,7 @@ static uint16_t nvme_rw(NvmeCtrl *n, NvmeRequest *req)
 uint64_t data_offset = nvme_l2b(ns, slba);
 enum BlockAcctType acct = req->cmd.opcode == NVME_CMD_WRITE ?
 BLOCK_ACCT_WRITE : BLOCK_ACCT_READ;
+BlockBackend *blk = ns->blkconf.blk;
 uint16_t status;
 
 trace_pci_nvme_rw(nvme_cid(req), nvme_io_opc_str(rw->opcode),
@@ -1045,7 +994,25 @@ static uint16_t nvme_rw(NvmeCtrl *n, NvmeRequest *req)
 goto invalid;
 }
 
-return nvme_do_aio(ns->blkconf.blk, data_offset, data_size, req);
+block_acct_start(blk_get_stats(blk), &req->acct, data_size, acct);
+if (req->qsg.sg) {
+if (acct == BLOCK_ACCT_WRITE) {
+req->aiocb = dma_blk_write(blk, &req->qsg, data_offset,
+   BDRV_SECTOR_SIZE, nvme_rw_cb, req);
+} else {
+req->aiocb = dma_blk_read(blk, &req->qsg, data_offset,
+  BDRV_SECTOR_SIZE, nvme_rw_cb, req);
+}
+} else {
+if (acct == BLOCK_ACCT_WRITE) {
+req->aiocb = blk_aio_pwritev(blk, data_offset, &req->iov, 0,
+ nvme_rw_cb, req);
+} else {
+req->aiocb = blk_aio_preadv(blk,

[PULL 25/30] hw/block/nvme: add nsid to get/setfeat trace events

[Klaus Jensen]

From: Klaus Jensen 

Include the namespace id in the pci_nvme_{get,set}feat trace events.

Signed-off-by: Klaus Jensen 
Signed-off-by: Keith Busch 
---
 hw/block/nvme.c   | 4 ++--
 hw/block/trace-events | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 32c35fe58768..5fd5a45a281a 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -1639,7 +1639,7 @@ static uint16_t nvme_get_feature(NvmeCtrl *n, NvmeRequest 
*req)
 [NVME_ARBITRATION] = NVME_ARB_AB_NOLIMIT,
 };
 
-trace_pci_nvme_getfeat(nvme_cid(req), fid, sel, dw11);
+trace_pci_nvme_getfeat(nvme_cid(req), nsid, fid, sel, dw11);
 
 if (!nvme_feature_support[fid]) {
 return NVME_INVALID_FIELD | NVME_DNR;
@@ -1777,7 +1777,7 @@ static uint16_t nvme_set_feature(NvmeCtrl *n, NvmeRequest 
*req)
 uint8_t fid = NVME_GETSETFEAT_FID(dw10);
 uint8_t save = NVME_SETFEAT_SAVE(dw10);
 
-trace_pci_nvme_setfeat(nvme_cid(req), fid, save, dw11);
+trace_pci_nvme_setfeat(nvme_cid(req), nsid, fid, save, dw11);
 
 if (save) {
 return NVME_FID_NOT_SAVEABLE | NVME_DNR;
diff --git a/hw/block/trace-events b/hw/block/trace-events
index 7b28091bd60a..2dc85281dc33 100644
--- a/hw/block/trace-events
+++ b/hw/block/trace-events
@@ -52,8 +52,8 @@ pci_nvme_identify_ns(uint32_t ns) "nsid %"PRIu32""
 pci_nvme_identify_nslist(uint32_t ns) "nsid %"PRIu32""
 pci_nvme_identify_ns_descr_list(uint32_t ns) "nsid %"PRIu32""
 pci_nvme_get_log(uint16_t cid, uint8_t lid, uint8_t lsp, uint8_t rae, uint32_t 
len, uint64_t off) "cid %"PRIu16" lid 0x%"PRIx8" lsp 0x%"PRIx8" rae 0x%"PRIx8" 
len %"PRIu32" off %"PRIu64""
-pci_nvme_getfeat(uint16_t cid, uint8_t fid, uint8_t sel, uint32_t cdw11) "cid 
%"PRIu16" fid 0x%"PRIx8" sel 0x%"PRIx8" cdw11 0x%"PRIx32""
-pci_nvme_setfeat(uint16_t cid, uint8_t fid, uint8_t save, uint32_t cdw11) "cid 
%"PRIu16" fid 0x%"PRIx8" save 0x%"PRIx8" cdw11 0x%"PRIx32""
+pci_nvme_getfeat(uint16_t cid, uint32_t nsid, uint8_t fid, uint8_t sel, 
uint32_t cdw11) "cid %"PRIu16" nsid 0x%"PRIx32" fid 0x%"PRIx8" sel 0x%"PRIx8" 
cdw11 0x%"PRIx32""
+pci_nvme_setfeat(uint16_t cid, uint32_t nsid, uint8_t fid, uint8_t save, 
uint32_t cdw11) "cid %"PRIu16" nsid 0x%"PRIx32" fid 0x%"PRIx8" save 0x%"PRIx8" 
cdw11 0x%"PRIx32""
 pci_nvme_getfeat_vwcache(const char* result) "get feature volatile write 
cache, result=%s"
 pci_nvme_getfeat_numq(int result) "get feature number of queues, result=%d"
 pci_nvme_setfeat_numq(int reqcq, int reqsq, int gotcq, int gotsq) "requested 
cq_count=%d sq_count=%d, responding with cq_count=%d sq_count=%d"
-- 
2.29.1

[PULL 11/30] hw/block/nvme: harden cmb access

[Klaus Jensen]

From: Klaus Jensen 

Since the controller has only supported PRPs so far it has not been
required to check the ending address (addr + len - 1) of the CMB access
for validity since it has been guaranteed to be in range of the CMB.

This changes when the controller adds support for SGLs (next patch), so
add that check.

Signed-off-by: Klaus Jensen 
Reviewed-by: Keith Busch 
---
 hw/block/nvme.c | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 0e916d48d763..c0f1f8ccd473 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -142,7 +142,12 @@ static inline void *nvme_addr_to_cmb(NvmeCtrl *n, hwaddr 
addr)
 
 static int nvme_addr_read(NvmeCtrl *n, hwaddr addr, void *buf, int size)
 {
-if (n->bar.cmbsz && nvme_addr_is_cmb(n, addr)) {
+hwaddr hi = addr + size - 1;
+if (hi < addr) {
+return 1;
+}
+
+if (n->bar.cmbsz && nvme_addr_is_cmb(n, addr) && nvme_addr_is_cmb(n, hi)) {
 memcpy(buf, nvme_addr_to_cmb(n, addr), size);
 return 0;
 }
-- 
2.29.1

[PULL 18/30] hw/block/nvme: update nsid when registered

[Klaus Jensen]

From: Klaus Jensen 

If the user does not specify an nsid parameter on the nvme-ns device,
nvme_register_namespace will find the first free namespace id and assign
that.

This fix makes sure the assigned id is saved.

Signed-off-by: Klaus Jensen 
Reviewed-by: Dmitry Fomichev 
---
 hw/block/nvme.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 5768a6804f41..2225b944f935 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -2578,7 +2578,7 @@ int nvme_register_namespace(NvmeCtrl *n, NvmeNamespace 
*ns, Error **errp)
 for (int i = 1; i <= n->num_namespaces; i++) {
 NvmeNamespace *ns = nvme_ns(n, i);
 if (!ns) {
-nsid = i;
+nsid = ns->params.nsid = i;
 break;
 }
 }
-- 
2.29.1

[PULL 23/30] hw/block/nvme: support for admin-only command set

[Klaus Jensen]

From: Keith Busch 

Signed-off-by: Keith Busch 
Signed-off-by: Klaus Jensen 
---
 include/block/nvme.h | 3 ++-
 hw/block/nvme.c  | 1 +
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/include/block/nvme.h b/include/block/nvme.h
index bc20a2ba5e87..521533fd2a10 100644
--- a/include/block/nvme.h
+++ b/include/block/nvme.h
@@ -83,7 +83,8 @@ enum NvmeCapMask {
 << CAP_PMR_SHIFT)
 
 enum NvmeCapCss {
-NVME_CAP_CSS_NVM = 1 << 0,
+NVME_CAP_CSS_NVM= 1 << 0,
+NVME_CAP_CSS_ADMIN_ONLY = 1 << 7,
 };
 
 enum NvmeCcShift {
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 94db06cf72be..c1323ca869f1 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -2751,6 +2751,7 @@ static void nvme_init_ctrl(NvmeCtrl *n, PCIDevice 
*pci_dev)
 NVME_CAP_SET_CQR(n->bar.cap, 1);
 NVME_CAP_SET_TO(n->bar.cap, 0xf);
 NVME_CAP_SET_CSS(n->bar.cap, NVME_CAP_CSS_NVM);
+NVME_CAP_SET_CSS(n->bar.cap, NVME_CAP_CSS_ADMIN_ONLY);
 NVME_CAP_SET_MPSMAX(n->bar.cap, 4);
 
 n->bar.vs = NVME_SPEC_VER;
-- 
2.29.1

[PULL 13/30] hw/block/nvme: add support for sgl bit bucket descriptor

[Klaus Jensen]

From: Gollu Appalanaidu 

This adds support for SGL descriptor type 0x1 (bit bucket descriptor).
See the NVM Express v1.3d specification, Section 4.4 ("Scatter Gather
List (SGL)").

Signed-off-by: Gollu Appalanaidu 
Signed-off-by: Klaus Jensen 
Reviewed-by: Keith Busch 
---
 hw/block/nvme.c | 33 +++--
 1 file changed, 27 insertions(+), 6 deletions(-)

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 63d0a17bc5f0..4f08f55a76a0 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -430,6 +430,10 @@ static uint16_t nvme_map_sgl_data(NvmeCtrl *n, QEMUSGList 
*qsg,
 uint8_t type = NVME_SGL_TYPE(segment[i].type);
 
 switch (type) {
+case NVME_SGL_DESCR_TYPE_BIT_BUCKET:
+if (req->cmd.opcode == NVME_CMD_WRITE) {
+continue;
+}
 case NVME_SGL_DESCR_TYPE_DATA_BLOCK:
 break;
 case NVME_SGL_DESCR_TYPE_SEGMENT:
@@ -440,6 +444,7 @@ static uint16_t nvme_map_sgl_data(NvmeCtrl *n, QEMUSGList 
*qsg,
 }
 
 dlen = le32_to_cpu(segment[i].len);
+
 if (!dlen) {
 continue;
 }
@@ -460,6 +465,11 @@ static uint16_t nvme_map_sgl_data(NvmeCtrl *n, QEMUSGList 
*qsg,
 }
 
 trans_len = MIN(*len, dlen);
+
+if (type == NVME_SGL_DESCR_TYPE_BIT_BUCKET) {
+goto next;
+}
+
 addr = le64_to_cpu(segment[i].addr);
 
 if (UINT64_MAX - addr < dlen) {
@@ -471,6 +481,7 @@ static uint16_t nvme_map_sgl_data(NvmeCtrl *n, QEMUSGList 
*qsg,
 return status;
 }
 
+next:
 *len -= trans_len;
 }
 
@@ -540,7 +551,8 @@ static uint16_t nvme_map_sgl(NvmeCtrl *n, QEMUSGList *qsg, 
QEMUIOVector *iov,
 seg_len = le32_to_cpu(sgld->len);
 
 /* check the length of the (Last) Segment descriptor */
-if (!seg_len || seg_len & 0xf) {
+if ((!seg_len || seg_len & 0xf) &&
+(NVME_SGL_TYPE(sgld->type) != NVME_SGL_DESCR_TYPE_BIT_BUCKET)) {
 return NVME_INVALID_SGL_SEG_DESCR | NVME_DNR;
 }
 
@@ -577,19 +589,27 @@ static uint16_t nvme_map_sgl(NvmeCtrl *n, QEMUSGList 
*qsg, QEMUIOVector *iov,
 
 last_sgld = &segment[nsgld - 1];
 
-/* if the segment ends with a Data Block, then we are done */
-if (NVME_SGL_TYPE(last_sgld->type) == NVME_SGL_DESCR_TYPE_DATA_BLOCK) {
+/*
+ * If the segment ends with a Data Block or Bit Bucket Descriptor Type,
+ * then we are done.
+ */
+switch (NVME_SGL_TYPE(last_sgld->type)) {
+case NVME_SGL_DESCR_TYPE_DATA_BLOCK:
+case NVME_SGL_DESCR_TYPE_BIT_BUCKET:
 status = nvme_map_sgl_data(n, qsg, iov, segment, nsgld, &len, req);
 if (status) {
 goto unmap;
 }
 
 goto out;
+
+default:
+break;
 }
 
 /*
- * If the last descriptor was not a Data Block, then the current
- * segment must not be a Last Segment.
+ * If the last descriptor was not a Data Block or Bit Bucket, then the
+ * current segment must not be a Last Segment.
  */
 if (NVME_SGL_TYPE(sgld->type) == NVME_SGL_DESCR_TYPE_LAST_SEGMENT) {
 status = NVME_INVALID_SGL_SEG_DESCR | NVME_DNR;
@@ -2651,7 +2671,8 @@ static void nvme_init_ctrl(NvmeCtrl *n, PCIDevice 
*pci_dev)
 id->nn = cpu_to_le32(n->num_namespaces);
 id->oncs = cpu_to_le16(NVME_ONCS_WRITE_ZEROES | NVME_ONCS_TIMESTAMP |
NVME_ONCS_FEATURES);
-id->sgls = cpu_to_le32(NVME_CTRL_SGLS_SUPPORT_NO_ALIGN);
+id->sgls = cpu_to_le32(NVME_CTRL_SGLS_SUPPORT_NO_ALIGN |
+   NVME_CTRL_SGLS_BITBUCKET);
 
 subnqn = g_strdup_printf("nqn.2019-08.org.qemu:%s", n->params.serial);
 strpadcpy((char *)id->subnqn, sizeof(id->subnqn), subnqn, '\0');
-- 
2.29.1

[PULL 24/30] hw/block/nvme: reject io commands if only admin command set selected

[Klaus Jensen]

From: Klaus Jensen 

If the host sets CC.CSS to 111b, all commands submitted to I/O queues
should be completed with status Invalid Command Opcode.

Note that this is technically a v1.4 feature, but it does not hurt to
implement before we finally bump the reported version implemented.

Reviewed-by: Dmitry Fomichev 
Signed-off-by: Klaus Jensen 
Signed-off-by: Keith Busch 
---
 include/block/nvme.h | 5 +
 hw/block/nvme.c  | 4 
 2 files changed, 9 insertions(+)

diff --git a/include/block/nvme.h b/include/block/nvme.h
index 521533fd2a10..6de2d5aa75a9 100644
--- a/include/block/nvme.h
+++ b/include/block/nvme.h
@@ -115,6 +115,11 @@ enum NvmeCcMask {
 #define NVME_CC_IOSQES(cc) ((cc >> CC_IOSQES_SHIFT) & CC_IOSQES_MASK)
 #define NVME_CC_IOCQES(cc) ((cc >> CC_IOCQES_SHIFT) & CC_IOCQES_MASK)
 
+enum NvmeCcCss {
+NVME_CC_CSS_NVM= 0x0,
+NVME_CC_CSS_ADMIN_ONLY = 0x7,
+};
+
 enum NvmeCstsShift {
 CSTS_RDY_SHIFT  = 0,
 CSTS_CFS_SHIFT  = 1,
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index c1323ca869f1..32c35fe58768 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -1026,6 +1026,10 @@ static uint16_t nvme_io_cmd(NvmeCtrl *n, NvmeRequest 
*req)
 trace_pci_nvme_io_cmd(nvme_cid(req), nsid, nvme_sqid(req),
   req->cmd.opcode, nvme_io_opc_str(req->cmd.opcode));
 
+if (NVME_CC_CSS(n->bar.cc) == NVME_CC_CSS_ADMIN_ONLY) {
+return NVME_INVALID_OPCODE | NVME_DNR;
+}
+
 if (!nvme_nsid_valid(n, nsid)) {
 return NVME_INVALID_NSID | NVME_DNR;
 }
-- 
2.29.1

[PULL 22/30] hw/block/nvme: validate command set selected

[Klaus Jensen]

From: Keith Busch 

Fail to start the controller if the user requests a command set that the
controller does not support.

Signed-off-by: Keith Busch 
Signed-off-by: Klaus Jensen 
---
 include/block/nvme.h  | 4 
 hw/block/nvme.c   | 6 +-
 hw/block/trace-events | 1 +
 3 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/include/block/nvme.h b/include/block/nvme.h
index 868cf53f0b25..bc20a2ba5e87 100644
--- a/include/block/nvme.h
+++ b/include/block/nvme.h
@@ -82,6 +82,10 @@ enum NvmeCapMask {
 #define NVME_CAP_SET_PMRS(cap, val) (cap |= (uint64_t)(val & CAP_PMR_MASK)\
 << CAP_PMR_SHIFT)
 
+enum NvmeCapCss {
+NVME_CAP_CSS_NVM = 1 << 0,
+};
+
 enum NvmeCcShift {
 CC_EN_SHIFT = 0,
 CC_CSS_SHIFT= 4,
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 5a9ae699afab..94db06cf72be 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -2045,6 +2045,10 @@ static int nvme_start_ctrl(NvmeCtrl *n)
 trace_pci_nvme_err_startfail_acq_misaligned(n->bar.acq);
 return -1;
 }
+if (unlikely(!(NVME_CAP_CSS(n->bar.cap) & (1 << NVME_CC_CSS(n->bar.cc) 
{
+trace_pci_nvme_err_startfail_css(NVME_CC_CSS(n->bar.cc));
+return -1;
+}
 if (unlikely(NVME_CC_MPS(n->bar.cc) <
  NVME_CAP_MPSMIN(n->bar.cap))) {
 trace_pci_nvme_err_startfail_page_too_small(
@@ -2746,7 +2750,7 @@ static void nvme_init_ctrl(NvmeCtrl *n, PCIDevice 
*pci_dev)
 NVME_CAP_SET_MQES(n->bar.cap, 0x7ff);
 NVME_CAP_SET_CQR(n->bar.cap, 1);
 NVME_CAP_SET_TO(n->bar.cap, 0xf);
-NVME_CAP_SET_CSS(n->bar.cap, 1);
+NVME_CAP_SET_CSS(n->bar.cap, NVME_CAP_CSS_NVM);
 NVME_CAP_SET_MPSMAX(n->bar.cap, 4);
 
 n->bar.vs = NVME_SPEC_VER;
diff --git a/hw/block/trace-events b/hw/block/trace-events
index e56d688b88d3..7b28091bd60a 100644
--- a/hw/block/trace-events
+++ b/hw/block/trace-events
@@ -132,6 +132,7 @@ pci_nvme_err_startfail_cqent_too_small(uint8_t log2ps, 
uint8_t maxlog2ps) "nvme_
 pci_nvme_err_startfail_cqent_too_large(uint8_t log2ps, uint8_t maxlog2ps) 
"nvme_start_ctrl failed because the completion queue entry size is too large: 
log2size=%u, max=%u"
 pci_nvme_err_startfail_sqent_too_small(uint8_t log2ps, uint8_t maxlog2ps) 
"nvme_start_ctrl failed because the submission queue entry size is too small: 
log2size=%u, min=%u"
 pci_nvme_err_startfail_sqent_too_large(uint8_t log2ps, uint8_t maxlog2ps) 
"nvme_start_ctrl failed because the submission queue entry size is too large: 
log2size=%u, max=%u"
+pci_nvme_err_startfail_css(uint8_t css) "nvme_start_ctrl failed because 
invalid command set selected:%u"
 pci_nvme_err_startfail_asqent_sz_zero(void) "nvme_start_ctrl failed because 
the admin submission queue size is zero"
 pci_nvme_err_startfail_acqent_sz_zero(void) "nvme_start_ctrl failed because 
the admin completion queue size is zero"
 pci_nvme_err_startfail(void) "setting controller enable bit failed"
-- 
2.29.1

[PULL 29/30] hw/block/nvme: fix create IO SQ/CQ status codes

[Klaus Jensen]

From: Gollu Appalanaidu 

Replace the Invalid Field in Command with the Invalid PRP Offset status
code in the nvme_create_{cq,sq} functions. Also, allow PRP1 to be
address 0x0.

Also replace the Completion Queue Invalid status code returned in
nvme_create_cq when the the queue identifier is invalid with the Invalid
Queue Identifier. The Completion Queue Invalid status code is
exclusively for indicating that the completion queue identifer given
when creating a submission queue is invalid.

See NVM Express v1.3d, Section 5.3 ("Create I/O Completion Queue
command") and 5.4("Create I/O Submission Queue command").

Signed-off-by: Gollu Appalanaidu 
Signed-off-by: Klaus Jensen 
Reviewed-by: Keith Busch 
---
 hw/block/nvme.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 2896bb49b9c0..5dfef0204c2c 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -1151,9 +1151,9 @@ static uint16_t nvme_create_sq(NvmeCtrl *n, NvmeRequest 
*req)
 trace_pci_nvme_err_invalid_create_sq_size(qsize);
 return NVME_MAX_QSIZE_EXCEEDED | NVME_DNR;
 }
-if (unlikely(!prp1 || prp1 & (n->page_size - 1))) {
+if (unlikely(prp1 & (n->page_size - 1))) {
 trace_pci_nvme_err_invalid_create_sq_addr(prp1);
-return NVME_INVALID_FIELD | NVME_DNR;
+return NVME_INVALID_PRP_OFFSET | NVME_DNR;
 }
 if (unlikely(!(NVME_SQ_FLAGS_PC(qflags {
 trace_pci_nvme_err_invalid_create_sq_qflags(NVME_SQ_FLAGS_PC(qflags));
@@ -1400,15 +1400,15 @@ static uint16_t nvme_create_cq(NvmeCtrl *n, NvmeRequest 
*req)
 
 if (unlikely(!cqid || !nvme_check_cqid(n, cqid))) {
 trace_pci_nvme_err_invalid_create_cq_cqid(cqid);
-return NVME_INVALID_CQID | NVME_DNR;
+return NVME_INVALID_QID | NVME_DNR;
 }
 if (unlikely(!qsize || qsize > NVME_CAP_MQES(n->bar.cap))) {
 trace_pci_nvme_err_invalid_create_cq_size(qsize);
 return NVME_MAX_QSIZE_EXCEEDED | NVME_DNR;
 }
-if (unlikely(!prp1)) {
+if (unlikely(prp1 & (n->page_size - 1))) {
 trace_pci_nvme_err_invalid_create_cq_addr(prp1);
-return NVME_INVALID_FIELD | NVME_DNR;
+return NVME_INVALID_PRP_OFFSET | NVME_DNR;
 }
 if (unlikely(!msix_enabled(&n->parent_obj) && vector)) {
 trace_pci_nvme_err_invalid_create_cq_vector(vector);
-- 
2.29.1

[PULL 28/30] hw/block/nvme: fix prp mapping status codes

[Klaus Jensen]

From: Gollu Appalanaidu 

Address 0 is not an invalid address. Remove those invalikd checks.

Unaligned PRP2 and PRP list entries should result in Invalid PRP Offset
status code and not Invalid Field. Fix that.

See NVMe Express v1.3d, Section 4.3 ("Physical Region Page Entry and
List").

Suggested-by: Keith Busch 
Signed-off-by: Gollu Appalanaidu 
Signed-off-by: Klaus Jensen 
Reviewed-by: Keith Busch 
---
 include/block/nvme.h  |  1 +
 hw/block/nvme.c   | 20 +---
 hw/block/trace-events |  4 +---
 3 files changed, 7 insertions(+), 18 deletions(-)

diff --git a/include/block/nvme.h b/include/block/nvme.h
index 6de2d5aa75a9..8a46d9cf015f 100644
--- a/include/block/nvme.h
+++ b/include/block/nvme.h
@@ -655,6 +655,7 @@ enum NvmeStatusCodes {
 NVME_MD_SGL_LEN_INVALID = 0x0010,
 NVME_SGL_DESCR_TYPE_INVALID = 0x0011,
 NVME_INVALID_USE_OF_CMB = 0x0012,
+NVME_INVALID_PRP_OFFSET = 0x0013,
 NVME_LBA_RANGE  = 0x0080,
 NVME_CAP_EXCEEDED   = 0x0081,
 NVME_NS_NOT_READY   = 0x0082,
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index b8c6be63186f..2896bb49b9c0 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -327,11 +327,6 @@ static uint16_t nvme_map_prp(NvmeCtrl *n, uint64_t prp1, 
uint64_t prp2,
 
 trace_pci_nvme_map_prp(trans_len, len, prp1, prp2, num_prps);
 
-if (unlikely(!prp1)) {
-trace_pci_nvme_err_invalid_prp();
-return NVME_INVALID_FIELD | NVME_DNR;
-}
-
 if (nvme_addr_is_cmb(n, prp1)) {
 qemu_iovec_init(iov, num_prps);
 } else {
@@ -345,11 +340,6 @@ static uint16_t nvme_map_prp(NvmeCtrl *n, uint64_t prp1, 
uint64_t prp2,
 
 len -= trans_len;
 if (len) {
-if (unlikely(!prp2)) {
-trace_pci_nvme_err_invalid_prp2_missing();
-return NVME_INVALID_FIELD | NVME_DNR;
-}
-
 if (len > n->page_size) {
 uint64_t prp_list[n->max_prp_ents];
 uint32_t nents, prp_trans;
@@ -370,9 +360,9 @@ static uint16_t nvme_map_prp(NvmeCtrl *n, uint64_t prp1, 
uint64_t prp2,
 uint64_t prp_ent = le64_to_cpu(prp_list[i]);
 
 if (i == n->max_prp_ents - 1 && len > n->page_size) {
-if (unlikely(!prp_ent || prp_ent & (n->page_size - 1))) {
+if (unlikely(prp_ent & (n->page_size - 1))) {
 trace_pci_nvme_err_invalid_prplist_ent(prp_ent);
-return NVME_INVALID_FIELD | NVME_DNR;
+return NVME_INVALID_PRP_OFFSET | NVME_DNR;
 }
 
 if (prp_list_in_cmb != nvme_addr_is_cmb(n, prp_ent)) {
@@ -391,9 +381,9 @@ static uint16_t nvme_map_prp(NvmeCtrl *n, uint64_t prp1, 
uint64_t prp2,
 prp_ent = le64_to_cpu(prp_list[i]);
 }
 
-if (unlikely(!prp_ent || prp_ent & (n->page_size - 1))) {
+if (unlikely(prp_ent & (n->page_size - 1))) {
 trace_pci_nvme_err_invalid_prplist_ent(prp_ent);
-return NVME_INVALID_FIELD | NVME_DNR;
+return NVME_INVALID_PRP_OFFSET | NVME_DNR;
 }
 
 trans_len = MIN(len, n->page_size);
@@ -408,7 +398,7 @@ static uint16_t nvme_map_prp(NvmeCtrl *n, uint64_t prp1, 
uint64_t prp2,
 } else {
 if (unlikely(prp2 & (n->page_size - 1))) {
 trace_pci_nvme_err_invalid_prp2_align(prp2);
-return NVME_INVALID_FIELD | NVME_DNR;
+return NVME_INVALID_PRP_OFFSET | NVME_DNR;
 }
 status = nvme_map_addr(n, qsg, iov, prp2, len);
 if (status) {
diff --git a/hw/block/trace-events b/hw/block/trace-events
index cab9913b1f2d..c1537e3ac0b0 100644
--- a/hw/block/trace-events
+++ b/hw/block/trace-events
@@ -97,10 +97,8 @@ pci_nvme_err_invalid_sgld(uint16_t cid, uint8_t typ) "cid 
%"PRIu16" type 0x%"PRI
 pci_nvme_err_invalid_num_sgld(uint16_t cid, uint8_t typ) "cid %"PRIu16" type 
0x%"PRIx8""
 pci_nvme_err_invalid_sgl_excess_length(uint16_t cid) "cid %"PRIu16""
 pci_nvme_err_invalid_dma(void) "PRP/SGL is too small for transfer size"
-pci_nvme_err_invalid_prplist_ent(uint64_t prplist) "PRP list entry is null or 
not page aligned: 0x%"PRIx64""
+pci_nvme_err_invalid_prplist_ent(uint64_t prplist) "PRP list entry is not page 
aligned: 0x%"PRIx64""
 pci_nvme_err_invalid_prp2_align(uint64_t prp2) "PRP2 is not page aligned: 
0x%"PRIx64""
-pci_nvme_err_invalid_prp2_missing(void) "PRP2 is null and more data to be 
transferred"
-pci_nvme_err_invalid_prp(void) "invalid PRP"
 pci_nvme_err_invalid_opc(uint8_t opc) "invalid opcode 0x%"PRIx8""
 pci_nvme_err_invalid_admin_opc(uint8_t opc) "invalid admin opcode 0x%"PRIx8""
 pci_nvme_err_invalid_lba_range(uint64_t start, uint64_t len, uint64_t limit) 
"Invalid LBA start=%"PRIu64" len=%"PRIu64" limit=%"PRIu64""
-- 
2.29.1

[PULL 16/30] pci: allocate pci id for nvme

[Klaus Jensen]

From: Klaus Jensen 

The emulated nvme device (hw/block/nvme.c) is currently using an
internal Intel device id.

Prepare to change that by allocating a device id under the 1b36 (Red
Hat, Inc.) vendor id.

Signed-off-by: Klaus Jensen 
Acked-by: Gerd Hoffmann 
Reviewed-by: Maxim Levitsky 
Reviewed-by: Keith Busch 
---
 docs/specs/nvme.txt| 23 +++
 docs/specs/pci-ids.txt |  1 +
 include/hw/pci/pci.h   |  1 +
 MAINTAINERS|  1 +
 4 files changed, 26 insertions(+)
 create mode 100644 docs/specs/nvme.txt

diff --git a/docs/specs/nvme.txt b/docs/specs/nvme.txt
new file mode 100644
index ..56d393884e7a
--- /dev/null
+++ b/docs/specs/nvme.txt
@@ -0,0 +1,23 @@
+NVM Express Controller
+==
+
+The nvme device (-device nvme) emulates an NVM Express Controller.
+
+
+Reference Specifications
+
+
+The device currently implements most mandatory features of NVMe v1.3d, see
+
+  https://nvmexpress.org/resources/specifications/
+
+for the specification.
+
+
+Known issues
+
+
+* The accounting numbers in the SMART/Health are reset across power cycles
+
+* Interrupt Coalescing is not supported and is disabled by default in volation
+  of the specification.
diff --git a/docs/specs/pci-ids.txt b/docs/specs/pci-ids.txt
index 4d53e5c7d9d5..abbdbca6be38 100644
--- a/docs/specs/pci-ids.txt
+++ b/docs/specs/pci-ids.txt
@@ -63,6 +63,7 @@ PCI devices (other than virtio):
 1b36:000b  PCIe Expander Bridge (-device pxb-pcie)
 1b36:000d  PCI xhci usb host adapter
 1b36:000f  mdpy (mdev sample device), linux/samples/vfio-mdev/mdpy.c
+1b36:0010  PCIe NVMe device (-device nvme)
 
 All these devices are documented in docs/specs.
 
diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index f19ffe6b4fe8..72ce649eee36 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -106,6 +106,7 @@ extern bool pci_available;
 #define PCI_DEVICE_ID_REDHAT_XHCI0x000d
 #define PCI_DEVICE_ID_REDHAT_PCIE_BRIDGE 0x000e
 #define PCI_DEVICE_ID_REDHAT_MDPY0x000f
+#define PCI_DEVICE_ID_REDHAT_NVME0x0010
 #define PCI_DEVICE_ID_REDHAT_QXL 0x0100
 
 #define FMT_PCIBUS  PRIx64
diff --git a/MAINTAINERS b/MAINTAINERS
index ef6f5c739984..9e215088ce8c 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1877,6 +1877,7 @@ L: qemu-bl...@nongnu.org
 S: Supported
 F: hw/block/nvme*
 F: tests/qtest/nvme-test.c
+F: docs/specs/nvme.txt
 T: git git://git.infradead.org/qemu-nvme.git nvme-next
 
 megasas
-- 
2.29.1

[PULL 20/30] hw/block/nvme: fix log page offset check

[Klaus Jensen]

From: Keith Busch 

Return error if the requested offset starts after the size of the log
being returned. Also, move the check for earlier in the function so
we're not doing unnecessary calculations.

Reviewed-by: Philippe Mathieu-Daudé 
Reviewed- by: Dmitry Fomichev 
Signed-off-by: Keith Busch 
Signed-off-by: Klaus Jensen 
---
 hw/block/nvme.c | 22 ++
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index a168f0bf4adb..aa725d1141b2 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -1179,6 +1179,10 @@ static uint16_t nvme_smart_info(NvmeCtrl *n, uint8_t 
rae, uint32_t buf_len,
 return NVME_INVALID_FIELD | NVME_DNR;
 }
 
+if (off >= sizeof(smart)) {
+return NVME_INVALID_FIELD | NVME_DNR;
+}
+
 for (int i = 1; i <= n->num_namespaces; i++) {
 NvmeNamespace *ns = nvme_ns(n, i);
 if (!ns) {
@@ -1193,10 +1197,6 @@ static uint16_t nvme_smart_info(NvmeCtrl *n, uint8_t 
rae, uint32_t buf_len,
 write_commands += s->nr_ops[BLOCK_ACCT_WRITE];
 }
 
-if (off > sizeof(smart)) {
-return NVME_INVALID_FIELD | NVME_DNR;
-}
-
 trans_len = MIN(sizeof(smart) - off, buf_len);
 
 memset(&smart, 0x0, sizeof(smart));
@@ -1234,12 +1234,11 @@ static uint16_t nvme_fw_log_info(NvmeCtrl *n, uint32_t 
buf_len, uint64_t off,
 .afi = 0x1,
 };
 
-strpadcpy((char *)&fw_log.frs1, sizeof(fw_log.frs1), "1.0", ' ');
-
-if (off > sizeof(fw_log)) {
+if (off >= sizeof(fw_log)) {
 return NVME_INVALID_FIELD | NVME_DNR;
 }
 
+strpadcpy((char *)&fw_log.frs1, sizeof(fw_log.frs1), "1.0", ' ');
 trans_len = MIN(sizeof(fw_log) - off, buf_len);
 
 return nvme_dma(n, (uint8_t *) &fw_log + off, trans_len,
@@ -1252,16 +1251,15 @@ static uint16_t nvme_error_info(NvmeCtrl *n, uint8_t 
rae, uint32_t buf_len,
 uint32_t trans_len;
 NvmeErrorLog errlog;
 
+if (off >= sizeof(errlog)) {
+return NVME_INVALID_FIELD | NVME_DNR;
+}
+
 if (!rae) {
 nvme_clear_events(n, NVME_AER_TYPE_ERROR);
 }
 
-if (off > sizeof(errlog)) {
-return NVME_INVALID_FIELD | NVME_DNR;
-}
-
 memset(&errlog, 0x0, sizeof(errlog));
-
 trans_len = MIN(sizeof(errlog) - off, buf_len);
 
 return nvme_dma(n, (uint8_t *)&errlog, trans_len,
-- 
2.29.1