[Qemu-devel] [RFC 04/15] monitor: move skip_flush into monitor_data_init

[Peter Xu]

It's part of the data init.  Collect it.

Reviewed-by: Dr. David Alan Gilbert 
Signed-off-by: Peter Xu 
---
 monitor.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/monitor.c b/monitor.c
index 9239f7a..8b32519 100644
--- a/monitor.c
+++ b/monitor.c
@@ -568,13 +568,14 @@ static void monitor_qapi_event_init(void)
 
 static void handle_hmp_command(Monitor *mon, const char *cmdline);
 
-static void monitor_data_init(Monitor *mon)
+static void monitor_data_init(Monitor *mon, bool skip_flush)
 {
 memset(mon, 0, sizeof(Monitor));
 qemu_mutex_init(>out_lock);
 mon->outbuf = qstring_new();
 /* Use *mon_cmds by default. */
 mon->cmd_table = mon_cmds;
+mon->skip_flush = skip_flush;
 }
 
 static void monitor_data_destroy(Monitor *mon)
@@ -594,8 +595,7 @@ char *qmp_human_monitor_command(const char *command_line, 
bool has_cpu_index,
 char *output = NULL;
 Monitor *old_mon, hmp;
 
-monitor_data_init();
-hmp.skip_flush = true;
+monitor_data_init(, true);
 
 old_mon = cur_mon;
 cur_mon = 
@@ -4098,7 +4098,7 @@ void monitor_init(Chardev *chr, int flags)
 }
 
 mon = g_malloc(sizeof(*mon));
-monitor_data_init(mon);
+monitor_data_init(mon, false);
 
 qemu_chr_fe_init(>chr, chr, _abort);
 mon->flags = flags;
-- 
2.7.4

[Qemu-devel] [RFC 06/15] monitor: move the cur_mon hack deeper for QMP

[Peter Xu]

In monitor_qmp_read(), we have the hack to temporarily replace the
cur_mon pointer.  Now we move this hack deeper inside the QMP dispatcher
routine since the Monitor pointer can be passed in to that using the new
JSON Parser opaque field now.

This does not make much sense as a single patch.  However, this will be
a big step for the next patch, when the QMP dispatcher routine will be
splitted from the QMP parser.

