[Qemu-devel] [PATCH v6 1/5] hypertrace: Add documentation

[Lluís Vilanova]

Signed-off-by: Lluís Vilanova 
---
 docs/devel/tracing.txt |3 +
 docs/hypertrace.txt|  225 
 2 files changed, 228 insertions(+)
 create mode 100644 docs/hypertrace.txt

diff --git a/docs/devel/tracing.txt b/docs/devel/tracing.txt
index 5768a0b7a2..9178a308da 100644
--- a/docs/devel/tracing.txt
+++ b/docs/devel/tracing.txt
@@ -5,6 +5,9 @@
 This document describes the tracing infrastructure in QEMU and how to use it
 for debugging, profiling, and observing execution.
 
+See "docs/hypertrace.txt" to correlate guest tracing events with those in the
+QEMU host.
+
 == Quickstart ==
 
 1. Build with the 'simple' trace backend:
diff --git a/docs/hypertrace.txt b/docs/hypertrace.txt
new file mode 100644
index 00..c3715db25b
--- /dev/null
+++ b/docs/hypertrace.txt
@@ -0,0 +1,225 @@
+= Hypertrace channel =
+
+Copyright (C) 2016-2017 Lluís Vilanova 
+
+This work is licensed under the terms of the GNU GPL, version 2 or later.
+See the COPYING file in the top-level directory.
+
+
+The hypertrace channel allows guest code to emit events in QEMU (the host) 
using
+its tracing infrastructure (see "docs/trace.txt"). This works in both 'system'
+and 'user' modes. Therefore, hypertrace is to tracing what hypercalls are to
+system calls.
+
+The hypertrace channel can be used for various purposes:
+
+* Using guest code semantics to guide which QEMU events to trace at each point
+  in time. The example "Quick guide" below shows how to use this to identify
+  "regions of interest" in your guest code. It then uses these regions to trace
+  QEMU's behaviour during their execution, without paying the price of tracing
+  events outside the interest regions.
+
+* Mark "progress points" in guest code (e.g., processed client requests,
+  scheduled processes, etc), so that they can be easily traced and correlated
+  between QEMU's various tracing events and the guest's own tracing
+  infrastructure (e.g., Linux's tracepoints).
+
+* You can also use regions of interest and progress points on the guest code to
+  time the performance of new TCG optimizations. Each hypertrace event comes
+  with a host timestamp, making it easy to compare the host execution times of
+  interesting guest code.
+
+Hypertrace features:
+
+* Works with 'system' and 'user' mode.
+
+* Minimal setup for the guest; QEMU provides support guest code libraries that
+  work out of the box.
+
+* Independent of guest architecture; the guest code uses accesses to special
+  memory regions, as opposed to redefining instruction semantics.
+
+* Negligible guest overhead; emitting a hypertrace event requires a single 
guest
+  memory access, making it as unobtrusive as possible.
+
+Warning: The hypertrace channel in 'system' mode only works in systems with
+support for PCI. You can get the list of guests with PCI support with 'grep
+pci.mak default-configs/*'.
+
+
+== Quick guide ==
+
+This shows an example of using the hypertrace channel to trace the guest memory
+accesses only in a specific guest code region, which is identified by calls to
+the hypertrace channel.
+
+We are going to trace memory accesses to disk using QEMU's "log" backend, and
+will use QEMU's "dtrace" backend (SystemTap) to ensure memory accesses are only
+traced in the guest code region of interest. The first time the guest code
+invokes the hypertrace channel, we will start tracing the
+"guest_mem_before_exec" event using dtrace, and then will disable it the second
+time around.
+
+Tracing is done with "log" because it is more efficient than using "dtrace" in
+high-volume events like memory accesses.
+
+1. Set the tracing backends and number of arguments for the hypertrace events:
+
+mkdir /tmp/qemu-build
+cd /tmp/qemu-build
+/path/to/qemu-source/configure  \
+--enable-trace-backends=dtrace,log  \
+--with-hypertrace-args=4\
+--prefix=/tmp/qemu-install
+make -j install
+
+2. Compile QEMU:
+
+make -C /tmp/qemu-build install -j
+
+3. Compile the guest support code:
+
+make -C /tmp/qemu-build/x86_64-linux-user/hypertrace/guest
+make -C /tmp/qemu-build/x86_64-softmmu/hypertrace/guest
+
+   If you need to cross-compile the guest library, set the 'CC' variable:
+
+make -C /tmp/qemu-build/mipsel-linux-user/hypertrace/guest 
CC=mipsel-gnu-linux-gcc
+
+4. Create a guest application that interacts with the hypertrace channel:
+
+cat > /tmp/my-hypertrace.c <<\EOF
+#include 
+#include 
+#include 
+#include 
+#include 
+
+
+int main(int argc, char **argv)
+{
+char *base = NULL;
+if (argc > 1) {
+base = argv[1];
+}
+
+/* In 'user' mode this path must be the same we will use to start 
QEMU. */
+if (qemu_hypertrace_init(base) != 0) {
+perror("error: qemu_hypertrace_init");
+abort();
+}
+
+/* Set additional event arguments (unused in 

[Qemu-devel] [PATCH v6 0/5] hypertrace: Lightweight guest-to-QEMU trace channel

[Lluís Vilanova]

The hypertrace channel allows guest code to emit events in QEMU (the host) using
its tracing infrastructure (see "docs/trace.txt"). This works in both 'system'
and 'user' modes, is architecture-agnostic and introduces minimal noise on the
guest.

See first commit for a full description, use-cases and an example.

Signed-off-by: Lluís Vilanova 
---

Changes in v6
=

* Fix compilation errors.


Changes in v5
=

* Rebase on 5a477a7806.
* Fix typo in "bsd-user/main.c" [Stephan Hajnoczi].
* Replace abort() with exit() in command-line errors [Stephan Hajnoczi].
* Fix alignment of data and control channels [Stephan Hajnoczi].
* Fix signal reflection in user-mode (SIGINT, SIGABRT, SIGSEGV) [Stephan 
Hajnoczi].
* Clarify semantics of hypertrace_guest_mmap_check() [Stephan Hajnoczi].
* Use uintptr_t instead of unsigned long in SEGV handler [Stephan Hajnoczi].
* Emit hypertrace's event with host-endian arguments [Stephan Hajnoczi].
* Enable true concurrency between user-mode guest threads by using a spearate 
control channel page per client [Stephan Hajnoczi].
* Remove unused PAGE_SIZE define [Stephan Hajnoczi].
* Submit linux kernel API module separately to Linux upstream [Stephan 
Hajnoczi].
* Assume guest code events are always enabled.


Changes in v4
=

* Fix typo in stap script example.
* Fix compilation instructions in doc/hypertrace.txt.
* Rebase on 0737f32daf.


Changes in v3
=

* Rebase on 4a58f35.
* Remove debugging printf's.
* Fix style issues identified by checkpatch.
* Fix control channel mapping in guest linux module.
* Add a short event description in "trace-events".
* Polish documentation in 1st patch.


Changes in v2
=

* Remove unnecessary casts for g2h() [Eric Blake].
* Use perror() [Eric Blake].
* Avoid expansions in application example [Eric Blake].
* Add copyright in document "hypertrace.txt" [Eric Blake].
* Make the user-mode hypertrace invocations thread-safe [Stefan Hajnoczi].
* Split dynamic hypertrace configuration into a separate "config" channel.

Lluís Vilanova (5):
  hypertrace: Add documentation
  hypertrace: Add tracing event "guest_hypertrace"
  hypertrace: [*-user] Add QEMU-side proxy to "guest_hypertrace" event
  hypertrace: [softmmu] Add QEMU-side proxy to "guest_hypertrace" event
  hypertrace: Add guest-side user-level library


 Makefile   |   12 +
 Makefile.objs  |6 +
 bsd-user/main.c|   17 +
 bsd-user/mmap.c|   15 +
 bsd-user/syscall.c |   34 ++-
 configure  |   36 +++
 docs/devel/tracing.txt |3 
 docs/hypertrace.txt|  225 
 hypertrace/Makefile.objs   |   23 ++
 hypertrace/common.c|   55 +
 hypertrace/common.h|   25 ++
 hypertrace/guest/Makefile  |   30 +++
 hypertrace/guest/common.c  |  301 ++
 hypertrace/guest/qemu-hypertrace.h |   80 +++
 hypertrace/softmmu.c   |  237 +
 hypertrace/user.c  |  414 
 hypertrace/user.h  |   71 ++
 include/hw/pci/pci.h   |2 
 include/qom/cpu.h  |4 
 linux-user/main.c  |   19 ++
 linux-user/mmap.c  |   16 +
 linux-user/qemu.h  |3 
 linux-user/signal.c|   12 +
 linux-user/syscall.c   |   31 ++-
 rules.mak  |2 
 trace-events   |   11 +
 26 files changed, 1654 insertions(+), 30 deletions(-)
 create mode 100644 docs/hypertrace.txt
 create mode 100644 hypertrace/Makefile.objs
 create mode 100644 hypertrace/common.c
 create mode 100644 hypertrace/common.h
 create mode 100644 hypertrace/guest/Makefile
 create mode 100644 hypertrace/guest/common.c
 create mode 100644 hypertrace/guest/qemu-hypertrace.h
 create mode 100644 hypertrace/softmmu.c
 create mode 100644 hypertrace/user.c
 create mode 100644 hypertrace/user.h


To: qemu-devel@nongnu.org
Cc: Stefan Hajnoczi 
Cc: Eric Blake 
Cc: Luiz Capitulino 
Cc: Daniel P Berrange 

Re: [Qemu-devel] [PATCH for 2.10 v2 12/20] syscall: fix dereference of undefined pointer

[Laurent Vivier]

Le 27/07/2017 à 04:42, Philippe Mathieu-Daudé a écrit :
> linux-user/syscall.c:5581:9: warning: Dereference of undefined pointer value
> if (*host_rt_dev_ptr != 0) {
> ^~~~
> 
> Reported-by: Clang Static Analyzer
> Suggested-by: Laurent Vivier 
> Signed-off-by: Philippe Mathieu-Daudé 

Reviewed-by: Laurent Vivier 

> ---
>  linux-user/syscall.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 003943b736..71d45a9963 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -5573,6 +5573,7 @@ static abi_long do_ioctl_rt(const IOCTLEntry *ie, 
> uint8_t *buf_temp,
>  field_types, THUNK_HOST);
>  }
>  unlock_user(argptr, arg, 0);
> +assert(host_rt_dev_ptr);
>  
>  ret = get_errno(safe_ioctl(fd, ie->host_cmd, buf_temp));
>  if (*host_rt_dev_ptr != 0) {
> 

Re: [Qemu-devel] [PATCH for 2.10 v2 14/20] syscall: check inotify() and eventfd() return value

[Laurent Vivier]

Le 27/07/2017 à 04:42, Philippe Mathieu-Daudé a écrit :
> linux-user/syscall.c:555:25: warning: Out of bound memory access (accessed 
> memory precedes memory block)
> target_fd_trans[fd] = trans;
> ^~~
> 
> Reported-by: Clang Static Analyzer
> Suggested-by: Laurent Vivier 
> Signed-off-by: Philippe Mathieu-Daudé 

Reviewed-by: Laurent Vivier 

> ---
>  linux-user/syscall.c | 16 
>  1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 81f52f7483..dfc1301e63 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -11742,7 +11742,9 @@ abi_long do_syscall(void *cpu_env, int num, abi_long 
> arg1,
>  #if defined(TARGET_NR_inotify_init) && defined(__NR_inotify_init)
>  case TARGET_NR_inotify_init:
>  ret = get_errno(sys_inotify_init());
> -fd_trans_register(ret, &target_inotify_trans);
> +if (ret >= 0) {
> +fd_trans_register(ret, &target_inotify_trans);
> +}
>  break;
>  #endif
>  #ifdef CONFIG_INOTIFY1
> @@ -11750,7 +11752,9 @@ abi_long do_syscall(void *cpu_env, int num, abi_long 
> arg1,
>  case TARGET_NR_inotify_init1:
>  ret = get_errno(sys_inotify_init1(target_to_host_bitmask(arg1,
>fcntl_flags_tbl)));
> -fd_trans_register(ret, &target_inotify_trans);
> +if (ret >= 0) {
> +fd_trans_register(ret, &target_inotify_trans);
> +}
>  break;
>  #endif
>  #endif
> @@ -11916,7 +11920,9 @@ abi_long do_syscall(void *cpu_env, int num, abi_long 
> arg1,
>  #if defined(TARGET_NR_eventfd)
>  case TARGET_NR_eventfd:
>  ret = get_errno(eventfd(arg1, 0));
> -fd_trans_register(ret, &target_eventfd_trans);
> +if (ret >= 0) {
> +fd_trans_register(ret, &target_eventfd_trans);
> +}
>  break;
>  #endif
>  #if defined(TARGET_NR_eventfd2)
> @@ -11930,7 +11936,9 @@ abi_long do_syscall(void *cpu_env, int num, abi_long 
> arg1,
>  host_flags |= O_CLOEXEC;
>  }
>  ret = get_errno(eventfd(arg1, host_flags));
> -fd_trans_register(ret, &target_eventfd_trans);
> +if (ret >= 0) {
> +fd_trans_register(ret, &target_eventfd_trans);
> +}
>  break;
>  }
>  #endif
> 

Re: [Qemu-devel] [Xen-devel] [PULL 3/3] xen-disk: add support for multi-page shared rings

[Olaf Hering]

On Tue, Jun 27, Stefano Stabellini wrote:

> From: Paul Durrant 
> The blkif protocol has had provision for negotiation of multi-page shared
> rings for some time now and many guest OS have support in their frontend
> drivers.

> +++ b/hw/block/xen_disk.c

> +domids = g_malloc0_n(blkdev->nr_ring_ref, sizeof(uint32_t));

According to [1] g_malloc0_n requires at least glib-2.24. As a result
compilation of qemu-2.10 fails in SLE11, which has just glib-2.22.

Olaf

[1] https://developer.gnome.org/glib/stable/glib-Memory-Allocation.html


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH for 2.10 v2 10/20] m68k/translate: fix incorrect copy/paste

[Richard Henderson]

On 07/26/2017 07:42 PM, Philippe Mathieu-Daudé wrote:

db3d7945ae extended gen_cc_cond() for cond [6, 7, 9, 10] but misswrote [4, 5]

target/m68k/translate.c:1323:70: warning: identical expressions on both sides 
of logical operator
 if (op == CC_OP_ADDB || op == CC_OP_ADDW || op == CC_OP_ADDL ||
 op == CC_OP_ADDB || op == CC_OP_ADDW || op == CC_OP_ADDL) {
  ^

Reported-by: Clang Static Analyzer
Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Laurent Vivier
---
  target/m68k/translate.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)


Reviewed-by: Richard Henderson 


r~

Re: [Qemu-devel] [for-2.11 PATCH 26/26] spapr: add hotplug hooks for PHB hotplug

[Alexey Kardashevskiy]

On 26/07/17 18:40, Greg Kurz wrote:
> Hotplugging PHBs is a machine-level operation, but PHBs reside on the
> main system bus, so we register spapr machine as the handler for the
> main system bus.
> 
> Signed-off-by: Michael Roth 
> Signed-off-by: Greg Kurz 
> ---
> - rebased against ppc-for-2.10
> - converted to unplug_request
> - handle drc_id at pre-plug
> - reset hotplugged PHB at plug
> - compatibility with older machine types
> ---
>  hw/ppc/spapr.c  |  114 
> +++
>  hw/ppc/spapr_drc.c  |1 
>  hw/ppc/spapr_pci.c  |2 -
>  include/hw/pci-host/spapr.h |2 +
>  include/hw/ppc/spapr.h  |1 
>  5 files changed, 118 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 90485054c2e7..589f76ef9fb8 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2540,6 +2540,10 @@ static void ppc_spapr_init(MachineState *machine)
>  register_savevm_live(NULL, "spapr/htab", -1, 1,
>   &savevm_htab_handlers, spapr);
>  
> +if (spapr->dr_phb_enabled) {
> +qbus_set_hotplug_handler(sysbus_get_default(), OBJECT(machine), 
> NULL);
> +}
> +
>  qemu_register_boot_set(spapr_boot_set, spapr);
>  
>  if (kvm_enabled()) {
> @@ -3238,6 +3242,103 @@ out:
>  error_propagate(errp, local_err);
>  }
>  
> +static void spapr_phb_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> +   Error **errp)
> +{
> +sPAPRPHBState *sphb = SPAPR_PCI_HOST_BRIDGE(dev);
> +
> +if (sphb->drc_id == (uint32_t)-1) {
> +sphb->drc_id = sphb->index;
> +}
> +
> +if (sphb->drc_id >= SPAPR_DRC_MAX_PHB) {
> +error_setg(errp, "PHB id %d out of range", sphb->drc_id);
> +}


sphb->index in considered 16bits in the existing code (even though it is
defined as 32bit) and SPAPR_DRC_MAX_PHB is just 256. I'd suggest using the
same limit for both, either 256 or 65536 is fine for me.

It is actually a bit weird - it is possible to completely configure few
PHBs in the command line so they will have index==-1 but PCI hotplug code -
spapr_phb_get_pci_func_drc() and spapr_phb_realize() - does not check for
this and just does (sphb->index << 16). May be just ditch drc_id, enforce
index not to be -1 and use it as drc_id?



> +}
> +
> +static void spapr_phb_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> +   Error **errp)
> +{
> +sPAPRMachineState *spapr = SPAPR_MACHINE(OBJECT(hotplug_dev));
> +sPAPRPHBState *sphb = SPAPR_PCI_HOST_BRIDGE(dev);
> +void *fdt = NULL;
> +int fdt_start_offset;
> +int fdt_size;
> +Error *local_err = NULL;
> +sPAPRDRConnector *drc;
> +uint32_t phandle;
> +int ret;
> +bool hotplugged = spapr_drc_hotplugged(dev);
> +
> +if (!spapr->dr_phb_enabled) {
> +return;
> +}
> +
> +drc = spapr_drc_by_id(TYPE_SPAPR_DRC_PHB, sphb->drc_id);
> +/* hotplug hooks should check it's enabled before getting this far */
> +g_assert(drc);
> +
> +if (hotplugged) {
> +if (spapr->xics_phandle == UINT32_MAX) {
> +error_setg(&local_err,
> +   "SLOF didn't update the XICS phandle. PHB hotplug 
> cancelled");
> +goto out;
> +}
> +phandle = spapr->xics_phandle;
> +
> +spapr_phb_reset(dev);


It could be DEVICE_GET_CLASS(dev)->reset(dev) without exporting
spapr_phb_reset. Not sure how this fits into the current QEMU coding style
though.



> +} else {
> +phandle = PHANDLE_XICP;
> +}
> +
> +fdt = create_device_tree(&fdt_size);
> +ret = spapr_populate_pci_dt(sphb, phandle, fdt, &fdt_start_offset);
> +if (ret < 0) {
> +error_setg(&local_err, "unable to create FDT for %sPHB",
> +   dev->hotplugged ? "hotplugged " : "");
> +goto out;
> +}
> +
> +if (hotplugged) {
> +/* generally SLOF creates these, for hotplug it's up to QEMU */
> +_FDT(fdt_setprop_string(fdt, fdt_start_offset, "name", "pci"));
> +}
> +
> +spapr_drc_attach(drc, DEVICE(dev), fdt, fdt_start_offset, &local_err);
> +out:
> +if (local_err) {
> +error_propagate(errp, local_err);
> +g_free(fdt);
> +return;
> +}
> +
> +if (hotplugged) {
> +spapr_hotplug_req_add_by_index(drc);
> +} else if (drc) {
> +spapr_drc_reset(drc);
> +}
> +}
> +
> +void spapr_phb_release(DeviceState *dev)
> +{
> +object_unparent(OBJECT(dev));
> +}
> +
> +static void spapr_phb_unplug_request(HotplugHandler *hotplug_dev,
> + DeviceState *dev, Error **errp)
> +{
> +sPAPRPHBState *sphb = SPAPR_PCI_HOST_BRIDGE(dev);
> +sPAPRDRConnector *drc;
> +
> +drc = spapr_drc_by_id(TYPE_SPAPR_DRC_PHB, sphb->drc_id);
> +g_assert(drc);
> +
> +if (!spapr_drc_unplug_requested(drc)) {
> +spapr_drc_detach(drc);
> +spapr_hotplug_req_remove_

Re: [Qemu-devel] [PATCH for 2.10 0/4] check dtc submodule is outdated

[Philippe Mathieu-Daudé]

On 07/26/2017 06:40 PM, Philippe Mathieu-Daudé wrote:> patch 4: if no 
system libdtc and submodule present, compile the dtc submodule

and verify it is at least v1.4.2. Prefixed RFC because I'm not sure about
these 3 lines:

+make -C dtc 1>/dev/null


which can leads to:

/bin/sh: 1: cannot create libfdt/fdt_sw.d: Read-only file system
 DEP libfdt/fdt_wip.c

so the libdtc build should be handle more seriously :(



yes, we need to build the libdtc to be able to run the compile_prog link step

+fdt_cflags="-I${source_path}/dtc/libfdt"

$source_path seems ok...

+fdt_libs="-L$(pwd)/dtc/libfdt $fdt_libs"

maybe there is a better option than `pwd`

Re: [Qemu-devel] [PATCH for 2.10 v2 19/20] spapr_vio: fix overflow of qdevs in spapr_dt_vdevice()

[Philippe Mathieu-Daudé]

On 07/27/2017 12:43 AM, David Gibson wrote:

On Wed, Jul 26, 2017 at 11:42:23PM -0300, Philippe Mathieu-Daudé wrote:

sizeof(ptr) was used instead of sizeof(struct)...

also use g_malloc_n() which take care of possible type overflow.

hw/ppc/spapr_vio.c:641:22: warning: The code calls sizeof() on a pointer type. 
This can produce an unexpected result
 qdevs = g_malloc(sizeof(qdev) * num);
  ^ ~~
hw/ppc/spapr_vio.c:648:23: warning: The code calls sizeof() on a pointer type. 
This can produce an unexpected result
 qsort(qdevs, num, sizeof(qdev), compare_reg);
   ^ ~~

Reported-by: Clang Static Analyzer
Signed-off-by: Philippe Mathieu-Daudé 


Nack.

Have a closer look, what's going in the array really is pointers, not
structures.  This is a false warning from clang, we need to find a
different way to suppress it.


Something was bothering me with that patch, wondering why it never 
explode previously, now I see/understand.



---
  hw/ppc/spapr_vio.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/ppc/spapr_vio.c b/hw/ppc/spapr_vio.c
index ea3bc8bd9e..9991b44c9f 100644
--- a/hw/ppc/spapr_vio.c
+++ b/hw/ppc/spapr_vio.c
@@ -638,14 +638,14 @@ void spapr_dt_vdevice(VIOsPAPRBus *bus, void *fdt)
  }
  
  /* Copy out into an array of pointers */


/ashamed the comment was in front of me...

Thank you David for the review!

Phil.


-qdevs = g_malloc(sizeof(qdev) * num);
+qdevs = g_malloc_n(num, sizeof(*qdev));
  num = 0;
  QTAILQ_FOREACH(kid, &bus->bus.children, sibling) {
  qdevs[num++] = kid->child;
  }
  
  /* Sort the array */

-qsort(qdevs, num, sizeof(qdev), compare_reg);
+qsort(qdevs, num, sizeof(*qdev), compare_reg);
  
  /* Hack alert. Give the devices to libfdt in reverse order, we happen

   * to know that will mean they are in forward order in the tree. */

Re: [Qemu-devel] [PATCH for 2.10 v2 19/20] spapr_vio: fix overflow of qdevs in spapr_dt_vdevice()

[David Gibson]

On Wed, Jul 26, 2017 at 11:42:23PM -0300, Philippe Mathieu-Daudé wrote:
> sizeof(ptr) was used instead of sizeof(struct)...
> 
> also use g_malloc_n() which take care of possible type overflow.
> 
> hw/ppc/spapr_vio.c:641:22: warning: The code calls sizeof() on a pointer 
> type. This can produce an unexpected result
> qdevs = g_malloc(sizeof(qdev) * num);
>  ^ ~~
> hw/ppc/spapr_vio.c:648:23: warning: The code calls sizeof() on a pointer 
> type. This can produce an unexpected result
> qsort(qdevs, num, sizeof(qdev), compare_reg);
>   ^ ~~
> 
> Reported-by: Clang Static Analyzer
> Signed-off-by: Philippe Mathieu-Daudé 

Nack.

Have a closer look, what's going in the array really is pointers, not
structures.  This is a false warning from clang, we need to find a
different way to suppress it.

> ---
>  hw/ppc/spapr_vio.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ppc/spapr_vio.c b/hw/ppc/spapr_vio.c
> index ea3bc8bd9e..9991b44c9f 100644
> --- a/hw/ppc/spapr_vio.c
> +++ b/hw/ppc/spapr_vio.c
> @@ -638,14 +638,14 @@ void spapr_dt_vdevice(VIOsPAPRBus *bus, void *fdt)
>  }
>  
>  /* Copy out into an array of pointers */
> -qdevs = g_malloc(sizeof(qdev) * num);
> +qdevs = g_malloc_n(num, sizeof(*qdev));
>  num = 0;
>  QTAILQ_FOREACH(kid, &bus->bus.children, sibling) {
>  qdevs[num++] = kid->child;
>  }
>  
>  /* Sort the array */
> -qsort(qdevs, num, sizeof(qdev), compare_reg);
> +qsort(qdevs, num, sizeof(*qdev), compare_reg);
>  
>  /* Hack alert. Give the devices to libfdt in reverse order, we happen
>   * to know that will mean they are in forward order in the tree. */

-- 
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: [Qemu-devel] [for-2.11 PATCH 07/26] spapr_drc: fix realize and unrealize

[David Gibson]

On Wed, Jul 26, 2017 at 11:36:43AM +0200, Greg Kurz wrote:
> On Wed, 26 Jul 2017 14:04:59 +1000
> David Gibson  wrote:
> 
> > On Tue, Jul 25, 2017 at 07:59:31PM +0200, Greg Kurz wrote:
> > > If object_property_add_alias() returns an error in realize(), we should
> > > propagate it to the caller and certainly not unref the DRC.
> > > 
> > > Same thing goes for unrealize(). Since object_property_del() is the last
> > > call, we can even get rid of the intermediate Error *.
> > > 
> > > And finally, unrealize() should undo all registrations performed by
> > > realize().
> > > 
> > > Signed-off-by: Greg Kurz   
> > 
> > I've applied this to ppc-for-2.11, but this looks like it could be a
> > real bug fix.  So I'm wondering if I should push it for 2.10 (we're in
> > hard freeze, but bugfixes can still be applied).
> > 
> 
> Yeah I guess it should also be merged in 2.10 but it has a dependency
> with patch 4 (spapr_drc: use g_strdup_printf() instead  of snprintf()).
> If you prefer, maybe I can repost this as a standalone fix for 2.10
> ?

Yes, that would be helpful, thank you.

> 
> > > ---
> > >  hw/ppc/spapr_drc.c |   15 ++-
> > >  1 file changed, 6 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> > > index e4e8383ec7b5..d72453bcb42f 100644
> > > --- a/hw/ppc/spapr_drc.c
> > > +++ b/hw/ppc/spapr_drc.c
> > > @@ -506,12 +506,12 @@ static void realize(DeviceState *d, Error **errp)
> > >  trace_spapr_drc_realize_child(spapr_drc_index(drc), child_name);
> > >  object_property_add_alias(root_container, link_name,
> > >drc->owner, child_name, &err);
> > > +g_free(child_name);
> > >  g_free(link_name);
> > >  if (err) {
> > > -error_report_err(err);
> > > -object_unref(OBJECT(drc));
> > > +error_propagate(errp, err);
> > > +return;
> > >  }
> > > -g_free(child_name);
> > >  vmstate_register(DEVICE(drc), spapr_drc_index(drc), 
> > > &vmstate_spapr_drc,
> > >   drc);
> > >  qemu_register_reset(drc_reset, drc);
> > > @@ -523,17 +523,14 @@ static void unrealize(DeviceState *d, Error **errp)
> > >  sPAPRDRConnector *drc = SPAPR_DR_CONNECTOR(d);
> > >  Object *root_container;
> > >  gchar *name;
> > > -Error *err = NULL;
> > >  
> > >  trace_spapr_drc_unrealize(spapr_drc_index(drc));
> > > +qemu_unregister_reset(drc_reset, drc);
> > > +vmstate_unregister(DEVICE(drc), &vmstate_spapr_drc, drc);
> > >  root_container = container_get(object_get_root(), 
> > > DRC_CONTAINER_PATH);
> > >  name = g_strdup_printf("%x", spapr_drc_index(drc));
> > > -object_property_del(root_container, name, &err);
> > > +object_property_del(root_container, name, errp);
> > >  g_free(name);
> > > -if (err) {
> > > -error_report_err(err);
> > > -object_unref(OBJECT(drc));
> > > -}
> > >  }
> > >  
> > >  sPAPRDRConnector *spapr_dr_connector_new(Object *owner, const char *type,
> > >   
> > 
> 



-- 
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

[Qemu-devel] why marking qcow2 img as corrupted

[lampahome]

I tried take snapshots on a demo.qcow2 10 times.
cmd is below:

> qemu-img snapshots -c tag_1 demo.qcow2


when I take snapshots 7 times and console shows:

> mark image as corrupted and preventing from invalid write


and I can't take snapshots anymore then.
Can I avoid this situation or remove the limit?

Re: [Qemu-devel] Commit 77af8a2b95b79699de650965d5228772743efe84 breaks Windows 2000 support

[Programmingkid]

I just realized what we need in order to test QEMU better. We need a list of 
people who
are willing to support a certain operating system. 

The list would probably be located here: http://wiki.qemu.org/Testing/Windows

It would look like this:


Operating systemTester

Windows 3.1 , , ...
Windows NT 3.1  , , ...
Windows NT 3.5  , , ...
Windows 95  , , ...
Windows NT 4.0  , , ...
Windows 98  , , ...
Windows ME  , , ...
Windows 2000, , ...
Windows XP  , , ...
Windows Vista   , , ...
Windows 7   , , ...
Windows 8   , , ...
Windows 10  , , ...
ReactOS , , ...


If someone has a patch that makes a change to the qemu-system-i386 emulator,
the patch maker can consult the list of people who might be willing to help 
test the patch. 
There are many people who work on QEMU with certain versions of Windows at their
disposal. With this knowledge compatibility testing can become more thorough.
Thoughts? Ideas?

Re: [Qemu-devel] [PATCH v12 5/8] virtio-balloon: VIRTIO_BALLOON_F_SG

[Wei Wang]

On 07/27/2017 01:02 AM, Michael S. Tsirkin wrote:

On Wed, Jul 26, 2017 at 11:48:41AM +0800, Wei Wang wrote:

On 07/23/2017 09:45 AM, Michael S. Tsirkin wrote:

On Fri, Jul 14, 2017 at 03:12:43PM +0800, Wei Wang wrote:

On 07/14/2017 04:19 AM, Michael S. Tsirkin wrote:

On Thu, Jul 13, 2017 at 03:42:35PM +0800, Wei Wang wrote:

On 07/12/2017 09:56 PM, Michael S. Tsirkin wrote:

So the way I see it, there are several issues:

- internal wait - forces multiple APIs like kick/kick_sync
  note how kick_sync can fail but your code never checks return code
- need to re-write the last descriptor - might not work
  for alternative layouts which always expose descriptors
  immediately

Probably it wasn't clear. Please let me explain the two functions here:

1) virtqueue_add_chain_desc(vq, head_id, prev_id,..):
grabs a desc from the vq and inserts it to the chain tail (which is indexed
by
prev_id, probably better to call it tail_id). Then, the new added desc
becomes
the tail (i.e. the last desc). The _F_NEXT flag is cleared for each desc
when it's
added to the chain, and set when another desc comes to follow later.

