[Qemu-devel] [PATCH v2 12/32] qmp: Redo how the client requests out-of-band execution

[Markus Armbruster]

Commit cf869d53172 "qmp: support out-of-band (oob) execution" added a
general mechanism for command-independent arguments just for an
out-of-band flag:

The "control" key is introduced to store this extra flag.  "control"
field is used to store arguments that are shared by all the commands,
rather than command specific arguments.  Let "run-oob" be the first.

However, it failed to reject unknown members of "control".  For
instance, in QMP command

{"execute": "query-name", "id": 42, "control": {"crap": true}}

"crap" gets silently ignored.

Instead of fixing this, revert the general "control" mechanism
(because YAGNI), and do it the way I initially proposed, with key
"exec-oob".  Simpler code, simper interface.

An out-of-band command

{"execute": "migrate-pause", "id": 42, "control": {"run-oob": true}}

becomes

{"exec-oob": "migrate-pause", "id": 42}

Signed-off-by: Markus Armbruster 
Reviewed-by: Eric Blake 
---
 docs/devel/qapi-code-gen.txt | 10 +++
 docs/interop/qmp-spec.txt| 18 ++---
 monitor.c|  3 +++
 qapi/qmp-dispatch.c  | 51 +++-
 tests/qmp-test.c |  7 ++---
 tests/test-qga.c |  3 +--
 6 files changed, 39 insertions(+), 53 deletions(-)

diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt
index 9625798d16..f020f6bab2 100644
--- a/docs/devel/qapi-code-gen.txt
+++ b/docs/devel/qapi-code-gen.txt
@@ -649,13 +649,11 @@ example:
  { 'command': 'migrate_recover',
'data': { 'uri': 'str' }, 'allow-oob': true }
 
-To execute a command with out-of-band priority, the client specifies
-the "control" field in the request, with "run-oob" set to
-true. Example:
+To execute a command with out-of-band priority, the client uses key
+"exec-oob" instead of "execute".  Example:
 
- => { "execute": "command-support-oob",
-  "arguments": { ... },
-  "control": { "run-oob": true } }
+ => { "exec-oob": "migrate-recover",
+  "arguments": { "uri": "tcp:192.168.1.200:12345" } }
  <= { "return": { } }
 
 Without it, even the commands that support out-of-band execution will
diff --git a/docs/interop/qmp-spec.txt b/docs/interop/qmp-spec.txt
index a1d6f9ee06..1566b8ae5e 100644
--- a/docs/interop/qmp-spec.txt
+++ b/docs/interop/qmp-spec.txt
@@ -92,12 +92,16 @@ Currently supported capabilities are:
 
 The format for command execution is:
 
-{ "execute": json-string, "arguments": json-object, "id": json-value,
-  "control": json-object }
+{ "execute": json-string, "arguments": json-object, "id": json-value }
+
+or
+
+{ "exec-oob": json-string, "arguments": json-object, "id": json-value }
 
  Where,
 
-- The "execute" member identifies the command to be executed by the Server
+- The "execute" or "exec-oob" member identifies the command to be
+  executed by the server.  The latter requests out-of-band execution.
 - The "arguments" member is used to pass any arguments required for the
   execution of the command, it is optional when no arguments are
   required. Each command documents what contents will be considered
@@ -106,9 +110,6 @@ The format for command execution is:
   command execution, it is optional and will be part of the response
   if provided.  The "id" member can be any json-value.  A json-number
   incremented for each successive command works fine.
-- The optional "control" member further specifies how the command is
-  to be executed.  Currently, its only member is optional "run-oob".
-  See section "2.3.1 Out-of-band execution" for details.
 
 2.3.1 Out-of-band execution
 ---
@@ -129,9 +130,6 @@ To be able to match responses back to their commands, the 
client needs
 to pass "id" with out-of-band commands.  Passing it with all commands
 is recommended for clients that accept capability "oob".
 
-To execute a command out-of-band, the client puts "run-oob": true into
-execute's member "control".
-
 If the client sends in-band commands faster than the server can
 execute them, the server will eventually drop commands to limit the
 queue length.  The sever sends event COMMAND_DROPPED then.
