Re: [PATCH-for-5.0 v2 04/11] hw/input/adb-kbd: Remove dead assignment

[David Gibson]

On Sat, Mar 21, 2020 at 03:41:03PM +0100, Philippe Mathieu-Daudé wrote:
> Since commit 5a1f49718 the 'olen' variable is not really
> used. Remove it to fix a warning reported by Clang static
> code analyzer:
> 
> CC  hw/input/adb-kbd.o
>   hw/input/adb-kbd.c:200:5: warning: Value stored to 'olen' is never read
>   olen = 0;
>   ^  ~
> 
> Fixes: 5a1f49718 (adb: add support for QKeyCode)
> Reported-by: Clang Static Analyzer
> Suggested-by: BALATON Zoltan 
> Signed-off-by: Philippe Mathieu-Daudé 

Acked-by: David Gibson 

> ---
> v2: Remove 'olen' (Zoltan)
> ---
>  hw/input/adb-kbd.c | 6 +-
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/hw/input/adb-kbd.c b/hw/input/adb-kbd.c
> index 0ba8207589..a6d5c9b7c9 100644
> --- a/hw/input/adb-kbd.c
> +++ b/hw/input/adb-kbd.c
> @@ -195,9 +195,7 @@ static int adb_kbd_poll(ADBDevice *d, uint8_t *obuf)
>  {
>  KBDState *s = ADB_KEYBOARD(d);
>  int keycode;
> -int olen;
>  
> -olen = 0;
>  if (s->count == 0) {
>  return 0;
>  }
> @@ -216,7 +214,6 @@ static int adb_kbd_poll(ADBDevice *d, uint8_t *obuf)
>  if (keycode == 0x7f) {
>  obuf[0] = 0x7f;
>  obuf[1] = 0x7f;
> -olen = 2;
>  } else {
>  obuf[0] = keycode;
>  /* NOTE: the power key key-up is the two byte sequence 0xff 0xff;
> @@ -224,10 +221,9 @@ static int adb_kbd_poll(ADBDevice *d, uint8_t *obuf)
>   * byte, but choose not to bother.
>   */
>  obuf[1] = 0xff;
> -olen = 2;
>  }
>  
> -return olen;
> +return 2;
>  }
>  
>  static int adb_kbd_request(ADBDevice *d, uint8_t *obuf,

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

Re: [PATCH] spapr: Fix memory leak in h_client_architecture_support()

[David Gibson]

On Sat, Mar 21, 2020 at 06:34:22PM +0100, Greg Kurz wrote:
> This is the only error path that needs to free the previously allocated
> ov1.
> 
> Reported-by: Coverity (CID 1421924)
> Fixes: cbd0d7f36322 "spapr: Fail CAS if option vector table cannot be parsed"
> Signed-off-by: Greg Kurz 

Applied to ppc-forr-5.0.

> ---
>  hw/ppc/spapr_hcall.c |1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index 40c86e91eb89..0d50fc911790 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -1726,6 +1726,7 @@ static target_ulong 
> h_client_architecture_support(PowerPCCPU *cpu,
>  }
>  ov5_guest = spapr_ovec_parse_vector(ov_table, 5);
>  if (!ov5_guest) {
> +spapr_ovec_cleanup(ov1_guest);
>  warn_report("guest didn't provide option vector 5");
>  return H_PARAMETER;
>  }
> 

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

Re: deprecation of in-tree builds

[BALATON Zoltan]

On Sat, 21 Mar 2020, Peter Maydell wrote:

AIUI from Paolo, the intention is to deprecate and eventually
stop supporting "in-tree" builds, so that the only option is
building in a separate build directory. I thought we should
probably mention that in the 5.0 changelog, so I wrote up some
text:

https://wiki.qemu.org/ChangeLog/5.0#Build_Information

Suggestions for changes/comments etc welcome.

(One thing we will need to fix before we can do separate build
tree is the Coverity Scan build process, which (a) does an
in-tree build (b) can't be easily switched to a builddir because
all the source paths get baked into the scan results and moving
to a builddir changes them all...)

We could also make configure actively warn if used in
the source tree.


This was discussed before. I think instead of annoying people with a 
warning, rather configure should be changed to create a build dir if run 
from source and have a Makefile in top dir that runs make -C builddir so 
people don't have to care about this or change their ways and can continue 
to run configure && make from source dir but you don't have to support 
in-tree build. Then you can deprecate in-tree builds but supporting only 
out-of-tree without this convenience would not just unnecessarily annoy 
those who prefer working in a single tree but people (and apparently some 
tools) expect sources to build with usual configure; make; make install so 
that should be the minimum to support.


Regards,
BALATON Zoltan

Re: [Qemu-devel] [PATCH PULL 0/4] RDMA queue

[Peter Maydell]

On Sat, 21 Mar 2020 at 19:13, Marcel Apfelbaum
 wrote:
>
> The following changes since commit 52a96afaa23d883d281bc7b95b9e69db7d6d3d3f:
>
>   Merge remote-tracking branch 
> 'remotes/vivier2/tags/linux-user-for-5.0-pull-request' into staging 
> (2020-03-20 16:00:21 +)
>
> are available in the Git repository at:
>
>   https://github.com/marcel-apf/qemu tags/rdma-pull-request
>
> for you to fetch changes up to f93cfdc583d4c26b2a878642adf574e11909863c:
>
>   hw/rdma: avoid suspicious strncpy() use (2020-03-21 19:21:20 +0200)
>
> 
> RDMA queue
>
> * hw/rdma: fix gcc 9.2 warnings
> * hw/rdma: eliminate data-path processing
> * hw/rdma: Replace strncpy with pstrcpy


Applied, thanks.

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

-- PMM

deprecation of in-tree builds

[Peter Maydell]

AIUI from Paolo, the intention is to deprecate and eventually
stop supporting "in-tree" builds, so that the only option is
building in a separate build directory. I thought we should
probably mention that in the 5.0 changelog, so I wrote up some
text:

https://wiki.qemu.org/ChangeLog/5.0#Build_Information

Suggestions for changes/comments etc welcome.

(One thing we will need to fix before we can do separate build
tree is the Coverity Scan build process, which (a) does an
in-tree build (b) can't be easily switched to a builddir because
all the source paths get baked into the scan results and moving
to a builddir changes them all...)

We could also make configure actively warn if used in
the source tree.

thanks
-- PMM

Re: [PULL 0/1] DTC queue for 5.0

[Peter Maydell]

On Fri, 20 Mar 2020 at 22:06, Alistair Francis  wrote:
>
> The following changes since commit 3d0ac346032a1fa9afafcaedc979a99f670e077e:
>
>   Merge remote-tracking branch 
> 'remotes/ehabkost/tags/python-next-pull-request' into staging (2020-03-20 
> 13:54:23 +)
>
> are available in the Git repository at:
>
>   g...@github.com:alistair23/qemu.git tags/pull-dtc-next-20200320-1
>
> for you to fetch changes up to 9f252c7c88eacbf21dadcfe117b0d08f2e88ceeb:
>
>   device_tree: Add info message when dumping dtb to file (2020-03-20 14:55:44 
> -0700)
>
> 
> DTC patches for 5.0
>
> 
> Leonardo Bras (1):
>   device_tree: Add info message when dumping dtb to file


Applied, thanks.

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

-- PMM

Re: In tree configure errors since 6116aea9

[Peter Maydell]

On Sat, 21 Mar 2020 at 18:32, BALATON Zoltan  wrote:
> On Sat, 21 Mar 2020, Laurent Vivier wrote:
> > I didn't see that because I always do an out-of-tree build.
>
> Isn't there a test for that or should there be one?

It's not in my set of CI tests (mostly because it's a bit
awkward to do). We're planning to drop support for
in-tree builds at some point: out-of-tree is just
generally better and trying to support both tends to
mean that one or the other gets accidentally broken
periodically.

thanks
-- PMM

Re: [PULL 21/36] hw/arm/allwinner-h3: add Boot ROM support

[Peter Maydell]

On Sat, 21 Mar 2020 at 17:17, Niek Linnenbank  wrote:
> By the way, I do not have the coverity tool unfortunately. So I can't really 
> check myself
> if any of the other Allwinner H3 files also have warnings that can be fixed..
> But if coverity finds more, just let me know, and I'll look into it.

Yeah, we only have Coverity as an online tool, so it
just runs once code gets into master, and then we
fix stuff after the fact. Either I or somebody else
will usually go through the reported new issues when
we get a new run and alert relevant people.

thanks
-- PMM

Re: [PULL v2 0/1] Slirp patches

[Peter Maydell]

On Fri, 20 Mar 2020 at 15:51, Marc-André Lureau
 wrote:
>
> The following changes since commit f57587c7d47b35b2d9b31def3a74d81bdb5475d7:
>
>   Merge remote-tracking branch 'remotes/armbru/tags/pull-qapi-2020-03-17' 
> into staging (2020-03-19 10:18:07 +)
>
> are available in the Git repository at:
>
>   https://github.com/elmarco/qemu.git tags/slirp-pull-request
>
> for you to fetch changes up to aa63573a84c92b14c23f557fcc93a12b1a93c187:
>
>   slirp: update submodule to v4.2.0+ (2020-03-20 16:50:12 +0100)
>
> 
>
> 
>
> Marc-André Lureau (1):
>   slirp: update submodule to v4.2.0+
>
>  slirp | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Hi; this fails to build on most platforms (though it works on
the BSDs, oddly):

/home/ubuntu/qemu/slirp/src/version.c: In function ‘slirp_version_string’:
/home/ubuntu/qemu/slirp/src/version.c:7:12: error:
‘SLIRP_VERSION_STRING’ undeclared (first use in this function); did
you mean ‘LIBSLIRP_VERSION_H_’?
 return SLIRP_VERSION_STRING;
^~~~
LIBSLIRP_VERSION_H_
/home/ubuntu/qemu/slirp/src/version.c:7:12: note: each undeclared
identifier is reported only once for each function it appears in
/home/ubuntu/qemu/slirp/src/version.c:8:1: error: control reaches end
of non-void function [-Werror=return-type]
 }
 ^
cc1: all warnings being treated as errors
Makefile:45: recipe for target
'/home/ubuntu/qemu/build/all/slirp/src/version.o' failed


thanks
-- PMM

[Qemu-devel] [PATCH PULL 4/4] hw/rdma: avoid suspicious strncpy() use

[Marcel Apfelbaum]

From: Stefan Hajnoczi 

gcc (GCC) 9.2.1 20190827 (Red Hat 9.2.1-1) with sanitizers enabled
reports the following error:

  CC  x86_64-softmmu/hw/rdma/vmw/pvrdma_dev_ring.o
In file included from /usr/include/string.h:495,
 from include/qemu/osdep.h:101,
 from hw/rdma/vmw/pvrdma_dev_ring.c:16:
In function ‘strncpy’,
inlined from ‘pvrdma_ring_init’ at hw/rdma/vmw/pvrdma_dev_ring.c:33:5:
/usr/include/bits/string_fortified.h:106:10: error: ‘__builtin_strncpy’ 
specified bound 32 equals destination size [-Werror=stringop-truncation]
  106 |   return __builtin___strncpy_chk (__dest, __src, __len, __bos (__dest));
  |  ^~

Use pstrcpy() instead of strncpy().  It is guaranteed to NUL-terminate
strings.

Signed-off-by: Stefan Hajnoczi 
Reviewed-by: Juan Quintela 
Reviewed-by: Yuval Shaia 
Message-Id: <20200316160702.478964-3-stefa...@redhat.com>
Signed-off-by: Marcel Apfelbaum 
---
 hw/rdma/vmw/pvrdma_dev_ring.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/rdma/vmw/pvrdma_dev_ring.c b/hw/rdma/vmw/pvrdma_dev_ring.c
index c2b3dd70a9..c122fe7035 100644
--- a/hw/rdma/vmw/pvrdma_dev_ring.c
+++ b/hw/rdma/vmw/pvrdma_dev_ring.c
@@ -14,6 +14,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/cutils.h"
 #include "hw/pci/pci.h"
 #include "cpu.h"
 #include "qemu/cutils.h"
-- 
2.17.2

[Qemu-devel] [PATCH PULL 2/4] hw/rdma: Cosmetic change - no need for two sge arrays

[Marcel Apfelbaum]

From: Yuval Shaia 

The function build_host_sge_array uses two sge arrays, one for input and
one for output.
Since the size of the two arrays is the same, the function can write
directly to the given source array (i.e. input/output argument).

Signed-off-by: Yuval Shaia 
Reviewed-by: Marcel Apfelbaum 
Message-Id: <20200320143429.9490-2-yuval.shaia...@gmail.com>
Signed-off-by: Marcel Apfelbaum 
---
 hw/rdma/rdma_backend.c | 40 
 1 file changed, 16 insertions(+), 24 deletions(-)