And this only works if there are multiple rings like
avail + descriptor ring.
It won't work e.g. with the proposed new layout where
writing out a descriptor exposes it immediately.

I think it can support the 1.1 proposal, too. But before getting
into that, I think we first need to deep dive into the implementation
and usage of _first/next/last. The usage would need to lock the vq
from the first to the end (otherwise, the returned info about the number
of available desc in the vq, i.e. num_free, would be invalid):

lock(vq);
add_first();
add_next();
add_last();
unlock(vq);

However, I think the case isn't this simple, since we need to check more
things
after each add_xx() step. For example, if only one entry is available at the
time
we start to use the vq, that is, num_free is 0 after add_first(), we
wouldn't be
able to add_next and add_last. So, it would work like this:

start:
  ...get free page block..
  lock(vq)
retry:
  ret = add_first(..,&num_free,);
  if(ret == -ENOSPC) {
  goto retry;
  } else if (!num_free) {
  add_chain_head();
  unlock(vq);
  kick & wait;
  goto start;
  }
next_one:
  ...get free page block..
  add_next(..,&num_free,);
  if (!num_free) {
  add_chain_head();
  unlock(vq);
  kick & wait;
  goto start;
  } if (num_free == 1) {
  ...get free page block..
  add_last(..);
  unlock(vq);
  kick & wait;
  goto start;
  } else {
  goto next_one;
  }

The above seems unnecessary to me to have three different APIs.
That's the reason to combine them into one virtqueue_add_chain_desc().

-- or, do you have a different thought about using the three APIs?


Implementation Reference:

struct desc_iterator {
  unsigned int head;
  unsigned int tail;
};

add_first(*vq, *desc_iterator, *num_free, ..)
{
  if (vq->vq.num_free < 1)
  return -ENOSPC;
  get_desc(&desc_id);
  desc[desc_id].flag &= ~_F_NEXT;
  desc_iterator->head = desc_id
  desc_iterator->tail = desc_iterator->head;
  *num_free = vq->vq.num_free;
}

add_next(vq, desc_iterator, *num_free,..)
{
  get_desc(&desc_id);
  desc[desc_id].flag &= ~_F_NEXT;
  desc[desc_iterator.tail].next = desc_id;
  desc[desc_iterator->tail].flag |= _F_NEXT;
  desc_iterator->tail = desc_id;
  *num_free = vq->vq.num_free;
}

add_last(vq, desc_iterator,..)
{
  get_desc(&desc_id);
  desc[desc_id].flag &= ~_F_NEXT;
  desc[desc_iterator.tail].next = desc_id;
  desc_iterator->tail = desc_id;

  add_chain_head(); // put the desc_iterator.head to the ring
}


Best,
Wei

OK I thought this over. While we might need these new APIs in
the future, I think that at the moment, there's a way to implement
this feature that is significantly simpler. Just add each s/g
as a separate input buffer.


Should it be an output buffer?

Hypervisor overwrites these pages with zeroes. Therefore it is
writeable by device: DMA_FROM_DEVICE.


Why would the hypervisor need to zero the buffer? I think it may only
need to read out the info(base,size).

I think it should be like this:
the cmd hdr buffer: input, because the hypervisor would write it to
send a cmd to the guest
the payload buffer: output, for the hypervisor to read the info


I think output means from the
driver to device (i.e. DMA_TO_DEVICE).

This part is correct I believe.


This needs zero new APIs.

I know that follow-up patches need to add a header in front
so you might be thinking: how am I going to add this
header? The answer is quite simple - add it as a separate
out header.

Host will be able to distinguish between header and pages
by looking at the direction, and - should we want to add
IN data to header - additionally size (<4K => header).


I think this works

[Qemu-devel] [PATCH for 2.10 v2 20/20] i2c/exynos4210: fix write to I2CADD register, bit 0 is not mapped

[Philippe Mathieu-Daudé]

>From the Exynos4210 User Manual [1]:

14.4.1.3 I2CADDn (MULTI-MASTER I2C-Bus Address Register)
  [7-1] slave address, latched from the I2C-bus.
  bit [0] is not mapped.

[1]: Exynos_4_Dual_45nm_User_Manaul_Public_REV1.00-0.pdf

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

diff --git a/hw/i2c/exynos4210_i2c.c b/hw/i2c/exynos4210_i2c.c
index c96fa7d7be..e6a9ca8f03 100644
--- a/hw/i2c/exynos4210_i2c.c
+++ b/hw/i2c/exynos4210_i2c.c
@@ -243,7 +243,7 @@ static void exynos4210_i2c_write(void *opaque, hwaddr 
offset,
 break;
 case I2CADD_ADDR:
 if ((s->i2cstat & I2CSTAT_OUTPUT_EN) == 0) {
-s->i2cadd = v;
+s->i2cadd = v & ~1;
 }
 break;
 case I2CDS_ADDR:
-- 
2.13.3

[Qemu-devel] [PATCH for 2.10 v2 18/20] 9pfs: avoid sign conversion error simplifying the code

[Philippe Mathieu-Daudé]

(note this is how other functions also handle the errors).

hw/9pfs/9p.c:948:18: warning: Loss of sign in implicit conversion
offset = err;
 ^~~

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

diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 333dbb6f8e..0a37c8bd13 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -945,7 +945,6 @@ static void coroutine_fn v9fs_version(void *opaque)
 v9fs_string_init(&version);
 err = pdu_unmarshal(pdu, offset, "ds", &s->msize, &version);
 if (err < 0) {
-offset = err;
 goto out;
 }
 trace_v9fs_version(pdu->tag, pdu->id, s->msize, version.data);
@@ -962,13 +961,12 @@ static void coroutine_fn v9fs_version(void *opaque)
 
 err = pdu_marshal(pdu, offset, "ds", s->msize, &version);
 if (err < 0) {
-offset = err;
 goto out;
 }
-offset += err;
+err += offset;
 trace_v9fs_version_return(pdu->tag, pdu->id, s->msize, version.data);
 out:
-pdu_complete(pdu, offset);
+pdu_complete(pdu, err);
 v9fs_string_free(&version);
 }
 
-- 
2.13.3

[Qemu-devel] [PATCH for 2.10 v2 19/20] spapr_vio: fix overflow of qdevs in spapr_dt_vdevice()

[Philippe Mathieu-Daudé]

sizeof(ptr) was used instead of sizeof(struct)...

also use g_malloc_n() which take care of possible type overflow.

hw/ppc/spapr_vio.c:641:22: warning: The code calls sizeof() on a pointer type. 
This can produce an unexpected result
qdevs = g_malloc(sizeof(qdev) * num);
 ^ ~~
hw/ppc/spapr_vio.c:648:23: warning: The code calls sizeof() on a pointer type. 
This can produce an unexpected result
qsort(qdevs, num, sizeof(qdev), compare_reg);
  ^ ~~

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

diff --git a/hw/ppc/spapr_vio.c b/hw/ppc/spapr_vio.c
index ea3bc8bd9e..9991b44c9f 100644
--- a/hw/ppc/spapr_vio.c
+++ b/hw/ppc/spapr_vio.c
@@ -638,14 +638,14 @@ void spapr_dt_vdevice(VIOsPAPRBus *bus, void *fdt)
 }
 
 /* Copy out into an array of pointers */
-qdevs = g_malloc(sizeof(qdev) * num);
+qdevs = g_malloc_n(num, sizeof(*qdev));
 num = 0;
 QTAILQ_FOREACH(kid, &bus->bus.children, sibling) {
 qdevs[num++] = kid->child;
 }
 
 /* Sort the array */
-qsort(qdevs, num, sizeof(qdev), compare_reg);
+qsort(qdevs, num, sizeof(*qdev), compare_reg);
 
 /* Hack alert. Give the devices to libfdt in reverse order, we happen
  * to know that will mean they are in forward order in the tree. */
-- 
2.13.3

[Qemu-devel] [PATCH for 2.10 v2 17/20] bt-sdp: fix memory leak in sdp_service_record_build()

[Philippe Mathieu-Daudé]