Signed-off-by: Peter Xu 
---
 monitor.c | 19 ++-
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/monitor.c b/monitor.c
index 9096b64..d7eb3c2 100644
--- a/monitor.c
+++ b/monitor.c
@@ -3822,7 +3822,7 @@ static void handle_qmp_command(JSONMessageParser *parser, 
GQueue *tokens,
 {
 QObject *req, *rsp = NULL, *id = NULL;
 QDict *qdict = NULL;
-Monitor *mon = cur_mon;
+Monitor *mon = opaque, *old_mon;
 Error *err = NULL;
 
 req = json_parser_parse_err(tokens, NULL, );
@@ -3847,8 +3847,13 @@ static void handle_qmp_command(JSONMessageParser 
*parser, GQueue *tokens,
 QDECREF(req_json);
 }
 
+old_mon = cur_mon;
+cur_mon = mon;
+
 rsp = qmp_dispatch(cur_mon->qmp.commands, req);
 
+cur_mon = old_mon;
+
 if (mon->qmp.commands == _cap_negotiation_commands) {
 qdict = qdict_get_qdict(qobject_to_qdict(rsp), "error");
 if (qdict
@@ -3885,13 +3890,9 @@ err_out:
 
 static void monitor_qmp_read(void *opaque, const uint8_t *buf, int size)
 {
-Monitor *old_mon = cur_mon;
-
-cur_mon = opaque;
-
-json_message_parser_feed(_mon->qmp.parser, (const char *) buf, size);
+Monitor *mon = opaque;
 
-cur_mon = old_mon;
+json_message_parser_feed(>qmp.parser, (const char *) buf, size);
 }
 
 static void monitor_read(void *opaque, const uint8_t *buf, int size)
@@ -3965,7 +3966,7 @@ static void monitor_qmp_event(void *opaque, int event)
 break;
 case CHR_EVENT_CLOSED:
 json_message_parser_destroy(>qmp.parser);
-json_message_parser_init(>qmp.parser, handle_qmp_command, NULL);
+json_message_parser_init(>qmp.parser, handle_qmp_command, mon);
 mon_refcount--;
 monitor_fdsets_cleanup();
 break;
@@ -4115,7 +4116,7 @@ void monitor_init(Chardev *chr, int flags)
 qemu_chr_fe_set_handlers(>chr, monitor_can_read, monitor_qmp_read,
  monitor_qmp_event, NULL, mon, NULL, true);
 qemu_chr_fe_set_echo(>chr, true);
-json_message_parser_init(>qmp.parser, handle_qmp_command, NULL);
+json_message_parser_init(>qmp.parser, handle_qmp_command, mon);
 } else {
 qemu_chr_fe_set_handlers(>chr, monitor_can_read, monitor_read,
  monitor_event, NULL, mon, NULL, true);
-- 
2.7.4

[Qemu-devel] [RFC 02/15] qobject: allow NULL for qstring_get_str()

[Peter Xu]

Then I can get NULL rather than crash when calling things like:

  qstring_get_str(qobject_to_qstring(object));

when key does not exist.

CC: Markus Armbruster 
Signed-off-by: Peter Xu 
---
 qobject/qstring.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qobject/qstring.c b/qobject/qstring.c
index 5da7b5f..c499fec 100644
--- a/qobject/qstring.c
+++ b/qobject/qstring.c
@@ -125,7 +125,7 @@ QString *qobject_to_qstring(const QObject *obj)
  */
 const char *qstring_get_str(const QString *qstring)
 {
-return qstring->string;
+return qstring ? qstring->string : NULL;
 }
 
 /**
-- 
2.7.4

[Qemu-devel] [RFC 00/15] QMP: out-of-band (OOB) execution support

[Peter Xu]

This series was born from this one:

  https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg04310.html

The design comes from Markus, and also the whole-bunch-of discussions
in previous thread.  My heartful thanks to Markus, Daniel, Dave,
Stefan, etc. on discussing the topic (...again!), providing shiny
ideas and suggestions.  Finally we got such a solution that seems to
satisfy everyone.

I re-started the versioning since this series is totally different
from previous one.  Now it's version 1.

In case new reviewers come along the way without reading previous
discussions, I will try to do a summary on what this is all about.

What is OOB execution?
==

It's the shortcut of Out-Of-Band execution, its name is given by
Markus.  It's a way to quickly execute a QMP request.  Say, originally
QMP is going throw these steps:

  JSON Parser --> QMP Dispatcher --> Respond
  /|\(2)(3) |
   (1) |   \|/ (4)
   +-  main thread  +

The requests are executed by the so-called QMP-dispatcher after the
JSON is parsed.  If OOB is on, we run the command directly in the
parser and quickly returns.

Yeah I know in current code the parser calls dispatcher directly
(please see handle_qmp_command()).  However it's not true again after
this series (parser will has its own IO thread, and dispatcher will
still be run in main thread).  So this OOB does brings something
different.

There are more details on why OOB and the difference/relationship
between OOB, async QMP, block/general jobs, etc.. but IMHO that's
slightly out of topic (and believe me, it's not easy for me to
summarize that).  For more information, please refers to [1].

Summary ends here.

Some Implementation Details
===

Again, I mentioned that the old QMP workflow is this:

  JSON Parser --> QMP Dispatcher --> Respond
  /|\(2)(3) |
   (1) |   \|/ (4)
   +-  main thread  +

What this series does is, firstly:

  JSON Parser QMP Dispatcher --> Respond
  /|\ |   /|\   (4) |
   |  | (2)| (3)|  (5)
   (1) |  +->  |   \|/
   +-  main thread  <---+

And further:

   queue/kick
 JSON Parser ==> QMP Dispatcher --> Respond
 /|\ | (3)   /|\(4)|
  (1) |  | (2)||  (5)
  | \|/   |   \|/
IO thread main thread  <---+

Then it introduced the "allow-oob" parameter in QAPI schema to define
commands, and "run-oob" flag to let oob-allowed command to run in the
parser.

The last patch enables this for "migrate-incoming" command.

Please review.  Thanks.

[1] https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg04310.html

Peter Xu (15):
  char-io: fix possible race on IOWatchPoll
  qobject: allow NULL for qstring_get_str()
  qobject: introduce qobject_to_str()
  monitor: move skip_flush into monitor_data_init
  qjson: add "opaque" field to JSONMessageParser
  monitor: move the cur_mon hack deeper for QMP
  monitor: unify global init
  monitor: create IO thread
  monitor: allow to use IO thread for parsing
  monitor: introduce monitor_qmp_respond()
  monitor: separate QMP parser and dispatcher
  monitor: enable IO thread for (qmp & !mux) typed
  qapi: introduce new cmd option "allow-oob"
  qmp: support out-of-band (oob) execution
  qmp: let migrate-incoming allow out-of-band

 chardev/char-io.c|  15 ++-
 docs/devel/qapi-code-gen.txt |  51 ++-
 include/monitor/monitor.h|   2 +-
 include/qapi/qmp/dispatch.h  |   2 +
 include/qapi/qmp/json-streamer.h |   8 +-
 include/qapi/qmp/qstring.h   |   1 +
 monitor.c| 283 +++
 qapi/introspect.json |   6 +-
 qapi/migration.json  |   3 +-
 qapi/qmp-dispatch.c  |  34 +
 qga/main.c   |   5 +-
 qobject/json-streamer.c  |   7 +-
 qobject/qjson.c  |   5 +-
 qobject/qstring.c|  13 +-
 scripts/qapi-commands.py |  19 ++-
 scripts/qapi-introspect.py   |  10 +-
 scripts/qapi.py  |  15 ++-
 scripts/qapi2texi.py |   2 +-
 tests/libqtest.c |   5 +-
 tests/qapi-schema/test-qapi.py   |   2 +-
 trace-events |   2 +
 vl.c |   3 +-
 22 files changed, 398 insertions(+), 95 deletions(-)

-- 
2.7.4

[Qemu-devel] [RFC 01/15] char-io: fix possible race on IOWatchPoll

[Peter Xu]

This is not a problem if we are only having one single loop thread like
before.  However, after per-monitor thread is introduced, this is not
true any more, and the race can happen.

The race can be triggered with "make check -j8" sometimes:

  qemu-system-x86_64: /root/git/qemu/chardev/char-io.c:91:
  io_watch_poll_finalize: Assertion `iwp->src == NULL' failed.

This patch keeps the reference for the watch object when creating in
io_add_watch_poll(), so that the object will never be released in the
context main loop, especially when the context loop is running in
another standalone thread.  Meanwhile, when we want to remove the watch
object, we always first detach the watch object from its owner context,
then we continue with the cleanup.

Without this patch, calling io_remove_watch_poll() in main loop thread
is not thread-safe, since the other per-monitor thread may be modifying
the watch object at the same time.

Reviewed-by: Marc-André Lureau 
Signed-off-by: Peter Xu 
---
 chardev/char-io.c | 15 +--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/chardev/char-io.c b/chardev/char-io.c
index f810524..3828c20 100644
--- a/chardev/char-io.c
+++ b/chardev/char-io.c
@@ -122,7 +122,6 @@ GSource *io_add_watch_poll(Chardev *chr,
 g_free(name);
 
 g_source_attach(>parent, context);
-g_source_unref(>parent);
 return (GSource *)iwp;
 }
 
@@ -131,12 +130,24 @@ static void io_remove_watch_poll(GSource *source)
 IOWatchPoll *iwp;
 
 iwp = io_watch_poll_from_source(source);
+
+/*
+ * Here the order of destruction really matters.  We need to first
+ * detach the IOWatchPoll object from the context (which may still
+ * be running in another loop thread), only after that could we
+ * continue to operate on iwp->src, or there may be race condition
+ * between current thread and the context loop thread.
+ *
+ * Let's blame the glib bug mentioned in commit 2b3167 (again) for
+ * this extra complexity.
+ */
+g_source_destroy(>parent);
 if (iwp->src) {
 g_source_destroy(iwp->src);
 g_source_unref(iwp->src);
 iwp->src = NULL;
 }
-g_source_destroy(>parent);
+g_source_unref(>parent);
 }
 
 void remove_fd_in_watch(Chardev *chr)
-- 
2.7.4

Re: [Qemu-devel] [PATCH] filter-mirror: segfault when specifying non existent device

[Michael Tokarev]

21.08.2017 18:50, Eduardo Otubo wrote:
> When using filter-mirror like the example below where the interface
> 'ndev0' does not exist on the host, QEMU crashes into segmentation
> fault.

Applied to -trivial, thanks!

/mjt

Re: [Qemu-devel] [PATCH v2 5/5] arm: drop intermediate cpu_model -> cpu type parsing and use cpu type directly

[Igor Mammedov]

On Thu, 14 Sep 2017 00:47:20 -0300
Philippe Mathieu-Daudé  wrote:

> Hi Igor,
> 
> awesome clean refactor!
Thanks,

there is more patches on this topic for other targets to post
but it's waiting on 1-3/5 to land in master so it would be
easier for maintainers to verify/test them without fishing out
dependencies from mail list.

hopefully everything will land in 2.11 so we won't have to deal
with cpu_model anywhere except of one place vl.c.

> just 1 comment inlined.
> 
> On 09/13/2017 01:04 PM, Igor Mammedov wrote:
> > there are 2 use cases to deal with:
> >1: fixed CPU models per board/soc
> >2: boards with user configurable cpu_model and fallback to
> >   default cpu_model if user hasn't specified one explicitly
> > 
> > For the 1st
> >drop intermediate cpu_model parsing and use const cpu type
> >directly, which replaces:
> >   typename = object_class_get_name(
> > cpu_class_by_name(TYPE_ARM_CPU, cpu_model))
> >   object_new(typename)
> >with
> >   object_new(FOO_CPU_TYPE_NAME)
> >or
> >   cpu_generic_init(BASE_CPU_TYPE, "my cpu model")
> >with
> >   cpu_create(FOO_CPU_TYPE_NAME)
> > 
> > as result 1st use case doesn't have to invoke not necessary
> > translation and not needed code is removed.
> > 
> > For the 2nd
> >   1: set default cpu type with MachineClass::default_cpu_type and
> >   2: use generic cpu_model parsing that done before machine_init()
> >  is run and:
> >  2.1: drop custom cpu_model parsing where pattern is:
> > typename = object_class_get_name(
> > cpu_class_by_name(TYPE_ARM_CPU, cpu_model))
> > [parse_features(typename, cpu_model, ) ]
> > 
> >  2.2: or replace cpu_generic_init() which does what
> >   2.1 does + create_cpu(typename) with just
> >   create_cpu(machine->cpu_type)
> > as result cpu_name -> cpu_type translation is done using
> > generic machine code one including parsing optional features
> > if supported/present (removes a bunch of duplicated cpu_model
> > parsing code) and default cpu type is defined in an uniform way
> > within machine_class_init callbacks instead of adhoc places
> > in boadr's machine_init code.
> > 
> > Signed-off-by: Igor Mammedov 
> > Reviewed-by: Eduardo Habkost 
> > ---
> > v2:
> >   - fix merge conflicts with ignore_memory_transaction_failures
> >   - fix couple merge conflicts where SoC type string where replaced by type 
> > macro
> >   - keep plain prefix string in: strncmp(cpu_type, "pxa27", 5)
> >   - s/"%s" ARM_CPU_TYPE_SUFFIX/ARM_CPU_TYPE_NAME("%s")/
> > ---
[...]

> > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > index fe96557..fe26e99 100644
> > --- a/hw/arm/virt.c
> > +++ b/hw/arm/virt.c
> > @@ -163,13 +163,13 @@ static const int a15irqmap[] = {
> >   };
> >   
> >   static const char *valid_cpus[] = {
> > -"cortex-a15",
> > -"cortex-a53",
> > -"cortex-a57",
> > -"host",
> > +ARM_CPU_TYPE_NAME("cortex-a15"),
> > +ARM_CPU_TYPE_NAME("cortex-a53"),
> > +ARM_CPU_TYPE_NAME("cortex-a57"),
> > +ARM_CPU_TYPE_NAME("host"),
> >   };
> >   
> > -static bool cpuname_valid(const char *cpu)
> > +static bool cpu_type_valid(const char *cpu)
> >   {
> >   int i;  
> 
> I'd just change this by:
> 
> static bool cpuname_valid(const char *cpu)
> {
>  static const char *valid_cpus[] = {
>  ARM_CPU_TYPE_NAME("cortex-a15"),
>  ARM_CPU_TYPE_NAME("cortex-a53"),
>  ARM_CPU_TYPE_NAME("cortex-a57"),
>  };
>  int i;
> 
>  for (i = 0; i < ARRAY_SIZE(valid_cpus); i++) {
>  if (strcmp(cpu, valid_cpus[i]) == 0) {
>  return true;
>  }
>  }

>  return kvm_enabled() && !strcmp(cpu, ARM_CPU_TYPE_NAME("host");
here is one more case to consider for valid_cpus refactoring,
CCing Alistair.

> }
> 
> Anyway this can be a later patch.
this check might be removed or superseded by generic valid_cpus work
that Alistair is working on, anyways it should be part of that work
as change is not directly related to this series.


[...]

Re: [Qemu-devel] [Qemu-trivial] [PATCH v3 00/15] add missing entries in MAINTAINERS

[Michael Tokarev]

08.09.2017 20:31, Philippe Mathieu-Daudé wrote:
> Hi,
> 
> I tried to have a more helpful ./scripts/get_maintainer.pl output, filling
> missing entries in MAINTAINERS.


Applied all to -trivial, thank you!

/mjt

Re: [Qemu-devel] [Qemu-trivial] [PATCH for-2.11] tests: Fix broken ivshmem-server-msi/-irq tests

[Michael Tokarev]

29.08.2017 21:13, Thomas Huth wrote:
> Broken with commit b4ba67d9a7025 ("libqos: Change PCI accessors to take
> opaque BAR handle") a while ago, but nobody noticed since the tests are
> only run in SPEED=slow mode: The msix_pba_bar is not correctly initialized
> anymore if bir_pba has the same value as bir_table. With this fix,
> "make check SPEED=slow" should work fine again.

Applied to -trivial, thanks!

/mjt

Re: [Qemu-devel] [PATCH for-2.11] hw/misc/ivshmem: Fix ivshmem_recv_msg() to also work on big endian systems

[Michael Tokarev]

30.08.2017 16:39, Thomas Huth пишет:
> The "slow" ivshmem-tests currently fail when they are running on a
> big endian host:

Applied to -trivial, thanks!

/mjt

Re: [Qemu-devel] [Qemu-trivial] [PATCH v2] Replace round_page() with TARGET_PAGE_ALIGN()

[Michael Tokarev]

11.09.2017 23:16, Kamil Rytarowski пишет:
> This change fixes conflict with the DragonFly BSD headers.

Applied to -trivial, thanks!

/mjt

Re: [Qemu-devel] [PATCH for-2.9?] configure: Remove unused code (found by shellcheck)

[Michael Tokarev]

16.08.2017 15:57, Stefan Weil пишет:
> It looks like this patch got lost somehow.
> 
> Stefan
> 
> See also https://patchwork.codeaurora.org/patch/210129/
> 
> 
> Am 28.03.2017 um 20:49 schrieb Stefan Weil:
>> smartcard_cflags is no longer needed since commit
>> 0b22ef0f57a8910d849602bef0940edcd0553d2c.

Applied to -trivial.. finally :)

Thank you!

/mjt

Re: [Qemu-devel] [PATCH] spapr_pci: make index property mandatory

[no-reply]

Hi,

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

Subject: [Qemu-devel] [PATCH] spapr_pci: make index property mandatory
Message-id: 150537259490.3298.1180094221641142666.stgit@bahia
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]   
patchew/150537259490.3298.1180094221641142666.stgit@bahia -> 
patchew/150537259490.3298.1180094221641142666.stgit@bahia
 t [tag update]patchew/20170913142036.2469-1-lviv...@redhat.com -> 
patchew/20170913142036.2469-1-lviv...@redhat.com
Switched to a new branch 'test'
705ee2bccc spapr_pci: make index property mandatory

=== OUTPUT BEGIN ===
Checking PATCH 1/1: spapr_pci: make index property mandatory...
ERROR: spaces required around that '-' (ctx:VxV)
#126: FILE: hw/ppc/spapr_pci.c:1920:
+sphb->mem64_win_addr = (hwaddr)-1;
^

total: 1 errors, 0 warnings, 92 lines checked

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

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-de...@freelists.org

[Qemu-devel] [PATCH] spapr_pci: make index property mandatory

[Greg Kurz]

Creating several PHBs without index property confuses the DRC code
and causes issues:
- only the first index-less PHB is functional, the other ones will
  silently ignore hotplugging of PCI devices
- QEMU will even terminate if these PHBs have cold-plugged devices

qemu-system-ppc64: -device virtio-net,bus=pci2.0: an attached device
 is still awaiting release

This happens because DR connectors for child PCI devices are created
with a DRC index that is derived from the PHB's index property. If the
PHBs are created without index, then the same value of -1 is used to
compute the DRC indexes for both PHBs, hence causing the collision.

Also, the index property is used to compute the placement of the PHB's
memory regions. It is limited to 31 or 255, depending on the machine
type version. This fits well with the requirements of DRC indexes, which
need the PHB index to be a 16-bit value.

This patch hence makes the index property mandatory. As a consequence,
the PHB's memory regions and BUID are now always configured according
to the index, and it is no longer possible to set them from the command
line. We have to introduce a PHB instance init function to initialize
the 64-bit window address to -1 because pseries-2.7 and older machines
don't set it.

This DOES BREAK backwards compat, but we don't think the non-index
PHB feature was used in practice (at least libvirt doesn't) and the
simplification is worth it.

Signed-off-by: Greg Kurz 
---
RFC->v1: - as suggested dy David, updated the changelog to explicitely
   mention that we intentionally break backwards compat.
---
 hw/ppc/spapr_pci.c |   52 ++--
 1 file changed, 10 insertions(+), 42 deletions(-)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index cf54160526fa..9a338b7f197b 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1523,16 +1523,6 @@ static void spapr_phb_realize(DeviceState *dev, Error 
**errp)
 sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
 Error *local_err = NULL;
 
-if ((sphb->buid != (uint64_t)-1) || (sphb->dma_liobn[0] != 
(uint32_t)-1)
-|| (sphb->dma_liobn[1] != (uint32_t)-1 && windows_supported == 2)
-|| (sphb->mem_win_addr != (hwaddr)-1)
-|| (sphb->mem64_win_addr != (hwaddr)-1)
-|| (sphb->io_win_addr != (hwaddr)-1)) {
-error_setg(errp, "Either \"index\" or other parameters must"
-   " be specified for PAPR PHB, not both");
-return;
-}
-
 smc->phb_placement(spapr, sphb->index,
>buid, >io_win_addr,
>mem_win_addr, >mem64_win_addr,
@@ -1541,36 +1531,12 @@ static void spapr_phb_realize(DeviceState *dev, Error 
**errp)
 error_propagate(errp, local_err);
 return;
 }
-}
-
-if (sphb->buid == (uint64_t)-1) {
-error_setg(errp, "BUID not specified for PHB");
-return;
-}
-
-if ((sphb->dma_liobn[0] == (uint32_t)-1) ||
-((sphb->dma_liobn[1] == (uint32_t)-1) && (windows_supported > 1))) {
-error_setg(errp, "LIOBN(s) not specified for PHB");
-return;
-}
-
-if (sphb->mem_win_addr == (hwaddr)-1) {
-error_setg(errp, "Memory window address not specified for PHB");
-return;
-}
-
-if (sphb->io_win_addr == (hwaddr)-1) {
-error_setg(errp, "IO window address not specified for PHB");
+} else {
+error_setg(errp, "\"index\" for PAPR PHB is mandatory");
 return;
 }
 
 if (sphb->mem64_win_size != 0) {
-if (sphb->mem64_win_addr == (hwaddr)-1) {
-error_setg(errp,
-   "64-bit memory window address not specified for PHB");
-return;
-}
-
 if (sphb->mem_win_size > SPAPR_PCI_MEM32_WIN_SIZE) {
 error_setg(errp, "32-bit memory window of size 0x%"HWADDR_PRIx
" (max 2 GiB)", sphb->mem_win_size);
@@ -1789,18 +1755,12 @@ static void spapr_phb_reset(DeviceState *qdev)
 
 static Property spapr_phb_properties[] = {
 DEFINE_PROP_UINT32("index", sPAPRPHBState, index, -1),
-DEFINE_PROP_UINT64("buid", sPAPRPHBState, buid, -1),
-DEFINE_PROP_UINT32("liobn", sPAPRPHBState, dma_liobn[0], -1),
-DEFINE_PROP_UINT32("liobn64", sPAPRPHBState, dma_liobn[1], -1),
-DEFINE_PROP_UINT64("mem_win_addr", sPAPRPHBState, mem_win_addr, -1),
 DEFINE_PROP_UINT64("mem_win_size", sPAPRPHBState, mem_win_size,
SPAPR_PCI_MEM32_WIN_SIZE),
-DEFINE_PROP_UINT64("mem64_win_addr", sPAPRPHBState, mem64_win_addr, -1),
 DEFINE_PROP_UINT64("mem64_win_size", sPAPRPHBState, mem64_win_size,
SPAPR_PCI_MEM64_WIN_SIZE),
 DEFINE_PROP_UINT64("mem64_win_pciaddr", sPAPRPHBState, mem64_win_pciaddr,
-1),
-DEFINE_PROP_UINT64("io_win_addr", sPAPRPHBState, io_win_addr, -1),
 

[Qemu-devel] QEMU terminates during reboot after memory unplug with vhost=on

[Bharata B Rao]

Hi,

QEMU hits the below assert

qemu-system-ppc64: used ring relocated for ring 2
qemu-system-ppc64: qemu/hw/virtio/vhost.c:649: vhost_commit: Assertion `r >= 0' 
failed.

in the following scenario:

1. Boot guest with vhost=on
  -netdev tap,id=mynet0,script=qemu-ifup,downscript=qemu-ifdown,vhost=on 
-device virtio-net-pci,netdev=mynet0
2. Hot add a DIMM device 
3. Reboot
   When the guest reboots, we can see
   vhost_virtqueue_start:vq->used_phys getting assigned an address that
   falls in the hotplugged memory range.
4. Remove the DIMM device
   Guest refuses the removal as the hotplugged memory is under use.
5. Reboot
   QEMU forces the removal of the DIMM device during reset and that's
   when we hit the above assert.

Any pointers on why we are hitting this assert ? Shouldn't vhost be
done with using the hotplugged memory when we hit reset ?

Regards,
Bharata.

Re: [Qemu-devel] [PATCH v4 2/4] hmp: fix "dump-quest-memory" segfault (arm)

[Auger Eric]

Hi Laurent,

On 13/09/2017 16:20, Laurent Vivier wrote:
> Running QEMU with
> qemu-system-aarch64 -M none -nographic -m 256
> and executing
> dump-guest-memory /dev/null 0 8192
> results in segfault
> 
> Fix by checking if we have CPU, and exit with
> error if there is no CPU:
> 
> (qemu) dump-guest-memory /dev/null
> this feature or command is not currently supported
> 
> Signed-off-by: Laurent Vivier 
> Reviewed-by: Thomas Huth 
> Reviewed-by: Greg Kurz 
Reviewed-by: Eric Auger 
Tested-by: Eric Auger 

Thanks

Eric

> ---
>  target/arm/arch_dump.c | 11 +--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/target/arm/arch_dump.c b/target/arm/arch_dump.c
> index 1a9861f69b..9e5b2fb31c 100644
> --- a/target/arm/arch_dump.c
> +++ b/target/arm/arch_dump.c
> @@ -273,11 +273,18 @@ int arm_cpu_write_elf32_note(WriteCoreDumpFunction f, 
> CPUState *cs,
>  int cpu_get_dump_info(ArchDumpInfo *info,
>const GuestPhysBlockList *guest_phys_blocks)
>  {
> -ARMCPU *cpu = ARM_CPU(first_cpu);
> -CPUARMState *env = >env;
> +ARMCPU *cpu;
> +CPUARMState *env;
>  GuestPhysBlock *block;
>  hwaddr lowest_addr = ULLONG_MAX;
>  
> +if (first_cpu == NULL) {
> +return -1;
> +}
> +
> +cpu = ARM_CPU(first_cpu);
> +env = >env;
> +
>  /* Take a best guess at the phys_base. If we get it wrong then crash
>   * will need '--machdep phys_offset=' added to its command
>   * line, which isn't any worse than assuming we can use zero, but being
> 

Re: [Qemu-devel] [PATCH v3] spapr: disable decrementer during reset

[Nikunj A Dadhania]

David Gibson  writes:

> On Wed, Jul 19, 2017 at 09:20:52AM +0530, Nikunj A Dadhania wrote:
>> David Gibson  writes:
>> 
>> > On Tue, Jul 18, 2017 at 10:53:01AM +0530, Nikunj A Dadhania wrote:
>> >> David Gibson  writes:
>> >> 
>> >> > On Mon, Jul 17, 2017 at 09:46:39AM +0530, Nikunj A Dadhania wrote:
>> >> >> Rebooting a SMP TCG guest is broken for both single/multi threaded TCG.
>> >> >> 
>> >> >> When reset happens, all the CPUs are in halted state. First CPU is 
>> >> >> brought out
>> >> >> of reset and secondary CPUs would be initialized by the guest kernel 
>> >> >> using a
>> >> >> rtas call start-cpu.
>> >> >> 
>> >> >> However, in case of TCG, decrementer interrupts keep on coming and 
>> >> >> waking the
>> >> >> secondary CPUs up.
>> >> >> 
>> >> >> These secondary CPUs would see the decrementer interrupt pending, 
>> >> >> which makes
>> >> >> cpu::has_work() to bring them out of wait loop and start executing
>> >> >> tcg_exec_cpu().
>> >> >> 
>> >> >> The problem with this is all the CPUs wake up and start booting SLOF 
>> >> >> image,
>> >> >> causing the following exception(4 CPUs TCG VM):
>> >> >
>> >> > Ok, I'm still trying to understand why the behaviour on reboot is
>> >> > different from the first boot.
>> >> 
>> >> During first boot, the cpu is in the stopped state, so
>> >> cpus.c:cpu_thread_is_idle returns true and CPU remains in halted state
>> >> until rtas start-cpu. Therefore, we never check the cpu_has_work()
>> >> 
>> >> In case of reboot, all CPUs are resumed after reboot. So we check the
>> >> next condition cpu_has_work() in cpu_thread_is_idle(), where we see a
>> >> DECR interrupt and remove the CPU from halted state as the CPU has
>> >> work.
>> >
>> > Ok, so it sounds like we should set stopped on all the secondary CPUs
>> > on reset as well.  What's causing them to be resumed after the reset
>> > at the moment?
>> 
>> That is part of the main loop in vl.c, when reset is requested. All the
>> vcpus are paused (stopped == true) then system reset is issued, and all
>> cpus are resumed (stopped == false). Which is correct.
>
> is it?  Seems we have a different value of 'stopped' on the first boot
> compared to reoboots, which doesn't seem right.

I have checked this with more debugging prints (patch at the end), as
you said, first boot and reboot does not have different value of
cpu::stopped

 cpu_ppc_decr_excp-cpu1: stop 0 stopped 1 halted 0 SPR_LPCR 0
 spapr_cpu_reset-cpu1: stop 0 stopped 1 halted 1 SPR_LPCR 2000
 spapr_cpu_reset-cpu1: stop 0 stopped 1 halted 1 SPR_LPCR 2000
 resume_all_vcpus-cpu0: stop 0 stopped 0 halted 0
 resume_all_vcpus-cpu1: stop 0 stopped 0 halted 1
 
 
 SLOF **
 QEMU Starting
 
 [ boot fine and then we reboot ]


 cpu_ppc_decr_excp-cpu1: stop 0 stopped 0 halted 0 SPR_LPCR 2000
 cpu_ppc_decr_excp-cpu1: stop 0 stopped 0 halted 0 SPR_LPCR 2000
 cpu_ppc_decr_excp-cpu1: stop 0 stopped 0 halted 0 SPR_LPCR 2000
 cpu_ppc_decr_excp-cpu1: stop 0 stopped 0 halted 1 SPR_LPCR 2000
 [   54.044388] reboot: Restarting system
 spapr_cpu_reset-cpu1: stop 0 stopped 1 halted 1 SPR_LPCR 2000
 resume_all_vcpus-cpu0: stop 0 stopped 0 halted 0
 resume_all_vcpus-cpu1: stop 0 stopped 0 halted 1
 cpu_ppc_decr_excp-cpu1: stop 0 stopped 0 halted 1 SPR_LPCR 2000
 
 
 
 
 SSLLOOFF ***[0m 
**
 QEMU Starting
  Build Date = Aug 16 2017 12:38:57
  FW Version = git-685af54d8a47a42d
 ***
 QEMU Starting
  Build Date = Aug 16 2017 12:38:57
  FW Version = git-685af54d8a47a42d
 ERROR: Flatten device tree not available!
 
  SPRG2 = RSPRG3 = 0 0 

One difference I see here is, the decr exception is delivered before
reset in case of first boot for secondary cpu, and then I do not see any
decr exception until rtas-start.

In case of a reboot, we are getting decr_exception at some interval,
then there is spapr_cpu_reset, after that the cpus are resumed, the
interrupt gets delivered just after that which brings out the CPU-1 from
halted state. Other thing is, could it be a stale interrupt, delivered
late? As I do not see any such prints after that.

Regards
Nikunj




diff --git a/cpus.c b/cpus.c
index 9bed61e..f6cfe65 100644
--- a/cpus.c
+++ b/cpus.c
@@ -1638,6 +1638,8 @@ void resume_all_vcpus(void)
 qemu_clock_enable(QEMU_CLOCK_VIRTUAL, true);
 CPU_FOREACH(cpu) {
 cpu_resume(cpu);
+fprintf(stderr, "%s-cpu%d: stop %d stopped %d halted %d\n",
+__func__, cpu->cpu_index, cpu->stop, cpu->stopped, 
cpu->halted);
 }
 }
 
diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
index 224184d..14823a8 100644
--- a/hw/ppc/ppc.c
+++ 

Re: [Qemu-devel] [PATCH v3 0/9] virtio: enhance virtio_error messages

[Ladi Prosek]

On Fri, Jul 21, 2017 at 5:21 PM, Stefan Hajnoczi  wrote:
> On Mon, Jul 17, 2017 at 10:11:43AM +0200, Ladi Prosek wrote:
>> Output like "Virtqueue size exceeded" is not much useful in identifying the
>> culprit. This series beefs up virtio_error to print the virtio device name
>> and id, and introduces virtqueue_error which additionally includes the index
>> of the virtqueue where the error occured.
>>
>> Patches 1 to 3 lay the groundwork, patches 4 to 8 convert virtio devices to
>> use virtqueue_error instead of virtio_error. Patch 9 adds virtio_error and
>> virtqueue_error to the list of error funcs in checkpatch.pl.
>>
>> v1->v2:
>> * Modified virtio_error and added virtqueue_error (Stefan)
>> * Now also printing device id (Stefan)
>> * Went over all virtio_error call sites and converted them to virtqueue_error
>>   as appropriate; added virtio device maintainers to cc
>>
>> v2->v3:
>> * Removed patch 1 (Stefan, Markus)
>> * Split patch 3 into 2 (adds virtqueue_error) and 3 (makes virtio.c call it)
>>   (Cornelia)
>> * Added patch 9 to modify $qemu_error_funcs in checkpatch.pl (Greg)
>> * s/includes queue index/includes the queue index/ in patch 3-9 commit
>>   messages (Cornelia)
>> * Fixed virtio_get_device_id to return empty string instead of NULL if the
>>   device doesn't have an id (Stefan)
>> * Simplified the change in virtio-crypto.c to use vcrypto->ctrl_vq instead
>>   of propagating the vq pointer in function arguments (Cornelia, Gonglei)
>>
>> Ladi Prosek (9):
>>   virtio: enhance virtio_error messages
>>   virtio: introduce virtqueue_error
>>   virtio: use virtqueue_error for errors with queue context
>>   virtio-9p: use virtqueue_error for errors with queue context
>>   virtio-blk: use virtqueue_error for errors with queue context
>>   virtio-net: use virtqueue_error for errors with queue context
>>   virtio-scsi: use virtqueue_error for errors with queue context
>>   virtio-crypto: use virtqueue_error for errors with queue context
>>   checkpatch: add virtio_error and virtqueue_error to error funcs
>>
>>  hw/9pfs/virtio-9p-device.c |  37 ++
>>  hw/block/virtio-blk.c  |   6 +--
>>  hw/net/virtio-net.c|  24 -
>>  hw/scsi/virtio-scsi.c  |   2 +-
>>  hw/virtio/virtio-crypto.c  |  49 ++-
>>  hw/virtio/virtio.c | 119 
>> +++--
>>  include/hw/virtio/virtio.h |   1 +
>>  scripts/checkpatch.pl  |   4 +-
>>  8 files changed, 143 insertions(+), 99 deletions(-)
>
> Looks good.  QEMU is currently in freeze for the 2.10 release.  You may
> need to resend so Michael Tsirkin can merge it after the release, or if
> he maintainers a -next branch he could merge it right away.

Should I resend the series, Michael?

Thanks,
Ladi