@@ -274,7 +272,7 @@ S: { "timestamp": { "seconds": 1258551470, "microseconds": 
802384 },
 3.7 Out-of-band execution
 -
 
-C: { "execute": "migrate-pause", "id": 42, "control": { "run-oob": true } }
+C: { "exec-oob": "migrate-pause", "id": 42 }
 S: { "id": 42,
  "error": { "class": "GenericError",
   "desc": "migrate-pause is currently only supported during 
postcopy-active state" } }
diff --git a/monitor.c b/monitor.c
index 2c28b05ecb..8b0c29ce06 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1299,6 +1299,9 @@ static bool qmp_cmd_oob_check(Monitor *mon, QDict *req, 
Error **errp)
 QmpCommand *cmd;
 
 command = qdict_get_try_str(req, "execute");
+if (!command) {
+command = qdict_get_try_str(req, "exec-oob");
+}
 if (!command) {
 error_setg(errp, "Command field 'execute' missing");
 r

[Qemu-devel] [PATCH v2 31/32] monitor: Improve some comments

[Markus Armbruster]

Signed-off-by: Markus Armbruster 
Reviewed-by: Eric Blake 
---
 monitor.c | 100 --
 1 file changed, 45 insertions(+), 55 deletions(-)

diff --git a/monitor.c b/monitor.c
index 590e5b5b04..14af7b7ea6 100644
--- a/monitor.c
+++ b/monitor.c
@@ -168,15 +168,16 @@ typedef struct {
 JSONMessageParser parser;
 /*
  * When a client connects, we're in capabilities negotiation mode.
- * When command qmp_capabilities succeeds, we go into command
- * mode.
+ * @commands is &qmp_cap_negotiation_commands then.  When command
+ * qmp_capabilities succeeds, we go into command mode, and
+ * @command becomes &qmp_commands.
  */
 QmpCommandList *commands;
 bool capab_offered[QMP_CAPABILITY__MAX]; /* capabilities offered */
 bool capab[QMP_CAPABILITY__MAX]; /* offered and accepted */
 /*
- * Protects qmp request/response queue.  Please take monitor_lock
- * first when used together.
+ * Protects qmp request/response queue.
+ * Take monitor_lock first when you need both.
  */
 QemuMutex qmp_queue_lock;
 /* Input queue that holds all the parsed QMP requests */
@@ -231,7 +232,7 @@ struct Monitor {
 QemuMutex mon_lock;
 
 /*
- * Fields that are protected by the per-monitor lock.
+ * Members that are protected by the per-monitor lock
  */
 QLIST_HEAD(, mon_fd_t) fds;
 QString *outbuf;
@@ -240,6 +241,7 @@ struct Monitor {
 int mux_out;
 };
 
+/* Shared monitor I/O thread */
 IOThread *mon_iothread;
 
 /* Bottom half to dispatch the requests received from I/O thread */
@@ -301,9 +303,9 @@ static inline bool monitor_is_qmp(const Monitor *mon)
 }
 
 /**
- * Whether @mon is using readline?  Note: not all HMP monitors use
- * readline, e.g., gdbserver has a non-interactive HMP monitor, so
- * readline is not used there.
+ * Is @mon is using readline?
+ * Note: not all HMP monitors use readline, e.g., gdbserver has a
+ * non-interactive HMP monitor, so readline is not used there.
  */
 static inline bool monitor_uses_readline(const Monitor *mon)
 {
@@ -317,14 +319,12 @@ static inline bool monitor_is_hmp_non_interactive(const 
Monitor *mon)
 
 /*
  * Return the clock to use for recording an event's time.
+ * It's QEMU_CLOCK_REALTIME, except for qtests it's
+ * QEMU_CLOCK_VIRTUAL, to support testing rate limits.
  * Beware: result is invalid before configure_accelerator().
  */
 static inline QEMUClockType monitor_get_event_clock(void)
 {
-/*
- * This allows us to perform tests on the monitor queues to verify
- * that the rate limits are enforced.
- */
 return qtest_enabled() ? QEMU_CLOCK_VIRTUAL : QEMU_CLOCK_REALTIME;
 }
 
@@ -367,7 +367,7 @@ static void qmp_request_free(QMPRequest *req)
 g_free(req);
 }
 
-/* Must with the mon->qmp.qmp_queue_lock held */
+/* Caller must hold mon->qmp.qmp_queue_lock */
 static void monitor_qmp_cleanup_req_queue_locked(Monitor *mon)
 {
 while (!g_queue_is_empty(mon->qmp.qmp_requests)) {
@@ -375,7 +375,7 @@ static void monitor_qmp_cleanup_req_queue_locked(Monitor 
*mon)
 }
 }
 
-/* Must with the mon->qmp.qmp_queue_lock held */
+/* Caller must hold the mon->qmp.qmp_queue_lock */
 static void monitor_qmp_cleanup_resp_queue_locked(Monitor *mon)
 {
 while (!g_queue_is_empty(mon->qmp.qmp_responses)) {
@@ -406,7 +406,7 @@ static gboolean monitor_unblocked(GIOChannel *chan, 
GIOCondition cond,
 return FALSE;
 }
 
-/* Called with mon->mon_lock held.  */
+/* Caller must hold mon->mon_lock */
 static void monitor_flush_locked(Monitor *mon)
 {
 int rc;
@@ -522,10 +522,8 @@ static void qmp_queue_response(Monitor *mon, QDict *rsp)
 {
 if (mon->use_io_thread) {
 /*
- * If using I/O thread, we need to queue the item so that I/O
- * thread will do the rest for us.  Take refcount so that
- * caller won't free the data (which will be finally freed in
- * responder thread).
+ * Push a reference to the response queue.  The I/O thread
+ * drains that queue and emits.
  */
 qemu_mutex_lock(&mon->qmp.qmp_queue_lock);
 g_queue_push_tail(mon->qmp.qmp_responses, qobject_ref(rsp));
@@ -533,8 +531,8 @@ static void qmp_queue_response(Monitor *mon, QDict *rsp)
 qemu_bh_schedule(qmp_respond_bh);
 } else {
 /*
- * If not using monitor I/O thread, then we are in main thread.
- * Do the emission right away.
+ * Not using monitor I/O thread, i.e. we are in the main thread.
+ * Emit right away.
  */
 qmp_send_response(mon, rsp);
 }
@@ -610,8 +608,9 @@ static MonitorQAPIEventConf 
monitor_qapi_event_conf[QAPI_EVENT__MAX] = {
 };
 
 /*
- * Emits the event to every monitor instance, @event is only used for trace
- * Called with monitor_lock held.
+ * Broadcast an event to all monitors.
+ * @qdict is the event object.  Its member "event" must match @event.
+ * Caller must hold monit

[Qemu-devel] [PATCH v2 30/32] qmp: Clean up capability negotiation after commit 02130314d8c

[Markus Armbruster]

qmp_greeting() offers capabilities to the client, and
qmp_qmp_capabilities() accepts or denies capabilities requested by the
client.  The two compute the set of available capabilities
independently.  Not nice.

Clean this up as follows.  Compute available capabilities just once in
monitor_qmp_caps_reset(), and store them in Monitor member
qmp.capab_offered[].  Have qmp_greeting() and qmp_qmp_capabilities()
use that.  Both are now oblivious of capability details.

Signed-off-by: Markus Armbruster 
Reviewed-by: Eric Blake 
---
 monitor.c | 93 ---
 1 file changed, 40 insertions(+), 53 deletions(-)

diff --git a/monitor.c b/monitor.c
index 876a3a23a7..590e5b5b04 100644
--- a/monitor.c
+++ b/monitor.c
@@ -172,7 +172,8 @@ typedef struct {
  * mode.
  */
 QmpCommandList *commands;
-bool qmp_caps[QMP_CAPABILITY__MAX];
+bool capab_offered[QMP_CAPABILITY__MAX]; /* capabilities offered */
+bool capab[QMP_CAPABILITY__MAX]; /* offered and accepted */
 /*
  * Protects qmp request/response queue.  Please take monitor_lock
  * first when used together.
@@ -1252,52 +1253,56 @@ static void monitor_init_qmp_commands(void)
  qmp_marshal_qmp_capabilities, QCO_ALLOW_PRECONFIG);
 }
 
-static bool qmp_cap_enabled(Monitor *mon, QMPCapability cap)
-{
-return mon->qmp.qmp_caps[cap];
-}
-
 static bool qmp_oob_enabled(Monitor *mon)
 {
-return qmp_cap_enabled(mon, QMP_CAPABILITY_OOB);
+return mon->qmp.capab[QMP_CAPABILITY_OOB];
 }
 
-static void qmp_caps_check(Monitor *mon, QMPCapabilityList *list,
-   Error **errp)
+static void monitor_qmp_caps_reset(Monitor *mon)
 {
+memset(mon->qmp.capab_offered, 0, sizeof(mon->qmp.capab_offered));
+memset(mon->qmp.capab, 0, sizeof(mon->qmp.capab));
+mon->qmp.capab_offered[QMP_CAPABILITY_OOB] = mon->use_io_thread;
+}
+
+/*
+ * Accept QMP capabilities in @list for @mon.
+ * On success, set mon->qmp.capab[], and return true.
+ * On error, set @errp, and return false.
+ */
+static bool qmp_caps_accept(Monitor *mon, QMPCapabilityList *list,
+Error **errp)
+{
+GString *unavailable = NULL;
+bool capab[QMP_CAPABILITY__MAX];
+
+memset(capab, 0, sizeof(capab));
+
 for (; list; list = list->next) {
-assert(list->value < QMP_CAPABILITY__MAX);
-switch (list->value) {
-case QMP_CAPABILITY_OOB:
-if (!mon->use_io_thread) {
-/*
- * Out-of-band only works with monitors that are
- * running on dedicated I/O thread.
- */
-error_setg(errp, "This monitor does not support "
-   "out-of-band (OOB)");
-return;
+if (!mon->qmp.capab_offered[list->value]) {
+if (!unavailable) {
+unavailable = g_string_new(QMPCapability_str(list->value));
+} else {
+g_string_append_printf(unavailable, ", %s",
+  QMPCapability_str(list->value));
 }
-break;
-default:
-break;
 }
+capab[list->value] = true;
 }
-}
 
-/* This function should only be called after capabilities are checked. */
-static void qmp_caps_apply(Monitor *mon, QMPCapabilityList *list)
-{
-for (; list; list = list->next) {
-mon->qmp.qmp_caps[list->value] = true;
+if (unavailable) {
+error_setg(errp, "Capability %s not available", unavailable->str);
+g_string_free(unavailable, true);
+return false;
 }
+
+memcpy(mon->qmp.capab, capab, sizeof(capab));
+return true;
 }
 
 void qmp_qmp_capabilities(bool has_enable, QMPCapabilityList *enable,
   Error **errp)
 {
-Error *local_err = NULL;
-
 if (cur_mon->qmp.commands == &qmp_commands) {
 error_set(errp, ERROR_CLASS_COMMAND_NOT_FOUND,
   "Capabilities negotiation is already complete, command "
@@ -1305,19 +1310,8 @@ void qmp_qmp_capabilities(bool has_enable, 
QMPCapabilityList *enable,
 return;
 }
 
-/* Enable QMP capabilities provided by the client if applicable. */
-if (has_enable) {
-qmp_caps_check(cur_mon, enable, &local_err);
-if (local_err) {
-/*
- * Failed check on any of the capabilities will fail the
- * entire command (and thus not apply any of the other
- * capabilities that were also requested).
- */
-error_propagate(errp, local_err);
-return;
-}
-qmp_caps_apply(cur_mon, enable);
+if (!qmp_caps_accept(cur_mon, enable, errp)) {
+return;
 }
 
 cur_mon->qmp.commands = &qmp_commands;
@@ -4384,11 +4378,9 @@ static QDict *qmp_greeting(Monitor *mon)
 qmp_marshal_query_version(NULL, &ver, NULL);
 
 for (cap = 0; cap < QMP_CAPABILITY__MAX; cap++

[Qemu-devel] [PATCH v2 04/32] qmp: Document COMMAND_DROPPED design flaw

[Markus Armbruster]

Events are broadcast to all monitors.  If another monitor's client has
a command with the same ID in flight, the event will incorrectly claim
that command was dropped.  This must be fixed before out-of-band
execution can graduate from "experimental".

Signed-off-by: Markus Armbruster 
Reviewed-by: Eric Blake 
---
 monitor.c  | 6 ++
 qapi/misc.json | 3 +++
 2 files changed, 9 insertions(+)

diff --git a/monitor.c b/monitor.c
index 3fb05d50aa..96e87d8664 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4330,6 +4330,12 @@ static void handle_qmp_command(JSONMessageParser 
*parser, GQueue *tokens)
 /* Drop the request if queue is full. */
 if (mon->qmp.qmp_requests->length >= QMP_REQ_QUEUE_LEN_MAX) {
 qemu_mutex_unlock(&mon->qmp.qmp_queue_lock);
+/*
+ * FIXME @id's scope is just @mon, and broadcasting it is
+ * wrong.  If another monitor's client has a command with
+ * the same ID in flight, the event will incorrectly claim
+ * that command was dropped.
+ */
 qapi_event_send_command_dropped(id,
 COMMAND_DROP_REASON_QUEUE_FULL,
 &error_abort);
diff --git a/qapi/misc.json b/qapi/misc.json
index 0446c3e48e..74cd97f237 100644
--- a/qapi/misc.json
+++ b/qapi/misc.json
@@ -3454,6 +3454,9 @@
 # only be dropped when the oob capability is enabled.
 #
 # @id: The dropped command's "id" field.
+# FIXME Broken by design.  Events are broadcast to all monitors.  If
+# another monitor's client has a command with the same ID in flight,
+# the event will incorrectly claim that command was dropped.
 #
 # @reason: The reason why the command is dropped.
 #
-- 
2.17.1

Re: [Qemu-devel] [PATCH v2 1/5] i386: Add support for IA32_PRED_CMD and IA32_ARCH_CAPABILITIES MSRs

[Paolo Bonzini]

On 03/07/2018 10:48, Robert Hoo wrote:
>>
>> However, I suggest adding it to the FeatureWord enum, since everything
>> that handles FeatureWord applies to this new kind of MSR as well.
>> Currently FeatureWord is only for CPUID leaves, but it doesn't have to
>> be like that.
>>
> I think this will be changing struct FeatureWordInfo, which is designed
> for cpuid enumerations. You must not want to do that. May I know more
> details about your thought?

The simplest way is to put CPUIDs first and MSRs second in FeatureWord.
Then you can do

 FEAT_XSAVE_COMP_LO, /* CPUID[EAX=0xd,ECX=0].EAX */
 FEAT_XSAVE_COMP_HI, /* CPUID[EAX=0xd,ECX=0].EDX */
+FEATURE_WORDS_NUM_CPUID,
+FEATURE_WORDS_FIRST_MSR = FEATURE_WORDS_NUM_CPUID,
+FEAT_MSR_ARCH_CAPABILITIES = FEATURE_WORDS_FIRST_MSR,
 FEATURE_WORDS,
};

#define FEATURE_WORDS_NUM_MSRS (FEATURE_WORDS - \
FEATURE_WORDS_FIRST_MSR)

Then the existing loops that use FeatureWordInfo can go up to
FEATURE_WORDS_NUM_CPUID.

Thanks,

Paolo

> And, if I implemented ARCH_CAPABILITIES-bits features in FeatureWord,
> then no necessity of having it in kvm_msr_entries, right?

Re: [Qemu-devel] [PATCH 00/32] qmp: Fixes and cleanups around OOB commands

[Markus Armbruster]

Markus Armbruster  writes:

> Marc-André Lureau  writes:
>
>> Hi
>>
>> On Mon, Jul 2, 2018 at 6:21 PM, Markus Armbruster  wrote:
>>> We're trying to get the out-of-band execution feature ready.  This
>>> series fixes a number of issues, and marks a design flaw FIXME.  More
>>> work is needed.
>>
>> They are several related worthy cleanups in the qapi-async series.
>>
>> They will now conflict with your changes though.
>
> Puh!
>
> I haven't looked closely at the qapi-async series; the qapi-if series
> took priority.
>
>> Please take a look roughly from 4/38 to 17/38, and let me know if you
>> want me to rebase it for 3.0.
>
> Extracting its preliminary cleanups into a separate series that could be
> merged more quickly makes could make sense.  I'll try to have a look.

PATCH 02-17 look like they could stand on their own.  Too bad I missed
them, and thus created a whole bunch of conflicts (-EQUEUE2LONG).  Would
be really nice if you could salvage the parts that still make sense on
top of this series.  Even if they should miss 3.0.

Re: [Qemu-devel] [PATCH 07/32] qmp: Make "id" optional again even in "oob" monitors

[Daniel P . Berrangé]

On Tue, Jul 03, 2018 at 08:14:35AM +0200, Markus Armbruster wrote:
> Peter Xu  writes:
> 
> > On Mon, Jul 02, 2018 at 06:21:53PM +0200, Markus Armbruster wrote:
> >> Commit cf869d53172 "qmp: support out-of-band (oob) execution" made
> >> "id" mandatory for all commands when the client accepted capability
> >> "oob".  This is rather onerous when you play with QMP by hand, and
> >> unnecessarily so: only out-of-band commands need an ID for reliable
> >> matching of response to command.
> >> 
> >> Revert that part of commit cf869d53172 for now.  We may still make
> >> "id" mandatory for out-of-band commands.
> >
> > This change should be okay with current implementation when
> > out-of-band commands are still in order themselves, though I'm still
> > not that confident on whether we really want this change if only for
> > the sake of easier usage for human beings.
> >
> > If we see Libvirt, the real player for QMP - it has the "id" field
> > even for in-band commands always.  I'd say the "id" field is really
> > helpful for machines, though not that friendly to us.
> >
> > Basically I'll read it as: machines like "id"s, humans hate "id"s.
> > And QMP is Qemu Machine Protocol after all... so not sure whether
> > it'll be good we change that for us humans.
> 
> "id" being optional doesn't hurt libvirt in any way.  Thus, I see no
> need to inconvenience humans.
> 
> Daniel has argued[*] for making "id" mandatory with OOB commands.  I'm
> not rejecting that argument.  But I needed to get this out in a hurry,
> and simply reverting something is quicker than debating and implementing
> an improvement.  There's still time to tweak this before the release.

I'm fine with this - I agree that it is more important to avoid creating
a regression, than to enforce "id" for OOB. We've still documented that
OOB will require "id", so it would not be a surprise once we enforce it
later. 

Reviewed-by: Daniel P. Berrangé 


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

[Qemu-devel] [PATCH v2 23/32] qmp: Use QDict * instead of QObject * for response objects

[Markus Armbruster]

By using the more specific type, we get fewer downcasts.  The
downcasts are safe, but not obviously so, at least not locally.

Signed-off-by: Markus Armbruster 
Reviewed-by: Eric Blake 
---
 include/qapi/qmp/dispatch.h |  4 ++--
 monitor.c   | 31 ---
 qapi/qmp-dispatch.c |  6 +++---
 qga/main.c  |  8 
 tests/test-qmp-cmds.c   | 17 +++--
 5 files changed, 32 insertions(+), 34 deletions(-)

diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h
index a53e11c9b1..4e2e749faf 100644
--- a/include/qapi/qmp/dispatch.h
+++ b/include/qapi/qmp/dispatch.h
@@ -48,8 +48,8 @@ bool qmp_command_is_enabled(const QmpCommand *cmd);
 const char *qmp_command_name(const QmpCommand *cmd);
 bool qmp_has_success_response(const QmpCommand *cmd);
 QDict *qmp_error_response(Error *err);
-QObject *qmp_dispatch(QmpCommandList *cmds, QObject *request,
-  bool allow_oob);
+QDict *qmp_dispatch(QmpCommandList *cmds, QObject *request,
+bool allow_oob);
 bool qmp_is_oob(QDict *dict);
 
 typedef void (*qmp_cmd_callback_fn)(QmpCommand *cmd, void *opaque);
diff --git a/monitor.c b/monitor.c
index db85cd0a57..0f7a96213f 100644
--- a/monitor.c
+++ b/monitor.c
@@ -378,7 +378,7 @@ static void monitor_qmp_cleanup_req_queue_locked(Monitor 
*mon)
 static void monitor_qmp_cleanup_resp_queue_locked(Monitor *mon)
 {
 while (!g_queue_is_empty(mon->qmp.qmp_responses)) {
-qobject_unref((QObject *)g_queue_pop_head(mon->qmp.qmp_responses));
+qobject_unref((QDict *)g_queue_pop_head(mon->qmp.qmp_responses));
 }
 }
 
@@ -527,7 +527,8 @@ static void monitor_json_emitter(Monitor *mon, QObject 
*data)
  * responder thread).
  */
 qemu_mutex_lock(&mon->qmp.qmp_queue_lock);
-g_queue_push_tail(mon->qmp.qmp_responses, qobject_ref(data));
+g_queue_push_tail(mon->qmp.qmp_responses,
+  qobject_ref(qobject_to(QDict, data)));
 qemu_mutex_unlock(&mon->qmp.qmp_queue_lock);
 qemu_bh_schedule(qmp_respond_bh);
 } else {
@@ -541,13 +542,13 @@ static void monitor_json_emitter(Monitor *mon, QObject 
*data)
 
 struct QMPResponse {
 Monitor *mon;
-QObject *data;
+QDict *data;
 };
 typedef struct QMPResponse QMPResponse;
 
-static QObject *monitor_qmp_response_pop_one(Monitor *mon)
+static QDict *monitor_qmp_response_pop_one(Monitor *mon)
 {
-QObject *data;
+QDict *data;
 
 qemu_mutex_lock(&mon->qmp.qmp_queue_lock);
 data = g_queue_pop_head(mon->qmp.qmp_responses);
@@ -558,10 +559,10 @@ static QObject *monitor_qmp_response_pop_one(Monitor *mon)
 
 static void monitor_qmp_response_flush(Monitor *mon)
 {
-QObject *data;
+QDict *data;
 
 while ((data = monitor_qmp_response_pop_one(mon))) {
-monitor_json_emitter_raw(mon, data);
+monitor_json_emitter_raw(mon, QOBJECT(data));
 qobject_unref(data);
 }
 }
@@ -573,7 +574,7 @@ static void monitor_qmp_response_flush(Monitor *mon)
 static bool monitor_qmp_response_pop_any(QMPResponse *response)
 {
 Monitor *mon;
-QObject *data = NULL;
+QDict *data = NULL;
 
 qemu_mutex_lock(&monitor_lock);
 QTAILQ_FOREACH(mon, &mon_list, entry) {
@@ -593,7 +594,7 @@ static void monitor_qmp_bh_responder(void *opaque)
 QMPResponse response;
 
 while (monitor_qmp_response_pop_any(&response)) {
-monitor_json_emitter_raw(response.mon, response.data);
+monitor_json_emitter_raw(response.mon, QOBJECT(response.data));
 qobject_unref(response.data);
 }
 }
@@ -4103,20 +4104,20 @@ static int monitor_can_read(void *opaque)
  * 2. rsp, err, and id may be NULL.
  * 3. If err != NULL then rsp must be NULL.
  */
-static void monitor_qmp_respond(Monitor *mon, QObject *rsp,
+static void monitor_qmp_respond(Monitor *mon, QDict *rsp,
 Error *err, QObject *id)
 {
 if (err) {
 assert(!rsp);
-rsp = QOBJECT(qmp_error_response(err));
+rsp = qmp_error_response(err);
 }
 
 if (rsp) {
 if (id) {
-qdict_put_obj(qobject_to(QDict, rsp), "id", qobject_ref(id));
+qdict_put_obj(rsp, "id", qobject_ref(id));
 }
 
-monitor_json_emitter(mon, rsp);
+monitor_json_emitter(mon, QOBJECT(rsp));
 }
 
 qobject_unref(id);
@@ -4126,7 +4127,7 @@ static void monitor_qmp_respond(Monitor *mon, QObject 
*rsp,
 static void monitor_qmp_dispatch(Monitor *mon, QObject *req, QObject *id)
 {
 Monitor *old_mon;
-QObject *rsp;
+QDict *rsp;
 QDict *error;
 
 old_mon = cur_mon;
@@ -4137,7 +4138,7 @@ static void monitor_qmp_dispatch(Monitor *mon, QObject 
*req, QObject *id)
 cur_mon = old_mon;
 
 if (mon->qmp.commands == &qmp_cap_negotiation_commands) {
-error = qdict_get_qdict(qobject_to(QDict, rsp), "error");
+error = qdict_get_qdict(rsp, "error");
 if (error

[Qemu-devel] [PATCH v2 14/32] qmp: Always free QMPRequest with qmp_request_free()

[Markus Armbruster]

monitor_qmp_dispatch_one() frees a QMPRequest manually, because it
needs to keep a reference to ->id.  Premature optimization.  Take an
additional reference so we can use qmp_request_free().

Signed-off-by: Markus Armbruster 
Reviewed-by: Eric Blake 
---
 monitor.c | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/monitor.c b/monitor.c
index 10991757f6..94f5660c3c 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4181,8 +4181,6 @@ static void monitor_qmp_dispatch_one(QMPRequest *req_obj)
 id = req_obj->id;
 need_resume = req_obj->need_resume;
 
-g_free(req_obj);
-
 old_mon = cur_mon;
 cur_mon = mon;
 
@@ -4191,14 +4189,14 @@ static void monitor_qmp_dispatch_one(QMPRequest 
*req_obj)
 cur_mon = old_mon;
 
 /* Respond if necessary */
-monitor_qmp_respond(mon, rsp, NULL, id);
+monitor_qmp_respond(mon, rsp, NULL, qobject_ref(id));
 
 /* This pairs with the monitor_suspend() in handle_qmp_command(). */
 if (need_resume) {
 monitor_resume(mon);
 }
 
-qobject_unref(req);
+qmp_request_free(req_obj);
 }
 
 /*
-- 
2.17.1

[Qemu-devel] [PULL 1/1] ui: do not build-depend or link with libdrm, it is not needed

[Gerd Hoffmann]

From: Michael Tokarev 

Opengl support brings up libdrm. But actually nothing uses this library
or includes any of its headers. Just remove checking for it from configure.

Signed-off-by: Michael Tokarev 
Reviewed-by: Philippe Mathieu-Daudé 
Message-id: 20180630165448.30795-1-...@msgid.tls.msk.ru
Signed-off-by: Gerd Hoffmann 
---
 configure | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/configure b/configure
index 65548df1fc..951ff022dd 100755
--- a/configure
+++ b/configure
@@ -3865,7 +3865,7 @@ libs_softmmu="$libs_softmmu $fdt_libs"
 # opengl probe (for sdl2, gtk, milkymist-tmu2)
 
 if test "$opengl" != "no" ; then
-  opengl_pkgs="epoxy libdrm gbm"
+  opengl_pkgs="epoxy gbm"
   if $pkg_config $opengl_pkgs; then
 opengl_cflags="$($pkg_config --cflags $opengl_pkgs)"
 opengl_libs="$($pkg_config --libs $opengl_pkgs)"
-- 
2.9.3

[Qemu-devel] [PATCH v2 32/32] qapi: Polish command flags documentation in qapi-code-gen.txt

[Markus Armbruster]

Signed-off-by: Markus Armbruster 
Reviewed-by: Eric Blake 
---
 docs/devel/qapi-code-gen.txt | 61 +++-
 1 file changed, 25 insertions(+), 36 deletions(-)

diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt
index f020f6bab2..8decd6f17d 100644
--- a/docs/devel/qapi-code-gen.txt
+++ b/docs/devel/qapi-code-gen.txt
@@ -624,60 +624,48 @@ its return value.
 In rare cases, QAPI cannot express a type-safe representation of a
 corresponding Client JSON Protocol command.  You then have to suppress
 generation of a marshalling function by including a key 'gen' with
-boolean value false, and instead write your own function.  Please try
-to avoid adding new commands that rely on this, and instead use
-type-safe unions.  For an example of this usage:
+boolean value false, and instead write your own function.  For
+example:
 
  { 'command': 'netdev_add',
'data': {'type': 'str', 'id': 'str'},
'gen': false }
 
+Please try to avoid adding new commands that rely on this, and instead
+use type-safe unions.
+
 Normally, the QAPI schema is used to describe synchronous exchanges,
 where a response is expected.  But in some cases, the action of a
 command is expected to change state in a way that a successful
 response is not possible (although the command will still return a
 normal dictionary error on failure).  When a successful reply is not
-possible, the command expression should include the optional key
+possible, the command expression includes the optional key
 'success-response' with boolean value false.  So far, only QGA makes
 use of this member.
 
-A command can be declared to support out-of-band (OOB) execution.  By
-default, commands do not support OOB.  To declare a command that
-supports it, the schema includes an extra 'allow-oob' field.  For
-example:
+Key 'allow-oob' declares whether the command supports out-of-band
+(OOB) execution.  It defaults to false.  For example:
 
  { 'command': 'migrate_recover',
'data': { 'uri': 'str' }, 'allow-oob': true }
 
-To execute a command with out-of-band priority, the client uses key
-"exec-oob" instead of "execute".  Example:
+See qmp-spec.txt for out-of-band execution syntax and semantics.
 
- => { "exec-oob": "migrate-recover",
-  "arguments": { "uri": "tcp:192.168.1.200:12345" } }
- <= { "return": { } }
+Commands supporting out-of-band execution can still be executed
+in-band.
 
-Without it, even the commands that support out-of-band execution will
-still be run in-band.
+When a command is executed in-band, its handler runs in the main
+thread with the BQL held.
 
-Under normal QMP command execution, the following apply to each
-command:
+When a command is executed out-of-band, its handler runs in a
+dedicated monitor I/O thread with the BQL *not* held.
 
-- They are executed in order,
-- They run only in main thread of QEMU,
-- They run with the BQL held.
+An OOB-capable command handler must satisfy the following conditions:
 
-When a command is executed with OOB, the following changes occur:
-
-- They can be completed before a pending in-band command,
-- They run in a dedicated monitor thread,
-- They run with the BQL not held.
-
-OOB command handlers must satisfy the following conditions:
-
-- It terminates quickly,
-- It does not invoke system calls that may block,
+- It terminates quickly.
+- It does not invoke system calls that may block.
 - It does not access guest RAM that may block when userfaultfd is
-  enabled for postcopy live migration,
+  enabled for postcopy live migration.
 - It takes only "fast" locks, i.e. all critical sections protected by
   any lock it takes also satisfy the conditions for OOB command
   handler code.
@@ -686,17 +674,18 @@ The restrictions on locking limit access to shared state. 
 Such access
 requires synchronization, but OOB commands can't take the BQL or any
 other "slow" lock.
 
-If in doubt, do not implement OOB execution support.
+When in doubt, do not implement OOB execution support.
 
-A command may use the optional 'allow-preconfig' key to permit its execution
-at early runtime configuration stage (preconfig runstate).
-If not specified then a command defaults to 'allow-preconfig': false.
+Key 'allow-preconfig' declares whether the command is available before
+the machine is built.  It defaults to false.  For example:
 
-An example of declaring a command that is enabled during preconfig:
  { 'command': 'qmp_capabilities',
'data': { '*enable': [ 'QMPCapability' ] },
'allow-preconfig': true }
 
+QMP is available before the machine is built only when QEMU was
+started with --preconfig.
+
 === Events ===
 
 Usage: { 'event': STRING, '*data': COMPLEX-TYPE-NAME-OR-DICT,
-- 
2.17.1

Re: [Qemu-devel] [PATCH v2 0/7] Misc sm501 improvements

[Peter Maydell]

On 3 July 2018 at 01:28, David Gibson  wrote:
> On Mon, Jul 02, 2018 at 10:42:07AM +0100, Peter Maydell wrote:
>> I can have a look, but really I think these should go via the
>> ppc tree -- the device is used in a ppc machine and an sh4 one,
>> and the ppc tree is much more active than sh4.
>
> Hm, well, if you like.  Although the gradual creep of my
> maintainership scope into things I know less and less about makes me
> rather nervous.  I tried to convince Balaton Zoltan to become sm501
> maintainer, but he didn't bite.

It's not like I know anything more about the sm501 than you do :-)

The arm parts of the tree have a lot of board and device code
I'm not familiar with either. Generally the approach I use is:
 * eyeball the code to see if it's doing anything that looks
   "obviously wrong", failing to use correct QEMU APIs, etc
 * anything that touches 'core' code or code common to multiple
   machines gets closer scrutiny
 * I might check the data sheet if I'm feeling enthusastic or
   if the code looks like it's doing weird things, but often
   not, especially if the code has had review from somebody else
 * if I have a test image to hand I'll do a smoke test, but often
   I don't have a test image

Basically I think the important thing is to make sure the
codebase stays maintainable and generally the quality bar
in terms of using the right APIs and design approaches
tends to ratchet up rather than down. If our implementation
of an obscure device isn't actually right, that doesn't
really affect very many users, so I worry less about that
side of things.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v4 2/2] iotests: add 222 to test basic fleecing

[Vladimir Sementsov-Ogievskiy]

02.07.2018 22:46, John Snow wrote:

Signed-off-by: John Snow 



Reviewed-by: Vladimir Sementsov-Ogievskiy 

--

Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v1] qemu-pr-helper: garbage response structure can be used to write data

[Dima Stepanov]

On Mon, Jul 02, 2018 at 02:21:41PM +0200, Paolo Bonzini wrote:
> On 02/07/2018 10:52, Dima Stepanov wrote:
> > Ping.
> > 
> > On Fri, Jun 15, 2018 at 12:11:44PM +0300, Dima Stepanov wrote:
> >> The prh_co_entry() routine handles requests. The first part is to read a
> >> request by calling the prh_read_request() routine, if:
> >>   1. scsi_cdb_xfer(req->cdb) call returns 0, and
> >>   2. req->cdb[0] == PERSISTENT_RESERVE_IN, then
> >> The resp->result field will be uninitialized. As a result the resp.sz
> >> field will be also uninitialized in the prh_co_entry() function.
> >> The second part is to send the response by calling the
> >> prh_write_response() routine:
> >>   1. For the PERSISTENT_RESERVE_IN command, and
> >>   2. resp->result == GOOD (previous successful reply or just luck), then
> >> There is a probability that the following assert will not be trigered:
> >>   assert(resp->sz <= req->sz && resp->sz <= sizeof(client->data));
> >> As a result some uninitialized response will be sent.
> >>
> >> The fix is to initialize the response structure to CHECK_CONDITION and 0
> >> values before calling the prh_read_request() routine.
> 
> The actual bug is that the "if (sz > 0)" should apply only to 
> PERSISTENT_RESERVE_OUT, and in fact it can be done in do_pr_out.  
> PERSISTENT_RESERVE_IN with sz == 0 is weird but okay.
> 
> This simplifies the code a bit too, because we can handle closing the 
> file descriptor in prh_co_entry.
> 
> Does something like this work for you?

Thanks for the feedback. Yes, this will work for me. Should i update the
patch and resend it or you will just pick the version you suggested?

Regards, Dima.

> 
> diff --git a/scsi/qemu-pr-helper.c b/scsi/qemu-pr-helper.c
> index 0218d65bbf..c89a446a45 100644
> --- a/scsi/qemu-pr-helper.c
> +++ b/scsi/qemu-pr-helper.c
> @@ -455,6 +455,14 @@ static int multipath_pr_out(int fd, const uint8_t *cdb, 
> uint8_t *sense,
>  char transportids[PR_HELPER_DATA_SIZE];
>  int r;
>  
> +if (sz < PR_OUT_FIXED_PARAM_SIZE) {
> +/* Illegal request, Parameter list length error.  This isn't fatal;
> + * we have read the data, send an error without closing the socket.
> + */
> +scsi_build_sense(sense, SENSE_CODE(INVALID_PARAM_LEN));
> +return CHECK_CONDITION;
> +}
> +
>  switch (rq_servact) {
>  case MPATH_PROUT_REG_SA:
>  case MPATH_PROUT_RES_SA:
> @@ -574,6 +582,12 @@ static int do_pr_out(int fd, const uint8_t *cdb, uint8_t 
> *sense,
>   const uint8_t *param, int sz)
>  {
>  int resp_sz;
> +
> +if ((fcntl(fd, F_GETFL) & O_ACCMODE) == O_RDONLY) {
> +scsi_build_sense(sense, SENSE_CODE(INVALID_OPCODE));
> +return CHECK_CONDITION;
> +}
> +
>  #ifdef CONFIG_MPATH
>  if (is_mpath(fd)) {
>  return multipath_pr_out(fd, cdb, sense, param, sz);
> @@ -690,21 +704,6 @@ static int coroutine_fn prh_read_request(PRHelperClient 
> *client,
>   errp) < 0) {
>  goto out_close;
>  }
> -if ((fcntl(client->fd, F_GETFL) & O_ACCMODE) == O_RDONLY) {
> -scsi_build_sense(resp->sense, SENSE_CODE(INVALID_OPCODE));
> -sz = 0;
> -} else if (sz < PR_OUT_FIXED_PARAM_SIZE) {
> -/* Illegal request, Parameter list length error.  This isn't 
> fatal;
> - * we have read the data, send an error without closing the 
> socket.
> - */
> -scsi_build_sense(resp->sense, SENSE_CODE(INVALID_PARAM_LEN));
> -sz = 0;
> -}
> -if (sz == 0) {
> -resp->result = CHECK_CONDITION;
> -close(client->fd);
> -client->fd = -1;
> -}
>  }
>  
>  req->fd = client->fd;
> @@ -785,25 +784,23 @@ static void coroutine_fn prh_co_entry(void *opaque)
>  break;
>  }
>  
> -if (sz > 0) {
> -num_active_sockets++;
> -if (req.cdb[0] == PERSISTENT_RESERVE_OUT) {
> -r = do_pr_out(req.fd, req.cdb, resp.sense,
> -  client->data, sz);
> -resp.sz = 0;
> -} else {
> -resp.sz = sizeof(client->data);
> -r = do_pr_in(req.fd, req.cdb, resp.sense,
> - client->data, &resp.sz);
> -resp.sz = MIN(resp.sz, sz);
> -}
> -num_active_sockets--;
> -close(req.fd);
> -if (r == -1) {
> -break;
> -}
> -resp.result = r;
> +num_active_sockets++;
> +if (req.cdb[0] == PERSISTENT_RESERVE_OUT) {
> +r = do_pr_out(req.fd, req.cdb, resp.sense,
> +  client->data, sz);
> +resp.sz = 0;
> +} else {
> +resp.sz = sizeof(client->data);
> +r = do_pr_in(req.fd, req.cdb, resp.sense,
> + client->data, &resp.sz);
> +r

[Qemu-devel] [PULL 0/1] Ui 20180703 patches

[Gerd Hoffmann]

The following changes since commit ab08440a4ee09032d1a9cb22fdcab23bc7e1c656:

  Merge remote-tracking branch 'remotes/rth/tags/pull-tcg-20180702' into 
staging (2018-07-02 17:57:46 +0100)

are available in the git repository at:

  git://git.kraxel.org/qemu tags/ui-20180703-pull-request

for you to fetch changes up to 4939a1df7d60b4a24458544557ae262852e28567:

  ui: do not build-depend or link with libdrm, it is not needed (2018-07-03 
10:30:25 +0200)


ui: drop libdrm dependency.



Michael Tokarev (1):
  ui: do not build-depend or link with libdrm, it is not needed

 configure | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

-- 
2.9.3

Re: [Qemu-devel] [RFC v2] arm: Add NRF51 SOC non-volatile memory controller

[Stefan Hajnoczi]

On Mon, Jul 02, 2018 at 01:18:35PM +0100, Peter Maydell wrote:
> On 27 June 2018 at 10:44, Stefan Hajnoczi  wrote:
> > On Tue, Jun 26, 2018 at 11:32:04AM +0200, Steffen Görtz wrote:
> >> Changes since V1:
> >> - Code style changes
> >
> > Please put the changelog below '---'.
> >
> >> diff --git a/hw/nvram/nrf51_nvmc.c b/hw/nvram/nrf51_nvmc.c
> >> new file mode 100644
> >> index 00..5dde3088a8
> >> --- /dev/null
> >> +++ b/hw/nvram/nrf51_nvmc.c
> >> @@ -0,0 +1,168 @@
> >> +/*
> >> + * nrf51_nvmc.c
> >> + *
> >> + * Copyright 2018 Steffen Görtz 
> >> + *
> >> + * This code is licensed under the GPL version 2 or later.  See
> >> + * the COPYING file in the top-level directory.
> >> + */
> >
> > Please mention the full name of the peripheral so its purpose is clear
> > and include a link to the datasheet.
> >
> > It's convenient to have this information in the .c file, since that's
> > where the majority of code changes happen.
> >
> >> diff --git a/include/hw/nvram/nrf51_nvmc.h b/include/hw/nvram/nrf51_nvmc.h
> >> new file mode 100644
> >> index 00..3a63b7e5ad
> >> --- /dev/null
> >> +++ b/include/hw/nvram/nrf51_nvmc.h
> >> @@ -0,0 +1,51 @@
> >> +/*
> >> + * nrf51_nvmc.h
> >> + *
> >> + * Copyright 2018 Steffen Görtz 
> >> + *
> >> + * This code is licensed under the GPL version 2 or later.  See
> >> + * the COPYING file in the top-level directory.
> >> + *
> >> + * See Nrf51 reference manual 6 Non-Volatile Memory Controller (NVMC)
> >> + * See Nrf51 product sheet 8.22 NVMC specifications
> >> + *
> >> + * QEMU interface:
> >> + * + sysbus MMIO regions 0: Memory Region with registers
> >> + *   to be mapped to the peripherals instance address by the SOC.
> >> + * + page_size property to set the page size in bytes.
> >> + * + code_size property to set the code size in number of pages.
> >> + *
> >> + * Accuracy of the peripheral model:
> >> + * + The NVMC is always ready, all requested erase operations succeed
> >> + *   immediately.
> >> + * + CONFIG.WEN and CONFIG.EEN flags can be written and read back
> >> + *   but are not evaluated to check whether a requested write/erase 
> >> operation
> >> + *   is legal.
> >
> > Checking CONFIG.EEN would be easy, please do it.
> >
> > Peter: CONFIG.WEN determines whether stores to the flash memory region
> > are allowed.  What is the best way to implement this protection?
> 
> What accesses do they gate? If this is just accesses that go
> via the io_read() and io_write() functions then this is easy:
> just put a check in those functions in the appropriate place.
> The harder case is if they affect accesses to a region that
> is implemented as an MMIO ram region which can be executed from;
> that can be done, but gets annoyingly complicated:
>  * unmap the RAM and map an MMIO region which handles the errors
>(only possible if the config bit gates the reads as well)
>  * use a ROM device memory region (only possible if reads are
>always ok but writes might not be
>  * use the IOMMU APIs (maximum flexibility, maximum pain)
> 
> A quick scan of the code suggests you're in the easy case
> where you can just have io_read() and io_write() handle
> the config switch checks, though.

Thanks!

> >> +MemoryRegion *mr;
> >> +AddressSpace as;
> >
> > I remember you said you tried
> > address_space_read/write(get_system_memory(), ...) but it didn't work.
> > Did you figure out why?
> 
> Devices directly talking to get_system_memory() isn't a great
> design anyway.
> 
> It's not clear to me what this device is doing with its
> AddressSpace, though; comments that went into more detail
> about what the device is and what the "memory" property is
> for might help.

I understand this issue now.  It's the same as qtest.

This device is only visible from cpu->as, it's not visible from
address_space_memory (system_memory).

The NVMC needs access to the SoC's flash memory region, and that's what
mr/as achieve here.  I agree that comments explaining the purpose of
mr/as would be useful.


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH v1] qemu-pr-helper: garbage response structure can be used to write data

[Paolo Bonzini]

On 03/07/2018 11:27, Dima Stepanov wrote:
> On Mon, Jul 02, 2018 at 02:21:41PM +0200, Paolo Bonzini wrote:
>> On 02/07/2018 10:52, Dima Stepanov wrote:
>>> Ping.
>>>
>>> On Fri, Jun 15, 2018 at 12:11:44PM +0300, Dima Stepanov wrote:
 The prh_co_entry() routine handles requests. The first part is to read a
 request by calling the prh_read_request() routine, if:
   1. scsi_cdb_xfer(req->cdb) call returns 0, and
   2. req->cdb[0] == PERSISTENT_RESERVE_IN, then
 The resp->result field will be uninitialized. As a result the resp.sz
 field will be also uninitialized in the prh_co_entry() function.
 The second part is to send the response by calling the
 prh_write_response() routine:
   1. For the PERSISTENT_RESERVE_IN command, and
   2. resp->result == GOOD (previous successful reply or just luck), then
 There is a probability that the following assert will not be trigered:
   assert(resp->sz <= req->sz && resp->sz <= sizeof(client->data));
 As a result some uninitialized response will be sent.

 The fix is to initialize the response structure to CHECK_CONDITION and 0
 values before calling the prh_read_request() routine.
>>
>> The actual bug is that the "if (sz > 0)" should apply only to 
>> PERSISTENT_RESERVE_OUT, and in fact it can be done in do_pr_out.  
>> PERSISTENT_RESERVE_IN with sz == 0 is weird but okay.
>>
>> This simplifies the code a bit too, because we can handle closing the 
>> file descriptor in prh_co_entry.
>>
>> Does something like this work for you?
> 
> Thanks for the feedback. Yes, this will work for me. Should i update the
> patch and resend it or you will just pick the version you suggested?

I will pick it, thanks!

Paolo

[Qemu-devel] [Bug 1779634] Re: qemu-x86_64 on aarch64 reports "Synchronous External Abort"

[He Yi]

thanks Peter, yes I tried to run an x86 strace under QEMU.

I'll stop this experiment since you are right this won't work for
utilities with device-level I/O and memory operations, I will raise this
requirement to Intel support website firstly.

Best Regards, Yi

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

Title:
  qemu-x86_64 on aarch64 reports "Synchronous External Abort"

Status in QEMU:
  New

Bug description:
  Purpose: to run x86_64 utilities on aarch64 platform (Intel/Dell network 
adapters' firmware upgrade tools)
  System: aarch64 server platform, with ubuntu 16.04 (xenial) Linux 
4.13.0-45-generic #50~16.04.1-Ubuntu SMP Wed May 30 11:14:25 UTC 2018 aarch64 
aarch64 aarch64 GNU/Linux

  Reproduce:
  1) build linux-user qemu-x86_64 static from source (tried both version 2.12.0 
& 2.11.2)
     ./configure --target-list=x86_64-linux-user --disable-system --static 
--enable-linux-user

  2) install the interpreter into binfmt_misc filesystem
     $ cat /proc/sys/fs/binfmt_misc/qemu-x86_64
   enabled
   interpreter /usr/local/bin/qemu-x86_64
   flags:
   offset 0
   magic 7f454c460201010002003e00
   mask fffefefcfeff

  3) packaging Intel/Dell upgrade utilities into docker images, I've published 
two on docker hub:
     REPOSITORY  TAG
     heyi/dellupdate latest
     heyi/nvmupdate64e   latest

  4) run the docker container on aarch64 server platform:
     docker run -it --privileged --network host --volume 
/usr/local/bin/qemu-x86_64:/usr/local/bin/qemu-x86_64 heyi/dellupdate:latest

  5) finally, within docker container run the upgrade tool:
     # ./Network_Firmware_T6VN9_LN_18.5.17_A00.BIN

  Errors: in dmesg it reports excessive 'Synchronous External Abort':

  kernel: [242850.159893] Synchronous External Abort: synchronous external 
abort (0x92000610) at 0x00429958
  kernel: [242850.169199] Unhandled fault: synchronous external abort 
(0x92000610) at 0x00429958

  thanks and best regards, Yi

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

[Qemu-devel] [PULL 1/4] virtio-gpu: tweak scanout disable.

[Gerd Hoffmann]

- Factor out the code to virtio_gpu_disable_scanout().
- Allow disable scanout 0, show a message then.
- Clear scanout->resource_id.

Signed-off-by: Gerd Hoffmann 
Reviewed-by: Marc-André Lureau 
Message-id: 20180702162443.16796-2-kra...@redhat.com
---
 hw/display/virtio-gpu.c | 47 +--
 1 file changed, 29 insertions(+), 18 deletions(-)

diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index 2dd3c3481a..054ec73c0a 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -399,6 +399,34 @@ static void virtio_gpu_resource_create_2d(VirtIOGPU *g,
 g->hostmem += res->hostmem;
 }
 
+static void virtio_gpu_disable_scanout(VirtIOGPU *g, int scanout_id)
+{
+struct virtio_gpu_scanout *scanout = &g->scanout[scanout_id];
+struct virtio_gpu_simple_resource *res;
+DisplaySurface *ds = NULL;
+
+if (scanout->resource_id == 0) {
+return;
+}
+
+res = virtio_gpu_find_resource(g, scanout->resource_id);
+if (res) {
+res->scanout_bitmask &= ~(1 << scanout_id);
+}
+
+if (scanout_id == 0) {
+/* primary head */
+ds = qemu_create_message_surface(scanout->width  ?: 640,
+ scanout->height ?: 480,
+ "Guest disabled display.");
+}
+dpy_gfx_replace_surface(scanout->con, ds);
+scanout->resource_id = 0;
+scanout->ds = NULL;
+scanout->width = 0;
+scanout->height = 0;
+}
+
 static void virtio_gpu_resource_destroy(VirtIOGPU *g,
 struct virtio_gpu_simple_resource *res)
 {
@@ -583,24 +611,7 @@ static void virtio_gpu_set_scanout(VirtIOGPU *g,
 
 g->enable = 1;
 if (ss.resource_id == 0) {
-scanout = &g->scanout[ss.scanout_id];
-if (scanout->resource_id) {
-res = virtio_gpu_find_resource(g, scanout->resource_id);
-if (res) {
-res->scanout_bitmask &= ~(1 << ss.scanout_id);
-}
-}
-if (ss.scanout_id == 0) {
-qemu_log_mask(LOG_GUEST_ERROR,
-  "%s: illegal scanout id specified %d",
-  __func__, ss.scanout_id);
-cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_SCANOUT_ID;
-return;
-}
-dpy_gfx_replace_surface(g->scanout[ss.scanout_id].con, NULL);
-scanout->ds = NULL;
-scanout->width = 0;
-scanout->height = 0;
+virtio_gpu_disable_scanout(g, ss.scanout_id);
 return;
 }
 
-- 
2.9.3

[Qemu-devel] [PULL 2/4] virtio-gpu: update old resource too.

[Gerd Hoffmann]

When switching scanout from one resource to another we must update the
scanout_bitmask field for both new (set bit) and old (clear bit)
resource.

Signed-off-by: Gerd Hoffmann 
Reviewed-by: Marc-André Lureau 
Message-id: 20180702162443.16796-3-kra...@redhat.com
---
 hw/display/virtio-gpu.c | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index 054ec73c0a..336dc59007 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -590,7 +590,7 @@ static void virtio_unref_resource(pixman_image_t *image, 
void *data)
 static void virtio_gpu_set_scanout(VirtIOGPU *g,
struct virtio_gpu_ctrl_command *cmd)
 {
-struct virtio_gpu_simple_resource *res;
+struct virtio_gpu_simple_resource *res, *ores;
 struct virtio_gpu_scanout *scanout;
 pixman_format_code_t format;
 uint32_t offset;
@@ -664,6 +664,11 @@ static void virtio_gpu_set_scanout(VirtIOGPU *g,
 dpy_gfx_replace_surface(g->scanout[ss.scanout_id].con, scanout->ds);
 }
 
+ores = virtio_gpu_find_resource(g, scanout->resource_id);
+if (ores) {
+ores->scanout_bitmask &= ~(1 << ss.scanout_id);
+}
+
 res->scanout_bitmask |= (1 << ss.scanout_id);
 scanout->resource_id = ss.resource_id;
 scanout->x = ss.r.x;
-- 
2.9.3

[Qemu-devel] [PULL 3/4] virtio-gpu: disable scanout when backing resource is destroyed

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann 
Reviewed-by: Marc-André Lureau 
Message-id: 20180702162443.16796-4-kra...@redhat.com
---
 hw/display/virtio-gpu.c | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index 336dc59007..08cd567218 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -430,6 +430,16 @@ static void virtio_gpu_disable_scanout(VirtIOGPU *g, int 
scanout_id)
 static void virtio_gpu_resource_destroy(VirtIOGPU *g,
 struct virtio_gpu_simple_resource *res)
 {
+int i;
+
+if (res->scanout_bitmask) {
+for (i = 0; i < g->conf.max_outputs; i++) {
+if (res->scanout_bitmask & (1 << i)) {
+virtio_gpu_disable_scanout(g, i);
+}
+}
+}
+
 pixman_image_unref(res->image);
 virtio_gpu_cleanup_mapping(res);
 QTAILQ_REMOVE(&g->reslist, res, next);
-- 
2.9.3

[Qemu-devel] [PULL 0/4] Vga 20180703 patches

[Gerd Hoffmann]

The following changes since commit ab08440a4ee09032d1a9cb22fdcab23bc7e1c656:

  Merge remote-tracking branch 'remotes/rth/tags/pull-tcg-20180702' into 
staging (2018-07-02 17:57:46 +0100)

are available in the git repository at:

  git://git.kraxel.org/qemu tags/vga-20180703-pull-request

for you to fetch changes up to 1fcfdc435a3e25ab9037f6f7b8ab4bdf89ba1269:

  vga: disable global_vmstate for 3.0+ machine types (2018-07-03 11:19:49 +0200)


vga: disable global_vmstate, virtio-gpu scanout tracking fixes.



Gerd Hoffmann (4):
  virtio-gpu: tweak scanout disable.
  virtio-gpu: update old resource too.
  virtio-gpu: disable scanout when backing resource is destroyed
  vga: disable global_vmstate for 3.0+ machine types

 hw/display/vga_int.h|  3 ++-
 include/hw/compat.h | 16 +
 hw/display/cirrus_vga.c |  9 ---
 hw/display/qxl.c|  3 ++-
 hw/display/vga-isa-mm.c |  3 ++-
 hw/display/vga-isa.c|  3 ++-
 hw/display/vga-pci.c|  5 ++--
 hw/display/vga.c|  4 ++--
 hw/display/virtio-gpu.c | 64 ++---
 hw/display/virtio-vga.c |  2 +-
 hw/display/vmware_vga.c |  4 +++-
 11 files changed, 84 insertions(+), 32 deletions(-)

-- 
2.9.3

Re: [Qemu-devel] [RFC v2] arm: Add NRF51 SOC non-volatile memory controller

[Peter Maydell]

On 3 July 2018 at 10:31, Stefan Hajnoczi  wrote:
> On Mon, Jul 02, 2018 at 01:18:35PM +0100, Peter Maydell wrote:
>> It's not clear to me what this device is doing with its
>> AddressSpace, though; comments that went into more detail
>> about what the device is and what the "memory" property is
>> for might help.
>
> I understand this issue now.  It's the same as qtest.
>
> This device is only visible from cpu->as, it's not visible from
> address_space_memory (system_memory).
>
> The NVMC needs access to the SoC's flash memory region, and that's what
> mr/as achieve here.  I agree that comments explaining the purpose of
> mr/as would be useful.

Is the flash memory region only accessible to the CPU
via the NVMC, or can the CPU get at it both directly
and via the NVMC? (That is, in hardware, does the flash
sit "behind" the NVMC?)

thanks
-- PMM

[Qemu-devel] [PULL 4/4] vga: disable global_vmstate for 3.0+ machine types

[Gerd Hoffmann]

Move global_vmstate from vga_common_init() parameter to VGACommonState
field.  Set global_vmstate to true for isa vga devices, so nothing
changes here.  virtio-vga and secondary-vga already set global_vmstate
to false so no change here either.  All other pci vga devices get a new
global-vmstate property, defaulting to false.  A compat property flips
it to true for older machine types.

With this in place you don't get a vmstate section naming conflict any
more when adding multiple pci vga devices to your vm.

Signed-off-by: Gerd Hoffmann 
Message-Id: <20180702163345.17892-1-kra...@redhat.com>
---
 hw/display/vga_int.h|  3 ++-
 include/hw/compat.h | 16 
 hw/display/cirrus_vga.c |  9 ++---
 hw/display/qxl.c|  3 ++-
 hw/display/vga-isa-mm.c |  3 ++-
 hw/display/vga-isa.c|  3 ++-
 hw/display/vga-pci.c|  5 +++--
 hw/display/vga.c|  4 ++--
 hw/display/virtio-vga.c |  2 +-
 hw/display/vmware_vga.c |  4 +++-
 10 files changed, 39 insertions(+), 13 deletions(-)

diff --git a/hw/display/vga_int.h b/hw/display/vga_int.h
index f8fcf62a56..339661bc01 100644
--- a/hw/display/vga_int.h
+++ b/hw/display/vga_int.h
@@ -133,6 +133,7 @@ typedef struct VGACommonState {
 bool full_update_gfx;
 bool big_endian_fb;
 bool default_endian_fb;
+bool global_vmstate;
 /* hardware mouse cursor support */
 uint32_t invalidated_y_table[VGA_MAX_HEIGHT / 32];
 uint32_t hw_cursor_x;
@@ -157,7 +158,7 @@ static inline int c6_to_8(int v)
 return (v << 2) | (b << 1) | b;
 }
 
-void vga_common_init(VGACommonState *s, Object *obj, bool global_vmstate);
+void vga_common_init(VGACommonState *s, Object *obj);
 void vga_init(VGACommonState *s, Object *obj, MemoryRegion *address_space,
   MemoryRegion *address_space_io, bool init_vga_ports);
 MemoryRegion *vga_init_io(VGACommonState *s, Object *obj,
diff --git a/include/hw/compat.h b/include/hw/compat.h
index 44d5964060..c08f4040bb 100644
--- a/include/hw/compat.h
+++ b/include/hw/compat.h
@@ -10,6 +10,22 @@
 .driver   = "hda-audio",\
 .property = "use-timer",\
 .value= "false",\
+},{\
+.driver   = "cirrus-vga",\
+.property = "global-vmstate",\
+.value= "true",\
+},{\
+.driver   = "VGA",\
+.property = "global-vmstate",\
+.value= "true",\
+},{\
+.driver   = "vmware-svga",\
+.property = "global-vmstate",\
+.value= "true",\
+},{\
+.driver   = "qxl-vga",\
+.property = "global-vmstate",\
+.value= "true",\
 },
 
 #define HW_COMPAT_2_11 \
diff --git a/hw/display/cirrus_vga.c b/hw/display/cirrus_vga.c
index 138ae961b9..1d050621f1 100644
--- a/hw/display/cirrus_vga.c
+++ b/hw/display/cirrus_vga.c
@@ -3048,7 +3048,8 @@ static void isa_cirrus_vga_realizefn(DeviceState *dev, 
Error **errp)
s->vram_size_mb);
 return;
 }
-vga_common_init(s, OBJECT(dev), true);
+s->global_vmstate = true;
+vga_common_init(s, OBJECT(dev));
 cirrus_init_common(&d->cirrus_vga, OBJECT(dev), CIRRUS_ID_CLGD5430, 0,
isa_address_space(isadev),
isa_address_space_io(isadev));
@@ -3062,7 +3063,7 @@ static Property isa_cirrus_vga_properties[] = {
 DEFINE_PROP_UINT32("vgamem_mb", struct ISACirrusVGAState,
cirrus_vga.vga.vram_size_mb, 4),
 DEFINE_PROP_BOOL("blitter", struct ISACirrusVGAState,
-   cirrus_vga.enable_blitter, true),
+ cirrus_vga.enable_blitter, true),
 DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -3105,7 +3106,7 @@ static void pci_cirrus_vga_realize(PCIDevice *dev, Error 
**errp)
  return;
  }
  /* setup VGA */
- vga_common_init(&s->vga, OBJECT(dev), true);
+ vga_common_init(&s->vga, OBJECT(dev));
  cirrus_init_common(s, OBJECT(dev), device_id, 1, pci_address_space(dev),
 pci_address_space_io(dev));
  s->vga.con = graphic_console_init(DEVICE(dev), 0, s->vga.hw_ops, &s->vga);
@@ -3134,6 +3135,8 @@ static Property pci_vga_cirrus_properties[] = {
cirrus_vga.vga.vram_size_mb, 4),
 DEFINE_PROP_BOOL("blitter", struct PCICirrusVGAState,
  cirrus_vga.enable_blitter, true),
+DEFINE_PROP_BOOL("global-vmstate", struct PCICirrusVGAState,
+ cirrus_vga.vga.global_vmstate, false),
 DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index a71714ccb4..3f740d7aa3 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -2168,7 +2168,7 @@ static void qxl_realize_primary(PCIDevice *dev, Error 
**errp)
 qxl_init_ramsize(qxl);
 vga->vbe_size = qxl->vgamem_size;
 vga->vram_size_mb = qxl->vga.vram_size >> 20;
-vga_common_init(vga, OBJECT(dev), true);
+vga_common_init(vga, OBJECT(dev));
 vga_init(vga, OBJECT(dev),
  pci_address_space(dev), pci_address_sp

Re: [Qemu-devel] [PATCH] fsdev: fix compilation with VIRTIO but not VIRTIO_9P

[Greg Kurz]

On Tue, 3 Jul 2018 09:46:01 +0100
Peter Maydell  wrote:

> On 3 July 2018 at 09:00, Greg Kurz  wrote:
> > On Mon,  2 Jul 2018 18:31:25 +0200
> > Paolo Bonzini  wrote:
> >  
> >> hw/9pfs/Makefile.objs uses CONFIG_VIRTIO_9P to guard the definition for
> >> FileOperations structs, while fsdev/Makefile.objs uses CONFIG_VIRTIO
> >> to guard the use.  Mismatch causes linking to fail when CONFIG_VIRTIO
> >> is set but CONFIG_VIRTIO_9P is not.
> >>
> >> Fix it and use if/else to clarify that the two lines are for opposite
> >> conditions.
> >>
> >> Reported-by: Peter Maydell 
> >> Fixes: b5dfdb082fc350f3e68dfa61dc988d97cad28cfe
> >> Signed-off-by: Paolo Bonzini 
> >> ---  
> >
> > Acked-by: Greg Kurz 
> > Tested-by: Greg Kurz 
> >
> > Peter,
> >
> > I see this patch is needed for the latest RISC-V pull req.  
> 
> It isn't, because that pullreq only fails because of what
> looks to me like a rebase mismerge that made it change how
> the riscv default-configs enable virtio. The riscv pullreq
> should be fixed. This is just a cleanup, really.
> 

Ok, thanks for the clarification.

I've applied this to 9p-next.

> thanks
> -- PMM

Re: [Qemu-devel] [PATCH v2 1/2] nbd/client: Add x-dirty-bitmap to query bitmap from server

[Vladimir Sementsov-Ogievskiy]

02.07.2018 22:14, Eric Blake wrote:

In order to test that the NBD server is properly advertising
dirty bitmaps, we need a bare minimum client that can request
and read the context.  Since feature freeze for 3.0 is imminent,
this is the smallest workable patch, which replaces the qemu
block status report with the results of the NBD server's dirty
bitmap (making it very easy to use 'qemu-img map --output=json'
to learn where the dirty portions are).  Note that the NBD
protocol defines a dirty section with the same bit but opposite
sense that normal "base:allocation" uses to report an allocated
section; so in qemu-img map output, "data":true corresponds to
clean, "data":false corresponds to dirty.

A more complete solution that allows dirty bitmaps to be queried
at the same time as normal block status will be required before
this addition can lose the x- prefix.  Until then, the fact that
this replaces normal status with dirty status means actions
like 'qemu-img convert' will likely misbehave due to treating
dirty regions of the file as if they are unallocated.

The next patch adds an iotest to exercise this new code.

Signed-off-by: Eric Blake 
---

  qapi/block-core.json |  7 ++-
  block/nbd-client.h   |  1 +
  include/block/nbd.h  |  1 +
  block/nbd-client.c   |  3 +++
  block/nbd.c  | 10 --
  nbd/client.c |  4 ++--
  6 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 577ce5e9991..90e554ed0ff 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3471,12 +3471,17 @@
  #
  # @tls-creds:   TLS credentials ID
  #
+# @x-dirty-bitmap: A "qemu:dirty-bitmap:NAME" string to query in place of
+#  traditional "base:allocation" block status (see
+#  NBD_OPT_LIST_META_CONTEXT in the NBD protocol) (since 3.0)
+#


"x-dirty-bitmap=qemu:dirty-bitmap:NAME", is a bit strange, looks like it 
should be "x-dirty-bitmap=NAME", and "qemu:dirty-bitmap" added 
automatically. (and you don't check it, so actually this parameter is 
x-meta-context, and user can select any context, for example, 
"base:allocation", so "x-meta-context=qemu:dirty-bitmap:NAME" sounds 
better too). But I'm ok to leave it as is for now, with x- prefix.




  # Since: 2.9
  ##
  { 'struct': 'BlockdevOptionsNbd',
'data': { 'server': 'SocketAddress',
  '*export': 'str',
-'*tls-creds': 'str' } }
+'*tls-creds': 'str',
+'*x-dirty-bitmap': 'str' } }

  ##
  # @BlockdevOptionsRaw:
diff --git a/block/nbd-client.h b/block/nbd-client.h
index 0ece76e5aff..cfc90550b99 100644
--- a/block/nbd-client.h
+++ b/block/nbd-client.h
@@ -45,6 +45,7 @@ int nbd_client_init(BlockDriverState *bs,
  const char *export_name,
  QCryptoTLSCreds *tlscreds,
  const char *hostname,
+const char *x_dirty_bitmap,
  Error **errp);
  void nbd_client_close(BlockDriverState *bs);

diff --git a/include/block/nbd.h b/include/block/nbd.h
index daaeae61bf9..4638c839f51 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -259,6 +259,7 @@ static inline bool nbd_reply_type_is_error(int type)
  struct NBDExportInfo {
  /* Set by client before nbd_receive_negotiate() */
  bool request_sizes;
+char *x_dirty_bitmap;

  /* In-out fields, set by client before nbd_receive_negotiate() and
   * updated by server results during nbd_receive_negotiate() */
diff --git a/block/nbd-client.c b/block/nbd-client.c
index 8d69eaaa32f..9686ecbd5ee 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -970,6 +970,7 @@ int nbd_client_init(BlockDriverState *bs,
  const char *export,
  QCryptoTLSCreds *tlscreds,
  const char *hostname,
+const char *x_dirty_bitmap,
  Error **errp)
  {
  NBDClientSession *client = nbd_get_client_session(bs);
@@ -982,9 +983,11 @@ int nbd_client_init(BlockDriverState *bs,
  client->info.request_sizes = true;
  client->info.structured_reply = true;
  client->info.base_allocation = true;
+client->info.x_dirty_bitmap = g_strdup(x_dirty_bitmap);
  ret = nbd_receive_negotiate(QIO_CHANNEL(sioc), export,
  tlscreds, hostname,
  &client->ioc, &client->info, errp);
+g_free(client->info.x_dirty_bitmap);


hm, pointer remains invalid. If you want free it here, what is the 
reason to strdup it? At least, it worth to zero out the pointer after 
g_free.. Or, free it not here but in client close.


with pointer zeroed or g_free to client destroy procedure,

Reviewed-by: Vladimir Sementsov-Ogievskiy 



--
Best regards,
Vladimir

Re: [Qemu-devel] [PULL 0/3] pc, virtio: fixes

[Peter Maydell]

On 3 July 2018 at 00:48, Michael S. Tsirkin  wrote:
> The following changes since commit 00928a421d47f49691cace1207481b7aad31b1f1:
>
>   Merge remote-tracking branch 
> 'remotes/pmaydell/tags/pull-target-arm-20180626' into staging (2018-06-26 
> 18:23:49 +0100)
>
> are available in the Git repository at:
>
>   git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git tags/for_upstream
>
> for you to fetch changes up to 5d9c9ea22ab4f3b3ee497523e34b6f4d3281f62d:
>
>   virtio-rng: process pending requests on DRIVER_OK (2018-06-28 04:46:16 
> +0300)
>
> 
> pc, virtio: fixes
>
> A couple of fixes to amd iommu, and a fix to virtio iommu.
>
> Signed-off-by: Michael S. Tsirkin 
>
Applied, thanks.

-- PMM

[Qemu-devel] [PATCH] pr-helper: Rework socket path handling

[Michal Privoznik]

When reviewing Paolo's pr-helper patches I've noticed couple of
problems:

1) socket_path needs to be calculated at two different places
(one for printing out help, the other if socket activation is NOT
used),

2) even though the default socket_path is allocated in
compute_default_paths() it is the only default path the function
handles. For instance, pidfile is allocated outside of this
function. And yet again, at different places than 1)

Signed-off-by: Michal Privoznik 
---
 scsi/qemu-pr-helper.c | 36 ++--
 1 file changed, 10 insertions(+), 26 deletions(-)

diff --git a/scsi/qemu-pr-helper.c b/scsi/qemu-pr-helper.c
index 0218d65bbf..59df3fd608 100644
--- a/scsi/qemu-pr-helper.c
+++ b/scsi/qemu-pr-helper.c
@@ -76,14 +76,12 @@ static int gid = -1;
 
 static void compute_default_paths(void)
 {
-if (!socket_path) {
-socket_path = qemu_get_local_state_pathname("run/qemu-pr-helper.sock");
-}
+socket_path = qemu_get_local_state_pathname("run/qemu-pr-helper.sock");
+pidfile = qemu_get_local_state_pathname("run/qemu-pr-helper.pid");
 }
 
 static void usage(const char *name)
 {
-compute_default_paths();
 (printf) (
 "Usage: %s [OPTIONS] FILE\n"
 "Persistent Reservation helper program for QEMU\n"
@@ -844,19 +842,6 @@ static gboolean accept_client(QIOChannel *ioc, 
GIOCondition cond, gpointer opaqu
 return TRUE;
 }
 
-
-/*
- * Check socket parameters compatibility when socket activation is used.
- */
-static const char *socket_activation_validate_opts(void)
-{
-if (socket_path != NULL) {
-return "Unix socket can't be set when using socket activation";
-}
-
-return NULL;
-}
-
 static void termsig_handler(int signum)
 {
 atomic_cmpxchg(&state, RUNNING, TERMINATE);
@@ -930,6 +915,7 @@ int main(int argc, char **argv)
 char *trace_file = NULL;
 bool daemonize = false;
 bool pidfile_specified = false;
+bool socket_path_specified = false;
 unsigned socket_activation;
 
 struct sigaction sa_sigterm;
@@ -946,12 +932,14 @@ int main(int argc, char **argv)
 qemu_add_opts(&qemu_trace_opts);
 qemu_init_exec_dir(argv[0]);
 
-pidfile = qemu_get_local_state_pathname("run/qemu-pr-helper.pid");
+compute_default_paths();
 
 while ((ch = getopt_long(argc, argv, sopt, lopt, &opt_ind)) != -1) {
 switch (ch) {
 case 'k':
-socket_path = optarg;
+g_free(socket_path);
+socket_path = g_strdup(optarg);
+socket_path_specified = true;
 if (socket_path[0] != '/') {
 error_report("socket path must be absolute");
 exit(EXIT_FAILURE);
@@ -1042,10 +1030,9 @@ int main(int argc, char **argv)
 socket_activation = check_socket_activation();
 if (socket_activation == 0) {
 SocketAddress saddr;
-compute_default_paths();
 saddr = (SocketAddress){
 .type = SOCKET_ADDRESS_TYPE_UNIX,
-.u.q_unix.path = g_strdup(socket_path)
+.u.q_unix.path = socket_path,
 };
 server_ioc = qio_channel_socket_new();
 if (qio_channel_socket_listen_sync(server_ioc, &saddr, &local_err) < 
0) {
@@ -1053,12 +1040,10 @@ int main(int argc, char **argv)
 error_report_err(local_err);
 return 1;
 }
-g_free(saddr.u.q_unix.path);
 } else {
 /* Using socket activation - check user didn't use -p etc. */
-const char *err_msg = socket_activation_validate_opts();
-if (err_msg != NULL) {
-error_report("%s", err_msg);
+if (socket_path_specified) {
+error_report("Unix socket can't be set when using socket 
activation");
 exit(EXIT_FAILURE);
 }
 
@@ -1075,7 +1060,6 @@ int main(int argc, char **argv)
  error_get_pretty(local_err));
 exit(EXIT_FAILURE);
 }
-socket_path = NULL;
 }
 
 if (qemu_init_main_loop(&local_err)) {
-- 
2.16.4

Re: [Qemu-devel] [PATCH v3 0/3] Use of unique identifier for pairing virtio and passthrough devices...

[Roman Kagan]

On Mon, Jul 02, 2018 at 02:14:52PM -0700, si-wei liu wrote:
> On 7/2/2018 9:14 AM, Roman Kagan wrote:
> > On Fri, Jun 29, 2018 at 05:19:03PM -0500, Venu Busireddy wrote:
> > > The patch set "Enable virtio_net to act as a standby for a passthru
> > > device" [1] deals with live migration of guests that use passthrough
> > > devices. However, that scheme uses the MAC address for pairing
> > > the virtio device and the passthrough device. The thread "netvsc:
> > > refactor notifier/event handling code to use the failover framework"
> > > [2] discusses an alternate mechanism, such as using an UUID, for pairing
> > > the devices. Based on that discussion, proposals "Add "Group Identifier"
> > > to virtio PCI capabilities." [3] and "RFC: Use of bridge devices to
> > > store pairing information..." [4] were made.
> > > 
> > > The current patch set includes all the feedback received for proposals [3]
> > > and [4]. For the sake of completeness, patch for the virtio specification
> > > is also included here. Following is the updated proposal.
> > > 
> > > 1. Extend the virtio specification to include a new virtio PCI capability
> > > "VIRTIO_PCI_CAP_GROUP_ID_CFG".
> > > 
> > > 2. Enhance the QEMU CLI to include a "failover-group-id" option to the
> > > virtio device. The "failover-group-id" is a 64 bit value.
> > > 
> > > 3. Enhance the QEMU CLI to include a "failover-group-id" option to the
> > > Red Hat PCI bridge device (support for the i440FX model).
> > > 
> > > 4. Add a new "pcie-downstream" device, with the option
> > > "failover-group-id" (support for the Q35 model).
> > > 
> > > 5. The operator creates a 64 bit unique identifier, failover-group-id.
> > > 
> > > 6. When the virtio device is created, the operator uses the
> > > "failover-group-id" option (for example, '-device
> > > virtio-net-pci,failover-group-id=') and specifies the
> > > failover-group-id created in step 4.
> > > 
> > > QEMU stores the failover-group-id in the virtio device's configuration
> > > space in the capability "VIRTIO_PCI_CAP_GROUP_ID_CFG".
> > > 
> > > 7. When assigning a PCI device to the guest in passthrough mode, the
> > > operator first creates a bridge using the "failover-group-id" option
> > > (for example, '-device 
> > > pcie-downstream,failover-group-id=')
> > > to specify the failover-group-id created in step 4, and then attaches
> > > the passthrough device to the bridge.
> > > 
> > > QEMU stores the failover-group-id in the configuration space of the
> > > bridge as Vendor-Specific capability (0x09). The "Vendor" here is
> > > not to be confused with a specific organization. Instead, the vendor
> > > of the bridge is QEMU.
> > > 
> > > 8. Patch 4 in patch series "Enable virtio_net to act as a standby for
> > > a passthru device" [1] needs to be modified to use the UUID values
> > > present in the bridge's configuration space and the virtio device's
> > > configuration space instead of the MAC address for pairing the 
> > > devices.
> > I'm still missing a few bits in the overall scheme.
> > 
> > Is the guest supposed to acknowledge the support for PT-PV failover?
> 
> Yes. We are leveraging virtio's feature negotiation mechanism for that.
> Guest which does not acknowledge the support will not have PT plugged in.
> 
> > Should the PT device be visibile to the guest before it acknowledges the
> > support for failover?
> No. QEMU will only expose PT device after guest acknowledges the support
> through virtio's feature negotiation.
> 
> >How is this supposed to work with legacy guests that don't support it?
> Only PV device will be exposed on legacy guest.

So how is this coordination going to work?  One possibility is that the
PV device emits a QMP event upon the guest driver confirming the support
for failover, the management layer intercepts the event and performs
device_add of the PT device.  Another is that the PT device is added
from the very beginning (e.g. on the QEMU command line) but its parent
PCI bridge subscribes a callback with the PV device to "activate" the PT
device upon negotiating the failover feature.

I think this needs to be decided within the scope of this patchset.

> > Is the guest supposed to signal the datapath switch to the host?
> No, guest doesn't need to be initiating datapath switch at all.

What happens if the guest supports failover in its PV driver, but lacks
the driver for the PT device?

> However, QMP
> events may be generated when exposing or hiding the PT device through hot
> plug/unplug to facilitate host to switch datapath.

The PT device hot plug/unplug are initiated by the host, aren't they?  Why
would it also need QMP events for them?

> > Is the scheme going to be applied/extended to other transports (vmbus,
> > virtio-ccw, etc.)?
> Well, it depends on the use case, and how feasible it can be extended to
> other transport due to constraints and transport specifics.
> 
> > Is the failover gr

Re: [Qemu-devel] [PATCH] qga: report disk size and free space

[Marc-André Lureau]

Hi

On Tue, Jul 3, 2018 at 10:31 AM, Tomáš Golembiovský  wrote:
> Report total file system size and free space in output of command
> "guest-get-fsinfo". Values are optional and it is not an error if they cannot
> be retrieved for some reason.
>

I am afraid this will miss 3.0, since it's a new feature

> Signed-off-by: Tomáš Golembiovský 
> ---
>  qga/commands-posix.c | 18 ++
>  qga/commands-win32.c | 16 
>  qga/qapi-schema.json |  5 -
>  3 files changed, 38 insertions(+), 1 deletion(-)
>
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index eae817191b..1f2fb25b91 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -13,6 +13,7 @@
>
>  #include "qemu/osdep.h"
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -1074,11 +1075,28 @@ static GuestFilesystemInfo *build_guest_fsinfo(struct 
> FsMount *mount,
>  GuestFilesystemInfo *fs = g_malloc0(sizeof(*fs));
>  char *devpath = g_strdup_printf("/sys/dev/block/%u:%u",
>  mount->devmajor, mount->devminor);
> +struct statvfs vfs_stats;
>
>  fs->mountpoint = g_strdup(mount->dirname);
>  fs->type = g_strdup(mount->devtype);
>  build_guest_fsinfo_for_device(devpath, fs, errp);
>
> +g_debug(" get filesystem statistics for '%s'", mount->dirname);
> +if (statvfs(mount->dirname, &vfs_stats) != 0) {
> +/* This is not fatal, just log this incident */
> +Error *local_err = NULL;
> +error_setg_errno(&local_err, errno, "statfs(\"%s\")",

statvfs()

> +mount->dirname);
> +slog("failed to stat filesystem: %s",
> +error_get_pretty(local_err));
> +error_free(local_err);

The usage of Error is a bit overkill, perhaps you may juste use slog()
/ strerror() directly.

> +} else {
> +fs->size = vfs_stats.f_frsize * vfs_stats.f_blocks;
> +fs->has_size = true;
> +fs->free = vfs_stats.f_frsize * vfs_stats.f_bfree;
> +fs->has_free = true;
> +}
> +
>  g_free(devpath);
>  return fs;
>  }
> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> index 70ee5379f6..6d6ca05281 100644
> --- a/qga/commands-win32.c
> +++ b/qga/commands-win32.c
> @@ -706,6 +706,22 @@ static GuestFilesystemInfo *build_guest_fsinfo(char 
> *guid, Error **errp)
>  }
>  fs->type = g_strdup(fs_name);
>  fs->disk = build_guest_disk_info(guid, errp);
> +
> +if (len > 0) {
> +if (GetDiskFreeSpaceEx(mnt_point, 0, (PULARGE_INTEGER)&fs->size,
> +(PULARGE_INTEGER)&fs->free) != 0) {

I would rather use some local ULARGE_INTEGER and access the QuadPart
to avoid the cast, but it should work.

> +/* This is not fatal, just log this incident */
> +Error *local_err = NULL;
> +error_setg_win32(&local_err, GetLastError(),
> +"failed to get free space on volume \"%s\"", mnt_point);
> +slog("%s", error_get_pretty(local_err));
> +error_free(local_err);
> +} else {
> +fs->has_size = true;
> +fs->has_free = true;
> +}
> +}
> +
>  free:
>  g_free(mnt_point);
>  return fs;
> diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
> index 17884c7c70..28a32444d3 100644
> --- a/qga/qapi-schema.json
> +++ b/qga/qapi-schema.json
> @@ -848,12 +848,15 @@
>  # @type: file system type string
>  # @disk: an array of disk hardware information that the volume lies on,
>  #which may be empty if the disk type is not supported
> +# @size: total number of bytes on the file system (Since 2.13)
> +# @free: number of bytes available on the file system (Since 2.13)

"Since 3.1", most probably

>  #
>  # Since: 2.2
>  ##
>  { 'struct': 'GuestFilesystemInfo',
>'data': {'name': 'str', 'mountpoint': 'str', 'type': 'str',
> -   'disk': ['GuestDiskAddress']} }
> +   'disk': ['GuestDiskAddress'], '*size': 'uint64',
> +   '*free': 'uint64'} }
>
>  ##
>  # @guest-get-fsinfo:
> --
> 2.17.1
>
>



-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH] pr-helper: Rework socket path handling

[Paolo Bonzini]

On 03/07/2018 11:51, Michal Privoznik wrote:
> When reviewing Paolo's pr-helper patches I've noticed couple of
> problems:
> 
> 1) socket_path needs to be calculated at two different places
> (one for printing out help, the other if socket activation is NOT
> used),
> 
> 2) even though the default socket_path is allocated in
> compute_default_paths() it is the only default path the function
> handles. For instance, pidfile is allocated outside of this
> function. And yet again, at different places than 1)
> 
> Signed-off-by: Michal Privoznik 

Queued, thanks.

Paolo

> ---
>  scsi/qemu-pr-helper.c | 36 ++--
>  1 file changed, 10 insertions(+), 26 deletions(-)
> 
> diff --git a/scsi/qemu-pr-helper.c b/scsi/qemu-pr-helper.c
> index 0218d65bbf..59df3fd608 100644
> --- a/scsi/qemu-pr-helper.c
> +++ b/scsi/qemu-pr-helper.c
> @@ -76,14 +76,12 @@ static int gid = -1;
>  
>  static void compute_default_paths(void)
>  {
> -if (!socket_path) {
> -socket_path = 
> qemu_get_local_state_pathname("run/qemu-pr-helper.sock");
> -}
> +socket_path = qemu_get_local_state_pathname("run/qemu-pr-helper.sock");
> +pidfile = qemu_get_local_state_pathname("run/qemu-pr-helper.pid");
>  }
>  
>  static void usage(const char *name)
>  {
> -compute_default_paths();
>  (printf) (
>  "Usage: %s [OPTIONS] FILE\n"
>  "Persistent Reservation helper program for QEMU\n"
> @@ -844,19 +842,6 @@ static gboolean accept_client(QIOChannel *ioc, 
> GIOCondition cond, gpointer opaqu
>  return TRUE;
>  }
>  
> -
> -/*
> - * Check socket parameters compatibility when socket activation is used.
> - */
> -static const char *socket_activation_validate_opts(void)
> -{
> -if (socket_path != NULL) {
> -return "Unix socket can't be set when using socket activation";
> -}
> -
> -return NULL;
> -}
> -
>  static void termsig_handler(int signum)
>  {
>  atomic_cmpxchg(&state, RUNNING, TERMINATE);
> @@ -930,6 +915,7 @@ int main(int argc, char **argv)
>  char *trace_file = NULL;
>  bool daemonize = false;
>  bool pidfile_specified = false;
> +bool socket_path_specified = false;
>  unsigned socket_activation;
>  
>  struct sigaction sa_sigterm;
> @@ -946,12 +932,14 @@ int main(int argc, char **argv)
>  qemu_add_opts(&qemu_trace_opts);
>  qemu_init_exec_dir(argv[0]);
>  
> -pidfile = qemu_get_local_state_pathname("run/qemu-pr-helper.pid");
> +compute_default_paths();
>  
>  while ((ch = getopt_long(argc, argv, sopt, lopt, &opt_ind)) != -1) {
>  switch (ch) {
>  case 'k':
> -socket_path = optarg;
> +g_free(socket_path);
> +socket_path = g_strdup(optarg);
> +socket_path_specified = true;
>  if (socket_path[0] != '/') {
>  error_report("socket path must be absolute");
>  exit(EXIT_FAILURE);
> @@ -1042,10 +1030,9 @@ int main(int argc, char **argv)
>  socket_activation = check_socket_activation();
>  if (socket_activation == 0) {
>  SocketAddress saddr;
> -compute_default_paths();
>  saddr = (SocketAddress){
>  .type = SOCKET_ADDRESS_TYPE_UNIX,
> -.u.q_unix.path = g_strdup(socket_path)
> +.u.q_unix.path = socket_path,
>  };
>  server_ioc = qio_channel_socket_new();
>  if (qio_channel_socket_listen_sync(server_ioc, &saddr, &local_err) < 
> 0) {
> @@ -1053,12 +1040,10 @@ int main(int argc, char **argv)
>  error_report_err(local_err);
>  return 1;
>  }
> -g_free(saddr.u.q_unix.path);
>  } else {
>  /* Using socket activation - check user didn't use -p etc. */
> -const char *err_msg = socket_activation_validate_opts();
> -if (err_msg != NULL) {
> -error_report("%s", err_msg);
> +if (socket_path_specified) {
> +error_report("Unix socket can't be set when using socket 
> activation");
>  exit(EXIT_FAILURE);
>  }
>  
> @@ -1075,7 +1060,6 @@ int main(int argc, char **argv)
>   error_get_pretty(local_err));
>  exit(EXIT_FAILURE);
>  }
> -socket_path = NULL;
>  }
>  
>  if (qemu_init_main_loop(&local_err)) {
> 

[Qemu-devel] [PATCH] pr-helper: avoid error on PR IN command with zero request size

[Paolo Bonzini]

After reading a PR IN command with zero request size in prh_read_request,
the resp->result field will be uninitialized and the resp.sz field will
be also uninitialized when returning to prh_co_entry.

If resp->result == GOOD (from a previous successful reply or just luck),
then the assert in prh_write_response might not be triggered and
uninitialized response will be sent.

The fix is to remove the whole handling of sz == 0 in prh_co_entry.
Those errors apply only to PR OUT commands and it's perfectly okay to
catch them later in do_pr_out and multipath_pr_out; the check for
too-short parameters in fact doesn't apply in the easy SG_IO case, as
it can be left to the target firmware even.

The result is that prh_read_request does not fail requests anymore and
prh_co_entry becomes simpler.

Reported-by: Dima Stepanov 
Signed-off-by: Paolo Bonzini 
---
 scsi/qemu-pr-helper.c | 63 +--
 1 file changed, 30 insertions(+), 33 deletions(-)

diff --git a/scsi/qemu-pr-helper.c b/scsi/qemu-pr-helper.c
index 0218d65bbf..c89a446a45 100644
--- a/scsi/qemu-pr-helper.c
+++ b/scsi/qemu-pr-helper.c
@@ -455,6 +455,14 @@ static int multipath_pr_out(int fd, const uint8_t *cdb, 
uint8_t *sense,
 char transportids[PR_HELPER_DATA_SIZE];
 int r;
 
+if (sz < PR_OUT_FIXED_PARAM_SIZE) {
+/* Illegal request, Parameter list length error.  This isn't fatal;
+ * we have read the data, send an error without closing the socket.
+ */
+scsi_build_sense(sense, SENSE_CODE(INVALID_PARAM_LEN));
+return CHECK_CONDITION;
+}
+
 switch (rq_servact) {
 case MPATH_PROUT_REG_SA:
 case MPATH_PROUT_RES_SA:
@@ -574,6 +582,12 @@ static int do_pr_out(int fd, const uint8_t *cdb, uint8_t 
*sense,
  const uint8_t *param, int sz)
 {
 int resp_sz;
+
+if ((fcntl(fd, F_GETFL) & O_ACCMODE) == O_RDONLY) {
+scsi_build_sense(sense, SENSE_CODE(INVALID_OPCODE));
+return CHECK_CONDITION;
+}
+
 #ifdef CONFIG_MPATH
 if (is_mpath(fd)) {
 return multipath_pr_out(fd, cdb, sense, param, sz);
@@ -690,21 +704,6 @@ static int coroutine_fn prh_read_request(PRHelperClient 
*client,
  errp) < 0) {
 goto out_close;
 }
-if ((fcntl(client->fd, F_GETFL) & O_ACCMODE) == O_RDONLY) {
-scsi_build_sense(resp->sense, SENSE_CODE(INVALID_OPCODE));
-sz = 0;
-} else if (sz < PR_OUT_FIXED_PARAM_SIZE) {
-/* Illegal request, Parameter list length error.  This isn't fatal;
- * we have read the data, send an error without closing the socket.
- */
-scsi_build_sense(resp->sense, SENSE_CODE(INVALID_PARAM_LEN));
-sz = 0;
-}
-if (sz == 0) {
-resp->result = CHECK_CONDITION;
-close(client->fd);
-client->fd = -1;
-}
 }
 
 req->fd = client->fd;
@@ -785,25 +784,23 @@ static void coroutine_fn prh_co_entry(void *opaque)
 break;
 }
 
-if (sz > 0) {
-num_active_sockets++;
-if (req.cdb[0] == PERSISTENT_RESERVE_OUT) {
-r = do_pr_out(req.fd, req.cdb, resp.sense,
-  client->data, sz);
-resp.sz = 0;
-} else {
-resp.sz = sizeof(client->data);
-r = do_pr_in(req.fd, req.cdb, resp.sense,
- client->data, &resp.sz);
-resp.sz = MIN(resp.sz, sz);
-}
-num_active_sockets--;
-close(req.fd);
-if (r == -1) {
-break;
-}
-resp.result = r;
+num_active_sockets++;
+if (req.cdb[0] == PERSISTENT_RESERVE_OUT) {
+r = do_pr_out(req.fd, req.cdb, resp.sense,
+  client->data, sz);
+resp.sz = 0;
+} else {
+resp.sz = sizeof(client->data);
+r = do_pr_in(req.fd, req.cdb, resp.sense,
+ client->data, &resp.sz);
+resp.sz = MIN(resp.sz, sz);
+}
+num_active_sockets--;
+close(req.fd);
+if (r == -1) {
+break;
 }
+resp.result = r;
 
 if (prh_write_response(client, &req, &resp, &local_err) < 0) {
 break;
-- 
2.17.1

Re: [Qemu-devel] [PATCH] migration: add capability to bypass the shared memory

[Stefan Hajnoczi]

On Mon, Jul 02, 2018 at 09:52:08PM +0800, Peng Tao wrote:
> On Mon, Jul 2, 2018 at 9:10 PM, Stefan Hajnoczi  wrote:
> > On Sat, Mar 31, 2018 at 04:45:00PM +0800, Lai Jiangshan wrote:
> > Risks:
> > 1. If one cloned VM is exploited then all other VMs are more likely to
> >be exploitable (e.g. kernel address space layout randomization).
> w.r.t. KASLR, any memory duplication technology would expose it. I
> remember there are CVEs (e.g., CVE-2015-2877) specific to this kind
> attack against KSM and it was stated that "Basically if you care about
> this attack vector, disable deduplication.". Share-until-written
> approaches for memory conservation among mutually untrusting tenants
> are inherently detectable for information disclosure, and can be
> classified as potentially misunderstood behaviors rather than
> vulnerabilities. [1]
> 
> I think the same applies to vm templating as well. Actually VM
> templating is more useful (than KSM) in this regard since we can
> create a template for each trusted tenant where as with KSM all VMs on
> a host are treated equally.
> 
> [1] https://access.redhat.com/security/cve/cve-2015-2877

That solves the problem between untrusted users but a breach in one
clone may reveal secrets of all other clones belonging to the same
tenant.  As a user, I would be uncomfortable knowing that if one of my
machines is breached then secrets used by all of my machines might be
exposed.

> > 2. If you give VMs cloned from the same template to untrusted users,
> >they may be able to determine the secrets other users' VMs.
> In kata and runv, vm templating is used carefully so that we do not
> use or save any secret keys before creating the template VM. IOW, the
> feature is not supposed to be used generally to create any template
> VMs at any stage.

At what point are templates captured to avoid these problems?  Is there
code that shows how to do this?

> >  Security is a
> > major factor for using Kata, so it's important not to leak secrets
> > between cloned VMs.
> >
> Yes, indeed! And it is all about trade-offs, VM templating or KSM. If
> we want security above anything, we should just disable all the
> sharing. But there is actually no ceiling (think about physical
> isolation!). So it's more about trade-offs. With Kata, VM templating
> and KSM give users options to achieve better performance and lower
> memory footprint with little sacrifice. The security advantage of
> running VM-based containers is still there.

Adding options to enable/disable features leads to confusion among
users, makes performance comparisons harder, and increases support
overhead.

Technical solutions to the security problems are possible.  I'm
interested in progress in this area because it means users don't need to
make a choice, they can benefit from the feature without sacrificing
security.

Stefan


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH v2 2/2] iotests: New test 223 for exporting dirty bitmap over NBD

[Vladimir Sementsov-Ogievskiy]

02.07.2018 22:14, Eric Blake wrote:

Although this test is NOT a full test of image fleecing (as it
intentionally uses just a single block device directly exported
over NBD, rather than trying to set up a blockdev-backup job with
multiple BDS involved), it DOES prove that qemu as a server is
able to properly expose a dirty bitmap over NBD.

When coupled with image fleecing, it is then possible for a
third-party client to do an incremental backup by using
qemu-img map with the x-dirty-bitmap option to learn which parts
of the file are dirty (perhaps confusingly, they are the portions
mapped as "data":false - which is part of the reason this is
still in the x- experimental namespace), along with another
normal client (perhaps 'qemu-nbd -c' to expose the server over
/dev/nbd0 and then just use normal I/O on that block device) to
read the dirty sections.

Signed-off-by: Eric Blake 
---
  tests/qemu-iotests/223 | 138 +
  tests/qemu-iotests/223.out |  49 
  tests/qemu-iotests/group   |   1 +
  3 files changed, 188 insertions(+)
  create mode 100755 tests/qemu-iotests/223
  create mode 100644 tests/qemu-iotests/223.out

diff --git a/tests/qemu-iotests/223 b/tests/qemu-iotests/223
new file mode 100755
index 000..b63b7a4f9e1
--- /dev/null
+++ b/tests/qemu-iotests/223
@@ -0,0 +1,138 @@
+#!/bin/bash


[...]


+
+echo
+echo "=== End NBD server ==="
+echo
+
+_send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-remove",
+  "arguments":{"name":"n"}}' "return"
+_send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-stop"}' "return"


blockdev-del is not necessary?

with or without:

Reviewed-by: Vladimir Sementsov-Ogievskiy 


--
Best regards,
Vladimir

[Qemu-devel] [PULL 03/20] build-system: remove per-test GCOV reporting

[Alex Bennée]

I'm not entirely sure who's using this information and certainly in a
CI environment it just washes over as additional noise. Later patches
will provide new reporting options so a user who wants to analyse
individual tests will be able to use that to get the information.

Signed-off-by: Alex Bennée 
Reviewed-by: Philippe Mathieu-Daudé 
Tested-by: Philippe Mathieu-Daudé 
Reviewed-by: Daniel P. Berrangé 

diff --git a/docs/devel/testing.rst b/docs/devel/testing.rst
index f33e5a8423..66ef219f69 100644
--- a/docs/devel/testing.rst
+++ b/docs/devel/testing.rst
@@ -158,12 +158,11 @@ rarely used. See "QEMU iotests" section below for more 
information.
 GCC gcov support
 
 
-``gcov`` is a GCC tool to analyze the testing coverage by instrumenting the
-tested code. To use it, configure QEMU with ``--enable-gcov`` option and build.
-Then run ``make check`` as usual. There will be additional ``gcov`` output as
-the testing goes on, showing the test coverage percentage numbers per analyzed
-source file. More detailed reports can be obtained by running ``gcov`` command
-on the output files under ``$build_dir/tests/``, please read the ``gcov``
+``gcov`` is a GCC tool to analyze the testing coverage by
+instrumenting the tested code. To use it, configure QEMU with
+``--enable-gcov`` option and build. Then run ``make check`` as usual.
+Reports can be obtained by running ``gcov`` command on the output
+files under ``$build_dir/tests/``, please read the ``gcov``
 documentation for more information.
 
 QEMU iotests
diff --git a/tests/Makefile.include b/tests/Makefile.include
index e8bb2d8f66..756474814a 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -891,26 +891,16 @@ GCOV_OPTIONS = -n $(if $(V),-f,)
 
 .PHONY: $(patsubst %, check-qtest-%, $(QTEST_TARGETS))
 $(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) \
MALLOC_PERTURB_=$${MALLOC_PERTURB_:-$$(( $${RANDOM:-0} % 255 + 
1))} \
gtester $(GTESTER_OPTIONS) -m=$(SPEED) $(check-qtest-$*-y) 
$(check-qtest-generic-y),"GTESTER","$@")
-   $(if $(CONFIG_GCOV),@for f in $(gcov-files-$*-y) 
$(gcov-files-generic-y); do \
- echo Gcov report for $$f:;\
- $(GCOV) $(GCOV_OPTIONS) $$f -o `dirname $$f`; \
-   done,)
 
 .PHONY: $(patsubst %, check-%, $(check-unit-y) $(check-speed-y))
 $(patsubst %, check-%, $(check-unit-y) $(check-speed-y)): check-%: %
-   $(if $(CONFIG_GCOV),@rm -f *.gcda */*.gcda */*/*.gcda */*/*/*.gcda,)
$(call quiet-command, \
MALLOC_PERTURB_=$${MALLOC_PERTURB_:-$$(( $${RANDOM:-0} % 255 + 
1))} \
gtester $(GTESTER_OPTIONS) -m=$(SPEED) $*,"GTESTER","$*")
-   $(if $(CONFIG_GCOV),@for f in $(gcov-files-$(subst tests/,,$*)-y) 
$(gcov-files-generic-y); do \
- echo Gcov report for $$f:;\
- $(GCOV) $(GCOV_OPTIONS) $$f -o `dirname $$f`; \
-   done,)
 
 # gtester tests with XML output
 
-- 
2.17.1

[Qemu-devel] [PULL 02/20] travis: test out-of-tree builds

[Alex Bennée]

From: Philippe Mathieu-Daudé 

Force one config to build 'out-of-tree' (object files and executables
are created in a tree outside the project source code).

Signed-off-by: Philippe Mathieu-Daudé 
Signed-off-by: Alex Bennée 
Reviewed-by: Daniel P. Berrangé 

diff --git a/.travis.yml b/.travis.yml
index 134d5331fe..32188d51f1 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -50,6 +50,8 @@ notifications:
 on_failure: always
 env:
   global:
+- SRC_DIR="."
+- BUILD_DIR="."
 - TEST_CMD="make check"
 - MAKEFLAGS="-j3"
   matrix:
@@ -68,11 +70,15 @@ before_install:
   - if [ "$TRAVIS_OS_NAME" == "osx" ]; then brew install libffi gettext glib 
pixman ; fi
   - git submodule update --init --recursive capstone dtc ui/keycodemapdb
 before_script:
-  - ./configure ${CONFIG} || { cat config.log && exit 1; }
+  - mkdir -p ${BUILD_DIR} && cd ${BUILD_DIR}
+  - ${SRC_DIR}/configure ${CONFIG} || { cat config.log && exit 1; }
 script:
   - make ${MAKEFLAGS} && ${TEST_CMD}
 matrix:
   include:
+# Test out-of-tree builds
+- env: CONFIG="--enable-debug --enable-debug-tcg"
+   BUILD_DIR="out-of-tree/build/dir" SRC_DIR="../../.."
 # Test with Clang for compile portability (Travis uses clang-5.0)
 - env: CONFIG="--disable-system"
   compiler: clang
-- 
2.17.1

[Qemu-devel] [PULL 00/20] Travis, Code Coverage and Cross Build updates

[Alex Bennée]

The following changes since commit 46d0885adff9b99622d72f23a8b04c298a8bf91d:

  Merge remote-tracking branch 'remotes/mst/tags/for_upstream' into staging 
(2018-07-03 09:49:20 +0100)

are available in the Git repository at:

  https://github.com/stsquad/qemu.git 
tags/pull-code-coverage-and-build-tweaks-030718-1

for you to fetch changes up to e84a4f16f0fd67e5e503ceb50a9d069f7b76fcb3:

  docker: add linux-user powered cross builder for QEMU (2018-07-03 11:09:56 
+0100)


Code coverage and other build tweaks

  - some travis speed-ups
  - modernise code coverage support
  - docker image cleanups
  - clean-up binfmt_misc docker infrastructure
  - add debian-powerpc-user-cross image for ppc32 build


Alex Bennée (14):
  build-system: remove per-test GCOV reporting
  .gitignore: add .gcov files
  docker: add gcovr to travis image
  travis: add gcovr summary for GCOV build
  build-system: add clean-coverage target
  build-system: add coverage-report target
  linux-user: introduce preexit_cleanup
  linux-user: add gcov support to preexit_cleanup
  docker: filter out linux-user builds for mingw
  docker: drop QEMU build-dep from bootstrap
  docker: debian-bootstrap.pre allow customising of variant/url
  docker: add special handling for FROM:debian-%-user targets
  docker: add special rule for deboostrapped images
  docker: add linux-user powered cross builder for QEMU

Philippe Mathieu-Daudé (6):
  travis: do not waste time cloning unused submodules
  travis: test out-of-tree builds
  docker: ubuntu: Update the package list before installing new ones
  docker: ubuntu: Use SDL2
  docker: Clean the MXE base image
  docker: Do not run tests in 'intermediate' images

 .gitignore |  1 +
 .travis.yml| 14 +++-
 MAINTAINERS|  1 +
 Makefile   | 24 +++
 docs/devel/testing.rst | 21 --
 linux-user/Makefile.objs   |  2 +-
 linux-user/exit.c  | 35 ++
 linux-user/qemu.h  |  8 +++
 linux-user/syscall.c   | 10 +--
 scripts/travis/coverage-summary.sh | 27 
 tests/Makefile.include | 10 ---
 tests/docker/Makefile.include  | 74 --
 tests/docker/docker.py |  4 ++
 tests/docker/dockerfiles/debian-bootstrap.docker   |  2 -
 tests/docker/dockerfiles/debian-bootstrap.pre  | 11 +++-
 .../dockerfiles/debian-powerpc-user-cross.docker   | 15 +
 tests/docker/dockerfiles/debian8-mxe.docker|  2 +-
 tests/docker/dockerfiles/travis.docker |  2 +-
 tests/docker/dockerfiles/ubuntu.docker |  8 +--
 19 files changed, 228 insertions(+), 43 deletions(-)
 create mode 100644 linux-user/exit.c
 create mode 100755 scripts/travis/coverage-summary.sh
 create mode 100644 tests/docker/dockerfiles/debian-powerpc-user-cross.docker

--
2.17.1

[Qemu-devel] [PULL 01/20] travis: do not waste time cloning unused submodules

[Alex Bennée]

From: Philippe Mathieu-Daudé 

Builds only require:
- dtc
- keycodemapdb
- capstone

Signed-off-by: Philippe Mathieu-Daudé 
[AJB: drop wget cache]
Signed-off-by: Alex Bennée 
Reviewed-by: Daniel P. Berrangé 

diff --git a/.travis.yml b/.travis.yml
index bd66c18fed..134d5331fe 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -66,8 +66,7 @@ git:
 before_install:
   - if [ "$TRAVIS_OS_NAME" == "osx" ]; then brew update ; fi
   - if [ "$TRAVIS_OS_NAME" == "osx" ]; then brew install libffi gettext glib 
pixman ; fi
-  - wget -O - 
http://people.linaro.org/~alex.bennee/qemu-submodule-git-seed.tar.xz | tar -xvJ
-  - git submodule update --init --recursive
+  - git submodule update --init --recursive capstone dtc ui/keycodemapdb
 before_script:
   - ./configure ${CONFIG} || { cat config.log && exit 1; }
 script:
-- 
2.17.1

[Qemu-devel] [PULL 07/20] build-system: add clean-coverage target

[Alex Bennée]

This can be used to remove any stale coverage data before any
particular test run. This is useful for analysing individual tests.

Signed-off-by: Alex Bennée 
Reviewed-by: Daniel P. Berrangé 
Reviewed-by: Philippe Mathieu-Daudé 
Tested-by: Philippe Mathieu-Daudé ---

diff --git a/Makefile b/Makefile
index 7ed9cc4a21..2b3413a5ba 100644
--- a/Makefile
+++ b/Makefile
@@ -723,6 +723,14 @@ module_block.h: 
$(SRC_PATH)/scripts/modules/module_block.py config-host.mak
$(addprefix $(SRC_PATH)/,$(patsubst %.mo,%.c,$(block-obj-m))), \
"GEN","$@")
 
+ifdef CONFIG_GCOV
+.PHONY: clean-coverage
+clean-coverage:
+   $(call quiet-command, \
+   find . \( -name '*.gcda' -o -name '*.gcov' \) -type f -exec rm 
{} +, \
+   "CLEAN", "coverage files")
+endif
+
 clean:
 # avoid old build problems by removing potentially incorrect old files
rm -f config.mak op-i386.h opc-i386.h gen-op-i386.h op-arm.h opc-arm.h 
gen-op-arm.h
@@ -1073,6 +1081,9 @@ endif
echo '')
@echo  'Cleaning targets:'
@echo  '  clean   - Remove most generated files but keep the 
config'
+ifdef CONFIG_GCOV
+   @echo  '  clean-coverage  - Remove coverage files'
+endif
@echo  '  distclean   - Remove all generated files'
@echo  '  dist- Build a distributable tarball'
@echo  ''
diff --git a/docs/devel/testing.rst b/docs/devel/testing.rst
index 66ef219f69..7f04ca104e 100644
--- a/docs/devel/testing.rst
+++ b/docs/devel/testing.rst
@@ -161,9 +161,14 @@ GCC gcov support
 ``gcov`` is a GCC tool to analyze the testing coverage by
 instrumenting the tested code. To use it, configure QEMU with
 ``--enable-gcov`` option and build. Then run ``make check`` as usual.
-Reports can be obtained by running ``gcov`` command on the output
-files under ``$build_dir/tests/``, please read the ``gcov``
-documentation for more information.
+
+If you want to gather coverage information on a single test the ``make
+clean-coverage`` target can be used to delete any existing coverage
+information before running a single test.
+
+Reports can be obtained by running ``gcov`` command
+on the output files under ``$build_dir/tests/``, please read the
+``gcov`` documentation for more information.
 
 QEMU iotests
 
-- 
2.17.1

[Qemu-devel] [PULL 06/20] travis: add gcovr summary for GCOV build

[Alex Bennée]

This gives a more useful summary, sorted by descending % coverage,
after the tests have run. The final numbers will give an idea if our
coverage is getting better or worse.

To keep the width sane we need to post process the file that the old
gcovr tool generates. This is done with a mix of sed, awk and column
in the scripts/coverage-summary.sh script.

As quite a lot of lines don't get covered at all we filter out all the
0% lines. If the file doesn't appear it is not being exercised.

Signed-off-by: Alex Bennée 
Reviewed-by: Daniel P. Berrangé 

diff --git a/.travis.yml b/.travis.yml
index 32188d51f1..95be6ec59f 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -38,6 +38,7 @@ addons:
   - libvte-2.90-dev
   - sparse
   - uuid-dev
+  - gcovr
 
 # The channel name "irc.oftc.net#qemu" is encrypted against qemu/qemu
 # to prevent IRC notifications from forks. This was created using:
@@ -86,6 +87,8 @@ matrix:
   compiler: clang
 # gprof/gcov are GCC features
 - env: CONFIG="--enable-gprof --enable-gcov --disable-pie 
--target-list=aarch64-softmmu,arm-softmmu,i386-softmmu,mips-softmmu,mips64-softmmu,ppc64-softmmu,riscv64-softmmu,s390x-softmmu,x86_64-softmmu"
+  after_success:
+- ${SRC_DIR}/scripts/travis/coverage-summary.sh
   compiler: gcc
 # We manually include builds which we disable "make check" for
 - env: CONFIG="--enable-debug --enable-tcg-interpreter"
diff --git a/MAINTAINERS b/MAINTAINERS
index 42a1892d6a..4917b8e48d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2165,6 +2165,7 @@ R: Philippe Mathieu-Daudé 
 L: qemu-devel@nongnu.org
 S: Maintained
 F: .travis.yml
+F: scripts/travis/
 F: .shippable.yml
 F: tests/docker/
 F: tests/vm/
diff --git a/scripts/travis/coverage-summary.sh 
b/scripts/travis/coverage-summary.sh
new file mode 100755
index 00..d7086cf9ca
--- /dev/null
+++ b/scripts/travis/coverage-summary.sh
@@ -0,0 +1,27 @@
+#!/bin/sh
+#
+# Author: Alex Bennée 
+#
+# Summerise the state of code coverage with gcovr and tweak the output
+# to be more sane on Travis hosts. As we expect to be executed on a
+# throw away CI instance we do spam temp files all over the shop. You
+# most likely don't want to execute this script but just call gcovr
+# directly. See also "make coverage-report"
+#
+# This code is licensed under the GPL version 2 or later.  See
+# the COPYING file in the top-level directory.
+
+# first generate the coverage report
+gcovr -p -o raw-report.txt
+
+# strip the full-path and line markers
+sed s@$PWD\/@@ raw-report.txt | sed s/[0-9]\*[,-]//g > simplified.txt
+
+# reflow lines that got split
+awk '/.[ch]$/ { printf("%s", $0); next } 1' simplified.txt > rejoined.txt
+
+# columnify
+column -t rejoined.txt > final.txt
+
+# and dump, stripping out 0% coverage
+grep -v "0%" final.txt
-- 
2.17.1

[Qemu-devel] [PULL 05/20] docker: add gcovr to travis image

[Alex Bennée]

Useful for debugging if nothing else as the gcovr on the Travis images
are a little old.

Signed-off-by: Alex Bennée 
Reviewed-by: Philippe Mathieu-Daudé 
Reviewed-by: Daniel P. Berrangé 

diff --git a/tests/docker/dockerfiles/travis.docker 
b/tests/docker/dockerfiles/travis.docker
index c5ad39b533..03ebfb0ef2 100644
--- a/tests/docker/dockerfiles/travis.docker
+++ b/tests/docker/dockerfiles/travis.docker
@@ -5,7 +5,7 @@ ENV LC_ALL en_US.UTF-8
 RUN cat /etc/apt/sources.list | sed "s/# deb-src/deb-src/" >> 
/etc/apt/sources.list
 RUN apt-get update
 RUN apt-get -y build-dep qemu
-RUN apt-get -y install device-tree-compiler python2.7 python-yaml 
dh-autoreconf gdb strace lsof net-tools
+RUN apt-get -y install device-tree-compiler python2.7 python-yaml 
dh-autoreconf gdb strace lsof net-tools gcovr
 # Travis tools require PhantomJS / Neo4j / Maven accessible
 # in their PATH (QEMU build won't access them).
 ENV PATH 
/usr/local/phantomjs/bin:/usr/local/phantomjs:/usr/local/neo4j-3.2.7/bin:/usr/local/maven-3.5.2/bin:/usr/local/cmake-3.9.2/bin:/usr/local/clang-5.0.0/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
-- 
2.17.1

[Qemu-devel] [PULL 10/20] linux-user: add gcov support to preexit_cleanup

[Alex Bennée]

As we don't always take the normal exit path when running a guest we
can skip the normal exit destructors where gcov normally dumps it's
info. The GCC manual suggests long running programs use __gcov_dump()
to flush out the coverage state periodically so we use that here.

Signed-off-by: Alex Bennée 
Reviewed-by: Philippe Mathieu-Daudé 

diff --git a/linux-user/exit.c b/linux-user/exit.c
index aed8713fae..14e94e28fa 100644
--- a/linux-user/exit.c
+++ b/linux-user/exit.c
@@ -19,10 +19,17 @@
 #include "qemu/osdep.h"
 #include "qemu.h"
 
+#ifdef CONFIG_GCOV
+extern void __gcov_dump(void);
+#endif
+
 void preexit_cleanup(CPUArchState *env, int code)
 {
 #ifdef TARGET_GPROF
 _mcleanup();
+#endif
+#ifdef CONFIG_GCOV
+__gcov_dump();
 #endif
 gdb_exit(env, code);
 }
-- 
2.17.1

[Qemu-devel] [PULL 08/20] build-system: add coverage-report target

[Alex Bennée]

This will build a coverage report under the current directory in
reports/coverage. At the users option a report can be generated by
directly invoking something like:

  make foo/bar/coverage-report.html

Signed-off-by: Alex Bennée 
Reviewed-by: Philippe Mathieu-Daudé 
Tested-by: Philippe Mathieu-Daudé 
Reviewed-by: Daniel P. Berrangé 

diff --git a/Makefile b/Makefile
index 2b3413a5ba..68af7b5d7c 100644
--- a/Makefile
+++ b/Makefile
@@ -986,6 +986,16 @@ docs/interop/qemu-qmp-ref.dvi 
docs/interop/qemu-qmp-ref.html \
 docs/interop/qemu-qmp-ref.txt docs/interop/qemu-qmp-ref.7: \
docs/interop/qemu-qmp-ref.texi docs/interop/qemu-qmp-qapi.texi
 
+# Reports/Analysis
+
+%/coverage-report.html:
+   @mkdir -p $*
+   $(call quiet-command,\
+   gcovr -p --html --html-details -o $@, \
+   "GEN", "coverage-report.html")
+
+.PHONY: coverage-report
+coverage-report: $(CURDIR)/reports/coverage/coverage-report.html
 
 ifdef CONFIG_WIN32
 
@@ -1095,6 +1105,9 @@ endif
@echo  'Documentation targets:'
@echo  '  html info pdf txt'
@echo  '  - Build documentation in specified format'
+ifdef CONFIG_GCOV
+   @echo  '  coverage-report - Create code coverage report'
+endif
@echo  ''
 ifdef CONFIG_WIN32
@echo  'Windows targets:'
diff --git a/docs/devel/testing.rst b/docs/devel/testing.rst
index 7f04ca104e..5e19cd50da 100644
--- a/docs/devel/testing.rst
+++ b/docs/devel/testing.rst
@@ -166,9 +166,14 @@ If you want to gather coverage information on a single 
test the ``make
 clean-coverage`` target can be used to delete any existing coverage
 information before running a single test.
 
-Reports can be obtained by running ``gcov`` command
-on the output files under ``$build_dir/tests/``, please read the
-``gcov`` documentation for more information.
+You can generate a HTML coverage report by executing ``make
+coverage-report`` which will create
+./reports/coverage/coverage-report.html. If you want to create it
+elsewhere simply execute ``make /foo/bar/baz/coverage-report.html``.
+
+Further analysis can be conducted by running the ``gcov`` command
+directly on the various .gcda output files. Please read the ``gcov``
+documentation for more information.
 
 QEMU iotests
 
-- 
2.17.1

[Qemu-devel] [PULL 04/20] .gitignore: add .gcov files

[Alex Bennée]

These are temporary files generated on gcov runs and shouldn't be
included in the source tree.

Signed-off-by: Alex Bennée 
Reviewed-by: Philippe Mathieu-Daudé 
Tested-by: Philippe Mathieu-Daudé 
Reviewed-by: Daniel P. Berrangé 

diff --git a/.gitignore b/.gitignore
index 9da3b3e626..5668d02782 100644
--- a/.gitignore
+++ b/.gitignore
@@ -155,6 +155,7 @@
 .sdk
 *.gcda
 *.gcno
+*.gcov
 /pc-bios/bios-pq/status
 /pc-bios/vgabios-pq/status
 /pc-bios/optionrom/linuxboot.asm
-- 
2.17.1

[Qemu-devel] [PULL 17/20] docker: debian-bootstrap.pre allow customising of variant/url

[Alex Bennée]

We default to the buildd variant as most of our images are for
building. However lets give the user the ability to specify "minbase"
if they want to create a simple base image for experimentation.

Allowing the tweaking of DEB_URL means we can also bootstrap other
Debian based OS's. For example:

  make docker-binfmt-image-debian-ubuntu-bionic-arm64 \
   DEB_ARCH=arm64 DEB_TYPE=bionic \
   DEB_VARIANT=minbase DEB_URL=http://ports.ubuntu.com/ \
   EXECUTABLE=./aarch64-linux-user/qemu-aarch64

Signed-off-by: Alex Bennée 
Reviewed-by: Philippe Mathieu-Daudé 
Tested-by: Philippe Mathieu-Daudé 

diff --git a/tests/docker/dockerfiles/debian-bootstrap.pre 
b/tests/docker/dockerfiles/debian-bootstrap.pre
index 7c76dce663..56e1aa7a21 100755
--- a/tests/docker/dockerfiles/debian-bootstrap.pre
+++ b/tests/docker/dockerfiles/debian-bootstrap.pre
@@ -32,6 +32,15 @@ if [ -z "${DEB_TYPE}" ]; then
 
 fi
 
+# The following allow finer grain control over the defaults
+if [ -z "${DEB_VARIANT}" ]; then
+DEB_VARIANT=buildd
+fi
+
+if [ -z "${DEB_URL}" ]; then
+DEB_URL="http://httpredir.debian.org/debian";
+fi
+
 # We check in order for
 #
 #  - DEBOOTSTRAP_DIR pointing at a development checkout
@@ -107,5 +116,5 @@ fi
 
 echo "Building a rootfs using ${FAKEROOT} and ${DEBOOTSTRAP} 
${DEB_ARCH}/${DEB_TYPE}"
 
-${FAKEROOT} ${DEBOOTSTRAP} --variant=buildd --foreign --arch=$DEB_ARCH 
$DEB_TYPE . http://httpredir.debian.org/debian || exit 1
+${FAKEROOT} ${DEBOOTSTRAP} --variant=$DEB_VARIANT --foreign --arch=$DEB_ARCH 
$DEB_TYPE . $DEB_URL || exit 1
 exit 0
-- 
2.17.1

[Qemu-devel] [PULL 18/20] docker: add special handling for FROM:debian-%-user targets

[Alex Bennée]

These will have been build with debootstrap so we need to check
against the debian-bootstrap dockerfile. This does mean sticking to
debian-FOO-user as the naming conventions for boot-strapped images.
The actual cross image is built on top.

Signed-off-by: Alex Bennée 
Reviewed-by: Philippe Mathieu-Daudé 
Tested-by: Philippe Mathieu-Daudé 

diff --git a/tests/docker/docker.py b/tests/docker/docker.py
index b279836154..69e7130db7 100755
--- a/tests/docker/docker.py
+++ b/tests/docker/docker.py
@@ -113,6 +113,10 @@ def _copy_binary_with_libs(src, dest_dir):
 _copy_with_mkdir(l , dest_dir, so_path)
 
 def _read_qemu_dockerfile(img_name):
+# special case for Debian linux-user images
+if img_name.startswith("debian") and img_name.endswith("user"):
+img_name = "debian-bootstrap"
+
 df = os.path.join(os.path.dirname(__file__), "dockerfiles",
   img_name + ".docker")
 return open(df, "r").read()
-- 
2.17.1

[Qemu-devel] [PULL 12/20] docker: ubuntu: Update the package list before installing new ones

[Alex Bennée]

From: Philippe Mathieu-Daudé 

Since docker caches the different layers, updating the package
list does not invalidate the previous "apt-get update" layer,
and it is likely "apt-get install" hits an outdated repository.

See 
https://docs.docker.com/develop/develop-images/dockerfile_best-practices/#apt-get

This fixes:

  $ make docker-image-ubuntu V=1
  ./tests/docker/docker.py build qemu:ubuntu 
tests/docker/dockerfiles/ubuntu.docker   --add-current-user
  Sending build context to Docker daemon  3.072kB
  [...]
  E: Failed to fetch 
http://archive.ubuntu.com/ubuntu/pool/main/m/mesa/libgles2-mesa_17.0.7-0ubuntu0.16.04.2_amd64.deb
  404  Not Found
  E: Failed to fetch 
http://archive.ubuntu.com/ubuntu/pool/main/m/mesa/libgles2-mesa-dev_17.0.7-0ubuntu0.16.04.2_amd64.deb
  404  Not Found
  E: Unable to fetch some archives, maybe run apt-get update or try with 
--fix-missing?
  The command '/bin/sh -c apt-get -y install $PACKAGES' returned a non-zero 
code: 100
  tests/docker/Makefile.include:40: recipe for target 'docker-image-ubuntu' 
failed
  make: *** [docker-image-ubuntu] Error 1

Signed-off-by: Philippe Mathieu-Daudé 
Signed-off-by: Alex Bennée 

diff --git a/tests/docker/dockerfiles/ubuntu.docker 
b/tests/docker/dockerfiles/ubuntu.docker
index dabbf2a8a4..c03520ce3f 100644
--- a/tests/docker/dockerfiles/ubuntu.docker
+++ b/tests/docker/dockerfiles/ubuntu.docker
@@ -1,7 +1,6 @@
 FROM ubuntu:16.04
 RUN echo "deb http://archive.ubuntu.com/ubuntu/ trusty universe multiverse" >> 
\
 /etc/apt/sources.list
-RUN apt-get update
 ENV PACKAGES flex bison \
 libusb-1.0-0-dev libiscsi-dev librados-dev libncurses5-dev 
libncursesw5-dev \
 libseccomp-dev libgnutls-dev libssh2-1-dev  libspice-server-dev \
@@ -13,6 +12,7 @@ ENV PACKAGES flex bison \
 libjemalloc-dev libcacard-dev libusbredirhost-dev libnfs-dev libcap-dev 
libattr1-dev \
 texinfo \
 gettext git make ccache python-yaml gcc clang sparse
-RUN apt-get -y install $PACKAGES
+RUN apt-get update && \
+apt-get -y install $PACKAGES
 RUN dpkg -l $PACKAGES | sort > /packages.txt
 ENV FEATURES clang pyyaml
-- 
2.17.1

[Qemu-devel] [PULL 19/20] docker: add special rule for deboostrapped images

[Alex Bennée]

We might as well have a custom rule for this. For one thing the
dependencies are different. As the primary dependency for
docker-image-% could never be docker-image-debian-bootstrap we can
drop that test in the main rule as well.

Missing EXECUTABLE, DEB_ARCH and DEB_TYPE are treated as hard faults
now. We also error out if the EXECUTABLE file isn't there. We should
really do this with a dependency on any source rules but currently
subdir-FOO-linux-user isn't enough on a clean build.

Signed-off-by: Alex Bennée 
Reviewed-by: Philippe Mathieu-Daudé 
Tested-by: Philippe Mathieu-Daudé 

diff --git a/tests/docker/Makefile.include b/tests/docker/Makefile.include
index 8641f5da2c..d43af0db32 100644
--- a/tests/docker/Makefile.include
+++ b/tests/docker/Makefile.include
@@ -49,9 +49,6 @@ docker-image-%: $(DOCKER_FILES_DIR)/%.docker
"CHECK", "$*")
 else
 docker-image-%: $(DOCKER_FILES_DIR)/%.docker
-   @if test "$@" = docker-image-debian-bootstrap -a -z "$(EXECUTABLE)"; 
then \
-   echo WARNING: EXECUTABLE is not set, debootstrap may fail. 2>&1 
; \
-   fi
$(call quiet-command,\
$(DOCKER_SCRIPT) build qemu:$* $< \
$(if $V,,--quiet) $(if $(NOCACHE),--no-cache) \
@@ -59,6 +56,28 @@ docker-image-%: $(DOCKER_FILES_DIR)/%.docker
$(if $(EXTRA_FILES),--extra-files $(EXTRA_FILES))\
$(if $(EXECUTABLE),--include-executable=$(EXECUTABLE)),\
"BUILD","$*")
+
+# Special rule for debootstraped binfmt linux-user images
+docker-binfmt-image-debian-%: $(DOCKER_FILES_DIR)/debian-bootstrap.docker
+   $(if $(EXECUTABLE),,\
+   $(error EXECUTABLE not set, debootstrap of debian-$* would 
fail))
+   @if ! test -f "$(EXECUTABLE)"; \
+   $(error Please build $(EXECUTABLE) first) \
+   fi
+   $(if $(DEB_ARCH),,\
+   $(error DEB_ARCH not set, debootstrap of debian-$* would fail))
+   $(if $(DEB_TYPE),,\
+   $(error DEB_TYPE not set, debootstrap of debian-$* would fail))
+   $(call quiet-command,   \
+   DEB_ARCH=$(DEB_ARCH)\
+   DEB_TYPE=$(DEB_TYPE)\
+   $(DOCKER_SCRIPT) build qemu:debian-$* $<\
+   $(if $V,,--quiet) $(if $(NOCACHE),--no-cache)   \
+   $(if $(NOUSER),,--add-current-user) \
+   $(if $(EXTRA_FILES),--extra-files $(EXTRA_FILES))   \
+   $(if $(EXECUTABLE),--include-executable=$(EXECUTABLE)), \
+   "BUILD","binfmt debian-$* (debootstrapped)")
+
 endif
 
 # Enforce dependencies for composite images
-- 
2.17.1

[Qemu-devel] [PULL 15/20] docker: Do not run tests in 'intermediate' images

[Alex Bennée]

From: Philippe Mathieu-Daudé 

We can still build the DOCKER_INTERMEDIATE_IMAGES images,
but they won't appear in 'make test*@$IMAGE'.

Signed-off-by: Philippe Mathieu-Daudé 
Signed-off-by: Alex Bennée 

diff --git a/tests/docker/Makefile.include b/tests/docker/Makefile.include
index 1813ec0781..8641f5da2c 100644
--- a/tests/docker/Makefile.include
+++ b/tests/docker/Makefile.include
@@ -5,6 +5,8 @@
 DOCKER_SUFFIX := .docker
 DOCKER_FILES_DIR := $(SRC_PATH)/tests/docker/dockerfiles
 DOCKER_DEPRECATED_IMAGES := debian
+# we don't run tests on intermediate images (used as base by another image)
+DOCKER_INTERMEDIATE_IMAGES := debian8 debian9 debian8-mxe debian-ports 
debian-sid
 DOCKER_IMAGES := $(filter-out $(DOCKER_DEPRECATED_IMAGES),$(sort $(notdir 
$(basename $(wildcard $(DOCKER_FILES_DIR)/*.docker)
 DOCKER_TARGETS := $(patsubst %,docker-image-%,$(DOCKER_IMAGES))
 # Use a global constant ccache directory to speed up repetitive builds
@@ -101,7 +103,7 @@ docker-image-travis: NOUSER=1
 docker-image-tricore-cross: docker-image-debian9
 
 # Expand all the pre-requistes for each docker image and test combination
-$(foreach i,$(DOCKER_IMAGES) $(DOCKER_DEPRECATED_IMAGES), \
+$(foreach i,$(filter-out $(DOCKER_INTERMEDIATE_IMAGES),$(DOCKER_IMAGES) 
$(DOCKER_DEPRECATED_IMAGES)), \
$(foreach t,$(DOCKER_TESTS) $(DOCKER_TOOLS), \
$(eval .PHONY: docker-$t@$i) \
$(eval docker-$t@$i: docker-image-$i docker-run-$t@$i) \
-- 
2.17.1

[Qemu-devel] [PULL 09/20] linux-user: introduce preexit_cleanup

[Alex Bennée]

To avoid repeating ourselves move our preexit clean-up code into a
helper function. I figured the continuing effort to split of the
syscalls made it worthwhile creating a new file for it now.

Signed-off-by: Alex Bennée 
Reviewed-by: Philippe Mathieu-Daudé 
Reviewed-by: Daniel P. Berrangé 
Reviewed-by: Laurent Vivier 

diff --git a/linux-user/Makefile.objs b/linux-user/Makefile.objs
index 59a5c17354..b5dfb71f25 100644
--- a/linux-user/Makefile.objs
+++ b/linux-user/Makefile.objs
@@ -1,7 +1,7 @@
 obj-y = main.o syscall.o strace.o mmap.o signal.o \
elfload.o linuxload.o uaccess.o uname.o \
safe-syscall.o $(TARGET_ABI_DIR)/signal.o \
-$(TARGET_ABI_DIR)/cpu_loop.o
+$(TARGET_ABI_DIR)/cpu_loop.o exit.o
 
 obj-$(TARGET_HAS_BFLT) += flatload.o
 obj-$(TARGET_I386) += vm86.o
diff --git a/linux-user/exit.c b/linux-user/exit.c
new file mode 100644
index 00..aed8713fae
--- /dev/null
+++ b/linux-user/exit.c
@@ -0,0 +1,28 @@
+/*
+ *  exit support for qemu
+ *
+ *  Copyright (c) 2018 Alex Bennée 
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, see .
+ */
+#include "qemu/osdep.h"
+#include "qemu.h"
+
+void preexit_cleanup(CPUArchState *env, int code)
+{
+#ifdef TARGET_GPROF
+_mcleanup();
+#endif
+gdb_exit(env, code);
+}
diff --git a/linux-user/qemu.h b/linux-user/qemu.h
index 793cd4df04..bb85c81aa4 100644
--- a/linux-user/qemu.h
+++ b/linux-user/qemu.h
@@ -623,6 +623,14 @@ static inline int is_error(abi_long ret)
 return (abi_ulong)ret >= (abi_ulong)(-4096);
 }
 
+/**
+ * preexit_cleanup: housekeeping before the guest exits
+ *
+ * env: the CPU state
+ * code: the exit code
+ */
+void preexit_cleanup(CPUArchState *env, int code);
+
 /* Include target-specific struct and function definitions;
  * they may need access to the target-independent structures
  * above, so include them last.
diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 2117fb13b4..7c66442357 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -8018,10 +8018,7 @@ abi_long do_syscall(void *cpu_env, int num, abi_long 
arg1,
 }
 
 cpu_list_unlock();
-#ifdef TARGET_GPROF
-_mcleanup();
-#endif
-gdb_exit(cpu_env, arg1);
+preexit_cleanup(cpu_env, arg1);
 _exit(arg1);
 ret = 0; /* avoid warning */
 break;
@@ -10127,10 +10124,7 @@ abi_long do_syscall(void *cpu_env, int num, abi_long 
arg1,
 #ifdef __NR_exit_group
 /* new thread calls */
 case TARGET_NR_exit_group:
-#ifdef TARGET_GPROF
-_mcleanup();
-#endif
-gdb_exit(cpu_env, arg1);
+preexit_cleanup(cpu_env, arg1);
 ret = get_errno(exit_group(arg1));
 break;
 #endif
-- 
2.17.1

[Qemu-devel] [PULL 13/20] docker: ubuntu: Use SDL2

[Alex Bennée]

From: Philippe Mathieu-Daudé 

Do not test the deprecated API versions (see cabd35840749d).
Debian MXE MinGW cross images are already using SDL2.

Signed-off-by: Philippe Mathieu-Daudé 
Signed-off-by: Alex Bennée 

diff --git a/tests/docker/dockerfiles/ubuntu.docker 
b/tests/docker/dockerfiles/ubuntu.docker
index c03520ce3f..7d724e7f53 100644
--- a/tests/docker/dockerfiles/ubuntu.docker
+++ b/tests/docker/dockerfiles/ubuntu.docker
@@ -5,7 +5,7 @@ ENV PACKAGES flex bison \
 libusb-1.0-0-dev libiscsi-dev librados-dev libncurses5-dev 
libncursesw5-dev \
 libseccomp-dev libgnutls-dev libssh2-1-dev  libspice-server-dev \
 libspice-protocol-dev libnss3-dev libfdt-dev \
-libgtk-3-dev libvte-2.91-dev libsdl1.2-dev libpng12-dev libpixman-1-dev \
+libgtk-3-dev libvte-2.91-dev libsdl2-dev libpng12-dev libpixman-1-dev \
 libvdeplug-dev liblzo2-dev libsnappy-dev libbz2-dev libxen-dev 
librdmacm-dev libibverbs-dev \
 libsasl2-dev libjpeg-turbo8-dev xfslibs-dev libcap-ng-dev libbrlapi-dev 
libcurl4-gnutls-dev \
 libbluetooth-dev librbd-dev libaio-dev glusterfs-common libnuma-dev 
libepoxy-dev libdrm-dev libgbm-dev \
@@ -15,4 +15,4 @@ ENV PACKAGES flex bison \
 RUN apt-get update && \
 apt-get -y install $PACKAGES
 RUN dpkg -l $PACKAGES | sort > /packages.txt
-ENV FEATURES clang pyyaml
+ENV FEATURES clang pyyaml sdl2
-- 
2.17.1

[Qemu-devel] [PULL 14/20] docker: Clean the MXE base image

[Alex Bennée]

From: Philippe Mathieu-Daudé 

Using the duplicated same package is confusing.

Reported-by: Peter Maydell 
Signed-off-by: Philippe Mathieu-Daudé 
Signed-off-by: Alex Bennée 

diff --git a/tests/docker/dockerfiles/debian8-mxe.docker 
b/tests/docker/dockerfiles/debian8-mxe.docker
index 9b8e577b03..2df4cc8c5c 100644
--- a/tests/docker/dockerfiles/debian8-mxe.docker
+++ b/tests/docker/dockerfiles/debian8-mxe.docker
@@ -14,6 +14,6 @@ RUN apt-get update
 RUN DEBIAN_FRONTEND=noninteractive eatmydata \
 apt-get install -y --no-install-recommends \
 libpython2.7-stdlib \
-$(apt-get -s install -y --no-install-recommends gw32.shared-mingw-w64 
gw32.shared-mingw-w64 | egrep "^Inst mxe-x86-64-unknown-" | cut -d\  -f2)
+$(apt-get -s install -y --no-install-recommends gw32.shared-mingw-w64 
| egrep "^Inst mxe-x86-64-unknown-" | cut -d\  -f2)
 
 ENV PATH $PATH:/usr/lib/mxe/usr/bin/ 
-- 
2.17.1

[Qemu-devel] [PULL 20/20] docker: add linux-user powered cross builder for QEMU

[Alex Bennée]

We can't use cross compilers in the current Debian stable and Debian
sid is sketchy as hell. So for powerpc fall back to dog-fooding our
own linux-user to do the build.

As we can only build the base image with a suitably configured
source tree we fall back to checking for its existence when we can't
build it from scratch. However this does mean you don't have to keep
a static powerpc-linux-user in your active configuration just to
update the cross build image.

Signed-off-by: Alex Bennée 
Reviewed-by: Philippe Mathieu-Daudé 
Tested-by: Philippe Mathieu-Daudé 

diff --git a/tests/docker/Makefile.include b/tests/docker/Makefile.include
index d43af0db32..12852ebbc4 100644
--- a/tests/docker/Makefile.include
+++ b/tests/docker/Makefile.include
@@ -121,6 +121,32 @@ docker-image-travis: NOUSER=1
 # Specialist build images, sometimes very limited tools
 docker-image-tricore-cross: docker-image-debian9
 
+# Rules for building linux-user powered images
+#
+# These are slower than using native cross compiler setups but can
+# work around issues with poorly working multi-arch systems and broken
+# packages.
+
+ifeq ($(filter ppc-linux-user,$(TARGET_LIST))$(CONFIG_STATIC),ppc-linux-usery)
+# Jessie is the last supported release for powerpc, but multi-arch is
+# broken so we need a qemu-linux-user for this target
+docker-binfmt-image-debian-powerpc-user: DEB_ARCH = powerpc
+docker-binfmt-image-debian-powerpc-user: DEB_TYPE = jessie
+docker-binfmt-image-debian-powerpc-user: EXECUTABLE = 
${BUILD_DIR}/ppc-linux-user/qemu-ppc
+DOCKER_USER_IMAGES += debian-powerpc-user
+else
+docker-binfmt-image-debian-powerpc-user:
+   $(if,$(call quiet-command, \
+   $(DOCKER_SCRIPT) check --quiet qemu:debian-powerpc-user \
+   $(DOCKER_FILES_DIR)/debian-bootstrap.docker, \
+   "CHECK", "debian-powerpc-user exists"),\
+   $(error To build $@ you need a --static ppc-linux-user and 
configured binfmt_misc setup))
+endif
+
+# We build the QEMU compiler environment on top of the base image
+docker-image-debian-powerpc-user-cross: docker-binfmt-image-debian-powerpc-user
+
+
 # Expand all the pre-requistes for each docker image and test combination
 $(foreach i,$(filter-out $(DOCKER_INTERMEDIATE_IMAGES),$(DOCKER_IMAGES) 
$(DOCKER_DEPRECATED_IMAGES)), \
$(foreach t,$(DOCKER_TESTS) $(DOCKER_TOOLS), \
@@ -156,6 +182,11 @@ docker:
@echo
@echo 'Available container images:'
@echo '$(DOCKER_IMAGES)'
+ifneq ($(DOCKER_USER_IMAGES),)
+   @echo
+   @echo 'Available linux-user images (docker-binfmt-image-debian-%):'
+   @echo '$(DOCKER_USER_IMAGES)'
+endif
@echo
@echo 'Available tests:'
@echo '$(DOCKER_TESTS)'
diff --git a/tests/docker/dockerfiles/debian-powerpc-user-cross.docker 
b/tests/docker/dockerfiles/debian-powerpc-user-cross.docker
new file mode 100644
index 00..6938a845ee
--- /dev/null
+++ b/tests/docker/dockerfiles/debian-powerpc-user-cross.docker
@@ -0,0 +1,15 @@
+#
+# Docker powerpc cross-compiler target for QEMU
+#
+# We can't use current Debian stable cross-compilers to build powerpc
+# as it has been dropped as a release architecture. Using Debian Sid
+# is just far too sketchy a build environment. This leaves us the
+# final option of using linux-user. This image is based of the
+# debootstrapped qemu:debian-powerpc-user but doesn't need any extra
+# magic once it is setup.
+#
+FROM qemu:debian-powerpc-user
+
+RUN echo man-db man-db/auto-update boolean false | debconf-set-selections
+RUN apt-get update && \
+DEBIAN_FRONTEND=noninteractive apt-get build-dep -yy qemu
-- 
2.17.1

Re: [Qemu-devel] [PATCH] module: Use QEMU_MODULE_PATH as a search path

[no-reply]

Hi,

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

Type: series
Message-id: 20180702202415.GA12440@computer
Subject: [Qemu-devel] [PATCH] module: Use QEMU_MODULE_PATH as a search path

=== 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
git config --local diff.algorithm histogram

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
Switched to a new branch 'test'
7643dd2b4e module: Use QEMU_MODULE_PATH as a search path

=== OUTPUT BEGIN ===
Checking PATCH 1/1: module: Use QEMU_MODULE_PATH as a search path...
ERROR: braces {} are necessary for all arms of this statement
#63: FILE: util/module.c:191:
+if (search_path != NULL)
[...]

total: 1 errors, 0 warnings, 43 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...@redhat.com

[Qemu-devel] [PULL 11/20] docker: filter out linux-user builds for mingw

[Alex Bennée]

The recent change from TARGET_DIRS to TARGET_LIST (208ecb3e1) had the
effect of defaulting all docker builds to the current configured set
of targets. This is actually reasonable behaviour but does run into
problems if you have linux-user builds configured and you want to test
the windows cross builds. This commit fixes that by adding a
DOCKER_FILTER_TARGETS variable which is special-cased for mingw builds
so we don't pass the whole set down.

Signed-off-by: Alex Bennée 
Cc: Paolo Bonzini 

diff --git a/tests/docker/Makefile.include b/tests/docker/Makefile.include
index 91d9665517..1813ec0781 100644
--- a/tests/docker/Makefile.include
+++ b/tests/docker/Makefile.include
@@ -20,6 +20,9 @@ DOCKER_SCRIPT=$(SRC_PATH)/tests/docker/docker.py
 TESTS ?= %
 IMAGES ?= %
 
+# This is used to filter targets from some docker builds
+DOCKER_FILTER_TARGETS ?=
+
 CUR_TIME := $(shell date +%Y-%m-%d-%H.%M.%S.)
 DOCKER_SRC_COPY := $(BUILD_DIR)/docker-src.$(CUR_TIME)
 
@@ -108,6 +111,12 @@ $(foreach i,$(DOCKER_IMAGES) $(DOCKER_DEPRECATED_IMAGES), \
) \
 )
 
+# Special cases
+#  mingw/windows builds cannot build linux-user
+docker-%-win32-cross: DOCKER_FILTER_TARGETS = %-linux-user
+docker-%-win64-cross: DOCKER_FILTER_TARGETS = %-linux-user
+docker-test-mingw@%: DOCKER_FILTER_TARGETS = %-linux-user
+
 docker:
@echo 'Build QEMU and run tests inside Docker containers'
@echo
@@ -174,7 +183,7 @@ docker-run: docker-qemu-src
$(if $V,,--rm)  \
$(if $(DEBUG),-ti,) \
$(if $(NETWORK),$(if $(subst 
$(NETWORK),,1),--net=$(NETWORK)),--net=none) \
-   -e TARGET_LIST=$(subst 
$(SPACE),$(COMMA),$(TARGET_LIST))\
+   -e TARGET_LIST=$(subst $(SPACE),$(COMMA),$(filter-out 
$(DOCKER_FILTER_TARGETS),$(TARGET_LIST))) \
-e EXTRA_CONFIGURE_OPTS="$(EXTRA_CONFIGURE_OPTS)" \
-e V=$V -e J=$J -e DEBUG=$(DEBUG)   \
-e SHOW_ENV=$(SHOW_ENV) \
@@ -195,7 +204,8 @@ docker-run: docker-qemu-src
 docker-run-%: CMD = $(shell echo '$@' | sed -e 
's/docker-run-\([^@]*\)@\(.*\)/\1/')
 docker-run-%: IMAGE = $(shell echo '$@' | sed -e 
's/docker-run-\([^@]*\)@\(.*\)/\2/')
 docker-run-%:
-   @$(MAKE) docker-run TEST=$(CMD) IMAGE=qemu:$(IMAGE)
+   @$(MAKE) docker-run TEST=$(CMD) IMAGE=qemu:$(IMAGE) 
DOCKER_FILTER_TARGETS=$(DOCKER_FILTER_TARGETS)
+
 
 docker-clean:
$(call quiet-command, $(DOCKER_SCRIPT) clean)
-- 
2.17.1

Re: [Qemu-devel] [PATCH v2 2/2] iotests: New test 223 for exporting dirty bitmap over NBD

[Vladimir Sementsov-Ogievskiy]

03.07.2018 00:27, John Snow wrote:


On 07/02/2018 03:14 PM, Eric Blake wrote:

Although this test is NOT a full test of image fleecing (as it
intentionally uses just a single block device directly exported
over NBD, rather than trying to set up a blockdev-backup job with
multiple BDS involved), it DOES prove that qemu as a server is
able to properly expose a dirty bitmap over NBD.

When coupled with image fleecing, it is then possible for a
third-party client to do an incremental backup by using
qemu-img map with the x-dirty-bitmap option to learn which parts
of the file are dirty (perhaps confusingly, they are the portions
mapped as "data":false - which is part of the reason this is
still in the x- experimental namespace), along with another
normal client (perhaps 'qemu-nbd -c' to expose the server over
/dev/nbd0 and then just use normal I/O on that block device) to
read the dirty sections.

Signed-off-by: Eric Blake 
---
  tests/qemu-iotests/223 | 138 +
  tests/qemu-iotests/223.out |  49 
  tests/qemu-iotests/group   |   1 +
  3 files changed, 188 insertions(+)
  create mode 100755 tests/qemu-iotests/223
  create mode 100644 tests/qemu-iotests/223.out

diff --git a/tests/qemu-iotests/223 b/tests/qemu-iotests/223
new file mode 100755
index 000..b63b7a4f9e1
--- /dev/null
+++ b/tests/qemu-iotests/223
@@ -0,0 +1,138 @@
+#!/bin/bash
+#
+# Test reading dirty bitmap over NBD
+#
+# Copyright (C) 2018 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see .
+#
+
+seq="$(basename $0)"
+echo "QA output created by $seq"
+
+here="$PWD"
+status=1 # failure is the default!
+
+_cleanup()
+{
+_cleanup_test_img
+_cleanup_qemu
+rm -f "$TEST_DIR/nbd"
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+. ./common.qemu
+
+_supported_fmt qcow2
+_supported_proto file # uses NBD as well
+_supported_os Linux
+
+function do_run_qemu()
+{
+echo Testing: "$@"
+$QEMU -nographic -qmp stdio -serial none "$@"
+echo
+}
+
+function run_qemu()
+{
+do_run_qemu "$@" 2>&1 | _filter_testdir | _filter_qmp \
+  | _filter_qemu | _filter_imgfmt \
+  | _filter_actual_image_size
+}
+
+echo
+echo "=== Create partially sparse image, then add dirty bitmap ==="
+echo
+
+_make_test_img 4M
+$QEMU_IO -c 'w -P 0x11 1M 2M' "$TEST_IMG" | _filter_qemu_io

- Write 0x11 from [1M, 3M), not recorded by a bitmap.


+run_qemu <
Saving a few precious bytes.


+"persistent": true
+  }
+}
+{ "execute": "quit" }
+EOF
+
+echo
+echo "=== Write part of the file under active bitmap ==="
+echo
+
+$QEMU_IO -c 'w -P 0x22 2M 2M' "$TEST_IMG" | _filter_qemu_io
+

- Write 0x22 to [2M, 4M) recorded by the bitmap.


+echo
+echo "=== End dirty bitmap, and start serving image over NBD ==="
+echo
+
+_launch_qemu 2> >(_filter_nbd)
+
+silent=
+_send_qemu_cmd $QEMU_HANDLE '{"execute":"qmp_capabilities"}' "return"
+_send_qemu_cmd $QEMU_HANDLE '{"execute":"blockdev-add",
+  "arguments":{"driver":"qcow2", "node-name":"n",
+"file":{"driver":"file", "filename":"'"$TEST_IMG"'"}}}' "return"
+_send_qemu_cmd $QEMU_HANDLE '{"execute":"x-block-dirty-bitmap-disable",
+  "arguments":{"node":"n", "name":"b"}}' "return"
+_send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-start",
+  "arguments":{"addr":{"type":"unix",
+"data":{"path":"'"$TEST_DIR/nbd"'"' "return"
+_send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-add",
+  "arguments":{"device":"n"}}' "return"
+_send_qemu_cmd $QEMU_HANDLE '{"execute":"x-nbd-server-add-bitmap",
+  "arguments":{"name":"n", "bitmap":"b"}}' "return"
+

So far, so good.


+echo
+echo "=== Contrast normal status with dirty-bitmap status ==="
+echo
+
+QEMU_IO_OPTIONS=$QEMU_IO_OPTIONS_NO_FMT
+IMG="driver=nbd,export=n,server.type=unix,server.path=$TEST_DIR/nbd"
+$QEMU_IO -r -c 'r -P 0 0 1m' -c 'r -P 0x11 1m 1m' \
+  -c 'r -P 0x22 2m 2m' --image-opts "$IMG" | _filter_qemu_io

Confirming that we've got 0x11 from [1M, 2M) and 0x22 from [2M, 4M).


+$QEMU_IMG map --output=json --image-opts \
+  "$IMG" | _filter_qemu_img_map

Normal allocation map. Ought to show [1M, 4M).


+$QEMU_IMG map --output=json --image-opts \
+  "$IMG,x-dirty-bitmap=qemu:dirty-bitmap:b" | _filter_qemu_img_map

Hacked bitmap allocation map. Ought to show [2M, 4M).


+
+echo

[Qemu-devel] [PULL 16/20] docker: drop QEMU build-dep from bootstrap

[Alex Bennée]

This is best done with any child images that actually need it.

Signed-off-by: Alex Bennée 
Reviewed-by: Philippe Mathieu-Daudé 
Tested-by: Philippe Mathieu-Daudé 

diff --git a/tests/docker/dockerfiles/debian-bootstrap.docker 
b/tests/docker/dockerfiles/debian-bootstrap.docker
index 3a9125e497..14212b9cf4 100644
--- a/tests/docker/dockerfiles/debian-bootstrap.docker
+++ b/tests/docker/dockerfiles/debian-bootstrap.docker
@@ -17,5 +17,3 @@ RUN /debootstrap/debootstrap --second-stage
 # At this point we can install additional packages if we want
 # Duplicate deb line as deb-src
 RUN cat /etc/apt/sources.list | sed "s/deb/deb-src/" >> /etc/apt/sources.list
-RUN apt-get update
-RUN apt-get -y build-dep qemu
-- 
2.17.1

Re: [Qemu-devel] [PATCH] qga: report disk size and free space

[Tomáš Golembiovský]

On Tue, 3 Jul 2018 12:01:55 +0200
Marc-André Lureau  wrote:

> Hi
> 
> On Tue, Jul 3, 2018 at 10:31 AM, Tomáš Golembiovský  
> wrote:
> > Report total file system size and free space in output of command
> > "guest-get-fsinfo". Values are optional and it is not an error if they 
> > cannot
> > be retrieved for some reason.
> >  
> 
> I am afraid this will miss 3.0, since it's a new feature

I kind of expected this.

> > Signed-off-by: Tomáš Golembiovský 
> > ---
> >  qga/commands-posix.c | 18 ++
> >  qga/commands-win32.c | 16 
> >  qga/qapi-schema.json |  5 -
> >  3 files changed, 38 insertions(+), 1 deletion(-)
> >
> > diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> > index eae817191b..1f2fb25b91 100644
> > --- a/qga/commands-posix.c
> > +++ b/qga/commands-posix.c
> > @@ -13,6 +13,7 @@
> >
> >  #include "qemu/osdep.h"
> >  #include 
> > +#include 
> >  #include 
> >  #include 
> >  #include 
> > @@ -1074,11 +1075,28 @@ static GuestFilesystemInfo 
> > *build_guest_fsinfo(struct FsMount *mount,
> >  GuestFilesystemInfo *fs = g_malloc0(sizeof(*fs));
> >  char *devpath = g_strdup_printf("/sys/dev/block/%u:%u",
> >  mount->devmajor, mount->devminor);
> > +struct statvfs vfs_stats;
> >
> >  fs->mountpoint = g_strdup(mount->dirname);
> >  fs->type = g_strdup(mount->devtype);
> >  build_guest_fsinfo_for_device(devpath, fs, errp);
> >
> > +g_debug(" get filesystem statistics for '%s'", mount->dirname);
> > +if (statvfs(mount->dirname, &vfs_stats) != 0) {
> > +/* This is not fatal, just log this incident */
> > +Error *local_err = NULL;
> > +error_setg_errno(&local_err, errno, "statfs(\"%s\")",  
> 
> statvfs()
> 
> > +mount->dirname);
> > +slog("failed to stat filesystem: %s",
> > +error_get_pretty(local_err));
> > +error_free(local_err);  
> 
> The usage of Error is a bit overkill, perhaps you may juste use slog()
> / strerror() directly.
> 
> > +} else {
> > +fs->size = vfs_stats.f_frsize * vfs_stats.f_blocks;
> > +fs->has_size = true;
> > +fs->free = vfs_stats.f_frsize * vfs_stats.f_bfree;
> > +fs->has_free = true;
> > +}
> > +
> >  g_free(devpath);
> >  return fs;
> >  }
> > diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> > index 70ee5379f6..6d6ca05281 100644
> > --- a/qga/commands-win32.c
> > +++ b/qga/commands-win32.c
> > @@ -706,6 +706,22 @@ static GuestFilesystemInfo *build_guest_fsinfo(char 
> > *guid, Error **errp)
> >  }
> >  fs->type = g_strdup(fs_name);
> >  fs->disk = build_guest_disk_info(guid, errp);
> > +
> > +if (len > 0) {
> > +if (GetDiskFreeSpaceEx(mnt_point, 0, (PULARGE_INTEGER)&fs->size,
> > +(PULARGE_INTEGER)&fs->free) != 0) {  
> 
> I would rather use some local ULARGE_INTEGER and access the QuadPart
> to avoid the cast, but it should work.

I tried that, but the returned values were nonsensical. Maybe I did
something wrong along the way. I'll give it one more try.

Thanks for quick response,

Tomas

> 
> > +/* This is not fatal, just log this incident */
> > +Error *local_err = NULL;
> > +error_setg_win32(&local_err, GetLastError(),
> > +"failed to get free space on volume \"%s\"", mnt_point);
> > +slog("%s", error_get_pretty(local_err));
> > +error_free(local_err);
> > +} else {
> > +fs->has_size = true;
> > +fs->has_free = true;
> > +}
> > +}
> > +
> >  free:
> >  g_free(mnt_point);
> >  return fs;
> > diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
> > index 17884c7c70..28a32444d3 100644
> > --- a/qga/qapi-schema.json
> > +++ b/qga/qapi-schema.json
> > @@ -848,12 +848,15 @@
> >  # @type: file system type string
> >  # @disk: an array of disk hardware information that the volume lies on,
> >  #which may be empty if the disk type is not supported
> > +# @size: total number of bytes on the file system (Since 2.13)
> > +# @free: number of bytes available on the file system (Since 2.13)  
> 
> "Since 3.1", most probably
> 
> >  #
> >  # Since: 2.2
> >  ##
> >  { 'struct': 'GuestFilesystemInfo',
> >'data': {'name': 'str', 'mountpoint': 'str', 'type': 'str',
> > -   'disk': ['GuestDiskAddress']} }
> > +   'disk': ['GuestDiskAddress'], '*size': 'uint64',
> > +   '*free': 'uint64'} }
> >
> >  ##
> >  # @guest-get-fsinfo:
> > --
> > 2.17.1
> >
> >  
> 
> 
> 
> -- 
> Marc-André Lureau


-- 
Tomáš Golembiovský 

Re: [Qemu-devel] [PATCH v3 13/20] kvm: arm64: Configure VTCR per VM

[Suzuki K Poulose]

On 02/07/18 13:16, Marc Zyngier wrote:

On 29/06/18 12:15, Suzuki K Poulose wrote:

We set VTCR_EL2 very early during the stage2 init and don't
touch it ever. This is fine as we had a fixed IPA size. This
patch changes the behavior to set the VTCR for a given VM,
depending on its stage2 table. The common configuration for
VTCR is still performed during the early init as we have to
retain the hardware access flag update bits (VTCR_EL2_HA)
per CPU (as they are only set for the CPUs which are capabile).


capable


The bits defining the number of levels in the page table (SL0)
and and the size of the Input address to the translation (T0SZ)
are programmed for each VM upon entry to the guest.

Cc: Marc Zyngier 
Cc: Christoffer Dall 
Signed-off-by: Suzuki K Poulose 
---
Change since V2:
  - Load VTCR for TLB operations
---
  arch/arm64/include/asm/kvm_arm.h  | 19 +--
  arch/arm64/include/asm/kvm_asm.h  |  2 +-
  arch/arm64/include/asm/kvm_host.h |  9 ++---
  arch/arm64/include/asm/kvm_hyp.h  | 11 +++
  arch/arm64/kvm/hyp/s2-setup.c | 17 +
  5 files changed, 28 insertions(+), 30 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_arm.h b/arch/arm64/include/asm/kvm_arm.h
index 11a7db0..b02c316 100644
--- a/arch/arm64/include/asm/kvm_arm.h
+++ b/arch/arm64/include/asm/kvm_arm.h
@@ -120,9 +120,7 @@
  #define VTCR_EL2_IRGN0_WBWA   TCR_IRGN0_WBWA
  #define VTCR_EL2_SL0_SHIFT6
  #define VTCR_EL2_SL0_MASK (3 << VTCR_EL2_SL0_SHIFT)
-#define VTCR_EL2_SL0_LVL1  (1 << VTCR_EL2_SL0_SHIFT)
  #define VTCR_EL2_T0SZ_MASK0x3f
-#define VTCR_EL2_T0SZ_40B  24
  #define VTCR_EL2_VS_SHIFT 19
  #define VTCR_EL2_VS_8BIT  (0 << VTCR_EL2_VS_SHIFT)
  #define VTCR_EL2_VS_16BIT (1 << VTCR_EL2_VS_SHIFT)
@@ -137,43 +135,44 @@
   * VTCR_EL2.PS is extracted from ID_AA64MMFR0_EL1.PARange at boot time
   * (see hyp-init.S).
   *
+ * VTCR_EL2.SL0 and T0SZ are configured per VM at runtime before switching to
+ * the VM.
+ *
   * Note that when using 4K pages, we concatenate two first level page tables
   * together. With 16K pages, we concatenate 16 first level page tables.
   *
   */
  
-#define VTCR_EL2_T0SZ_IPA	VTCR_EL2_T0SZ_40B

  #define VTCR_EL2_COMMON_BITS  (VTCR_EL2_SH0_INNER | VTCR_EL2_ORGN0_WBWA | \
 VTCR_EL2_IRGN0_WBWA | VTCR_EL2_RES1)
+#define VTCR_EL2_PRIVATE_MASK  (VTCR_EL2_SL0_MASK | VTCR_EL2_T0SZ_MASK)


What does "private" mean here? It really is the IPA configuration, so
I'd rather have a naming that reflects that.


  #ifdef CONFIG_ARM64_64K_PAGES
  /*
   * Stage2 translation configuration:
   * 64kB pages (TG0 = 1)
- * 2 level page tables (SL = 1)
   */
-#define VTCR_EL2_TGRAN_FLAGS   (VTCR_EL2_TG0_64K | VTCR_EL2_SL0_LVL1)
+#define VTCR_EL2_TGRAN VTCR_EL2_TG0_64K
  #define VTCR_EL2_TGRAN_SL0_BASE   3UL
  
  #elif defined(CONFIG_ARM64_16K_PAGES)

  /*
   * Stage2 translation configuration:
   * 16kB pages (TG0 = 2)
- * 2 level page tables (SL = 1)
   */
-#define VTCR_EL2_TGRAN_FLAGS   (VTCR_EL2_TG0_16K | VTCR_EL2_SL0_LVL1)
+#define VTCR_EL2_TGRAN VTCR_EL2_TG0_16K
  #define VTCR_EL2_TGRAN_SL0_BASE   3UL
  #else /* 4K */
  /*
   * Stage2 translation configuration:
   * 4kB pages (TG0 = 0)
- * 3 level page tables (SL = 1)
   */
-#define VTCR_EL2_TGRAN_FLAGS   (VTCR_EL2_TG0_4K | VTCR_EL2_SL0_LVL1)
+#define VTCR_EL2_TGRAN VTCR_EL2_TG0_4K
  #define VTCR_EL2_TGRAN_SL0_BASE   2UL
  #endif
  
-#define VTCR_EL2_FLAGS			(VTCR_EL2_COMMON_BITS | VTCR_EL2_TGRAN_FLAGS)

+#define VTCR_EL2_FLAGS (VTCR_EL2_COMMON_BITS | VTCR_EL2_TGRAN)
+
  /*
   * VTCR_EL2:SL0 indicates the entry level for Stage2 translation.
   * Interestingly, it depends on the page size.
diff --git a/arch/arm64/include/asm/kvm_asm.h b/arch/arm64/include/asm/kvm_asm.h
index 102b5a5..91372eb 100644
--- a/arch/arm64/include/asm/kvm_asm.h
+++ b/arch/arm64/include/asm/kvm_asm.h
@@ -72,7 +72,7 @@ extern void __vgic_v3_init_lrs(void);
  
  extern u32 __kvm_get_mdcr_el2(void);
  
-extern u32 __init_stage2_translation(void);

+extern void __init_stage2_translation(void);
  
  /* Home-grown __this_cpu_{ptr,read} variants that always work at HYP */

  #define __hyp_this_cpu_ptr(sym)   
\
diff --git a/arch/arm64/include/asm/kvm_host.h 
b/arch/arm64/include/asm/kvm_host.h
index fe8777b..328f472 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -442,10 +442,13 @@ int kvm_arm_vcpu_arch_has_attr(struct kvm_vcpu *vcpu,
  
  static inline void __cpu_init_stage2(void)

  {
-   u32 parange = kvm_call_hyp(__init_stage2_translation);
+   u32 ps;
  
-	WARN_ONCE(parange < 40,

- "PARange is %d bits, unsupported configuration!", parange);
+   kvm_call_hyp(__init_stage2_translation);
+   /* Sanity check for minimum IPA size support */
+   ps = id_aa64mmfr0_par

Re: [Qemu-devel] [PULL 0/6] NBD patches for 2018-07-02 (3.0 soft freeze)

[Peter Maydell]

On 3 July 2018 at 02:37, Eric Blake  wrote:
> The following changes since commit ab08440a4ee09032d1a9cb22fdcab23bc7e1c656:
>
>   Merge remote-tracking branch 'remotes/rth/tags/pull-tcg-20180702' into 
> staging (2018-07-02 17:57:46 +0100)
>
> are available in the Git repository at:
>
>   git://repo.or.cz/qemu/ericb.git tags/pull-nbd-2018-07-02
>
> for you to fetch changes up to a1532a225a183c9fa60b9c1e8ac8a00c7771f64d:
>
>   iotests: New test 223 for exporting dirty bitmap over NBD (2018-07-02 
> 19:50:37 -0500)
>
> Discussed but not included here: we need to ensure that during a
> backup sync:none job, writes must NOT be permitted to the fleecing
> node's backing file as long as reads are being served from the same
> area. Hopefully we can fix this via the existing job, instead of
> having to add a new filter node.
>
> 
> nbd patches for 2018-07-02
>
> Bug fixes and iotest exposure of fleecing via NBD (serving a
> read-only point-in-time view via blockdev-backup sync:none,
> as well as serving dirty bitmaps over NBD), including a new
> x-dirty-bitmap parameter when opening NBD clients as the
> counterpart to x-nbd-server-add-bitmap. Also a random fix
> for iscsi block_status spotted by Coverity that missed other
> miscellaneous trees.
>
> - Eric Blake: nbd/server: Fix dirty bitmap logic regression
> - Eric Blake: iscsi: Avoid potential for get_status overflow
> - John Snow/Vladimir Sementsov-Ogievskiy: 0/2 block: formalize and test 
> fleecing
> - Eric Blake: 0/2 test NBD bitmap export
>

Applied, thanks.

-- PMM

[Qemu-devel] [PATCH 0/2] transaction support for bitmap merge

[Vladimir Sementsov-Ogievskiy]

This is a last brick, necessary to play with nbd bitmap export in
conjunction with image fleecing.

Vladimir Sementsov-Ogievskiy (2):
  drity-bitmap: refactor merge: separte can_merge
  qapi: add transaction support for x-block-dirty-bitmap-merge

 qapi/transaction.json|  2 ++
 include/block/dirty-bitmap.h |  3 +++
 include/qemu/hbitmap.h   |  8 ++
 block/dirty-bitmap.c | 32 ++
 blockdev.c   | 63 
 util/hbitmap.c   |  6 -
 6 files changed, 97 insertions(+), 17 deletions(-)

-- 
2.11.1

[Qemu-devel] [PATCH 1/2] drity-bitmap: refactor merge: separte can_merge

[Vladimir Sementsov-Ogievskiy]

Separate can_merge, to reuse it for transaction support for merge
command.

Also, switch some asserts to errors.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 include/block/dirty-bitmap.h |  3 +++
 include/qemu/hbitmap.h   |  8 
 block/dirty-bitmap.c | 32 +++-
 blockdev.c   | 10 --
 util/hbitmap.c   |  6 +-
 5 files changed, 43 insertions(+), 16 deletions(-)

diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 288dc6adb6..412a333c02 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -71,6 +71,9 @@ void bdrv_dirty_bitmap_set_persistance(BdrvDirtyBitmap 
*bitmap,
 void bdrv_dirty_bitmap_set_qmp_locked(BdrvDirtyBitmap *bitmap, bool 
qmp_locked);
 void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src,
  Error **errp);
+bool bdrv_can_merge_dirty_bitmap(BdrvDirtyBitmap *dst,
+ const BdrvDirtyBitmap *src,
+ Error **errp);
 
 /* Functions that require manual locking.  */
 void bdrv_dirty_bitmap_lock(BdrvDirtyBitmap *bitmap);
diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h
index ddca52c48e..d08bc20ea3 100644
--- a/include/qemu/hbitmap.h
+++ b/include/qemu/hbitmap.h
@@ -85,6 +85,14 @@ void hbitmap_truncate(HBitmap *hb, uint64_t size);
 bool hbitmap_merge(HBitmap *a, const HBitmap *b);
 
 /**
+ * hbitmap_can_merge:
+ *
+ * Returns same value as hbitmap_merge, but do not do actual merge.
+ *
+ */
+bool hbitmap_can_merge(HBitmap *a, const HBitmap *b);
+
+/**
  * hbitmap_empty:
  * @hb: HBitmap to operate on.
  *
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index db1782ec1f..1137224aaa 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -784,6 +784,30 @@ int64_t bdrv_dirty_bitmap_next_zero(BdrvDirtyBitmap 
*bitmap, uint64_t offset)
 return hbitmap_next_zero(bitmap->bitmap, offset);
 }
 
+bool bdrv_can_merge_dirty_bitmap(BdrvDirtyBitmap *dst,
+ const BdrvDirtyBitmap *src,
+ Error **errp)
+{
+if (bdrv_dirty_bitmap_frozen(dst)) {
+error_setg(errp, "Bitmap '%s' is frozen and cannot be modified",
+   dst->name);
+return false;
+}
+
+if (bdrv_dirty_bitmap_readonly(dst)) {
+error_setg(errp, "Bitmap '%s' is readonly and cannot be modified",
+   dst->name);
+return false;
+}
+
+if (!hbitmap_can_merge(dst->bitmap, src->bitmap)) {
+error_setg(errp, "Bitmaps are incompatible and can't be merged");
+return false;
+}
+
+return true;
+}
+
 void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src,
  Error **errp)
 {
@@ -792,11 +816,9 @@ void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const 
BdrvDirtyBitmap *src,
 
 qemu_mutex_lock(dest->mutex);
 
-assert(bdrv_dirty_bitmap_enabled(dest));
-assert(!bdrv_dirty_bitmap_readonly(dest));
-
-if (!hbitmap_merge(dest->bitmap, src->bitmap)) {
-error_setg(errp, "Bitmaps are incompatible and can't be merged");
+if (bdrv_can_merge_dirty_bitmap(dest, src, errp)) {
+bool ret = hbitmap_merge(dest->bitmap, src->bitmap);
+assert(ret);
 }
 
 qemu_mutex_unlock(dest->mutex);
diff --git a/blockdev.c b/blockdev.c
index 58d7570932..63c4d33124 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2955,16 +2955,6 @@ void qmp_x_block_dirty_bitmap_merge(const char *node, 
const char *dst_name,
 return;
 }
 
-if (bdrv_dirty_bitmap_frozen(dst)) {
-error_setg(errp, "Bitmap '%s' is frozen and cannot be modified",
-   dst_name);
-return;
-} else if (bdrv_dirty_bitmap_readonly(dst)) {
-error_setg(errp, "Bitmap '%s' is readonly and cannot be modified",
-   dst_name);
-return;
-}
-
 src = bdrv_find_dirty_bitmap(bs, src_name);
 if (!src) {
 error_setg(errp, "Dirty bitmap '%s' not found", src_name);
diff --git a/util/hbitmap.c b/util/hbitmap.c
index bcd304041a..b56377b043 100644
--- a/util/hbitmap.c
+++ b/util/hbitmap.c
@@ -723,6 +723,10 @@ void hbitmap_truncate(HBitmap *hb, uint64_t size)
 }
 }
 
+bool hbitmap_can_merge(HBitmap *a, const HBitmap *b)
+{
+return (a->size == b->size) && (a->granularity == b->granularity);
+}
 
 /**
  * Given HBitmaps A and B, let A := A (BITOR) B.
@@ -736,7 +740,7 @@ bool hbitmap_merge(HBitmap *a, const HBitmap *b)
 int i;
 uint64_t j;
 
-if ((a->size != b->size) || (a->granularity != b->granularity)) {
+if (!hbitmap_can_merge(a, b)) {
 return false;
 }
 
-- 
2.11.1

Re: [Qemu-devel] [libvirt] [PULL 25/26] block: Remove deprecated -drive option serial

[Christian Borntraeger]

On 07/02/2018 10:04 AM, Kevin Wolf wrote:
> Am 25.06.2018 um 13:45 hat Peter Krempa geschrieben:
>> On Mon, Jun 25, 2018 at 13:41:06 +0200, Kevin Wolf wrote:
>>> Am 25.06.2018 um 11:53 hat Daniel P. Berrangé geschrieben:
 On Fri, Jun 22, 2018 at 03:31:46PM +0100, Daniel P. Berrangé wrote:
> On Fri, Jun 22, 2018 at 04:25:13PM +0200, Kevin Wolf wrote:
>> Am 22.06.2018 um 15:36 hat Christian Borntraeger geschrieben:
>>
>> [...]
>>
>>>
>>> Thanks!
>>>
>>> I'll look into werror/rerror support for usb-storage. It shouldn't be
>>> too hard, though it's strictly speaking a separate problem related to
>>> using -blockdev rather than option deprecation.
>>>
>>> If Peter wants to wait for QEMU support before converting werror/rerror
>>
>> Definitely. I don't want to keep around yet another hack that will
>> satisfy one specific case and then add another capability for it. We
>> should then gate the moving of the feature based on the presence of
>> werror for usb-storage.
>>
>>> to -device, maybe it would make sense to split your patch for v2 so that
>>> geometry and serial can get fixed right away?
>>
>> Yes this can be done right away.
> 
> Has serial/gemoetry been fixed meanwhile and will it make it into the
> next release?

I cannot find an archive that has it, but it is on the libvirt mailing
list as "[libvirt] [PATCH v3] qemu: format serial and geometry on frontend disk 
device".
Review seems done, but it has missed libvirt 4.5 which was released today. 

Christian

[Qemu-devel] [PATCH 2/2] qapi: add transaction support for x-block-dirty-bitmap-merge

[Vladimir Sementsov-Ogievskiy]

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 qapi/transaction.json |  2 ++
 blockdev.c| 53 ++-
 2 files changed, 54 insertions(+), 1 deletion(-)

diff --git a/qapi/transaction.json b/qapi/transaction.json
index d7e4274550..f9da6ad47f 100644
--- a/qapi/transaction.json
+++ b/qapi/transaction.json
@@ -48,6 +48,7 @@
 # - @block-dirty-bitmap-clear: since 2.5
 # - @x-block-dirty-bitmap-enable: since 3.0
 # - @x-block-dirty-bitmap-disable: since 3.0
+# - @x-block-dirty-bitmap-merge: since 3.0
 # - @blockdev-backup: since 2.3
 # - @blockdev-snapshot: since 2.5
 # - @blockdev-snapshot-internal-sync: since 1.7
@@ -63,6 +64,7 @@
'block-dirty-bitmap-clear': 'BlockDirtyBitmap',
'x-block-dirty-bitmap-enable': 'BlockDirtyBitmap',
'x-block-dirty-bitmap-disable': 'BlockDirtyBitmap',
+   'x-block-dirty-bitmap-merge': 'BlockDirtyBitmapMerge',
'blockdev-backup': 'BlockdevBackup',
'blockdev-snapshot': 'BlockdevSnapshot',
'blockdev-snapshot-internal-sync': 'BlockdevSnapshotInternal',
diff --git a/blockdev.c b/blockdev.c
index 63c4d33124..d0f2d1a4e9 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1946,6 +1946,13 @@ typedef struct BlockDirtyBitmapState {
 bool was_enabled;
 } BlockDirtyBitmapState;
 
+typedef struct BlockDirtyBitmapMergeState {
+BlkActionState common;
+BlockDriverState *bs;
+BdrvDirtyBitmap *dst;
+BdrvDirtyBitmap *src;
+} BlockDirtyBitmapMergeState;
+
 static void block_dirty_bitmap_add_prepare(BlkActionState *common,
Error **errp)
 {
@@ -2112,6 +2119,45 @@ static void 
block_dirty_bitmap_disable_abort(BlkActionState *common)
 }
 }
 
+static void block_dirty_bitmap_merge_prepare(BlkActionState *common,
+ Error **errp)
+{
+BlockDirtyBitmapMerge *action;
+BlockDirtyBitmapMergeState *state = DO_UPCAST(BlockDirtyBitmapMergeState,
+  common, common);
+
+if (action_check_completion_mode(common, errp) < 0) {
+return;
+}
+
+action = common->action->u.x_block_dirty_bitmap_merge.data;
+state->dst = block_dirty_bitmap_lookup(action->node,
+   action->dst_name,
+   &state->bs,
+   errp);
+if (!state->dst) {
+return;
+}
+
+state->src = bdrv_find_dirty_bitmap(state->bs, action->src_name);
+if (!state->src) {
+return;
+}
+
+if (!bdrv_can_merge_dirty_bitmap(state->dst, state->src, errp)) {
+return;
+}
+}
+
+static void block_dirty_bitmap_merge_commit(BlkActionState *common)
+{
+BlockDirtyBitmapMergeState *state = DO_UPCAST(BlockDirtyBitmapMergeState,
+  common, common);
+
+/* success is guaranteed by bdrv_can_merge_dirty_bitmap() */
+bdrv_merge_dirty_bitmap(state->dst, state->src, &error_abort);
+}
+
 static void abort_prepare(BlkActionState *common, Error **errp)
 {
 error_setg(errp, "Transaction aborted using Abort action");
@@ -2182,7 +2228,12 @@ static const BlkActionOps actions[] = {
 .instance_size = sizeof(BlockDirtyBitmapState),
 .prepare = block_dirty_bitmap_disable_prepare,
 .abort = block_dirty_bitmap_disable_abort,
- }
+},
+[TRANSACTION_ACTION_KIND_X_BLOCK_DIRTY_BITMAP_MERGE] = {
+.instance_size = sizeof(BlockDirtyBitmapMergeState),
+.prepare = block_dirty_bitmap_merge_prepare,
+.commit = block_dirty_bitmap_merge_commit,
+}
 };
 
 /**
-- 
2.11.1

[Qemu-devel] [Bug 1776478] Re: Getting qemu: uncaught target signal 6 when running lv2 plugin cross-compilation

[guysoft]

Hey,
So how do I enable -g option?
The command that activates qemu is:

sudo chroot .

How do I set -g?

Also minor update - the new rasbian image (aka updated g++ libs) makes
lv2_ttl_generator hang on:

Writing drowaudio-flanger.ttl... done!
Generate ttl data for './drowaudio-tremolo.so', basename: 'drowaudio-tremolo'

So definitely something is strange with how lv2_ttl_generator runs the
.so files provided to it.

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

Title:
  Getting qemu: uncaught target signal 6 when running  lv2 plugin cross-
  compilation

Status in QEMU:
  New

Bug description:
  Hey,
  I am part of the Zynthian team and we use qemu-arm-static to cross compile 
lv2 audio plugins.

  When running a compilation of DISTRHO-Ports we get:

  lv2_ttl_generator: pthread_mutex_lock.c:81: __pthread_mutex_lock: Assertion 
`mutex->__data.__owner == 0' failed.
  qemu: uncaught target signal 6 (Aborted) - core dumped
  ./scripts/generate-ttl.sh: line 27: 16524 Aborted $GEN ./$FILE
  Makefile:62: recipe for target 'gen_lv2' failed
  make[1]: *** [gen_lv2] Error 134
  make[1]: Leaving directory '/home/pi/zynthian-sw/plugins/DISTRHO-Ports'
  Makefile:104: recipe for target 'lv2' failed
  make: *** [lv2] Error 2

  
  lv2_ttl_generator source is here:
  https://github.com/DISTRHO/DISTRHO-Ports/tree/master/libs/lv2-ttl-generator

  The command that is ruining is
  lv2_ttl_generator ./TAL-Filter-2.so 

  And ./TAL-Filter-2.so source is here:
  https://github.com/DISTRHO/DISTRHO-Ports/tree/master/ports/tal-filter-2/source


  Is there a way to debug what is going on?
  This runs fine on a Raspberrypi which is armv7

  A workaround would also help.

  
  Bug in Zynthian:
  https://github.com/zynthian/zynthian-sys/issues/59
  Bug in DISTRHO-Ports:
  https://github.com/DISTRHO/DISTRHO-Ports/issues/29

  Using qemu-arm-static version from master from two days ago:
  qemu-arm version 2.12.50 (v2.12.0-1182-ga7a7309ca5-dirty), commit: 
a7a7309ca52c327c6603d60db90ae4feeae719f7

  Also saw this in qemu-arm version 2.12.0 (Debian 1:2.12+dfsg-3)

  Thanks,
  Guy

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

Re: [Qemu-devel] [PATCH] qga: report disk size and free space

[Tomáš Golembiovský]

On Tue,  3 Jul 2018 10:31:20 +0200
Tomáš Golembiovský  wrote:

> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> index 70ee5379f6..6d6ca05281 100644
> --- a/qga/commands-win32.c
> +++ b/qga/commands-win32.c
> @@ -706,6 +706,22 @@ static GuestFilesystemInfo *build_guest_fsinfo(char 
> *guid, Error **errp)
>  }
>  fs->type = g_strdup(fs_name);
>  fs->disk = build_guest_disk_info(guid, errp);
> +
> +if (len > 0) {
> +if (GetDiskFreeSpaceEx(mnt_point, 0, (PULARGE_INTEGER)&fs->size,
> +(PULARGE_INTEGER)&fs->free) != 0) {

Btw, it seems to me that checkpatch.pl returns some false positives
here. See:

ERROR: spaces required around that '&' (ctx:VxV)


#72: FILE: qga/commands-win32.c:711:
+if (GetDiskFreeSpaceEx(mnt_point, 0, (PULARGE_INTEGER)&fs->size,
^

ERROR: spaces required around that '&' (ctx:VxV)
#73: FILE: qga/commands-win32.c:712:
+(PULARGE_INTEGER)&fs->free) != 0) {
^

Seems that & is mistakenly interpreted as bitwise-AND operator. 
Or am I expected to format that as

  ..., (PULARGE_INTEGER) & fs->size, ...

because that looks... weird.


Also, checkpatch.pl itself suggest to look into MAINTAINERS to find out
whom to report false positives, but there is no maintainer there. :)

Tomas


-- 
Tomáš Golembiovský 

Re: [Qemu-devel] [RFC v2] arm: Add NRF51 SOC non-volatile memory controller

[Steffen Görtz]

> Is the flash memory region only accessible to the CPU
> via the NVMC, or can the CPU get at it both directly
> and via the NVMC? (That is, in hardware, does the flash
> sit "behind" the NVMC?)
> 
> thanks
> -- PMM
> 

The latter is the case.

Steffen

Re: [Qemu-devel] [PATCH V9 11/20] qapi/migration.json: Rename COLO unknown mode to none mode.

[Markus Armbruster]

Zhang Chen  writes:

> From: Zhang Chen 
>
> Suggested by Markus Armbruster rename COLO unknown mode to none mode.
>
> Signed-off-by: Zhang Chen 
> ---
>  migration/colo-failover.c |  2 +-
>  migration/colo.c  |  2 +-
>  qapi/migration.json   | 10 +-
>  3 files changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/migration/colo-failover.c b/migration/colo-failover.c
> index 0ae0c41221..4854a96c92 100644
> --- a/migration/colo-failover.c
> +++ b/migration/colo-failover.c
> @@ -77,7 +77,7 @@ FailoverStatus failover_get_state(void)
>  
>  void qmp_x_colo_lost_heartbeat(Error **errp)
>  {
> -if (get_colo_mode() == COLO_MODE_UNKNOWN) {
> +if (get_colo_mode() == COLO_MODE_NONE) {
>  error_setg(errp, QERR_FEATURE_DISABLED, "colo");
>  return;
>  }
> diff --git a/migration/colo.c b/migration/colo.c
> index ab484ad754..8fdb79ac73 100644
> --- a/migration/colo.c
> +++ b/migration/colo.c
> @@ -160,7 +160,7 @@ COLOMode get_colo_mode(void)
>  } else if (migration_incoming_in_colo_state()) {
>  return COLO_MODE_SECONDARY;
>  } else {
> -return COLO_MODE_UNKNOWN;
> +return COLO_MODE_NONE;
>  }
>  }
>  
> diff --git a/qapi/migration.json b/qapi/migration.json
> index d8c3b2e443..c24f114104 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -864,18 +864,18 @@
>  ##
>  # @COLOMode:
>  #
> -# The colo mode
> +# The COLO current mode.
>  #
> -# @unknown: unknown mode
> +# @none: None mode.

This could use a bit of polish.  Perhaps:

   # @none: COLO is disabled.

>  #
> -# @primary: master side
> +# @primary: COLO node in primary side.
>  #
> -# @secondary: slave side
> +# @secondary: COLO node in slave side.
>  #
>  # Since: 2.8
>  ##
>  { 'enum': 'COLOMode',
> -  'data': [ 'unknown', 'primary', 'secondary'] }
> +  'data': [ 'none', 'primary', 'secondary'] }
>  
>  ##
>  # @FailoverStatus:

Preferably with the comment improved:
Reviewed-by: Markus Armbruster 

Re: [Qemu-devel] [PATCH v3 13/20] kvm: arm64: Configure VTCR per VM

[Marc Zyngier]

On 03/07/18 11:48, Suzuki K Poulose wrote:
> On 02/07/18 13:16, Marc Zyngier wrote:
>> On 29/06/18 12:15, Suzuki K Poulose wrote:
>>> We set VTCR_EL2 very early during the stage2 init and don't
>>> touch it ever. This is fine as we had a fixed IPA size. This
>>> patch changes the behavior to set the VTCR for a given VM,
>>> depending on its stage2 table. The common configuration for
>>> VTCR is still performed during the early init as we have to
>>> retain the hardware access flag update bits (VTCR_EL2_HA)
>>> per CPU (as they are only set for the CPUs which are capabile).
>>
>> capable
>>
>>> The bits defining the number of levels in the page table (SL0)
>>> and and the size of the Input address to the translation (T0SZ)
>>> are programmed for each VM upon entry to the guest.
>>>
>>> Cc: Marc Zyngier 
>>> Cc: Christoffer Dall 
>>> Signed-off-by: Suzuki K Poulose 
>>> ---
>>> Change since V2:
>>>   - Load VTCR for TLB operations
>>> ---
>>>   arch/arm64/include/asm/kvm_arm.h  | 19 +--
>>>   arch/arm64/include/asm/kvm_asm.h  |  2 +-
>>>   arch/arm64/include/asm/kvm_host.h |  9 ++---
>>>   arch/arm64/include/asm/kvm_hyp.h  | 11 +++
>>>   arch/arm64/kvm/hyp/s2-setup.c | 17 +
>>>   5 files changed, 28 insertions(+), 30 deletions(-)
>>>
>>> diff --git a/arch/arm64/include/asm/kvm_arm.h 
>>> b/arch/arm64/include/asm/kvm_arm.h
>>> index 11a7db0..b02c316 100644
>>> --- a/arch/arm64/include/asm/kvm_arm.h
>>> +++ b/arch/arm64/include/asm/kvm_arm.h
>>> @@ -120,9 +120,7 @@
>>>   #define VTCR_EL2_IRGN0_WBWA   TCR_IRGN0_WBWA
>>>   #define VTCR_EL2_SL0_SHIFT6
>>>   #define VTCR_EL2_SL0_MASK (3 << VTCR_EL2_SL0_SHIFT)
>>> -#define VTCR_EL2_SL0_LVL1  (1 << VTCR_EL2_SL0_SHIFT)
>>>   #define VTCR_EL2_T0SZ_MASK0x3f
>>> -#define VTCR_EL2_T0SZ_40B  24
>>>   #define VTCR_EL2_VS_SHIFT 19
>>>   #define VTCR_EL2_VS_8BIT  (0 << VTCR_EL2_VS_SHIFT)
>>>   #define VTCR_EL2_VS_16BIT (1 << VTCR_EL2_VS_SHIFT)
>>> @@ -137,43 +135,44 @@
>>>* VTCR_EL2.PS is extracted from ID_AA64MMFR0_EL1.PARange at boot time
>>>* (see hyp-init.S).
>>>*
>>> + * VTCR_EL2.SL0 and T0SZ are configured per VM at runtime before switching 
>>> to
>>> + * the VM.
>>> + *
>>>* Note that when using 4K pages, we concatenate two first level page 
>>> tables
>>>* together. With 16K pages, we concatenate 16 first level page tables.
>>>*
>>>*/
>>>   
>>> -#define VTCR_EL2_T0SZ_IPA  VTCR_EL2_T0SZ_40B
>>>   #define VTCR_EL2_COMMON_BITS  (VTCR_EL2_SH0_INNER | 
>>> VTCR_EL2_ORGN0_WBWA | \
>>>  VTCR_EL2_IRGN0_WBWA | VTCR_EL2_RES1)
>>> +#define VTCR_EL2_PRIVATE_MASK  (VTCR_EL2_SL0_MASK | VTCR_EL2_T0SZ_MASK)
>>
>> What does "private" mean here? It really is the IPA configuration, so
>> I'd rather have a naming that reflects that.
>>
>>>   #ifdef CONFIG_ARM64_64K_PAGES
>>>   /*
>>>* Stage2 translation configuration:
>>>* 64kB pages (TG0 = 1)
>>> - * 2 level page tables (SL = 1)
>>>*/
>>> -#define VTCR_EL2_TGRAN_FLAGS   (VTCR_EL2_TG0_64K | 
>>> VTCR_EL2_SL0_LVL1)
>>> +#define VTCR_EL2_TGRAN VTCR_EL2_TG0_64K
>>>   #define VTCR_EL2_TGRAN_SL0_BASE   3UL
>>>   
>>>   #elif defined(CONFIG_ARM64_16K_PAGES)
>>>   /*
>>>* Stage2 translation configuration:
>>>* 16kB pages (TG0 = 2)
>>> - * 2 level page tables (SL = 1)
>>>*/
>>> -#define VTCR_EL2_TGRAN_FLAGS   (VTCR_EL2_TG0_16K | 
>>> VTCR_EL2_SL0_LVL1)
>>> +#define VTCR_EL2_TGRAN VTCR_EL2_TG0_16K
>>>   #define VTCR_EL2_TGRAN_SL0_BASE   3UL
>>>   #else /* 4K */
>>>   /*
>>>* Stage2 translation configuration:
>>>* 4kB pages (TG0 = 0)
>>> - * 3 level page tables (SL = 1)
>>>*/
>>> -#define VTCR_EL2_TGRAN_FLAGS   (VTCR_EL2_TG0_4K | 
>>> VTCR_EL2_SL0_LVL1)
>>> +#define VTCR_EL2_TGRAN VTCR_EL2_TG0_4K
>>>   #define VTCR_EL2_TGRAN_SL0_BASE   2UL
>>>   #endif
>>>   
>>> -#define VTCR_EL2_FLAGS (VTCR_EL2_COMMON_BITS | 
>>> VTCR_EL2_TGRAN_FLAGS)
>>> +#define VTCR_EL2_FLAGS (VTCR_EL2_COMMON_BITS | VTCR_EL2_TGRAN)
>>> +
>>>   /*
>>>* VTCR_EL2:SL0 indicates the entry level for Stage2 translation.
>>>* Interestingly, it depends on the page size.
>>> diff --git a/arch/arm64/include/asm/kvm_asm.h 
>>> b/arch/arm64/include/asm/kvm_asm.h
>>> index 102b5a5..91372eb 100644
>>> --- a/arch/arm64/include/asm/kvm_asm.h
>>> +++ b/arch/arm64/include/asm/kvm_asm.h
>>> @@ -72,7 +72,7 @@ extern void __vgic_v3_init_lrs(void);
>>>   
>>>   extern u32 __kvm_get_mdcr_el2(void);
>>>   
>>> -extern u32 __init_stage2_translation(void);
>>> +extern void __init_stage2_translation(void);
>>>   
>>>   /* Home-grown __this_cpu_{ptr,read} variants that always work at HYP */
>>>   #define __hyp_this_cpu_ptr(sym)   
>>> \
>>> diff --git a/arch/arm64/include/asm/kvm_host.h 
>>> b/arch/arm64/include/asm/kvm_host.h
>>> index

Re: [Qemu-devel] [PATCH v2 1/5] i386: Add support for IA32_PRED_CMD and IA32_ARCH_CAPABILITIES MSRs

[Eduardo Habkost]

On Tue, Jul 03, 2018 at 11:06:00AM +0200, Paolo Bonzini wrote:
> On 03/07/2018 10:48, Robert Hoo wrote:
> >>
> >> However, I suggest adding it to the FeatureWord enum, since everything
> >> that handles FeatureWord applies to this new kind of MSR as well.
> >> Currently FeatureWord is only for CPUID leaves, but it doesn't have to
> >> be like that.
> >>
> > I think this will be changing struct FeatureWordInfo, which is designed
> > for cpuid enumerations. You must not want to do that. May I know more
> > details about your thought?
> 
> The simplest way is to put CPUIDs first and MSRs second in FeatureWord.
> Then you can do
> 
>  FEAT_XSAVE_COMP_LO, /* CPUID[EAX=0xd,ECX=0].EAX */
>  FEAT_XSAVE_COMP_HI, /* CPUID[EAX=0xd,ECX=0].EDX */
> +FEATURE_WORDS_NUM_CPUID,
> +FEATURE_WORDS_FIRST_MSR = FEATURE_WORDS_NUM_CPUID,
> +FEAT_MSR_ARCH_CAPABILITIES = FEATURE_WORDS_FIRST_MSR,
>  FEATURE_WORDS,
> };
> 
> #define FEATURE_WORDS_NUM_MSRS (FEATURE_WORDS - \
> FEATURE_WORDS_FIRST_MSR)
> 
> Then the existing loops that use FeatureWordInfo can go up to
> FEATURE_WORDS_NUM_CPUID.

I assume we want to make some (or most) of the loops go up to
FEATURE_WORDS (e.g. make x86_cpu_get_supported_feature_word()
support MSRs too), otherwise it would be pointless to reuse the
same array.

I would be OK with both approaches, though.  If the first version
doesn't use the `features[]` array and implements this with a
separate `msr_features[]` or `msr_arch_capabilities` field, it
would work for me (especially if this means we can get this
implemented in time for QEMU 3.0).

-- 
Eduardo

Re: [Qemu-devel] [PATCH v2 1/5] i386: Add support for IA32_PRED_CMD and IA32_ARCH_CAPABILITIES MSRs

[Robert Hoo]

On Tue, 2018-07-03 at 11:06 +0200, Paolo Bonzini wrote:
> On 03/07/2018 10:48, Robert Hoo wrote:
> >>
> >> However, I suggest adding it to the FeatureWord enum, since everything
> >> that handles FeatureWord applies to this new kind of MSR as well.
> >> Currently FeatureWord is only for CPUID leaves, but it doesn't have to
> >> be like that.
> >>
> > I think this will be changing struct FeatureWordInfo, which is designed
> > for cpuid enumerations. You must not want to do that. May I know more
> > details about your thought?
> 
> The simplest way is to put CPUIDs first and MSRs second in FeatureWord.
> Then you can do
> 
>  FEAT_XSAVE_COMP_LO, /* CPUID[EAX=0xd,ECX=0].EAX */
>  FEAT_XSAVE_COMP_HI, /* CPUID[EAX=0xd,ECX=0].EDX */
> +FEATURE_WORDS_NUM_CPUID,
> +FEATURE_WORDS_FIRST_MSR = FEATURE_WORDS_NUM_CPUID,
> +FEAT_MSR_ARCH_CAPABILITIES = FEATURE_WORDS_FIRST_MSR,
>  FEATURE_WORDS,
> };
> 
> #define FEATURE_WORDS_NUM_MSRS (FEATURE_WORDS - \
> FEATURE_WORDS_FIRST_MSR)
> 
> Then the existing loops that use FeatureWordInfo can go up to
> FEATURE_WORDS_NUM_CPUID.

Emm... Understand your point now. It is a little risky, all references
to FEATURE_WORDS need to be updated carefully.
OK, let me try to think in this way.
Perhaps, I'll need to define a new 'struct FeautureWordMsrInfo' to
describe feature words from MSR, in parallel to current FeatureWordInfo
(or better rename it to FeatureWordCpuidInfo).
> 
> Thanks,
> 
> Paolo
> 
> > And, if I implemented ARCH_CAPABILITIES-bits features in FeatureWord,
> > then no necessity of having it in kvm_msr_entries, right?
> 
And would you help confirm with my this point?

Re: [Qemu-devel] [PATCH V9 12/20] qapi: Add new command to query colo status

[Markus Armbruster]

Zhang Chen  writes:

> Libvirt or other high level software can use this command query colo status.
> You can test this command like that:
> {'execute':'query-colo-status'}
>
> Signed-off-by: Zhang Chen 
> ---
>  migration/colo.c| 39 +++
>  qapi/migration.json | 34 ++
>  2 files changed, 73 insertions(+)
>
> diff --git a/migration/colo.c b/migration/colo.c
> index 8fdb79ac73..eb21978bff 100644
> --- a/migration/colo.c
> +++ b/migration/colo.c
> @@ -29,6 +29,7 @@
>  #include "net/colo.h"
>  #include "block/block.h"
>  #include "qapi/qapi-events-migration.h"
> +#include "qapi/qmp/qerror.h"
>  
>  static bool vmstate_loading;
>  static Notifier packets_compare_notifier;
> @@ -237,6 +238,44 @@ void qmp_xen_colo_do_checkpoint(Error **errp)
>  #endif
>  }
>  
> +COLOStatus *qmp_query_colo_status(Error **errp)
> +{
> +int state;
> +COLOStatus *s = g_new0(COLOStatus, 1);
> +
> +s->mode = get_colo_mode();
> +
> +switch (s->mode) {
> +case COLO_MODE_NONE:
> +error_setg(errp, "COLO is disabled");
> +state = MIGRATION_STATUS_NONE;
> +break;
> +case COLO_MODE_PRIMARY:
> +state = migrate_get_current()->state;
> +break;
> +case COLO_MODE_SECONDARY:
> +state = migration_incoming_get_current()->state;
> +break;
> +default:
> +abort();
> +}
> +
> +s->active = state == MIGRATION_STATUS_COLO;
> +
> +switch (failover_get_state()) {
> +case FAILOVER_STATUS_NONE:
> +s->reason = COLO_EXIT_REASON_NONE;
> +break;
> +case FAILOVER_STATUS_REQUIRE:
> +s->reason = COLO_EXIT_REASON_REQUEST;
> +break;
> +default:
> +s->reason = COLO_EXIT_REASON_ERROR;
> +}
> +
> +return s;
> +}
> +
>  static void colo_send_message(QEMUFile *f, COLOMessage msg,
>Error **errp)
>  {
> diff --git a/qapi/migration.json b/qapi/migration.json
> index c24f114104..73c64686ec 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -1248,6 +1248,40 @@
>  ##
>  { 'command': 'xen-colo-do-checkpoint' }
>  
> +##
> +# @COLOStatus:
> +#
> +# The result format for 'query-colo-status'.
> +#
> +# @mode: COLO running mode. If COLO is running, this field will return
> +#'primary' or 'secondary'.

Please mention that @mode is "none" when COLO is not running.

> +#
> +# @active: true if COLO is active.

Please use consistent terminology: pick one of "COLO is running", "COLO
is active" and stick to it.  v8 had that here, v9 regressed.  Also use
this wording for the comment improvement I requested for PATCH 11.

However, isn't @active redundant with @mode?

> +#
> +# @reason: describes the reason for the COLO exit.
> +#
> +# Since: 3.0
> +##
> +{ 'struct': 'COLOStatus',
> +  'data': { 'mode': 'COLOMode', 'active': 'bool', 'reason': 'COLOExitReason' 
> } }
> +
> +##
> +# @query-colo-status:
> +#
> +# Query COLO status while the vm is running.
> +#
> +# Returns: A @COLOStatus object showing the status.
> +#
> +# Example:
> +#
> +# -> { "execute": "query-colo-status" }
> +# <- { "return": { "mode": "primary", "active": true, "reason": "request" } }
> +#
> +# Since: 3.0
> +##
> +{ 'command': 'query-colo-status',
> +  'returns': 'COLOStatus' }
> +
>  ##
>  # @migrate-recover:
>  #

Re: [Qemu-devel] [PATCH v2 2/3] block/fleecing-filter: new filter driver for fleecing

[Kevin Wolf]

Am 02.07.2018 um 14:09 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 29.06.2018 20:40, Eric Blake wrote:
> > On 06/29/2018 12:30 PM, John Snow wrote:
> > > 
> > > 
> > > On 06/29/2018 11:15 AM, Vladimir Sementsov-Ogievskiy wrote:
> > > > We need to synchronize backup job with reading from fleecing image
> > > > like it was done in block/replication.c.
> > > > 
> > > > Otherwise, the following situation is theoretically possible:
> > > > 
> > > > 1. client start reading
> > > > 2. client understand, that there is no corresponding cluster in
> > > >     fleecing image
> > > 
> > > I don't think the client refocuses the read, but QEMU does. (the client
> > > has no idea if it is reading from the fleecing node or the backing
> > > file.)
> > > 
> > > ... but understood:
> > > 
> > > > 3. client is going to read from backing file (i.e. active image)
> > > > 4. guest writes to active image
> > > 
> > > My question here is if QEMU will allow reads and writes to interleave in
> > > general. If the client is reading from the backing file, the active
> > > image, will QEMU allow a write to modify that data before we're done
> > > getting that data?
> > > 
> > > > 5. this write is stopped by backup(sync=none) and cluster is copied to
> > > >     fleecing image
> > > > 6. guest write continues...
> > > > 7. and client reads _new_ (or partly new) date from active image
> > > > 
> > > 
> > > I can't answer this for myself one way or the other right now, but I
> > > imagine you observed a failure in practice in your test setups, which
> > > motivated this patch?
> > > 
> > > A test or some proof would help justification for this patch. It would
> > > also help prove that it solves what it claims to!
> > 
> > In fact, do we really need a new filter, or do we just need to make the
> > "sync":"none" blockdev-backup job take the appropriate synchronization
> > locks?
> > 
> 
> How? We'll need additional mechanism like serializing requests.. Or a way to
> reuse serializing requests. Using backup internal synchronization looks
> simpler, and it is already used in block replication.

But it also just an ugly hack that fixes one special case and leaves
everything else broken. replication is usually not a good example for
anything. It always gives me bad surprises when I have to look at it.

We'll have to figure out where to fix this problem (or what it really
is, once you look more than just at fleecing), but I think requiring the
user to add a filter driver to work around missing serialisation in
other code, and corrupting their image if they forget to, is not a
reasonable solution.

I see at least two things wrong in this context:

* The permissions don't seem to match reality. The NBD server
  unconditionally shares PERM_WRITE, which is wrong in this case. The
  client wants to see a point-in-time snapshot that never changes. This
  should become an option so that it can be properly reflected in the
  permissions used.

* Once we have proper permissions, the fleecing setup breaks down
  because the guest needs PERM_WRITE on the backing file, but the
  fleecing overlay allows that only if the NBD client allows it (which
  it doesn't for fleecing).

  Now we can implement an exception right into backup that installs a
  backup filter driver between source and target if the source is the
  backing file of the target. The filter driver would be similar to the
  commit filter driver in that it simply promises !PERM_WRITE to its
  parents, but allows PERM_WRITE on the source because it has installed
  the before_write_notifier that guarantees this condition.

  All writes to the target that are made by the backup job in this setup
  (including before_write_notifier writes) need to be marked as
  serialising so that any concurrent reads are completed first.

And if we decide to add a target filter to backup, we should probably at
the same time use a filter driver for intercepting source writes instead
of using before_write_notifier.

Max, I think you intended to make both source and target children of the
same block job node (or at least for mirror). But wouldn't that create
loops in a setup like this? I think we may need two filters that are
only connected through the block job, but not with block graph edges.

Kevin

Re: [Qemu-devel] [PATCH v6] crypto: Implement TLS Pre-Shared Keys (PSK).

[Daniel P . Berrangé]

On Tue, Jul 03, 2018 at 09:03:03AM +0100, Richard W.M. Jones wrote:
> Pre-Shared Keys (PSK) is a simpler mechanism for enabling TLS
> connections than using certificates.  It requires only a simple secret
> key:
> 
>   $ mkdir -m 0700 /tmp/keys
>   $ psktool -u rjones -p /tmp/keys/keys.psk
>   $ cat /tmp/keys/keys.psk
>   rjones:d543770c15ad93d76443fb56f501a31969235f47e999720ae8d2336f6a13fcbc
> 
> The key can be secretly shared between clients and servers.  Clients
> must specify the directory containing the "keys.psk" file and a
> username (defaults to "qemu").  Servers must specify only the
> directory.
> 
> Example NBD client:
> 
>   $ qemu-img info \
> --object 
> tls-creds-psk,id=tls0,dir=/tmp/keys,username=rjones,endpoint=client \
> --image-opts \
> 
> file.driver=nbd,file.host=localhost,file.port=10809,file.tls-creds=tls0,file.export=/
> 
> Example NBD server using qemu-nbd:
> 
>   $ qemu-nbd -t -x / \
> --object tls-creds-psk,id=tls0,endpoint=server,dir=/tmp/keys \
> --tls-creds tls0 \
> image.qcow2
> 
> Example NBD server using nbdkit:
> 
>   $ nbdkit -n -e / -fv \
> --tls=on --tls-psk=/tmp/keys/keys.psk \
> file file=disk.img
> 
> Signed-off-by: Richard W.M. Jones 
> ---
>  crypto/Makefile.objs   |   1 +
>  crypto/tlscredspsk.c   | 308 +
>  crypto/tlssession.c|  56 +-
>  crypto/trace-events|   3 +
>  include/crypto/tlscredspsk.h   | 106 
>  qemu-doc.texi  |  37 
>  qemu-options.hx|  24 +++
>  tests/Makefile.include |   4 +-
>  tests/crypto-tls-psk-helpers.c |  50 ++
>  tests/crypto-tls-psk-helpers.h |  29 
>  tests/test-crypto-tlssession.c | 185 +---
>  11 files changed, 777 insertions(+), 26 deletions(-)

Signed-off-by: Daniel P. Berrangé 


I'll send a pull request with it shortly


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH v2 2/3] block/fleecing-filter: new filter driver for fleecing

[Kevin Wolf]

Am 02.07.2018 um 13:57 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 29.06.2018 20:30, John Snow wrote:
> > 
> > On 06/29/2018 11:15 AM, Vladimir Sementsov-Ogievskiy wrote:
> > > We need to synchronize backup job with reading from fleecing image
> > > like it was done in block/replication.c.
> > > 
> > > Otherwise, the following situation is theoretically possible:
> > > 
> > > 1. client start reading
> > > 2. client understand, that there is no corresponding cluster in
> > > fleecing image
> > I don't think the client refocuses the read, but QEMU does. (the client
> > has no idea if it is reading from the fleecing node or the backing file.)
> > 
> > ... but understood:
> > 
> > > 3. client is going to read from backing file (i.e. active image)
> > > 4. guest writes to active image
> > My question here is if QEMU will allow reads and writes to interleave in
> > general. If the client is reading from the backing file, the active
> > image, will QEMU allow a write to modify that data before we're done
> > getting that data?
> 
> If I understand correctly: yes. Physical drives allows intersecting
> operations too and there no guarantee on result. We have serializing
> requests, but in general only unaligned requests are marked serializing.

Copy on read (in the context of image streaming) is the feature which
actually introduced it. It has some similarities with the backup job, so
the idea of using it for backup, too, isn't exactly revolutionary.

Kevin

Re: [Qemu-devel] [libvirt] [PULL 25/26] block: Remove deprecated -drive option serial

[Daniel P . Berrangé]

On Tue, Jul 03, 2018 at 12:53:44PM +0200, Christian Borntraeger wrote:
> 
> 
> On 07/02/2018 10:04 AM, Kevin Wolf wrote:
> > Am 25.06.2018 um 13:45 hat Peter Krempa geschrieben:
> >> On Mon, Jun 25, 2018 at 13:41:06 +0200, Kevin Wolf wrote:
> >>> Am 25.06.2018 um 11:53 hat Daniel P. Berrangé geschrieben:
>  On Fri, Jun 22, 2018 at 03:31:46PM +0100, Daniel P. Berrangé wrote:
> > On Fri, Jun 22, 2018 at 04:25:13PM +0200, Kevin Wolf wrote:
> >> Am 22.06.2018 um 15:36 hat Christian Borntraeger geschrieben:
> >>
> >> [...]
> >>
> >>>
> >>> Thanks!
> >>>
> >>> I'll look into werror/rerror support for usb-storage. It shouldn't be
> >>> too hard, though it's strictly speaking a separate problem related to
> >>> using -blockdev rather than option deprecation.
> >>>
> >>> If Peter wants to wait for QEMU support before converting werror/rerror
> >>
> >> Definitely. I don't want to keep around yet another hack that will
> >> satisfy one specific case and then add another capability for it. We
> >> should then gate the moving of the feature based on the presence of
> >> werror for usb-storage.
> >>
> >>> to -device, maybe it would make sense to split your patch for v2 so that
> >>> geometry and serial can get fixed right away?
> >>
> >> Yes this can be done right away.
> > 
> > Has serial/gemoetry been fixed meanwhile and will it make it into the
> > next release?
> 
> I cannot find an archive that has it, but it is on the libvirt mailing
> list as "[libvirt] [PATCH v3] qemu: format serial and geometry on frontend 
> disk device".
> Review seems done, but it has missed libvirt 4.5 which was released today.

Just posted latest version here:

  https://www.redhat.com/archives/libvir-list/2018-July/msg00130.html

It will be in the next release on ~ Aug 1st

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [libvirt] [PULL 25/26] block: Remove deprecated -drive option serial

[Kevin Wolf]

Am 03.07.2018 um 13:22 hat Daniel P. Berrangé geschrieben:
> On Tue, Jul 03, 2018 at 12:53:44PM +0200, Christian Borntraeger wrote:
> > 
> > 
> > On 07/02/2018 10:04 AM, Kevin Wolf wrote:
> > > Am 25.06.2018 um 13:45 hat Peter Krempa geschrieben:
> > >> On Mon, Jun 25, 2018 at 13:41:06 +0200, Kevin Wolf wrote:
> > >>> Am 25.06.2018 um 11:53 hat Daniel P. Berrangé geschrieben:
> >  On Fri, Jun 22, 2018 at 03:31:46PM +0100, Daniel P. Berrangé wrote:
> > > On Fri, Jun 22, 2018 at 04:25:13PM +0200, Kevin Wolf wrote:
> > >> Am 22.06.2018 um 15:36 hat Christian Borntraeger geschrieben:
> > >>
> > >> [...]
> > >>
> > >>>
> > >>> Thanks!
> > >>>
> > >>> I'll look into werror/rerror support for usb-storage. It shouldn't be
> > >>> too hard, though it's strictly speaking a separate problem related to
> > >>> using -blockdev rather than option deprecation.
> > >>>
> > >>> If Peter wants to wait for QEMU support before converting werror/rerror
> > >>
> > >> Definitely. I don't want to keep around yet another hack that will
> > >> satisfy one specific case and then add another capability for it. We
> > >> should then gate the moving of the feature based on the presence of
> > >> werror for usb-storage.
> > >>
> > >>> to -device, maybe it would make sense to split your patch for v2 so that
> > >>> geometry and serial can get fixed right away?
> > >>
> > >> Yes this can be done right away.
> > > 
> > > Has serial/gemoetry been fixed meanwhile and will it make it into the
> > > next release?
> > 
> > I cannot find an archive that has it, but it is on the libvirt mailing
> > list as "[libvirt] [PATCH v3] qemu: format serial and geometry on frontend 
> > disk device".
> > Review seems done, but it has missed libvirt 4.5 which was released today.
> 
> Just posted latest version here:
> 
>   https://www.redhat.com/archives/libvir-list/2018-July/msg00130.html
> 
> It will be in the next release on ~ Aug 1st

It would have been a lot nicer to have it the July release because this
means that we'll have the released libvirt broken during almost the
whole rc phase of QEMU 3.0, but the release is planned for Aug 8th the
earliest, so I guess we're still okay. People using QEMU from git will
just need libvirt from git as well.

Kevin

Re: [Qemu-devel] [libvirt] [PULL 25/26] block: Remove deprecated -drive option serial

[Peter Maydell]

On 3 July 2018 at 12:32, Kevin Wolf  wrote:
> Am 03.07.2018 um 13:22 hat Daniel P. Berrangé geschrieben:
>> Just posted latest version here:
>>
>>   https://www.redhat.com/archives/libvir-list/2018-July/msg00130.html
>>
>> It will be in the next release on ~ Aug 1st
>
> It would have been a lot nicer to have it the July release because this
> means that we'll have the released libvirt broken during almost the
> whole rc phase of QEMU 3.0, but the release is planned for Aug 8th the
> earliest, so I guess we're still okay. People using QEMU from git will
> just need libvirt from git as well.

I'm still not clear what we gain from having a QEMU that's dropped
a feature that is still used by everything except leading-edge
not-yet-released versions of libvirt. Is there a strong reason we
can't just revert the deletion of the deprecated feature for a QEMU
release or two?

thanks
-- PMM

Re: [Qemu-devel] [libvirt] [PULL 25/26] block: Remove deprecated -drive option serial

[Daniel P . Berrangé]

On Tue, Jul 03, 2018 at 01:32:29PM +0200, Kevin Wolf wrote:
> Am 03.07.2018 um 13:22 hat Daniel P. Berrangé geschrieben:
> > On Tue, Jul 03, 2018 at 12:53:44PM +0200, Christian Borntraeger wrote:
> > > 
> > > 
> > > On 07/02/2018 10:04 AM, Kevin Wolf wrote:
> > > > Am 25.06.2018 um 13:45 hat Peter Krempa geschrieben:
> > > >> On Mon, Jun 25, 2018 at 13:41:06 +0200, Kevin Wolf wrote:
> > > >>> Am 25.06.2018 um 11:53 hat Daniel P. Berrangé geschrieben:
> > >  On Fri, Jun 22, 2018 at 03:31:46PM +0100, Daniel P. Berrangé wrote:
> > > > On Fri, Jun 22, 2018 at 04:25:13PM +0200, Kevin Wolf wrote:
> > > >> Am 22.06.2018 um 15:36 hat Christian Borntraeger geschrieben:
> > > >>
> > > >> [...]
> > > >>
> > > >>>
> > > >>> Thanks!
> > > >>>
> > > >>> I'll look into werror/rerror support for usb-storage. It shouldn't be
> > > >>> too hard, though it's strictly speaking a separate problem related to
> > > >>> using -blockdev rather than option deprecation.
> > > >>>
> > > >>> If Peter wants to wait for QEMU support before converting 
> > > >>> werror/rerror
> > > >>
> > > >> Definitely. I don't want to keep around yet another hack that will
> > > >> satisfy one specific case and then add another capability for it. We
> > > >> should then gate the moving of the feature based on the presence of
> > > >> werror for usb-storage.
> > > >>
> > > >>> to -device, maybe it would make sense to split your patch for v2 so 
> > > >>> that
> > > >>> geometry and serial can get fixed right away?
> > > >>
> > > >> Yes this can be done right away.
> > > > 
> > > > Has serial/gemoetry been fixed meanwhile and will it make it into the
> > > > next release?
> > > 
> > > I cannot find an archive that has it, but it is on the libvirt mailing
> > > list as "[libvirt] [PATCH v3] qemu: format serial and geometry on 
> > > frontend disk device".
> > > Review seems done, but it has missed libvirt 4.5 which was released today.
> > 
> > Just posted latest version here:
> > 
> >   https://www.redhat.com/archives/libvir-list/2018-July/msg00130.html
> > 
> > It will be in the next release on ~ Aug 1st
> 
> It would have been a lot nicer to have it the July release because this
> means that we'll have the released libvirt broken during almost the
> whole rc phase of QEMU 3.0, but the release is planned for Aug 8th the
> earliest, so I guess we're still okay. People using QEMU from git will
> just need libvirt from git as well.

Yeah, unfortunately i just missed the window of possibility to get it
into yesterday's release.

Fedora at least will be ok, and when other distros do pick up new QEMU
they'll likely pick up the corresponding libvirt release at same time
anyway.

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

[Qemu-devel] [Bug 1776478] Re: Getting qemu: uncaught target signal 6 when running lv2 plugin cross-compilation

[Peter Maydell]

You want to execute a single program in the chroot (whatever the binary
that is failing is):

sudo chroot . usr/bin/qemu-arm-static -g program/to/run arguments

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

Title:
  Getting qemu: uncaught target signal 6 when running  lv2 plugin cross-
  compilation

Status in QEMU:
  New

Bug description:
  Hey,
  I am part of the Zynthian team and we use qemu-arm-static to cross compile 
lv2 audio plugins.

  When running a compilation of DISTRHO-Ports we get:

  lv2_ttl_generator: pthread_mutex_lock.c:81: __pthread_mutex_lock: Assertion 
`mutex->__data.__owner == 0' failed.
  qemu: uncaught target signal 6 (Aborted) - core dumped
  ./scripts/generate-ttl.sh: line 27: 16524 Aborted $GEN ./$FILE
  Makefile:62: recipe for target 'gen_lv2' failed
  make[1]: *** [gen_lv2] Error 134
  make[1]: Leaving directory '/home/pi/zynthian-sw/plugins/DISTRHO-Ports'
  Makefile:104: recipe for target 'lv2' failed
  make: *** [lv2] Error 2

  
  lv2_ttl_generator source is here:
  https://github.com/DISTRHO/DISTRHO-Ports/tree/master/libs/lv2-ttl-generator

  The command that is ruining is
  lv2_ttl_generator ./TAL-Filter-2.so 

  And ./TAL-Filter-2.so source is here:
  https://github.com/DISTRHO/DISTRHO-Ports/tree/master/ports/tal-filter-2/source


  Is there a way to debug what is going on?
  This runs fine on a Raspberrypi which is armv7

  A workaround would also help.

  
  Bug in Zynthian:
  https://github.com/zynthian/zynthian-sys/issues/59
  Bug in DISTRHO-Ports:
  https://github.com/DISTRHO/DISTRHO-Ports/issues/29

  Using qemu-arm-static version from master from two days ago:
  qemu-arm version 2.12.50 (v2.12.0-1182-ga7a7309ca5-dirty), commit: 
a7a7309ca52c327c6603d60db90ae4feeae719f7

  Also saw this in qemu-arm version 2.12.0 (Debian 1:2.12+dfsg-3)

  Thanks,
  Guy

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

Re: [Qemu-devel] [PATCH v2 2/3] block/fleecing-filter: new filter driver for fleecing

[Vladimir Sementsov-Ogievskiy]

03.07.2018 14:15, Kevin Wolf wrote:

Am 02.07.2018 um 14:09 hat Vladimir Sementsov-Ogievskiy geschrieben:

29.06.2018 20:40, Eric Blake wrote:

On 06/29/2018 12:30 PM, John Snow wrote:


On 06/29/2018 11:15 AM, Vladimir Sementsov-Ogievskiy wrote:

We need to synchronize backup job with reading from fleecing image
like it was done in block/replication.c.

Otherwise, the following situation is theoretically possible:

1. client start reading
2. client understand, that there is no corresponding cluster in
     fleecing image

I don't think the client refocuses the read, but QEMU does. (the client
has no idea if it is reading from the fleecing node or the backing
file.)

... but understood:


3. client is going to read from backing file (i.e. active image)
4. guest writes to active image

My question here is if QEMU will allow reads and writes to interleave in
general. If the client is reading from the backing file, the active
image, will QEMU allow a write to modify that data before we're done
getting that data?


5. this write is stopped by backup(sync=none) and cluster is copied to
     fleecing image
6. guest write continues...
7. and client reads _new_ (or partly new) date from active image


I can't answer this for myself one way or the other right now, but I
imagine you observed a failure in practice in your test setups, which
motivated this patch?

A test or some proof would help justification for this patch. It would
also help prove that it solves what it claims to!

In fact, do we really need a new filter, or do we just need to make the
"sync":"none" blockdev-backup job take the appropriate synchronization
locks?


How? We'll need additional mechanism like serializing requests.. Or a way to
reuse serializing requests. Using backup internal synchronization looks
simpler, and it is already used in block replication.

But it also just an ugly hack


agree.


  that fixes one special case and leaves
everything else broken. replication is usually not a good example for
anything. It always gives me bad surprises when I have to look at it.

We'll have to figure out where to fix this problem (or what it really
is, once you look more than just at fleecing), but I think requiring the
user to add a filter driver to work around missing serialisation in
other code, and corrupting their image if they forget to, is not a
reasonable solution.

I see at least two things wrong in this context:

* The permissions don't seem to match reality. The NBD server
   unconditionally shares PERM_WRITE, which is wrong in this case. The
   client wants to see a point-in-time snapshot that never changes. This
   should become an option so that it can be properly reflected in the
   permissions used.

* Once we have proper permissions, the fleecing setup breaks down
   because the guest needs PERM_WRITE on the backing file, but the
   fleecing overlay allows that only if the NBD client allows it (which
   it doesn't for fleecing).

   Now we can implement an exception right into backup that installs a
   backup filter driver between source and target if the source is the
   backing file of the target. The filter driver would be similar to the
   commit filter driver in that it simply promises !PERM_WRITE to its
   parents, but allows PERM_WRITE on the source because it has installed
   the before_write_notifier that guarantees this condition.

   All writes to the target that are made by the backup job in this setup
   (including before_write_notifier writes) need to be marked as
   serialising so that any concurrent reads are completed first.

And if we decide to add a target filter to backup, we should probably at
the same time use a filter driver for intercepting source writes instead
of using before_write_notifier.


and this is the point, where we can drop backup job at all from fleecing 
scheme, as actually backup(sync=none) == such special filter driver




Max, I think you intended to make both source and target children of the
same block job node (or at least for mirror). But wouldn't that create
loops in a setup like this? I think we may need two filters that are
only connected through the block job, but not with block graph edges.

Kevin


--
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v6 07/15] qapi: add #if/#endif helpers

[Markus Armbruster]

Marc-André Lureau  writes:

> Add helpers to wrap generated code with #if/#endif lines.
>
> Add QAPIGenCSnippet class to write C snippet code, make QAPIGenC
> inherit from it, for full C files with copyright headers etc.

It's called QAPIGenCCode now.  Can touch up when I apply, along with
another instance below.

Leaves the reader wondering *why* you need to splice QAPIGenCCode
between QAPIGen and QAPIGenC.  Becomes clear only in PATCH 10.
Providing the class now reduces code churn, but you should explain why.
Perhaps:

  A later patch wants to use QAPIGen for generating C snippets rather
  than full C files with copyright headers etc.  Splice in class
  QAPIGenCCode between QAPIGen and QAPIGenC.

> Add a 'with' statement context manager that will be used to wrap
> generator visitor methods.  The manager will check if code was
> generated before adding #if/#endif lines on QAPIGenCSnippet
> objects. Used in the following patches.
>
> Signed-off-by: Marc-André Lureau 
> ---
>  scripts/qapi/common.py | 101 +++--
>  1 file changed, 97 insertions(+), 4 deletions(-)
>
> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
> index 1b56065a80..44eaf25850 100644
> --- a/scripts/qapi/common.py
> +++ b/scripts/qapi/common.py
> @@ -12,6 +12,7 @@
>  # See the COPYING file in the top-level directory.
>  
>  from __future__ import print_function
> +from contextlib import contextmanager
>  import errno
>  import os
>  import re
> @@ -1974,6 +1975,40 @@ def guardend(name):
>   name=guardname(name))
>  
>  
> +def gen_if(ifcond):
> +ret = ''
> +for ifc in ifcond:
> +ret += mcgen('''
> +#if %(cond)s
> +''', cond=ifc)
> +return ret
> +
> +
> +def gen_endif(ifcond):
> +ret = ''
> +for ifc in reversed(ifcond):
> +ret += mcgen('''
> +#endif /* %(cond)s */
> +''', cond=ifc)
> +return ret
> +
> +
> +def wrap_ifcond(ifcond, before, after):

Looks like a helper method.  Name it _wrap_ifcond?

> +if ifcond is None or before == after:
> +return after

The before == after part suppresses empty conditionals.  Suggest

   return after# suppress empty #if ... #endif

The ifcond is None part is suppresses "non-conditionals".  How can this
happen?  If it can't, let's drop this part.

Another non-conditional is [].  See below.

> +
> +assert after.startswith(before)
> +out = before
> +added = after[len(before):]
> +if added[0] == '\n':
> +out += '\n'
> +added = added[1:]

The conditional adjusts vertical space around #if.  This might need
tweaking, but let's not worry about that now.

> +out += gen_if(ifcond)
> +out += added
> +out += gen_endif(ifcond)

There's no similar adjustment for #endif.  Again, let's not worry about
that now.

> +return out
> +
> +

Since gen_if([]) and gen_endif([]) return '', wrap_ifcond([], before,
after) returns after.  Okay.

>  def gen_enum_lookup(name, values, prefix=None):
>  ret = mcgen('''
>  
> @@ -2064,6 +2099,7 @@ class QAPIGen(object):
>  def __init__(self):
>  self._preamble = ''
>  self._body = ''
> +self._start_if = None
>  
>  def preamble_add(self, text):
>  self._preamble += text
> @@ -2071,6 +2107,25 @@ class QAPIGen(object):
>  def add(self, text):
>  self._body += text
>  
> +def start_if(self, ifcond):
> +assert self._start_if is None
> +

Let's drop this blank line.

> +self._start_if = (ifcond, self._body, self._preamble)

It's now obvious that you can't nest .start_if() ... .endif().  Good.

> +
> +def _wrap_ifcond(self):
> +pass
> +
> +def end_if(self):
> +assert self._start_if
> +

Let's drop this blank line.

> +self._wrap_ifcond()
> +self._start_if = None
> +
> +def get_content(self, fname=None):
> +assert self._start_if is None
> +return (self._top(fname) + self._preamble + self._body
> ++ self._bottom(fname))
> +
>  def _top(self, fname):
>  return ''
>  

Note for later: all this code has no effect unless ._wrap_ifcond() is
overridden.

> @@ -2091,8 +2146,7 @@ class QAPIGen(object):
>  f = open(fd, 'r+', encoding='utf-8')
>  else:
>  f = os.fdopen(fd, 'r+')
> -text = (self._top(fname) + self._preamble + self._body
> -+ self._bottom(fname))
> +text = self.get_content(fname)
>  oldtext = f.read(len(text) + 1)
>  if text != oldtext:
>  f.seek(0)
> @@ -2101,10 +2155,49 @@ class QAPIGen(object):
>  f.close()
>  
>  
> -class QAPIGenC(QAPIGen):
> +@contextmanager
> +def ifcontext(ifcond, *args):
> +"""A 'with' statement context manager to wrap with start_if()/end_if()
>  
> -def __init__(self, blurb, pydoc):
> +*args: variable length argument list of QAPIGen

Perhaps: any number of QAPIGen objects

> +
> +Example::
> +
> +with if

Re: [Qemu-devel] [PATCH v3 10/20] kvm: arm64: Dynamic configuration of VTTBR mask

[Suzuki K Poulose]

Hi Eric,

On 02/07/18 15:41, Auger Eric wrote:

Hi Suzuki,

On 06/29/2018 01:15 PM, Suzuki K Poulose wrote:

On arm64 VTTBR_EL2:BADDR holds the base address for the stage2
translation table. The Arm ARM mandates that the bits BADDR[x-1:0]
should be 0, where 'x' is defined for a given IPA Size and the
number of levels for a translation granule size. It is defined
using some magical constants. This patch is a reverse engineered
implementation to calculate the 'x' at runtime for a given ipa and
number of page table levels. See patch for more details.

Cc: Marc Zyngier 
Cc: Christoffer Dall 
Signed-off-by: Suzuki K Poulose 
---
Changes since V2:
  - Part 1 of spilt from VTCR & VTTBR dynamic configuration
---
  arch/arm64/include/asm/kvm_arm.h | 60 +---
  arch/arm64/include/asm/kvm_mmu.h | 25 -
  2 files changed, 80 insertions(+), 5 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_arm.h b/arch/arm64/include/asm/kvm_arm.h
index 3dffd38..c557f45 100644
--- a/arch/arm64/include/asm/kvm_arm.h
+++ b/arch/arm64/include/asm/kvm_arm.h
@@ -140,8 +140,6 @@
   * Note that when using 4K pages, we concatenate two first level page tables
   * together. With 16K pages, we concatenate 16 first level page tables.
   *
- * The magic numbers used for VTTBR_X in this patch can be found in Tables
- * D4-23 and D4-25 in ARM DDI 0487A.b.

Isn't it a pretty old reference? Could you refer to C.a?


Sure, I will update the references everywhere.


+ *
+ * The algorithm defines the expectations on the BaseAddress (for the page
+ * table) bits resolved at each level based on the page size, entry level
+ * and T0SZ. The variable "x" in the algorithm also affects the VTTBR:BADDR
+ * for stage2 page table.
+ *
+ * The value of "x" is calculated as :
+ * x = Magic_N - T0SZ
+ *
+ * where Magic_N is an integer depending on the page size and the entry
+ * level of the page table as below:
+ *
+ * 
+ * | Entry level   |  4K16K   64K |
+ * 
+ * | Level: 0 (4 levels)   | 28   |  -  |  -  |
+ * 
+ * | Level: 1 (3 levels)   | 37   | 31  | 25  |
+ * 
+ * | Level: 2 (2 levels)   | 46   | 42  | 38  |
+ * 
+ * | Level: 3 (1 level)| -| 53  | 51  |
+ * 

I understand entry level = Lookup level in the table.


Entry level => The level at which we start the page table walk for
a given address (This is in line with the ARM ARM). So,

Entry_level = (4 - Number_of_Page_table_levels)


But you may want to compute x for BaseAddress matching lookup level 2
with number of levels = 4.


No, the BaseAddress is only calcualted for the "Entry_level". So the
above case doesn't exist at all.


So shouldn't you s/Number of levels/4 - entry_level?


Ok, I now understood what you are referring to [0]

for BADDR we want the BaseAddr of the initial lookup level so
effectively the entry level we are interested in is 4 - number of levels
and we don't care or d) condition. At least this is my understanding ;-)
If correct you may slightly reword the explanation?




+ *
+ * We have a magic formula for the Magic_N below.
+ *
+ *  Magic_N(PAGE_SIZE, Entry_Level) = 64 - ((PAGE_SHIFT - 3) * Number of 
levels)


[0] ^^^




+ *
+ * where number of levels = (4 - Entry_Level).


^^^ Doesn't this help make it clear ? Using the expansion makes it a bit more
unreadable below.

  
+/*

+ * Get the magic number 'x' for VTTBR:BADDR of this KVM instance.
+ * With v8.2 LVA extensions, 'x' should be a minimum of 6 with
+ * 52bit IPS.

Link to the spec?


Sure, will add it.

Thanks for the patience to review :-)

Cheers
Suzuki

Re: [Qemu-devel] [RFC 5/6] hw/arm/virt: support kvm_type property

[Andrew Jones]

On Wed, Jun 20, 2018 at 03:07:32PM +0200, Eric Auger wrote:
> The kvm-type property currently is used to pass
> a user parameter to KVM_CREATE_VM. This matches
> the way KVM/ARM expects to pass the max_vm_phys_shift
> parameter.
> 
> This patch adds the support or the kvm-type property in
> machvirt and also implements the machine class kvm_type()
> callback so that it either returns the kvm-type value
> provided by the user or returns the max_vm_phys_shift
> exposed by KVM.
> 
> for instance, the usespace can use the following option to
> instantiate a 42b IPA guest: -machine kvm-type=42

'kvm-type' is a very generic name. It looks like you're creating a KVM
VM of type 42 (which I assume is the ultimate KVM VM that answers the
meaning to Life, the Universe, and Everything), but it's not obvious
how it relates to physical address bits. Why not call this property
something like 'min_vm_phys_shift'? Notice the 'min' in the name,
because this is where the user is stating what the minimum number of
physical address bits required for the VM is. IIUC, if KVM supports
more, then it shouldn't be a problem.

Thanks,
drew

[Qemu-devel] [PULL 0/1] Qcrypto next patches

[Daniel P . Berrangé]

The following changes since commit 9b75dcb15f562577a937ae01f324946513586e59:

  Merge remote-tracking branch 'remotes/ericb/tags/pull-nbd-2018-07-02' into 
staging (2018-07-03 10:47:02 +0100)

are available in the Git repository at:

  https://github.com/berrange/qemu tags/qcrypto-next-pull-request

for you to fetch changes up to e1a6dc91ddb55ef77a705b62b6e62634631fd57d:

  crypto: Implement TLS Pre-Shared Keys (PSK). (2018-07-03 13:04:38 +0100)


Add support for PSK credentials with TLS



Richard W.M. Jones (1):
  crypto: Implement TLS Pre-Shared Keys (PSK).

 crypto/Makefile.objs   |   1 +
 crypto/tlscredspsk.c   | 308 +
 crypto/tlssession.c|  56 +-
 crypto/trace-events|   3 +
 include/crypto/tlscredspsk.h   | 106 
 qemu-doc.texi  |  37 
 qemu-options.hx|  24 +++
 tests/Makefile.include |   4 +-
 tests/crypto-tls-psk-helpers.c |  50 ++
 tests/crypto-tls-psk-helpers.h |  29 
 tests/test-crypto-tlssession.c | 179 ---
 11 files changed, 774 insertions(+), 23 deletions(-)
 create mode 100644 crypto/tlscredspsk.c
 create mode 100644 include/crypto/tlscredspsk.h
 create mode 100644 tests/crypto-tls-psk-helpers.c
 create mode 100644 tests/crypto-tls-psk-helpers.h

-- 
2.17.1

[Qemu-devel] [PULL 1/1] crypto: Implement TLS Pre-Shared Keys (PSK).

[Daniel P . Berrangé]

From: "Richard W.M. Jones" 

Pre-Shared Keys (PSK) is a simpler mechanism for enabling TLS
connections than using certificates.  It requires only a simple secret
key:

  $ mkdir -m 0700 /tmp/keys
  $ psktool -u rjones -p /tmp/keys/keys.psk
  $ cat /tmp/keys/keys.psk
  rjones:d543770c15ad93d76443fb56f501a31969235f47e999720ae8d2336f6a13fcbc

The key can be secretly shared between clients and servers.  Clients
must specify the directory containing the "keys.psk" file and a
username (defaults to "qemu").  Servers must specify only the
directory.

Example NBD client:

  $ qemu-img info \
--object 
tls-creds-psk,id=tls0,dir=/tmp/keys,username=rjones,endpoint=client \
--image-opts \

file.driver=nbd,file.host=localhost,file.port=10809,file.tls-creds=tls0,file.export=/

Example NBD server using qemu-nbd:

  $ qemu-nbd -t -x / \
--object tls-creds-psk,id=tls0,endpoint=server,dir=/tmp/keys \
--tls-creds tls0 \
image.qcow2

Example NBD server using nbdkit:

  $ nbdkit -n -e / -fv \
--tls=on --tls-psk=/tmp/keys/keys.psk \
file file=disk.img

Signed-off-by: Richard W.M. Jones 
Signed-off-by: Daniel P. Berrangé 
---
 crypto/Makefile.objs   |   1 +
 crypto/tlscredspsk.c   | 308 +
 crypto/tlssession.c|  56 +-
 crypto/trace-events|   3 +
 include/crypto/tlscredspsk.h   | 106 
 qemu-doc.texi  |  37 
 qemu-options.hx|  24 +++
 tests/Makefile.include |   4 +-
 tests/crypto-tls-psk-helpers.c |  50 ++
 tests/crypto-tls-psk-helpers.h |  29 
 tests/test-crypto-tlssession.c | 179 ---
 11 files changed, 774 insertions(+), 23 deletions(-)
 create mode 100644 crypto/tlscredspsk.c
 create mode 100644 include/crypto/tlscredspsk.h
 create mode 100644 tests/crypto-tls-psk-helpers.c
 create mode 100644 tests/crypto-tls-psk-helpers.h

diff --git a/crypto/Makefile.objs b/crypto/Makefile.objs
index 2b99e08062..756bab111b 100644
--- a/crypto/Makefile.objs
+++ b/crypto/Makefile.objs
@@ -15,6 +15,7 @@ crypto-obj-$(CONFIG_AF_ALG) += cipher-afalg.o
 crypto-obj-$(CONFIG_AF_ALG) += hash-afalg.o
 crypto-obj-y += tlscreds.o
 crypto-obj-y += tlscredsanon.o
+crypto-obj-y += tlscredspsk.o
 crypto-obj-y += tlscredsx509.o
 crypto-obj-y += tlssession.o
 crypto-obj-y += secret.o
diff --git a/crypto/tlscredspsk.c b/crypto/tlscredspsk.c
new file mode 100644
index 00..7be7c8efdd
--- /dev/null
+++ b/crypto/tlscredspsk.c
@@ -0,0 +1,308 @@
+/*
+ * QEMU crypto TLS Pre-Shared Keys (PSK) support
+ *
+ * Copyright (c) 2018 Red Hat, Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see .
+ *
+ */
+
+#include "qemu/osdep.h"
+#include "crypto/tlscredspsk.h"
+#include "tlscredspriv.h"
+#include "qapi/error.h"
+#include "qom/object_interfaces.h"
+#include "trace.h"
+
+
+#ifdef CONFIG_GNUTLS
+
+static int
+lookup_key(const char *pskfile, const char *username, gnutls_datum_t *key,
+   Error **errp)
+{
+const size_t ulen = strlen(username);
+GError *gerr = NULL;
+char *content = NULL;
+char **lines = NULL;
+size_t clen = 0, i;
+int ret = -1;
+
+if (!g_file_get_contents(pskfile, &content, &clen, &gerr)) {
+error_setg(errp, "Cannot read PSK file %s: %s",
+   pskfile, gerr->message);
+g_error_free(gerr);
+return -1;
+}
+
+lines = g_strsplit(content, "\n", -1);
+for (i = 0; lines[i] != NULL; ++i) {
+if (strncmp(lines[i], username, ulen) == 0 && lines[i][ulen] == ':') {
+key->data = (unsigned char *) g_strdup(&lines[i][ulen + 1]);
+key->size = strlen(lines[i]) - ulen - 1;
+ret = 0;
+goto out;
+}
+}
+error_setg(errp, "Username %s not found in PSK file %s",
+   username, pskfile);
+
+ out:
+free(content);
+g_strfreev(lines);
+return ret;
+}
+
+static int
+qcrypto_tls_creds_psk_load(QCryptoTLSCredsPSK *creds,
+   Error **errp)
+{
+char *pskfile = NULL, *dhparams = NULL;
+const char *username;
+int ret;
+int rv = -1;
+gnutls_datum_t key = { .data = NULL };
+
+trace_qcrypto_tls_creds_psk_load(creds,
+creds->parent_obj.dir ? creds->parent_obj.dir : "");
+
+if (creds->parent_obj.endpoint == QCRYPTO_TLS_CREDS_ENDPOINT_SERVER) {
+

Re: [Qemu-devel] [PATCH v6 09/15] qapi-introspect: add preprocessor conditions to generated QLit

[Markus Armbruster]

Marc-André Lureau  writes:

> The generator will take (obj, condition) tuples to wrap generated QLit
> objects for 'obj' with #if/#endif conditions.
>
> This commit adds 'ifcond' condition to top-level QLit objects.
>
> See generated tests/test-qmp-introspect.c. Example diff after this patch:
>
> --- before2018-01-08 11:55:24.757083654 +0100
> +++ tests/test-qmp-introspect.c   2018-01-08 13:08:44.477641629 +0100
> @@ -51,6 +51,8 @@
>  { "name", QLIT_QSTR("EVENT_F"), },
>  {}
>  })),
> +#if defined(TEST_IF_CMD)
> +#if defined(TEST_IF_STRUCT)
>  QLIT_QDICT(((QLitDictEntry[]) {
>  { "arg-type", QLIT_QSTR("5"), },
>  { "meta-type", QLIT_QSTR("command"), },
> @@ -58,12 +60,16 @@
>  { "ret-type", QLIT_QSTR("0"), },
>  {}
>  })),
> +#endif /* defined(TEST_IF_STRUCT) */
> +#endif /* defined(TEST_IF_CMD) */
>
> Signed-off-by: Marc-André Lureau 
> ---
>  scripts/qapi/introspect.py | 31 +--
>  1 file changed, 21 insertions(+), 10 deletions(-)
>
> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
> index bd7e1219be..71d4a779ce 100644
> --- a/scripts/qapi/introspect.py
> +++ b/scripts/qapi/introspect.py
> @@ -18,6 +18,15 @@ def to_qlit(obj, level=0, suppress_first_indent=False):
>  def indent(level):
>  return level * 4 * ' '
>  
> +if isinstance(obj, tuple):
> +ifobj, ifcond = obj
> +ret = gen_if(ifcond)
> +ret += to_qlit(ifobj, level)
> +endif = gen_endif(ifcond)
> +if endif:
> +ret += '\n' + endif
> +return ret
> +
>  ret = ''
>  if not suppress_first_indent:
>  ret += indent(level)

@obj represents a JSON object.  It consists of dictionaries, lists,
strings, booleans and None.  Numbers aren't implemented.  Your patch
splices in tuples to represent conditionals at any level.

So far, tuples occur only as elements of the outermost list (see
._gen_qlit() below), but to_qlit() supports them anywhere.  I figure
that will be needed later.  Can you point me to such a later use?

I'm asking because if there is one, the whole thing is a bit of a hack,
but at least it's a simple hack.  If not, then I'd like to look for a
solution that feels less hackish, possibly in a follow-up patch.

> @@ -26,7 +35,7 @@ def to_qlit(obj, level=0, suppress_first_indent=False):
>  elif isinstance(obj, str):
>  ret += 'QLIT_QSTR(' + to_c_string(obj) + ')'
>  elif isinstance(obj, list):
> -elts = [to_qlit(elt, level + 1)
> +elts = [to_qlit(elt, level + 1).strip('\n')
>  for elt in obj]
>  elts.append(indent(level + 1) + "{}")
>  ret += 'QLIT_QLIST(((QLitObject[]) {\n'

The .strip('\n') looks odd.  Reminds me of the asymmetry I noted in
PATCH 08: you take care to adjust vertical space around #if there, but
not around #endif.  Let's not worry about it now.

> @@ -128,12 +137,12 @@ const QLitObject %(c_name)s = %(c_string)s;
>  return '[' + self._use_type(typ.element_type) + ']'
>  return self._name(typ.name)
>  
> -def _gen_qlit(self, name, mtype, obj):
> +def _gen_qlit(self, name, mtype, obj, ifcond):
>  if mtype not in ('command', 'event', 'builtin', 'array'):
>  name = self._name(name)
>  obj['name'] = name
>  obj['meta-type'] = mtype
> -self._qlits.append(obj)
> +self._qlits.append((obj, ifcond))

This is where we produce ._qlits.  It's also the only place where we
create tuples.  Thus, all elements of ._qlits are tuples, and no tuples
occur at deeper levels.

>  
>  def _gen_member(self, member):
>  ret = {'name': member.name, 'type': self._use_type(member.type)}
> @@ -149,26 +158,27 @@ const QLitObject %(c_name)s = %(c_string)s;
>  return {'case': variant.name, 'type': self._use_type(variant.type)}
>  
>  def visit_builtin_type(self, name, info, json_type):
> -self._gen_qlit(name, 'builtin', {'json-type': json_type})
> +self._gen_qlit(name, 'builtin', {'json-type': json_type}, [])
>  
>  def visit_enum_type(self, name, info, ifcond, values, prefix):
> -self._gen_qlit(name, 'enum', {'values': values})
> +self._gen_qlit(name, 'enum', {'values': values}, ifcond)
>  
>  def visit_array_type(self, name, info, ifcond, element_type):
>  element = self._use_type(element_type)
> -self._gen_qlit('[' + element + ']', 'array', {'element-type': 
> element})
> +self._gen_qlit('[' + element + ']', 'array', {'element-type': 
> element},
> +   ifcond)
>  
>  def visit_object_type_flat(self, name, info, ifcond, members, variants):
>  obj = {'members': [self._gen_member(m) for m in members]}
>  if variants:
>  obj.update(self._gen_variants(variants.tag_member.name,
>v

[Qemu-devel] [PULL 1/3] audio/hda: adjust larger gaps faster

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann 
Message-id: 20180702145513.11481-1-kra...@redhat.com
---
 hw/audio/hda-codec.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/audio/hda-codec.c b/hw/audio/hda-codec.c
index 31c66d4255..9f630fa37f 100644
--- a/hw/audio/hda-codec.c
+++ b/hw/audio/hda-codec.c
@@ -203,6 +203,9 @@ static inline void hda_timer_sync_adjust(HDAAudioStream 
*st, int64_t target_pos)
 if (target_pos < -limit) {
 corr = -HDA_TIMER_TICKS;
 }
+if (target_pos < -(2 * limit)) {
+corr = -(4 * HDA_TIMER_TICKS);
+}
 if (corr == 0) {
 return;
 }
-- 
2.9.3

[Qemu-devel] [PULL 2/3] audio/hda: fix CID 1393631

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann 
Reviewed-by: Philippe Mathieu-Daudé 
Message-id: 20180702145513.11481-2-kra...@redhat.com
---
 hw/audio/hda-codec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/audio/hda-codec.c b/hw/audio/hda-codec.c
index 9f630fa37f..2b58c3505b 100644
--- a/hw/audio/hda-codec.c
+++ b/hw/audio/hda-codec.c
@@ -189,7 +189,7 @@ struct HDAAudioState {
 
 static inline int64_t hda_bytes_per_second(HDAAudioStream *st)
 {
-return 2 * st->as.nchannels * st->as.freq;
+return 2LL * st->as.nchannels * st->as.freq;
 }
 
 static inline void hda_timer_sync_adjust(HDAAudioStream *st, int64_t 
target_pos)
-- 
2.9.3

[Qemu-devel] [PULL 3/3] audio: add audio timer trace points

[Gerd Hoffmann]

Track audio timer starts and stops.  Also trace delayed audio timer calls.

Signed-off-by: Gerd Hoffmann 
Reviewed-by: Philippe Mathieu-Daudé 
Tested-by: Philippe Mathieu-Daudé 
Message-id: 20180702161524.17268-1-kra...@redhat.com
---
 audio/audio.c  | 28 +---
 audio/trace-events |  5 +
 2 files changed, 30 insertions(+), 3 deletions(-)

diff --git a/audio/audio.c b/audio/audio.c
index d6e91901aa..1ace47f510 100644
--- a/audio/audio.c
+++ b/audio/audio.c
@@ -29,6 +29,7 @@
 #include "sysemu/sysemu.h"
 #include "qemu/cutils.h"
 #include "sysemu/replay.h"
+#include "trace.h"
 
 #define AUDIO_CAP "audio"
 #include "audio_int.h"
@@ -1129,6 +1130,10 @@ static void audio_pcm_print_info (const char *cap, 
struct audio_pcm_info *info)
 /*
  * Timer
  */
+
+static bool audio_timer_running;
+static uint64_t audio_timer_last;
+
 static int audio_is_timer_needed (void)
 {
 HWVoiceIn *hwi = NULL;
@@ -1148,14 +1153,31 @@ static void audio_reset_timer (AudioState *s)
 if (audio_is_timer_needed ()) {
 timer_mod_anticipate_ns(s->ts,
 qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + conf.period.ticks);
-}
-else {
-timer_del (s->ts);
+if (!audio_timer_running) {
+audio_timer_running = true;
+audio_timer_last = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
+trace_audio_timer_start(conf.period.ticks / SCALE_MS);
+}
+} else {
+timer_del(s->ts);
+if (audio_timer_running) {
+audio_timer_running = false;
+trace_audio_timer_stop();
+}
 }
 }
 
 static void audio_timer (void *opaque)
 {
+int64_t now, diff;
+
+now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
+diff = now - audio_timer_last;
+if (diff > conf.period.ticks * 3 / 2) {
+trace_audio_timer_delayed(diff / SCALE_MS);
+}
+audio_timer_last = now;
+
 audio_run ("timer");
 audio_reset_timer (opaque);
 }
diff --git a/audio/trace-events b/audio/trace-events
index d37639e611..c986469319 100644
--- a/audio/trace-events
+++ b/audio/trace-events
@@ -15,3 +15,8 @@ alsa_no_frames(int state) "No frames available and ALSA state 
is %d"
 # audio/ossaudio.c
 oss_version(int version) "OSS version = 0x%x"
 oss_invalid_available_size(int size, int bufsize) "Invalid available size, 
size=%d bufsize=%d"
+
+# audio/audio.c
+audio_timer_start(int interval) "interval %d ms"
+audio_timer_stop(void) ""
+audio_timer_delayed(int interval) "interval %d ms"
-- 
2.9.3

[Qemu-devel] [PULL 0/3] Audio 20180703 patches

[Gerd Hoffmann]

The following changes since commit ab08440a4ee09032d1a9cb22fdcab23bc7e1c656:

  Merge remote-tracking branch 'remotes/rth/tags/pull-tcg-20180702' into 
staging (2018-07-02 17:57:46 +0100)

are available in the git repository at:

  git://git.kraxel.org/qemu tags/audio-20180703-pull-request

for you to fetch changes up to 1a961e785f59eba1fa8e06d5b1ebc84927b4b3a3:

  audio: add audio timer trace points (2018-07-03 11:46:47 +0200)


audio: hda fixes, timer tracing



Gerd Hoffmann (3):
  audio/hda: adjust larger gaps faster
  audio/hda: fix CID 1393631
  audio: add audio timer trace points

 audio/audio.c| 28 +---
 hw/audio/hda-codec.c |  5 -
 audio/trace-events   |  5 +
 3 files changed, 34 insertions(+), 4 deletions(-)

-- 
2.9.3

Re: [Qemu-devel] [PATCH v2 2/5] i386: Add CPUID bit and feature words for IA32_ARCH_CAPABILITIES MSR

[Eduardo Habkost]

On Tue, Jul 03, 2018 at 03:35:13PM +0800, Robert Hoo wrote:
> On Thu, 2018-06-28 at 15:28 -0300, Eduardo Habkost wrote:
> > On Wed, Jun 27, 2018 at 07:27:21PM +0800, Robert Hoo wrote:
> > > Support of IA32_PRED_CMD MSR already be enumerated by same CPUID bit as
> > > SPEC_CTRL.
> > > 
> > > Signed-off-by: Robert Hoo 
> > 
> > Based on kernel commit 1eaafe91, it looks like we must always set
> > IA32_ARCH_CAPABILITIES.RSBA[bit 2] unless we're really sure the
> > VM will not be migrated to a vulnerable processor.
> > 
> > Considering this, I'd like to make "+arch-capabilities" set
> > IA32_ARCH_CAPABILITIES.RSBA by default, unless RSBA is explicitly
> > disabled by management software.
> > 
> Agree. But this seems beyond Icelake CPU model scope. How about I think
> about this carefully and compose another patch (set) for this?

This plan makes sense to me, as I don't want to make this
decision block IceLake from being in QEMU 3.0.

However, enabling CPUID_7_0_EDX_ARCH_CAPABILITIES in IceLake but
setting the MSR to 0 seems pointless.

I think we should add IceLake without
CPUID_7_0_EDX_ARCH_CAPABILITIES first, and later (after deciding
on a reasonable default value for MSR_IA32_ARCH_CAPABILITIES),
enable the CPUID bit on IceLake (hopefully in time for QEMU 3.0).


> And you'd like to set  IA32_ARCH_CAPABILITIES.RSBA by default in qemu or
> kvm layer?

Probably we need to make this decision in QEMU.  If KVM set RSBA
automatically on .get_msr_feature(), QEMU won't be able to
differentiate a host with RSBA set from a host with RSBA unset.

-- 
Eduardo

[Qemu-devel] [PATCH] pulseaudio: process audio data in smaller chunks

[Gerd Hoffmann]

The rate of pulseaudio absorbing the audio stream is used to control the
the rate of the guests audio stream.  When the emulated hardware uses
small chunks (like intel-hda does) we need small chunks on the audio
backend side too, otherwise that feedback loop doesn't work very well.

Cc: Max Ehrlich 
Cc: Martin Schrodt 
Signed-off-by: Gerd Hoffmann 
---
 audio/paaudio.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/audio/paaudio.c b/audio/paaudio.c
index 949769774d..4c100bc318 100644
--- a/audio/paaudio.c
+++ b/audio/paaudio.c
@@ -227,7 +227,7 @@ static void *qpa_thread_out (void *arg)
 }
 }
 
-decr = to_mix = audio_MIN (pa->live, pa->g->conf.samples >> 2);
+decr = to_mix = audio_MIN(pa->live, pa->g->conf.samples >> 5);
 rpos = pa->rpos;
 
 if (audio_pt_unlock(&pa->pt, __func__)) {
@@ -319,7 +319,7 @@ static void *qpa_thread_in (void *arg)
 }
 }
 
-incr = to_grab = audio_MIN (pa->dead, pa->g->conf.samples >> 2);
+incr = to_grab = audio_MIN(pa->dead, pa->g->conf.samples >> 5);
 wpos = pa->wpos;
 
 if (audio_pt_unlock(&pa->pt, __func__)) {
-- 
2.9.3

[Qemu-devel] [Bug 1776478] Re: Getting qemu: uncaught target signal 6 when running lv2 plugin cross-compilation

[guysoft]

Ok, got it to run with (had to septate arguments with --):

sudo chroot . usr/bin/qemu-arm-static -g -- DISTRHO-
Ports/libs/lv2_ttl_generator DISTRHO-Ports/bin/lv2/TAL-Filter-2.lv2/TAL-
Filter-2.so

When running I can see two processes:

root 31993  0.0  0.0  86556  4696 pts/9S+   15:06   0:00 sudo chroot . 
usr/bin/qemu-arm-static -g -- DISTRHO-Ports/libs/lv2_ttl_generator 
DISTRHO-Ports/bin/lv2/TAL-Filter-2.lv2/TAL-Filter-2.so
root 31994  0.0  0.0 4240444 9592 pts/9Rl+  15:06   0:00 
usr/bin/qemu-arm-static -g -- DISTRHO-Ports/libs/lv2_ttl_generator 
DISTRHO-Ports/bin/lv2/TAL-Filter-2.lv2/TAL-Filter-2.so


I can attach to the pid using something like this:

sudo gdb -p $(ps aux | grep usr/bin/qemu-arm-static | grep -v grep |
grep -v chroot | awk '{  print $2 }')


When I continue I get:
Attaching to process 32519
[New LWP 32520]
0x607a0973 in ?? ()
(gdb) c
Continuing.

Thread 1 "qemu-arm-static" received signal SIGABRT, Aborted.
0x601786cd in ?? ()


The other process I can get with removing "-v" chroot so:

...

sudo gdb -p $(ps aux | grep usr/bin/qemu-arm-static | grep -v grep |
grep chroot | awk '{  print $2 }')

Reading symbols from /lib/x86_64-linux-gnu/libsystemd.so.0...(no debugging 
symbols found)...done.
Reading symbols from /lib/x86_64-linux-gnu/liblzma.so.5...(no debugging symbols 
found)...done.
Reading symbols from /usr/lib/x86_64-linux-gnu/liblz4.so.1...(no debugging 
symbols found)...done.
Reading symbols from /lib/x86_64-linux-gnu/libgcrypt.so.20...(no debugging 
symbols found)...done.
Reading symbols from /lib/x86_64-linux-gnu/libgpg-error.so.0...(no debugging 
symbols found)...done.
0x7f65cfd51bc4 in __GI___poll (fds=0x560bc6fe3040, nfds=2, timeout=-1) at 
../sysdeps/unix/sysv/linux/poll.c:29
29  ../sysdeps/unix/sysv/linux/poll.c: No such file or directory.
(gdb) 


I think the first one is the interesting run, but being verbose.

Any tips on adding symbols?

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

Title:
  Getting qemu: uncaught target signal 6 when running  lv2 plugin cross-
  compilation

Status in QEMU:
  New

Bug description:
  Hey,
  I am part of the Zynthian team and we use qemu-arm-static to cross compile 
lv2 audio plugins.

  When running a compilation of DISTRHO-Ports we get:

  lv2_ttl_generator: pthread_mutex_lock.c:81: __pthread_mutex_lock: Assertion 
`mutex->__data.__owner == 0' failed.
  qemu: uncaught target signal 6 (Aborted) - core dumped
  ./scripts/generate-ttl.sh: line 27: 16524 Aborted $GEN ./$FILE
  Makefile:62: recipe for target 'gen_lv2' failed
  make[1]: *** [gen_lv2] Error 134
  make[1]: Leaving directory '/home/pi/zynthian-sw/plugins/DISTRHO-Ports'
  Makefile:104: recipe for target 'lv2' failed
  make: *** [lv2] Error 2

  
  lv2_ttl_generator source is here:
  https://github.com/DISTRHO/DISTRHO-Ports/tree/master/libs/lv2-ttl-generator

  The command that is ruining is
  lv2_ttl_generator ./TAL-Filter-2.so 

  And ./TAL-Filter-2.so source is here:
  https://github.com/DISTRHO/DISTRHO-Ports/tree/master/ports/tal-filter-2/source


  Is there a way to debug what is going on?
  This runs fine on a Raspberrypi which is armv7

  A workaround would also help.

  
  Bug in Zynthian:
  https://github.com/zynthian/zynthian-sys/issues/59
  Bug in DISTRHO-Ports:
  https://github.com/DISTRHO/DISTRHO-Ports/issues/29

  Using qemu-arm-static version from master from two days ago:
  qemu-arm version 2.12.50 (v2.12.0-1182-ga7a7309ca5-dirty), commit: 
a7a7309ca52c327c6603d60db90ae4feeae719f7

  Also saw this in qemu-arm version 2.12.0 (Debian 1:2.12+dfsg-3)

  Thanks,
  Guy

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

Re: [Qemu-devel] [RFC 5/6] hw/arm/virt: support kvm_type property

[Auger Eric]

Hi Drew,

On 07/03/2018 01:55 PM, Andrew Jones wrote:
> On Wed, Jun 20, 2018 at 03:07:32PM +0200, Eric Auger wrote:
>> The kvm-type property currently is used to pass
>> a user parameter to KVM_CREATE_VM. This matches
>> the way KVM/ARM expects to pass the max_vm_phys_shift
>> parameter.
>>
>> This patch adds the support or the kvm-type property in
>> machvirt and also implements the machine class kvm_type()
>> callback so that it either returns the kvm-type value
>> provided by the user or returns the max_vm_phys_shift
>> exposed by KVM.
>>
>> for instance, the usespace can use the following option to
>> instantiate a 42b IPA guest: -machine kvm-type=42
> 
> 'kvm-type' is a very generic name. It looks like you're creating a KVM
> VM of type 42 (which I assume is the ultimate KVM VM that answers the
> meaning to Life, the Universe, and Everything), but it's not obvious
> how it relates to physical address bits. Why not call this property
> something like 'min_vm_phys_shift'? Notice the 'min' in the name,
> because this is where the user is stating what the minimum number of
> physical address bits required for the VM is. IIUC, if KVM supports
> more, then it shouldn't be a problem.

Well I agree with you that using kvm-type=42 is not very nice.

On the other hand the current kernel API to pass the VM GPA address size
is though the KVM_CREATE_VM kvm_type argument.

in accel/kvm/kvm-all.c there is all the infrastructure to fetch the
generic machine kvm-type machine option and decode it into type, which
is passed to KVM_CREATE_VM.

"
kvm_type = qemu_opt_get(qemu_get_machine_opts(), "kvm-type");
if (mc->kvm_type) {
type = mc->kvm_type(ms, kvm_type);
} else if (kvm_type) {
ret = -EINVAL;
fprintf(stderr, "Invalid argument kvm-type=%s\n", kvm_type);
goto err;
}

do {
ret = kvm_ioctl(s, KVM_CREATE_VM, type);
} while (ret == -EINTR);
"

This infrastructure already is used in hw/ppc/spapr.c

Whould it be better if we would pass something like kvm-type=48bGPA?
Otherwise I can decode another virt machine option (min_vm_phys_shift)
in kvm_type callback.

Thanks

Eric


> 
> Thanks,
> drew
> 

Re: [Qemu-devel] [PATCH v6 07/15] qapi: add #if/#endif helpers

[Marc-André Lureau]

Hi

On Tue, Jul 3, 2018 at 1:53 PM, Markus Armbruster  wrote:
> Marc-André Lureau  writes:
>
>> Add helpers to wrap generated code with #if/#endif lines.
>>
>> Add QAPIGenCSnippet class to write C snippet code, make QAPIGenC
>> inherit from it, for full C files with copyright headers etc.
>
> It's called QAPIGenCCode now.  Can touch up when I apply, along with
> another instance below.
>
> Leaves the reader wondering *why* you need to splice QAPIGenCCode
> between QAPIGen and QAPIGenC.  Becomes clear only in PATCH 10.
> Providing the class now reduces code churn, but you should explain why.
> Perhaps:
>
>   A later patch wants to use QAPIGen for generating C snippets rather
>   than full C files with copyright headers etc.  Splice in class
>   QAPIGenCCode between QAPIGen and QAPIGenC.

sure, thanks

>
>> Add a 'with' statement context manager that will be used to wrap
>> generator visitor methods.  The manager will check if code was
>> generated before adding #if/#endif lines on QAPIGenCSnippet
>> objects. Used in the following patches.
>>
>> Signed-off-by: Marc-André Lureau 
>> ---
>>  scripts/qapi/common.py | 101 +++--
>>  1 file changed, 97 insertions(+), 4 deletions(-)
>>
>> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
>> index 1b56065a80..44eaf25850 100644
>> --- a/scripts/qapi/common.py
>> +++ b/scripts/qapi/common.py
>> @@ -12,6 +12,7 @@
>>  # See the COPYING file in the top-level directory.
>>
>>  from __future__ import print_function
>> +from contextlib import contextmanager
>>  import errno
>>  import os
>>  import re
>> @@ -1974,6 +1975,40 @@ def guardend(name):
>>   name=guardname(name))
>>
>>
>> +def gen_if(ifcond):
>> +ret = ''
>> +for ifc in ifcond:
>> +ret += mcgen('''
>> +#if %(cond)s
>> +''', cond=ifc)
>> +return ret
>> +
>> +
>> +def gen_endif(ifcond):
>> +ret = ''
>> +for ifc in reversed(ifcond):
>> +ret += mcgen('''
>> +#endif /* %(cond)s */
>> +''', cond=ifc)
>> +return ret
>> +
>> +
>> +def wrap_ifcond(ifcond, before, after):
>
> Looks like a helper method.  Name it _wrap_ifcond?

Not fond of functions with _. Iirc, it was suggested by a linter to
make it a top-level function. I don't mind if you change it on commit.

>
>> +if ifcond is None or before == after:
>> +return after
>
> The before == after part suppresses empty conditionals.  Suggest
>
>return after# suppress empty #if ... #endif
>
> The ifcond is None part is suppresses "non-conditionals".  How can this
> happen?  If it can't, let's drop this part.

because the function can be called without additional checks on ifcond
is None. I don't see what would be the benefit in moving this to the
caller responsibility.

>
> Another non-conditional is [].  See below.
>
>> +
>> +assert after.startswith(before)
>> +out = before
>> +added = after[len(before):]
>> +if added[0] == '\n':
>> +out += '\n'
>> +added = added[1:]
>
> The conditional adjusts vertical space around #if.  This might need
> tweaking, but let's not worry about that now.
>
>> +out += gen_if(ifcond)
>> +out += added
>> +out += gen_endif(ifcond)
>
> There's no similar adjustment for #endif.  Again, let's not worry about
> that now.
>
>> +return out
>> +
>> +
>
> Since gen_if([]) and gen_endif([]) return '', wrap_ifcond([], before,
> after) returns after.  Okay.
>
>>  def gen_enum_lookup(name, values, prefix=None):
>>  ret = mcgen('''
>>
>> @@ -2064,6 +2099,7 @@ class QAPIGen(object):
>>  def __init__(self):
>>  self._preamble = ''
>>  self._body = ''
>> +self._start_if = None
>>
>>  def preamble_add(self, text):
>>  self._preamble += text
>> @@ -2071,6 +2107,25 @@ class QAPIGen(object):
>>  def add(self, text):
>>  self._body += text
>>
>> +def start_if(self, ifcond):
>> +assert self._start_if is None
>> +
>
> Let's drop this blank line.

I prefer to have preconditions separated from the code, but ok

>
>> +self._start_if = (ifcond, self._body, self._preamble)
>
> It's now obvious that you can't nest .start_if() ... .endif().  Good.
>
>> +
>> +def _wrap_ifcond(self):
>> +pass
>> +
>> +def end_if(self):
>> +assert self._start_if
>> +
>
> Let's drop this blank line.
>
>> +self._wrap_ifcond()
>> +self._start_if = None
>> +
>> +def get_content(self, fname=None):
>> +assert self._start_if is None
>> +return (self._top(fname) + self._preamble + self._body
>> ++ self._bottom(fname))
>> +
>>  def _top(self, fname):
>>  return ''
>>
>
> Note for later: all this code has no effect unless ._wrap_ifcond() is
> overridden.
>
>> @@ -2091,8 +2146,7 @@ class QAPIGen(object):
>>  f = open(fd, 'r+', encoding='utf-8')
>>  else:
>>  f = os.fdopen(fd, 'r+')
>> -text = (self._top(fname) + self._pream

Re: [Qemu-devel] [libvirt] [PULL 25/26] block: Remove deprecated -drive option serial

[Christian Borntraeger]

On 07/03/2018 01:35 PM, Peter Maydell wrote:
> On 3 July 2018 at 12:32, Kevin Wolf  wrote:
>> Am 03.07.2018 um 13:22 hat Daniel P. Berrangé geschrieben:
>>> Just posted latest version here:
>>>
>>>   https://www.redhat.com/archives/libvir-list/2018-July/msg00130.html
>>>
>>> It will be in the next release on ~ Aug 1st
>>
>> It would have been a lot nicer to have it the July release because this
>> means that we'll have the released libvirt broken during almost the
>> whole rc phase of QEMU 3.0, but the release is planned for Aug 8th the
>> earliest, so I guess we're still okay. People using QEMU from git will
>> just need libvirt from git as well.
> 
> I'm still not clear what we gain from having a QEMU that's dropped
> a feature that is still used by everything except leading-edge
> not-yet-released versions of libvirt. Is there a strong reason we
> can't just revert the deletion of the deprecated feature for a QEMU
> release or two?

+1

Re: [Qemu-devel] [PATCH 2/2] qapi: Drop qapi-gen --unmask option

[Eric Blake]

On 07/03/2018 12:51 AM, Markus Armbruster wrote:

Eric Blake  writes:


On 07/02/2018 01:48 PM, Markus Armbruster wrote:

Eric Blake  writes:


Now that we have useful access to the type name as a comment
in the generated qapi-introspect.c, we don't need to regenerate
code with a temporary -u option just to get at type names.




 # three lines: greeting, output of qmp_capabilities, query-qmp-schema
 return json.loads(out.split('\n')[2])['return']

This returns an abstract syntax tree, represented in Python the obvious
way.  I can then explore it in Python, say search for object types with
certain properties.  For example, commit 2860b2b2cb8:

 Thus, the flaw puts an artificial restriction on the QAPI schema: we
 can't have potentially empty objects and arrays within
 BlockdevOptions, except when they're optional and "empty" has the same
 meaning as "absent".

--> Our QAPI schema satisfies this restriction (I checked), but it's a
 trap for the unwary, and a temptation to employ awkward workarounds
 for the wary.  Let's get rid of it.

I checked with a Python script that read the schema as shown above.
Without -u, I'd have to revert the identifier hiding.  I could certainly
write some more Python to read the mapping from the generated C, but
that feels like busy work.


Good argument. Okay, I'm dropping this patch, and tweaking the other 
patch commit message to explain that -u is still useful, even with the 
addition of comment aids.


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

Re: [Qemu-devel] [PATCH] exec.c: check RAMBlock validity before changing its flag

[Cédric Le Goater]

On 07/02/2018 05:57 AM, Peter Xu wrote:
> On Sun, Jul 01, 2018 at 07:19:53PM +0200, Cédric Le Goater wrote:
>> When a PCI device is unplugged, the PCI memory regions are deleted
>> before the optional ROM RAMBlock is flagged non-migratable. But, when
>> this is done, the RAMBlock has already been cleared from the region,
>> leading to a segv.
>>
>> Fix the issue by testing the RAMBlock before flagging it, as it is
>> done in qemu_ram_unset_idstr()
>>
>> Signed-off-by: Cédric Le Goater 
>> ---
>>
>>  I caught this bug while deleting a passthrough device from a pseries
>>  machine. Here is the stack:
>>
>>#0  qemu_ram_unset_migratable (rb=0x0) at 
>> /home/legoater/work/qemu/qemu-xive-3.0.git/exec.c:1994
>>#1  0x00010072def0 in vmstate_unregister_ram (mr=0x101796af0, 
>> dev=)
>>#2  0x000100694e5c in pci_del_option_rom (pdev=0x101796330)
>>#3  pci_qdev_unrealize (dev=, errp=)
>>#4  0x0001005ff910 in device_set_realized (obj=0x101796330, 
>> value=, errp=0x0)
>>#5  0x0001007a487c in property_set_bool (obj=0x101796330, 
>> v=, name=, 
>>#6  0x0001007a7878 in object_property_set (obj=0x101796330, 
>> v=0x7fff70033110, 
>>#7  0x0001007aaf1c in object_property_set_qobject (obj=0x101796330, 
>> value=, 
>>#8  0x0001007a7b90 in object_property_set_bool (obj=0x101796330, 
>> value=, 
>>#9  0x0001005fcdd8 in device_unparent (obj=0x101796330)
>>#10 0x0001007a6dd0 in object_finalize_child_property (obj=> out>, name=, 
>>#11 0x0001007a50c0 in object_property_del_child (obj=0x10111f800, 
>> child=0x101796330, 
>>#12 0x000100425cc0 in spapr_phb_remove_pci_device_cb (dev=0x101796330)
>>#13 0x000100427974 in spapr_drc_release (drc=0x1017e2df0)
>>#14 0x000100429098 in spapr_drc_detach (drc=0x1017e2df0)
>>#15 0x0001004294e0 in drc_isolate_physical (drc=0x1017e2df0)
>>#16 0x00010042a50c in rtas_set_isolation_state (state=0, 
>> idx=)
>>
>>  May be we should call pci_del_option_rom() before
>>  pci_unregister_io_regions() ?
> 
> This seems to make more sense to me.
> 
> Meanwhile I assume the name pci_del_option_rom() is a bit misleading -
> it's not really deleting the ROM but unregistering the ROM only.
> Instead IIUC it's pci_unregister_io_regions() which deleted that. So
> maybe we can either rename the function pci_del_option_rom(), or we
> can pick the ROM destruction out of pci_unregister_io_regions() and
> put it into pci_del_option_rom() to make sure it's done as the last
> step?

So it is a little more complex than I thought. 

The PCI device is a vfio PCI device and the PCI ROM region is initialized 
in vfio_pci_size_rom() with memory_region_init_io(), which does not 
allocate the RAMBlock, but has_rom is still set to true. 

When the device is deleted, pci_del_option_rom() is called and with it, 
vmstate_unregister_ram() because has_rom is set to true. Leading to the
SEGV.

I am not sure how to handle this case. It seems that the realize routine 
of VFIOPCIDevice is hijacking a little the PCIDevice layer.


C.

Re: [Qemu-devel] [RFC 5/6] hw/arm/virt: support kvm_type property

[Andrew Jones]

On Tue, Jul 03, 2018 at 02:31:02PM +0200, Auger Eric wrote:
> Hi Drew,
> 
> On 07/03/2018 01:55 PM, Andrew Jones wrote:
> > On Wed, Jun 20, 2018 at 03:07:32PM +0200, Eric Auger wrote:
> >> The kvm-type property currently is used to pass
> >> a user parameter to KVM_CREATE_VM. This matches
> >> the way KVM/ARM expects to pass the max_vm_phys_shift
> >> parameter.
> >>
> >> This patch adds the support or the kvm-type property in
> >> machvirt and also implements the machine class kvm_type()
> >> callback so that it either returns the kvm-type value
> >> provided by the user or returns the max_vm_phys_shift
> >> exposed by KVM.
> >>
> >> for instance, the usespace can use the following option to
> >> instantiate a 42b IPA guest: -machine kvm-type=42
> > 
> > 'kvm-type' is a very generic name. It looks like you're creating a KVM
> > VM of type 42 (which I assume is the ultimate KVM VM that answers the
> > meaning to Life, the Universe, and Everything), but it's not obvious
> > how it relates to physical address bits. Why not call this property
> > something like 'min_vm_phys_shift'? Notice the 'min' in the name,
> > because this is where the user is stating what the minimum number of
> > physical address bits required for the VM is. IIUC, if KVM supports
> > more, then it shouldn't be a problem.
> 
> Well I agree with you that using kvm-type=42 is not very nice.
> 
> On the other hand the current kernel API to pass the VM GPA address size
> is though the KVM_CREATE_VM kvm_type argument.
> 
> in accel/kvm/kvm-all.c there is all the infrastructure to fetch the
> generic machine kvm-type machine option and decode it into type, which
> is passed to KVM_CREATE_VM.
> 
> "
> kvm_type = qemu_opt_get(qemu_get_machine_opts(), "kvm-type");
> if (mc->kvm_type) {
> type = mc->kvm_type(ms, kvm_type);
> } else if (kvm_type) {
> ret = -EINVAL;
> fprintf(stderr, "Invalid argument kvm-type=%s\n", kvm_type);
> goto err;
> }
> 
> do {
> ret = kvm_ioctl(s, KVM_CREATE_VM, type);
> } while (ret == -EINTR);
> "
> 
> This infrastructure already is used in hw/ppc/spapr.c
> 
> Whould it be better if we would pass something like kvm-type=48bGPA?
> Otherwise I can decode another virt machine option (min_vm_phys_shift)
> in kvm_type callback.

Yes, this is what I'm thinking. I don't believe we have to expose the
details of the KVM API to the user through the QEMU command line. The
details are actually more complicated anyway, as the phys-shift is
only the lower 8-bits of KVM type[*], not the whole value.

Thanks,
drew

[*] Looks like Suzuki's series is missing the Documentation/virtual/kvm/api.txt
update needed to specify that.

> 
> Thanks
> 
> Eric
> 
> 
> > 
> > Thanks,
> > drew
> > 
>