diff --git a/hw/rdma/rdma_backend.c b/hw/rdma/rdma_backend.c
index c346407cd3..b7ffbef9c0 100644
--- a/hw/rdma/rdma_backend.c
+++ b/hw/rdma/rdma_backend.c
@@ -378,30 +378,25 @@ static void ah_cache_init(void)
 }
 
 static int build_host_sge_array(RdmaDeviceResources *rdma_dev_res,
-struct ibv_sge *dsge, struct ibv_sge *ssge,
-uint8_t num_sge, uint64_t *total_length)
+struct ibv_sge *sge, uint8_t num_sge,
+uint64_t *total_length)
 {
 RdmaRmMR *mr;
-int ssge_idx;
+int idx;
 
-for (ssge_idx = 0; ssge_idx < num_sge; ssge_idx++) {
-mr = rdma_rm_get_mr(rdma_dev_res, ssge[ssge_idx].lkey);
+for (idx = 0; idx < num_sge; idx++) {
+mr = rdma_rm_get_mr(rdma_dev_res, sge[idx].lkey);
 if (unlikely(!mr)) {
-rdma_error_report("Invalid lkey 0x%x", ssge[ssge_idx].lkey);
-return VENDOR_ERR_INVLKEY | ssge[ssge_idx].lkey;
+rdma_error_report("Invalid lkey 0x%x", sge[idx].lkey);
+return VENDOR_ERR_INVLKEY | sge[idx].lkey;
 }
 
 #ifdef LEGACY_RDMA_REG_MR
-dsge->addr = (uintptr_t)mr->virt + ssge[ssge_idx].addr - mr->start;
-#else
-dsge->addr = ssge[ssge_idx].addr;
+sge[idx].addr = (uintptr_t)mr->virt + sge[idx].addr - mr->start;
 #endif
-dsge->length = ssge[ssge_idx].length;
-dsge->lkey = rdma_backend_mr_lkey(>backend_mr);
+sge[idx].lkey = rdma_backend_mr_lkey(>backend_mr);
 
-*total_length += dsge->length;
-
-dsge++;
+*total_length += sge[idx].length;
 }
 
 return 0;
@@ -484,7 +479,6 @@ void rdma_backend_post_send(RdmaBackendDev *backend_dev,
 void *ctx)
 {
 BackendCtx *bctx;
-struct ibv_sge new_sge[MAX_SGE];
 uint32_t bctx_id;
 int rc;
 struct ibv_send_wr wr = {}, *bad_wr;
@@ -518,7 +512,7 @@ void rdma_backend_post_send(RdmaBackendDev *backend_dev,
 
 rdma_protected_gslist_append_int32(>cqe_ctx_list, bctx_id);
 
-rc = build_host_sge_array(backend_dev->rdma_dev_res, new_sge, sge, num_sge,
+rc = build_host_sge_array(backend_dev->rdma_dev_res, sge, num_sge,
   _dev->rdma_dev_res->stats.tx_len);
 if (rc) {
 complete_work(IBV_WC_GENERAL_ERR, rc, ctx);
@@ -538,7 +532,7 @@ void rdma_backend_post_send(RdmaBackendDev *backend_dev,
 wr.num_sge = num_sge;
 wr.opcode = IBV_WR_SEND;
 wr.send_flags = IBV_SEND_SIGNALED;
-wr.sg_list = new_sge;
+wr.sg_list = sge;
 wr.wr_id = bctx_id;
 
 rc = ibv_post_send(qp->ibqp, , _wr);
@@ -601,7 +595,6 @@ void rdma_backend_post_recv(RdmaBackendDev *backend_dev,
 struct ibv_sge *sge, uint32_t num_sge, void *ctx)
 {
 BackendCtx *bctx;
-struct ibv_sge new_sge[MAX_SGE];
 uint32_t bctx_id;
 int rc;
 struct ibv_recv_wr wr = {}, *bad_wr;
@@ -635,7 +628,7 @@ void rdma_backend_post_recv(RdmaBackendDev *backend_dev,
 
 rdma_protected_gslist_append_int32(>cqe_ctx_list, bctx_id);
 
-rc = build_host_sge_array(backend_dev->rdma_dev_res, new_sge, sge, num_sge,
+rc = build_host_sge_array(backend_dev->rdma_dev_res, sge, num_sge,
   _dev->rdma_dev_res->stats.rx_bufs_len);
 if (rc) {
 complete_work(IBV_WC_GENERAL_ERR, rc, ctx);
@@ -643,7 +636,7 @@ void rdma_backend_post_recv(RdmaBackendDev *backend_dev,
 }
 
 wr.num_sge = num_sge;
-wr.sg_list = new_sge;
+wr.sg_list = sge;
 wr.wr_id = bctx_id;
 rc = ibv_post_recv(qp->ibqp, , _wr);
 if (rc) {
@@ -671,7 +664,6 @@ void rdma_backend_post_srq_recv(RdmaBackendDev *backend_dev,
 uint32_t num_sge, void *ctx)
 {
 BackendCtx *bctx;
-struct ibv_sge new_sge[MAX_SGE];
 uint32_t bctx_id;
 int rc;
 struct ibv_recv_wr wr = {}, *bad_wr;
@@ -688,7 +680,7 @@ void rdma_backend_post_srq_recv(RdmaBackendDev *backend_dev,
 
 rdma_protected_gslist_append_int32(>cqe_ctx_list, bctx_id);
 
-rc = build_host_sge_array(backend_dev->rdma_dev_res, new_sge, sge, num_sge,
+rc = build_host_sge_array(backend_dev->rdma_dev_res, sge, num_sge,
   _dev->rdma_dev_res->stats.rx_bufs_len);
 if (rc) {
 complete_work(IBV_WC_GENERAL_ERR, rc, ctx);
@@ -696,7 +688,7 @@ void rdma_backend_post_srq_recv(RdmaBackendDev *backend_dev,
 }
 
 wr.num_sge = 

[Qemu-devel] [PATCH PULL 1/4] hw/rdma/vmw/pvrdma_dev_ring: Replace strncpy with pstrcpy

[Marcel Apfelbaum]

From: Julia Suvorova 

ring->name is defined as 'char name[MAX_RING_NAME_SZ]'. Replace untruncated
strncpy with QEMU function.
This case prevented QEMU from compiling with --enable-sanitizers.

Signed-off-by: Julia Suvorova 
Message-Id: <20200318134849.237011-1-jus...@redhat.com>
Reviewed-by: Yuval Shaia 
Reviewed-by: Stefan Hajnoczi 
Signed-off-by: Marcel Apfelbaum 
---
 hw/rdma/vmw/pvrdma_dev_ring.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/rdma/vmw/pvrdma_dev_ring.c b/hw/rdma/vmw/pvrdma_dev_ring.c
index d7bc7f5ccc..c2b3dd70a9 100644
--- a/hw/rdma/vmw/pvrdma_dev_ring.c
+++ b/hw/rdma/vmw/pvrdma_dev_ring.c
@@ -16,6 +16,7 @@
 #include "qemu/osdep.h"
 #include "hw/pci/pci.h"
 #include "cpu.h"
+#include "qemu/cutils.h"
 
 #include "trace.h"
 
@@ -30,8 +31,7 @@ int pvrdma_ring_init(PvrdmaRing *ring, const char *name, 
PCIDevice *dev,
 int i;
 int rc = 0;
 
-strncpy(ring->name, name, MAX_RING_NAME_SZ);
-ring->name[MAX_RING_NAME_SZ - 1] = 0;
+pstrcpy(ring->name, MAX_RING_NAME_SZ, name);
 ring->dev = dev;
 ring->ring_state = ring_state;
 ring->max_elems = max_elems;
-- 
2.17.2

[Qemu-devel] [PATCH PULL 0/4] RDMA queue

[Marcel Apfelbaum]

The following changes since commit 52a96afaa23d883d281bc7b95b9e69db7d6d3d3f:

  Merge remote-tracking branch 
'remotes/vivier2/tags/linux-user-for-5.0-pull-request' into staging (2020-03-20 
16:00:21 +)

are available in the Git repository at:

  https://github.com/marcel-apf/qemu tags/rdma-pull-request

for you to fetch changes up to f93cfdc583d4c26b2a878642adf574e11909863c:

  hw/rdma: avoid suspicious strncpy() use (2020-03-21 19:21:20 +0200)


RDMA queue

* hw/rdma: fix gcc 9.2 warnings
* hw/rdma: eliminate data-path processing
* hw/rdma: Replace strncpy with pstrcpy


Julia Suvorova (1):
  hw/rdma/vmw/pvrdma_dev_ring: Replace strncpy with pstrcpy

Stefan Hajnoczi (1):
  hw/rdma: avoid suspicious strncpy() use

Yuval Shaia (2):
  hw/rdma: Cosmetic change - no need for two sge arrays
  hw/rdma: Skip data-path mr_id translation

 hw/rdma/rdma_backend.c| 61 +--
 hw/rdma/rdma_backend.h|  5 
 hw/rdma/rdma_rm.c | 13 +
 hw/rdma/vmw/pvrdma_dev_ring.c |  5 ++--
 4 files changed, 39 insertions(+), 45 deletions(-)

-- 
2.17.2

[Qemu-devel] [PATCH PULL 3/4] hw/rdma: Skip data-path mr_id translation

[Marcel Apfelbaum]

From: Yuval Shaia 

With the change made in commit 68b89aee71 ("Utilize ibv_reg_mr_iova for
memory registration") the MR emulation is no longer needed in order to
translate the guest addresses into host addresses.
With that, the next obvious step is to skip entirely the processing in
data-path.
To accomplish this, return the backend's lkey to driver so we will not
need to do the emulated mr_id to backend mr_id translation in data-path.

The function build_host_sge_array is still called in data-path but only
for backward computability with statistics collection.

While there, as a cosmetic change to make the code cleaner - make one
copy of the function rdma_backend_create_mr and leave the redundant
guest_start argument in the legacy code.

Signed-off-by: Yuval Shaia 
Reviewed-by: Marcel Apfelbaum 
Message-Id: <20200320143429.9490-3-yuval.shaia...@gmail.com>
Signed-off-by: Marcel Apfelbaum 
---
 hw/rdma/rdma_backend.c | 21 ++---
 hw/rdma/rdma_backend.h |  5 -
 hw/rdma/rdma_rm.c  | 13 ++---
 3 files changed, 20 insertions(+), 19 deletions(-)

diff --git a/hw/rdma/rdma_backend.c b/hw/rdma/rdma_backend.c
index b7ffbef9c0..3dd39fe1a7 100644
--- a/hw/rdma/rdma_backend.c
+++ b/hw/rdma/rdma_backend.c
@@ -377,6 +377,7 @@ static void ah_cache_init(void)
 destroy_ah_hash_key, destroy_ah_hast_data);
 }
 
+#ifdef LEGACY_RDMA_REG_MR
 static int build_host_sge_array(RdmaDeviceResources *rdma_dev_res,
 struct ibv_sge *sge, uint8_t num_sge,
 uint64_t *total_length)
@@ -391,9 +392,7 @@ static int build_host_sge_array(RdmaDeviceResources 
*rdma_dev_res,
 return VENDOR_ERR_INVLKEY | sge[idx].lkey;
 }
 
-#ifdef LEGACY_RDMA_REG_MR
 sge[idx].addr = (uintptr_t)mr->virt + sge[idx].addr - mr->start;
-#endif
 sge[idx].lkey = rdma_backend_mr_lkey(>backend_mr);
 
 *total_length += sge[idx].length;
@@ -401,6 +400,19 @@ static int build_host_sge_array(RdmaDeviceResources 
*rdma_dev_res,
 
 return 0;
 }
+#else
+static inline int build_host_sge_array(RdmaDeviceResources *rdma_dev_res,
+   struct ibv_sge *sge, uint8_t num_sge,
+   uint64_t *total_length)
+{
+int idx;
+
+for (idx = 0; idx < num_sge; idx++) {
+*total_length += sge[idx].length;
+}
+return 0;
+}
+#endif
 
 static void trace_mad_message(const char *title, char *buf, int len)
 {
@@ -731,13 +743,8 @@ void rdma_backend_destroy_pd(RdmaBackendPD *pd)
 }
 }
 
-#ifdef LEGACY_RDMA_REG_MR
-int rdma_backend_create_mr(RdmaBackendMR *mr, RdmaBackendPD *pd, void *addr,
-   size_t length, int access)
-#else
 int rdma_backend_create_mr(RdmaBackendMR *mr, RdmaBackendPD *pd, void *addr,
size_t length, uint64_t guest_start, int access)
-#endif
 {
 #ifdef LEGACY_RDMA_REG_MR
 mr->ibmr = ibv_reg_mr(pd->ibpd, addr, length, access);
diff --git a/hw/rdma/rdma_backend.h b/hw/rdma/rdma_backend.h
index 127f96e2d5..225af481e0 100644
--- a/hw/rdma/rdma_backend.h
+++ b/hw/rdma/rdma_backend.h
@@ -78,13 +78,8 @@ int rdma_backend_query_port(RdmaBackendDev *backend_dev,
 int rdma_backend_create_pd(RdmaBackendDev *backend_dev, RdmaBackendPD *pd);
 void rdma_backend_destroy_pd(RdmaBackendPD *pd);
 
-#ifdef LEGACY_RDMA_REG_MR
-int rdma_backend_create_mr(RdmaBackendMR *mr, RdmaBackendPD *pd, void *addr,
-   size_t length, int access);
-#else
 int rdma_backend_create_mr(RdmaBackendMR *mr, RdmaBackendPD *pd, void *addr,
size_t length, uint64_t guest_start, int access);
-#endif
 void rdma_backend_destroy_mr(RdmaBackendMR *mr);
 
 int rdma_backend_create_cq(RdmaBackendDev *backend_dev, RdmaBackendCQ *cq,
diff --git a/hw/rdma/rdma_rm.c b/hw/rdma/rdma_rm.c
index 1524dfaeaa..7e9ea283c9 100644
--- a/hw/rdma/rdma_rm.c
+++ b/hw/rdma/rdma_rm.c
@@ -227,21 +227,20 @@ int rdma_rm_alloc_mr(RdmaDeviceResources *dev_res, 
uint32_t pd_handle,
 mr->length = guest_length;
 mr->virt += (mr->start & (TARGET_PAGE_SIZE - 1));
 
-#ifdef LEGACY_RDMA_REG_MR
-ret = rdma_backend_create_mr(>backend_mr, >backend_pd, 
mr->virt,
- mr->length, access_flags);
-#else
 ret = rdma_backend_create_mr(>backend_mr, >backend_pd, 
mr->virt,
  mr->length, guest_start, access_flags);
-#endif
 if (ret) {
 ret = -EIO;
 goto out_dealloc_mr;
 }
+#ifdef LEGACY_RDMA_REG_MR
+/* We keep mr_handle in lkey so send and recv get get mr ptr */
+*lkey = *mr_handle;
+#else
+*lkey = rdma_backend_mr_lkey(>backend_mr);
+#endif
 }
 
-/* We keep mr_handle in lkey so send and recv get get mr ptr */
-*lkey = *mr_handle;
 *rkey = -1;
 
 mr->pd_handle = pd_handle;
-- 
2.17.2

Re: In tree configure errors since 6116aea9

[BALATON Zoltan]

On Sat, 21 Mar 2020, Laurent Vivier wrote:

Le 21/03/2020 à 18:29, BALATON Zoltan a écrit :

Hello,

Since 6116aea99, or actually 4d6a835d (linux-user: introduce parameters
to generate syscall_nr.h) but only next commit starts to enable it I get
these errors when running configure in source tree:

grep: ./.gitlab-ci.d: Is a directory
grep: ./scripts/qemu-guest-agent/fsfreeze-hook.d: Is a directory

for each entry in that loop over arches. Could this be silenced?


I didn't see that because I always do an out-of-tree build.


Isn't there a test for that or should there be one?


Could you try this?

--- a/configure
+++ b/configure
@@ -1911,6 +1911,7 @@ for arch in alpha hppa m68k xtensa sh4 microblaze
arm ppc s390x sparc sparc64 \
rm -f "${source_path}/linux-user/${arch}/syscall_nr.h"
# remove the dependency files
find . -name "*.d" \
+   -type f \
   -exec grep -q
"${source_path}/linux-user/${arch}/syscall_nr.h" {} \; \
   -exec rm {} \;
done


This gets rid of the errors but seems to be much slower:

with 4d6a835d running my usual configure script:

real0m5.968s
user0m4.642s
sys 0m1.402s

with HEAD and above patch:

real0m20.246s
user0m14.143s
sys 0m6.152s

Given that configure is rerun when some files change if there's a way to 
get at least the previous speed back might be better if possible.


Regards,
BALATON Zoltan

Re: [PATCH] spapr: Fix memory leak in h_client_architecture_support()

[Philippe Mathieu-Daudé]

On 3/21/20 6:34 PM, Greg Kurz wrote:

This is the only error path that needs to free the previously allocated
ov1.

Reported-by: Coverity (CID 1421924)
Fixes: cbd0d7f36322 "spapr: Fail CAS if option vector table cannot be parsed"
Signed-off-by: Greg Kurz 
---
  hw/ppc/spapr_hcall.c |1 +
  1 file changed, 1 insertion(+)

diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index 40c86e91eb89..0d50fc911790 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -1726,6 +1726,7 @@ static target_ulong 
h_client_architecture_support(PowerPCCPU *cpu,
  }
  ov5_guest = spapr_ovec_parse_vector(ov_table, 5);
  if (!ov5_guest) {
+spapr_ovec_cleanup(ov1_guest);


Quicker than using g_autoptr(), so for 5.0:

Reviewed-by: Philippe Mathieu-Daudé 


  warn_report("guest didn't provide option vector 5");
  return H_PARAMETER;
  }

Re: In tree configure errors since 6116aea9

[Laurent Vivier]

Le 21/03/2020 à 18:29, BALATON Zoltan a écrit :
> Hello,
> 
> Since 6116aea99, or actually 4d6a835d (linux-user: introduce parameters
> to generate syscall_nr.h) but only next commit starts to enable it I get
> these errors when running configure in source tree:
> 
> grep: ./.gitlab-ci.d: Is a directory
> grep: ./scripts/qemu-guest-agent/fsfreeze-hook.d: Is a directory
> 
> for each entry in that loop over arches. Could this be silenced?

I didn't see that because I always do an out-of-tree build.

Could you try this?

--- a/configure
+++ b/configure
@@ -1911,6 +1911,7 @@ for arch in alpha hppa m68k xtensa sh4 microblaze
arm ppc s390x sparc sparc64 \
 rm -f "${source_path}/linux-user/${arch}/syscall_nr.h"
 # remove the dependency files
 find . -name "*.d" \
+   -type f \
-exec grep -q
"${source_path}/linux-user/${arch}/syscall_nr.h" {} \; \
-exec rm {} \;
 done


Thanks,
Laurent

[PATCH] spapr: Fix memory leak in h_client_architecture_support()

[Greg Kurz]

This is the only error path that needs to free the previously allocated
ov1.

Reported-by: Coverity (CID 1421924)
Fixes: cbd0d7f36322 "spapr: Fail CAS if option vector table cannot be parsed"
Signed-off-by: Greg Kurz 
---
 hw/ppc/spapr_hcall.c |1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index 40c86e91eb89..0d50fc911790 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -1726,6 +1726,7 @@ static target_ulong 
h_client_architecture_support(PowerPCCPU *cpu,
 }
 ov5_guest = spapr_ovec_parse_vector(ov_table, 5);
 if (!ov5_guest) {
+spapr_ovec_cleanup(ov1_guest);
 warn_report("guest didn't provide option vector 5");
 return H_PARAMETER;
 }

In tree configure errors since 6116aea9

[BALATON Zoltan]

Hello,

Since 6116aea99, or actually 4d6a835d (linux-user: introduce parameters to 
generate syscall_nr.h) but only next commit starts to enable it I get 
these errors when running configure in source tree:


grep: ./.gitlab-ci.d: Is a directory
grep: ./scripts/qemu-guest-agent/fsfreeze-hook.d: Is a directory

for each entry in that loop over arches. Could this be silenced?

Regards,
BALATON Zoltan

Re: [PATCH 2/2] hw/rdma: avoid suspicious strncpy() use

[Marcel Apfelbaum]

Hi Stefan,

On 3/20/20 1:55 PM, Yuval Shaia wrote:



On Mon, 16 Mar 2020 at 18:07, Stefan Hajnoczi > wrote:


gcc (GCC) 9.2.1 20190827 (Red Hat 9.2.1-1) with sanitizers enabled
reports the following error:

  CC      x86_64-softmmu/hw/rdma/vmw/pvrdma_dev_ring.o
In file included from /usr/include/string.h:495,
                 from include/qemu/osdep.h:101,
                 from hw/rdma/vmw/pvrdma_dev_ring.c:16:
In function ‘strncpy’,
    inlined from ‘pvrdma_ring_init’ at
hw/rdma/vmw/pvrdma_dev_ring.c:33:5:
/usr/include/bits/string_fortified.h:106:10: error:
‘__builtin_strncpy’ specified bound 32 equals destination size
[-Werror=stringop-truncation]
  106 |   return __builtin___strncpy_chk (__dest, __src, __len,
__bos (__dest));
      | ^~

Use pstrcpy() instead of strncpy().  It is guaranteed to NUL-terminate
strings.

Signed-off-by: Stefan Hajnoczi mailto:stefa...@redhat.com>>
---
 hw/rdma/vmw/pvrdma_dev_ring.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/rdma/vmw/pvrdma_dev_ring.c
b/hw/rdma/vmw/pvrdma_dev_ring.c
index d7bc7f5ccc..74b8fa834c 100644
--- a/hw/rdma/vmw/pvrdma_dev_ring.c
+++ b/hw/rdma/vmw/pvrdma_dev_ring.c
@@ -14,6 +14,7 @@
  */

 #include "qemu/osdep.h"
+#include "qemu/cutils.h"
 #include "hw/pci/pci.h"
 #include "cpu.h"

@@ -30,8 +31,7 @@ int pvrdma_ring_init(PvrdmaRing *ring, const
char *name, PCIDevice *dev,
     int i;
     int rc = 0;

-    strncpy(ring->name, name, MAX_RING_NAME_SZ);
-    ring->name[MAX_RING_NAME_SZ - 1] = 0;
+    pstrcpy(ring->name, MAX_RING_NAME_SZ, name);
     ring->dev = dev;
     ring->ring_state = ring_state;
     ring->max_elems = max_elems;
-- 
2.24.1



Thanks,

Reviewed-by: Yuval Shaia >


I'll add this patch to the pvrdma queue.

Thanks,
Marcel

Re: [PULL 21/36] hw/arm/allwinner-h3: add Boot ROM support

[Niek Linnenbank]

Hi Peter,


On Fri, Mar 20, 2020 at 1:08 PM Peter Maydell 
wrote:

> On Thu, 12 Mar 2020 at 16:45, Peter Maydell 
> wrote:
> >
> > From: Niek Linnenbank 
> >
> > A real Allwinner H3 SoC contains a Boot ROM which is the
> > first code that runs right after the SoC is powered on.
> > The Boot ROM is responsible for loading user code (e.g. a bootloader)
> > from any of the supported external devices and writing the downloaded
> > code to internal SRAM. After loading the SoC begins executing the code
> > written to SRAM.
> >
> > This commits adds emulation of the Boot ROM firmware setup functionality
> > by loading user code from SD card in the A1 SRAM. While the A1 SRAM is
> > 64KiB, we limit the size to 32KiB because the real H3 Boot ROM also
> rejects
> > sizes larger than 32KiB. For reference, this behaviour is documented
> > by the Linux Sunxi project wiki at:
> >
> >   https://linux-sunxi.org/BROM#U-Boot_SPL_limitations
> >
> > Signed-off-by: Niek Linnenbank 
> > Reviewed-by: Alex Bennée 
> > Message-id: 20200311221854.30370-11-nieklinnenb...@gmail.com
> > Signed-off-by: Peter Maydell 
>
> Hi; Coverity (CID 1421882) points out a possible NULL
> pointer dereference in this change:
>
> > diff --git a/hw/arm/orangepi.c b/hw/arm/orangepi.c
> > index d65bbf8a2fe..b8ebcb08b76 100644
> > --- a/hw/arm/orangepi.c
> > +++ b/hw/arm/orangepi.c
> > @@ -97,6 +97,11 @@ static void orangepi_init(MachineState *machine)
> >  memory_region_add_subregion(get_system_memory(),
> h3->memmap[AW_H3_SDRAM],
> >  machine->ram);
> >
> > +/* Load target kernel or start using BootROM */
> > +if (!machine->kernel_filename && blk_is_available(blk)) {
> > +/* Use Boot ROM to copy data from SD card to SRAM */
> > +allwinner_h3_bootrom_setup(h3, blk);
> > +}
>
> blk_is_available() assumes its argument is non-NULL, but
> earlier in the function we set up blk with:
>blk = di ? blk_by_legacy_dinfo(di) : NULL;
>
> so it can be NULL here.
>
>
Right, indeed.


> Could you send a patch to fix this, please? Probably
>
just adding '&& blk' in the middle of the condition
> is sufficient.
>

Sure, I'll make a small patch for this, and also the other one you reported
for the SDRAM controller.

By the way, I do not have the coverity tool unfortunately. So I can't
really check myself
if any of the other Allwinner H3 files also have warnings that can be
fixed..
But if coverity finds more, just let me know, and I'll look into it.

Regards,
Niek



>
> thanks
> -- PMM
>


-- 
Niek Linnenbank

Re: [PATCH v1 2/2] hw/rdma: Skip data-path mr_id translation

[Marcel Apfelbaum]

On 3/20/20 4:34 PM, Yuval Shaia wrote:

With the change made in commit 68b89aee71 ("Utilize ibv_reg_mr_iova for
memory registration") the MR emulation is no longer needed in order to
translate the guest addresses into host addresses.
With that, the next obvious step is to skip entirely the processing in
data-path.
To accomplish this, return the backend's lkey to driver so we will not
need to do the emulated mr_id to backend mr_id translation in data-path.

The function build_host_sge_array is still called in data-path but only
for backward computability with statistics collection.

While there, as a cosmetic change to make the code cleaner - make one
copy of the function rdma_backend_create_mr and leave the redundant
guest_start argument in the legacy code.

Signed-off-by: Yuval Shaia 
---
  hw/rdma/rdma_backend.c | 21 ++---
  hw/rdma/rdma_backend.h |  5 -
  hw/rdma/rdma_rm.c  | 13 ++---
  3 files changed, 20 insertions(+), 19 deletions(-)

diff --git a/hw/rdma/rdma_backend.c b/hw/rdma/rdma_backend.c
index b7ffbef9c0..3dd39fe1a7 100644
--- a/hw/rdma/rdma_backend.c
+++ b/hw/rdma/rdma_backend.c
@@ -377,6 +377,7 @@ static void ah_cache_init(void)
  destroy_ah_hash_key, 
destroy_ah_hast_data);
  }
  
+#ifdef LEGACY_RDMA_REG_MR

  static int build_host_sge_array(RdmaDeviceResources *rdma_dev_res,
  struct ibv_sge *sge, uint8_t num_sge,
  uint64_t *total_length)
@@ -391,9 +392,7 @@ static int build_host_sge_array(RdmaDeviceResources 
*rdma_dev_res,
  return VENDOR_ERR_INVLKEY | sge[idx].lkey;
  }
  
-#ifdef LEGACY_RDMA_REG_MR

  sge[idx].addr = (uintptr_t)mr->virt + sge[idx].addr - mr->start;
-#endif
  sge[idx].lkey = rdma_backend_mr_lkey(>backend_mr);
  
  *total_length += sge[idx].length;

@@ -401,6 +400,19 @@ static int build_host_sge_array(RdmaDeviceResources 
*rdma_dev_res,
  
  return 0;

  }
+#else
+static inline int build_host_sge_array(RdmaDeviceResources *rdma_dev_res,
+   struct ibv_sge *sge, uint8_t num_sge,
+   uint64_t *total_length)
+{
+int idx;
+
+for (idx = 0; idx < num_sge; idx++) {
+*total_length += sge[idx].length;
+}
+return 0;
+}
+#endif
  
  static void trace_mad_message(const char *title, char *buf, int len)

  {
@@ -731,13 +743,8 @@ void rdma_backend_destroy_pd(RdmaBackendPD *pd)
  }
  }
  
-#ifdef LEGACY_RDMA_REG_MR

-int rdma_backend_create_mr(RdmaBackendMR *mr, RdmaBackendPD *pd, void *addr,
-   size_t length, int access)
-#else
  int rdma_backend_create_mr(RdmaBackendMR *mr, RdmaBackendPD *pd, void *addr,
 size_t length, uint64_t guest_start, int access)
-#endif
  {
  #ifdef LEGACY_RDMA_REG_MR
  mr->ibmr = ibv_reg_mr(pd->ibpd, addr, length, access);
diff --git a/hw/rdma/rdma_backend.h b/hw/rdma/rdma_backend.h
index 127f96e2d5..225af481e0 100644
--- a/hw/rdma/rdma_backend.h
+++ b/hw/rdma/rdma_backend.h
@@ -78,13 +78,8 @@ int rdma_backend_query_port(RdmaBackendDev *backend_dev,
  int rdma_backend_create_pd(RdmaBackendDev *backend_dev, RdmaBackendPD *pd);
  void rdma_backend_destroy_pd(RdmaBackendPD *pd);
  
-#ifdef LEGACY_RDMA_REG_MR

-int rdma_backend_create_mr(RdmaBackendMR *mr, RdmaBackendPD *pd, void *addr,
-   size_t length, int access);
-#else
  int rdma_backend_create_mr(RdmaBackendMR *mr, RdmaBackendPD *pd, void *addr,
 size_t length, uint64_t guest_start, int access);
-#endif
  void rdma_backend_destroy_mr(RdmaBackendMR *mr);
  
  int rdma_backend_create_cq(RdmaBackendDev *backend_dev, RdmaBackendCQ *cq,

diff --git a/hw/rdma/rdma_rm.c b/hw/rdma/rdma_rm.c
index 1524dfaeaa..7e9ea283c9 100644
--- a/hw/rdma/rdma_rm.c
+++ b/hw/rdma/rdma_rm.c
@@ -227,21 +227,20 @@ int rdma_rm_alloc_mr(RdmaDeviceResources *dev_res, 
uint32_t pd_handle,
  mr->length = guest_length;
  mr->virt += (mr->start & (TARGET_PAGE_SIZE - 1));
  
-#ifdef LEGACY_RDMA_REG_MR

-ret = rdma_backend_create_mr(>backend_mr, >backend_pd, 
mr->virt,
- mr->length, access_flags);
-#else
  ret = rdma_backend_create_mr(>backend_mr, >backend_pd, 
mr->virt,
   mr->length, guest_start, access_flags);
-#endif
  if (ret) {
  ret = -EIO;
  goto out_dealloc_mr;
  }
+#ifdef LEGACY_RDMA_REG_MR
+/* We keep mr_handle in lkey so send and recv get get mr ptr */
+*lkey = *mr_handle;
+#else
+*lkey = rdma_backend_mr_lkey(>backend_mr);
+#endif
  }
  
-/* We keep mr_handle in lkey so send and recv get get mr ptr */

-*lkey = *mr_handle;
  *rkey = -1;
  
  mr->pd_handle = pd_handle;



Reviewed-by: Marcel Apfelbaum 

Thanks,
Marcel

Re: [PATCH v1 1/2] hw/rdma: Cosmetic change - no need for two sge arrays

[Marcel Apfelbaum]

On 3/20/20 4:34 PM, Yuval Shaia wrote:

The function build_host_sge_array uses two sge arrays, one for input and
one for output.
Since the size of the two arrays is the same, the function can write
directly to the given source array (i.e. input/output argument).

Signed-off-by: Yuval Shaia 
---
  hw/rdma/rdma_backend.c | 40 
  1 file changed, 16 insertions(+), 24 deletions(-)

diff --git a/hw/rdma/rdma_backend.c b/hw/rdma/rdma_backend.c
index c346407cd3..b7ffbef9c0 100644
--- a/hw/rdma/rdma_backend.c
+++ b/hw/rdma/rdma_backend.c
@@ -378,30 +378,25 @@ static void ah_cache_init(void)
  }
  
  static int build_host_sge_array(RdmaDeviceResources *rdma_dev_res,

-struct ibv_sge *dsge, struct ibv_sge *ssge,
-uint8_t num_sge, uint64_t *total_length)
+struct ibv_sge *sge, uint8_t num_sge,
+uint64_t *total_length)
  {
  RdmaRmMR *mr;
-int ssge_idx;
+int idx;
  
-for (ssge_idx = 0; ssge_idx < num_sge; ssge_idx++) {

-mr = rdma_rm_get_mr(rdma_dev_res, ssge[ssge_idx].lkey);
+for (idx = 0; idx < num_sge; idx++) {
+mr = rdma_rm_get_mr(rdma_dev_res, sge[idx].lkey);
  if (unlikely(!mr)) {
-rdma_error_report("Invalid lkey 0x%x", ssge[ssge_idx].lkey);
-return VENDOR_ERR_INVLKEY | ssge[ssge_idx].lkey;
+rdma_error_report("Invalid lkey 0x%x", sge[idx].lkey);
+return VENDOR_ERR_INVLKEY | sge[idx].lkey;
  }
  
  #ifdef LEGACY_RDMA_REG_MR

-dsge->addr = (uintptr_t)mr->virt + ssge[ssge_idx].addr - mr->start;
-#else
-dsge->addr = ssge[ssge_idx].addr;
+sge[idx].addr = (uintptr_t)mr->virt + sge[idx].addr - mr->start;
  #endif
-dsge->length = ssge[ssge_idx].length;
-dsge->lkey = rdma_backend_mr_lkey(>backend_mr);
-
-*total_length += dsge->length;
+sge[idx].lkey = rdma_backend_mr_lkey(>backend_mr);
  
-dsge++;

+*total_length += sge[idx].length;
  }
  
  return 0;

@@ -484,7 +479,6 @@ void rdma_backend_post_send(RdmaBackendDev *backend_dev,
  void *ctx)
  {
  BackendCtx *bctx;
-struct ibv_sge new_sge[MAX_SGE];
  uint32_t bctx_id;
  int rc;
  struct ibv_send_wr wr = {}, *bad_wr;
@@ -518,7 +512,7 @@ void rdma_backend_post_send(RdmaBackendDev *backend_dev,
  
  rdma_protected_gslist_append_int32(>cqe_ctx_list, bctx_id);
  
-rc = build_host_sge_array(backend_dev->rdma_dev_res, new_sge, sge, num_sge,

+rc = build_host_sge_array(backend_dev->rdma_dev_res, sge, num_sge,
_dev->rdma_dev_res->stats.tx_len);
  if (rc) {
  complete_work(IBV_WC_GENERAL_ERR, rc, ctx);
@@ -538,7 +532,7 @@ void rdma_backend_post_send(RdmaBackendDev *backend_dev,
  wr.num_sge = num_sge;
  wr.opcode = IBV_WR_SEND;
  wr.send_flags = IBV_SEND_SIGNALED;
-wr.sg_list = new_sge;
+wr.sg_list = sge;
  wr.wr_id = bctx_id;
  
  rc = ibv_post_send(qp->ibqp, , _wr);

@@ -601,7 +595,6 @@ void rdma_backend_post_recv(RdmaBackendDev *backend_dev,
  struct ibv_sge *sge, uint32_t num_sge, void *ctx)
  {
  BackendCtx *bctx;
-struct ibv_sge new_sge[MAX_SGE];
  uint32_t bctx_id;
  int rc;
  struct ibv_recv_wr wr = {}, *bad_wr;
@@ -635,7 +628,7 @@ void rdma_backend_post_recv(RdmaBackendDev *backend_dev,
  
  rdma_protected_gslist_append_int32(>cqe_ctx_list, bctx_id);
  
-rc = build_host_sge_array(backend_dev->rdma_dev_res, new_sge, sge, num_sge,

+rc = build_host_sge_array(backend_dev->rdma_dev_res, sge, num_sge,
_dev->rdma_dev_res->stats.rx_bufs_len);
  if (rc) {
  complete_work(IBV_WC_GENERAL_ERR, rc, ctx);
@@ -643,7 +636,7 @@ void rdma_backend_post_recv(RdmaBackendDev *backend_dev,
  }
  
  wr.num_sge = num_sge;

-wr.sg_list = new_sge;
+wr.sg_list = sge;
  wr.wr_id = bctx_id;
  rc = ibv_post_recv(qp->ibqp, , _wr);
  if (rc) {
@@ -671,7 +664,6 @@ void rdma_backend_post_srq_recv(RdmaBackendDev *backend_dev,
  uint32_t num_sge, void *ctx)
  {
  BackendCtx *bctx;
-struct ibv_sge new_sge[MAX_SGE];
  uint32_t bctx_id;
  int rc;
  struct ibv_recv_wr wr = {}, *bad_wr;
@@ -688,7 +680,7 @@ void rdma_backend_post_srq_recv(RdmaBackendDev *backend_dev,
  
  rdma_protected_gslist_append_int32(>cqe_ctx_list, bctx_id);
  
-rc = build_host_sge_array(backend_dev->rdma_dev_res, new_sge, sge, num_sge,

+rc = build_host_sge_array(backend_dev->rdma_dev_res, sge, num_sge,
_dev->rdma_dev_res->stats.rx_bufs_len);
  if (rc) {
  complete_work(IBV_WC_GENERAL_ERR, rc, ctx);
@@ -696,7 +688,7 @@ void rdma_backend_post_srq_recv(RdmaBackendDev *backend_dev,
  }
  
  wr.num_sge = num_sge;

-

Re: [PULL v4 00/32] Linux user for 5.0 patches

[Peter Maydell]

On Fri, 20 Mar 2020 at 15:27, Laurent Vivier  wrote:
>
> The following changes since commit 373c7068dd610e97f0b551b5a6d0a27cd6da4506:
>
>   qemu.nsi: Install Sphinx documentation (2020-03-09 16:45:00 +)
>
> are available in the Git repository at:
>
>   git://github.com/vivier/qemu.git tags/linux-user-for-5.0-pull-request
>
> for you to fetch changes up to a64ddbb03acf1ee916c826ae89e0e1aa6500d5ae:
>
>   linux-user, openrisc: sync syscall numbers with kernel v5.5 (2020-03-20 
> 16:02:00 +0100)
>
> 
> update syscall numbers to linux 5.5 (with scripts)
> add clock_gettime64/clock_settime64
> add AT_EXECFN
>
> v4: restore syscall.tbl series but remove vsyscall series
> v3: remove syscall.tbl series
> v2: guard copy_to_user_timezone() with TARGET_NR_gettimeofday
> remove "Support futex_time64" patch
> guard sys_futex with TARGET_NR_exit
>
> 

Applied, thanks.

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

-- PMM

Re: [PATCH-for-5.0 01/11] block: Remove dead assignment

[BALATON Zoltan]

On Sat, 21 Mar 2020, Aleksandar Markovic wrote:

12:49 PM Sub, 21.03.2020. Philippe Mathieu-Daudé  је
написао/ла:


Fix warning reported by Clang static code analyzer:

  block.c:3167:5: warning: Value stored to 'ret' is never read
  ret = bdrv_fill_options(, filename, , _err);
  ^ ~

Reported-by: Clang Static Analyzer


Peter, and others,

Is this allowed use of "Reported-by:" mark?

I did not notice it being used this way before. And was under impression
that all similar tags/marks must be followed by a person, not a tool.


I think there were previous patches with Reported-by: Euler Robot which 
may be similar to this usage.


Regards,
BALATON Zoltan



Regards,
Aleksandar


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

diff --git a/block.c b/block.c
index a2542c977b..908c109a8c 100644
--- a/block.c
+++ b/block.c
@@ -3164,7 +3164,7 @@ static BlockDriverState *bdrv_open_inherit(const

char *filename,

 parent->open_flags, parent->options);
 }

-ret = bdrv_fill_options(, filename, , _err);
+bdrv_fill_options(, filename, , _err);
 if (local_err) {
 goto fail;
 }
--
2.21.1

[PATCH] monitor/hmp-cmds: remove redundant check for tls_authz in hmp_info_migrate_parameters

[Mao Zhongyi]

'params->has_tls_authz = true' has been hardcoded as true in
qmp_query_migrate_parameters, so remove the redundant check.

Signed-off-by: Mao Zhongyi 
---
 monitor/hmp-cmds.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index f8be6bbb16..d18826309b 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -461,7 +461,7 @@ void hmp_info_migrate_parameters(Monitor *mon, const QDict 
*qdict)
 params->max_postcopy_bandwidth);
 monitor_printf(mon, "%s: '%s'\n",
 MigrationParameter_str(MIGRATION_PARAMETER_TLS_AUTHZ),
-params->has_tls_authz ? params->tls_authz : "");
+params->tls_authz);
 }
 
 qapi_free_MigrationParameters(params);
-- 
2.17.1

Re: [PATCH-for-5.0 v2 00/11] misc: Trivial static code analyzer fixes

[Philippe Mathieu-Daudé]

On 3/21/20 3:40 PM, Philippe Mathieu-Daudé wrote:

Fix trivial warnings reported by the Clang static code analyzer.


I forgot to add the official Clang static code analyzer documentation is 
on https://clang-analyzer.llvm.org/ and on Fedora I simply used it as:


$ sudo dnf install clang-analyzer
$ ../configure
$ scan-build make



Since v1:
- Addressed Markus/Zoltan/Aleksandar review comments

Philippe Mathieu-Daudé (11):
   block: Avoid dead assignment
   blockdev: Remove dead assignment
   hw/i2c/pm_smbus: Remove dead assignment
   hw/input/adb-kbd: Remove dead assignment
   hw/ide/sii3112: Remove dead assignment
   hw/isa/i82378: Remove dead assignment
   hw/gpio/aspeed_gpio: Remove dead assignment
   hw/timer/exynos4210_mct: Remove dead assignments
   hw/timer/stm32f2xx_timer: Remove dead assignment
   hw/timer/pxa2xx_timer: Add assertion to silent static analyzer warning
   hw/scsi/esp-pci: Remove dead assignment

  block.c| 2 +-
  blockdev.c | 2 +-
  hw/gpio/aspeed_gpio.c  | 2 +-
  hw/i2c/pm_smbus.c  | 1 -
  hw/ide/sii3112.c   | 5 +++--
  hw/input/adb-kbd.c | 6 +-
  hw/isa/i82378.c| 8 
  hw/scsi/esp-pci.c  | 1 -
  hw/timer/exynos4210_mct.c  | 3 ---
  hw/timer/pxa2xx_timer.c| 1 +
  hw/timer/stm32f2xx_timer.c | 1 -
  11 files changed, 12 insertions(+), 20 deletions(-)

[PATCH-for-5.0 v2 09/11] hw/timer/stm32f2xx_timer: Remove dead assignment

[Philippe Mathieu-Daudé]

Fix warning reported by Clang static code analyzer:

CC  hw/timer/stm32f2xx_timer.o
  hw/timer/stm32f2xx_timer.c:225:9: warning: Value stored to 'value' is never 
read
  value = timer_val;
  ^   ~

Reported-by: Clang Static Analyzer
Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/timer/stm32f2xx_timer.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/hw/timer/stm32f2xx_timer.c b/hw/timer/stm32f2xx_timer.c
index 06ec8a02c2..ba8694dcd3 100644
--- a/hw/timer/stm32f2xx_timer.c
+++ b/hw/timer/stm32f2xx_timer.c
@@ -222,7 +222,6 @@ static void stm32f2xx_timer_write(void *opaque, hwaddr 
offset,
 case TIM_PSC:
 timer_val = stm32f2xx_ns_to_ticks(s, now) - s->tick_offset;
 s->tim_psc = value & 0x;
-value = timer_val;
 break;
 case TIM_CNT:
 timer_val = value;
-- 
2.21.1

Re: [PATCH] target/mips: Fix loongson multimedia condition instructions

[Huacai Chen]

On Sat, Mar 21, 2020 at 7:17 PM Jiaxun Yang  wrote:
>
>
>
> 于 2020年3月21日 GMT+08:00 下午6:57:54, Jiaxun Yang  写到:
> >
> >
> >于 2020年3月21日 GMT+08:00 下午6:39:21, "Philippe Mathieu-Daudé"
> > 写到:
> >>On 3/21/20 5:56 AM, Jiaxun Yang wrote:
> >>> Loongson multimedia condition instructions were previously
> >>implemented as
> >>> write 0 to rd due to lack of documentation. So I just confirmed with
> >>Loongson
> >>> about their encoding and implemented them correctly.
> >>
> >>Can you refer to the datasheet in the commit message, or have someone
> >>from Loongson Technology, Lemote Tech or with access to the specs ack
> >>your patch?
> >
> >I just confirmed with Loongson guys on IM.
> >
> >+ Huacai
>
> +Huacai's Gmail as his Lemote mail rejected my bounce.
>
> >
> >Hi Huacai,
> >
> >Could you please acknowledge this change,
> >Thanks.
> >
> >--
> >Jiaxun Yang
Acked-by: Huacai Chen 

> >
> >>
> >>>
> >>> Signed-off-by: Jiaxun Yang 
> >>> ---
> >>>   target/mips/translate.c | 40
> >>++--
> >>>   1 file changed, 34 insertions(+), 6 deletions(-)
> >>>
> >>> diff --git a/target/mips/translate.c b/target/mips/translate.c
> >>> index d745bd2803..43be8d27b5 100644
> >>> --- a/target/mips/translate.c
> >>> +++ b/target/mips/translate.c
> >>> @@ -5529,6 +5529,8 @@ static void
> >>gen_loongson_multimedia(DisasContext *ctx, int rd, int rs, int rt)
> >>>   {
> >>>   uint32_t opc, shift_max;
> >>>   TCGv_i64 t0, t1;
> >>> +TCGCond cond;
> >>> +TCGLabel *lab;
> >>>
> >>>   opc = MASK_LMI(ctx->opcode);
> >>>   switch (opc) {
> >>> @@ -5816,7 +5818,7 @@ static void
> >>gen_loongson_multimedia(DisasContext *ctx, int rd, int rs, int rt)
> >>>   case OPC_DADD_CP2:
> >>>   {
> >>>   TCGv_i64 t2 = tcg_temp_new_i64();
> >>> -TCGLabel *lab = gen_new_label();
> >>> +lab = gen_new_label();
> >>>
> >>>   tcg_gen_mov_i64(t2, t0);
> >>>   tcg_gen_add_i64(t0, t1, t2);
> >>> @@ -5837,7 +5839,7 @@ static void
> >>gen_loongson_multimedia(DisasContext *ctx, int rd, int rs, int rt)
> >>>   case OPC_DSUB_CP2:
> >>>   {
> >>>   TCGv_i64 t2 = tcg_temp_new_i64();
> >>> -TCGLabel *lab = gen_new_label();
> >>> +lab = gen_new_label();
> >>>
> >>>   tcg_gen_mov_i64(t2, t0);
> >>>   tcg_gen_sub_i64(t0, t1, t2);
> >>> @@ -5862,14 +5864,39 @@ static void
> >>gen_loongson_multimedia(DisasContext *ctx, int rd, int rs, int rt)
> >>>
> >>>   case OPC_SEQU_CP2:
> >>>   case OPC_SEQ_CP2:
> >>> +cond = TCG_COND_EQ;
> >>> +goto do_cc_cond;
> >>> +break;
> >>> +
> >>>   case OPC_SLTU_CP2:
> >>> +cond = TCG_COND_LTU;
> >>> +goto do_cc_cond;
> >>> +break;
> >>> +
> >>>   case OPC_SLT_CP2:
> >>> +cond = TCG_COND_LT;
> >>> +goto do_cc_cond;
> >>> +break;
> >>> +
> >>>   case OPC_SLEU_CP2:
> >>> +cond = TCG_COND_LEU;
> >>> +goto do_cc_cond;
> >>> +break;
> >>> +
> >>>   case OPC_SLE_CP2:
> >>> -/*
> >>> - * ??? Document is unclear: Set FCC[CC].  Does that mean
> >the
> >>> - * FD field is the CC field?
> >>> - */
> >>> +cond = TCG_COND_LE;
> >>> +do_cc_cond:
> >>> +{
> >>> +int cc = (ctx->opcode >> 8) & 0x7;
> >>> +lab = gen_new_label();
> >>> +tcg_gen_ori_i32(fpu_fcr31, fpu_fcr31, 1 <<
> >>get_fp_bit(cc));
> >>> +tcg_gen_brcond_i64(cond, t0, t1, lab);
> >>> +tcg_gen_xori_i32(fpu_fcr31, fpu_fcr31, 1 <<
> >>get_fp_bit(cc));
> >>> +gen_set_label(lab);
> >>> +}
> >>> +goto no_rd;
> >>> +break;
> >>> +
> >>>   default:
> >>>   MIPS_INVAL("loongson_cp2");
> >>>   generate_exception_end(ctx, EXCP_RI);
> >>> @@ -5878,6 +5905,7 @@ static void
> >>gen_loongson_multimedia(DisasContext *ctx, int rd, int rs, int rt)
> >>>
> >>>   gen_store_fpr64(ctx, t0, rd);
> >>>
> >>> +no_rd:
> >>>   tcg_temp_free_i64(t0);
> >>>   tcg_temp_free_i64(t1);
> >>>   }
> >>>
>
> --
> Jiaxun Yang

[PATCH-for-5.0 v2 08/11] hw/timer/exynos4210_mct: Remove dead assignments

[Philippe Mathieu-Daudé]

Fix warnings reported by Clang static code analyzer:

  hw/timer/exynos4210_mct.c:1370:9: warning: Value stored to 'index' is never 
read
index = GET_L_TIMER_CNT_REG_IDX(offset, lt_i);
^   ~
  hw/timer/exynos4210_mct.c:1399:9: warning: Value stored to 'index' is never 
read
index = GET_L_TIMER_CNT_REG_IDX(offset, lt_i);
^   ~
  hw/timer/exynos4210_mct.c:1441:9: warning: Value stored to 'index' is never 
read
index = GET_L_TIMER_CNT_REG_IDX(offset, lt_i);
^   ~

Reported-by: Clang Static Analyzer
Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/timer/exynos4210_mct.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/hw/timer/exynos4210_mct.c b/hw/timer/exynos4210_mct.c
index 944120aea5..c0a25e71ec 100644
--- a/hw/timer/exynos4210_mct.c
+++ b/hw/timer/exynos4210_mct.c
@@ -1367,7 +1367,6 @@ static void exynos4210_mct_write(void *opaque, hwaddr 
offset,
 
 case L0_TCNTB: case L1_TCNTB:
 lt_i = GET_L_TIMER_IDX(offset);
-index = GET_L_TIMER_CNT_REG_IDX(offset, lt_i);
 
 /*
  * TCNTB is updated to internal register only after CNT expired.
@@ -1396,7 +1395,6 @@ static void exynos4210_mct_write(void *opaque, hwaddr 
offset,
 
 case L0_ICNTB: case L1_ICNTB:
 lt_i = GET_L_TIMER_IDX(offset);
-index = GET_L_TIMER_CNT_REG_IDX(offset, lt_i);
 
 s->l_timer[lt_i].reg.wstat |= L_WSTAT_ICNTB_WRITE;
 s->l_timer[lt_i].reg.cnt[L_REG_CNT_ICNTB] = value &
@@ -1438,7 +1436,6 @@ static void exynos4210_mct_write(void *opaque, hwaddr 
offset,
 
 case L0_FRCNTB: case L1_FRCNTB:
 lt_i = GET_L_TIMER_IDX(offset);
-index = GET_L_TIMER_CNT_REG_IDX(offset, lt_i);
 
 DPRINTF("local timer[%d] FRCNTB write %llx\n", lt_i, value);
 
-- 
2.21.1

[PATCH-for-5.0 v2 10/11] hw/timer/pxa2xx_timer: Add assertion to silent static analyzer warning

[Philippe Mathieu-Daudé]

pxa2xx_timer_tick4() takes an opaque pointer, then calls
pxa2xx_timer_update4(), so the static analyzer can not
verify that the 'n < 8':

  425 static void pxa2xx_timer_tick4(void *opaque)
  426 {
  427 PXA2xxTimer4 *t = (PXA2xxTimer4 *) opaque;
  428 PXA2xxTimerInfo *i = (PXA2xxTimerInfo *) t->tm.info;
  429
  430 pxa2xx_timer_tick(>tm);
  433 if (t->control & (1 << 6))
  434 pxa2xx_timer_update4(i, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL), 
t->tm.num - 4);

  135 static void pxa2xx_timer_update4(void *opaque, uint64_t now_qemu, int n)
  136 {
  137 PXA2xxTimerInfo *s = (PXA2xxTimerInfo *) opaque;
  140 static const int counters[8] = { 0, 0, 0, 0, 4, 4, 6, 6 };
  142
  143 if (s->tm4[n].control & (1 << 7))
  144 counter = n;
  145 else
  146 counter = counters[n];

Add an assert() to give the static analyzer a hint, this fixes a
warning reported by Clang static code analyzer:

CC  hw/timer/pxa2xx_timer.o
  hw/timer/pxa2xx_timer.c:146:17: warning: Assigned value is garbage or 
undefined
  counter = counters[n];
  ^ ~~~

Reported-by: Clang Static Analyzer
Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/timer/pxa2xx_timer.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/timer/pxa2xx_timer.c b/hw/timer/pxa2xx_timer.c
index cd172cc1e9..944c165889 100644
--- a/hw/timer/pxa2xx_timer.c
+++ b/hw/timer/pxa2xx_timer.c
@@ -140,6 +140,7 @@ static void pxa2xx_timer_update4(void *opaque, uint64_t 
now_qemu, int n)
 static const int counters[8] = { 0, 0, 0, 0, 4, 4, 6, 6 };
 int counter;
 
+assert(n < ARRAY_SIZE(counters));
 if (s->tm4[n].control & (1 << 7))
 counter = n;
 else
-- 
2.21.1

[PATCH-for-5.0 v2 07/11] hw/gpio/aspeed_gpio: Remove dead assignment

[Philippe Mathieu-Daudé]

Fix warning reported by Clang static code analyzer:

  hw/gpio/aspeed_gpio.c:717:18: warning: Value stored to 'g_idx' during its 
initialization is never read
  int set_idx, g_idx = *group_idx;
   ^   ~~

Reported-by: Clang Static Analyzer
Signed-off-by: Philippe Mathieu-Daudé 
---
v2: Do not declare g_idx in for() (Zoltan)
---
 hw/gpio/aspeed_gpio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/gpio/aspeed_gpio.c b/hw/gpio/aspeed_gpio.c
index 41e11ea9b0..bd19db31f4 100644
--- a/hw/gpio/aspeed_gpio.c
+++ b/hw/gpio/aspeed_gpio.c
@@ -714,7 +714,7 @@ static void aspeed_gpio_write(void *opaque, hwaddr offset, 
uint64_t data,
 static int get_set_idx(AspeedGPIOState *s, const char *group, int *group_idx)
 {
 AspeedGPIOClass *agc = ASPEED_GPIO_GET_CLASS(s);
-int set_idx, g_idx = *group_idx;
+int set_idx, g_idx;
 
 for (set_idx = 0; set_idx < agc->nr_gpio_sets; set_idx++) {
 const GPIOSetProperties *set_props = >props[set_idx];
-- 
2.21.1

[PATCH-for-5.0 v2 04/11] hw/input/adb-kbd: Remove dead assignment

[Philippe Mathieu-Daudé]

Since commit 5a1f49718 the 'olen' variable is not really
used. Remove it to fix a warning reported by Clang static
code analyzer:

CC  hw/input/adb-kbd.o
  hw/input/adb-kbd.c:200:5: warning: Value stored to 'olen' is never read
  olen = 0;
  ^  ~

Fixes: 5a1f49718 (adb: add support for QKeyCode)
Reported-by: Clang Static Analyzer
Suggested-by: BALATON Zoltan 
Signed-off-by: Philippe Mathieu-Daudé 
---
v2: Remove 'olen' (Zoltan)
---
 hw/input/adb-kbd.c | 6 +-
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/hw/input/adb-kbd.c b/hw/input/adb-kbd.c
index 0ba8207589..a6d5c9b7c9 100644
--- a/hw/input/adb-kbd.c
+++ b/hw/input/adb-kbd.c
@@ -195,9 +195,7 @@ static int adb_kbd_poll(ADBDevice *d, uint8_t *obuf)
 {
 KBDState *s = ADB_KEYBOARD(d);
 int keycode;
-int olen;
 
-olen = 0;
 if (s->count == 0) {
 return 0;
 }
@@ -216,7 +214,6 @@ static int adb_kbd_poll(ADBDevice *d, uint8_t *obuf)
 if (keycode == 0x7f) {
 obuf[0] = 0x7f;
 obuf[1] = 0x7f;
-olen = 2;
 } else {
 obuf[0] = keycode;
 /* NOTE: the power key key-up is the two byte sequence 0xff 0xff;
@@ -224,10 +221,9 @@ static int adb_kbd_poll(ADBDevice *d, uint8_t *obuf)
  * byte, but choose not to bother.
  */
 obuf[1] = 0xff;
-olen = 2;
 }
 
-return olen;
+return 2;
 }
 
 static int adb_kbd_request(ADBDevice *d, uint8_t *obuf,
-- 
2.21.1

[PATCH-for-5.0 v2 11/11] hw/scsi/esp-pci: Remove dead assignment

[Philippe Mathieu-Daudé]

Fix warning reported by Clang static code analyzer:

CC  hw/scsi/esp-pci.o
  hw/scsi/esp-pci.c:198:9: warning: Value stored to 'size' is never read
  size = 4;
  ^  ~

Reported-by: Clang Static Analyzer
Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/scsi/esp-pci.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/hw/scsi/esp-pci.c b/hw/scsi/esp-pci.c
index d5a1f9e017..2e6cc07d4e 100644
--- a/hw/scsi/esp-pci.c
+++ b/hw/scsi/esp-pci.c
@@ -195,7 +195,6 @@ static void esp_pci_io_write(void *opaque, hwaddr addr,
 val <<= shift;
 val |= current & ~(mask << shift);
 addr &= ~3;
-size = 4;
 }
 
 if (addr < 0x40) {
-- 
2.21.1

[PATCH-for-5.0 v2 06/11] hw/isa/i82378: Remove dead assignment

[Philippe Mathieu-Daudé]

Rename the unique variable assigned as 'pit' which better
represents what it holds, to fix a warning reported by the
Clang static code analyzer:

CC  hw/isa/i82378.o
  hw/isa/i82378.c:108:5: warning: Value stored to 'isa' is never read
  isa = isa_create_simple(isabus, "i82374");
  ^ ~~~

Reported-by: Clang Static Analyzer
Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/isa/i82378.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/isa/i82378.c b/hw/isa/i82378.c
index dcb6b479ea..d9e6c7fa00 100644
--- a/hw/isa/i82378.c
+++ b/hw/isa/i82378.c
@@ -67,7 +67,7 @@ static void i82378_realize(PCIDevice *pci, Error **errp)
 I82378State *s = I82378(dev);
 uint8_t *pci_conf;
 ISABus *isabus;
-ISADevice *isa;
+ISADevice *pit;
 
 pci_conf = pci->config;
 pci_set_word(pci_conf + PCI_COMMAND,
@@ -99,13 +99,13 @@ static void i82378_realize(PCIDevice *pci, Error **errp)
 isa_bus_irqs(isabus, s->i8259);
 
 /* 1 82C54 (pit) */
-isa = i8254_pit_init(isabus, 0x40, 0, NULL);
+pit = i8254_pit_init(isabus, 0x40, 0, NULL);
 
 /* speaker */
-pcspk_init(isabus, isa);
+pcspk_init(isabus, pit);
 
 /* 2 82C37 (dma) */
-isa = isa_create_simple(isabus, "i82374");
+isa_create_simple(isabus, "i82374");
 }
 
 static void i82378_init(Object *obj)
-- 
2.21.1

[PATCH-for-5.0 v2 03/11] hw/i2c/pm_smbus: Remove dead assignment

[Philippe Mathieu-Daudé]

Fix warning reported by Clang static code analyzer:

CC  hw/i2c/pm_smbus.o
  hw/i2c/pm_smbus.c:187:17: warning: Value stored to 'ret' is never read
  ret = 0;
  ^ ~

Reported-by: Clang Static Analyzer
Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/i2c/pm_smbus.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/hw/i2c/pm_smbus.c b/hw/i2c/pm_smbus.c
index 36994ff585..4728540c37 100644
--- a/hw/i2c/pm_smbus.c
+++ b/hw/i2c/pm_smbus.c
@@ -184,7 +184,6 @@ static void smb_transaction(PMSMBus *s)
 s->smb_stat |= STS_HOST_BUSY | STS_BYTE_DONE;
 s->smb_data[0] = s->smb_blkdata;
 s->smb_index = 0;
-ret = 0;
 }
 goto out;
 }
-- 
2.21.1

[PATCH-for-5.0 v2 05/11] hw/ide/sii3112: Remove dead assignment

[Philippe Mathieu-Daudé]

Fix warning reported by Clang static code analyzer:

CC  hw/ide/sii3112.o
  hw/ide/sii3112.c:204:9: warning: Value stored to 'val' is never read
  val = 0;
  ^ ~

Fixes: a9dd6604
Reported-by: Clang Static Analyzer
Reviewed-by: BALATON Zoltan 
Signed-off-by: Philippe Mathieu-Daudé 
---
v2: Fix the correct function (Aleksandar review)
---
 hw/ide/sii3112.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/ide/sii3112.c b/hw/ide/sii3112.c
index 06605d7af2..b2ff6dd6d9 100644
--- a/hw/ide/sii3112.c
+++ b/hw/ide/sii3112.c
@@ -42,7 +42,7 @@ static uint64_t sii3112_reg_read(void *opaque, hwaddr addr,
 unsigned int size)
 {
 SiI3112PCIState *d = opaque;
-uint64_t val = 0;
+uint64_t val;
 
 switch (addr) {
 case 0x00:
@@ -126,6 +126,7 @@ static uint64_t sii3112_reg_read(void *opaque, hwaddr addr,
 break;
 default:
 val = 0;
+break;
 }
 trace_sii3112_read(size, addr, val);
 return val;
@@ -201,7 +202,7 @@ static void sii3112_reg_write(void *opaque, hwaddr addr,
 d->regs[1].sien = (val >> 16) & 0x3eed;
 break;
 default:
-val = 0;
+break;
 }
 }
 
-- 
2.21.1

[PATCH-for-5.0 v2 02/11] blockdev: Remove dead assignment

[Philippe Mathieu-Daudé]

Fix warning reported by Clang static code analyzer:

CC  blockdev.o
  blockdev.c:2744:5: warning: Value stored to 'ret' is never read
  ret = blk_truncate(blk, size, false, PREALLOC_MODE_OFF, errp);
  ^ ~~~

Reported-by: Clang Static Analyzer
Signed-off-by: Philippe Mathieu-Daudé 
---
 blockdev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/blockdev.c b/blockdev.c
index fa8630cb41..6effd5afaa 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2741,7 +2741,7 @@ void qmp_block_resize(bool has_device, const char *device,
 }
 
 bdrv_drained_begin(bs);
-ret = blk_truncate(blk, size, false, PREALLOC_MODE_OFF, errp);
+blk_truncate(blk, size, false, PREALLOC_MODE_OFF, errp);
 bdrv_drained_end(bs);
 
 out:
-- 
2.21.1

[PATCH-for-5.0 v2 01/11] block: Avoid dead assignment

[Philippe Mathieu-Daudé]

Fix warning reported by Clang static code analyzer:

  block.c:3167:5: warning: Value stored to 'ret' is never read
  ret = bdrv_fill_options(, filename, , _err);
  ^ ~

Fixes: 462f5bcf6
Reported-by: Clang Static Analyzer
Suggested-by: Markus Armbruster 
Signed-off-by: Philippe Mathieu-Daudé 
---
v2: Keep 'ret' assigned and check it (Markus)
---
 block.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block.c b/block.c
index a2542c977b..a6f069d8bd 100644
--- a/block.c
+++ b/block.c
@@ -3165,7 +3165,7 @@ static BlockDriverState *bdrv_open_inherit(const char 
*filename,
 }
 
 ret = bdrv_fill_options(, filename, , _err);
-if (local_err) {
+if (ret < 0) {
 goto fail;
 }
 
-- 
2.21.1

[PATCH-for-5.0 v2 00/11] misc: Trivial static code analyzer fixes

[Philippe Mathieu-Daudé]

Fix trivial warnings reported by the Clang static code analyzer.

Since v1:
- Addressed Markus/Zoltan/Aleksandar review comments

Philippe Mathieu-Daudé (11):
  block: Avoid dead assignment
  blockdev: Remove dead assignment
  hw/i2c/pm_smbus: Remove dead assignment
  hw/input/adb-kbd: Remove dead assignment
  hw/ide/sii3112: Remove dead assignment
  hw/isa/i82378: Remove dead assignment
  hw/gpio/aspeed_gpio: Remove dead assignment
  hw/timer/exynos4210_mct: Remove dead assignments
  hw/timer/stm32f2xx_timer: Remove dead assignment
  hw/timer/pxa2xx_timer: Add assertion to silent static analyzer warning
  hw/scsi/esp-pci: Remove dead assignment

 block.c| 2 +-
 blockdev.c | 2 +-
 hw/gpio/aspeed_gpio.c  | 2 +-
 hw/i2c/pm_smbus.c  | 1 -
 hw/ide/sii3112.c   | 5 +++--
 hw/input/adb-kbd.c | 6 +-
 hw/isa/i82378.c| 8 
 hw/scsi/esp-pci.c  | 1 -
 hw/timer/exynos4210_mct.c  | 3 ---
 hw/timer/pxa2xx_timer.c| 1 +
 hw/timer/stm32f2xx_timer.c | 1 -
 11 files changed, 12 insertions(+), 20 deletions(-)

-- 
2.21.1

POWERPC_EXCP_PROGRAM POWERPC_EXCP_INVAL in PowerPC simulation.

[Yonggang Luo]

I am facing a exception when running the following code:
```
00e86000 :
excConnectCode():
  e86000: 3c 60 00 e1 lis r3,225
  e86004: 60 63 c3 80 ori r3,r3,50048
  e86008: 7c 68 03 a6 mtlrr3
```
How to implement an instruction in ppc translate?

Raise exception at 00e86000 => 0060 (21)
-- 
 此致
礼
罗勇刚
Yours
sincerely,
Yonggang Luo

[PATCH v8 1/4] qcow2: introduce compression type feature

[Denis Plotnikov]

The patch adds some preparation parts for incompatible compression type
feature to qcow2 allowing the use different compression methods for
image clusters (de)compressing.

It is implied that the compression type is set on the image creation and
can be changed only later by image conversion, thus compression type
defines the only compression algorithm used for the image, and thus,
for all image clusters.

The goal of the feature is to add support of other compression methods
to qcow2. For example, ZSTD which is more effective on compression than ZLIB.

The default compression is ZLIB. Images created with ZLIB compression type
are backward compatible with older qemu versions.

Adding of the compression type breaks a number of tests because now the
compression type is reported on image creation and there are some changes
in the qcow2 header in size and offsets.

The tests are fixed in the following ways:
* filter out compression_type for many tests
* fix header size, feature table size and backing file offset
  affected tests: 031, 036, 061, 080
  header_size +=8: 1 byte compression type
   7 bytes padding
  feature_table += 48: incompatible feature compression type
  backing_file_offset += 56 (8 + 48 -> header_change + feature_table_change)
* add "compression type" for test output matching when it isn't filtered
  affected tests: 049, 060, 061, 065, 144, 182, 242, 255

Signed-off-by: Denis Plotnikov 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
---
 qapi/block-core.json |  22 +-
 block/qcow2.h|  20 +-
 include/block/block_int.h|   1 +
 block/qcow2.c| 113 +++
 tests/qemu-iotests/031.out   |  14 ++--
 tests/qemu-iotests/036.out   |   4 +-
 tests/qemu-iotests/049.out   | 102 ++--
 tests/qemu-iotests/060.out   |   1 +
 tests/qemu-iotests/061.out   |  34 ++
 tests/qemu-iotests/065   |  28 +---
 tests/qemu-iotests/080   |   2 +-
 tests/qemu-iotests/144.out   |   4 +-
 tests/qemu-iotests/182.out   |   2 +-
 tests/qemu-iotests/242.out   |   5 ++
 tests/qemu-iotests/255.out   |   8 +--
 tests/qemu-iotests/common.filter |   3 +-
 16 files changed, 267 insertions(+), 96 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 85e27bb61f..a306484973 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -78,6 +78,8 @@
 #
 # @bitmaps: A list of qcow2 bitmap details (since 4.0)
 #
+# @compression-type: the image cluster compression method (since 5.0)
+#
 # Since: 1.7
 ##
 { 'struct': 'ImageInfoSpecificQCow2',
@@ -89,7 +91,8 @@
   '*corrupt': 'bool',
   'refcount-bits': 'int',
   '*encrypt': 'ImageInfoSpecificQCow2Encryption',
-  '*bitmaps': ['Qcow2BitmapInfo']
+  '*bitmaps': ['Qcow2BitmapInfo'],
+  'compression-type': 'Qcow2CompressionType'
   } }
 
 ##
@@ -4392,6 +4395,18 @@
   'data': [ 'v2', 'v3' ] }
 
 
+##
+# @Qcow2CompressionType:
+#
+# Compression type used in qcow2 image file
+#
+# @zlib: zlib compression, see 
+#
+# Since: 5.0
+##
+{ 'enum': 'Qcow2CompressionType',
+  'data': [ 'zlib' ] }
+
 ##
 # @BlockdevCreateOptionsQcow2:
 #
@@ -4415,6 +4430,8 @@
 # allowed values: off, falloc, full, metadata)
 # @lazy-refcounts: True if refcounts may be updated lazily (default: off)
 # @refcount-bits: Width of reference counts in bits (default: 16)
+# @compression-type: The image cluster compression method
+#(default: zlib, since 5.0)
 #
 # Since: 2.12
 ##
@@ -4430,7 +4447,8 @@
 '*cluster-size':'size',
 '*preallocation':   'PreallocMode',
 '*lazy-refcounts':  'bool',
-'*refcount-bits':   'int' } }
+'*refcount-bits':   'int',
+'*compression-type':'Qcow2CompressionType' } }
 
 ##
 # @BlockdevCreateOptionsQed:
diff --git a/block/qcow2.h b/block/qcow2.h
index 0942126232..cb6bf2ab83 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -146,8 +146,16 @@ typedef struct QCowHeader {
 
 uint32_t refcount_order;
 uint32_t header_length;
+
+/* Additional fields */
+uint8_t compression_type;
+
+/* header must be a multiple of 8 */
+uint8_t padding[7];
 } QEMU_PACKED QCowHeader;
 
+QEMU_BUILD_BUG_ON(sizeof(QCowHeader) % 8 != 0);
+
 typedef struct QEMU_PACKED QCowSnapshotHeader {
 /* header is 8 byte aligned */
 uint64_t l1_table_offset;
@@ -216,13 +224,16 @@ enum {
 QCOW2_INCOMPAT_DIRTY_BITNR  = 0,
 QCOW2_INCOMPAT_CORRUPT_BITNR= 1,
 QCOW2_INCOMPAT_DATA_FILE_BITNR  = 2,
+QCOW2_INCOMPAT_COMPRESSION_BITNR = 3,
 QCOW2_INCOMPAT_DIRTY= 1 << QCOW2_INCOMPAT_DIRTY_BITNR,
 QCOW2_INCOMPAT_CORRUPT  = 1 << QCOW2_INCOMPAT_CORRUPT_BITNR,
 QCOW2_INCOMPAT_DATA_FILE= 1 << QCOW2_INCOMPAT_DATA_FILE_BITNR,
+QCOW2_INCOMPAT_COMPRESSION  = 1 << 

[PATCH v8 0/4] qcow2: Implement zstd cluster compression method

[Denis Plotnikov]

v8:
   * 03: switch zstd API from simple to stream [Eric]
 No need to state a special cluster layout for zstd
 compressed clusters.
v7:
   * use qapi_enum_parse instead of the open-coding [Eric]
   * fix wording, typos and spelling [Eric]

v6:
   * "block/qcow2-threads: fix qcow2_decompress" is removed from the series
  since it has been accepted by Max already
   * add compile time checking for Qcow2Header to be a multiple of 8 [Max, 
Alberto]
   * report error on qcow2 amending when the compression type is actually 
chnged [Max]
   * remove the extra space and the extra new line [Max]
   * re-arrange acks and signed-off-s [Vladimir]

v5:
   * replace -ENOTSUP with abort in qcow2_co_decompress [Vladimir]
   * set cluster size for all test cases in the beginning of the 287 test

v4:
   * the series is rebased on top of 01 "block/qcow2-threads: fix 
qcow2_decompress"
   * 01 is just a no-change resend to avoid extra dependencies. Still, it may 
be merged in separate

v3:
   * remove redundant max compression type value check [Vladimir, Eric]
 (the switch below checks everything)
   * prevent compression type changing on "qemu-img amend" [Vladimir]
   * remove zstd config setting, since it has been added already by
 "migration" patches [Vladimir]
   * change the compression type error message [Vladimir] 
   * fix alignment and 80-chars exceeding [Vladimir]

v2:
   * rework compression type setting [Vladimir]
   * squash iotest changes to the compression type introduction patch 
[Vladimir, Eric]
   * fix zstd availability checking in zstd iotest [Vladimir]
   * remove unnecessry casting [Eric]
   * remove rudundant checks [Eric]
   * fix compressed cluster layout in qcow2 spec [Vladimir]
   * fix wording [Eric, Vladimir]
   * fix compression type filtering in iotests [Eric]

v1:
   the initial series



Denis Plotnikov (4):
  qcow2: introduce compression type feature
  qcow2: rework the cluster compression routine
  qcow2: add zstd cluster compression
  iotests: 287: add qcow2 compression type test

 docs/interop/qcow2.txt   |   1 +
 configure|   2 +-
 qapi/block-core.json |  23 +++-
 block/qcow2.h|  20 +++-
 include/block/block_int.h|   1 +
 block/qcow2-threads.c| 200 +--
 block/qcow2.c| 120 +++
 tests/qemu-iotests/031.out   |  14 +--
 tests/qemu-iotests/036.out   |   4 +-
 tests/qemu-iotests/049.out   | 102 
 tests/qemu-iotests/060.out   |   1 +
 tests/qemu-iotests/061.out   |  34 +++---
 tests/qemu-iotests/065   |  28 +++--
 tests/qemu-iotests/080   |   2 +-
 tests/qemu-iotests/144.out   |   4 +-
 tests/qemu-iotests/182.out   |   2 +-
 tests/qemu-iotests/242.out   |   5 +
 tests/qemu-iotests/255.out   |   8 +-
 tests/qemu-iotests/287   | 128 
 tests/qemu-iotests/287.out   |  43 +++
 tests/qemu-iotests/common.filter |   3 +-
 tests/qemu-iotests/group |   1 +
 22 files changed, 638 insertions(+), 108 deletions(-)
 create mode 100755 tests/qemu-iotests/287
 create mode 100644 tests/qemu-iotests/287.out

-- 
2.17.0

[PATCH v8 4/4] iotests: 287: add qcow2 compression type test

[Denis Plotnikov]

The test checks fulfilling qcow2 requiriements for the compression
type feature and zstd compression type operability.

Signed-off-by: Denis Plotnikov 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
---
 tests/qemu-iotests/287 | 128 +
 tests/qemu-iotests/287.out |  43 +
 tests/qemu-iotests/group   |   1 +
 3 files changed, 172 insertions(+)
 create mode 100755 tests/qemu-iotests/287
 create mode 100644 tests/qemu-iotests/287.out

diff --git a/tests/qemu-iotests/287 b/tests/qemu-iotests/287
new file mode 100755
index 00..49d15b3d43
--- /dev/null
+++ b/tests/qemu-iotests/287
@@ -0,0 +1,128 @@
+#!/usr/bin/env bash
+#
+# Test case for an image using zstd compression
+#
+# Copyright (c) 2020 Virtuozzo International GmbH
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see .
+#
+
+# creator
+owner=dplotni...@virtuozzo.com
+
+seq="$(basename $0)"
+echo "QA output created by $seq"
+
+status=1   # failure is the default!
+
+_cleanup()
+{
+   _cleanup_test_img
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# standard environment
+. ./common.rc
+. ./common.filter
+
+# This tests qocw2-specific low-level functionality
+_supported_fmt qcow2
+_supported_proto file
+_supported_os Linux
+
+# for all the cases
+CLUSTER_SIZE=65536
+
+# Check if we can run this test.
+
+IMGOPTS='compression_type=zstd' _make_test_img 64M | grep "Invalid parameter 
'zstd'" 2>&1 1>/dev/null
+
+ZSTD_SUPPORTED=$?
+
+if (($ZSTD_SUPPORTED==0)); then
+_notrun "ZSTD is disabled"
+fi
+
+# Test: when compression is zlib the incompatible bit is unset
+echo
+echo "=== Testing compression type incompatible bit setting for zlib ==="
+echo
+
+IMGOPTS='compression_type=zlib' _make_test_img 64M
+$PYTHON qcow2.py "$TEST_IMG" dump-header | grep incompatible_features
+
+# Test: when compression differs from zlib the incompatible bit is set
+echo
+echo "=== Testing compression type incompatible bit setting for zstd ==="
+echo
+
+IMGOPTS='compression_type=zstd' _make_test_img 64M
+$PYTHON qcow2.py "$TEST_IMG" dump-header | grep incompatible_features
+
+# Test: an image can't be openned if compression type is zlib and
+#   incompatible feature compression type is set
+echo
+echo "=== Testing zlib with incompatible bit set  ==="
+echo
+
+IMGOPTS='compression_type=zlib' _make_test_img 64M
+$PYTHON qcow2.py "$TEST_IMG" set-feature-bit incompatible 3
+# to make sure the bit was actually set
+$PYTHON qcow2.py "$TEST_IMG" dump-header | grep incompatible_features
+$QEMU_IMG info "$TEST_IMG" 2>1 1>/dev/null
+if (($?==0)); then
+echo "Error: The image openned successfully. The image must not be openned"
+fi
+
+# Test: an image can't be openned if compression type is NOT zlib and
+#   incompatible feature compression type is UNSET
+echo
+echo "=== Testing zstd with incompatible bit unset  ==="
+echo
+
+IMGOPTS='compression_type=zstd' _make_test_img 64M
+$PYTHON qcow2.py "$TEST_IMG" set-header incompatible_features 0
+# to make sure the bit was actually unset
+$PYTHON qcow2.py "$TEST_IMG" dump-header | grep incompatible_features
+$QEMU_IMG info "$TEST_IMG" 2>1 1>/dev/null
+if (($?==0)); then
+echo "Error: The image openned successfully. The image must not be openned"
+fi
+# Test: check compression type values
+echo
+echo "=== Testing compression type values  ==="
+echo
+# zlib=0
+IMGOPTS='compression_type=zlib' _make_test_img 64M
+od -j104 -N1 -An -vtu1 "$TEST_IMG"
+
+# zstd=1
+IMGOPTS='compression_type=zstd' _make_test_img 64M
+od -j104 -N1 -An -vtu1 "$TEST_IMG"
+
+# Test: using zstd compression, write to and read from an image
+echo
+echo "=== Testing reading and writing with zstd ==="
+echo
+
+IMGOPTS='compression_type=zstd' _make_test_img 64M
+$QEMU_IO -c "write -c -P 0xAC 65536 64k " "$TEST_IMG" | _filter_qemu_io
+$QEMU_IO -c "read -P 0xAC 65536 65536 " "$TEST_IMG" | _filter_qemu_io
+$QEMU_IO -c "read -v 131070 8 " "$TEST_IMG" | _filter_qemu_io
+$QEMU_IO -c "read -v 65534 8" "$TEST_IMG" | _filter_qemu_io
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/287.out b/tests/qemu-iotests/287.out
new file mode 100644
index 00..8e51c3078d
--- /dev/null
+++ b/tests/qemu-iotests/287.out
@@ -0,0 +1,43 @@
+QA output created by 287
+
+=== Testing compression type incompatible bit setting for zlib ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864

[PATCH v8 3/4] qcow2: add zstd cluster compression

[Denis Plotnikov]

zstd significantly reduces cluster compression time.
It provides better compression performance maintaining
the same level of the compression ratio in comparison with
zlib, which, at the moment, is the only compression
method available.

The performance test results:
Test compresses and decompresses qemu qcow2 image with just
installed rhel-7.6 guest.
Image cluster size: 64K. Image on disk size: 2.2G

The test was conducted with brd disk to reduce the influence
of disk subsystem to the test results.
The results is given in seconds.

compress cmd:
  time ./qemu-img convert -O qcow2 -c -o compression_type=[zlib|zstd]
  src.img [zlib|zstd]_compressed.img
decompress cmd
  time ./qemu-img convert -O qcow2
  [zlib|zstd]_compressed.img uncompressed.img

   compression   decompression
 zlib   zstd   zlib zstd

real 65.5   16.3 (-75 %)1.9  1.6 (-16 %)
user 65.0   15.85.3  2.5
sys   3.30.22.0  2.0

Both ZLIB and ZSTD gave the same compression ratio: 1.57
compressed image size in both cases: 1.4G

Signed-off-by: Denis Plotnikov 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Alberto Garcia 
QAPI part:
Acked-by: Markus Armbruster 
---
 docs/interop/qcow2.txt |   1 +
 configure  |   2 +-
 qapi/block-core.json   |   3 +-
 block/qcow2-threads.c  | 129 +
 block/qcow2.c  |   7 +++
 5 files changed, 140 insertions(+), 2 deletions(-)

diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
index 5597e24474..795dbb21dd 100644
--- a/docs/interop/qcow2.txt
+++ b/docs/interop/qcow2.txt
@@ -208,6 +208,7 @@ version 2.
 
 Available compression type values:
 0: zlib 
+1: zstd 
 
 
 === Header padding ===
diff --git a/configure b/configure
index caa65f5883..b2a0aa241a 100755
--- a/configure
+++ b/configure
@@ -1835,7 +1835,7 @@ disabled with --disable-FEATURE, default is enabled if 
available:
   lzfse   support of lzfse compression library
   (for reading lzfse-compressed dmg images)
   zstdsupport for zstd compression library
-  (for migration compression)
+  (for migration compression and qcow2 cluster compression)
   seccomp seccomp support
   coroutine-pool  coroutine freelist (better performance)
   glusterfs   GlusterFS backend
diff --git a/qapi/block-core.json b/qapi/block-core.json
index a306484973..8953451818 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -4401,11 +4401,12 @@
 # Compression type used in qcow2 image file
 #
 # @zlib: zlib compression, see 
+# @zstd: zstd compression, see 
 #
 # Since: 5.0
 ##
 { 'enum': 'Qcow2CompressionType',
-  'data': [ 'zlib' ] }
+  'data': [ 'zlib', { 'name': 'zstd', 'if': 'defined(CONFIG_ZSTD)' } ] }
 
 ##
 # @BlockdevCreateOptionsQcow2:
diff --git a/block/qcow2-threads.c b/block/qcow2-threads.c
index 7dbaf53489..ee4bad8d5b 100644
--- a/block/qcow2-threads.c
+++ b/block/qcow2-threads.c
@@ -28,6 +28,11 @@
 #define ZLIB_CONST
 #include 
 
+#ifdef CONFIG_ZSTD
+#include 
+#include 
+#endif
+
 #include "qcow2.h"
 #include "block/thread-pool.h"
 #include "crypto.h"
@@ -166,6 +171,120 @@ static ssize_t qcow2_zlib_decompress(void *dest, size_t 
dest_size,
 return ret;
 }
 
+#ifdef CONFIG_ZSTD
+
+/*
+ * qcow2_zstd_compress()
+ *
+ * Compress @src_size bytes of data using zstd compression method
+ *
+ * @dest - destination buffer, @dest_size bytes
+ * @src - source buffer, @src_size bytes
+ *
+ * Returns: compressed size on success
+ *  -ENOMEM destination buffer is not enough to store compressed data
+ *  -EIOon any other error
+ */
+static ssize_t qcow2_zstd_compress(void *dest, size_t dest_size,
+   const void *src, size_t src_size)
+{
+size_t ret;
+ZSTD_outBuffer output = { dest, dest_size, 0 };
+ZSTD_inBuffer input = { src, src_size, 0 };
+ZSTD_CCtx *cctx = ZSTD_createCCtx();
+
+if (!cctx) {
+return -EIO;
+}
+
+ret = ZSTD_CCtx_setParameter(cctx, ZSTD_c_compressionLevel,
+ ZSTD_CLEVEL_DEFAULT);
+if (ZSTD_isError(ret)) {
+ret = -EIO;
+goto out;
+}
+
+{
+/*
+ * Instruct zstd to compress the whole buffer and write
+ * the frame epilogue. This allows as to use zstd streaming
+ * semantics and don't store the compressed size for the
+ * zstd decompression.
+ */
+ret = ZSTD_compressStream2(cctx,  , , ZSTD_e_end);
+if (ZSTD_isError(ret)) {
+ret = -EIO;
+goto out;
+}
+/* Dest buffer isn't big 

[PATCH v8 2/4] qcow2: rework the cluster compression routine

[Denis Plotnikov]

The patch enables processing the image compression type defined
for the image and chooses an appropriate method for image clusters
(de)compression.

Signed-off-by: Denis Plotnikov 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Alberto Garcia 
---
 block/qcow2-threads.c | 71 ---
 1 file changed, 60 insertions(+), 11 deletions(-)

diff --git a/block/qcow2-threads.c b/block/qcow2-threads.c
index a68126f291..7dbaf53489 100644
--- a/block/qcow2-threads.c
+++ b/block/qcow2-threads.c
@@ -74,7 +74,9 @@ typedef struct Qcow2CompressData {
 } Qcow2CompressData;
 
 /*
- * qcow2_compress()
+ * qcow2_zlib_compress()
+ *
+ * Compress @src_size bytes of data using zlib compression method
  *
  * @dest - destination buffer, @dest_size bytes
  * @src - source buffer, @src_size bytes
@@ -83,8 +85,8 @@ typedef struct Qcow2CompressData {
  *  -ENOMEM destination buffer is not enough to store compressed data
  *  -EIOon any other error
  */
-static ssize_t qcow2_compress(void *dest, size_t dest_size,
-  const void *src, size_t src_size)
+static ssize_t qcow2_zlib_compress(void *dest, size_t dest_size,
+   const void *src, size_t src_size)
 {
 ssize_t ret;
 z_stream strm;
@@ -119,10 +121,10 @@ static ssize_t qcow2_compress(void *dest, size_t 
dest_size,
 }
 
 /*
- * qcow2_decompress()
+ * qcow2_zlib_decompress()
  *
  * Decompress some data (not more than @src_size bytes) to produce exactly
- * @dest_size bytes.
+ * @dest_size bytes using zlib compression method
  *
  * @dest - destination buffer, @dest_size bytes
  * @src - source buffer, @src_size bytes
@@ -130,8 +132,8 @@ static ssize_t qcow2_compress(void *dest, size_t dest_size,
  * Returns: 0 on success
  *  -EIO on fail
  */
-static ssize_t qcow2_decompress(void *dest, size_t dest_size,
-const void *src, size_t src_size)
+static ssize_t qcow2_zlib_decompress(void *dest, size_t dest_size,
+ const void *src, size_t src_size)
 {
 int ret;
 z_stream strm;
@@ -191,20 +193,67 @@ qcow2_co_do_compress(BlockDriverState *bs, void *dest, 
size_t dest_size,
 return arg.ret;
 }
 
+/*
+ * qcow2_co_compress()
+ *
+ * Compress @src_size bytes of data using the compression
+ * method defined by the image compression type
+ *
+ * @dest - destination buffer, @dest_size bytes
+ * @src - source buffer, @src_size bytes
+ *
+ * Returns: compressed size on success
+ *  a negative error code on failure
+ */
 ssize_t coroutine_fn
 qcow2_co_compress(BlockDriverState *bs, void *dest, size_t dest_size,
   const void *src, size_t src_size)
 {
-return qcow2_co_do_compress(bs, dest, dest_size, src, src_size,
-qcow2_compress);
+BDRVQcow2State *s = bs->opaque;
+Qcow2CompressFunc fn;
+
+switch (s->compression_type) {
+case QCOW2_COMPRESSION_TYPE_ZLIB:
+fn = qcow2_zlib_compress;
+break;
+
+default:
+abort();
+}
+
+return qcow2_co_do_compress(bs, dest, dest_size, src, src_size, fn);
 }
 
+/*
+ * qcow2_co_decompress()
+ *
+ * Decompress some data (not more than @src_size bytes) to produce exactly
+ * @dest_size bytes using the compression method defined by the image
+ * compression type
+ *
+ * @dest - destination buffer, @dest_size bytes
+ * @src - source buffer, @src_size bytes
+ *
+ * Returns: 0 on success
+ *  a negative error code on failure
+ */
 ssize_t coroutine_fn
 qcow2_co_decompress(BlockDriverState *bs, void *dest, size_t dest_size,
 const void *src, size_t src_size)
 {
-return qcow2_co_do_compress(bs, dest, dest_size, src, src_size,
-qcow2_decompress);
+BDRVQcow2State *s = bs->opaque;
+Qcow2CompressFunc fn;
+
+switch (s->compression_type) {
+case QCOW2_COMPRESSION_TYPE_ZLIB:
+fn = qcow2_zlib_decompress;
+break;
+
+default:
+abort();
+}
+
+return qcow2_co_do_compress(bs, dest, dest_size, src, src_size, fn);
 }
 
 
-- 
2.17.0

Re: [PATCH-for-5.0 05/11] hw/ide/sii3112: Remove dead assignment

[Philippe Mathieu-Daudé]

On 3/21/20 3:14 PM, Philippe Mathieu-Daudé wrote:

On 3/21/20 2:39 PM, Aleksandar Markovic wrote:



On Saturday, March 21, 2020, Philippe Mathieu-Daudé > wrote:


    Fix warning reported by Clang static code analyzer:

     CC      hw/ide/sii3112.o
   hw/ide/sii3112.c:204:9: warning: Value stored to 'val' is never 
read

           val = 0;
           ^     ~

    Reported-by: Clang Static Analyzer
    Signed-off-by: Philippe Mathieu-Daudé mailto:phi...@redhat.com>>
    ---
  hw/ide/sii3112.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

    diff --git a/hw/ide/sii3112.c b/hw/ide/sii3112.c
    index 06605d7af2..36f1905ddb 100644
    --- a/hw/ide/sii3112.c
    +++ b/hw/ide/sii3112.c
    @@ -125,7 +125,7 @@ static uint64_t sii3112_reg_read(void *opaque,
    hwaddr addr,
          val = (uint32_t)d->regs[1].sien << 16;
          break;
      default:
    -        val = 0;
    +        break;


There is another function in the same file, having a similar switch 
statement. There is no warning for that second tunction, since "val" 
is its parameter, not a local varioble, like is the case here. This 
means that the proposed change introduces inconsistency between two 
functions, therefore it is better to remove the initialization of 
"val" to 0, than to remove this line in "default" section.


No clue why there is no warnings emitted for sii3112_reg_write()...

Do you mind sending a patch?


Don't worry I'll follow up.





Regards,
Aleksandar

      }
      trace_sii3112_read(size, addr, val);
      return val;
    --     2.21.1

Re: [PATCH-for-5.0 05/11] hw/ide/sii3112: Remove dead assignment

[BALATON Zoltan]

On Sat, 21 Mar 2020, Philippe Mathieu-Daudé wrote:

On 3/21/20 3:12 PM, BALATON Zoltan wrote:

On Sat, 21 Mar 2020, Aleksandar Markovic wrote:

On Saturday, March 21, 2020, Philippe Mathieu-Daudé 
wrote:


Fix warning reported by Clang static code analyzer:

    CC  hw/ide/sii3112.o
  hw/ide/sii3112.c:204:9: warning: Value stored to 'val' is never read
  val = 0;
  ^ ~

Reported-by: Clang Static Analyzer
Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/ide/sii3112.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/ide/sii3112.c b/hw/ide/sii3112.c
index 06605d7af2..36f1905ddb 100644
--- a/hw/ide/sii3112.c
+++ b/hw/ide/sii3112.c
@@ -125,7 +125,7 @@ static uint64_t sii3112_reg_read(void *opaque, hwaddr
addr,
 val = (uint32_t)d->regs[1].sien << 16;
 break;
 default:
-    val = 0;
+    break;



There is another function in the same file, having a similar switch
statement. There is no warning for that second tunction, since "val" is 
its
parameter, not a local varioble, like is the case here. This means that 
the

proposed change introduces inconsistency between two functions, therefore
it is better to remove the initialization of "val" to 0, than to remove
this line in "default" section.


Oh, actually I think the warning was for that statement not for the one 
patched as it makes more sense there where val is assigned in void 
sii3112_reg_write() where it's indeed not used so maybe that was meant to 
be patched instead?


Ah, the warning is for hw/ide/sii3112.c:204 but I incorrectly patched 
hw/ide/sii3112.c:128 =)


Now you may patch both to replace val = 0 with break to keep symmetry. My 
Reviewed-by stands (even if apparently not much use). Thanks for 
Aleksandar for spotting it.


Regards,
BALATON Zoltan

Re: [PATCH-for-5.0 05/11] hw/ide/sii3112: Remove dead assignment

[Philippe Mathieu-Daudé]

On 3/21/20 3:12 PM, BALATON Zoltan wrote:

On Sat, 21 Mar 2020, Aleksandar Markovic wrote:

On Saturday, March 21, 2020, Philippe Mathieu-Daudé 
wrote:


Fix warning reported by Clang static code analyzer:

    CC  hw/ide/sii3112.o
  hw/ide/sii3112.c:204:9: warning: Value stored to 'val' is never read
  val = 0;
  ^ ~

Reported-by: Clang Static Analyzer
Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/ide/sii3112.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/ide/sii3112.c b/hw/ide/sii3112.c
index 06605d7af2..36f1905ddb 100644
--- a/hw/ide/sii3112.c
+++ b/hw/ide/sii3112.c
@@ -125,7 +125,7 @@ static uint64_t sii3112_reg_read(void *opaque, 
hwaddr

addr,
 val = (uint32_t)d->regs[1].sien << 16;
 break;
 default:
-    val = 0;
+    break;



There is another function in the same file, having a similar switch
statement. There is no warning for that second tunction, since "val" 
is its
parameter, not a local varioble, like is the case here. This means 
that the

proposed change introduces inconsistency between two functions, therefore
it is better to remove the initialization of "val" to 0, than to remove
this line in "default" section.


Oh, actually I think the warning was for that statement not for the one 
patched as it makes more sense there where val is assigned in void 
sii3112_reg_write() where it's indeed not used so maybe that was meant 
to be patched instead?


Ah, the warning is for hw/ide/sii3112.c:204 but I incorrectly patched 
hw/ide/sii3112.c:128 =)




Regards,
BALATON Zoltan

Re: [PATCH-for-5.0 05/11] hw/ide/sii3112: Remove dead assignment

[Philippe Mathieu-Daudé]

On 3/21/20 2:39 PM, Aleksandar Markovic wrote:



On Saturday, March 21, 2020, Philippe Mathieu-Daudé > wrote:


Fix warning reported by Clang static code analyzer:

     CC      hw/ide/sii3112.o
   hw/ide/sii3112.c:204:9: warning: Value stored to 'val' is never read
           val = 0;
           ^     ~

Reported-by: Clang Static Analyzer
Signed-off-by: Philippe Mathieu-Daudé mailto:phi...@redhat.com>>
---
  hw/ide/sii3112.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/ide/sii3112.c b/hw/ide/sii3112.c
index 06605d7af2..36f1905ddb 100644
--- a/hw/ide/sii3112.c
+++ b/hw/ide/sii3112.c
@@ -125,7 +125,7 @@ static uint64_t sii3112_reg_read(void *opaque,
hwaddr addr,
          val = (uint32_t)d->regs[1].sien << 16;
          break;
      default:
-        val = 0;
+        break;


There is another function in the same file, having a similar switch 
statement. There is no warning for that second tunction, since "val" is 
its parameter, not a local varioble, like is the case here. This means 
that the proposed change introduces inconsistency between two functions, 
therefore it is better to remove the initialization of "val" to 0, than 
to remove this line in "default" section.


No clue why there is no warnings emitted for sii3112_reg_write()...

Do you mind sending a patch?



Regards,
Aleksandar

      }
      trace_sii3112_read(size, addr, val);
      return val;
-- 
2.21.1

Re: [PATCH-for-5.0 05/11] hw/ide/sii3112: Remove dead assignment

[BALATON Zoltan]

On Sat, 21 Mar 2020, Aleksandar Markovic wrote:

On Saturday, March 21, 2020, Philippe Mathieu-Daudé 
wrote:


Fix warning reported by Clang static code analyzer:

CC  hw/ide/sii3112.o
  hw/ide/sii3112.c:204:9: warning: Value stored to 'val' is never read
  val = 0;
  ^ ~

Reported-by: Clang Static Analyzer
Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/ide/sii3112.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/ide/sii3112.c b/hw/ide/sii3112.c
index 06605d7af2..36f1905ddb 100644
--- a/hw/ide/sii3112.c
+++ b/hw/ide/sii3112.c
@@ -125,7 +125,7 @@ static uint64_t sii3112_reg_read(void *opaque, hwaddr
addr,
 val = (uint32_t)d->regs[1].sien << 16;
 break;
 default:
-val = 0;
+break;



There is another function in the same file, having a similar switch
statement. There is no warning for that second tunction, since "val" is its
parameter, not a local varioble, like is the case here. This means that the
proposed change introduces inconsistency between two functions, therefore
it is better to remove the initialization of "val" to 0, than to remove
this line in "default" section.


Oh, actually I think the warning was for that statement not for the one 
patched as it makes more sense there where val is assigned in void 
sii3112_reg_write() where it's indeed not used so maybe that was meant to 
be patched instead?


Regards,
BALATON Zoltan

Re: [PATCH-for-5.0 07/11] hw/gpio/aspeed_gpio: Remove dead assignment

[Philippe Mathieu-Daudé]

On 3/21/20 2:22 PM, BALATON Zoltan wrote:

On Sat, 21 Mar 2020, Philippe Mathieu-Daudé wrote:

Fix warning reported by Clang static code analyzer:

 hw/gpio/aspeed_gpio.c:717:18: warning: Value stored to 'g_idx' during 
its initialization is never read

 int set_idx, g_idx = *group_idx;
  ^   ~~

Reported-by: Clang Static Analyzer
Signed-off-by: Philippe Mathieu-Daudé 
---
hw/gpio/aspeed_gpio.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/gpio/aspeed_gpio.c b/hw/gpio/aspeed_gpio.c
index 41e11ea9b0..cc07ab9866 100644
--- a/hw/gpio/aspeed_gpio.c
+++ b/hw/gpio/aspeed_gpio.c
@@ -714,11 +714,11 @@ static void aspeed_gpio_write(void *opaque, 
hwaddr offset, uint64_t data,
static int get_set_idx(AspeedGPIOState *s, const char *group, int 
*group_idx)

{
    AspeedGPIOClass *agc = ASPEED_GPIO_GET_CLASS(s);
-    int set_idx, g_idx = *group_idx;
+    int set_idx;

    for (set_idx = 0; set_idx < agc->nr_gpio_sets; set_idx++) {
    const GPIOSetProperties *set_props = >props[set_idx];
-    for (g_idx = 0; g_idx < ASPEED_GROUPS_PER_SET; g_idx++) {
+    for (int g_idx = 0; g_idx < ASPEED_GROUPS_PER_SET; g_idx++) {


Is declaring variables here allowed by coding style? Shouldn't you only 
remove init value from above?


You are right, it is not (yet?) allowed by QEMU coding style.



Regards,
BALATON Zoltan

    if (!strncmp(group, set_props->group_label[g_idx], 
strlen(group))) {

    *group_idx = g_idx;
    return set_idx;

Re: [PATCH] gdbstub: add support to Xfer:auxv:read: packet

[Alex Bennée]

Lirong Yuan  writes:

> On Fri, Mar 20, 2020 at 2:17 AM Alex Bennée  wrote:

>>
>> Sorry I missed this on my radar. There was a minor re-factor of gdbstub
>> that was just merged which will mean this patch needs a re-base to use
>> g_string_* functions to expand stings.
>>
>> Also we have some simple gdbstub tests now - could we come up with a
>> multiarch gdbstub test to verify this is working properly?
>>

> For sure, I will re-base this patch to use g_string_* functions.
>
> Currently we are using qemu aarch64. I am not sure how to do this yet, but
> I could try to add something to
> https://github.com/qemu/qemu/tree/master/tests/tcg/aarch64/gdbstub

If the auxv support is appropriate to all linux-user targets you can
plumb it into the multiarch tests - you can even use the existing
binaries.

So you need:

  - a stanza in the makefiles to launch the test (see
tests/tcg/aarch64/Makefile.target)

  - a .py test script that manipulates gdbstub to check things are working

So something like:

.PHONY: gdbstub-foo-binary
run-gdbstub-foo-binary: foo-binary
$(call run-test, $@, $(GDB_SCRIPT) \
--gdb $(HAVE_GDB_BIN) \
--qemu $(QEMU) --qargs "$(QEMU_OPTS)" \
--bin $< --test $(MULTIARCH_SRC)/gdbstub/test-foo.py, \
"basic gdbstub FOO support")


>
> Does this sound good?

Hope that helps.

>
> Thanks!
> Lirong


-- 
Alex Bennée

Re: [PATCH-for-5.0 05/11] hw/ide/sii3112: Remove dead assignment

[Aleksandar Markovic]

On Saturday, March 21, 2020, Philippe Mathieu-Daudé 
wrote:

> Fix warning reported by Clang static code analyzer:
>
> CC  hw/ide/sii3112.o
>   hw/ide/sii3112.c:204:9: warning: Value stored to 'val' is never read
>   val = 0;
>   ^ ~
>
> Reported-by: Clang Static Analyzer
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  hw/ide/sii3112.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/ide/sii3112.c b/hw/ide/sii3112.c
> index 06605d7af2..36f1905ddb 100644
> --- a/hw/ide/sii3112.c
> +++ b/hw/ide/sii3112.c
> @@ -125,7 +125,7 @@ static uint64_t sii3112_reg_read(void *opaque, hwaddr
> addr,
>  val = (uint32_t)d->regs[1].sien << 16;
>  break;
>  default:
> -val = 0;
> +break;


There is another function in the same file, having a similar switch
statement. There is no warning for that second tunction, since "val" is its
parameter, not a local varioble, like is the case here. This means that the
proposed change introduces inconsistency between two functions, therefore
it is better to remove the initialization of "val" to 0, than to remove
this line in "default" section.

Regards,
Aleksandar



>  }
>  trace_sii3112_read(size, addr, val);
>  return val;
> --
> 2.21.1
>
>
>

[PATCH v2] migration: use "" instead of (null) for tls-authz

[Mao Zhongyi]

run:
(qemu) info migrate_parameters
announce-initial: 50 ms
...
announce-max: 550 ms
multifd-compression: none
xbzrle-cache-size: 4194304
max-postcopy-bandwidth: 0
 tls-authz: '(null)'

Migration parameter 'tls-authz' is used to provide the QOM ID
of a QAuthZ subclass instance that provides the access control
check, default is NULL. But the empty string is not a valid
object ID, so use "" instead of the default. Although it will
fail when lookup an object with ID "", it is harmless, just
consistent with tls_creds.

Also fixed the bad indentation on the last line.

Signed-off-by: Mao Zhongyi 
---
 migration/migration.c | 3 ++-
 monitor/hmp-cmds.c| 2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index c1d88ace7f..b060153ef7 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -790,7 +790,8 @@ MigrationParameters *qmp_query_migrate_parameters(Error 
**errp)
 params->has_tls_hostname = true;
 params->tls_hostname = g_strdup(s->parameters.tls_hostname);
 params->has_tls_authz = true;
-params->tls_authz = g_strdup(s->parameters.tls_authz);
+params->tls_authz = s->parameters.tls_authz ? \
+g_strdup(s->parameters.tls_authz) : g_strdup("");
 params->has_max_bandwidth = true;
 params->max_bandwidth = s->parameters.max_bandwidth;
 params->has_downtime_limit = true;
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index 58724031ea..f8be6bbb16 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -459,7 +459,7 @@ void hmp_info_migrate_parameters(Monitor *mon, const QDict 
*qdict)
 monitor_printf(mon, "%s: %" PRIu64 "\n",
 MigrationParameter_str(MIGRATION_PARAMETER_MAX_POSTCOPY_BANDWIDTH),
 params->max_postcopy_bandwidth);
-monitor_printf(mon, " %s: '%s'\n",
+monitor_printf(mon, "%s: '%s'\n",
 MigrationParameter_str(MIGRATION_PARAMETER_TLS_AUTHZ),
 params->has_tls_authz ? params->tls_authz : "");
 }
-- 
2.17.1

Re: [PATCH-for-5.0 07/11] hw/gpio/aspeed_gpio: Remove dead assignment

[BALATON Zoltan]

On Sat, 21 Mar 2020, Philippe Mathieu-Daudé wrote:

Fix warning reported by Clang static code analyzer:

 hw/gpio/aspeed_gpio.c:717:18: warning: Value stored to 'g_idx' during its 
initialization is never read
 int set_idx, g_idx = *group_idx;
  ^   ~~

Reported-by: Clang Static Analyzer
Signed-off-by: Philippe Mathieu-Daudé 
---
hw/gpio/aspeed_gpio.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/gpio/aspeed_gpio.c b/hw/gpio/aspeed_gpio.c
index 41e11ea9b0..cc07ab9866 100644
--- a/hw/gpio/aspeed_gpio.c
+++ b/hw/gpio/aspeed_gpio.c
@@ -714,11 +714,11 @@ static void aspeed_gpio_write(void *opaque, hwaddr 
offset, uint64_t data,
static int get_set_idx(AspeedGPIOState *s, const char *group, int *group_idx)
{
AspeedGPIOClass *agc = ASPEED_GPIO_GET_CLASS(s);
-int set_idx, g_idx = *group_idx;
+int set_idx;

for (set_idx = 0; set_idx < agc->nr_gpio_sets; set_idx++) {
const GPIOSetProperties *set_props = >props[set_idx];
-for (g_idx = 0; g_idx < ASPEED_GROUPS_PER_SET; g_idx++) {
+for (int g_idx = 0; g_idx < ASPEED_GROUPS_PER_SET; g_idx++) {


Is declaring variables here allowed by coding style? Shouldn't you only 
remove init value from above?


Regards,
BALATON Zoltan


if (!strncmp(group, set_props->group_label[g_idx], strlen(group))) {
*group_idx = g_idx;
return set_idx;

Re: [PATCH-for-5.0 01/11] block: Remove dead assignment

[Markus Armbruster]

Laurent Vivier  writes:

> Le 21/03/2020 à 12:46, Philippe Mathieu-Daudé a écrit :
>> Fix warning reported by Clang static code analyzer:
>> 
>>   block.c:3167:5: warning: Value stored to 'ret' is never read
>>   ret = bdrv_fill_options(, filename, , _err);
>>   ^ ~
>> 
>> Reported-by: Clang Static Analyzer
>> Signed-off-by: Philippe Mathieu-Daudé 
>> ---
>>  block.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/block.c b/block.c
>> index a2542c977b..908c109a8c 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -3164,7 +3164,7 @@ static BlockDriverState *bdrv_open_inherit(const char 
>> *filename,
>>  parent->open_flags, parent->options);
>>  }
>>  
>> -ret = bdrv_fill_options(, filename, , _err);
>> +bdrv_fill_options(, filename, , _err);
>>  if (local_err) {
>>  goto fail;
>>  }
>> 
>
> I would be sruprised if coverity doesn't warn about an unused return value.

Coverity recognizes the fact that some return values can be safely
ignored, and reports only ignored return values it sees commonly checked
elsewhere.

This function is used called nowhere else.  Coverity won't complain.

However, I'd prefer

ret = bdrv_fill_options(, filename, , _err);
   -if (local_err) {
   +if (ret < 0) {
goto fail;
}

Re: [PATCH-for-5.0 05/11] hw/ide/sii3112: Remove dead assignment

[BALATON Zoltan]

On Sat, 21 Mar 2020, Philippe Mathieu-Daudé wrote:

Fix warning reported by Clang static code analyzer:

   CC  hw/ide/sii3112.o
 hw/ide/sii3112.c:204:9: warning: Value stored to 'val' is never read
 val = 0;
 ^ ~

Reported-by: Clang Static Analyzer
Signed-off-by: Philippe Mathieu-Daudé 
---
hw/ide/sii3112.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/ide/sii3112.c b/hw/ide/sii3112.c
index 06605d7af2..36f1905ddb 100644
--- a/hw/ide/sii3112.c
+++ b/hw/ide/sii3112.c
@@ -125,7 +125,7 @@ static uint64_t sii3112_reg_read(void *opaque, hwaddr addr,
val = (uint32_t)d->regs[1].sien << 16;
break;
default:
-val = 0;
+break;
}
trace_sii3112_read(size, addr, val);
return val;


Value is clearly used in trace and return so don't really get why the 
compiler complains here. Looks like wrong warning to me. It's true however 
that since val is init to 0 at the beginning this assignment is not 
strictily needed and this should work as well, so


Reviewed-by: BALATON Zoltan 

Regards,
BALATON Zoltan

Re: [PATCH-for-5.0 04/11] hw/input/adb-kbd: Remove dead assignment

[BALATON Zoltan]

On Sat, 21 Mar 2020, Philippe Mathieu-Daudé wrote:

Fix warning reported by Clang static code analyzer:

   CC  hw/input/adb-kbd.o
 hw/input/adb-kbd.c:200:5: warning: Value stored to 'olen' is never read
 olen = 0;
 ^  ~

Reported-by: Clang Static Analyzer
Signed-off-by: Philippe Mathieu-Daudé 
---
hw/input/adb-kbd.c | 1 -
1 file changed, 1 deletion(-)

diff --git a/hw/input/adb-kbd.c b/hw/input/adb-kbd.c
index 0ba8207589..b0565be21b 100644
--- a/hw/input/adb-kbd.c
+++ b/hw/input/adb-kbd.c
@@ -197,7 +197,6 @@ static int adb_kbd_poll(ADBDevice *d, uint8_t *obuf)
int keycode;
int olen;

-olen = 0;
if (s->count == 0) {
return 0;
}


Is this variable still needed at all? Looks like it's a remnant after 
a1f4971863 that can now only return 0 or 2 so you could just remove this 
variable and return 2 where now it's assigned as 2 or change return olen 
at the end to return 2 or maybe keep olen and change the return 0 above to 
return olen to silence warning but I think this could be cleaned up.


Regards,
BALATON Zoltan

Re: [PATCH-for-5.0 00/11] misc: Trivial static code analyzer fixes

[Philippe Mathieu-Daudé]

On 3/21/20 1:01 PM, Aleksandar Markovic wrote:
12:47 PM Sub, 21.03.2020. Philippe Mathieu-Daudé > је написао/ла:

 >
 > Fix trivial warnings reported by the Clang static code analyzer.
 >

Philippe,

It would be useful and customary for this type of fixes to provide here 
the environment you used for obtaining the warnings (clang version, 
configure parameters, and all needed elenents to repro the warnings).


https://clang-analyzer.llvm.org/

$ sudo dnf install clang-analyzer
$ ../configure
$ scan-build make



Regards,
Aleksandar

 > Philippe Mathieu-Daudé (11):
 >   block: Remove dead assignment
 >   blockdev: Remove dead assignment
 >   hw/i2c/pm_smbus: Remove dead assignment
 >   hw/input/adb-kbd: Remove dead assignment
 >   hw/ide/sii3112: Remove dead assignment
 >   hw/isa/i82378: Remove dead assignment
 >   hw/gpio/aspeed_gpio: Remove dead assignment
 >   hw/timer/exynos4210_mct: Remove dead assignments
 >   hw/timer/stm32f2xx_timer: Remove dead assignment
 >   hw/timer/pxa2xx_timer: Add assertion to silent static analyzer warning
 >   hw/scsi/esp-pci: Remove dead assignment
 >
 >  block.c                    | 2 +-
 >  blockdev.c                 | 2 +-
 >  hw/gpio/aspeed_gpio.c      | 4 ++--
 >  hw/i2c/pm_smbus.c          | 1 -
 >  hw/ide/sii3112.c           | 2 +-
 >  hw/input/adb-kbd.c         | 1 -
 >  hw/isa/i82378.c            | 8 
 >  hw/scsi/esp-pci.c          | 1 -
 >  hw/timer/exynos4210_mct.c  | 3 ---
 >  hw/timer/pxa2xx_timer.c    | 1 +
 >  hw/timer/stm32f2xx_timer.c | 1 -
 >  11 files changed, 10 insertions(+), 16 deletions(-)
 >
 > --
 > 2.21.1
 >
 >

Re: [PATCH-for-5.0] tools/virtiofsd/passthrough_ll: Fix double close()

[Philippe Mathieu-Daudé]

On 3/21/20 1:06 PM, Philippe Mathieu-Daudé wrote:

On success, the fdopendir() call closes fd. Later on the error
path we try to close an already-closed fd. This can lead to
use-after-free. Fix by only closing the fd if the fdopendir()
call failed.

Cc: qemu-sta...@nongnu.org
Fixes: 7c6b66027 (Import passthrough_ll from libfuse fuse-3.8.0)


libfuse is correct, the bug was introduced in commit b39bce121b, so:

Fixes: b39bce121b (add dirp_map to hide lo_dirp pointers)


Reported-by: Coverity (CID 1421933 USE_AFTER_FREE)
Suggested-by: Peter Maydell 
Signed-off-by: Philippe Mathieu-Daudé 
---
  tools/virtiofsd/passthrough_ll.c | 3 +--
  1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index 4f259aac70..4c35c95b25 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -1520,8 +1520,7 @@ out_err:
  if (d) {
  if (d->dp) {
  closedir(d->dp);
-}
-if (fd != -1) {
+} else if (fd != -1) {
  close(fd);
  }
  free(d);

[PATCH-for-5.0] tools/virtiofsd/passthrough_ll: Fix double close()

[Philippe Mathieu-Daudé]

On success, the fdopendir() call closes fd. Later on the error
path we try to close an already-closed fd. This can lead to
use-after-free. Fix by only closing the fd if the fdopendir()
call failed.

Cc: qemu-sta...@nongnu.org
Fixes: 7c6b66027 (Import passthrough_ll from libfuse fuse-3.8.0)
Reported-by: Coverity (CID 1421933 USE_AFTER_FREE)
Suggested-by: Peter Maydell 
Signed-off-by: Philippe Mathieu-Daudé 
---
 tools/virtiofsd/passthrough_ll.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index 4f259aac70..4c35c95b25 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -1520,8 +1520,7 @@ out_err:
 if (d) {
 if (d->dp) {
 closedir(d->dp);
-}
-if (fd != -1) {
+} else if (fd != -1) {
 close(fd);
 }
 free(d);
-- 
2.21.1

Re: [PATCH-for-5.0 00/11] misc: Trivial static code analyzer fixes

[Aleksandar Markovic]

12:47 PM Sub, 21.03.2020. Philippe Mathieu-Daudé  је
написао/ла:
>
> Fix trivial warnings reported by the Clang static code analyzer.
>

Philippe,

It would be useful and customary for this type of fixes to provide here the
environment you used for obtaining the warnings (clang version, configure
parameters, and all needed elenents to repro the warnings).

Regards,
Aleksandar

> Philippe Mathieu-Daudé (11):
>   block: Remove dead assignment
>   blockdev: Remove dead assignment
>   hw/i2c/pm_smbus: Remove dead assignment
>   hw/input/adb-kbd: Remove dead assignment
>   hw/ide/sii3112: Remove dead assignment
>   hw/isa/i82378: Remove dead assignment
>   hw/gpio/aspeed_gpio: Remove dead assignment
>   hw/timer/exynos4210_mct: Remove dead assignments
>   hw/timer/stm32f2xx_timer: Remove dead assignment
>   hw/timer/pxa2xx_timer: Add assertion to silent static analyzer warning
>   hw/scsi/esp-pci: Remove dead assignment
>
>  block.c| 2 +-
>  blockdev.c | 2 +-
>  hw/gpio/aspeed_gpio.c  | 4 ++--
>  hw/i2c/pm_smbus.c  | 1 -
>  hw/ide/sii3112.c   | 2 +-
>  hw/input/adb-kbd.c | 1 -
>  hw/isa/i82378.c| 8 
>  hw/scsi/esp-pci.c  | 1 -
>  hw/timer/exynos4210_mct.c  | 3 ---
>  hw/timer/pxa2xx_timer.c| 1 +
>  hw/timer/stm32f2xx_timer.c | 1 -
>  11 files changed, 10 insertions(+), 16 deletions(-)
>
> --
> 2.21.1
>
>

Re: [PATCH-for-5.0 01/11] block: Remove dead assignment

[Laurent Vivier]

Le 21/03/2020 à 12:46, Philippe Mathieu-Daudé a écrit :
> Fix warning reported by Clang static code analyzer:
> 
>   block.c:3167:5: warning: Value stored to 'ret' is never read
>   ret = bdrv_fill_options(, filename, , _err);
>   ^ ~
> 
> Reported-by: Clang Static Analyzer
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  block.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/block.c b/block.c
> index a2542c977b..908c109a8c 100644
> --- a/block.c
> +++ b/block.c
> @@ -3164,7 +3164,7 @@ static BlockDriverState *bdrv_open_inherit(const char 
> *filename,
>  parent->open_flags, parent->options);
>  }
>  
> -ret = bdrv_fill_options(, filename, , _err);
> +bdrv_fill_options(, filename, , _err);
>  if (local_err) {
>  goto fail;
>  }
> 

I would be sruprised if coverity doesn't warn about an unused return value.

Thanks,
Laurent

Re: [PATCH-for-5.0 01/11] block: Remove dead assignment

[Aleksandar Markovic]

12:49 PM Sub, 21.03.2020. Philippe Mathieu-Daudé  је
написао/ла:
>
> Fix warning reported by Clang static code analyzer:
>
>   block.c:3167:5: warning: Value stored to 'ret' is never read
>   ret = bdrv_fill_options(, filename, , _err);
>   ^ ~
>
> Reported-by: Clang Static Analyzer

Peter, and others,

Is this allowed use of "Reported-by:" mark?

I did not notice it being used this way before. And was under impression
that all similar tags/marks must be followed by a person, not a tool.

Regards,
Aleksandar

> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  block.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/block.c b/block.c
> index a2542c977b..908c109a8c 100644
> --- a/block.c
> +++ b/block.c
> @@ -3164,7 +3164,7 @@ static BlockDriverState *bdrv_open_inherit(const
char *filename,
>  parent->open_flags, parent->options);
>  }
>
> -ret = bdrv_fill_options(, filename, , _err);
> +bdrv_fill_options(, filename, , _err);
>  if (local_err) {
>  goto fail;
>  }
> --
> 2.21.1
>
>

[PATCH-for-5.0 10/11] hw/timer/pxa2xx_timer: Add assertion to silent static analyzer warning

[Philippe Mathieu-Daudé]

pxa2xx_timer_tick4() takes an opaque pointer, then calls
pxa2xx_timer_update4(), so the static analyzer can not
verify that the 'n < 8':

  425 static void pxa2xx_timer_tick4(void *opaque)
  426 {
  427 PXA2xxTimer4 *t = (PXA2xxTimer4 *) opaque;
  428 PXA2xxTimerInfo *i = (PXA2xxTimerInfo *) t->tm.info;
  429
  430 pxa2xx_timer_tick(>tm);
  433 if (t->control & (1 << 6))
  434 pxa2xx_timer_update4(i, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL), 
t->tm.num - 4);

  135 static void pxa2xx_timer_update4(void *opaque, uint64_t now_qemu, int n)
  136 {
  137 PXA2xxTimerInfo *s = (PXA2xxTimerInfo *) opaque;
  140 static const int counters[8] = { 0, 0, 0, 0, 4, 4, 6, 6 };
  142
  143 if (s->tm4[n].control & (1 << 7))
  144 counter = n;
  145 else
  146 counter = counters[n];

Add an assert() to give the static analyzer a hint, this fixes a
warning reported by Clang static code analyzer:

CC  hw/timer/pxa2xx_timer.o
  hw/timer/pxa2xx_timer.c:146:17: warning: Assigned value is garbage or 
undefined
  counter = counters[n];
  ^ ~~~

Reported-by: Clang Static Analyzer
Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/timer/pxa2xx_timer.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/timer/pxa2xx_timer.c b/hw/timer/pxa2xx_timer.c
index cd172cc1e9..944c165889 100644
--- a/hw/timer/pxa2xx_timer.c
+++ b/hw/timer/pxa2xx_timer.c
@@ -140,6 +140,7 @@ static void pxa2xx_timer_update4(void *opaque, uint64_t 
now_qemu, int n)
 static const int counters[8] = { 0, 0, 0, 0, 4, 4, 6, 6 };
 int counter;
 
+assert(n < ARRAY_SIZE(counters));
 if (s->tm4[n].control & (1 << 7))
 counter = n;
 else
-- 
2.21.1

Re: [PATCH-for-5.0 2/4] tests/docker: Install gcrypt devel package in Debian image

[Aleksandar Markovic]

8:06 PM Pet, 20.03.2020. Philippe Mathieu-Daudé  је
написао/ла:
>
> Apparently Debian Stretch was listing gcrypt as a QEMU dependency,
> but this is not the case anymore in Buster, so we need to install
> it manually (it it not listed by 'apt-get -s build-dep qemu' in
> the common debian10.docker anymore).
>
>  $ ../configure $QEMU_CONFIGURE_OPTS
>
>   ERROR: User requested feature gcrypt
>  configure was not able to find it.
>  Install gcrypt devel >= 1.5.0
>
> Fixes: 698a71edbed & 6f8bbb374be
> Signed-off-by: Philippe Mathieu-Daudé 
> ---

If the problem is caused by Debian Buster change in behavior, I think you
should not enumerate QEMU commits as something that is fixed by this patch.
That implies that something was wrong with these commits, while they were,
I suppose, fine at the moment of their integration.

Very confusing!

If you think that these commit ids deserve to be mentioned, you should say:
"Related commits are..." or similar.

Regards,
Aleksandar

>  tests/docker/dockerfiles/debian-amd64.docker | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/tests/docker/dockerfiles/debian-amd64.docker
b/tests/docker/dockerfiles/debian-amd64.docker
> index d4849f509f..957f0bc2e7 100644
> --- a/tests/docker/dockerfiles/debian-amd64.docker
> +++ b/tests/docker/dockerfiles/debian-amd64.docker
> @@ -16,6 +16,7 @@ RUN apt update && \
>  apt install -y --no-install-recommends \
>  libbz2-dev \
>  liblzo2-dev \
> +libgcrypt20-dev \
>  librdmacm-dev \
>  libsasl2-dev \
>  libsnappy-dev \
> --
> 2.21.1
>
>

[PATCH-for-5.0 08/11] hw/timer/exynos4210_mct: Remove dead assignments

[Philippe Mathieu-Daudé]

Fix warnings reported by Clang static code analyzer:

  hw/timer/exynos4210_mct.c:1370:9: warning: Value stored to 'index' is never 
read
index = GET_L_TIMER_CNT_REG_IDX(offset, lt_i);
^   ~
  hw/timer/exynos4210_mct.c:1399:9: warning: Value stored to 'index' is never 
read
index = GET_L_TIMER_CNT_REG_IDX(offset, lt_i);
^   ~
  hw/timer/exynos4210_mct.c:1441:9: warning: Value stored to 'index' is never 
read
index = GET_L_TIMER_CNT_REG_IDX(offset, lt_i);
^   ~

Reported-by: Clang Static Analyzer
Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/timer/exynos4210_mct.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/hw/timer/exynos4210_mct.c b/hw/timer/exynos4210_mct.c
index 944120aea5..c0a25e71ec 100644
--- a/hw/timer/exynos4210_mct.c
+++ b/hw/timer/exynos4210_mct.c
@@ -1367,7 +1367,6 @@ static void exynos4210_mct_write(void *opaque, hwaddr 
offset,
 
 case L0_TCNTB: case L1_TCNTB:
 lt_i = GET_L_TIMER_IDX(offset);
-index = GET_L_TIMER_CNT_REG_IDX(offset, lt_i);
 
 /*
  * TCNTB is updated to internal register only after CNT expired.
@@ -1396,7 +1395,6 @@ static void exynos4210_mct_write(void *opaque, hwaddr 
offset,
 
 case L0_ICNTB: case L1_ICNTB:
 lt_i = GET_L_TIMER_IDX(offset);
-index = GET_L_TIMER_CNT_REG_IDX(offset, lt_i);
 
 s->l_timer[lt_i].reg.wstat |= L_WSTAT_ICNTB_WRITE;
 s->l_timer[lt_i].reg.cnt[L_REG_CNT_ICNTB] = value &
@@ -1438,7 +1436,6 @@ static void exynos4210_mct_write(void *opaque, hwaddr 
offset,
 
 case L0_FRCNTB: case L1_FRCNTB:
 lt_i = GET_L_TIMER_IDX(offset);
-index = GET_L_TIMER_CNT_REG_IDX(offset, lt_i);
 
 DPRINTF("local timer[%d] FRCNTB write %llx\n", lt_i, value);
 
-- 
2.21.1

[PATCH-for-5.0 09/11] hw/timer/stm32f2xx_timer: Remove dead assignment

[Philippe Mathieu-Daudé]

Fix warning reported by Clang static code analyzer:

CC  hw/timer/stm32f2xx_timer.o
  hw/timer/stm32f2xx_timer.c:225:9: warning: Value stored to 'value' is never 
read
  value = timer_val;
  ^   ~

Reported-by: Clang Static Analyzer
Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/timer/stm32f2xx_timer.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/hw/timer/stm32f2xx_timer.c b/hw/timer/stm32f2xx_timer.c
index 06ec8a02c2..ba8694dcd3 100644
--- a/hw/timer/stm32f2xx_timer.c
+++ b/hw/timer/stm32f2xx_timer.c
@@ -222,7 +222,6 @@ static void stm32f2xx_timer_write(void *opaque, hwaddr 
offset,
 case TIM_PSC:
 timer_val = stm32f2xx_ns_to_ticks(s, now) - s->tick_offset;
 s->tim_psc = value & 0x;
-value = timer_val;
 break;
 case TIM_CNT:
 timer_val = value;
-- 
2.21.1

[PATCH-for-5.0 04/11] hw/input/adb-kbd: Remove dead assignment

[Philippe Mathieu-Daudé]

Fix warning reported by Clang static code analyzer:

CC  hw/input/adb-kbd.o
  hw/input/adb-kbd.c:200:5: warning: Value stored to 'olen' is never read
  olen = 0;
  ^  ~

Reported-by: Clang Static Analyzer
Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/input/adb-kbd.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/hw/input/adb-kbd.c b/hw/input/adb-kbd.c
index 0ba8207589..b0565be21b 100644
--- a/hw/input/adb-kbd.c
+++ b/hw/input/adb-kbd.c
@@ -197,7 +197,6 @@ static int adb_kbd_poll(ADBDevice *d, uint8_t *obuf)
 int keycode;
 int olen;
 
-olen = 0;
 if (s->count == 0) {
 return 0;
 }
-- 
2.21.1

[PATCH-for-5.0 07/11] hw/gpio/aspeed_gpio: Remove dead assignment

[Philippe Mathieu-Daudé]

Fix warning reported by Clang static code analyzer:

  hw/gpio/aspeed_gpio.c:717:18: warning: Value stored to 'g_idx' during its 
initialization is never read
  int set_idx, g_idx = *group_idx;
   ^   ~~

Reported-by: Clang Static Analyzer
Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/gpio/aspeed_gpio.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/gpio/aspeed_gpio.c b/hw/gpio/aspeed_gpio.c
index 41e11ea9b0..cc07ab9866 100644
--- a/hw/gpio/aspeed_gpio.c
+++ b/hw/gpio/aspeed_gpio.c
@@ -714,11 +714,11 @@ static void aspeed_gpio_write(void *opaque, hwaddr 
offset, uint64_t data,
 static int get_set_idx(AspeedGPIOState *s, const char *group, int *group_idx)
 {
 AspeedGPIOClass *agc = ASPEED_GPIO_GET_CLASS(s);
-int set_idx, g_idx = *group_idx;
+int set_idx;
 
 for (set_idx = 0; set_idx < agc->nr_gpio_sets; set_idx++) {
 const GPIOSetProperties *set_props = >props[set_idx];
-for (g_idx = 0; g_idx < ASPEED_GROUPS_PER_SET; g_idx++) {
+for (int g_idx = 0; g_idx < ASPEED_GROUPS_PER_SET; g_idx++) {
 if (!strncmp(group, set_props->group_label[g_idx], strlen(group))) 
{
 *group_idx = g_idx;
 return set_idx;
-- 
2.21.1

[PATCH-for-5.0 06/11] hw/isa/i82378: Remove dead assignment

[Philippe Mathieu-Daudé]

Rename the unique variable assigned as 'pit' which better
represents what it holds, to fix a warning reported by the
Clang static code analyzer:

CC  hw/isa/i82378.o
  hw/isa/i82378.c:108:5: warning: Value stored to 'isa' is never read
  isa = isa_create_simple(isabus, "i82374");
  ^ ~~~

Reported-by: Clang Static Analyzer
Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/isa/i82378.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/isa/i82378.c b/hw/isa/i82378.c
index dcb6b479ea..d9e6c7fa00 100644
--- a/hw/isa/i82378.c
+++ b/hw/isa/i82378.c
@@ -67,7 +67,7 @@ static void i82378_realize(PCIDevice *pci, Error **errp)
 I82378State *s = I82378(dev);
 uint8_t *pci_conf;
 ISABus *isabus;
-ISADevice *isa;
+ISADevice *pit;
 
 pci_conf = pci->config;
 pci_set_word(pci_conf + PCI_COMMAND,
@@ -99,13 +99,13 @@ static void i82378_realize(PCIDevice *pci, Error **errp)
 isa_bus_irqs(isabus, s->i8259);
 
 /* 1 82C54 (pit) */
-isa = i8254_pit_init(isabus, 0x40, 0, NULL);
+pit = i8254_pit_init(isabus, 0x40, 0, NULL);
 
 /* speaker */
-pcspk_init(isabus, isa);
+pcspk_init(isabus, pit);
 
 /* 2 82C37 (dma) */
-isa = isa_create_simple(isabus, "i82374");
+isa_create_simple(isabus, "i82374");
 }
 
 static void i82378_init(Object *obj)
-- 
2.21.1

[PATCH-for-5.0 11/11] hw/scsi/esp-pci: Remove dead assignment

[Philippe Mathieu-Daudé]

Fix warning reported by Clang static code analyzer:

CC  hw/scsi/esp-pci.o
  hw/scsi/esp-pci.c:198:9: warning: Value stored to 'size' is never read
  size = 4;
  ^  ~

Reported-by: Clang Static Analyzer
Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/scsi/esp-pci.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/hw/scsi/esp-pci.c b/hw/scsi/esp-pci.c
index d5a1f9e017..2e6cc07d4e 100644
--- a/hw/scsi/esp-pci.c
+++ b/hw/scsi/esp-pci.c
@@ -195,7 +195,6 @@ static void esp_pci_io_write(void *opaque, hwaddr addr,
 val <<= shift;
 val |= current & ~(mask << shift);
 addr &= ~3;
-size = 4;
 }
 
 if (addr < 0x40) {
-- 
2.21.1

[PATCH-for-5.0 01/11] block: Remove dead assignment

[Philippe Mathieu-Daudé]

Fix warning reported by Clang static code analyzer:

  block.c:3167:5: warning: Value stored to 'ret' is never read
  ret = bdrv_fill_options(, filename, , _err);
  ^ ~

Reported-by: Clang Static Analyzer
Signed-off-by: Philippe Mathieu-Daudé 
---
 block.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block.c b/block.c
index a2542c977b..908c109a8c 100644
--- a/block.c
+++ b/block.c
@@ -3164,7 +3164,7 @@ static BlockDriverState *bdrv_open_inherit(const char 
*filename,
 parent->open_flags, parent->options);
 }
 
-ret = bdrv_fill_options(, filename, , _err);
+bdrv_fill_options(, filename, , _err);
 if (local_err) {
 goto fail;
 }
-- 
2.21.1

[PATCH-for-5.0 03/11] hw/i2c/pm_smbus: Remove dead assignment

[Philippe Mathieu-Daudé]

Fix warning reported by Clang static code analyzer:

CC  hw/i2c/pm_smbus.o
  hw/i2c/pm_smbus.c:187:17: warning: Value stored to 'ret' is never read
  ret = 0;
  ^ ~

Reported-by: Clang Static Analyzer
Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/i2c/pm_smbus.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/hw/i2c/pm_smbus.c b/hw/i2c/pm_smbus.c
index 36994ff585..4728540c37 100644
--- a/hw/i2c/pm_smbus.c
+++ b/hw/i2c/pm_smbus.c
@@ -184,7 +184,6 @@ static void smb_transaction(PMSMBus *s)
 s->smb_stat |= STS_HOST_BUSY | STS_BYTE_DONE;
 s->smb_data[0] = s->smb_blkdata;
 s->smb_index = 0;
-ret = 0;
 }
 goto out;
 }
-- 
2.21.1

[PATCH-for-5.0 05/11] hw/ide/sii3112: Remove dead assignment

[Philippe Mathieu-Daudé]

Fix warning reported by Clang static code analyzer:

CC  hw/ide/sii3112.o
  hw/ide/sii3112.c:204:9: warning: Value stored to 'val' is never read
  val = 0;
  ^ ~

Reported-by: Clang Static Analyzer
Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/ide/sii3112.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/ide/sii3112.c b/hw/ide/sii3112.c
index 06605d7af2..36f1905ddb 100644
--- a/hw/ide/sii3112.c
+++ b/hw/ide/sii3112.c
@@ -125,7 +125,7 @@ static uint64_t sii3112_reg_read(void *opaque, hwaddr addr,
 val = (uint32_t)d->regs[1].sien << 16;
 break;
 default:
-val = 0;
+break;
 }
 trace_sii3112_read(size, addr, val);
 return val;
-- 
2.21.1

[PATCH-for-5.0 02/11] blockdev: Remove dead assignment

[Philippe Mathieu-Daudé]

Fix warning reported by Clang static code analyzer:

CC  blockdev.o
  blockdev.c:2744:5: warning: Value stored to 'ret' is never read
  ret = blk_truncate(blk, size, false, PREALLOC_MODE_OFF, errp);
  ^ ~~~

Reported-by: Clang Static Analyzer
Signed-off-by: Philippe Mathieu-Daudé 
---
 blockdev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/blockdev.c b/blockdev.c
index fa8630cb41..6effd5afaa 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2741,7 +2741,7 @@ void qmp_block_resize(bool has_device, const char *device,
 }
 
 bdrv_drained_begin(bs);
-ret = blk_truncate(blk, size, false, PREALLOC_MODE_OFF, errp);
+blk_truncate(blk, size, false, PREALLOC_MODE_OFF, errp);
 bdrv_drained_end(bs);
 
 out:
-- 
2.21.1

[PATCH-for-5.0 00/11] misc: Trivial static code analyzer fixes

[Philippe Mathieu-Daudé]

Fix trivial warnings reported by the Clang static code analyzer.

Philippe Mathieu-Daudé (11):
  block: Remove dead assignment
  blockdev: Remove dead assignment
  hw/i2c/pm_smbus: Remove dead assignment
  hw/input/adb-kbd: Remove dead assignment
  hw/ide/sii3112: Remove dead assignment
  hw/isa/i82378: Remove dead assignment
  hw/gpio/aspeed_gpio: Remove dead assignment
  hw/timer/exynos4210_mct: Remove dead assignments
  hw/timer/stm32f2xx_timer: Remove dead assignment
  hw/timer/pxa2xx_timer: Add assertion to silent static analyzer warning
  hw/scsi/esp-pci: Remove dead assignment

 block.c| 2 +-
 blockdev.c | 2 +-
 hw/gpio/aspeed_gpio.c  | 4 ++--
 hw/i2c/pm_smbus.c  | 1 -
 hw/ide/sii3112.c   | 2 +-
 hw/input/adb-kbd.c | 1 -
 hw/isa/i82378.c| 8 
 hw/scsi/esp-pci.c  | 1 -
 hw/timer/exynos4210_mct.c  | 3 ---
 hw/timer/pxa2xx_timer.c| 1 +
 hw/timer/stm32f2xx_timer.c | 1 -
 11 files changed, 10 insertions(+), 16 deletions(-)

-- 
2.21.1

Re: [PATCH] monitor/hmp-cmds: fix bad indentation in 'info migrate_parameters' cmd output

[maozy]

On 3/21/20 3:14 PM, Markus Armbruster wrote:

"Dr. David Alan Gilbert"  writes:


* Daniel P. Berrangé (berra...@redhat.com) wrote:

On Fri, Mar 20, 2020 at 05:31:17PM +, Dr. David Alan Gilbert wrote:

(Rearranging the text a bit)

* Markus Armbruster (arm...@redhat.com) wrote:


David (cc'ed) should be able to tell us which fix is right.

@tls_creds and @tls_hostname look like they could have the same issue.

A certain Markus removed the Null checks in 8cc99dc because 4af245d
guaranteed they would be None-Null for tls-creds/hostname - so we
should be OK for those.

But tls-authz came along a lot later in d2f1d29 and doesn't
seem to have the initialisation, which is now in
migration_instance_init.

So I *think* the fix for this is to do the modern equivalent of 4af245d
:

diff --git a/migration/migration.c b/migration/migration.c
index c1d88ace7f..0bc1b93277 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -3686,6 +3686,7 @@ static void migration_instance_init(Object *obj)
  
  params->tls_hostname = g_strdup("");

  params->tls_creds = g_strdup("");
+params->tls_authz = g_strdup("");
  
  /* Set has_* up only for parameter checks */

  params->has_compress_level = true;

Copying in Dan to check that wouldn't break tls.

It *will* break TLS, because it will cause the TLS code to lookup
an object with the ID of "".  NULL must be preserved when calling
the TLS APIs.

OK, good I asked...


The assignment of "" to tls_hostname would also have broken TLS,
so the migration_tls_channel_connect method had to turn it back
into a real NULL.

The use of "" for tls_creds will similarly cause it to try and
lookup an object with ID of "", and fail. That one's harmless
though, because it would also fail if it were NULL.

OK.

It looks like the output of query-migrate-parameters though already
turns it into "", so I don't think you can tell it's NULL from that:

{"QMP": {"version": {"qemu": {"micro": 0, "minor": 2, "major": 4}, "package": "qemu-4.2.0-4.fc31"}, 
"capabilities": ["oob"]}}
{ "execute": "qmp_capabilities" }
{"return": {}}
{ "execute": "query-migrate-parameters" }
{"return": {"xbzrle-cache-size": 67108864, "cpu-throttle-initial": 20, "announce-max": 550, "decompress-threads": 2, "compress-threads": 8, "compress-level": 1, "multifd-channels": 2, "announce-initial": 50, "block-incremental": 
false, "compress-wait-thread": true, "downtime-limit": 300, "tls-authz": "", "announce-rounds": 5, "announce-step": 100, "tls-creds": "", "max-cpu-throttle": 99, "max-postcopy-bandwidth": 0, "tls-hostname": 
"", "max-bandwidth": 33554432, "x-checkpoint-delay": 2, "cpu-throttle-increment": 10}}

I'm not sure who turns a Null into a "" but I guess it's somewhere in
the json output iterator.

It's this wart:

 static void qobject_output_type_str(Visitor *v, const char *name, char 
**obj,
 Error **errp)
 {
 QObjectOutputVisitor *qov = to_qov(v);
 if (*obj) {
 qobject_output_add(qov, name, qstring_from_str(*obj));
 } else {
 qobject_output_add(qov, name, qstring_from_str(""));
 }
 }

Warty since day 1.

In theory, !*obj should not happen, since QAPI type 'str' is not
nullable.

In practice, it handwritten code can easily make it happen.


So we can fix this problem either in qmp_query_migrate_parameters
and just strdup a "", or substitute it in hmp_info_migrate_parameters.

Fixing it in qmp_query_migrate_parameters() is cleaner: it avoids null
'str', and bypasses the wart.


OK,  thanks for the kind reply, will fix it in v2.

Thanks,
Mao

Re: [PATCH] target/mips: Fix loongson multimedia condition instructions

[Jiaxun Yang]

于 2020年3月21日 GMT+08:00 下午6:57:54, Jiaxun Yang  写到:
>
>
>于 2020年3月21日 GMT+08:00 下午6:39:21, "Philippe Mathieu-Daudé"
> 写到:
>>On 3/21/20 5:56 AM, Jiaxun Yang wrote:
>>> Loongson multimedia condition instructions were previously
>>implemented as
>>> write 0 to rd due to lack of documentation. So I just confirmed with
>>Loongson
>>> about their encoding and implemented them correctly.
>>
>>Can you refer to the datasheet in the commit message, or have someone 
>>from Loongson Technology, Lemote Tech or with access to the specs ack 
>>your patch?
>
>I just confirmed with Loongson guys on IM.
>
>+ Huacai

+Huacai's Gmail as his Lemote mail rejected my bounce.

>
>Hi Huacai,
>
>Could you please acknowledge this change,
>Thanks.
>
>--
>Jiaxun Yang
>
>>
>>> 
>>> Signed-off-by: Jiaxun Yang 
>>> ---
>>>   target/mips/translate.c | 40
>>++--
>>>   1 file changed, 34 insertions(+), 6 deletions(-)
>>> 
>>> diff --git a/target/mips/translate.c b/target/mips/translate.c
>>> index d745bd2803..43be8d27b5 100644
>>> --- a/target/mips/translate.c
>>> +++ b/target/mips/translate.c
>>> @@ -5529,6 +5529,8 @@ static void
>>gen_loongson_multimedia(DisasContext *ctx, int rd, int rs, int rt)
>>>   {
>>>   uint32_t opc, shift_max;
>>>   TCGv_i64 t0, t1;
>>> +TCGCond cond;
>>> +TCGLabel *lab;
>>>   
>>>   opc = MASK_LMI(ctx->opcode);
>>>   switch (opc) {
>>> @@ -5816,7 +5818,7 @@ static void
>>gen_loongson_multimedia(DisasContext *ctx, int rd, int rs, int rt)
>>>   case OPC_DADD_CP2:
>>>   {
>>>   TCGv_i64 t2 = tcg_temp_new_i64();
>>> -TCGLabel *lab = gen_new_label();
>>> +lab = gen_new_label();
>>>   
>>>   tcg_gen_mov_i64(t2, t0);
>>>   tcg_gen_add_i64(t0, t1, t2);
>>> @@ -5837,7 +5839,7 @@ static void
>>gen_loongson_multimedia(DisasContext *ctx, int rd, int rs, int rt)
>>>   case OPC_DSUB_CP2:
>>>   {
>>>   TCGv_i64 t2 = tcg_temp_new_i64();
>>> -TCGLabel *lab = gen_new_label();
>>> +lab = gen_new_label();
>>>   
>>>   tcg_gen_mov_i64(t2, t0);
>>>   tcg_gen_sub_i64(t0, t1, t2);
>>> @@ -5862,14 +5864,39 @@ static void
>>gen_loongson_multimedia(DisasContext *ctx, int rd, int rs, int rt)
>>>   
>>>   case OPC_SEQU_CP2:
>>>   case OPC_SEQ_CP2:
>>> +cond = TCG_COND_EQ;
>>> +goto do_cc_cond;
>>> +break;
>>> +
>>>   case OPC_SLTU_CP2:
>>> +cond = TCG_COND_LTU;
>>> +goto do_cc_cond;
>>> +break;
>>> +
>>>   case OPC_SLT_CP2:
>>> +cond = TCG_COND_LT;
>>> +goto do_cc_cond;
>>> +break;
>>> +
>>>   case OPC_SLEU_CP2:
>>> +cond = TCG_COND_LEU;
>>> +goto do_cc_cond;
>>> +break;
>>> +
>>>   case OPC_SLE_CP2:
>>> -/*
>>> - * ??? Document is unclear: Set FCC[CC].  Does that mean
>the
>>> - * FD field is the CC field?
>>> - */
>>> +cond = TCG_COND_LE;
>>> +do_cc_cond:
>>> +{
>>> +int cc = (ctx->opcode >> 8) & 0x7;
>>> +lab = gen_new_label();
>>> +tcg_gen_ori_i32(fpu_fcr31, fpu_fcr31, 1 <<
>>get_fp_bit(cc));
>>> +tcg_gen_brcond_i64(cond, t0, t1, lab);
>>> +tcg_gen_xori_i32(fpu_fcr31, fpu_fcr31, 1 <<
>>get_fp_bit(cc));
>>> +gen_set_label(lab);
>>> +}
>>> +goto no_rd;
>>> +break;
>>> +
>>>   default:
>>>   MIPS_INVAL("loongson_cp2");
>>>   generate_exception_end(ctx, EXCP_RI);
>>> @@ -5878,6 +5905,7 @@ static void
>>gen_loongson_multimedia(DisasContext *ctx, int rd, int rs, int rt)
>>>   
>>>   gen_store_fpr64(ctx, t0, rd);
>>>   
>>> +no_rd:
>>>   tcg_temp_free_i64(t0);
>>>   tcg_temp_free_i64(t1);
>>>   }
>>> 

-- 
Jiaxun Yang

Re: [PULL v2 05/13] target/rx: CPU definitions

[Philippe Mathieu-Daudé]

On 3/21/20 12:05 PM, Philippe Mathieu-Daudé wrote:

On 3/20/20 5:37 PM, Peter Maydell wrote:
On Fri, 20 Mar 2020 at 16:32, Philippe Mathieu-Daudé 
 wrote:

-fwrapv is here indeed.

I use
--extra-cflags=-fsanitize=address,alignment,array-bounds,bool,builtin,enum,float-cast-overflow,float-divide-by-zero,function,integer-divide-by-zero,nonnull-attribute,null,pointer-overflow,return,returns-nonnull-attribute,shift-base,shift-exponent,signed-integer-overflow,unreachable,vla-bound,vptr 



There was a bug in older clang versions where the shift-base
sanitizer didn't honour -fwrapv:
https://bugs.llvm.org/show_bug.cgi?id=25552

https://wiki.qemu.org/Testing#clang_UBSan
says you can work around the clang bug with -fno-sanitize=shift-base.

The bug was fixed upstream back in 2016, though, so the
fix ought to be in clang 4, I think. Are you using an
old clang version, or has it regressed in newer clang?


I am sorry I am very confused here.

$ clang -v
clang version 8.0.0 (Fedora 8.0.0-3.fc30)

I use scan-build which set CC=ccc-analyzer.

$ ccc-analyzer -v
gcc version 9.2.1 20190827 (Red Hat 9.2.1-1) (GCC)

I use the output of 'make V=1' and run directly ccc-analyzer, I get:

gcc: error: unrecognized argument to ‘-fsanitize=’ option: ‘array-bounds’
gcc: error: unrecognized argument to ‘-fsanitize=’ option: ‘function’

So Clang is used.

I notice ccc-analyzer appends -Wno-shift-negative-value, however if I 
run the 'make V=1' output using clang instead (with all the -fsanitize 
options, -fwrapv, -Wno-shift-negative-value) then no warning are emitted.


So I think this is simply a problem with scan-build/ccc-analyzer, or I 
should tune it more for QEMU.


Eh I simply needed to RTFM more attentively:

 --keep-cc

   Do not override CC and CXX make variables. Useful when
   running make in autoconf-based (and similar) projects
   where configure can add extra flags to those variables.

Re: [PULL v2 05/13] target/rx: CPU definitions

[Philippe Mathieu-Daudé]

On 3/20/20 5:37 PM, Peter Maydell wrote:

On Fri, 20 Mar 2020 at 16:32, Philippe Mathieu-Daudé  wrote:

-fwrapv is here indeed.

I use
--extra-cflags=-fsanitize=address,alignment,array-bounds,bool,builtin,enum,float-cast-overflow,float-divide-by-zero,function,integer-divide-by-zero,nonnull-attribute,null,pointer-overflow,return,returns-nonnull-attribute,shift-base,shift-exponent,signed-integer-overflow,unreachable,vla-bound,vptr


There was a bug in older clang versions where the shift-base
sanitizer didn't honour -fwrapv:
https://bugs.llvm.org/show_bug.cgi?id=25552

https://wiki.qemu.org/Testing#clang_UBSan
says you can work around the clang bug with -fno-sanitize=shift-base.

The bug was fixed upstream back in 2016, though, so the
fix ought to be in clang 4, I think. Are you using an
old clang version, or has it regressed in newer clang?


I am sorry I am very confused here.

$ clang -v
clang version 8.0.0 (Fedora 8.0.0-3.fc30)

I use scan-build which set CC=ccc-analyzer.

$ ccc-analyzer -v
gcc version 9.2.1 20190827 (Red Hat 9.2.1-1) (GCC)

I use the output of 'make V=1' and run directly ccc-analyzer, I get:

gcc: error: unrecognized argument to ‘-fsanitize=’ option: ‘array-bounds’
gcc: error: unrecognized argument to ‘-fsanitize=’ option: ‘function’

So Clang is used.

I notice ccc-analyzer appends -Wno-shift-negative-value, however if I 
run the 'make V=1' output using clang instead (with all the -fsanitize 
options, -fwrapv, -Wno-shift-negative-value) then no warning are emitted.


So I think this is simply a problem with scan-build/ccc-analyzer, or I 
should tune it more for QEMU.

Re: [PATCH] target/mips: Fix loongson multimedia condition instructions

[Jiaxun Yang]

于 2020年3月21日 GMT+08:00 下午6:39:21, "Philippe Mathieu-Daudé"  
写到:
>On 3/21/20 5:56 AM, Jiaxun Yang wrote:
>> Loongson multimedia condition instructions were previously
>implemented as
>> write 0 to rd due to lack of documentation. So I just confirmed with
>Loongson
>> about their encoding and implemented them correctly.
>
>Can you refer to the datasheet in the commit message, or have someone 
>from Loongson Technology, Lemote Tech or with access to the specs ack 
>your patch?

I just confirmed with Loongson guys on IM.

+ Huacai

Hi Huacai,

Could you please acknowledge this change,
Thanks.

--
Jiaxun Yang

>
>> 
>> Signed-off-by: Jiaxun Yang 
>> ---
>>   target/mips/translate.c | 40
>++--
>>   1 file changed, 34 insertions(+), 6 deletions(-)
>> 
>> diff --git a/target/mips/translate.c b/target/mips/translate.c
>> index d745bd2803..43be8d27b5 100644
>> --- a/target/mips/translate.c
>> +++ b/target/mips/translate.c
>> @@ -5529,6 +5529,8 @@ static void
>gen_loongson_multimedia(DisasContext *ctx, int rd, int rs, int rt)
>>   {
>>   uint32_t opc, shift_max;
>>   TCGv_i64 t0, t1;
>> +TCGCond cond;
>> +TCGLabel *lab;
>>   
>>   opc = MASK_LMI(ctx->opcode);
>>   switch (opc) {
>> @@ -5816,7 +5818,7 @@ static void
>gen_loongson_multimedia(DisasContext *ctx, int rd, int rs, int rt)
>>   case OPC_DADD_CP2:
>>   {
>>   TCGv_i64 t2 = tcg_temp_new_i64();
>> -TCGLabel *lab = gen_new_label();
>> +lab = gen_new_label();
>>   
>>   tcg_gen_mov_i64(t2, t0);
>>   tcg_gen_add_i64(t0, t1, t2);
>> @@ -5837,7 +5839,7 @@ static void
>gen_loongson_multimedia(DisasContext *ctx, int rd, int rs, int rt)
>>   case OPC_DSUB_CP2:
>>   {
>>   TCGv_i64 t2 = tcg_temp_new_i64();
>> -TCGLabel *lab = gen_new_label();
>> +lab = gen_new_label();
>>   
>>   tcg_gen_mov_i64(t2, t0);
>>   tcg_gen_sub_i64(t0, t1, t2);
>> @@ -5862,14 +5864,39 @@ static void
>gen_loongson_multimedia(DisasContext *ctx, int rd, int rs, int rt)
>>   
>>   case OPC_SEQU_CP2:
>>   case OPC_SEQ_CP2:
>> +cond = TCG_COND_EQ;
>> +goto do_cc_cond;
>> +break;
>> +
>>   case OPC_SLTU_CP2:
>> +cond = TCG_COND_LTU;
>> +goto do_cc_cond;
>> +break;
>> +
>>   case OPC_SLT_CP2:
>> +cond = TCG_COND_LT;
>> +goto do_cc_cond;
>> +break;
>> +
>>   case OPC_SLEU_CP2:
>> +cond = TCG_COND_LEU;
>> +goto do_cc_cond;
>> +break;
>> +
>>   case OPC_SLE_CP2:
>> -/*
>> - * ??? Document is unclear: Set FCC[CC].  Does that mean the
>> - * FD field is the CC field?
>> - */
>> +cond = TCG_COND_LE;
>> +do_cc_cond:
>> +{
>> +int cc = (ctx->opcode >> 8) & 0x7;
>> +lab = gen_new_label();
>> +tcg_gen_ori_i32(fpu_fcr31, fpu_fcr31, 1 <<
>get_fp_bit(cc));
>> +tcg_gen_brcond_i64(cond, t0, t1, lab);
>> +tcg_gen_xori_i32(fpu_fcr31, fpu_fcr31, 1 <<
>get_fp_bit(cc));
>> +gen_set_label(lab);
>> +}
>> +goto no_rd;
>> +break;
>> +
>>   default:
>>   MIPS_INVAL("loongson_cp2");
>>   generate_exception_end(ctx, EXCP_RI);
>> @@ -5878,6 +5905,7 @@ static void
>gen_loongson_multimedia(DisasContext *ctx, int rd, int rs, int rt)
>>   
>>   gen_store_fpr64(ctx, t0, rd);
>>   
>> +no_rd:
>>   tcg_temp_free_i64(t0);
>>   tcg_temp_free_i64(t1);
>>   }
>> 

-- 
Jiaxun Yang

Re: [PATCH] target/mips: Fix loongson multimedia condition instructions

[Philippe Mathieu-Daudé]

On 3/21/20 5:56 AM, Jiaxun Yang wrote:

Loongson multimedia condition instructions were previously implemented as
write 0 to rd due to lack of documentation. So I just confirmed with Loongson
about their encoding and implemented them correctly.


Can you refer to the datasheet in the commit message, or have someone 
from Loongson Technology, Lemote Tech or with access to the specs ack 
your patch?




Signed-off-by: Jiaxun Yang 
---
  target/mips/translate.c | 40 ++--
  1 file changed, 34 insertions(+), 6 deletions(-)

diff --git a/target/mips/translate.c b/target/mips/translate.c
index d745bd2803..43be8d27b5 100644
--- a/target/mips/translate.c
+++ b/target/mips/translate.c
@@ -5529,6 +5529,8 @@ static void gen_loongson_multimedia(DisasContext *ctx, 
int rd, int rs, int rt)
  {
  uint32_t opc, shift_max;
  TCGv_i64 t0, t1;
+TCGCond cond;
+TCGLabel *lab;
  
  opc = MASK_LMI(ctx->opcode);

  switch (opc) {
@@ -5816,7 +5818,7 @@ static void gen_loongson_multimedia(DisasContext *ctx, 
int rd, int rs, int rt)
  case OPC_DADD_CP2:
  {
  TCGv_i64 t2 = tcg_temp_new_i64();
-TCGLabel *lab = gen_new_label();
+lab = gen_new_label();
  
  tcg_gen_mov_i64(t2, t0);

  tcg_gen_add_i64(t0, t1, t2);
@@ -5837,7 +5839,7 @@ static void gen_loongson_multimedia(DisasContext *ctx, 
int rd, int rs, int rt)
  case OPC_DSUB_CP2:
  {
  TCGv_i64 t2 = tcg_temp_new_i64();
-TCGLabel *lab = gen_new_label();
+lab = gen_new_label();
  
  tcg_gen_mov_i64(t2, t0);

  tcg_gen_sub_i64(t0, t1, t2);
@@ -5862,14 +5864,39 @@ static void gen_loongson_multimedia(DisasContext *ctx, 
int rd, int rs, int rt)
  
  case OPC_SEQU_CP2:

  case OPC_SEQ_CP2:
+cond = TCG_COND_EQ;
+goto do_cc_cond;
+break;
+
  case OPC_SLTU_CP2:
+cond = TCG_COND_LTU;
+goto do_cc_cond;
+break;
+
  case OPC_SLT_CP2:
+cond = TCG_COND_LT;
+goto do_cc_cond;
+break;
+
  case OPC_SLEU_CP2:
+cond = TCG_COND_LEU;
+goto do_cc_cond;
+break;
+
  case OPC_SLE_CP2:
-/*
- * ??? Document is unclear: Set FCC[CC].  Does that mean the
- * FD field is the CC field?
- */
+cond = TCG_COND_LE;
+do_cc_cond:
+{
+int cc = (ctx->opcode >> 8) & 0x7;
+lab = gen_new_label();
+tcg_gen_ori_i32(fpu_fcr31, fpu_fcr31, 1 << get_fp_bit(cc));
+tcg_gen_brcond_i64(cond, t0, t1, lab);
+tcg_gen_xori_i32(fpu_fcr31, fpu_fcr31, 1 << get_fp_bit(cc));
+gen_set_label(lab);
+}
+goto no_rd;
+break;
+
  default:
  MIPS_INVAL("loongson_cp2");
  generate_exception_end(ctx, EXCP_RI);
@@ -5878,6 +5905,7 @@ static void gen_loongson_multimedia(DisasContext *ctx, 
int rd, int rs, int rt)
  
  gen_store_fpr64(ctx, t0, rd);
  
+no_rd:

  tcg_temp_free_i64(t0);
  tcg_temp_free_i64(t1);
  }

Re: Qemu on Windows 10 - no acceleration found

[Jerry Geis]

Perfect Stefan - that did the trick.
Thank you.

Jerry

On Sat, Mar 21, 2020 at 3:14 AM Stefan Weil  wrote:

> Am 20.03.20 um 21:22 schrieb Jerry Geis:
>
> > So I tried --enable-whpx and I get Invalid option. Im on Windows 10
> > and QEMU 4.2.0
> >
> > I'm confused.  Then I don't know where to download the HAXM. The place
> > I found is GIT and it wants the user to compile it. I was looking for
> > just an EXE.
> >
> > Thanks
> >
> > Jerry
>
>
> Sorry, the FAQ was outdated. I updated it now.
>
> Run `qemu-system-x86_64 --accel whpx`.
>
> Using WHPX is easier than using HAX.
>
> Stefan
>
>

Re: [PATCH] target/mips: Fix loongson multimedia condition instructions

[Aleksandar Markovic]

5:58 AM Sub, 21.03.2020. Jiaxun Yang  је
написао/ла:
>
> Loongson multimedia condition instructions were previously implemented as
> write 0 to rd due to lack of documentation. So I just confirmed with
Loongson
> about their encoding and implemented them correctly.
>
> Signed-off-by: Jiaxun Yang 
> ---

Thanks, Jiaxun!

I feel relieved that this "mistery" is about to be solved. This patch
actually deals with a long-standing known bug, and, on that basis, it is
eligible to be integrated into QEMU 5.0 after soft-freeze.

I'll take a closer look at the details and their possible improvements in
next few days. But, again, in general, I salute this patch.

Yours,
Aleksansdar

>  target/mips/translate.c | 40 ++--
>  1 file changed, 34 insertions(+), 6 deletions(-)
>
> diff --git a/target/mips/translate.c b/target/mips/translate.c
> index d745bd2803..43be8d27b5 100644
> --- a/target/mips/translate.c
> +++ b/target/mips/translate.c
> @@ -5529,6 +5529,8 @@ static void gen_loongson_multimedia(DisasContext
*ctx, int rd, int rs, int rt)
>  {
>  uint32_t opc, shift_max;
>  TCGv_i64 t0, t1;
> +TCGCond cond;
> +TCGLabel *lab;
>
>  opc = MASK_LMI(ctx->opcode);
>  switch (opc) {
> @@ -5816,7 +5818,7 @@ static void gen_loongson_multimedia(DisasContext
*ctx, int rd, int rs, int rt)
>  case OPC_DADD_CP2:
>  {
>  TCGv_i64 t2 = tcg_temp_new_i64();
> -TCGLabel *lab = gen_new_label();
> +lab = gen_new_label();
>
>  tcg_gen_mov_i64(t2, t0);
>  tcg_gen_add_i64(t0, t1, t2);
> @@ -5837,7 +5839,7 @@ static void gen_loongson_multimedia(DisasContext
*ctx, int rd, int rs, int rt)
>  case OPC_DSUB_CP2:
>  {
>  TCGv_i64 t2 = tcg_temp_new_i64();
> -TCGLabel *lab = gen_new_label();
> +lab = gen_new_label();
>
>  tcg_gen_mov_i64(t2, t0);
>  tcg_gen_sub_i64(t0, t1, t2);
> @@ -5862,14 +5864,39 @@ static void gen_loongson_multimedia(DisasContext
*ctx, int rd, int rs, int rt)
>
>  case OPC_SEQU_CP2:
>  case OPC_SEQ_CP2:
> +cond = TCG_COND_EQ;
> +goto do_cc_cond;
> +break;
> +
>  case OPC_SLTU_CP2:
> +cond = TCG_COND_LTU;
> +goto do_cc_cond;
> +break;
> +
>  case OPC_SLT_CP2:
> +cond = TCG_COND_LT;
> +goto do_cc_cond;
> +break;
> +
>  case OPC_SLEU_CP2:
> +cond = TCG_COND_LEU;
> +goto do_cc_cond;
> +break;
> +
>  case OPC_SLE_CP2:
> -/*
> - * ??? Document is unclear: Set FCC[CC].  Does that mean the
> - * FD field is the CC field?
> - */
> +cond = TCG_COND_LE;
> +do_cc_cond:
> +{
> +int cc = (ctx->opcode >> 8) & 0x7;
> +lab = gen_new_label();
> +tcg_gen_ori_i32(fpu_fcr31, fpu_fcr31, 1 << get_fp_bit(cc));
> +tcg_gen_brcond_i64(cond, t0, t1, lab);
> +tcg_gen_xori_i32(fpu_fcr31, fpu_fcr31, 1 << get_fp_bit(cc));
> +gen_set_label(lab);
> +}
> +goto no_rd;
> +break;
> +
>  default:
>  MIPS_INVAL("loongson_cp2");
>  generate_exception_end(ctx, EXCP_RI);
> @@ -5878,6 +5905,7 @@ static void gen_loongson_multimedia(DisasContext
*ctx, int rd, int rs, int rt)
>
>  gen_store_fpr64(ctx, t0, rd);
>
> +no_rd:
>  tcg_temp_free_i64(t0);
>  tcg_temp_free_i64(t1);
>  }
> --
> 2.26.0.rc2
>
>
>

Re: Qemu on Windows 10 - no acceleration found

[Stefan Weil]

Am 20.03.20 um 21:22 schrieb Jerry Geis:

> So I tried --enable-whpx and I get Invalid option. Im on Windows 10
> and QEMU 4.2.0
>
> I'm confused.  Then I don't know where to download the HAXM. The place
> I found is GIT and it wants the user to compile it. I was looking for
> just an EXE.
>
> Thanks
>
> Jerry


Sorry, the FAQ was outdated. I updated it now.

Run `qemu-system-x86_64 --accel whpx`.

Using WHPX is easier than using HAX.

Stefan

Re: [PATCH] monitor/hmp-cmds: fix bad indentation in 'info migrate_parameters' cmd output

[Markus Armbruster]

"Dr. David Alan Gilbert"  writes:

> * Daniel P. Berrangé (berra...@redhat.com) wrote:
>> On Fri, Mar 20, 2020 at 05:31:17PM +, Dr. David Alan Gilbert wrote:
>> > (Rearranging the text a bit)
>> > 
>> > * Markus Armbruster (arm...@redhat.com) wrote:
>> > 
>> > > David (cc'ed) should be able to tell us which fix is right.
>> > > 
>> > > @tls_creds and @tls_hostname look like they could have the same issue.
>> > 
>> > A certain Markus removed the Null checks in 8cc99dc because 4af245d
>> > guaranteed they would be None-Null for tls-creds/hostname - so we
>> > should be OK for those.
>> > 
>> > But tls-authz came along a lot later in d2f1d29 and doesn't
>> > seem to have the initialisation, which is now in
>> > migration_instance_init.
>> > 
>> > So I *think* the fix for this is to do the modern equivalent of 4af245d
>> > :
>> > 
>> > diff --git a/migration/migration.c b/migration/migration.c
>> > index c1d88ace7f..0bc1b93277 100644
>> > --- a/migration/migration.c
>> > +++ b/migration/migration.c
>> > @@ -3686,6 +3686,7 @@ static void migration_instance_init(Object *obj)
>> >  
>> >  params->tls_hostname = g_strdup("");
>> >  params->tls_creds = g_strdup("");
>> > +params->tls_authz = g_strdup("");
>> >  
>> >  /* Set has_* up only for parameter checks */
>> >  params->has_compress_level = true;
>> > 
>> > Copying in Dan to check that wouldn't break tls.
>> 
>> It *will* break TLS, because it will cause the TLS code to lookup
>> an object with the ID of "".  NULL must be preserved when calling
>> the TLS APIs.
>
> OK, good I asked...
>
>> The assignment of "" to tls_hostname would also have broken TLS,
>> so the migration_tls_channel_connect method had to turn it back
>> into a real NULL.
>> 
>> The use of "" for tls_creds will similarly cause it to try and
>> lookup an object with ID of "", and fail. That one's harmless
>> though, because it would also fail if it were NULL.
>
> OK.
>
> It looks like the output of query-migrate-parameters though already
> turns it into "", so I don't think you can tell it's NULL from that:
>
> {"QMP": {"version": {"qemu": {"micro": 0, "minor": 2, "major": 4}, "package": 
> "qemu-4.2.0-4.fc31"}, "capabilities": ["oob"]}}
> { "execute": "qmp_capabilities" }
> {"return": {}}
> { "execute": "query-migrate-parameters" }
> {"return": {"xbzrle-cache-size": 67108864, "cpu-throttle-initial": 20, 
> "announce-max": 550, "decompress-threads": 2, "compress-threads": 8, 
> "compress-level": 1, "multifd-channels": 2, "announce-initial": 50, 
> "block-incremental": false, "compress-wait-thread": true, "downtime-limit": 
> 300, "tls-authz": "", "announce-rounds": 5, "announce-step": 100, 
> "tls-creds": "", "max-cpu-throttle": 99, "max-postcopy-bandwidth": 0, 
> "tls-hostname": "", "max-bandwidth": 33554432, "x-checkpoint-delay": 2, 
> "cpu-throttle-increment": 10}}
>
> I'm not sure who turns a Null into a "" but I guess it's somewhere in
> the json output iterator.

It's this wart:

static void qobject_output_type_str(Visitor *v, const char *name, char 
**obj,
Error **errp)
{
QObjectOutputVisitor *qov = to_qov(v);
if (*obj) {
qobject_output_add(qov, name, qstring_from_str(*obj));
} else {
qobject_output_add(qov, name, qstring_from_str(""));
}
}

Warty since day 1.

In theory, !*obj should not happen, since QAPI type 'str' is not
nullable.

In practice, it handwritten code can easily make it happen.

> So we can fix this problem either in qmp_query_migrate_parameters
> and just strdup a "", or substitute it in hmp_info_migrate_parameters.

Fixing it in qmp_query_migrate_parameters() is cleaner: it avoids null
'str', and bypasses the wart.