hw/bt/sdp.c:753:5: warning: Potential leak of memory pointed to by 'data'
qsort(record->attribute_list, record->attributes,
^

Reported-by: Clang Static Analyzer
Signed-off-by: Philippe Mathieu-Daudé 
---
hw/bt/*:
get_maintainer.pl: No maintainers found

 hw/bt/sdp.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/bt/sdp.c b/hw/bt/sdp.c
index f67b3b89c0..7b2186e1f4 100644
--- a/hw/bt/sdp.c
+++ b/hw/bt/sdp.c
@@ -711,7 +711,7 @@ static void sdp_service_record_build(struct 
sdp_service_record_s *record,
 struct sdp_def_service_s *def, int handle)
 {
 int len = 0;
-uint8_t *data;
+uint8_t *buf, *data;
 int *uuid;
 
 record->uuids = 0;
@@ -725,7 +725,8 @@ static void sdp_service_record_build(struct 
sdp_service_record_s *record,
 g_malloc0(record->attributes * sizeof(*record->attribute_list));
 record->uuid =
 g_malloc0(record->uuids * sizeof(*record->uuid));
-data = g_malloc(len);
+buf = g_malloc(len);
+data = buf;
 
 record->attributes = 0;
 uuid = record->uuid;
@@ -748,6 +749,7 @@ static void sdp_service_record_build(struct 
sdp_service_record_s *record,
 record->attribute_list[record->attributes ++].len = len;
 data += len;
 }
+g_free(buf);
 
 /* Sort the attribute list by the AttributeID */
 qsort(record->attribute_list, record->attributes,
-- 
2.13.3

[Qemu-devel] [PATCH for 2.10 v2 12/20] syscall: fix dereference of undefined pointer

[Philippe Mathieu-Daudé]

linux-user/syscall.c:5581:9: warning: Dereference of undefined pointer value
if (*host_rt_dev_ptr != 0) {
^~~~

Reported-by: Clang Static Analyzer
Suggested-by: Laurent Vivier 
Signed-off-by: Philippe Mathieu-Daudé 
---
 linux-user/syscall.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 003943b736..71d45a9963 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -5573,6 +5573,7 @@ static abi_long do_ioctl_rt(const IOCTLEntry *ie, uint8_t 
*buf_temp,
 field_types, THUNK_HOST);
 }
 unlock_user(argptr, arg, 0);
+assert(host_rt_dev_ptr);
 
 ret = get_errno(safe_ioctl(fd, ie->host_cmd, buf_temp));
 if (*host_rt_dev_ptr != 0) {
-- 
2.13.3

[Qemu-devel] [PATCH for 2.10 v2 13/20] syscall: fix use of uninitialized values

[Philippe Mathieu-Daudé]

linux-user/syscall.c:1627:35: warning: 1st function call argument is an 
uninitialized value
target_saddr->sa_family = tswap16(addr->sa_family);
  ^~~~
linux-user/syscall.c:1629:25: warning: The left operand of '==' is a garbage 
value
if (addr->sa_family == AF_NETLINK && len >= sizeof(struct sockaddr_nl)) {
~~~ ^

Reported-by: Clang Static Analyzer
Signed-off-by: Philippe Mathieu-Daudé 
Reviewed-by: Laurent Vivier 
---
 linux-user/syscall.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 71d45a9963..81f52f7483 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -1622,6 +1622,7 @@ static inline abi_long host_to_target_sockaddr(abi_ulong 
target_addr,
 if (len == 0) {
 return 0;
 }
+assert(addr);
 
 target_saddr = lock_user(VERIFY_WRITE, target_addr, len, 0);
 if (!target_saddr)
-- 
2.13.3

[Qemu-devel] [PATCH for 2.10 v2 14/20] syscall: check inotify() and eventfd() return value

[Philippe Mathieu-Daudé]

linux-user/syscall.c:555:25: warning: Out of bound memory access (accessed 
memory precedes memory block)
target_fd_trans[fd] = trans;
^~~

Reported-by: Clang Static Analyzer
Suggested-by: Laurent Vivier 
Signed-off-by: Philippe Mathieu-Daudé 
---
 linux-user/syscall.c | 16 
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 81f52f7483..dfc1301e63 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -11742,7 +11742,9 @@ abi_long do_syscall(void *cpu_env, int num, abi_long 
arg1,
 #if defined(TARGET_NR_inotify_init) && defined(__NR_inotify_init)
 case TARGET_NR_inotify_init:
 ret = get_errno(sys_inotify_init());
-fd_trans_register(ret, &target_inotify_trans);
+if (ret >= 0) {
+fd_trans_register(ret, &target_inotify_trans);
+}
 break;
 #endif
 #ifdef CONFIG_INOTIFY1
@@ -11750,7 +11752,9 @@ abi_long do_syscall(void *cpu_env, int num, abi_long 
arg1,
 case TARGET_NR_inotify_init1:
 ret = get_errno(sys_inotify_init1(target_to_host_bitmask(arg1,
   fcntl_flags_tbl)));
-fd_trans_register(ret, &target_inotify_trans);
+if (ret >= 0) {
+fd_trans_register(ret, &target_inotify_trans);
+}
 break;
 #endif
 #endif
@@ -11916,7 +11920,9 @@ abi_long do_syscall(void *cpu_env, int num, abi_long 
arg1,
 #if defined(TARGET_NR_eventfd)
 case TARGET_NR_eventfd:
 ret = get_errno(eventfd(arg1, 0));
-fd_trans_register(ret, &target_eventfd_trans);
+if (ret >= 0) {
+fd_trans_register(ret, &target_eventfd_trans);
+}
 break;
 #endif
 #if defined(TARGET_NR_eventfd2)
@@ -11930,7 +11936,9 @@ abi_long do_syscall(void *cpu_env, int num, abi_long 
arg1,
 host_flags |= O_CLOEXEC;
 }
 ret = get_errno(eventfd(arg1, host_flags));
-fd_trans_register(ret, &target_eventfd_trans);
+if (ret >= 0) {
+fd_trans_register(ret, &target_eventfd_trans);
+}
 break;
 }
 #endif
-- 
2.13.3

[Qemu-devel] [PATCH for 2.10 v2 07/20] net/eth: fix incorrect check of iov_to_buf() return value

[Philippe Mathieu-Daudé]

So we have sizeof(struct in6_address) != sizeof(uintptr_t)
and Clang > Coverity on this, see 4555ca6816c :)

net/eth.c:426:30: warning: The code calls sizeof() on a pointer type. This can 
produce an unexpected result
return bytes_read == sizeof(dst_addr);
 ^ ~~
net/eth.c:475:34: warning: The code calls sizeof() on a pointer type. This can 
produce an unexpected result
return bytes_read == sizeof(src_addr);
 ^ ~~

Reported-by: Clang Static Analyzer
Signed-off-by: Philippe Mathieu-Daudé 
Reviewed-by: Dmitry Fleytman 
---
 net/eth.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/eth.c b/net/eth.c
index 5b9ba26a56..ae5d881aae 100644
--- a/net/eth.c
+++ b/net/eth.c
@@ -423,7 +423,7 @@ _eth_get_rss_ex_dst_addr(const struct iovec *pkt, int 
pkt_frags,
 rthdr_offset + sizeof(*ext_hdr),
 dst_addr, sizeof(*dst_addr));
 
-return bytes_read == sizeof(dst_addr);
+return bytes_read == sizeof(*dst_addr);
 }
 
 return false;
@@ -472,7 +472,7 @@ _eth_get_rss_ex_src_addr(const struct iovec *pkt, int 
pkt_frags,
 opt_offset + sizeof(opthdr),
 src_addr, sizeof(*src_addr));
 
-return bytes_read == sizeof(src_addr);
+return bytes_read == sizeof(*src_addr);
 }
 
 opt_offset += optlen;
-- 
2.13.3

[Qemu-devel] [PATCH for 2.10 v2 10/20] m68k/translate: fix incorrect copy/paste

[Philippe Mathieu-Daudé]

db3d7945ae extended gen_cc_cond() for cond [6, 7, 9, 10] but misswrote [4, 5]

target/m68k/translate.c:1323:70: warning: identical expressions on both sides 
of logical operator
if (op == CC_OP_ADDB || op == CC_OP_ADDW || op == CC_OP_ADDL ||
op == CC_OP_ADDB || op == CC_OP_ADDW || op == CC_OP_ADDL) {
 ^

Reported-by: Clang Static Analyzer
Signed-off-by: Philippe Mathieu-Daudé 
Reviewed-by: Laurent Vivier 
---
 target/m68k/translate.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/m68k/translate.c b/target/m68k/translate.c
index ada2a91b64..be24355080 100644
--- a/target/m68k/translate.c
+++ b/target/m68k/translate.c
@@ -1321,7 +1321,7 @@ static void gen_cc_cond(DisasCompare *c, DisasContext *s, 
int cond)
 case 5: /* CS (C) */
 /* Some cases fold C into X.  */
 if (op == CC_OP_ADDB || op == CC_OP_ADDW || op == CC_OP_ADDL ||
-op == CC_OP_ADDB || op == CC_OP_ADDW || op == CC_OP_ADDL) {
+op == CC_OP_SUBB || op == CC_OP_SUBW || op == CC_OP_SUBL) {
 tcond = TCG_COND_NE;
 c->v1 = QREG_CC_X;
 goto done;
-- 
2.13.3

[Qemu-devel] [PATCH for 2.10 v2 09/20] vfio/pci: fix use of freed memory

[Philippe Mathieu-Daudé]

hw/vfio/pci.c:308:29: warning: Use of memory after it is freed
qemu_set_fd_handler(*pfd, NULL, NULL, vdev);
^~~~

Reported-by: Clang Static Analyzer
Signed-off-by: Philippe Mathieu-Daudé 
Reviewed-by: Paolo Bonzini 
---
 hw/vfio/pci.c | 11 +++
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index d4051cb951..31e1edf447 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -257,7 +257,7 @@ static void vfio_intx_update(PCIDevice *pdev)
 static int vfio_intx_enable(VFIOPCIDevice *vdev, Error **errp)
 {
 uint8_t pin = vfio_pci_read_config(&vdev->pdev, PCI_INTERRUPT_PIN, 1);
-int ret, argsz;
+int ret, argsz, retval = 0;
 struct vfio_irq_set *irq_set;
 int32_t *pfd;
 Error *err = NULL;
@@ -302,12 +302,12 @@ static int vfio_intx_enable(VFIOPCIDevice *vdev, Error 
**errp)
 qemu_set_fd_handler(*pfd, vfio_intx_interrupt, NULL, vdev);
 
 ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set);
-g_free(irq_set);
 if (ret) {
 error_setg_errno(errp, -ret, "failed to setup INTx fd");
 qemu_set_fd_handler(*pfd, NULL, NULL, vdev);
 event_notifier_cleanup(&vdev->intx.interrupt);
-return -errno;
+retval = -errno;
+goto cleanup;
 }
 
 vfio_intx_enable_kvm(vdev, &err);
@@ -319,7 +319,10 @@ static int vfio_intx_enable(VFIOPCIDevice *vdev, Error 
**errp)
 
 trace_vfio_intx_enable(vdev->vbasedev.name);
 
-return 0;
+cleanup:
+g_free(irq_set);
+
+return retval;
 }
 
 static void vfio_intx_disable(VFIOPCIDevice *vdev)
-- 
2.13.3

[Qemu-devel] [PATCH for 2.10 v2 06/20] ui/vnc: fix leak of SocketAddress **

[Philippe Mathieu-Daudé]

Extract the (correct) cleaning code as a new function vnc_free_addresses() then
use it to remove the memory leaks.

Reported-by: Clang Static Analyzer
Signed-off-by: Philippe Mathieu-Daudé 
Reviewed-by: Daniel P. Berrange 
---
 ui/vnc.c | 36 ++--
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/ui/vnc.c b/ui/vnc.c
index eb91559b6b..651cbb8606 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -3521,6 +3521,20 @@ static int vnc_display_get_address(const char *addrstr,
 return ret;
 }
 
+static void vnc_free_addresses(SocketAddress ***retsaddr,
+   size_t *retnsaddr)
+{
+size_t i;
+
+for (i = 0; i < *retnsaddr; i++) {
+qapi_free_SocketAddress((*retsaddr)[i]);
+}
+g_free(*retsaddr);
+
+*retsaddr = NULL;
+*retnsaddr = 0;
+}
+
 static int vnc_display_get_addresses(QemuOpts *opts,
  bool reverse,
  SocketAddress ***retsaddr,
@@ -3538,7 +3552,6 @@ static int vnc_display_get_addresses(QemuOpts *opts,
 bool has_ipv6 = qemu_opt_get(opts, "ipv6");
 bool ipv4 = qemu_opt_get_bool(opts, "ipv4", false);
 bool ipv6 = qemu_opt_get_bool(opts, "ipv6", false);
-size_t i;
 int displaynum = -1;
 int ret = -1;
 
@@ -3614,16 +3627,8 @@ static int vnc_display_get_addresses(QemuOpts *opts,
 ret = 0;
  cleanup:
 if (ret < 0) {
-for (i = 0; i < *retnsaddr; i++) {
-qapi_free_SocketAddress((*retsaddr)[i]);
-}
-g_free(*retsaddr);
-for (i = 0; i < *retnwsaddr; i++) {
-qapi_free_SocketAddress((*retwsaddr)[i]);
-}
-g_free(*retwsaddr);
-*retsaddr = *retwsaddr = NULL;
-*retnsaddr = *retnwsaddr = 0;
+vnc_free_addresses(retsaddr, retnsaddr);
+vnc_free_addresses(retwsaddr, retnwsaddr);
 }
 return ret;
 }
@@ -3772,7 +3777,6 @@ void vnc_display_open(const char *id, Error **errp)
 int acl = 0;
 int lock_key_sync = 1;
 int key_delay_ms;
-size_t i;
 
 if (!vd) {
 error_setg(errp, "VNC display not active");
@@ -3993,12 +3997,8 @@ void vnc_display_open(const char *id, Error **errp)
 }
 
  cleanup:
-for (i = 0; i < nsaddr; i++) {
-qapi_free_SocketAddress(saddr[i]);
-}
-for (i = 0; i < nwsaddr; i++) {
-qapi_free_SocketAddress(wsaddr[i]);
-}
+vnc_free_addresses(&saddr, &nsaddr);
+vnc_free_addresses(&wsaddr, &nwsaddr);
 return;
 
 fail:
-- 
2.13.3

[Qemu-devel] [PATCH for 2.10 v2 05/20] qcow2: fix null pointer dereference

[Philippe Mathieu-Daudé]

It seems this assert() was somehow misplaced.

block/qcow2-refcount.c:2193:42: warning: Array access (from variable 
'on_disk_reftable') results in a null pointer dereference
on_disk_reftable[refblock_index] = refblock_offset;
 ^

Reported-by: Clang Static Analyzer
Signed-off-by: Philippe Mathieu-Daudé 
Reviewed-by: Eric Blake 
---
 block/qcow2-refcount.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index c9b0dcb4f3..168fc32e7b 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -2189,6 +2189,8 @@ write_refblocks:
  * this will leak that range, but we can easily fix that by running
  * a leak-fixing check after this rebuild operation */
 reftable_offset = -1;
+} else {
+assert(on_disk_reftable);
 }
 on_disk_reftable[refblock_index] = refblock_offset;
 
@@ -2258,8 +2260,6 @@ write_refblocks:
 goto write_refblocks;
 }
 
-assert(on_disk_reftable);
-
 for (refblock_index = 0; refblock_index < reftable_size; refblock_index++) 
{
 cpu_to_be64s(&on_disk_reftable[refblock_index]);
 }
-- 
2.13.3

[Qemu-devel] [PATCH for 2.10 v2 08/20] vfio/platform: fix use of freed memory

[Philippe Mathieu-Daudé]

free the data _after_ using it.

hw/vfio/platform.c:126:29: warning: Use of memory after it is freed
qemu_set_fd_handler(*pfd, NULL, NULL, NULL);
^~~~

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

diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c
index 7c09deda61..da84abf4fc 100644
--- a/hw/vfio/platform.c
+++ b/hw/vfio/platform.c
@@ -120,11 +120,11 @@ static int vfio_set_trigger_eventfd(VFIOINTp *intp,
 *pfd = event_notifier_get_fd(intp->interrupt);
 qemu_set_fd_handler(*pfd, (IOHandler *)handler, NULL, intp);
 ret = ioctl(vbasedev->fd, VFIO_DEVICE_SET_IRQS, irq_set);
-g_free(irq_set);
 if (ret < 0) {
 error_report("vfio: Failed to set trigger eventfd: %m");
 qemu_set_fd_handler(*pfd, NULL, NULL, NULL);
 }
+g_free(irq_set);
 return ret;
 }
 
-- 
2.13.3

[Qemu-devel] [PATCH for 2.10 v2 15/20] thunk: assert nb_fields is valid

[Philippe Mathieu-Daudé]

thunk.c:91:32: warning: Call to 'malloc' has an allocation size of 0 bytes
se->field_offsets[i] = malloc(nb_fields * sizeof(int));
   ^~~

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

diff --git a/thunk.c b/thunk.c
index 2dac3d..d5d8645cd4 100644
--- a/thunk.c
+++ b/thunk.c
@@ -67,7 +67,6 @@ void thunk_register_struct(int id, const char *name, const 
argtype *types)
 int nb_fields, offset, max_align, align, size, i, j;
 
 assert(id < max_struct_entries);
-se = struct_entries + id;
 
 /* first we count the number of fields */
 type_ptr = types;
@@ -76,6 +75,8 @@ void thunk_register_struct(int id, const char *name, const 
argtype *types)
 type_ptr = thunk_type_next(type_ptr);
 nb_fields++;
 }
+assert(nb_fields > 0);
+se = struct_entries + id;
 se->field_types = types;
 se->nb_fields = nb_fields;
 se->name = name;
-- 
2.13.3

[Qemu-devel] [PATCH for 2.10 v2 04/20] nbd: fix memory leak in nbd_opt_go()

[Philippe Mathieu-Daudé]

nbd/client.c:385:12: warning: Potential leak of memory pointed to by 'buf'

Reported-by: Clang Static Analyzer
Signed-off-by: Philippe Mathieu-Daudé 
Reviewed-by: Eric Blake 
---
 nbd/client.c | 8 +---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/nbd/client.c b/nbd/client.c
index 509ed5e4ba..0a17de80b5 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -376,9 +376,11 @@ static int nbd_opt_go(QIOChannel *ioc, const char 
*wantname,
 if (info->request_sizes) {
 stw_be_p(buf + 4 + len + 2, NBD_INFO_BLOCK_SIZE);
 }
-if (nbd_send_option_request(ioc, NBD_OPT_GO,
-4 + len + 2 + 2 * info->request_sizes, buf,
-errp) < 0) {
+error = nbd_send_option_request(ioc, NBD_OPT_GO,
+4 + len + 2 + 2 * info->request_sizes,
+buf, errp);
+g_free(buf);
+if (error < 0) {
 return -1;
 }
 
-- 
2.13.3

[Qemu-devel] [PATCH for 2.10 v2 00/20] fix bugs reported by Clang Static Analyzer

[Philippe Mathieu-Daudé]

Hi,

This series is the result of [now NOT] having fun with Clang's Static Analyzer
(see https://clang-analyzer.llvm.org/).

v2:
- addressed review feedbacks,
- added various R-b,
- dropped noise (Peter sharp eye),
- dropped dup patches

Patches 1-13 are already reviewed,
Patches 14,15 address feedbacks from v1,
Patch 16 test if patch collecting tools can handle missing patches,
Patches 17-19 are new,
Patch 20 is new but not very important ;) bonus that can wait 2.11.

Regards,

Phil.

v1:

Patch 1 was in another series (delayed for 2.11), it would be nice to have it
in 2.10.

I ran Clang static analyzer "scan-build" via a docker image based on
debian/unstable to use bleeding code, if one is interested in reproduce or use
it the Dockerfile is available here:
http://lists.nongnu.org/archive/html/qemu-devel/2017-07/msg07487.html

I used the following commands:

$ ../configure --host-cc=clang-5.0 --cc=clang-5.0 --cxx=clang++-5.0 \
  --disable-docs --enable-debug
$ scan-build-5.0 -o testresults --keep-going -maxloop 2 -no-failure-reports \
  -analyzer-config stable-report-filename=true \
  -disable-checker alpha.clone.CloneChecker \
  -enable-checker alpha.core.CastSize \
  -enable-checker alpha.core.Conversion \
  -enable-checker alpha.core.IdenticalExpr \
  -enable-checker alpha.core.SizeofPtr \
  -disable-checker alpha.deadcode.UnreachableCode \
  -enable-checker alpha.security.ArrayBoundV2 \
  -enable-checker alpha.security.MallocOverflow \
  -enable-checker alpha.unix.cstring.BufferOverlap \
  -enable-checker alpha.unix.cstring.OutOfBounds \
  -disable-checker deadcode.DeadStores \
  -disable-checker optin.performance.Padding \
  -enable-checker optin.portability.UnixAPI \
  -disable-checker security.insecureAPI.getpw \
  -disable-checker security.insecureAPI.gets \
  -enable-checker security.insecureAPI.strcpy \
  -disable-checker unix.Vfork \
make -k -j4

Philippe Mathieu-Daudé (20):
  tests: add missing dependency to build QTEST_QEMU_BINARY
  loader: check get_image_size() return value
  ivshmem: fix incorrect error handling in ivshmem_recv_msg()
  nbd: fix memory leak in nbd_opt_go()
  qcow2: fix null pointer dereference
  ui/vnc: fix leak of SocketAddress **
  net/eth: fix incorrect check of iov_to_buf() return value
  vfio/platform: fix use of freed memory
  vfio/pci: fix use of freed memory
  m68k/translate: fix incorrect copy/paste
  linux-user/sh4: fix incorrect memory write
  syscall: fix dereference of undefined pointer
  syscall: fix use of uninitialized values
  syscall: check inotify() and eventfd() return value
  thunk: assert nb_fields is valid
  bt-sdp: fix memory leak in sdp_service_record_build()
  9pfs: avoid sign conversion error simplifying the code
  spapr_vio: fix overflow of qdevs in spapr_dt_vdevice()
  i2c/exynos4210: fix write to I2CADD register, bit 0 is not mapped

 block/qcow2-refcount.c  |  4 +--
 hw/9pfs/9p.c|  6 ++---
 hw/bt/sdp.c |  6 +++--
 hw/core/loader.c|  4 +--
 hw/i2c/exynos4210_i2c.c |  2 +-
 hw/misc/ivshmem.c   |  5 +++-
 hw/ppc/spapr_vio.c  |  4 +--
 hw/vfio/pci.c   | 11 ++---
 hw/vfio/platform.c  |  2 +-
 linux-user/elfload.c|  2 +-
 linux-user/syscall.c| 18 +++---
 nbd/client.c|  8 +++---
 net/eth.c   |  4 +--
 target/m68k/translate.c |  2 +-
 tests/Makefile.include  |  2 +-
 thunk.c |  3 ++-
 ui/vnc.c| 66 -
 17 files changed, 83 insertions(+), 66 deletions(-)

-- 
2.13.3

[Qemu-devel] [PATCH for 2.10 v2 11/20] linux-user/sh4: fix incorrect memory write

[Philippe Mathieu-Daudé]

not hit since 2009! :)

linux-user/elfload.c:1102:20: warning: Out of bound memory access (access 
exceeds upper limit of memory block)
(*regs[i]) = tswap32(env->gregs[i]);
~~~^~~~

Reported-by: Clang Static Analyzer
Signed-off-by: Philippe Mathieu-Daudé 
Reviewed-by: Laurent Vivier 
---
 linux-user/elfload.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index 2a902f7806..79062882ba 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -1099,7 +1099,7 @@ static inline void 
elf_core_copy_regs(target_elf_gregset_t *regs,
 int i;
 
 for (i = 0; i < 16; i++) {
-(*regs[i]) = tswapreg(env->gregs[i]);
+(*regs)[i] = tswapreg(env->gregs[i]);
 }
 
 (*regs)[TARGET_REG_PC] = tswapreg(env->pc);
-- 
2.13.3

[Qemu-devel] [PATCH for 2.10 v2 02/20] loader: check get_image_size() return value

[Philippe Mathieu-Daudé]

since a negative value means it errored.

hw/core/loader.c:149:9: warning: Loss of sign in implicit conversion
if (size > max_sz) {
^~~~
hw/core/loader.c:171:9: warning: Loss of sign in implicit conversion
if (size > memory_region_size(mr)) {
^~~~

Reported-by: Clang Static Analyzer
Signed-off-by: Philippe Mathieu-Daudé 
Reviewed-by: Eric Blake 
Reviewed-by: Alistair Francis 
---
 hw/core/loader.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/core/loader.c b/hw/core/loader.c
index c17ace0a2e..4bb176f284 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -146,7 +146,7 @@ int load_image_targphys_as(const char *filename,
 int size;
 
 size = get_image_size(filename);
-if (size > max_sz) {
+if (size < 0 || size > max_sz) {
 return -1;
 }
 if (size > 0) {
@@ -168,7 +168,7 @@ int load_image_mr(const char *filename, MemoryRegion *mr)
 
 size = get_image_size(filename);
 
-if (size > memory_region_size(mr)) {
+if (size < 0 || size > memory_region_size(mr)) {
 return -1;
 }
 if (size > 0) {
-- 
2.13.3

[Qemu-devel] [PATCH for 2.10 v2 01/20] tests: add missing dependency to build QTEST_QEMU_BINARY

[Philippe Mathieu-Daudé]

This allow a one liner from fresh repository clone, i.e.:

  ./configure && make -j check-qtest-aarch64

Signed-off-by: Philippe Mathieu-Daudé 
Reviewed-by: Alex Bennée 
Reviewed-by: Stefan Hajnoczi 
Reviewed-by: John Snow 
---
 tests/Makefile.include | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/Makefile.include b/tests/Makefile.include
index 7af278db55..b55fe39d94 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -830,7 +830,7 @@ GCOV_OPTIONS = -n $(if $(V),-f,)
 # gtester tests, possibly with verbose output
 
 .PHONY: $(patsubst %, check-qtest-%, $(QTEST_TARGETS))
-$(patsubst %, check-qtest-%, $(QTEST_TARGETS)): check-qtest-%: $(check-qtest-y)
+$(patsubst %, check-qtest-%, $(QTEST_TARGETS)): check-qtest-%: 
subdir-%-softmmu $(check-qtest-y)
$(if $(CONFIG_GCOV),@rm -f *.gcda */*.gcda */*/*.gcda */*/*/*.gcda,)
$(call quiet-command,QTEST_QEMU_BINARY=$*-softmmu/qemu-system-$* \
QTEST_QEMU_IMG=qemu-img$(EXESUF) \
-- 
2.13.3

[Qemu-devel] [PATCH for 2.10 v2 03/20] ivshmem: fix incorrect error handling in ivshmem_recv_msg()

[Philippe Mathieu-Daudé]

Screwed up in commit 3a55fc0f, v2.6.0.

If qemu_chr_fe_read_all() returns -EINTR the do {} statement continues and the
n accumulator used to complete reads upto sizeof(msg) is decremented by 4 (the
value of EINTR on Linux).
To avoid that, use simpler if() statements and continue if EINTR occured.

hw/misc/ivshmem.c:650:14: warning: Loss of sign in implicit conversion
} while (n < sizeof(msg));
 ^

Reported-by: Clang Static Analyzer
Signed-off-by: Philippe Mathieu-Daudé 
Reviewed-by: Markus Armbruster 
---
 hw/misc/ivshmem.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
index a58f9ee579..47a015f072 100644
--- a/hw/misc/ivshmem.c
+++ b/hw/misc/ivshmem.c
@@ -642,7 +642,10 @@ static int64_t ivshmem_recv_msg(IVShmemState *s, int *pfd, 
Error **errp)
 do {
 ret = qemu_chr_fe_read_all(&s->server_chr, (uint8_t *)&msg + n,
sizeof(msg) - n);
-if (ret < 0 && ret != -EINTR) {
+if (ret < 0) {
+if (ret == -EINTR) {
+continue;
+}
 error_setg_errno(errp, -ret, "read from server failed");
 return INT64_MIN;
 }
-- 
2.13.3

Re: [Qemu-devel] Commit 77af8a2b95b79699de650965d5228772743efe84 breaks Windows 2000 support

[Programmingkid]

> On Jul 26, 2017, at 10:28 AM, qemu-devel-requ...@nongnu.org wrote:
> 
> Message: 3
> Date: Wed, 26 Jul 2017 15:30:20 +0200
> From: Igor Mammedov 
> To: Paolo Bonzini 
> Cc: Laszlo Ersek , Phil Dennis-Jordan
>   , "Daniel P. Berrange" ,
>   PhilDennis-Jordan , ehabk...@redhat.com,
>   "qemu-devel@nongnu.org qemu-devel" ,
>   Programmingkid  , Richard Henderson
>   , "MichaelS. Tsirkin" 
> Subject: Re: [Qemu-devel] Commit
>   77af8a2b95b79699de650965d5228772743efe84 breaks Windows 2000 support
> Message-ID: <20170726153020.4c20b...@nial.brq.redhat.com>
> Content-Type: text/plain; charset=US-ASCII
> 
> On Wed, 26 Jul 2017 15:10:40 +0200
> Paolo Bonzini  wrote:
> 
>> On 26/07/2017 15:08, Igor Mammedov wrote:
>>> On Tue, 25 Jul 2017 18:23:22 +0200
>>> Paolo Bonzini  wrote:
>>> 
 On 25/07/2017 18:14, Laszlo Ersek wrote:  
>  "No regressions became apparent in tests with a range of Windows
>   (XP-10)"
> 
> In theory, w2k falls within that range.
 
 Nope, Windows 2000 is like NT 5.0, XP is like NT 5.1. :(
 
 One possibility is to fix it in SeaBIOS instead: if you get a 2.0 FADT
 and an XSDT and no RSDT, it can build an RSDT and a 1.0 FADT itself,
 patching the RSDT to point to it.
 
 It's a hack, but it's the only place I see to make it "just work".  And
 it could be extended nicely in the future.
 
 All QEMU would have to do is to provide an XSDT _instead_ of an RSDT.  
>>> I'd support it, however it would break migrated guests with old BIOS
>>> image in RAM on reboot.  
>> 
>> Why?  Shouldn't the old ACPI tables get migrated together with the old
>> BIOS?  Or are they rebuilt after reset?
> they are rebuild on reset, but I've been wrong
> Looking at SeaBIOS something similar to your suggestion also should work,
> if 
>RsdpAddr = find_acpi_rsdp();
> fails, current SeaBIOS falls back to its own ACPI tables.
> 
> but it seems that we don't even need to go to that extent,
> all user have to do is to use "-no-acpi" CLI option with QEMU
> for any SeaBIOS to fallback to embedded legacy ACPI tables.
> 
> Maybe we should just fix wiki
>  http://wiki.qemu.org/Windows2000
> to recommend using '-no-acpi' option when running w2k and
> leave PC machine at rev3 and mention it in release notes.
> 
> Opinions?

I just tried booting Windows 2000 using '-no-acpi' and ended up in a restart 
loop.
Windows would look like its booting up properly then the screen is blanked.
Seabios text would appear on the screen for a fraction of a 
second then be replaced by the Windows startup screen again. This repeats over 
and
over.

[Qemu-devel] [PATCH v2] migration: optimize the downtime

[Jay Zhou]

Qemu_savevm_state_cleanup takes about 300ms in my ram migration tests
with a 8U24G vm(20G is really occupied), the main cost comes from
KVM_SET_USER_MEMORY_REGION ioctl when mem.memory_size = 0 in
kvm_set_user_memory_region. In kmod, the main cost is
kvm_zap_obsolete_pages, which traverses the active_mmu_pages list to
zap the unsync sptes.

It can be optimized by delaying memory_global_dirty_log_stop to the next
vm_start.

Changes v1->v2:
 - create a VMChangeStateHandler in memory.c to reduce the coupling [Paolo]

Signed-off-by: Jay Zhou 
---
 memory.c | 27 ++-
 1 file changed, 26 insertions(+), 1 deletion(-)

diff --git a/memory.c b/memory.c
index a7bc70a..4c22b7e 100644
--- a/memory.c
+++ b/memory.c
@@ -2357,8 +2357,14 @@ void memory_global_dirty_log_sync(void)
 }
 }
 
+static VMChangeStateEntry *vmstate_change;
+
 void memory_global_dirty_log_start(void)
 {
+if (vmstate_change) {
+qemu_del_vm_change_state_handler(vmstate_change);
+}
+
 global_dirty_log = true;
 
 MEMORY_LISTENER_CALL_GLOBAL(log_global_start, Forward);
@@ -2369,7 +2375,7 @@ void memory_global_dirty_log_start(void)
 memory_region_transaction_commit();
 }
 
-void memory_global_dirty_log_stop(void)
+static void memory_global_dirty_log_do_stop(void)
 {
 global_dirty_log = false;
 
@@ -2381,6 +2387,25 @@ void memory_global_dirty_log_stop(void)
 MEMORY_LISTENER_CALL_GLOBAL(log_global_stop, Reverse);
 }
 
+static void memory_vm_change_state_handler(void *opaque, int running,
+   RunState state)
+{
+if (running) {
+memory_global_dirty_log_do_stop();
+}
+}
+
+void memory_global_dirty_log_stop(void)
+{
+if (!runstate_is_running()) {
+vmstate_change = qemu_add_vm_change_state_handler(
+memory_vm_change_state_handler, NULL);
+return;
+}
+
+memory_global_dirty_log_do_stop();
+}
+
 static void listener_add_address_space(MemoryListener *listener,
AddressSpace *as)
 {
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v8 3/3] migration: add bitmap for received page

[Peter Xu]

On Wed, Jul 26, 2017 at 06:24:11PM +0300, Alexey Perevalov wrote:
> On 07/26/2017 11:43 AM, Peter Xu wrote:
> >On Wed, Jul 26, 2017 at 11:07:17AM +0300, Alexey Perevalov wrote:
> >>On 07/26/2017 04:49 AM, Peter Xu wrote:
> >>>On Thu, Jul 20, 2017 at 09:52:34AM +0300, Alexey Perevalov wrote:
> This patch adds ability to track down already received
> pages, it's necessary for calculation vCPU block time in
> postcopy migration feature, maybe for restore after
> postcopy migration failure.
> Also it's necessary to solve shared memory issue in
> postcopy livemigration. Information about received pages
> will be transferred to the software virtual bridge
> (e.g. OVS-VSWITCHD), to avoid fallocate (unmap) for
> already received pages. fallocate syscall is required for
> remmaped shared memory, due to remmaping itself blocks
> ioctl(UFFDIO_COPY, ioctl in this case will end with EEXIT
> error (struct page is exists after remmap).
> 
> Bitmap is placed into RAMBlock as another postcopy/precopy
> related bitmaps.
> 
> Reviewed-by: Peter Xu 
> Signed-off-by: Alexey Perevalov 
> ---
> >>>[...]
> >>>
>   static int qemu_ufd_copy_ioctl(int userfault_fd, void *host_addr,
> -void *from_addr, uint64_t pagesize)
> +   void *from_addr, uint64_t pagesize, 
> RAMBlock *rb)
>   {
> +int ret;
>   if (from_addr) {
>   struct uffdio_copy copy_struct;
>   copy_struct.dst = (uint64_t)(uintptr_t)host_addr;
>   copy_struct.src = (uint64_t)(uintptr_t)from_addr;
>   copy_struct.len = pagesize;
>   copy_struct.mode = 0;
> -return ioctl(userfault_fd, UFFDIO_COPY, ©_struct);
> +ret = ioctl(userfault_fd, UFFDIO_COPY, ©_struct);
>   } else {
>   struct uffdio_zeropage zero_struct;
>   zero_struct.range.start = (uint64_t)(uintptr_t)host_addr;
>   zero_struct.range.len = pagesize;
>   zero_struct.mode = 0;
> -return ioctl(userfault_fd, UFFDIO_ZEROPAGE, &zero_struct);
> +ret = ioctl(userfault_fd, UFFDIO_ZEROPAGE, &zero_struct);
> +}
> +if (!ret) {
> +ramblock_recv_bitmap_set(host_addr, rb);
> >>>Wait...
> >>>
> >>>Now we are using 4k-page/bit bitmap, do we need to take care of the
> >>>huge pages here?  Looks like we are only setting the first bit of it
> >>>if it is a huge page?
> >>First version was per ramblock page size, IOW bitmap was smaller in
> >>case of hugepages.
> >Yes, but this is not the first version any more. :)
> >
> >This patch is using:
> >
> >   bitmap_new(rb->max_length >> TARGET_PAGE_BITS);
> >
> >to allocate bitmap, so it is using small pages always for bitmap,
> >right? (I should not really say "4k" pages, here I think the size is
> >host page size, which is the thing returned from getpagesize()).
> >
> >>
> >>You mentioned that TARGET_PAGE_SIZE is reasonable for precopy case,
> >>in "Re: [Qemu-devel] [PATCH v1 2/2] migration: add bitmap for copied page"
> >>I though TARGET_PAGE_SIZE as transmition unit, is using in precopy even
> >>hugepage case.
> >>But it's not so logically, page being marked as dirty, should be sent as a
> >>whole page.
> >Sorry if I misunderstood, but I didn't see anything wrong - we are
> >sending pages in small pages, but when postcopy is there, we do
> >UFFDIO_COPY in huge page, so everything is fine?
> I think yes, we chose TARGET_PAGE_SIZE because of wider
> use case ranges.

So... are you going to post another version? IIUC we just need to use
a bitmap_set() to replace the ramblock_recv_bitmap_set(), while set
the size with "pagesize / TARGET_PAGE_SIZE"?

(I think I was wrong when saying getpagesize() above: the small page
 should be target page size, while the huge page should be the host's)

-- 
Peter Xu

Re: [Qemu-devel] [PATCH V3] rtc: fix a infinite loop inwindowsvmstartup

[peng.hao2]

> On 26/07/2017 03:28, peng.h...@zte.com.cn wrote:




> > 
> > 
> > when the problem happens , windows kernel is checking  whether REG_A_UIP is 
> > changing after periodic timer has stopped. windows kernel access REG_A
> > according to INB instrunction and it will spend several microseconds because
> > of VM_EXIT. 

> A vmexit to the RTC timer should be around 10.000 clock cycles, which is
> less than a microsecond.
I use perf tools getting data(windows VM)

IO Port   AccessSamples  Samples% Time%   Min Time   Max Time 
Avg time 

0x71:  PIN   126 56.76%   32.62%1us 2us 
 1.83us ( +-   0.62% )


0x70:POUT 63  28.38%51.56%4us 15us  
   5.78us ( +-   3.12% )

But I'm not sure it is accurate.

> > update timer has changed to a long expire time (as alarm timer)on the
> > one hand.
> > 
> > on the other hand  244 microseconds in one second is too short to hit
> > the region.
> > 
> > windows kernel may check REG_A_UIP when considering RTC something wrong.
> > many windows VM reboot at the same time and rtc periodic timer may delay
> > badly..

> Does Windows do this test when Hyper-V englightenments are enabled
> (especially hv-relaxed)?
yes ,I enable hv-relaxed.in my windows VM.But I'm not sure the interconnection 
between them.

> It seems to be a Windows issue to me.  I'm not sure adding hacks to the

> device model is the right thing to do, especially because I don't
> understand why your fix worked.

1. guest closes periodic timer，from then on rtc don't send interrupt anymore 
and no rtc handler can run to read


   RTC_REG_C（this read set RTC_REG_C = 0, clear REG_C_UF)


2. update timer expires, rtc_update_timer clears REG_A_UIP, and set REG_C_UF


3. because REG_C_UF is setted, check_update_timer sets update_timer's expire 
time to next_alarm_time, which is 


   over ten hours


4. guest checks that REG_A_UIP changes from 0 to 1 in a infinte loop (we don't 
know why windows kernel does this) by read RTC_REG_A


5. update_in_progress called from cmos_ioport_read


6. beause update_timer is switched to alarm timer, which has a too big expire 
time to satisfes the fowllowing condition:


if (qemu_clock_get_ns(rtc_clock) >=


(next_update_time - UIP_HOLD_LENGTH)) {


s->cmos_data[RTC_REG_A] |= REG_A_UIP


return 1


}


7. update_in_progress's second condition don't satisfes:


if ((guest_nsec % NANOSECONDS_PER_SECOND) >=


(NANOSECONDS_PER_SECOND - UIP_HOLD_LENGTH)) {


return 1


}


   because when many vms startup, inb instruction wastes much more time than 
normal situation and 


   guest_nsec % NANOSECONDS_PER_SECOND is nearly < NANOSECONDS_PER_SECOND - 
UIP_HOLD_LENGTH always.


8. finally, REG_A_UIP is always cleared, REG_C_UF is always set, update_timer 
is always alarm timer, 


   guest is always checking REG_A_UIP, and guest hung up.

Re: [Qemu-devel] [PATCH v3 4/4] intel_iommu: implement mru list for iotlb

[Peter Xu]

On Wed, Jul 26, 2017 at 11:37:13PM +0300, Michael S. Tsirkin wrote:
> On Mon, Jul 17, 2017 at 09:53:27AM +0800, Peter Xu wrote:
> > On Fri, Jul 14, 2017 at 03:28:09PM +0800, Jason Wang wrote:
> > > 
> > > 
> > > On 2017年07月14日 12:32, Peter Xu wrote:
> > > >On Thu, Jul 13, 2017 at 04:48:42PM +0800, Jason Wang wrote:
> > > >>
> > > >>On 2017年07月12日 16:13, Peter Xu wrote:
> > > >>>It is not wise to disgard all the IOTLB cache when cache size reaches
> > > >>>max, but that's what we do now. A slightly better (but still simple) 
> > > >>>way
> > > >>>to do this is, we just throw away the least recent used cache entry.
> > > >>>
> > > >>>This patch implemented MRU list algorithm for VT-d IOTLB. The main 
> > > >>>logic
> > > >>>is to maintain a Most Recently Used list for the IOTLB entries. The 
> > > >>>hash
> > > >>>table is still used for lookup, though a new list field is added to 
> > > >>>each
> > > >>>IOTLB entry for a iotlb MRU list. For each active IOTLB entry, it's 
> > > >>>both
> > > >>>in the hash table in s->iotlb, and also linked into the MRU list of
> > > >>>s->iotlb_head. The hash helps in fast lookup, and the MRU list helps in
> > > >>>knowing whether the cache is still hot.
> > > >>>
> > > >>>After we have such a MRU list, replacing all the iterators of IOTLB
> > > >>>entries by using list iterations rather than hash table iterations.
> > > >>Any reason of doing this, I thought hashtable was even a little bit 
> > > >>faster?
> > > >Could I ask why?
> > > >
> > > >I thought they are merely the same (when iterating all the items)?
> > > >
> > > 
> > > Ok, looks like I was wrong, but it they are merely the same, why bother?
> > 
> > Because imho looping over list needs fewer LOCs and is also more
> > direct. E.g., for domain flush, hash needs this:
> > 
> > static gboolean vtd_hash_remove_by_domain(gpointer key, gpointer value,
> >   gpointer user_data)
> > {
> > VTDIOTLBEntry *entry = (VTDIOTLBEntry *)value;
> > uint16_t domain_id = *(uint16_t *)user_data;
> > return entry->domain_id == domain_id;
> > }
> > 
> > Then:
> > 
> > g_hash_table_foreach_remove(s->iotlb, vtd_hash_remove_by_domain,
> > &domain_id);
> > 
> > For list it is only:
> > 
> > FOREACH_IOTLB_SAFE(entry, s, entry_n) {
> > if (entry->domain_id == domain_id) {
> > vtd_iotlb_remove_entry(s, entry);
> > }
> > }
> > 
> > Thanks,
> 
> Well the LOC seems to have gone up with this patch.
> If we are trying to simplify code, please state this
> in commit log.

Ok. I can split the patch into two (one to introduce MRU list, one to
replace the iterations of hash tables) if it will be needed in the
future. Currently since we don't have plan to merge it, I'll put it
aside for now. Thanks.

-- 
Peter Xu

[Qemu-devel] [PATCH] tcg/README: fix a description error.

[Jiang Biao]

The atomics.txt is not in the docs directory but in docs/devel/
instead.

Signed-off-by: Jiang Biao 
---
 tcg/README | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tcg/README b/tcg/README
index bf49e82..03bfb6a 100644
--- a/tcg/README
+++ b/tcg/README
@@ -446,7 +446,7 @@ when MTTCG is enabled.
 The guest translators should generate this opcode for all guest instructions
 which have ordering side effects.
 
-Please see docs/atomics.txt for more information on memory barriers.
+Please see docs/devel/atomics.txt for more information on memory barriers.
 
 * 64-bit guest on 32-bit host support
 
-- 
2.7.4

[Qemu-devel] [PATCH] fsdev: fix memory leak in main()

[ZhiPeng Lu]

@rpath and @ sock_name are not freed and leaked.

Signed-off-by: Zhipeng Lu lu.zhip...@zte.com.cn
---
 fsdev/virtfs-proxy-helper.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fsdev/virtfs-proxy-helper.c b/fsdev/virtfs-proxy-helper.c
index 6c066ec..8e48500 100644
--- a/fsdev/virtfs-proxy-helper.c
+++ b/fsdev/virtfs-proxy-helper.c
@@ -1162,6 +1162,8 @@ int main(int argc, char **argv)
 
 process_requests(sock);
 error:
+g_free(rpath);
+g_free(sock_name);
 do_log(LOG_INFO, "Done\n");
 closelog();
 return 0;
-- 
1.8.3.1

[Qemu-devel] [PATCH 2/3] s390x/css: generate solicited crw for rchp completion signaling

[Dong Jia Shi]

A successful completion of rchp should signal a solicited channel path
initialized CRW (channel report word), while the current implementation
always generates an un-solicited one. Let's fix this.

Reported-by: Halil Pasic 
Signed-off-by: Dong Jia Shi 
---
 hw/s390x/css.c | 15 +--
 include/hw/s390x/css.h |  2 +-
 2 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/hw/s390x/css.c b/hw/s390x/css.c
index 5321ca016b..60e1592d5c 100644
--- a/hw/s390x/css.c
+++ b/hw/s390x/css.c
@@ -1745,10 +1745,10 @@ int css_do_rchp(uint8_t cssid, uint8_t chpid)
 }
 
 /* We don't really use a channel path, so we're done here. */
-css_queue_crw(CRW_RSC_CHP, CRW_ERC_INIT,
+css_queue_crw(CRW_RSC_CHP, CRW_ERC_INIT, 1,
   channel_subsys.max_cssid > 0 ? 1 : 0, chpid);
 if (channel_subsys.max_cssid > 0) {
-css_queue_crw(CRW_RSC_CHP, CRW_ERC_INIT, 0, real_cssid << 8);
+css_queue_crw(CRW_RSC_CHP, CRW_ERC_INIT, 1, 0, real_cssid << 8);
 }
 return 0;
 }
@@ -2028,7 +2028,7 @@ void css_subch_assign(uint8_t cssid, uint8_t ssid, 
uint16_t schid,
 }
 }
 
-void css_queue_crw(uint8_t rsc, uint8_t erc, int chain, uint16_t rsid)
+void css_queue_crw(uint8_t rsc, uint8_t erc, int s, int chain, uint16_t rsid)
 {
 CrwContainer *crw_cont;
 
@@ -2040,6 +2040,9 @@ void css_queue_crw(uint8_t rsc, uint8_t erc, int chain, 
uint16_t rsid)
 return;
 }
 crw_cont->crw.flags = (rsc << 8) | erc;
+if (s) {
+crw_cont->crw.flags |= CRW_FLAGS_MASK_S;
+}
 if (chain) {
 crw_cont->crw.flags |= CRW_FLAGS_MASK_C;
 }
@@ -2086,9 +2089,9 @@ void css_generate_sch_crws(uint8_t cssid, uint8_t ssid, 
uint16_t schid,
 }
 chain_crw = (channel_subsys.max_ssid > 0) ||
 (channel_subsys.max_cssid > 0);
-css_queue_crw(CRW_RSC_SUBCH, CRW_ERC_IPI, chain_crw ? 1 : 0, schid);
+css_queue_crw(CRW_RSC_SUBCH, CRW_ERC_IPI, 0, chain_crw ? 1 : 0, schid);
 if (chain_crw) {
-css_queue_crw(CRW_RSC_SUBCH, CRW_ERC_IPI, 0,
+css_queue_crw(CRW_RSC_SUBCH, CRW_ERC_IPI, 0, 0,
   (guest_cssid << 8) | (ssid << 4));
 }
 /* RW_ERC_IPI --> clear pending interrupts */
@@ -2103,7 +2106,7 @@ void css_generate_chp_crws(uint8_t cssid, uint8_t chpid)
 void css_generate_css_crws(uint8_t cssid)
 {
 if (!channel_subsys.sei_pending) {
-css_queue_crw(CRW_RSC_CSS, CRW_ERC_EVENT, 0, cssid);
+css_queue_crw(CRW_RSC_CSS, CRW_ERC_EVENT, 0, 0, cssid);
 }
 channel_subsys.sei_pending = true;
 }
diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h
index 5c5fe6b202..d03b4ffeac 100644
--- a/include/hw/s390x/css.h
+++ b/include/hw/s390x/css.h
@@ -150,7 +150,7 @@ void copy_scsw_to_guest(SCSW *dest, const SCSW *src);
 void css_inject_io_interrupt(SubchDev *sch);
 void css_reset(void);
 void css_reset_sch(SubchDev *sch);
-void css_queue_crw(uint8_t rsc, uint8_t erc, int chain, uint16_t rsid);
+void css_queue_crw(uint8_t rsc, uint8_t erc, int s, int chain, uint16_t rsid);
 void css_generate_sch_crws(uint8_t cssid, uint8_t ssid, uint16_t schid,
int hotplugged, int add);
 void css_generate_chp_crws(uint8_t cssid, uint8_t chpid);
-- 
2.11.2

[Qemu-devel] [PATCH 3/3] s390x/css: generate channel path initialized CRW for channel path hotplug

[Dong Jia Shi]

When a channel path is hot plugged into a CSS, we should generate
a channel path initialized CRW (channel report word). The current
code does not do that, instead it puts a stub function with a TODO
reminder there.

This implements the css_generate_chp_crws() function by:
1. refactor the existing code.
2. add an @add parameter to provide future callers with the
   capability of generating channel path permanent error with
   facility not initialized CRW.
3. add a @hotplugged parameter, so to opt out generating initialized
   CRWs for predefined channel paths.

Signed-off-by: Dong Jia Shi 
---
 hw/s390x/3270-ccw.c   |  3 ++-
 hw/s390x/css.c| 55 ---
 hw/s390x/s390-ccw.c   |  2 +-
 hw/s390x/virtio-ccw.c |  3 ++-
 include/hw/s390x/css.h|  8 ---
 include/hw/s390x/ioinst.h |  1 +
 6 files changed, 53 insertions(+), 19 deletions(-)

diff --git a/hw/s390x/3270-ccw.c b/hw/s390x/3270-ccw.c
index 1554aa2484..20305b8554 100644
--- a/hw/s390x/3270-ccw.c
+++ b/hw/s390x/3270-ccw.c
@@ -125,7 +125,8 @@ static void emulated_ccw_3270_realize(DeviceState *ds, 
Error **errp)
 sch->id.reserved = 0xff;
 sch->id.cu_type = EMULATED_CCW_3270_CU_TYPE;
 css_sch_build_virtual_schib(sch, (uint8_t)chpid,
-EMULATED_CCW_3270_CHPID_TYPE);
+EMULATED_CCW_3270_CHPID_TYPE,
+parent->hotplugged);
 sch->do_subchannel_work = do_subchannel_work_virtual;
 sch->ccw_cb = emulated_ccw_3270_cb;
 
diff --git a/hw/s390x/css.c b/hw/s390x/css.c
index 60e1592d5c..ffa93e8a62 100644
--- a/hw/s390x/css.c
+++ b/hw/s390x/css.c
@@ -1745,11 +1745,8 @@ int css_do_rchp(uint8_t cssid, uint8_t chpid)
 }
 
 /* We don't really use a channel path, so we're done here. */
-css_queue_crw(CRW_RSC_CHP, CRW_ERC_INIT, 1,
-  channel_subsys.max_cssid > 0 ? 1 : 0, chpid);
-if (channel_subsys.max_cssid > 0) {
-css_queue_crw(CRW_RSC_CHP, CRW_ERC_INIT, 1, 0, real_cssid << 8);
-}
+css_generate_chp_crws(cssid, chpid, 0, 1, 1);
+
 return 0;
 }
 
@@ -1791,7 +1788,7 @@ unsigned int css_find_free_chpid(uint8_t cssid)
 }
 
 static int css_add_chpid(uint8_t cssid, uint8_t chpid, uint8_t type,
- bool is_virt)
+ bool is_virt, int hotplugged)
 {
 CssImage *css;
 
@@ -1807,12 +1804,13 @@ static int css_add_chpid(uint8_t cssid, uint8_t chpid, 
uint8_t type,
 css->chpids[chpid].type = type;
 css->chpids[chpid].is_virtual = is_virt;
 
-css_generate_chp_crws(cssid, chpid);
+css_generate_chp_crws(cssid, chpid, hotplugged, 1, 0);
 
 return 0;
 }
 
-void css_sch_build_virtual_schib(SubchDev *sch, uint8_t chpid, uint8_t type)
+void css_sch_build_virtual_schib(SubchDev *sch, uint8_t chpid, uint8_t type,
+ int hotplugged)
 {
 PMCW *p = &sch->curr_status.pmcw;
 SCSW *s = &sch->curr_status.scsw;
@@ -1829,7 +1827,7 @@ void css_sch_build_virtual_schib(SubchDev *sch, uint8_t 
chpid, uint8_t type)
 p->pam = 0x80;
 p->chpid[0] = chpid;
 if (!css->chpids[chpid].in_use) {
-css_add_chpid(sch->cssid, chpid, type, true);
+css_add_chpid(sch->cssid, chpid, type, true, hotplugged);
 }
 
 memset(s, 0, sizeof(SCSW));
@@ -2098,9 +2096,40 @@ void css_generate_sch_crws(uint8_t cssid, uint8_t ssid, 
uint16_t schid,
 css_clear_io_interrupt(css_do_build_subchannel_id(cssid, ssid), schid);
 }
 
-void css_generate_chp_crws(uint8_t cssid, uint8_t chpid)
+void css_generate_chp_crws(uint8_t cssid, uint8_t chpid,
+   int hotplugged, int add, int s)
 {
-/* TODO */
+uint8_t guest_cssid;
+bool chain_crw;
+int erc;
+
+if (add && !s && !hotplugged) {
+return;
+}
+if (channel_subsys.max_cssid == 0) {
+/* Default cssid shows up as 0. */
+guest_cssid = (cssid == channel_subsys.default_cssid) ? 0 : cssid;
+} else {
+/* Show real cssid to the guest. */
+guest_cssid = cssid;
+}
+/*
+ * Only notify for higher subchannel sets/channel subsystems if the
+ * guest has enabled it.
+ */
+if ((guest_cssid > channel_subsys.max_cssid) ||
+((channel_subsys.max_cssid == 0) &&
+ (cssid != channel_subsys.default_cssid))) {
+return;
+}
+
+erc = add ? CRW_ERC_INIT : CRW_ERC_PERRN;
+chain_crw = channel_subsys.max_cssid > 0;
+
+css_queue_crw(CRW_RSC_CHP, erc, s, chain_crw ? 1 : 0, chpid);
+if (chain_crw) {
+css_queue_crw(CRW_RSC_CHP, erc, s, 0, cssid << 8);
+}
 }
 
 void css_generate_css_crws(uint8_t cssid)
@@ -2433,7 +2462,7 @@ static int css_sch_get_chpid_type(uint8_t chpid, uint32_t 
*type,
  * guest subchannel information block without considering the migration 
feature.
  * We need to revisit this problem when we want to add migration support.
  */
-int css_sch_build_schib(SubchDev *sch, CssDevId 

[Qemu-devel] [PATCH 0/3] Channel Path realted CRW generation

[Dong Jia Shi]

This series is trying to:
1. clear up CRW related code.
2. generate the right channel path related CRW at the right time.

I did this mainly because it's a requirement from my current work, that is I'm
in preparation of a group of patch for channel path virtualization. I can use
the inerface that provided by this series later, so as to, for vfio-ccw
devices, notify the guest with channel path status vary that happens on the
host side.

During an internal discussion, Halil and Pierre pointed out that for path
hotplug, generating a CRW seems logical, but how is it covered by the AR is not
clear - we have problem in understanding some grammar ambiguous paragraphs.
While certain parts of the AR is not available outside, but I'm still wondering
if the author ;) could give us some clue... BTW, we know that, in Linux kernel
we had code that handles un-solicited chp crw, so we tend to believe it's right
to generate channel path initialized CRW for path hotplug. It's just we can not
find the reason from the document.

Pierre also suggested to add an @erc param for css_generate_chp_crws() in 
patch3,
while others have a different opinion. This is for your consideration.

Best regards!

Dong Jia Shi (3):
  s390x/css: use macro for event-information pending error recover code
  s390x/css: generate solicited crw for rchp completion signaling
  s390x/css: generate channel path initialized CRW for channel path
hotplug

 hw/s390x/3270-ccw.c   |  3 ++-
 hw/s390x/css.c| 66 +++
 hw/s390x/s390-ccw.c   |  2 +-
 hw/s390x/virtio-ccw.c |  3 ++-
 include/hw/s390x/css.h| 10 ---
 include/hw/s390x/ioinst.h |  6 +++--
 6 files changed, 64 insertions(+), 26 deletions(-)

-- 
2.11.2

[Qemu-devel] [PATCH 1/3] s390x/css: use macro for event-information pending error recover code

[Dong Jia Shi]

Let's use a macro for the ERC (error recover code) when generating a
Channel Subsystem Event-information pending CRW (channel report word).

Signed-off-by: Dong Jia Shi 
---
 hw/s390x/css.c| 2 +-
 include/hw/s390x/ioinst.h | 5 +++--
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/hw/s390x/css.c b/hw/s390x/css.c
index 6a42b95cee..5321ca016b 100644
--- a/hw/s390x/css.c
+++ b/hw/s390x/css.c
@@ -2103,7 +2103,7 @@ void css_generate_chp_crws(uint8_t cssid, uint8_t chpid)
 void css_generate_css_crws(uint8_t cssid)
 {
 if (!channel_subsys.sei_pending) {
-css_queue_crw(CRW_RSC_CSS, 0, 0, cssid);
+css_queue_crw(CRW_RSC_CSS, CRW_ERC_EVENT, 0, cssid);
 }
 channel_subsys.sei_pending = true;
 }
diff --git a/include/hw/s390x/ioinst.h b/include/hw/s390x/ioinst.h
index 92d15655e4..c1d1052279 100644
--- a/include/hw/s390x/ioinst.h
+++ b/include/hw/s390x/ioinst.h
@@ -201,8 +201,9 @@ typedef struct CRW {
 #define CRW_FLAGS_MASK_A 0x0080
 #define CRW_FLAGS_MASK_ERC 0x003f
 
-#define CRW_ERC_INIT 0x02
-#define CRW_ERC_IPI  0x04
+#define CRW_ERC_EVENT  0x00
+#define CRW_ERC_INIT   0x02
+#define CRW_ERC_IPI0x04
 
 #define CRW_RSC_SUBCH 0x3
 #define CRW_RSC_CHP   0x4
-- 
2.11.2

[Qemu-devel] [Bug 1706825] [NEW] qemu-user fails to run wineserver on ppc64el host

[Timothy Pearson]

Public bug reported:

When attempting to run wineserver on a 64-bit ppc64el host via QEMU's
user-mode i386 emulation, a file locking operation fails.

Command line:
qemu-i386-static /usr/lib/wine-development/wineserver32

Output:
wineserver: fcntl /tmp/.wine-0/server-17-14d21bf/lock: Invalid argument

Relevant portion of strace:
fcntl(6, F_SETLK64, 0x3fffe8802218) = -1 EINVAL (Invalid argument)

** Affects: qemu
 Importance: Undecided
 Status: New

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

Title:
  qemu-user fails to run wineserver on ppc64el host

Status in QEMU:
  New

Bug description:
  When attempting to run wineserver on a 64-bit ppc64el host via QEMU's
  user-mode i386 emulation, a file locking operation fails.

  Command line:
  qemu-i386-static /usr/lib/wine-development/wineserver32

  Output:
  wineserver: fcntl /tmp/.wine-0/server-17-14d21bf/lock: Invalid argument

  Relevant portion of strace:
  fcntl(6, F_SETLK64, 0x3fffe8802218) = -1 EINVAL (Invalid argument)

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

[Qemu-devel] [PATCH for-2.10 0/3] qdev/vfio: defer DEVICE_DEL to avoid races with libvirt

[Michael Roth]

This series was motivated by the discussion in this thread:

  https://www.redhat.com/archives/libvir-list/2017-June/msg01370.html

The issue this series addresses is that when libvirt unplugs a VFIO PCI device,
it may attempt to bind the host device back to the host driver when QEMU emits
the DEVICE_DELETED event for the corresponding vfio-pci device. However, the
VFIO group FD is not actually cleaned up until vfio-pci device is *finalized*
by QEMU, whereas the event is emitted earlier during device_unparent.
Depending on the host device and how long certain operations like resetting the
device might take, this can in result in libvirt trying to rebind the device
back to the host while it is still in use by VFIO, leading to host crashes or
other unexpected behavior.

In particular, Mellanox CX4 adapters on PowerNV hosts might not be fully
quiesced by vfio-pci's finalize() routine until up to 6s after the
DEVICE_DELETED was emitted, leading to detach-device on the libvirt side pretty
much always crashing the host.

Implementing this change requires 2 prereqs to ensure the same information is
available when the DEVICE_DELETED is finally emitted:

1) Storing the path in the composition patch, which is addressed by PATCH 1,
   which was plucked from another pending series from Greg Kurz:

   https://lists.gnu.org/archive/html/qemu-devel/2017-07/msg07922.html

   since we are now "disconnected" at the time the event is emitted, and

2) Deferring qemu_opts_del of the DeviceState->QemuOpts till finalize, since
   that is where DeviceState->id is stored. This was actually how it was
   done in the past, so PATCH 2 simply reverts the change which moved it to
   device_unparent.

>From there it's just a mechanical move of the event from device_unparent to
device_finalize.

 hw/core/qdev.c | 30 +++---
 include/hw/qdev-core.h |  1 +
 2 files changed, 20 insertions(+), 11 deletions(-)

[Qemu-devel] [PATCH for-2.10 2/3] Revert "qdev: Free QemuOpts when the QOM path goes away"

[Michael Roth]

This reverts commit abed886ec60cf239a03515cf0b30fb11fa964c44.

This patch originally addressed an issue where a DEVICE_DELETED
event could be emitted (in device_unparent()) before a Device's
QemuOpts were cleaned up (in device_finalize()), leading to a
"duplicate ID" error if management attempted to immediately add
a device with the same ID in response to the DEVICE_DELETED event.

An alternative will be implemented in a subsequent patch where we
defer the DEVICE_DELETED event until device_finalize(), which would
also prevent the race, so we revert the original fix in preparation.

Signed-off-by: Michael Roth 
---
 hw/core/qdev.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index a64b35c..08c4061 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -1067,6 +1067,7 @@ static void device_finalize(Object *obj)
 NamedGPIOList *ngl, *next;
 
 DeviceState *dev = DEVICE(obj);
+qemu_opts_del(dev->opts);
 
 QLIST_FOREACH_SAFE(ngl, &dev->gpios, node, next) {
 QLIST_REMOVE(ngl, node);
@@ -1116,9 +1117,6 @@ static void device_unparent(Object *obj)
 g_free(dev->canonical_path);
 dev->canonical_path = NULL;
 }
-
-qemu_opts_del(dev->opts);
-dev->opts = NULL;
 }
 
 static void device_class_init(ObjectClass *class, void *data)
-- 
2.7.4

[Qemu-devel] [PATCH for-2.10 1/3] qdev: store DeviceState's canonical path to use when unparenting

[Michael Roth]

device_unparent(dev, ...) is called when a device is unparented,
either directly, or as a result of a parent device being
finalized, and handles some final cleanup for the device. Part
of this includes emiting a DEVICE_DELETED QMP event to notify
management, which includes the device's path in the composition
tree as provided by object_get_canonical_path().

object_get_canonical_path() assumes the device is still connected
to the machine/root container, and will assert otherwise, but
in some situations this isn't the case:

If the parent is finalized as a result of object_unparent(), it
will still be attached to the composition tree at the time any
children are unparented as a result of that same call to
object_unparent(). However, in some cases, object_unparent()
will complete without finalizing the parent device, due to
lingering references that won't be released till some time later.
One such example is if the parent has MemoryRegion children (which
take a ref on their parent), who in turn have AddressSpace's (which
take a ref on their regions), since those AddressSpaces get cleaned
up asynchronously by the RCU thread.

In this case qdev:device_unparent() may be called for a child Device
that no longer has a path to the root/machine container, causing
object_get_canonical_path() to assert.

Fix this by storing the canonical path during realize() so the
information will still be available for device_unparent() in such
cases.

Cc: Michael S. Tsirkin 
Cc: Paolo Bonzini 
Signed-off-by: Michael Roth 
Signed-off-by: Greg Kurz 
Signed-off-by: Michael Roth 
---
 hw/core/qdev.c | 15 ---
 include/hw/qdev-core.h |  1 +
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 606ab53..a64b35c 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -928,6 +928,12 @@ static void device_set_realized(Object *obj, bool value, 
Error **errp)
 goto post_realize_fail;
 }
 
+/* always re-initialize since we clean up in device_unparent() instead
+ * of unrealize()
+ */
+g_free(dev->canonical_path);
+dev->canonical_path = object_get_canonical_path(OBJECT(dev));
+
 if (qdev_get_vmsd(dev)) {
 if (vmstate_register_with_alias_id(dev, -1, qdev_get_vmsd(dev), 
dev,
dev->instance_id_alias,
@@ -984,6 +990,7 @@ child_realize_fail:
 }
 
 post_realize_fail:
+g_free(dev->canonical_path);
 if (dc->unrealize) {
 dc->unrealize(dev, NULL);
 }
@@ -1102,10 +1109,12 @@ static void device_unparent(Object *obj)
 
 /* Only send event if the device had been completely realized */
 if (dev->pending_deleted_event) {
-gchar *path = object_get_canonical_path(OBJECT(dev));
+g_assert(dev->canonical_path);
 
-qapi_event_send_device_deleted(!!dev->id, dev->id, path, &error_abort);
-g_free(path);
+qapi_event_send_device_deleted(!!dev->id, dev->id, dev->canonical_path,
+   &error_abort);
+g_free(dev->canonical_path);
+dev->canonical_path = NULL;
 }
 
 qemu_opts_del(dev->opts);
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index ae31728..9237b684 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -153,6 +153,7 @@ struct DeviceState {
 /*< public >*/
 
 const char *id;
+char *canonical_path;
 bool realized;
 bool pending_deleted_event;
 QemuOpts *opts;
-- 
2.7.4

[Qemu-devel] [PATCH for-2.10 3/3] qdev: defer DEVICE_DEL event until instance_finalize()

[Michael Roth]

DEVICE_DEL is currently emitted when a Device is unparented, as
opposed to when it is finalized. The main design motivation for this
seems to be that after unparent()/unrealize(), the Device is no
longer visible to the guest, and thus the operation is complete
from the perspective of management.

However, there are cases where remaining host-side cleanup is also
pertinent to management. The is generally handled by treating these
resources as aspects of the "backend", which can be managed via
separate interfaces/events, such as blockdev_add/del, netdev_add/del,
object_add/del, etc, but some devices do not have this level of
compartmentalization, namely vfio-pci, and possibly to lend themselves
well to it.

In the case of vfio-pci, the "backend" cleanup happens as part of
the finalization of the vfio-pci device itself, in particular the
cleanup of the VFIO group FD. Failing to wait for this cleanup can
result in tools like libvirt attempting to rebind the device to
the host while it's still being used by VFIO, which can result in
host crashes or other misbehavior depending on the host driver.

Deferring DEVICE_DEL still affords us the ability to manage backends
explicitly, while also addressing cases like vfio-pci's, so we
implement that approach here.

An alternative proposal involving having VFIO emit a separate event
to denote completion of host-side cleanup was discussed, but the
prevailing opinion seems to be that it is not worth the added
complexity, and leaves the issue open for other Device implementations
solve in the future.

Signed-off-by: Michael Roth 
---
 hw/core/qdev.c | 23 ---
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 08c4061..d14acba 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -1067,7 +1067,6 @@ static void device_finalize(Object *obj)
 NamedGPIOList *ngl, *next;
 
 DeviceState *dev = DEVICE(obj);
-qemu_opts_del(dev->opts);
 
 QLIST_FOREACH_SAFE(ngl, &dev->gpios, node, next) {
 QLIST_REMOVE(ngl, node);
@@ -1078,6 +1077,18 @@ static void device_finalize(Object *obj)
  * here
  */
 }
+
+/* Only send event if the device had been completely realized */
+if (dev->pending_deleted_event) {
+g_assert(dev->canonical_path);
+
+qapi_event_send_device_deleted(!!dev->id, dev->id, dev->canonical_path,
+   &error_abort);
+g_free(dev->canonical_path);
+dev->canonical_path = NULL;
+}
+
+qemu_opts_del(dev->opts);
 }
 
 static void device_class_base_init(ObjectClass *class, void *data)
@@ -1107,16 +1118,6 @@ static void device_unparent(Object *obj)
 object_unref(OBJECT(dev->parent_bus));
 dev->parent_bus = NULL;
 }
-
-/* Only send event if the device had been completely realized */
-if (dev->pending_deleted_event) {
-g_assert(dev->canonical_path);
-
-qapi_event_send_device_deleted(!!dev->id, dev->id, dev->canonical_path,
-   &error_abort);
-g_free(dev->canonical_path);
-dev->canonical_path = NULL;
-}
 }
 
 static void device_class_init(ObjectClass *class, void *data)
-- 
2.7.4

Re: [Qemu-devel] [PATCH for 2.10 2/2] Revert "elf-loader: warn about invalid endianness"

[Philippe Mathieu-Daudé]

On 07/26/2017 08:56 PM, Aurelien Jarno wrote:

From: Alexey Kardashevskiy 

This reverts c8e1158cf611 "elf-loader: warn about invalid endianness"
as it produces a useless message every time an LE kernel image is
passed via -kernel on a ppc64-pseries machine. The pseries machine
already checks for ELF_LOAD_WRONG_ENDIAN and tries with big_endian=0.

Signed-off-by: Alexey Kardashevskiy 
Signed-off-by: Aurelien Jarno 


Reviewed-by: Philippe Mathieu-Daudé 


---
  hw/core/loader.c | 1 -
  1 file changed, 1 deletion(-)

diff --git a/hw/core/loader.c b/hw/core/loader.c
index c17ace0a2e..e5e8cbb638 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -480,7 +480,6 @@ int load_elf_ram(const char *filename,
  }
  
  if (target_data_order != e_ident[EI_DATA]) {

-fprintf(stderr, "%s: wrong endianness\n", filename);
  ret = ELF_LOAD_WRONG_ENDIAN;
  goto fail;
  }

Re: [Qemu-devel] [PATCH for 2.10 1/2] hw/mips: load_elf_strerror to report kernel loading failure

[Philippe Mathieu-Daudé]

On 07/26/2017 08:56 PM, Aurelien Jarno wrote:

Emulated MIPS boards bail out with a simple "could not load kernel" when
a kernel could not be load, without specifying the underlying reason.
Fix that by calling load_elf_strerror.

At the same time use error_report to report the error instead of
fprintf.

Signed-off-by: Aurelien Jarno 


Thank you Aurelien :)

Reviewed-by: Philippe Mathieu-Daudé 
Tested-by: Philippe Mathieu-Daudé 


---
  hw/mips/mips_fulong2e.c | 15 +--
  hw/mips/mips_malta.c| 14 --
  hw/mips/mips_mipssim.c  |  5 +++--
  hw/mips/mips_r4k.c  |  6 --
  4 files changed, 24 insertions(+), 16 deletions(-)

diff --git a/hw/mips/mips_fulong2e.c b/hw/mips/mips_fulong2e.c
index 3f3cb32651..3532399a13 100644
--- a/hw/mips/mips_fulong2e.c
+++ b/hw/mips/mips_fulong2e.c
@@ -110,16 +110,19 @@ static int64_t load_kernel (CPUMIPSState *env)
  {
  int64_t kernel_entry, kernel_low, kernel_high;
  int index = 0;
-long initrd_size;
+long kernel_size, initrd_size;
  ram_addr_t initrd_offset;
  uint32_t *prom_buf;
  long prom_size;
  
-if (load_elf(loaderparams.kernel_filename, cpu_mips_kseg0_to_phys, NULL,

- (uint64_t *)&kernel_entry, (uint64_t *)&kernel_low,
- (uint64_t *)&kernel_high, 0, EM_MIPS, 1, 0) < 0) {
-fprintf(stderr, "qemu: could not load kernel '%s'\n",
-loaderparams.kernel_filename);
+kernel_size = load_elf(loaderparams.kernel_filename, 
cpu_mips_kseg0_to_phys,
+   NULL, (uint64_t *)&kernel_entry,
+   (uint64_t *)&kernel_low, (uint64_t *)&kernel_high,
+   0, EM_MIPS, 1, 0);
+if (kernel_size < 0) {
+error_report("qemu: could not load kernel '%s': %s",
+ loaderparams.kernel_filename,
+ load_elf_strerror(kernel_size));
  exit(1);
  }
  
diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c

index 3487d16f61..8ecd544baa 100644
--- a/hw/mips/mips_malta.c
+++ b/hw/mips/mips_malta.c
@@ -794,7 +794,7 @@ static void GCC_FMT_ATTR(3, 4) prom_set(uint32_t* prom_buf, 
int index,
  static int64_t load_kernel (void)
  {
  int64_t kernel_entry, kernel_high;
-long initrd_size;
+long kernel_size, initrd_size;
  ram_addr_t initrd_offset;
  int big_endian;
  uint32_t *prom_buf;
@@ -808,11 +808,13 @@ static int64_t load_kernel (void)
  big_endian = 0;
  #endif
  
-if (load_elf(loaderparams.kernel_filename, cpu_mips_kseg0_to_phys, NULL,

- (uint64_t *)&kernel_entry, NULL, (uint64_t *)&kernel_high,
- big_endian, EM_MIPS, 1, 0) < 0) {
-fprintf(stderr, "qemu: could not load kernel '%s'\n",
-loaderparams.kernel_filename);
+kernel_size = load_elf(loaderparams.kernel_filename, 
cpu_mips_kseg0_to_phys,
+   NULL, (uint64_t *)&kernel_entry, NULL,
+   (uint64_t *)&kernel_high, big_endian, EM_MIPS, 1, 
0);
+if (kernel_size < 0) {
+error_report("qemu: could not load kernel '%s': %s",
+ loaderparams.kernel_filename,
+ load_elf_strerror(kernel_size));
  exit(1);
  }
  
diff --git a/hw/mips/mips_mipssim.c b/hw/mips/mips_mipssim.c

index 6990b1b0dd..07fc4c2300 100644
--- a/hw/mips/mips_mipssim.c
+++ b/hw/mips/mips_mipssim.c
@@ -78,8 +78,9 @@ static int64_t load_kernel(void)
  if ((entry & ~0x7fffULL) == 0x8000)
  entry = (int32_t)entry;
  } else {
-fprintf(stderr, "qemu: could not load kernel '%s'\n",
-loaderparams.kernel_filename);
+error_report("qemu: could not load kernel '%s': %s",
+ loaderparams.kernel_filename,
+ load_elf_strerror(kernel_size));
  exit(1);
  }
  
diff --git a/hw/mips/mips_r4k.c b/hw/mips/mips_r4k.c

index 690874be2b..2f5ced7409 100644
--- a/hw/mips/mips_r4k.c
+++ b/hw/mips/mips_r4k.c
@@ -31,6 +31,7 @@
  #include "sysemu/block-backend.h"
  #include "exec/address-spaces.h"
  #include "sysemu/qtest.h"
+#include "qemu/error-report.h"
  
  #define MAX_IDE_BUS 2
  
@@ -96,8 +97,9 @@ static int64_t load_kernel(void)

  if ((entry & ~0x7fffULL) == 0x8000)
  entry = (int32_t)entry;
  } else {
-fprintf(stderr, "qemu: could not load kernel '%s'\n",
-loaderparams.kernel_filename);
+error_report("qemu: could not load kernel '%s': %s",
+ loaderparams.kernel_filename,
+ load_elf_strerror(kernel_size));
  exit(1);
  }
  

Re: [Qemu-devel] About the trace framework

[Wang Dong]

On 07/14/2017 09:25 PM, Stefan Hajnoczi wrote:

On Thu, Jul 13, 2017 at 04:01:09PM +0800, Wang Dong wrote:

On 07/10/2017 08:40 PM, Stefan Hajnoczi wrote:

On Mon, Jul 10, 2017 at 01:24:23PM +0800, Xie Changlong wrote:

在 7/9/2017 5:57 PM, Wang Dong 写道:

Hi,

I am new to QEMU. But I got some problem so that  I want to figure it out.

So I try to debug qemu to see what happened.

And I found trace framework. I think this will help me understand the
point.

So I compiled qemu with option:

## *--enable-trace-backends=simple*

And did as the docs/tracing.txt tell. But when I execute the example
command in it, nothing just happens.

*./scripts/simpletrace.py trace-events-all trace-xx(my trace file
produced by qemu)
I am not sure what happened. Just ask this.
Any clue will be appreciated. Thanks in advance.
*


Did you set *trace-event*? I've encountered this issue in the past :)
(qemu) help trace-event
trace-event name on|off -- changes status of a specific trace event
(qemu) info trace-events

Yes, you need to enable specific trace events that you are interested
in.  'trace-event' does that from the QEMU monitor at runtime.  If you'd
rather keep a file with a list of events to enable:

$ echo bdrv_aio_readv   > /tmp/events
$ echo bdrv_aio_writev >> /tmp/events
$ qemu -trace events=/tmp/events ... # your normal QEMU invocation

It's hard to know what is missing without seeing your QEMU command-line
and monitor commands you used.

Stefan

I thought the example in doc was enabled by default.

Thanks for your reply. Below is my command:
When to compile:

./configure --enable-debug --enable-trace-backends=simple

$ echo bdrv_aio_readv   > /tmp/events
$ echo bdrv_aio_writev >> /tmp/events


qemu [...]

-chardev socket,id=mon1,host=localhost,port=,server,nowait \
-mon chardev=mon1,mode=control,pretty=on \
-trace events=/tmp/events


Did I miss something?

Recent QEMU versions do not have bdrv_aio_readv/bdrv_aio_writev trace
events, so you may not get any output.

Which events do you actually want to trace?

I will update the example in docs/devel/tracing.txt with trace events
that are more likely to be triggered.

Stefan

Hi Stefan,
Thanks for the reply and your patch.
I just want to learn qemu with debugging.
I think it will be easier for me to learn the detail with this 
trace framework.


So I just tried it. I will try it again with your updated.


--
Best regards. Wang Dong

Re: [Qemu-devel] [PATCH 1/2] s390x/css: check ccw address validity

[Dong Jia Shi]

* Halil Pasic  [2017-07-26 18:45:34 +0200]:

[...]

> >>> @@ -946,6 +953,17 @@ static void sch_handle_start_func_virtual(SubchDev 
> >>> *sch)
> >>>  suspend_allowed = true;
> >>>  }
> >>>  sch->last_cmd_valid = false;
> >>> +if (sch->channel_prog & (CCW1_ADDR_MASK |
> >>> + sch->ccw_fmt_1 ? 0 : 0xff00)) {
> >> I have problem in recognizing the operator precedence here:
> >> (CCW1_ADDR_MASK | sch->ccw_fmt_1 ? 0 : 0xff00)
> >>
> >> Bitwise '|' has higher precedence than '?:', so the above equals to:
> >> (CCW1_ADDR_MASK | sch->ccw_fmt_1) ? 0 : 0xff00
> >>
> >> So you will always get a '0', no?
> >>
> > 
> > I'm afraid you are right. Good catch! This was supposed to be
> > (CCW1_ADDR_MASK | (sch->ccw_fmt_1 ? 0 : 0xff00))
> > 
> > 
> >>> +/* generate channel program check */
> >>> +s->ctrl &= ~SCSW_ACTL_START_PEND;
> >>> +s->cstat = SCSW_CSTAT_PROG_CHECK;
> >>> +s->ctrl &= ~SCSW_CTRL_MASK_STCTL;
> >>> +s->ctrl |= SCSW_STCTL_PRIMARY | SCSW_STCTL_SECONDARY |
> >>> +SCSW_STCTL_ALERT | SCSW_STCTL_STATUS_PEND;
> >>> +s->cpa = sch->channel_prog + 8;
> >>> +return;
> >>> +}
> >> I think you could let css_interpret_ccw() do the sanity check on its
> >> @ccw_addr parameter when (sch->last_cmd_valid == false), to reuse the
> >> code of generating channel program check.
> >>
> > 
> > I'm working on factoring out the code manipulating SCSW (among others). If 
> > we
> > do that this will look nicer. What you propose is certainly viable, althoug
> > maybe little less straight forward.
> > 
> > Yet another option would be to use a label and jump into the loop (AFAIR 
> > that
> > would be also valid).
> > 
> > Let us see what is Connie's opinion.
> > 
> 
> After re-examining the PoP I'm inclined to say we have to check this on every
> iteration because of how "main-storage location is unavailable" is defined in
> this context: the definition depends on the ccw format.
Sounds natural!

> There is nothing about this in the ccw chaining section of the pop but
> it can be found in the I/O interrupts chapter.
> 
> I think I will have to redo this patch :/
Ok.

> 
> Regards,
> Halil
> 
> > 
> >>>  do {
> >>>  ret = css_interpret_ccw(sch, sch->channel_prog, suspend_allowed);
> >>>  switch (ret) {
> >>> -- 
> >>> 2.11.2
> >>>
> >>
> > 
> > 
> 
> 

-- 
Dong Jia Shi

Re: [Qemu-devel] KVM "fake DAX" flushing interface - discussion

[Dan Williams]

On Wed, Jul 26, 2017 at 4:46 PM, Rik van Riel  wrote:
> On Wed, 2017-07-26 at 14:40 -0700, Dan Williams wrote:
>> On Wed, Jul 26, 2017 at 2:27 PM, Rik van Riel 
>> wrote:
>> > On Wed, 2017-07-26 at 09:47 -0400, Pankaj Gupta wrote:
>> > > >
>> > >
>> > > Just want to summarize here(high level):
>> > >
>> > > This will require implementing new 'virtio-pmem' device which
>> > > presents
>> > > a DAX address range(like pmem) to guest with read/write(direct
>> > > access)
>> > > & device flush functionality. Also, qemu should implement
>> > > corresponding
>> > > support for flush using virtio.
>> > >
>> >
>> > Alternatively, the existing pmem code, with
>> > a flush-only block device on the side, which
>> > is somehow associated with the pmem device.
>> >
>> > I wonder which alternative leads to the least
>> > code duplication, and the least maintenance
>> > hassle going forward.
>>
>> I'd much prefer to have another driver. I.e. a driver that refactors
>> out some common pmem details into a shared object and can attach to
>> ND_DEVICE_NAMESPACE_{IO,PMEM}. A control device on the side seems
>> like
>> a recipe for confusion.
>
> At that point, would it make sense to expose these special
> virtio-pmem areas to the guest in a slightly different way,
> so the regions that need virtio flushing are not bound by
> the regular driver, and the regular driver can continue to
> work for memory regions that are backed by actual pmem in
> the host?

Hmm, yes that could be feasible especially if it uses the ACPI NFIT
mechanism. It would basically involve defining a new SPA (System
Phyiscal Address) range GUID type, and then teaching libnvdimm to
treat that as a new pmem device type.

See usage of UUID_PERSISTENT_MEMORY in drivers/acpi/nfit/ and the
eventual region description sent to nvdimm_pmem_region_create(). We
would then need to plumb a new flag so that nd_region_to_nstype() in
libnvdimm returns a different namespace type number for this virtio
use case, but otherwise the rest of libnvdimm should treat the region
as pmem.

Re: [Qemu-devel] [PATCH 2/2] s390x/css: fix bits must be zero check for TIC

[Dong Jia Shi]

* Halil Pasic  [2017-07-26 13:38:33 +0200]:

> 
> 
> On 07/26/2017 05:01 AM, Dong Jia Shi wrote:
> > Hello Halil,
> > 
> > * Halil Pasic  [2017-07-26 00:44:42 +0200]:
> > 
> >> According to the PoP bit positions 0-3 and 8-32 of the format-1 CCW must
> >> contain zeros.  Bits 0-3 are already covered by cmd_code validity
> >> checking, and bit 32 is covered by the CCW address checking.
> >>
> >> Bits 8-31 correspond to CCW1.flags and CCW1.count.  Currently we only
> >> check for the absence of certain flags.  Let's fix this.
> >>
> >> Signed-off-by: Halil Pasic 
> >> ---
> >>  hw/s390x/css.c | 3 ++-
> >>  1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> >> index d17e21b7af..1f04ce4a1b 100644
> >> --- a/hw/s390x/css.c
> >> +++ b/hw/s390x/css.c
> >> @@ -884,7 +884,8 @@ static int css_interpret_ccw(SubchDev *sch, hwaddr 
> >> ccw_addr,
> >>  ret = -EINVAL;
> >>  break;
> >>  }
> >> -if (ccw.flags & (CCW_FLAG_CC | CCW_FLAG_DC)) {
> >> +if (ccw.flags || ccw.count) {
> >> +/* We have already sanitized these if fmt 0. */
> > ccw0 does not have the same restrictions as ccw1. We don't sanitize
> > these for ccw0.
> > 
> 
> Yes you have misunderstood. For fmt 1 these bits have to be zero
> otherwise a program-check condition is to be recognized. Thus we don't
> sanitize for fmt 1.
> 
> For fmt 0 these bits are ignored. We sanitize them in
> for some time now by setting them to zero when making a CCW1
> out of an CCW0. If we would recognize a program-check for
> fmt 0 that would be wrong.
Yup, I know this.

> 
> The comment tells us why this code is good for CCW0 too,
> and why can we omit sch->ccw_fmt_1 from the conditon.
Ahh, I see the point now. Yes, I misunderstood.

Another point is we have translated ccw0 to ccw1. So here we only focus
on handling ccw1 stuff. Mentioning ccw0 seems a little redundant.

Anyway, I will leave this to you to decide. No problem from my side now.

> 
> Regards,
> Halil
> 
> > (This comment is still here. Did I misunderstand things? :)
> > 
> >>  ret = -EINVAL;
> >>  break;
> >>  }
> >> -- 
> >> 2.11.2
> >>
> > 
> > With the comment removed:
> > Reviewed-by: Dong Jia Shi 
> > 
> 
> 

-- 
Dong Jia Shi

Re: [Qemu-devel] [PATCH 1/2] vhost-user-blk: introduce a new vhost-user-blk host device

[Liu, Changpeng]

> -Original Message-
> From: Stefan Hajnoczi [mailto:stefa...@redhat.com]
> Sent: Wednesday, July 26, 2017 6:35 PM
> To: Liu, Changpeng 
> Cc: qemu-devel@nongnu.org; pbonz...@redhat.com; fel...@nutanix.com;
> m...@redhat.com
> Subject: Re: [PATCH 1/2] vhost-user-blk: introduce a new vhost-user-blk host
> device
> 
> On Thu, Jul 27, 2017 at 10:00:50AM +0800, Changpeng Liu wrote:
> > +static void vhost_user_blk_set_config(VirtIODevice *vdev, const uint8_t
> *config)
> > +{
> > +VHostUserBlk *s = VHOST_USER_BLK(vdev);
> > +struct virtio_blk_config blkcfg;
> > +
> > +memcpy(&blkcfg, config, sizeof(blkcfg));
> > +
> > +if (blkcfg.wce != s->config_wce) {
> > +error_report("vhost-user-blk: does not support the operation");
> 
> If vhost-user-blk doesn't support the operation then please remove the
> VIRTIO_BLK_F_CONFIG_WCE feature bit.  That way the guest knows it cannot
> modify this field.
> 
> > +static void vhost_user_blk_device_realize(DeviceState *dev, Error **errp)
> > +{
> > +VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> > +VHostUserBlk *s = VHOST_USER_BLK(vdev);
> > +int ret;
> > +uint64_t size;
> > +
> > +if (!s->chardev.chr) {
> > +error_setg(errp, "vhost-user-blk: chardev is mandatary");
> 
> mandatory
> 
> > +return;
> > +}
> > +
> > +if (!s->num_queues) {
> > +error_setg(errp, "vhost-user-blk: invalid number of IO queues");
> > +return;
> > +}
> > +
> > +if (!s->queue_size) {
> > +error_setg(errp, "vhost-user-blk: invalid count of the IO queue");
> 
> "count of the IO queue" sounds like number of queues.  I suggest saying
> "queue size must be non-zero".
> 
> > +return;
> > +}
> > +
> > +if (!s->size) {
> > +error_setg(errp, "vhost-user-blk: block capacity must be assigned,"
> > +  "size can be specified by GiB or MiB");
> > +return;
> > +}
> > +
> > +ret = qemu_strtosz_MiB(s->size, NULL, &size);
> > +if (ret < 0) {
> > +error_setg(errp, "vhost-user-blk: invalid size %s in GiB/MiB", 
> > s->size);
> > +return;
> > +}
> > +s->capacity = size / 512;
> > +
> > +/* block size with default 512 Bytes */
> > +if (!s->blkcfg.logical_block_size) {
> > +s->blkcfg.logical_block_size = 512;
> > +}
> > +
> > +virtio_init(vdev, "virtio-blk", VIRTIO_ID_BLOCK,
> > +sizeof(struct virtio_blk_config));
> > +virtio_add_queue(vdev, s->queue_size, vhost_user_blk_handle_output);
> > +
> > +s->dev.nvqs = s->num_queues;
> > +s->dev.vqs = g_new(struct vhost_virtqueue, s->dev.nvqs);
> 
> Please test multi-queue, it's currently broken.  virtio_add_queue() must
> be called for each queue.
Yes, should move virtio_add_queue into loop.
> 
> > +s->dev.vq_index = 0;
> > +s->dev.backend_features = 0;
> > +
> > +ret = vhost_dev_init(&s->dev, (void *)&s->chardev,
> 
> The compiler automatically converts any pointer type to void * without a
> warning in C.  (This is different from C++!)
> 
> The (void *) cast can be dropped.
> 
> > + VHOST_BACKEND_TYPE_USER, 0);
> > +if (ret < 0) {
> > +error_setg(errp, "vhost-user-blk: vhost initialization failed: %s",
> > +   strerror(-ret));
> 
> If realize fails unrealize() will not be called.  Cleanup must be
> performed here.
> 
> > +return;
> > +}
> > +}
> > +
> > +static void vhost_user_blk_device_unrealize(DeviceState *dev, Error **errp)
> > +{
> > +VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> > +VHostUserBlk *s = VHOST_USER_BLK(dev);
> > +
> > +vhost_user_blk_set_status(vdev, 0);
> > +vhost_dev_cleanup(&s->dev);
> > +g_free(s->dev.vqs);
> > +virtio_cleanup(vdev);
> > +}
> > +
> > +static void vhost_user_blk_instance_init(Object *obj)
> > +{
> > +VHostUserBlk *s = VHOST_USER_BLK(obj);
> > +
> > +device_add_bootindex_property(obj, &s->bootindex, "bootindex",
> > +  "/disk@0,0", DEVICE(obj), NULL);
> > +}
> > +
> > +static const VMStateDescription vmstate_vhost_user_blk = {
> > +.name = "vhost-user-blk",
> > +.minimum_version_id = 2,
> > +.version_id = 2,
> 
> Why is version_id 2?  There has never been a vhost-user-blk device in
> qemu.git before, so I would expect version to be 1.
> 
> > +.fields = (VMStateField[]) {
> > +VMSTATE_VIRTIO_DEVICE,
> > +VMSTATE_END_OF_LIST()
> > +},
> > +};
> > +
> > +static Property vhost_user_blk_properties[] = {
> > +DEFINE_PROP_CHR("chardev", VHostUserBlk, chardev),
> > +DEFINE_BLOCK_PROPERTIES(VHostUserBlk, blkcfg),
> 
> DEFINE_BLOCK_PROPERTIES cannot be used directly because it includes the
> 'drive' (blkcfg.blk) parameter.  The command-line should not allow a
> drive= parameter.
> 
> Also, parameters like the discard granularity, optimal I/O size, logical
> block size, etc can be initialized to sensible defaults by QEMU's block
> layer when d

[Qemu-devel] [PATCH for 2.10 0/2] Move endianness error reporting to the MIPS boards

[Aurelien Jarno]

This patchset improves the MIPS board error reporting when loading a 
kernel with the wrong endianness, using load_elf_strerror.

That way the check added to loader.c can be removed, as it harms the
pseries platform. Indeed it can change its endianness dynamically at
runtime and thus can load both little and big endian kernels.

Alexey Kardashevskiy (1):
  Revert "elf-loader: warn about invalid endianness"

Aurelien Jarno (1):
  hw/mips: load_elf_strerror to report kernel loading failure

 hw/core/loader.c|  1 -
 hw/mips/mips_fulong2e.c | 15 +--
 hw/mips/mips_malta.c| 14 --
 hw/mips/mips_mipssim.c  |  5 +++--
 hw/mips/mips_r4k.c  |  6 --
 5 files changed, 24 insertions(+), 17 deletions(-)

-- 
2.11.0

Re: [Qemu-devel] [PATCH qemu] Revert "elf-loader: warn about invalid endianness"

[Aurelien Jarno]

On 2017-07-21 17:30, Alexey Kardashevskiy wrote:
> On 21/07/17 16:48, Philippe Mathieu-Daudé wrote:
> > Hi Alexey,
> > 
> > On 07/21/2017 01:19 AM, Alexey Kardashevskiy wrote:
> >> This reverts c8e1158cf611 "elf-loader: warn about invalid endianness"
> >> as it produces a useless message every time an LE kernel image is
> >> passed via -kernel on a ppc64-pseries machine. The pseries machine
> >> already checks for ELF_LOAD_WRONG_ENDIAN and tries with big_endian=0.
> >>
> >> Signed-off-by: Alexey Kardashevskiy 
> >> ---
> >>   hw/core/loader.c | 1 -
> >>   1 file changed, 1 deletion(-)
> >>
> >> diff --git a/hw/core/loader.c b/hw/core/loader.c
> >> index c17ace0a2e..e5e8cbb638 100644
> >> --- a/hw/core/loader.c
> >> +++ b/hw/core/loader.c
> >> @@ -480,7 +480,6 @@ int load_elf_ram(const char *filename,
> >>   }
> >> if (target_data_order != e_ident[EI_DATA]) {
> >> -fprintf(stderr, "%s: wrong endianness\n", filename);
> >>   ret = ELF_LOAD_WRONG_ENDIAN;
> >>   goto fail;
> >>   }
> >>
> > 
> > I submitted this patch because I spent some time debugging while QEMU was
> > failing silently using a MIPS kernel image which used to work, after
> > realizing I was in an incorrect build_dir using qemu-system-mipsel to load
> > a big endian image and felt stupid [1]. This dumb error can happen to other
> > people so I added this warning here.
> 
> Been there too. This is why we try loading images twice in pseries.

Indeed. For a better understanding of the issue, it should be noted that
the pseries platform can dynamically change its endianness at runtime
and thus can load both little and big endian kernel. This is done by
calling load_elf twice with both endianness. Therefore the fact that the
endianness *does not* match is not an issue in that case.

> > I was not aware of the ELF_LOAD_WRONG_ENDIAN related code, and at least the
> > MIPS arch is not using it.
> > As I can see in MAINTAINERS, sPAPR is "Supported" meaning "Someone is
> > actually paid to look after this", while there is no such paid person for
> > the MIPS part.
> > It seems each arch had a different way to load images and hw/core/loader.c
> > was an effort to merge common code but mostly "Supported" arch are using it.
> 
> afaict MIPS calls load_elf() in 4 places, each checks for the return value
> and already prints the message, how come that that message is not enough?
> What would probably make sense here is if MIPS also printed the return code
> from load_elf() but in any case it is easily visible from gdb.

It display an error message, but this message doesn't display why the
ELF kernel failed to be loaded.
 
> > While your revert does fixes your sPAPR warning issue, looking at the
> > problem roots I think the correct fix is to improve the MIPS port and
> > eventually the less loved archs to unify the loader.c calls and avoid such
> > problems.
> > I don't object reverting this patch for 2.10 and improve the loader.c usage
> > during 2.11 cycle, I only wonder if this is another corporate/hobbyist> 
> > conflict of interest with corporate crushing on hobbyist instead of
> 
> Come on mate...

I would not call it a conflict, but it's definitely a corporte/hobbyist
issue. The corporate people designed a nice load_elf_strerror function
to report error and the hobbyist people failed to use it.

Anyway I have just sent a simple patch series improving the MIPS error
reporting. That should make everybody happy.

-- 
Aurelien Jarno  GPG: 4096R/1DDD8C9B
aurel...@aurel32.net http://www.aurel32.net

[Qemu-devel] [PATCH for 2.10 2/2] Revert "elf-loader: warn about invalid endianness"

[Aurelien Jarno]

From: Alexey Kardashevskiy 

This reverts c8e1158cf611 "elf-loader: warn about invalid endianness"
as it produces a useless message every time an LE kernel image is
passed via -kernel on a ppc64-pseries machine. The pseries machine
already checks for ELF_LOAD_WRONG_ENDIAN and tries with big_endian=0.

Signed-off-by: Alexey Kardashevskiy 
Signed-off-by: Aurelien Jarno 
---
 hw/core/loader.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/hw/core/loader.c b/hw/core/loader.c
index c17ace0a2e..e5e8cbb638 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -480,7 +480,6 @@ int load_elf_ram(const char *filename,
 }
 
 if (target_data_order != e_ident[EI_DATA]) {
-fprintf(stderr, "%s: wrong endianness\n", filename);
 ret = ELF_LOAD_WRONG_ENDIAN;
 goto fail;
 }
-- 
2.11.0

[Qemu-devel] [PATCH for 2.10 1/2] hw/mips: load_elf_strerror to report kernel loading failure

[Aurelien Jarno]

Emulated MIPS boards bail out with a simple "could not load kernel" when
a kernel could not be load, without specifying the underlying reason.
Fix that by calling load_elf_strerror.

At the same time use error_report to report the error instead of
fprintf.

Signed-off-by: Aurelien Jarno 
---
 hw/mips/mips_fulong2e.c | 15 +--
 hw/mips/mips_malta.c| 14 --
 hw/mips/mips_mipssim.c  |  5 +++--
 hw/mips/mips_r4k.c  |  6 --
 4 files changed, 24 insertions(+), 16 deletions(-)

diff --git a/hw/mips/mips_fulong2e.c b/hw/mips/mips_fulong2e.c
index 3f3cb32651..3532399a13 100644
--- a/hw/mips/mips_fulong2e.c
+++ b/hw/mips/mips_fulong2e.c
@@ -110,16 +110,19 @@ static int64_t load_kernel (CPUMIPSState *env)
 {
 int64_t kernel_entry, kernel_low, kernel_high;
 int index = 0;
-long initrd_size;
+long kernel_size, initrd_size;
 ram_addr_t initrd_offset;
 uint32_t *prom_buf;
 long prom_size;
 
-if (load_elf(loaderparams.kernel_filename, cpu_mips_kseg0_to_phys, NULL,
- (uint64_t *)&kernel_entry, (uint64_t *)&kernel_low,
- (uint64_t *)&kernel_high, 0, EM_MIPS, 1, 0) < 0) {
-fprintf(stderr, "qemu: could not load kernel '%s'\n",
-loaderparams.kernel_filename);
+kernel_size = load_elf(loaderparams.kernel_filename, 
cpu_mips_kseg0_to_phys,
+   NULL, (uint64_t *)&kernel_entry,
+   (uint64_t *)&kernel_low, (uint64_t *)&kernel_high,
+   0, EM_MIPS, 1, 0);
+if (kernel_size < 0) {
+error_report("qemu: could not load kernel '%s': %s",
+ loaderparams.kernel_filename,
+ load_elf_strerror(kernel_size));
 exit(1);
 }
 
diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c
index 3487d16f61..8ecd544baa 100644
--- a/hw/mips/mips_malta.c
+++ b/hw/mips/mips_malta.c
@@ -794,7 +794,7 @@ static void GCC_FMT_ATTR(3, 4) prom_set(uint32_t* prom_buf, 
int index,
 static int64_t load_kernel (void)
 {
 int64_t kernel_entry, kernel_high;
-long initrd_size;
+long kernel_size, initrd_size;
 ram_addr_t initrd_offset;
 int big_endian;
 uint32_t *prom_buf;
@@ -808,11 +808,13 @@ static int64_t load_kernel (void)
 big_endian = 0;
 #endif
 
-if (load_elf(loaderparams.kernel_filename, cpu_mips_kseg0_to_phys, NULL,
- (uint64_t *)&kernel_entry, NULL, (uint64_t *)&kernel_high,
- big_endian, EM_MIPS, 1, 0) < 0) {
-fprintf(stderr, "qemu: could not load kernel '%s'\n",
-loaderparams.kernel_filename);
+kernel_size = load_elf(loaderparams.kernel_filename, 
cpu_mips_kseg0_to_phys,
+   NULL, (uint64_t *)&kernel_entry, NULL,
+   (uint64_t *)&kernel_high, big_endian, EM_MIPS, 1, 
0);
+if (kernel_size < 0) {
+error_report("qemu: could not load kernel '%s': %s",
+ loaderparams.kernel_filename,
+ load_elf_strerror(kernel_size));
 exit(1);
 }
 
diff --git a/hw/mips/mips_mipssim.c b/hw/mips/mips_mipssim.c
index 6990b1b0dd..07fc4c2300 100644
--- a/hw/mips/mips_mipssim.c
+++ b/hw/mips/mips_mipssim.c
@@ -78,8 +78,9 @@ static int64_t load_kernel(void)
 if ((entry & ~0x7fffULL) == 0x8000)
 entry = (int32_t)entry;
 } else {
-fprintf(stderr, "qemu: could not load kernel '%s'\n",
-loaderparams.kernel_filename);
+error_report("qemu: could not load kernel '%s': %s",
+ loaderparams.kernel_filename,
+ load_elf_strerror(kernel_size));
 exit(1);
 }
 
diff --git a/hw/mips/mips_r4k.c b/hw/mips/mips_r4k.c
index 690874be2b..2f5ced7409 100644
--- a/hw/mips/mips_r4k.c
+++ b/hw/mips/mips_r4k.c
@@ -31,6 +31,7 @@
 #include "sysemu/block-backend.h"
 #include "exec/address-spaces.h"
 #include "sysemu/qtest.h"
+#include "qemu/error-report.h"
 
 #define MAX_IDE_BUS 2
 
@@ -96,8 +97,9 @@ static int64_t load_kernel(void)
 if ((entry & ~0x7fffULL) == 0x8000)
 entry = (int32_t)entry;
 } else {
-fprintf(stderr, "qemu: could not load kernel '%s'\n",
-loaderparams.kernel_filename);
+error_report("qemu: could not load kernel '%s': %s",
+ loaderparams.kernel_filename,
+ load_elf_strerror(kernel_size));
 exit(1);
 }
 
-- 
2.11.0

Re: [Qemu-devel] KVM "fake DAX" flushing interface - discussion

[Rik van Riel]

On Wed, 2017-07-26 at 14:40 -0700, Dan Williams wrote:
> On Wed, Jul 26, 2017 at 2:27 PM, Rik van Riel 
> wrote:
> > On Wed, 2017-07-26 at 09:47 -0400, Pankaj Gupta wrote:
> > > > 
> > > 
> > > Just want to summarize here(high level):
> > > 
> > > This will require implementing new 'virtio-pmem' device which
> > > presents
> > > a DAX address range(like pmem) to guest with read/write(direct
> > > access)
> > > & device flush functionality. Also, qemu should implement
> > > corresponding
> > > support for flush using virtio.
> > > 
> > 
> > Alternatively, the existing pmem code, with
> > a flush-only block device on the side, which
> > is somehow associated with the pmem device.
> > 
> > I wonder which alternative leads to the least
> > code duplication, and the least maintenance
> > hassle going forward.
> 
> I'd much prefer to have another driver. I.e. a driver that refactors
> out some common pmem details into a shared object and can attach to
> ND_DEVICE_NAMESPACE_{IO,PMEM}. A control device on the side seems
> like
> a recipe for confusion.

At that point, would it make sense to expose these special
virtio-pmem areas to the guest in a slightly different way,
so the regions that need virtio flushing are not bound by
the regular driver, and the regular driver can continue to
work for memory regions that are backed by actual pmem in
the host?

> With a $new_driver in hand you can just do:
> 
>    modprobe $new_driver
>    echo $namespace > /sys/bus/nd/drivers/nd_pmem/unbind
>    echo $namespace > /sys/bus/nd/drivers/$new_driver/new_id
>    echo $namespace > /sys/bus/nd/drivers/$new_driver/bind
> 
> ...and the guest can arrange for $new_driver to be the default, so
> you
> don't need to do those steps each boot of the VM, by doing:
> 
> echo "blacklist nd_pmem" > /etc/modprobe.d/virt-dax-flush.conf
> echo "alias nd:t4* $new_driver" >> /etc/modprobe.d/virt-dax-
> flush.conf
> echo "alias nd:t5* $new_driver" >> /etc/modprobe.d/virt-dax-
> flush.conf

Re: [Qemu-devel] [PATCH v3 04/12] tests: Pass literal format strings directly to qmp_FOO()

[John Snow]

On 07/25/2017 05:15 PM, Eric Blake wrote:

From: Markus Armbruster 

The qmp_FOO() take a printf-like format string.  In a few places, we
assign a string literal to a variable and pass that instead of simply
passing the literal.  Clean that up.

Bonus: gets rid of non-literal format strings.  A step towards
compile-time format string checking without triggering
-Wformat-nonliteral.

Signed-off-by: Markus Armbruster 
Message-Id: <1500645206-13559-4-git-send-email-arm...@redhat.com>
Signed-off-by: Eric Blake 
Reviewed-by: Stefan Hajnoczi 


Reviewed-by: John Snow 

Re: [Qemu-devel] [RFC PATCH v2 4/6] hw/pci: introduce bridge-only vendor-specific capability to provide some hints to firmware

[Michael S. Tsirkin]

On Thu, Jul 27, 2017 at 12:54:07AM +0300, Alexander Bezzubikov wrote:
> 2017-07-26 22:43 GMT+03:00 Michael S. Tsirkin :
> > On Sun, Jul 23, 2017 at 01:15:41AM +0300, Aleksandr Bezzubikov wrote:
> >> On PCI init PCI bridges may need some
> >> extra info about bus number to reserve, IO, memory and
> >> prefetchable memory limits. QEMU can provide this
> >> with special
> >
> > with a special
> >
> >> vendor-specific PCI capability.
> >>
> >> Sizes of limits match ones from
> >> PCI Type 1 Configuration Space Header,
> >> number of buses to reserve occupies only 1 byte
> >> since it is the size of Subordinate Bus Number register.
> >>
> >> Signed-off-by: Aleksandr Bezzubikov 
> >> ---
> >>  hw/pci/pci_bridge.c | 27 +++
> >>  include/hw/pci/pci_bridge.h | 18 ++
> >>  2 files changed, 45 insertions(+)
> >>
> >> diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c
> >> index 720119b..8ec6c2c 100644
> >> --- a/hw/pci/pci_bridge.c
> >> +++ b/hw/pci/pci_bridge.c
> >> @@ -408,6 +408,33 @@ void pci_bridge_map_irq(PCIBridge *br, const char* 
> >> bus_name,
> >>  br->bus_name = bus_name;
> >>  }
> >>
> >> +
> >> +int pci_bridge_help_cap_init(PCIDevice *dev, int cap_offset,
> >
> > help? should be qemu_cap_init?
> >
> >> +  uint8_t bus_reserve, uint32_t io_limit,
> >> +  uint16_t mem_limit, uint64_t pref_limit,
> >> +  Error **errp)
> >> +{
> >> +size_t cap_len = sizeof(PCIBridgeQemuCap);
> >> +PCIBridgeQemuCap cap;
> >
> > This leaks info to guest. You want to init all fields here:
> >
> > cap = {
> >  .len = 
> > };
> 
> I surely can do this for len field, but as Laszlo proposed
> we can use mutually exclusive fields,
> e.g. pref_32 and pref_64, the only way I have left
> is to use ternary operator (if we surely need this
> big initializer). Keeping some if's would look better,
> I think.
> 
> >
> >> +
> >> +cap.len = cap_len;
> >> +cap.bus_res = bus_reserve;
> >> +cap.io_lim = io_limit & 0xFF;
> >> +cap.io_lim_upper = io_limit >> 8 & 0x;
> >> +cap.mem_lim = mem_limit;
> >> +cap.pref_lim = pref_limit & 0x;
> >> +cap.pref_lim_upper = pref_limit >> 16 & 0x;
> >
> > Please use pci_set_word etc or cpu_to_leXX.
> >
> 
> Since now we've decided to avoid fields separation into  + 
> ,
> this bitmask along with pci_set_word are no longer needed.
> 
> > I think it's easiest to replace struct with a set of macros then
> > pci_set_word does the work for you.
> >
> 
> I don't really want to use macros here because structure
> show us the whole capability layout and this can
> decrease documenting efforts. More than that,
> memcpy usage is very convenient here, and I wouldn't like
> to lose it.
> 
> >
> >> +
> >> +int offset = pci_add_capability(dev, PCI_CAP_ID_VNDR,
> >> +cap_offset, cap_len, errp);
> >> +if (offset < 0) {
> >> +return offset;
> >> +}
> >> +
> >> +memcpy(dev->config + offset + 2, (char *)&cap + 2, cap_len - 2);
> >
> > +2 is yacky. See how virtio does it:
> >
> > memcpy(dev->config + offset + PCI_CAP_FLAGS, &cap->cap_len,
> >cap->cap_len - PCI_CAP_FLAGS);
> >
> >
> 
> OK.
> 
> >> +return 0;
> >> +}
> >> +
> >>  static const TypeInfo pci_bridge_type_info = {
> >>  .name = TYPE_PCI_BRIDGE,
> >>  .parent = TYPE_PCI_DEVICE,
> >> diff --git a/include/hw/pci/pci_bridge.h b/include/hw/pci/pci_bridge.h
> >> index ff7cbaa..c9f642c 100644
> >> --- a/include/hw/pci/pci_bridge.h
> >> +++ b/include/hw/pci/pci_bridge.h
> >> @@ -67,4 +67,22 @@ void pci_bridge_map_irq(PCIBridge *br, const char* 
> >> bus_name,
> >>  #define  PCI_BRIDGE_CTL_DISCARD_STATUS   0x400   /* Discard timer 
> >> status */
> >>  #define  PCI_BRIDGE_CTL_DISCARD_SERR 0x800   /* Discard timer SERR# 
> >> enable */
> >>
> >> +typedef struct PCIBridgeQemuCap {
> >> +uint8_t id; /* Standard PCI capability header field */
> >> +uint8_t next;   /* Standard PCI capability header field */
> >> +uint8_t len;/* Standard PCI vendor-specific capability header 
> >> field */
> >> +uint8_t bus_res;
> >> +uint32_t pref_lim_upper;
> >
> > Big endian? Ugh.
> >
> 
> Agreed, and this's gonna to disappear with
> the new layout.
> 
> >> +uint16_t pref_lim;
> >> +uint16_t mem_lim;
> >
> > I'd say we need 64 bit for memory.
> >
> 
> Why? Non-prefetchable MEMORY_LIMIT register is 16 bits long.

Hmm ok, but e.g. for io there are bridges that have extra registers
to specify non-standard non-aligned registers.

> >> +uint16_t io_lim_upper;
> >> +uint8_t io_lim;
> >> +uint8_t padding;
> >
> > IMHO each type should have a special "don't care" flag
> > that would mean "I don't know".
> >
> >
> 
> Don't know what? Now 0 is an indicator to do nothing with this field.

In that case how do you say "don't allocate any memory"?


> >> +} PCIBridgeQemuCap;
> >
> > Y

Re: [Qemu-devel] [PATCH for 2.10 17/35] usb/dev-mtp: fix use of uninitialized values

[Philippe Mathieu-Daudé]

On 07/24/2017 03:27 PM, Philippe Mathieu-Daudé wrote:

Reported-by: Clang Static Analyzer
Signed-off-by: Philippe Mathieu-Daudé 
---
  hw/usb/dev-mtp.c | 36 +---
  1 file changed, 21 insertions(+), 15 deletions(-)

diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
index 6dfece9ea9..ad64495f05 100644
--- a/hw/usb/dev-mtp.c
+++ b/hw/usb/dev-mtp.c
@@ -1134,7 +1134,7 @@ static void usb_mtp_command(MTPState *s, MTPControl *c)
   c->trans, 1, s->session, 0);
  return;
  }
-if (c->argv[0] == 0) {
+if (c->argc == 0 || c->argv[0] == 0) {
  usb_mtp_queue_result(s, RES_INVALID_PARAMETER,
   c->trans, 0, 0, 0);


^ This is OK,

but part below is incorrect, after reading the MTP specs 1.1 I 
understood the correct code to return is RES_INVALID_PARAMETER.



  return;
@@ -1162,8 +1162,9 @@ static void usb_mtp_command(MTPState *s, MTPControl *c)
  data_in = usb_mtp_get_storage_ids(s, c);
  break;
  case CMD_GET_STORAGE_INFO:
-if (c->argv[0] != QEMU_STORAGE_ID &&
-c->argv[0] != 0x) {
+if (c->argc == 0 ||
+   (c->argv[0] != QEMU_STORAGE_ID &&
+c->argv[0] != 0x)) {
  usb_mtp_queue_result(s, RES_INVALID_STORAGE_ID,
   c->trans, 0, 0, 0);
  return;
@@ -1172,22 +1173,25 @@ static void usb_mtp_command(MTPState *s, MTPControl *c)
  break;
  case CMD_GET_NUM_OBJECTS:
  case CMD_GET_OBJECT_HANDLES:
-if (c->argv[0] != QEMU_STORAGE_ID &&
-c->argv[0] != 0x) {
+if (c->argc == 0 ||
+   (c->argv[0] != QEMU_STORAGE_ID &&
+c->argv[0] != 0x)) {
  usb_mtp_queue_result(s, RES_INVALID_STORAGE_ID,
   c->trans, 0, 0, 0);
  return;
  }
-if (c->argv[1] != 0x) {
+if (c->argc > 1 && c->argv[1] != 0x) {
  usb_mtp_queue_result(s, RES_SPEC_BY_FORMAT_UNSUPPORTED,
   c->trans, 0, 0, 0);
  return;
  }
-if (c->argv[2] == 0x ||
-c->argv[2] == 0x) {
-o = QTAILQ_FIRST(&s->objects);
-} else {
-o = usb_mtp_object_lookup(s, c->argv[2]);
+if (c->argc > 2) {
+if (c->argv[2] == 0x ||
+c->argv[2] == 0x) {
+o = QTAILQ_FIRST(&s->objects);
+} else {
+o = usb_mtp_object_lookup(s, c->argv[2]);
+}
  }
  if (o == NULL) {
  usb_mtp_queue_result(s, RES_INVALID_OBJECT_HANDLE,
@@ -1264,8 +1268,9 @@ static void usb_mtp_command(MTPState *s, MTPControl *c)
  res0 = data_in->length;
  break;
  case CMD_GET_OBJECT_PROPS_SUPPORTED:
-if (c->argv[0] != FMT_UNDEFINED_OBJECT &&
-c->argv[0] != FMT_ASSOCIATION) {
+if (c->argc == 0 ||
+   (c->argv[0] != FMT_UNDEFINED_OBJECT &&
+c->argv[0] != FMT_ASSOCIATION)) {
  usb_mtp_queue_result(s, RES_INVALID_OBJECT_FORMAT_CODE,
   c->trans, 0, 0, 0);
  return;
@@ -1273,8 +1278,9 @@ static void usb_mtp_command(MTPState *s, MTPControl *c)
  data_in = usb_mtp_get_object_props_supported(s, c);
  break;
  case CMD_GET_OBJECT_PROP_DESC:
-if (c->argv[1] != FMT_UNDEFINED_OBJECT &&
-c->argv[1] != FMT_ASSOCIATION) {
+if (c->argc > 1 &&
+   (c->argv[1] != FMT_UNDEFINED_OBJECT &&
+c->argv[1] != FMT_ASSOCIATION)) {
  usb_mtp_queue_result(s, RES_INVALID_OBJECT_FORMAT_CODE,
   c->trans, 0, 0, 0);
  return;

Re: [Qemu-devel] [PATCH for 2.10 16/35] usb/dev-mtp: fix use of uninitialized values

[Philippe Mathieu-Daudé]

On 07/25/2017 09:34 AM, Gerd Hoffmann wrote:

  case CMD_GET_OBJECT_INFO:

-o = usb_mtp_object_lookup(s, c->argv[0]);
+if (c->argc > 0) {
+o = usb_mtp_object_lookup(s, c->argv[0]);
+}


How about zero-initializing c->argv instead?


I checked the MTP specs rev. 1.1 and I understand the case argc == 0 
fits in "Invalid Parameter" section (F.2.30, code 0x201d).


So the correct patch is to queue a RES_INVALID_PARAMETER result.

I'll send another patch but since this require heavy testing this is 
probably 2.11 material now.


Regards,

Phil.

Re: [Qemu-devel] [SeaBIOS] [RFC PATCH v2 4/6] hw/pci: introduce bridge-only vendor-specific capability to provide some hints to firmware

[Laszlo Ersek]

On 07/26/17 23:54, Alexander Bezzubikov wrote:
> 2017-07-26 22:43 GMT+03:00 Michael S. Tsirkin :
>> On Sun, Jul 23, 2017 at 01:15:41AM +0300, Aleksandr Bezzubikov wrote:

>>> +PCIBridgeQemuCap cap;
>>
>> This leaks info to guest. You want to init all fields here:
>>
>> cap = {
>>  .len = 
>> };
> 
> I surely can do this for len field, but as Laszlo proposed
> we can use mutually exclusive fields,
> e.g. pref_32 and pref_64, the only way I have left
> is to use ternary operator (if we surely need this
> big initializer). Keeping some if's would look better,
> I think.

I think it's fine to use "if"s in order to set up the structure
partially / gradually, but then please clear the structure up-front:


  PCIBridgeQemuCap cap = { 0 };

(In general "{ 0 }" is the best initializer ever, because it can
zero-init a variable of *any* type at all. Gcc might complain about the
inexact depth of {} nesting of course, but it's nonetheless valid C.)

Or else add a memset-to-zero.

Or else, do just

  PCIBridgeQemuCap cap = { .len = ... };

which will zero-fill every other field. ("[...] all subobjects that are
not initialized explicitly shall be initialized implicitly the same as
objects that have static storage duration").

Thanks
Laszlo

Re: [Qemu-devel] [PATCH for 2.10 20/35] arm/boot: fix undefined instruction on secondary smp cpu bootloader

[Philippe Mathieu-Daudé]

On 07/24/2017 06:06 PM, Peter Maydell wrote:

On 24 July 2017 at 19:27, Philippe Mathieu-Daudé  wrote:

In a ARM multicore system, write_secondary_boot() only initializes fixups for
FIXUP_GIC_CPU_IF and FIXUP_BOOTREG, while smpboot[] also uses FIXUP_DSB.
This results in write_bootloader() using uninitialized fixupcontext[FIXUP_DSB]
instruction in the bootloader code...


Hmm? The code does:

 if (arm_feature(&cpu->env, ARM_FEATURE_V7)) {
 fixupcontext[FIXUP_DSB] = DSB_INSN;
 } else {
 fixupcontext[FIXUP_DSB] = CP15_DSB_INSN;
 }

so fixupcontext[FIXUP_DSB] is guaranteed initialized,
as are FIXUP_GIC_CPU_IF and FIXUP_BOOTREG, which are
the only fixups that the smpboot[] code uses.


Indeed :)

Sorry for the noise, I'll add few hints to the analyzer.

Re: [Qemu-devel] [PATCH for 2.10 03/35] thunk: check nb_fields is valid before continuing

[Philippe Mathieu-Daudé]

On 07/24/2017 03:37 PM, Eric Blake wrote:

On 07/24/2017 01:27 PM, Philippe Mathieu-Daudé wrote:

thunk.c:91:32: warning: Call to 'malloc' has an allocation size of 0 bytes
 se->field_offsets[i] = malloc(nb_fields * sizeof(int));
^~~

Reported-by: Clang Static Analyzer
Signed-off-by: Philippe Mathieu-Daudé 
---
  thunk.c | 5 -
  1 file changed, 4 insertions(+), 1 deletion(-)


Better would be fixing the code to use g_new0, and the corresponding free.


Ok, for 2.11 although (not a fix).

Also thunk* alloc'd are never free'd during process lifetime, so will 
keep like that (no g_free).

[Qemu-devel] How to debug crash in TCG code?

[BALATON Zoltan]

Hello,

I'm getting a segfault in generated code that I don't know how to debug 
further. The back trace shows:


Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7fffe87f7700 (LWP 24372)]
0x557ee0a1 in io_readx (env=0x7fffe88002a0, iotlbentry=0x7fffe8811d60, 
addr=3623882752, retaddr=140737096497196, size=2)
at accel/tcg/cputlb.c:766
766 if (mr->global_locking) {
(gdb) bt
#0  0x557ee0a1 in io_readx (env=0x7fffe88002a0, 
iotlbentry=0x7fffe8811d60, addr=3623882752, retaddr=140737096497196, size=2)
at accel/tcg/cputlb.c:766
#1  0x557eede9 in io_readw (env=0x7fffe88002a0, mmu_idx=1, index=4, 
addr=3623882752, retaddr=140737096497196)
at softmmu_template.h:104
#2  0x557ef1f0 in helper_be_lduw_mmu (env=0x7fffe88002a0, 
addr=3623882752, oi=145, retaddr=140737096497196)
at softmmu_template.h:208
#3  0x7fffe8a4b8d3 in code_gen_buffer ()
#4  0x557f69b8 in cpu_tb_exec (cpu=0x7fffe87f8010, itb=0x7fffe8a4b660 
)
at accel/tcg/cpu-exec.c:166
#5  0x557f769f in cpu_loop_exec_tb (cpu=0x7fffe87f8010, tb=0x7fffe8a4b660 
,
last_tb=0x7fffe87f6af8, tb_exit=0x7fffe87f6af4) at accel/tcg/cpu-exec.c:578
#6  0x557f7992 in cpu_exec (cpu=0x7fffe87f8010) at 
accel/tcg/cpu-exec.c:676
#7  0x557c2955 in tcg_cpu_exec (cpu=0x7fffe87f8010) at cpus.c:1270
#8  0x557c2b8c in qemu_tcg_rr_cpu_thread_fn (arg=0x7fffe87f8010) at 
cpus.c:1365
#9  0x75d515bd in start_thread () from /lib64/libpthread.so.0
#10 0x742d062d in clone () from /lib64/libc.so.6
(gdb) p mr
$1 = (MemoryRegion *) 0x0

This is happening while reading from an emulated ATAPI DVD and happens 
after several successful reads from the same device with similar calls 
succeeding without a problem until hitting the above error. The point 
where this happens seems to depend on the ammount of guest code executed. 
The more code is there, the sooner this happens. (This is running in 
TCG ppc-softmmu on x86_64 host in case that's relevant but I can't make 
an easy test case to reproduce it.)


First I thought it may be related to MTTCG or removing the iothread lock 
but I could also get the same with 791158d9, where the back trace is:


#0  0x557e1de5 in memory_region_access_valid (mr=0x0, addr=0, size=2, 
is_write=false) at memory.c:1204
#1  0x557e200a in memory_region_dispatch_read (mr=0x0, addr=0, 
pval=0x7fffe4854488, size=2, attrs=...)
at memory.c:1268
#2  0x557e7f9c in io_readx (env=0x77e232a0, 
iotlbentry=0x77e34d58, addr=3623882752,
 retaddr=140737066697996, size=2) at cputlb.c:506
#3  0x557e8a9e in io_readw (env=0x77e232a0, mmu_idx=1, index=4, 
addr=3623882752, retaddr=140737066697996)
at softmmu_template.h:104
#4  0x557e8eb0 in helper_be_lduw_mmu (env=0x77e232a0, 
addr=3623882752, oi=145, retaddr=140737066697996)
at softmmu_template.h:208
#5  0x7fffe6de05b3 in code_gen_buffer ()
#6  0x55783fca in cpu_tb_exec (cpu=0x77e1b010, itb=0x7fffe49a2080) 
at cpu-exec.c:164
#7  0x55784b97 in cpu_loop_exec_tb (cpu=0x77e1b010, 
tb=0x7fffe49a2080, last_tb=0x7fffe4854af8,
 tb_exit=0x7fffe4854af4, sc=0x7fffe4854b10) at cpu-exec.c:550
#8  0x55784ea0 in cpu_exec (cpu=0x77e1b010) at cpu-exec.c:655
#9  0x557c8da3 in tcg_cpu_exec (cpu=0x77e1b010) at cpus.c:1253
#10 0x557c900d in qemu_tcg_cpu_thread_fn (arg=0x77e1b010) at 
cpus.c:1345
#11 0x745b65bd in start_thread () from /lib64/libpthread.so.0
#12 0x742f262d in clone () from /lib64/libc.so.6

So it seems to be caused not by thread locking issues by recent changes 
but maybe by somehow referencing an invalid iotlb entry in a TB. My theory 
(without knowing anything about how this part of QEMU works) is that as 
code is executed instruction and data exceptions are triggered which make 
changes in TLB entries but this does not correctly invalidate a TB that 
already references this entry and this causes the crash when this happens 
(but it works until the TLB is not changed which explains why less code 
works and more code which makes these exceptions more frequent triggers it 
sooner). But I have no idea if this theory is correct or how to verify it 
and where to look for the problem and fix.


Does anyone have any idea that could help or point me to the right 
direction please?


Thank you,
BALATON Zoltan

Re: [Qemu-devel] [PATCH 2/2] iotests: Redirect stderr to stdout in 186

[Jeff Cody]

On Tue, Jul 25, 2017 at 05:56:44PM +0200, Max Reitz wrote:
> Without redirecting qemu's stderr to stdout, _filter_qemu will not apply
> to warnings.  This results in $QEMU_PROG not being replaced by QEMU_PROG
> which is not great if your qemu executable is not called
> qemu-system-x86_64 (e.g. qemu-system-i386).
> 
> Signed-off-by: Max Reitz 
> ---
>  tests/qemu-iotests/186 |  2 +-
>  tests/qemu-iotests/186.out | 12 ++--
>  2 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/tests/qemu-iotests/186 b/tests/qemu-iotests/186
> index ab83ee4..2b9f618 100755
> --- a/tests/qemu-iotests/186
> +++ b/tests/qemu-iotests/186
> @@ -56,7 +56,7 @@ function do_run_qemu()
>  done
>  fi
>  echo quit
> -) | $QEMU -S -nodefaults -display none -device virtio-scsi-pci -monitor 
> stdio "$@"
> +) | $QEMU -S -nodefaults -display none -device virtio-scsi-pci -monitor 
> stdio "$@" 2>&1
>  echo
>  }
>  
> diff --git a/tests/qemu-iotests/186.out b/tests/qemu-iotests/186.out
> index b8bf9a2..c8377fe 100644
> --- a/tests/qemu-iotests/186.out
> +++ b/tests/qemu-iotests/186.out
> @@ -442,28 +442,28 @@ ide0-cd0 (NODE_NAME): null-co:// (null-co, read-only)
>  Cache mode:   writeback
>  (qemu) quit
>  
> -qemu-system-x86_64: -drive if=scsi,driver=null-co: warning: bus=0,unit=0 is 
> deprecated with this machine type
>  Testing: -drive if=scsi,driver=null-co
>  QEMU X.Y.Z monitor - type 'help' for more information
> -(qemu) info block
> +(qemu) QEMU_PROG: -drive if=scsi,driver=null-co: warning: bus=0,unit=0 is 
> deprecated with this machine type
> +info block
>  scsi0-hd0 (NODE_NAME): null-co:// (null-co)
>  Attached to:  /machine/unattached/device[27]/scsi.0/legacy[0]
>  Cache mode:   writeback
>  (qemu) quit
>  
> -qemu-system-x86_64: -drive if=scsi,media=cdrom: warning: bus=0,unit=0 is 
> deprecated with this machine type
>  Testing: -drive if=scsi,media=cdrom
>  QEMU X.Y.Z monitor - type 'help' for more information
> -(qemu) info block
> +(qemu) QEMU_PROG: -drive if=scsi,media=cdrom: warning: bus=0,unit=0 is 
> deprecated with this machine type
> +info block
>  scsi0-cd0: [not inserted]
>  Attached to:  /machine/unattached/device[27]/scsi.0/legacy[0]
>  Removable device: not locked, tray closed
>  (qemu) quit
>  
> -qemu-system-x86_64: -drive if=scsi,driver=null-co,media=cdrom: warning: 
> bus=0,unit=0 is deprecated with this machine type
>  Testing: -drive if=scsi,driver=null-co,media=cdrom
>  QEMU X.Y.Z monitor - type 'help' for more information
> -(qemu) info block
> +(qemu) QEMU_PROG: -drive if=scsi,driver=null-co,media=cdrom: warning: 
> bus=0,unit=0 is deprecated with this machine type
> +info block
>  scsi0-cd0 (NODE_NAME): null-co:// (null-co, read-only)
>  Attached to:  /machine/unattached/device[27]/scsi.0/legacy[0]
>  Removable device: not locked, tray closed
> -- 
> 2.9.4
> 
> 

Reviewed-by: Jeff Cody 

Re: [Qemu-devel] [PATCH 1/2] iotests: Fix test 156

[Jeff Cody]

On Tue, Jul 25, 2017 at 05:56:43PM +0200, Max Reitz wrote:
> On one hand, the _make_test_img invocation for creating the target image
> was missing a -u because its backing file is not supposed to exist at
> that point.
> 
> On the other hand, nobody noticed probably because the backing file is
> created later on and _cleanup failed to remove it: The quotation marks
> were misplaced so bash tried to deleted a file literally called
> "$TEST_IMG{,.target}..." instead of resolving the globs.  Thus, the
> files stayed around after the first run and qemu-img create did not
> complain about a missing backing file on any run but the first.
> 
> Signed-off-by: Max Reitz 
> ---
>  tests/qemu-iotests/156 | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/qemu-iotests/156 b/tests/qemu-iotests/156
> index 2c4a06e..e75dc4d 100755
> --- a/tests/qemu-iotests/156
> +++ b/tests/qemu-iotests/156
> @@ -38,7 +38,7 @@ status=1# failure is the default!
>  _cleanup()
>  {
>  _cleanup_qemu
> -rm -f "$TEST_IMG{,.target}{,.backing,.overlay}"
> +rm -f "$TEST_IMG"{,.target}{,.backing,.overlay}
>  }
>  trap "_cleanup; exit \$status" 0 1 2 3 15
>  
> @@ -83,7 +83,7 @@ _send_qemu_cmd $QEMU_HANDLE \
>  'return'
>  
>  # Create target image
> -TEST_IMG="$TEST_IMG.target.overlay" _make_test_img -b "$TEST_IMG.target" 1M
> +TEST_IMG="$TEST_IMG.target.overlay" _make_test_img -u -b "$TEST_IMG.target" 
> 1M
>  
>  # Mirror snapshot
>  _send_qemu_cmd $QEMU_HANDLE \
> -- 
> 2.9.4
> 
> 

Reviewed-by: Jeff Cody 

Re: [Qemu-devel] [PATCH for 2.10 0/4] check dtc submodule is outdated

[Philippe Mathieu-Daudé]

On 07/26/2017 06:56 PM, Eric Blake wrote:

On 07/26/2017 04:47 PM, no-re...@patchew.org wrote:

This series failed build test on s390x host. Please find the details below.


[...]

=== PACKAGES === >> dtc-1.4.2-1.fc25.s390x




=== TEST BEGIN ===
Using CC: /home/fam/bin/cc

ERROR: fdt disabled but some requested targets require it.
You can turn off fdt only if you also disable all the system emulation
targets which need it (by specifying a cut down --target-list).


Well, your new configure check certainly fired!  The question is whether
it was supposed to (meaning patchew needs to tweak what it tries to
configure), or whether there's a logic bug in your patch (meaning it
fails when it shouldn't have).


It seems I missed something :(

Re: [Qemu-devel] [PATCH 1/4] configure: remember the user to run 'git submodule' command in source dir

[Philippe Mathieu-Daudé]

On 07/26/2017 06:51 PM, Eric Blake wrote:

On 07/26/2017 04:40 PM, Philippe Mathieu-Daudé wrote:

In the subject: s/remember/remind/


Signed-off-by: Philippe Mathieu-Daudé 
---
  configure | 5 +++--
  1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/configure b/configure
index f8b1d014d7..063de32773 100755
--- a/configure
+++ b/configure
@@ -502,6 +502,7 @@ if test -f "./configure"; then
  pwd_is_source_path="y"
  else
  pwd_is_source_path="n"
+git_submodule_path_info="(in ${source_path})"
  fi
  
  check_define() {

@@ -3313,7 +3314,7 @@ else
  error_exit "pixman >= 0.21.8 not present. Your options:" \
  "  (1) Preferred: Install the pixman devel package (any recent" \
  "  distro should have packages as Xorg needs pixman too)." \
-"  (2) Fetch the pixman submodule, using:" \
+"  (2) Fetch the pixman submodule, using: $git_submodule_path_info" \
  "  git submodule update --init pixman"


Pre-patch (or in-tree build), we're merely giving the user something
they can directly paste into their terminal.  But "(in /path/to/xyz)" is
not directly useful.  Can we spell it:

(2) Fetch the pixman submodule, using:
 ( cd $source_path &&
   git submodule update --init pixman
 )

for even easier copy-and-paste on the user's part?


Ok will update and resend for rc1

Re: [Qemu-devel] [PATCH for 2.10 0/4] check dtc submodule is outdated

[Eric Blake]

On 07/26/2017 04:47 PM, no-re...@patchew.org wrote:
> Hi,
> 
> This series failed build test on s390x host. Please find the details below.
> 
> Type: series
> Subject: [Qemu-devel] [PATCH for 2.10 0/4] check dtc submodule is outdated
> Message-id: 20170726214010.420-1-f4...@amsat.org
> 

> === TEST BEGIN ===
> Using CC: /home/fam/bin/cc
> 
> ERROR: fdt disabled but some requested targets require it.
>You can turn off fdt only if you also disable all the system emulation
>targets which need it (by specifying a cut down --target-list).

Well, your new configure check certainly fired!  The question is whether
it was supposed to (meaning patchew needs to tweak what it tries to
configure), or whether there's a logic bug in your patch (meaning it
fails when it shouldn't have).

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [RFC PATCH v2 4/6] hw/pci: introduce bridge-only vendor-specific capability to provide some hints to firmware

[Alexander Bezzubikov]

2017-07-26 22:43 GMT+03:00 Michael S. Tsirkin :
> On Sun, Jul 23, 2017 at 01:15:41AM +0300, Aleksandr Bezzubikov wrote:
>> On PCI init PCI bridges may need some
>> extra info about bus number to reserve, IO, memory and
>> prefetchable memory limits. QEMU can provide this
>> with special
>
> with a special
>
>> vendor-specific PCI capability.
>>
>> Sizes of limits match ones from
>> PCI Type 1 Configuration Space Header,
>> number of buses to reserve occupies only 1 byte
>> since it is the size of Subordinate Bus Number register.
>>
>> Signed-off-by: Aleksandr Bezzubikov 
>> ---
>>  hw/pci/pci_bridge.c | 27 +++
>>  include/hw/pci/pci_bridge.h | 18 ++
>>  2 files changed, 45 insertions(+)
>>
>> diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c
>> index 720119b..8ec6c2c 100644
>> --- a/hw/pci/pci_bridge.c
>> +++ b/hw/pci/pci_bridge.c
>> @@ -408,6 +408,33 @@ void pci_bridge_map_irq(PCIBridge *br, const char* 
>> bus_name,
>>  br->bus_name = bus_name;
>>  }
>>
>> +
>> +int pci_bridge_help_cap_init(PCIDevice *dev, int cap_offset,
>
> help? should be qemu_cap_init?
>
>> +  uint8_t bus_reserve, uint32_t io_limit,
>> +  uint16_t mem_limit, uint64_t pref_limit,
>> +  Error **errp)
>> +{
>> +size_t cap_len = sizeof(PCIBridgeQemuCap);
>> +PCIBridgeQemuCap cap;
>
> This leaks info to guest. You want to init all fields here:
>
> cap = {
>  .len = 
> };

I surely can do this for len field, but as Laszlo proposed
we can use mutually exclusive fields,
e.g. pref_32 and pref_64, the only way I have left
is to use ternary operator (if we surely need this
big initializer). Keeping some if's would look better,
I think.

>
>> +
>> +cap.len = cap_len;
>> +cap.bus_res = bus_reserve;
>> +cap.io_lim = io_limit & 0xFF;
>> +cap.io_lim_upper = io_limit >> 8 & 0x;
>> +cap.mem_lim = mem_limit;
>> +cap.pref_lim = pref_limit & 0x;
>> +cap.pref_lim_upper = pref_limit >> 16 & 0x;
>
> Please use pci_set_word etc or cpu_to_leXX.
>

Since now we've decided to avoid fields separation into  + ,
this bitmask along with pci_set_word are no longer needed.

> I think it's easiest to replace struct with a set of macros then
> pci_set_word does the work for you.
>

I don't really want to use macros here because structure
show us the whole capability layout and this can
decrease documenting efforts. More than that,
memcpy usage is very convenient here, and I wouldn't like
to lose it.

>
>> +
>> +int offset = pci_add_capability(dev, PCI_CAP_ID_VNDR,
>> +cap_offset, cap_len, errp);
>> +if (offset < 0) {
>> +return offset;
>> +}
>> +
>> +memcpy(dev->config + offset + 2, (char *)&cap + 2, cap_len - 2);
>
> +2 is yacky. See how virtio does it:
>
> memcpy(dev->config + offset + PCI_CAP_FLAGS, &cap->cap_len,
>cap->cap_len - PCI_CAP_FLAGS);
>
>

OK.

>> +return 0;
>> +}
>> +
>>  static const TypeInfo pci_bridge_type_info = {
>>  .name = TYPE_PCI_BRIDGE,
>>  .parent = TYPE_PCI_DEVICE,
>> diff --git a/include/hw/pci/pci_bridge.h b/include/hw/pci/pci_bridge.h
>> index ff7cbaa..c9f642c 100644
>> --- a/include/hw/pci/pci_bridge.h
>> +++ b/include/hw/pci/pci_bridge.h
>> @@ -67,4 +67,22 @@ void pci_bridge_map_irq(PCIBridge *br, const char* 
>> bus_name,
>>  #define  PCI_BRIDGE_CTL_DISCARD_STATUS   0x400   /* Discard timer 
>> status */
>>  #define  PCI_BRIDGE_CTL_DISCARD_SERR 0x800   /* Discard timer SERR# enable 
>> */
>>
>> +typedef struct PCIBridgeQemuCap {
>> +uint8_t id; /* Standard PCI capability header field */
>> +uint8_t next;   /* Standard PCI capability header field */
>> +uint8_t len;/* Standard PCI vendor-specific capability header field 
>> */
>> +uint8_t bus_res;
>> +uint32_t pref_lim_upper;
>
> Big endian? Ugh.
>

Agreed, and this's gonna to disappear with
the new layout.

>> +uint16_t pref_lim;
>> +uint16_t mem_lim;
>
> I'd say we need 64 bit for memory.
>

Why? Non-prefetchable MEMORY_LIMIT register is 16 bits long.

>> +uint16_t io_lim_upper;
>> +uint8_t io_lim;
>> +uint8_t padding;
>
> IMHO each type should have a special "don't care" flag
> that would mean "I don't know".
>
>

Don't know what? Now 0 is an indicator to do nothing with this field.

>> +} PCIBridgeQemuCap;
>
> You don't really need this struct in the header. And pls document all fields.
>
>> +
>> +int pci_bridge_help_cap_init(PCIDevice *dev, int cap_offset,
>> +  uint8_t bus_reserve, uint32_t io_limit,
>> +  uint16_t mem_limit, uint64_t pref_limit,
>> +  Error **errp);
>> +
>>  #endif /* QEMU_PCI_BRIDGE_H */
>> --
>> 2.7.4



--
Alexander Bezzubikov

Re: [Qemu-devel] [PATCH for 2.10 2/4] fdt: check fdt_required condition can be satisfied _after_ testing libfdt

[Eric Blake]

On 07/26/2017 04:40 PM, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  configure | 18 +-
>  1 file changed, 9 insertions(+), 9 deletions(-)

Reviewed-by: Eric Blake 

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH 1/4] configure: remember the user to run 'git submodule' command in source dir

[Eric Blake]

On 07/26/2017 04:40 PM, Philippe Mathieu-Daudé wrote:

In the subject: s/remember/remind/

> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  configure | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/configure b/configure
> index f8b1d014d7..063de32773 100755
> --- a/configure
> +++ b/configure
> @@ -502,6 +502,7 @@ if test -f "./configure"; then
>  pwd_is_source_path="y"
>  else
>  pwd_is_source_path="n"
> +git_submodule_path_info="(in ${source_path})"
>  fi
>  
>  check_define() {
> @@ -3313,7 +3314,7 @@ else
>  error_exit "pixman >= 0.21.8 not present. Your options:" \
>  "  (1) Preferred: Install the pixman devel package (any recent" \
>  "  distro should have packages as Xorg needs pixman too)." \
> -"  (2) Fetch the pixman submodule, using:" \
> +"  (2) Fetch the pixman submodule, using: $git_submodule_path_info" \
>  "  git submodule update --init pixman"

Pre-patch (or in-tree build), we're merely giving the user something
they can directly paste into their terminal.  But "(in /path/to/xyz)" is
not directly useful.  Can we spell it:

(2) Fetch the pixman submodule, using:
( cd $source_path &&
  git submodule update --init pixman
)

for even easier copy-and-paste on the user's part?

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH for 2.10 0/4] check dtc submodule is outdated

[no-reply]

Hi,

This series failed build test on s390x host. Please find the details below.

Type: series
Subject: [Qemu-devel] [PATCH for 2.10 0/4] check dtc submodule is outdated
Message-id: 20170726214010.420-1-f4...@amsat.org

=== TEST SCRIPT BEGIN ===
#!/bin/bash
# Testing script will be invoked under the git checkout with
# HEAD pointing to a commit that has the patches applied on top of "base"
# branch
set -e
echo "=== ENV ==="
env
echo "=== PACKAGES ==="
rpm -qa
echo "=== TEST BEGIN ==="
CC=$HOME/bin/cc
INSTALL=$PWD/install
BUILD=$PWD/build
echo -n "Using CC: "
realpath $CC
mkdir -p $BUILD $INSTALL
SRC=$PWD
cd $BUILD
$SRC/configure --cc=$CC --prefix=$INSTALL
make -j4
# XXX: we need reliable clean up
# make check -j4 V=1
make install
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag] patchew/20170726214010.420-1-f4...@amsat.org -> 
patchew/20170726214010.420-1-f4...@amsat.org
Switched to a new branch 'test'
4d01ae8 fdt: compile dtc submodule to check it is up-to-date
7503360 fdt: probe for v1.4.2 using fdt_setprop_inplace_namelen_partial()
0664a64 fdt: check fdt_required condition can be satisfied _after_ testing 
libfdt
cb07f9e configure: remember the user to run 'git submodule' command in source 
dir

=== OUTPUT BEGIN ===
=== ENV ===
XDG_SESSION_ID=4
SHELL=/bin/sh
USER=fam
PATCHEW=/home/fam/patchew/patchew-cli -s http://patchew.org --nodebug
PATH=/usr/bin:/bin
PWD=/var/tmp/patchew-tester-tmp-2vn8fdmj/src
LANG=en_US.UTF-8
HOME=/home/fam
SHLVL=2
LOGNAME=fam
DBUS_SESSION_BUS_ADDRESS=unix:path=/run/user/1012/bus
XDG_RUNTIME_DIR=/run/user/1012
_=/usr/bin/env
=== PACKAGES ===
gpg-pubkey-873529b8-54e386ff
xz-libs-5.2.2-2.fc24.s390x
libxshmfence-1.2-3.fc24.s390x
giflib-4.1.6-15.fc24.s390x
trousers-lib-0.3.13-6.fc24.s390x
ncurses-base-6.0-6.20160709.fc25.noarch
gmp-6.1.1-1.fc25.s390x
libidn-1.33-1.fc25.s390x
slang-2.3.0-7.fc25.s390x
pkgconfig-0.29.1-1.fc25.s390x
alsa-lib-1.1.1-2.fc25.s390x
yum-metadata-parser-1.1.4-17.fc25.s390x
python3-slip-dbus-0.6.4-4.fc25.noarch
python2-cssselect-0.9.2-1.fc25.noarch
createrepo_c-libs-0.10.0-6.fc25.s390x
initscripts-9.69-1.fc25.s390x
parted-3.2-21.fc25.s390x
flex-2.6.0-3.fc25.s390x
colord-libs-1.3.4-1.fc25.s390x
python-osbs-client-0.33-3.fc25.noarch
perl-Pod-Simple-3.35-1.fc25.noarch
python2-simplejson-3.10.0-1.fc25.s390x
brltty-5.4-2.fc25.s390x
librados2-10.2.4-2.fc25.s390x
tcp_wrappers-7.6-83.fc25.s390x
libcephfs_jni1-10.2.4-2.fc25.s390x
nettle-devel-3.3-1.fc25.s390x
bzip2-devel-1.0.6-21.fc25.s390x
libuuid-2.28.2-2.fc25.s390x
python3-dnf-1.1.10-6.fc25.noarch
texlive-kpathsea-doc-svn41139-33.fc25.1.noarch
openssh-7.4p1-4.fc25.s390x
texlive-kpathsea-bin-svn40473-33.20160520.fc25.1.s390x
texlive-graphics-svn41015-33.fc25.1.noarch
texlive-dvipdfmx-def-svn40328-33.fc25.1.noarch
texlive-mfware-svn40768-33.fc25.1.noarch
texlive-texlive-scripts-svn41433-33.fc25.1.noarch
texlive-euro-svn22191.1.1-33.fc25.1.noarch
texlive-etex-svn37057.0-33.fc25.1.noarch
texlive-iftex-svn29654.0.2-33.fc25.1.noarch
texlive-palatino-svn31835.0-33.fc25.1.noarch
texlive-texlive-docindex-svn41430-33.fc25.1.noarch
texlive-xunicode-svn30466.0.981-33.fc25.1.noarch
texlive-koma-script-svn41508-33.fc25.1.noarch
texlive-pst-grad-svn15878.1.06-33.fc25.1.noarch
texlive-pst-blur-svn15878.2.0-33.fc25.1.noarch
texlive-jknapltx-svn19440.0-33.fc25.1.noarch
texinfo-6.1-4.fc25.s390x
openssl-devel-1.0.2k-1.fc25.s390x
gdk-pixbuf2-2.36.6-1.fc25.s390x
nspr-4.14.0-2.fc25.s390x
nss-softokn-freebl-3.30.2-1.0.fc25.s390x
jansson-2.10-2.fc25.s390x
fedora-repos-25-4.noarch
python3-libs-3.5.3-6.fc25.s390x
perl-Errno-1.25-387.fc25.s390x
acl-2.2.52-13.fc25.s390x
pcre2-utf16-10.23-8.fc25.s390x
pango-1.40.5-1.fc25.s390x
systemd-pam-231-17.fc25.s390x
python2-gluster-3.10.4-1.fc25.s390x
NetworkManager-libnm-1.4.4-5.fc25.s390x
selinux-policy-3.13.1-225.18.fc25.noarch
poppler-0.45.0-5.fc25.s390x
ccache-3.3.4-1.fc25.s390x
valgrind-3.12.0-9.fc25.s390x
perl-open-1.10-387.fc25.noarch
libaio-0.3.110-6.fc24.s390x
libfontenc-1.1.3-3.fc24.s390x
lzo-2.08-8.fc24.s390x
isl-0.14-5.fc24.s390x
libXau-1.0.8-6.fc24.s390x
linux-atm-libs-2.5.1-14.fc24.s390x
libXext-1.3.3-4.fc24.s390x
libXxf86vm-1.1.4-3.fc24.s390x
bison-3.0.4-4.fc24.s390x
perl-srpm-macros-1-20.fc25.noarch
gawk-4.1.3-8.fc25.s390x
libwayland-client-1.12.0-1.fc25.s390x
perl-Exporter-5.72-366.fc25.noarch
perl-version-0.99.17-1.fc25.s390x
fftw-libs-double-3.3.5-3.fc25.s390x
libssh2-1.8.0-1.fc25.s390x
ModemManager-glib-1.6.4-1.fc25.s390x
newt-python3-0.52.19-2.fc25.s390x
python-munch-2.0.4-3.fc25.noarch
python-bugzilla-1.2.2-4.fc25.noarch
libedit-3.1-16.20160618cvs.fc25.s390x
createrepo_c-0.10.0-6.fc25.s390x
device-mapper-multipath-libs-0.4.9-83.fc25.s390x
yum-3.4.3-510.fc25.noarch
dracut-config-rescue-044-78.fc25.s390x
mozjs17-17.0.0-16.fc25.s390x
libselinux-2.5-13.fc25.s390x
libgo-devel-6.3.1-1.fc25.s390x
python2-pyparsing-2.1.10-1.fc25.noarch
cairo-gobject-1.14.8-1.fc25.s390x
ethtool-4.8-1.fc25.s390x
xorg-x11-p

Re: [Qemu-devel] KVM "fake DAX" flushing interface - discussion

[Dan Williams]

On Wed, Jul 26, 2017 at 2:27 PM, Rik van Riel  wrote:
> On Wed, 2017-07-26 at 09:47 -0400, Pankaj Gupta wrote:
>> >
>> Just want to summarize here(high level):
>>
>> This will require implementing new 'virtio-pmem' device which
>> presents
>> a DAX address range(like pmem) to guest with read/write(direct
>> access)
>> & device flush functionality. Also, qemu should implement
>> corresponding
>> support for flush using virtio.
>>
> Alternatively, the existing pmem code, with
> a flush-only block device on the side, which
> is somehow associated with the pmem device.
>
> I wonder which alternative leads to the least
> code duplication, and the least maintenance
> hassle going forward.

I'd much prefer to have another driver. I.e. a driver that refactors
out some common pmem details into a shared object and can attach to
ND_DEVICE_NAMESPACE_{IO,PMEM}. A control device on the side seems like
a recipe for confusion.

With a $new_driver in hand you can just do:

   modprobe $new_driver
   echo $namespace > /sys/bus/nd/drivers/nd_pmem/unbind
   echo $namespace > /sys/bus/nd/drivers/$new_driver/new_id
   echo $namespace > /sys/bus/nd/drivers/$new_driver/bind

...and the guest can arrange for $new_driver to be the default, so you
don't need to do those steps each boot of the VM, by doing:

echo "blacklist nd_pmem" > /etc/modprobe.d/virt-dax-flush.conf
echo "alias nd:t4* $new_driver" >> /etc/modprobe.d/virt-dax-flush.conf
echo "alias nd:t5* $new_driver" >> /etc/modprobe.d/virt-dax-flush.conf

[Qemu-devel] [RFC PATCH for 2.10 4/4] fdt: compile dtc submodule to check it is up-to-date

[Philippe Mathieu-Daudé]

Reported-by: John Arbuckle 
Message-Id: <65ed9743-b53c-4e6a-866f-c88365091...@gmail.com>
Signed-off-by: Philippe Mathieu-Daudé 
---
 configure | 24 +++-
 1 file changed, 19 insertions(+), 5 deletions(-)

diff --git a/configure b/configure
index 2d803d6a77..386262ec33 100755
--- a/configure
+++ b/configure
@@ -3575,15 +3575,29 @@ EOF
 fdt=yes
   elif test -d ${source_path}/dtc/libfdt ; then
 # have submodule DTC - use it
-fdt=yes
 dtc_internal="yes"
 mkdir -p dtc
 if [ "$pwd_is_source_path" != "y" ] ; then
-   symlink "$source_path/dtc/Makefile" "dtc/Makefile"
-   symlink "$source_path/dtc/scripts" "dtc/scripts"
+  for f in $(cd $source_path/dtc && ls Makefile* *.?); do
+symlink "$source_path/dtc/$f" "dtc/$f"
+  done
+  for d in scripts libfdt tests; do
+symlink "$source_path/dtc/$d" "dtc/$d"
+  done
+  SRC_PATH=$source_path
+fi
+make -C dtc 1>/dev/null
+
+fdt_cflags="-I${source_path}/dtc/libfdt"
+fdt_libs="-L$(pwd)/dtc/libfdt $fdt_libs"
+if compile_prog "$fdt_cflags" "$fdt_libs" ; then
+  fdt=yes
+else
+  error_exit "Your DTC submodule is outdated. Your options:" \
+"  (1) Preferred: Install the DTC (libfdt) devel package" \
+"  (2) Update the DTC submodule, using: $git_submodule_path_info" \
+"  git submodule update --init dtc"
 fi
-fdt_cflags="-I\$(SRC_PATH)/dtc/libfdt"
-fdt_libs="-L\$(BUILD_DIR)/dtc/libfdt $fdt_libs"
   elif test "$fdt" = "yes" ; then
 # have neither and want - prompt for system/submodule install
 error_exit "DTC (libfdt) version >= 1.4.2 not present. Your options:" \
-- 
2.13.3

[Qemu-devel] [PATCH for 2.10 3/4] fdt: probe for v1.4.2 using fdt_setprop_inplace_namelen_partial()

[Philippe Mathieu-Daudé]

instead of fdt_first_subnode() which is v1.4.0

Signed-off-by: Philippe Mathieu-Daudé 
---
 configure | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/configure b/configure
index 0d5bdb3ae9..2d803d6a77 100755
--- a/configure
+++ b/configure
@@ -3565,7 +3565,10 @@ if test "$fdt" != "no" ; then
   cat > $TMPC << EOF
 #include 
 #include 
-int main(void) { fdt_first_subnode(0, 0); return 0; }
+int main(void) {
+fdt_setprop_inplace_namelen_partial(0, 0, 0, 0, 0, 0, 0);
+return 0;
+}
 EOF
   if compile_prog "" "$fdt_libs" ; then
 # system DTC is good - use it
-- 
2.13.3

[Qemu-devel] [PATCH for 2.10 0/4] check dtc submodule is outdated

[Philippe Mathieu-Daudé]

On http://lists.nongnu.org/archive/html/qemu-devel/2017-07/msg07987.html John
hit the following error:

hw/core/loader-fit.c:105:41: error: expected expression
*addr = fdt32_to_cpu(*(fdt32_t *)prop);
^
having his ./configure --target-list=mips64el-softmmu on OSX returning:
fdt support   yes

It seems his dtc submodule was out of date but the ./configure did not emit
warning. I could reproduce checking out dtc tag v1.4.1.

patch 1: if you are out of the source directory "git submodule" fails, try to
improve the warning remembering the user to run this command in source/.

patch 2: a check exists to warn --disable-fdt --target-list=mips64el-softmmu
is invalid. move the warning to also warn AFTER checking if libfdt is
available and sane.

patch 3: fdt_first_subnode() is available since v1.4.0 while we are requiring
v1.4.2, better check a v1.4.2 function such as:
fdt_setprop_inplace_namelen_partial()

patch 4: if no system libdtc and submodule present, compile the dtc submodule
and verify it is at least v1.4.2. Prefixed RFC because I'm not sure about
these 3 lines:

+make -C dtc 1>/dev/null

yes, we need to build the libdtc to be able to run the compile_prog link step

+fdt_cflags="-I${source_path}/dtc/libfdt"

$source_path seems ok...

+fdt_libs="-L$(pwd)/dtc/libfdt $fdt_libs"

maybe there is a better option than `pwd`

how to reproduce:

((v2.10.0-rc0))$ (cd dtc && git checkout v1.3.0)
((v2.10.0-rc0))$ ./configure --target-list=mips64el-softmmu 
--extra-cflags=-fmax-errors=1 | fgrep fdt
fdt support   yes
((v2.10.0-rc0))$ make hw/core/loader-fit.o
hw/core/loader-fit.c: In function ‘fit_image_addr’:
hw/core/loader-fit.c:105:32: error: ‘fdt32_t’ undeclared (first use in this 
function)
 *addr = fdt32_to_cpu(*(fdt32_t *)prop);
^~~
compilation terminated due to -fmax-errors=1.
rules.mak:66: recipe for target 'hw/core/loader-fit.o' failed

(dtc-fix)$ mkdir test && cd test
(dtc-fix)$ ../configure --target-list=mips64el-softmmu 
--extra-cflags=-fmax-errors=1
ERROR: Your DTC submodule is outdated. Your options:
 (1) Preferred: Install the DTC (libfdt) devel package
 (2) Update the DTC submodule, using: (in /home/phil/source/qemu)
 git submodule update --init dtc

Regards,

Phil.

Philippe Mathieu-Daudé (4):
  configure: remember the user to run 'git submodule' command in source dir
  fdt: check fdt_required condition can be satisfied _after_ testing libfdt
  fdt: probe for v1.4.2 using fdt_setprop_inplace_namelen_partial()
  fdt: compile dtc submodule to check it is up-to-date

 configure | 52 +++-
 1 file changed, 35 insertions(+), 17 deletions(-)

-- 
2.13.3

[Qemu-devel] [PATCH for 2.10 2/4] fdt: check fdt_required condition can be satisfied _after_ testing libfdt

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé 
---
 configure | 18 +-
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/configure b/configure
index 063de32773..0d5bdb3ae9 100755
--- a/configure
+++ b/configure
@@ -3558,15 +3558,6 @@ for target in $target_list; do
   esac
 done
 
-if test "$fdt_required" = "yes"; then
-  if test "$fdt" = "no"; then
-error_exit "fdt disabled but some requested targets require it." \
-  "You can turn off fdt only if you also disable all the system emulation" 
\
-  "targets which need it (by specifying a cut down --target-list)."
-  fi
-  fdt=yes
-fi
-
 if test "$fdt" != "no" ; then
   fdt_libs="-lfdt"
   # explicitly check for libfdt_env.h as it is missing in some stable installs
@@ -3603,6 +3594,15 @@ EOF
   fi
 fi
 
+if test "$fdt_required" = "yes"; then
+  if test "$fdt" = "no"; then
+error_exit "fdt disabled but some requested targets require it." \
+  "You can turn off fdt only if you also disable all the system emulation" 
\
+  "targets which need it (by specifying a cut down --target-list)."
+  fi
+  fdt=yes
+fi
+
 libs_softmmu="$libs_softmmu $fdt_libs"
 
 ##
-- 
2.13.3

[Qemu-devel] [PATCH 1/4] configure: remember the user to run 'git submodule' command in source dir

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé 
---
 configure | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/configure b/configure
index f8b1d014d7..063de32773 100755
--- a/configure
+++ b/configure
@@ -502,6 +502,7 @@ if test -f "./configure"; then
 pwd_is_source_path="y"
 else
 pwd_is_source_path="n"
+git_submodule_path_info="(in ${source_path})"
 fi
 
 check_define() {
@@ -3313,7 +3314,7 @@ else
 error_exit "pixman >= 0.21.8 not present. Your options:" \
 "  (1) Preferred: Install the pixman devel package (any recent" \
 "  distro should have packages as Xorg needs pixman too)." \
-"  (2) Fetch the pixman submodule, using:" \
+"  (2) Fetch the pixman submodule, using: $git_submodule_path_info" \
 "  git submodule update --init pixman"
   fi
   mkdir -p pixman/pixman
@@ -3593,7 +3594,7 @@ EOF
 # have neither and want - prompt for system/submodule install
 error_exit "DTC (libfdt) version >= 1.4.2 not present. Your options:" \
 "  (1) Preferred: Install the DTC (libfdt) devel package" \
-"  (2) Fetch the DTC submodule, using:" \
+"  (2) Fetch the DTC submodule, using: $git_submodule_path_info" \
 "  git submodule update --init dtc"
   else
 # don't have and don't want
-- 
2.13.3

Re: [Qemu-devel] KVM "fake DAX" flushing interface - discussion

[Rik van Riel]

On Wed, 2017-07-26 at 09:47 -0400, Pankaj Gupta wrote:
> > 
> Just want to summarize here(high level):
> 
> This will require implementing new 'virtio-pmem' device which
> presents 
> a DAX address range(like pmem) to guest with read/write(direct
> access)
> & device flush functionality. Also, qemu should implement
> corresponding
> support for flush using virtio.
> 
Alternatively, the existing pmem code, with
a flush-only block device on the side, which
is somehow associated with the pmem device.

I wonder which alternative leads to the least
code duplication, and the least maintenance
hassle going forward.

-- 
All rights reversed

signature.asc
Description: This is a digitally signed message part

Re: [Qemu-devel] [sw-dev] RFC: QEMU RISC-V modular ISA decoding

[Michael Clark]

> On 27 Jul 2017, at 8:58 AM, kr...@berkeley.edu wrote:
> 
> 
> Given that one of the goals of RISC-V is extensibility, it would be
> nice if the QEMU port was done in a way to make it easier to extend by
> third parties, including other automated tools.  I'm sure that, over
> time, the preprocessor can be improved to automatically incorporate
> optimizations for better performance.

I had a look at the s390x code in more detail.

It essentially does some length and major opcode unpacking to 
construct/linearise the opcode space, and would be equivalent to constructing a 
15-bit opcode for RISC-V. It does some pre-decoding to find length, majors, and 
has a switch on majors to find instruction type, and then append minors, if any.

- https://hastebin.com/taceyuyore.cpp 

It would be akin to something list this (at a high-level):

len = inst_len(inst)/* bits[1:0] */
op = major_opcode(inst) /* bits[6:2] */

switch op
- rvc
/* hairy bit */
- r-type
op2 = funct7 << 3 | funct3
- i-type
- s-type
op2 = funct3
- u-type
op2 = 0

op = op | op2 << 5

Re: [Qemu-devel] [sw-dev] RFC: QEMU RISC-V modular ISA decoding

[krste]

Given that one of the goals of RISC-V is extensibility, it would be
nice if the QEMU port was done in a way to make it easier to extend by
third parties, including other automated tools.  I'm sure that, over
time, the preprocessor can be improved to automatically incorporate
optimizations for better performance.

Krste

> On Wed, 26 Jul 2017 18:36:28 +0100, "Richard W.M. Jones" 
>  said:

| On Wed, Jul 26, 2017 at 02:00:14PM +0200, Bastian Koppelmann wrote:
|| Hi Samuel,
|| 
|| On 07/25/2017 04:31 PM, Samuel Falvo II wrote:
|| > For those of us who are not in the know, how does target/s390 decoding 
work?
|| 
|| sorry about that. I was going into this with a QEMU-dev mindset :)
|| 
|| The basic idea of s390x is to have every instruction + instruction
|| formats specified in files that are parsed by the preprocessor and then
|| used through preprocessor magic to generate switch-case statements for
|| insn selection and data structures filled with the decoded data.
|| 
|| s390x has two files:
|| - insn-data.def -> contains all the instructions, including opcodes,
|| name, ref to insn specific translate function,
|| ref to insn format, and some more
|| - insn-format.def -> contains all the instruction formats
|| 
|| these are then used to automatically generate the switch-case statements
|| and decoding code.

| I looked at the s390x TCG code for the first time now and this is a
| far more sensible way of doing it.  We should do it for all the arches :-)
| I wonder if there's a performance penalty?

| Rich.

|| If you want to extend this, you add your own insn format to the
|| insn-format.def files add functions for decoding parameters in
|| translate.c. And then add your insn referencing the new format to
|| insn-def.data and add translation functions for each of them.
|| 
|| The main benefit here is that you don't have to bother with writing all
|| that boring glue code.
|| 
|| I hope that helped :)
|| 
|| Cheers,
|| Bastian
|| 
|| > 
|| > I've maintained a 65816 emulator
|| > (https://bitbucket.org/kc5tja/lib65816/src) which also uses a giant
|| > case construct.  This method is used because it's fast.  Any
|| > alternative approaches you decide to take might well work and satisfy
|| > extensibility requirements, but it'll likely take a performance hit as
|| > well.
|| > 
|| > On Tue, Jul 25, 2017 at 6:04 AM, Bastian Koppelmann
|| >  wrote:
|| >> Hi QEMU devs, hi risc-v-sw devs,
|| >>
|| >> I'm posting this cross mailing list since I'd like to get feedback from
|| >> the both sides.
|| >>
|| >> Right now the RISC-V port for QEMU uses the classic decoding scheme of
|| >> one function decoding the first opcode (and prefixes) and then branches
|| >> to different functions for decoding the rest (as in target/arm or most
|| >> of the other targets). This is all done using switch, case statements.
|| >>
|| >> This is a little bit tricky to extend, especially for third parties. I
|| >> don't think it's too bad, but it can definitely be improved and I really
|| >> like the way target/s390x does it, but I'm not sure about it's drawbacks.
|| >>
|| >> I see three options to proceed from here:
|| >>
|| >> 1) Completely move to a decoding scheme similar to target/s390x in
|| >>QEMU. On the plus side it make it super easy to add new
|| >>instructions and/or new instruction formats, and reduces decoding
|| >>errors. I don't know the major drawbacks to this approach, maybe
|| >>performance. Does anyone know? Other than that it needs a major
|| >>rewrite of the decoder, which will take some time and thus delay
|| >>the development of RISC-V QEMU upstream. (I think RV32/64I can
|| >>be left as is, since everybody has to implement it anyways)
|| >>
|| >> 2) The compromise: Leave the core as is, i.e. RV32GC, and provide a
|| >>nice interface for any other extension similar to target/s390.
|| >>The big plus here is that no code needs to be changed and only
|| >>the interface needs to be added. We don't add any performance
|| >>overhead (if s390x-style decoding adds any), but this might
|| >>result in nobody using it, since they don't know about the
|| >>interface and they just hack their stuff in. Then it was a waste
|| >>of our time to implement the interface.
|| >>
|| >> 3) The status quo. Just leave it as is.
|| >>
|| >> Any comments?
|| >>
|| >> Cheers,
|| >> Bastian
|| >>
|| >>
|| >>
|| >> --
|| >> You received this message because you are subscribed to the Google Groups 
"RISC-V SW Dev" group.
|| >> To unsubscribe from this group and stop receiving emails from it, send an 
email to sw-dev+unsubscr...@groups.riscv.org.
|| >> To post to this group, send email to sw-...@groups.riscv.org.
|| >> Visit this group at 
https://groups.google.com/a/groups.riscv.org/group/sw-dev/.
|| >> To view this discussion on the web visit 
https://groups.google.com/a/groups.riscv.org/d/msgid/sw-dev/e071dd23-8d19-

Re: [Qemu-devel] hw/core/loader-fit.c:105:41: error: expected expression

[Philippe Mathieu-Daudé]

On 07/26/2017 01:46 PM, Programmingkid wrote:

On Jul 26, 2017, at 6:24 AM, Peter Maydell  wrote:
On 26 July 2017 at 06:15, Programmingkid  wrote:

On Jul 26, 2017, at 12:13 AM, Philippe Mathieu-Daudé  wrote:

Hi John,

On 07/25/2017 07:55 PM, Programmingkid wrote:

While compiling the mips64el-softmmu target I encountered these errors:
  CC  hw/display/g364fb.o
hw/core/loader-fit.c:105:41: error: expected expression
*addr = fdt32_to_cpu(*(fdt32_t *)prop);
^
hw/core/loader-fit.c:105:32: error: use of undeclared identifier 'fdt32_t'


It seems you are missing the libfdt headers, so indeed you found bug.


Configure requires libfdt to exist to enable mips64el-softmmu,
so something is going wrong with our configure test compared
to how QEMU itself is being built.


That appears to help make things move past the loader-fit.c file. Here are the 
new errors:

  CC  hw/core/loader-fit.o
  CC  hw/dma/rc4030.o
  CC  hw/ide/via.o


It looks you ran "make -j3" firing 3 CC tasks.


hw/core/loader-fit.c:105:41: error: expected expression
*addr = fdt32_to_cpu(*(fdt32_t *)prop);
^


...this is still failing on loader-fit.c in the same way.


And also the ./configure output, I'm interested by:

"fdt support   no”


Actually it was "fdt support   yes”.


Configure thinks the fdt headers are available…


They appear to be available. The dtc/libfdt folder does have the files QEMU 
uses.




./configure --target-list=mips64el-softmmu
Install prefix/usr/local
BIOS directory/usr/local/share/qemu
binary directory  /usr/local/bin
library directory /usr/local/lib
module directory  /usr/local/lib/qemu
libexec directory /usr/local/libexec
include directory /usr/local/include
config directory  /usr/local/etc
local state directory   /usr/local/var
Manual directory  /usr/local/share/man
ELF interp prefix /usr/gnemul/qemu-%M
Source path   /Users/john/Documents/Development/Projects/Qemu/qemu-git
C compilercc
Host C compiler   cc
C++ compiler  c++
Objective-C compiler clang
ARFLAGS   rv
CFLAGS-O2 -g
QEMU_CFLAGS   -I/usr/local/Cellar/pixman/0.34.0/include/pixman-1 
-I$(SRC_PATH)/dtc/libfdt -D_REENTRANT -I/usr/local/Cellar/glib/2.46.2/include


...and it's put our local copy of libfdt on the include path.

The fdt32_t type that the compiler is complaining
about ought to be defined in the header in
dtc/libfdt/libfdt_env.h.


The fdt32_t type is not there. Here is the full libfdt_env.h file:

#ifndef _LIBFDT_ENV_H
#define _LIBFDT_ENV_H

#include 
#include 
#include 

#define _B(n)   ((unsigned long long)((uint8_t *)&x)[n])
static inline uint32_t fdt32_to_cpu(uint32_t x)
{
return (_B(0) << 24) | (_B(1) << 16) | (_B(2) << 8) | _B(3);
}
#define cpu_to_fdt32(x) fdt32_to_cpu(x)

static inline uint64_t fdt64_to_cpu(uint64_t x)
{
return (_B(0) << 56) | (_B(1) << 48) | (_B(2) << 40) | (_B(3) << 32)
| (_B(4) << 24) | (_B(5) << 16) | (_B(6) << 8) | _B(7);
}
#define cpu_to_fdt64(x) fdt64_to_cpu(x)
#undef _B

#endif /* _LIBFDT_ENV_H */



I wonder if you have an old copy of that submodule;
if so then "git submodule update" ought to fix it.


I’m not sure how to obtain the version of libfdt. I did see a file called 
version.lds.
At the top of this file is this text: "LIBFDT_1.2 {"
So maybe I am using version 1.2. Perhaps the configure test should declare a 
variable
of the fdt32_t type in the test code.

After doing a git submodule update qemu-system-mips64el compiled successfully. 
Thank you.


Oops I wanted to ask your "git submodule status" output before that, to 
reproduce your bug. Anyway checking out a pre-v1.4.2 is enough.


Peter I guess I found the culprit, compile_prog is not called for dtc 
submodule. I'll send a patch.


Regards,

Phil.

Re: [Qemu-devel] hw/core/loader-fit.c:105:41: error: expected expression

[Philippe Mathieu-Daudé]

On 07/26/2017 01:46 PM, Programmingkid wrote:

On Jul 26, 2017, at 6:24 AM, Peter Maydell  wrote:
On 26 July 2017 at 06:15, Programmingkid  wrote:

On Jul 26, 2017, at 12:13 AM, Philippe Mathieu-Daudé  wrote:

Hi John,

On 07/25/2017 07:55 PM, Programmingkid wrote:

While compiling the mips64el-softmmu target I encountered these errors:
  CC  hw/display/g364fb.o
hw/core/loader-fit.c:105:41: error: expected expression
*addr = fdt32_to_cpu(*(fdt32_t *)prop);
^
hw/core/loader-fit.c:105:32: error: use of undeclared identifier 'fdt32_t'


It seems you are missing the libfdt headers, so indeed you found bug.


Configure requires libfdt to exist to enable mips64el-softmmu,
so something is going wrong with our configure test compared
to how QEMU itself is being built.


That appears to help make things move past the loader-fit.c file. Here are the 
new errors:

  CC  hw/core/loader-fit.o
  CC  hw/dma/rc4030.o
  CC  hw/ide/via.o


It looks you ran "make -j3" firing 3 CC tasks.


hw/core/loader-fit.c:105:41: error: expected expression
*addr = fdt32_to_cpu(*(fdt32_t *)prop);
^


...this is still failing on loader-fit.c in the same way.


And also the ./configure output, I'm interested by:

"fdt support   no”


Actually it was "fdt support   yes”.


Configure thinks the fdt headers are available…


They appear to be available. The dtc/libfdt folder does have the files QEMU 
uses.




./configure --target-list=mips64el-softmmu
Install prefix/usr/local
BIOS directory/usr/local/share/qemu
binary directory  /usr/local/bin
library directory /usr/local/lib
module directory  /usr/local/lib/qemu
libexec directory /usr/local/libexec
include directory /usr/local/include
config directory  /usr/local/etc
local state directory   /usr/local/var
Manual directory  /usr/local/share/man
ELF interp prefix /usr/gnemul/qemu-%M
Source path   /Users/john/Documents/Development/Projects/Qemu/qemu-git
C compilercc
Host C compiler   cc
C++ compiler  c++
Objective-C compiler clang
ARFLAGS   rv
CFLAGS-O2 -g
QEMU_CFLAGS   -I/usr/local/Cellar/pixman/0.34.0/include/pixman-1 
-I$(SRC_PATH)/dtc/libfdt -D_REENTRANT -I/usr/local/Cellar/glib/2.46.2/include


...and it's put our local copy of libfdt on the include path.

The fdt32_t type that the compiler is complaining
about ought to be defined in the header in
dtc/libfdt/libfdt_env.h.


The fdt32_t type is not there. Here is the full libfdt_env.h file:

#ifndef _LIBFDT_ENV_H
#define _LIBFDT_ENV_H

#include 
#include 
#include 

#define _B(n)   ((unsigned long long)((uint8_t *)&x)[n])
static inline uint32_t fdt32_to_cpu(uint32_t x)
{
return (_B(0) << 24) | (_B(1) << 16) | (_B(2) << 8) | _B(3);
}
#define cpu_to_fdt32(x) fdt32_to_cpu(x)

static inline uint64_t fdt64_to_cpu(uint64_t x)
{
return (_B(0) << 56) | (_B(1) << 48) | (_B(2) << 40) | (_B(3) << 32)
| (_B(4) << 24) | (_B(5) << 16) | (_B(6) << 8) | _B(7);
}
#define cpu_to_fdt64(x) fdt64_to_cpu(x)
#undef _B

#endif /* _LIBFDT_ENV_H */



I wonder if you have an old copy of that submodule;
if so then "git submodule update" ought to fix it.


I’m not sure how to obtain the version of libfdt. I did see a file called 
version.lds.
At the top of this file is this text: "LIBFDT_1.2 {"
So maybe I am using version 1.2. Perhaps the configure test should declare a 
variable
of the fdt32_t type in the test code.

After doing a git submodule update qemu-system-mips64el compiled successfully. 
Thank you.


Oops I wanted to ask your "git submodule status" output before that, to 
reproduce your bug. Anyway checking out a pre-v1.4.2 is enough.


Peter I guess I found the culprit, compile_prog is not called for dtc 
submodule. I'll send a patch.


Regards,

Phil.

Re: [Qemu-devel] [PATCH v3 4/4] intel_iommu: implement mru list for iotlb

[Michael S. Tsirkin]

On Mon, Jul 17, 2017 at 09:53:27AM +0800, Peter Xu wrote:
> On Fri, Jul 14, 2017 at 03:28:09PM +0800, Jason Wang wrote:
> > 
> > 
> > On 2017年07月14日 12:32, Peter Xu wrote:
> > >On Thu, Jul 13, 2017 at 04:48:42PM +0800, Jason Wang wrote:
> > >>
> > >>On 2017年07月12日 16:13, Peter Xu wrote:
> > >>>It is not wise to disgard all the IOTLB cache when cache size reaches
> > >>>max, but that's what we do now. A slightly better (but still simple) way
> > >>>to do this is, we just throw away the least recent used cache entry.
> > >>>
> > >>>This patch implemented MRU list algorithm for VT-d IOTLB. The main logic
> > >>>is to maintain a Most Recently Used list for the IOTLB entries. The hash
> > >>>table is still used for lookup, though a new list field is added to each
> > >>>IOTLB entry for a iotlb MRU list. For each active IOTLB entry, it's both
> > >>>in the hash table in s->iotlb, and also linked into the MRU list of
> > >>>s->iotlb_head. The hash helps in fast lookup, and the MRU list helps in
> > >>>knowing whether the cache is still hot.
> > >>>
> > >>>After we have such a MRU list, replacing all the iterators of IOTLB
> > >>>entries by using list iterations rather than hash table iterations.
> > >>Any reason of doing this, I thought hashtable was even a little bit 
> > >>faster?
> > >Could I ask why?
> > >
> > >I thought they are merely the same (when iterating all the items)?
> > >
> > 
> > Ok, looks like I was wrong, but it they are merely the same, why bother?
> 
> Because imho looping over list needs fewer LOCs and is also more
> direct. E.g., for domain flush, hash needs this:
> 
> static gboolean vtd_hash_remove_by_domain(gpointer key, gpointer value,
>   gpointer user_data)
> {
> VTDIOTLBEntry *entry = (VTDIOTLBEntry *)value;
> uint16_t domain_id = *(uint16_t *)user_data;
> return entry->domain_id == domain_id;
> }
> 
> Then:
> 
> g_hash_table_foreach_remove(s->iotlb, vtd_hash_remove_by_domain,
> &domain_id);
> 
> For list it is only:
> 
> FOREACH_IOTLB_SAFE(entry, s, entry_n) {
> if (entry->domain_id == domain_id) {
> vtd_iotlb_remove_entry(s, entry);
> }
> }
> 
> Thanks,

Well the LOC seems to have gone up with this patch.
If we are trying to simplify code, please state this
in commit log.


> -- 
> Peter Xu

Re: [Qemu-devel] [Qemu-ppc] [for-2.11 PATCH 00/26] spapr: add support for PHB hotplug

[Daniel Henrique Barboza]

I've tested the patch set using Greg's Github branch. It worked fine in 
my tests

using a Fedora 26 and an Ubuntu 17.04 guests. I have two observations
though:

1 - This is not related to this patch set per se because it is 
reproducible on master, but
I think it is interfering with this new feature.  There is a 
warning/error message in

the kernel right after SLOF that goes:

(...)
 -> smp_release_cpus()
spinning_secondaries = 0
 <- smp_release_cpus()
Linux ppc64le
#1 SMP Mon Jul 1[0.030450] pci :00:02.0: of_irq_parse_pci: 
failed with rc=-22

[0.030552] pci :00:0f.0: of_irq_parse_pci: failed with rc=-22
[  OK  ] Started Security Auditing Service.
(...)

I get this same message after hotplugging a PCI device with this series, 
but the

hotplug shows up in the lspci as expected:

(qemu) device_add virtio-net-pci,id=hp2.0,bus=phb2.0
(qemu) [  412.233441] pci 0002:00:00.0: of_irq_parse_pci: failed with rc=-22


2 - when hotplugging the same PHB for the second time (device_add phb2,
device_del phb2, device_add phb2 again) the hotplug works but dmesg got 
spammed

by the messages:

(qemu) device_add spapr-pci-host-bridge,index=2,id=phb2
(qemu) [  378.309490] rpaphp: rpaphp_register_slot: slot[C131106] is 
already registered
[  378.309674] rpaphp: rpaphp_register_slot: slot[C131074] is already 
registered
[  378.309841] rpaphp: rpaphp_register_slot: slot[C131087] is already 
registered
[  378.310847] rpaphp: rpaphp_register_slot: slot[C131078] is already 
registered

(  about 250+ messages like that )

Looks like device_del isn't cleaning up everything after the first hotplug.



Thanks,


Daniel


On 07/25/2017 02:57 PM, Greg Kurz wrote:

This series is based on patches from Michel Roth posted in 2015:

https://lists.gnu.org/archive/html/qemu-devel/2015-04/msg04246.html

It addresses comments made during the RFC review, and also provides a bunch
of preliminary cleanup/fix patches. Since we have reached hard-freeze, this
feature is provided by a new pseries-2.11 machine type, introduced by this
series. It is based on David Gibson's ppc-for-2.10 branch, and I believe some
of the preliminary fixes are eligible for 2.10.

Note that it also requires an updated SLOF that supports a new private hcall:
KVMPPC_H_UPDATE_PHANDLE. This is needed because SLOF changes FDT phandles to
Open Firmware phandles. Since the guest only sees the latter, QEMU must use
the updated value when populating the FDT for the hotplugged PHB (otherwise
the guest can't setup IRQs for the PCI devices). SLOF part is already upstream:

http://git.qemu.org/?p=SLOF.git;h=604d28cc3f791657414f8b21103921fa0147fc63

With these patches we support the following:

(qemu) device_add spapr-pci-host-bridge,index=2,id=phb2
(qemu) device_add virtio-net-pci,id=hp2.0,bus=phb2.0
(qemu) device_del hp2.0
(qemu) device_del phb2

I could run some successful tests with a fedora25 guest:
- hotplug PHB + migrate + unplug PHB
- hotplug PHB + hotplug PCI device + unplug PHB => PCI device gets unplugged
- migrate before OS starts + hotplug PHB => destination uses OF phandles
- no regression observed with older machine types

All the patches are also available here:

https://github.com/gkurz/qemu/commits/spapr-hotplug-phb

Cheers,

--
Greg

---

Greg Kurz (14):
   spapr: move spapr_create_phb() to core machine code
   spapr_pci: use memory_region_add_subregion() with DMA windows
   spapr_iommu: use g_strdup_printf() instead of snprintf()
   spapr_drc: use g_strdup_printf() instead of snprintf()
   spapr_iommu: convert TCE table object to realize()
   spapr_pci: parent the MSI memory region to the PHB
   spapr_drc: fix realize and unrealize
   spapr_drc: add unrealize method to physical DRC class
   spapr_iommu: unregister vmstate at unrealize time
   spapr: add pseries-2.11 machine type
   spapr_pci: introduce drc_id property
   spapr: allow guest to update the XICS phandle
   spapr_pci: drop abusive sanity check when migrating the LSI table
   spapr: add hotplug hooks for PHB hotplug

Michael Roth (11):
   spapr_drc: pass object ownership to parent/owner
   spapr_iommu: pass object ownership to parent/owner
   pci: allow cleanup/unregistration of PCI buses
   qdev: store DeviceState's canonical path to use when unparenting
   spapr_pci: add PHB unrealize
   spapr: enable PHB hotplug for pseries-2.11
   spapr: create DR connectors for PHBs
   spapr_events: add support for phb hotplug events
   qdev: pass an Object * to qbus_set_hotplug_handler()
   spapr_pci: provide node start offset via spapr_populate_pci_dt()
   spapr_pci: add ibm, my-drc-index property for PHB hotplug

Nathan Fontenot (1):
   spapr: populate PHB DRC entries for root DT node


  hw/acpi/piix4.c   |2
  hw/char/virtio-serial-bus.c   |2
  hw/core/bus.c |   11 --
  hw/core/qdev.c|   15 ++-
  hw/pci/pci.c  |   33 ++

Re: [Qemu-devel] [qemu PATCH for 2.10] i386: acpi: provide an XSDT instead of an RSDT

[Michael S. Tsirkin]

On Wed, Jul 26, 2017 at 04:24:41PM -0400, Paolo Bonzini wrote:
> 
> > The point is that for PC we really should not keep piling up hacks,
> > compatibility is more important.
> 
> Non-explosion of the test matrix is just as important.

Absolutely.

> > > Doing it for PC only would mean switching
> > > back from FADT rev3 to rev1, which is worse for guest OS support,
> > 
> > It's only OSX AFAIK and IIRC OSX doesn't run on PC anyway.
> 
> Sure it does.  You can run it on VMware Workstation which emulates
> some i440FX-like chipset.

I don't think it works on our PC though. Never tried, going by
Gabriel's word though.

> > It's definitely way more code. I'll change my mind if there's
> > a guest that needs the new FADT.
> 
> Then you can prepare a patch to revert the FADTv3 commit, or do the
> work to make it specific to the 2.10 Q35 machine type.
> 
> Paolo

Right, Igor said he's doing that.

-- 
MST

Re: [Qemu-devel] [qemu PATCH for 2.10] i386: acpi: provide an XSDT instead of an RSDT

[Paolo Bonzini]

> The point is that for PC we really should not keep piling up hacks,
> compatibility is more important.

Non-explosion of the test matrix is just as important.

> > Doing it for PC only would mean switching
> > back from FADT rev3 to rev1, which is worse for guest OS support,
> 
> It's only OSX AFAIK and IIRC OSX doesn't run on PC anyway.

Sure it does.  You can run it on VMware Workstation which emulates
some i440FX-like chipset.

> It's definitely way more code. I'll change my mind if there's
> a guest that needs the new FADT.

Then you can prepare a patch to revert the FADTv3 commit, or do the
work to make it specific to the 2.10 Q35 machine type.

Paolo

Re: [Qemu-devel] [seabios PATCH 1/2] seabios: build RSDT from XSDT

[Paolo Bonzini]

- Original Message -
> From: "Michael S. Tsirkin" 
> To: "Paolo Bonzini" 
> Cc: seab...@seabios.org, "Kevin O'Connor" , 
> qemu-devel@nongnu.org, ler...@redhat.com,
> li...@philjordan.eu, imamm...@redhat.com, p...@philjordan.eu, 
> programmingk...@gmail.com, kra...@redhat.com
> Sent: Wednesday, July 26, 2017 10:05:03 PM
> Subject: Re: [seabios PATCH 1/2] seabios: build RSDT from XSDT
> 
> On Wed, Jul 26, 2017 at 11:42:34AM +0200, Paolo Bonzini wrote:
> > Old operating systems would like to have a rev1 (ACPI 1.0) FADT, but
> > new operating systems would like to have rev3 (ACPI 2.0).
> > 
> > Since old operating systems do not know about XSDTs, the
> > solution is to point the RSDT to a rev1 FADT and the XSDT to a
> > rev3 FADT.
> > 
> > But, edk2 is not able to handle QEMU providing two FADTs and barfs when
> > it sees the second; edk2 subscribes to the view that the platform code
> > (meaning not only OVMF, but transitively QEMU's ACPI table builder)
> > should not handle such hacks; it's common edk2 code that should handle
> > FADT rev1 vs. rev3 and RSDT vs. XSDT.
> 
> What exactly does it do wrt RSDT?

It ignores the one produced by QEMU and builds it from scratch.  Same
for RSDP and XSDT.

Paolo

Re: [Qemu-devel] [SeaBIOS] Commit 77af8a2b95b79699de650965d5228772743efe84 breaks Windows 2000 support

[Paolo Bonzini]

> > (4) would be acceptable I guess.  However I think it's a bit worse
> > because fw-cfg files are a somewhat scarce resource.  The "legacy"
> > aspect is something that SeaBIOS is in the best position to address,
> > because it knows what OSes are running on it; QEMU instead only takes
> > care of describing the hardware.
> 
> SeaBIOS is used with both modern and legacy OSes, and it doesn't have
> any knowledge about what kind of OS will be used.  If anything, I'd
> argue that QEMU has more knowledge about the guest OS than SeaBIOS
> does (due to command-line options like machine version).

No, the machine type is independent from the guest OS.  The aim is to
let "qemu-system-x86_64 -m MEMORYSZ image.qcow2" work just fine with
every guest OS.

> As I see it, fundamentally the proposal here is to deploy different
> ACPI tables when using SeaBIOS then when using OVMF.  I think that's
> fine, but I think we should directly address that issue then.

The different ACPI tables are essentially a bug in OVMF.  They can be
fixed to be the same.

> Specifically, I have the following concerns with the original approach:
> 
> A - It would require deploying SeaBIOS and QEMU in lock-step.  To get
> this in for QEMU v2.10 would require making QEMU changes during
> the soft freeze and would require a SeaBIOS "stable" release that
> introduces ACPI table manipulation.

Yes.

> B - I don't have full confidence the proposed ACPI changes wont expose
> a quirk in some obscure OS from the last 25 years.  If it does
> expose a quirk, any work-around would likely require deploying a
> new SeaBIOS and QEMU in lock-step.

Well, the solution is proposed by ACPI itself, and the worst that can
happen is that some OS prefers the RSDT to the XSDT (which we already
test on Windows 2000).

> C - We'd be introducing "shared ownership" of the acpi tables.  Some
> of the tables would be produced by QEMU and some of them by
> SeaBIOS.  Explaining when and why to future developers would be a
> challenge.

The advantage is that the same shared ownership is already present in
OVMF.  The RSDP/RSDT/XSDT are entirely created by the firmware in OVMF.
(The rev1 FADT isn't but that's just missing code; the table manager
in general would be ready for that).  In any case this doesn't seem
like something that cannot be solved by code comments.

Paolo

Re: [Qemu-devel] [PATCH v2] pc: acpi: force FADT rev1 for 440fx based machine types

[Michael S. Tsirkin]

On Wed, Jul 26, 2017 at 04:09:37PM +0200, Igor Mammedov wrote:
> On Tue, 25 Jul 2017 16:36:06 +0300
> "Michael S. Tsirkin"  wrote:
> 
> > On Mon, Jul 24, 2017 at 03:50:20PM +0200, Igor Mammedov wrote:
> > > w2k used to boot on QEMU until revision of FADT has
> > > been bumped to rev3
> > > (commit 77af8a2b hw/i386: Use Rev3 FADT (ACPI 2.0) instead of Rev1 to 
> > > improve guest OS support.)
> > > 
> > > Keep PC machine at rev1 to remain compatible and Q35
> > > at rev3 where w2k isn't supported anyway so OSX could
> > > run as well.
> > > 
> > > Signed-off-by: Igor Mammedov 
> > > ---
> > > v2:
> > >- make pc rev1 and q35 rev3.
> > >"Michael S. Tsirkin" 
> > > 
> > > PS:
> > > Only compile tested.
> > > 
> > > 
> > > CC: Programmingkid 
> > > CC: Phil Dennis-Jordan 
> > > CC: "Michael S. Tsirkin" 
> > > 
> > > ---
> > >  hw/i386/acpi-build.c | 22 ++
> > >  1 file changed, 18 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > > index 6b7bade..b9c245c 100644
> > > --- a/hw/i386/acpi-build.c
> > > +++ b/hw/i386/acpi-build.c
> > > @@ -90,6 +90,7 @@ typedef struct AcpiMcfgInfo {
> > >  } AcpiMcfgInfo;
> > >  
> > >  typedef struct AcpiPmInfo {
> > > +bool force_rev1_fadt;
> > >  bool s3_disabled;
> > >  bool s4_disabled;
> > >  bool pcihp_bridge_en;
> > > @@ -129,10 +130,13 @@ static void acpi_get_pm_info(AcpiPmInfo *pm)
> > >  Object *obj = NULL;
> > >  QObject *o;
> > >  
> > > +pm->force_rev1_fadt = false;
> > >  pm->cpu_hp_io_base = 0;
> > >  pm->pcihp_io_base = 0;
> > >  pm->pcihp_io_len = 0;
> > >  if (piix) {
> > > +/* w2k requires FADT(rev1) or it won't boot, keep PC compatible 
> > > */
> > > +pm->force_rev1_fadt = true;
> > >  obj = piix;
> > >  pm->cpu_hp_io_base = PIIX4_CPU_HOTPLUG_IO_BASE;
> > >  pm->pcihp_io_base =
> > > @@ -304,6 +308,9 @@ static void fadt_setup(AcpiFadtDescriptorRev3 *fadt, 
> > > AcpiPmInfo *pm)
> > >  fadt->flags |= cpu_to_le32(1 << 
> > > ACPI_FADT_F_FORCE_APIC_CLUSTER_MODEL);
> > >  }
> > >  fadt->century = RTC_CENTURY;
> > > +if (pm->force_rev1_fadt) {
> > > +return;
> > > +}
> > >  
> > >  fadt->flags |= cpu_to_le32(1 << ACPI_FADT_F_RESET_REG_SUP);
> > >  fadt->reset_value = 0xf;
> > > @@ -342,6 +349,8 @@ build_fadt(GArray *table_data, BIOSLinker *linker, 
> > > AcpiPmInfo *pm,
> > >  unsigned fw_ctrl_offset = (char *)&fadt->firmware_ctrl - 
> > > table_data->data;
> > >  unsigned dsdt_entry_offset = (char *)&fadt->dsdt - table_data->data;
> > >  unsigned xdsdt_entry_offset = (char *)&fadt->x_dsdt - 
> > > table_data->data;
> > > +int fadt_size = sizeof(*fadt);
> > > +int rev = 3;
> > >  
> > >  /* FACS address to be filled by Guest linker */
> > >  bios_linker_loader_add_pointer(linker,
> > > @@ -353,12 +362,17 @@ build_fadt(GArray *table_data, BIOSLinker *linker, 
> > > AcpiPmInfo *pm,
> > >  bios_linker_loader_add_pointer(linker,
> > >  ACPI_BUILD_TABLE_FILE, dsdt_entry_offset, sizeof(fadt->dsdt),
> > >  ACPI_BUILD_TABLE_FILE, dsdt_tbl_offset);
> > > -bios_linker_loader_add_pointer(linker,
> > > -ACPI_BUILD_TABLE_FILE, xdsdt_entry_offset, sizeof(fadt->x_dsdt),
> > > -ACPI_BUILD_TABLE_FILE, dsdt_tbl_offset);
> > > +if (pm->force_rev1_fadt) {
> > > +rev = 1;
> > > +fadt_size = offsetof(typeof(*fadt), reset_register);  
> > 
> > I don't really like it that we are using structs to format fields - I
> > would prefer build_append_int_noprefix based APIs - but since we do,
> > let's add a proper structure for FADT rev 1 instead of this hack.
> I'll do it.
> 
> > 
> > While we are at it, maybe we should replace macro hacks like
> > ACPI_FADT_COMMON_DEF with simple sub-structures (unless you see a reason
> > not to).
> that would be rewriting every user that touches fields,
> I'd ratter slowly /one table at time/ switch to build_append_int_noprefix() 
> API
> instead of amending legacy approach.

Fine with me.

> > 
> > 
> > > +} else {
> > > +bios_linker_loader_add_pointer(linker,
> > > +ACPI_BUILD_TABLE_FILE, xdsdt_entry_offset, 
> > > sizeof(fadt->x_dsdt),
> > > +ACPI_BUILD_TABLE_FILE, dsdt_tbl_offset);
> > > +}
> > >  
> > >  build_header(linker, table_data,
> > > - (void *)fadt, "FACP", sizeof(*fadt), 3, oem_id, 
> > > oem_table_id);
> > > + (void *)fadt, "FACP", fadt_size, rev, oem_id, 
> > > oem_table_id);
> > >  }
> > >  
> > >  void pc_madt_cpu_entry(AcpiDeviceIf *adev, int uid,
> > > -- 
> > > 2.7.4  
> > 

Re: [Qemu-devel] [qemu PATCH for 2.10] i386: acpi: provide an XSDT instead of an RSDT

[Michael S. Tsirkin]

On Wed, Jul 26, 2017 at 03:01:25PM +0200, Paolo Bonzini wrote:
> On 26/07/2017 14:52, Michael S. Tsirkin wrote:
> > On Wed, Jul 26, 2017 at 11:31:36AM +0200, Paolo Bonzini wrote:
> >> The tables that QEMU provides are not ACPI 1.0 compatible since commit
> >> 77af8a2b95 ("hw/i386: Use Rev3 FADT (ACPI 2.0) instead of Rev1 to improve
> >> guest OS support.", 2017-05-03).  This is visible with Windows 2000,
> >> which refuses to parse the rev3 FADT and fails to boot.
> >>
> >> The recommended solution in this case is to build two FADTs, v1 being
> >> pointed to by the RSDT and v3 by the XSDT.  However, we leave this task
> >> to the firmware.  This patch simply switches the RSDT to the XSDT, which
> >> is valid for all ACPI 2.0-friendly operating systems and also leaves
> >> SeaBIOS the freedom to build an RSDT that points to the compatibility
> >> FADT.
> >>
> >> When running Windows 2000 with an old BIOS, Windows would simply fall
> >> back to a non-ACPI HAL; however, the plan should be to include a BIOS with
> >> the new feature in 2.10.
> >>
> >> Reported-by: Programmingkid 
> >> Signed-off-by: Paolo Bonzini 
> > 
> > I'm not against this but let's do it for q35 only please. PC is a legacy
> > machine type and let's just leave it alone.
> 
> I disagree with calling PC legacy when 99.99% of our users (probably
> underestimated) are using it.

Call it compatibility then :)

The point is that for PC we really should not keep piling up hacks,
compatibility is more important. Let's face it - we have addressed all
their needs for a lot of users a while ago. New features are just churn
and opportunity for bugs for them.

It seems like a rather clean solution to maintain two machines with
more and with less features.

> Doing it for PC only would mean switching
> back from FADT rev3 to rev1, which is worse for guest OS support,

It's only OSX AFAIK and IIRC OSX doesn't run on PC anyway.

> and
> adds yet another little-tested path.

So I think we'll be moving to a cleaner pc/q35 split, sharing
less and less code.

> Together with the corresponding SeaBIOS patch, this provides the best of
> both worlds IMO.
> 
> Paolo

It's definitely way more code. I'll change my mind if there's
a guest that needs the new FADT.

-- 
MST

Re: [Qemu-devel] [seabios PATCH 1/2] seabios: build RSDT from XSDT

[Michael S. Tsirkin]

On Wed, Jul 26, 2017 at 11:42:34AM +0200, Paolo Bonzini wrote:
> Old operating systems would like to have a rev1 (ACPI 1.0) FADT, but
> new operating systems would like to have rev3 (ACPI 2.0).
> 
> Since old operating systems do not know about XSDTs, the
> solution is to point the RSDT to a rev1 FADT and the XSDT to a
> rev3 FADT.
> 
> But, edk2 is not able to handle QEMU providing two FADTs and barfs when
> it sees the second; edk2 subscribes to the view that the platform code
> (meaning not only OVMF, but transitively QEMU's ACPI table builder)
> should not handle such hacks; it's common edk2 code that should handle
> FADT rev1 vs. rev3 and RSDT vs. XSDT.

What exactly does it do wrt RSDT?


> These patches make SeaBIOS follow the same model as edk2, the only
> difference being how the two identify ACPI tables from the BIOS
> linker/loader script.  For SeaBIOS, this task is actually much
> simpler since it can just look into the RSDP: if QEMU only
> provides an XSDT, SeaBIOS takes care of building the RSDT and
> rev1 FADT to satisfy ACPI 1.0-compliant operating systems.
> 
> This part makes SeaBIOS build an RSDT out of an existing XSDT,
> patching the RSDP to point to the RSDT.
> 
> Reviewed-by: Phil Dennis-Jordan 
> Signed-off-by: Paolo Bonzini 
> ---
>  src/fw/paravirt.c | 49 -
>  src/std/acpi.h| 11 +++
>  2 files changed, 59 insertions(+), 1 deletion(-)
> 
> diff --git a/src/fw/paravirt.c b/src/fw/paravirt.c
> index 5b23d78..927fd75 100644
> --- a/src/fw/paravirt.c
> +++ b/src/fw/paravirt.c
> @@ -25,6 +25,7 @@
>  #include "x86.h" // cpuid
>  #include "xen.h" // xen_biostable_setup
>  #include "stacks.h" // yield
> +#include "std/acpi.h"
>  
>  // Amount of continuous ram under 4Gig
>  u32 RamSize;
> @@ -147,6 +148,50 @@ static void msr_feature_control_setup(void)
>  wrmsr_smp(MSR_IA32_FEATURE_CONTROL, feature_control_bits);
>  }
>  
> +static void
> +build_compatibility_rsdt(void)
> +{
> +if (RsdpAddr->rsdt_physical_address)
> +return;
> +
> +u64 xsdt_addr = RsdpAddr->xsdt_physical_address;
> +if (xsdt_addr & ~0xULL)
> +return;
> +
> +struct xsdt_descriptor_rev1 *xsdt = (void*)(u32)xsdt_addr;
> +void *end = (void*)xsdt + xsdt->length;
> +struct rsdt_descriptor_rev1 *rsdt;
> +int rsdt_size = offsetof(struct rsdt_descriptor_rev1, 
> table_offset_entry[0]);
> +int i;
> +for (i=0; (void*)&xsdt->table_offset_entry[i] < end; i++) {
> +u64 tbl_addr = xsdt->table_offset_entry[i];
> +if (!tbl_addr || (tbl_addr & ~0xULL))
> +continue;
> +rsdt_size += 4;
> +}
> +
> +rsdt = malloc_high(rsdt_size);
> +RsdpAddr->rsdt_physical_address = (u32)rsdt;
> +RsdpAddr->checksum -= checksum(RsdpAddr,
> +offsetof(struct rsdp_descriptor, length));
> +RsdpAddr->extended_checksum -= checksum(RsdpAddr,
> + sizeof(struct rsdp_descriptor));
> +
> +memcpy(rsdt, xsdt, sizeof(struct acpi_table_header));
> +rsdt->signature = RSDT_SIGNATURE;
> +rsdt->length = rsdt_size;
> +rsdt->revision = 1;
> +int j;
> +for (i=j=0; (void*)&xsdt->table_offset_entry[i] < end; i++) {
> +u64 tbl_addr = xsdt->table_offset_entry[i];
> +if (!tbl_addr || (tbl_addr & ~0xULL))
> +continue;
> +rsdt->table_offset_entry[j++] = (u32)tbl_addr;
> +}
> +
> +rsdt->checksum -= checksum(rsdt, rsdt_size);
> +}
> +
>  void
>  qemu_platform_setup(void)
>  {
> @@ -186,8 +231,10 @@ qemu_platform_setup(void)
>  
>  RsdpAddr = find_acpi_rsdp();
>  
> -if (RsdpAddr)
> +if (RsdpAddr) {
> +build_compatibility_rsdt();
>  return;
> +}
>  
>  /* If present, loader should have installed an RSDP.
>   * Not installed? We might still be able to continue
> diff --git a/src/std/acpi.h b/src/std/acpi.h
> index c2ea707..a77b53c 100644
> --- a/src/std/acpi.h
> +++ b/src/std/acpi.h
> @@ -133,6 +133,17 @@ struct rsdt_descriptor_rev1
>  } PACKED;
>  
>  /*
> + * ACPI 2.0 Extended System Description Table (XSDT)
> + */
> +#define XSDT_SIGNATURE 0x54445358 // XSDT
> +struct xsdt_descriptor_rev1
> +{
> +ACPI_TABLE_HEADER_DEF   /* ACPI common table header */
> +u64 table_offset_entry[0];  /* Array of pointers to other */
> +/* ACPI tables */
> +} PACKED;
> +
> +/*
>   * ACPI 1.0 Firmware ACPI Control Structure (FACS)
>   */
>  #define FACS_SIGNATURE 0x53434146 // FACS
> -- 
> 2.13.3
> 

Re: [Qemu-devel] [RFC PATCH v2 4/6] hw/pci: introduce bridge-only vendor-specific capability to provide some hints to firmware

[Michael S. Tsirkin]

On Sun, Jul 23, 2017 at 01:15:41AM +0300, Aleksandr Bezzubikov wrote:
> On PCI init PCI bridges may need some
> extra info about bus number to reserve, IO, memory and
> prefetchable memory limits. QEMU can provide this
> with special

with a special

> vendor-specific PCI capability.
> 
> Sizes of limits match ones from
> PCI Type 1 Configuration Space Header,
> number of buses to reserve occupies only 1 byte 
> since it is the size of Subordinate Bus Number register.
> 
> Signed-off-by: Aleksandr Bezzubikov 
> ---
>  hw/pci/pci_bridge.c | 27 +++
>  include/hw/pci/pci_bridge.h | 18 ++
>  2 files changed, 45 insertions(+)
> 
> diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c
> index 720119b..8ec6c2c 100644
> --- a/hw/pci/pci_bridge.c
> +++ b/hw/pci/pci_bridge.c
> @@ -408,6 +408,33 @@ void pci_bridge_map_irq(PCIBridge *br, const char* 
> bus_name,
>  br->bus_name = bus_name;
>  }
>  
> +
> +int pci_bridge_help_cap_init(PCIDevice *dev, int cap_offset,

help? should be qemu_cap_init?

> +  uint8_t bus_reserve, uint32_t io_limit,
> +  uint16_t mem_limit, uint64_t pref_limit,
> +  Error **errp)
> +{
> +size_t cap_len = sizeof(PCIBridgeQemuCap);
> +PCIBridgeQemuCap cap;

This leaks info to guest. You want to init all fields here:

cap = {
 .len = 
};

> +
> +cap.len = cap_len;
> +cap.bus_res = bus_reserve;
> +cap.io_lim = io_limit & 0xFF;
> +cap.io_lim_upper = io_limit >> 8 & 0x;
> +cap.mem_lim = mem_limit;
> +cap.pref_lim = pref_limit & 0x;
> +cap.pref_lim_upper = pref_limit >> 16 & 0x;

Please use pci_set_word etc or cpu_to_leXX.

I think it's easiest to replace struct with a set of macros then
pci_set_word does the work for you.


> +
> +int offset = pci_add_capability(dev, PCI_CAP_ID_VNDR,
> +cap_offset, cap_len, errp);
> +if (offset < 0) {
> +return offset;
> +}
> +
> +memcpy(dev->config + offset + 2, (char *)&cap + 2, cap_len - 2);

+2 is yacky. See how virtio does it:

memcpy(dev->config + offset + PCI_CAP_FLAGS, &cap->cap_len,
   cap->cap_len - PCI_CAP_FLAGS);


> +return 0;
> +}
> +
>  static const TypeInfo pci_bridge_type_info = {
>  .name = TYPE_PCI_BRIDGE,
>  .parent = TYPE_PCI_DEVICE,
> diff --git a/include/hw/pci/pci_bridge.h b/include/hw/pci/pci_bridge.h
> index ff7cbaa..c9f642c 100644
> --- a/include/hw/pci/pci_bridge.h
> +++ b/include/hw/pci/pci_bridge.h
> @@ -67,4 +67,22 @@ void pci_bridge_map_irq(PCIBridge *br, const char* 
> bus_name,
>  #define  PCI_BRIDGE_CTL_DISCARD_STATUS   0x400   /* Discard timer status 
> */
>  #define  PCI_BRIDGE_CTL_DISCARD_SERR 0x800   /* Discard timer SERR# enable */
>  
> +typedef struct PCIBridgeQemuCap {
> +uint8_t id; /* Standard PCI capability header field */
> +uint8_t next;   /* Standard PCI capability header field */
> +uint8_t len;/* Standard PCI vendor-specific capability header field 
> */
> +uint8_t bus_res;
> +uint32_t pref_lim_upper;

Big endian? Ugh.

> +uint16_t pref_lim;
> +uint16_t mem_lim;

I'd say we need 64 bit for memory.

> +uint16_t io_lim_upper;
> +uint8_t io_lim;
> +uint8_t padding;

IMHO each type should have a special "don't care" flag
that would mean "I don't know".


> +} PCIBridgeQemuCap;

You don't really need this struct in the header. And pls document all fields.

> +
> +int pci_bridge_help_cap_init(PCIDevice *dev, int cap_offset,
> +  uint8_t bus_reserve, uint32_t io_limit,
> +  uint16_t mem_limit, uint64_t pref_limit,
> +  Error **errp);
> +
>  #endif /* QEMU_PCI_BRIDGE_H */
> -- 
> 2.7.4